Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Checking IDs against containers should be explicitly optional #971

Closed
squaremo opened this issue Jun 21, 2015 · 14 comments
Closed

Checking IDs against containers should be explicitly optional #971

squaremo opened this issue Jun 21, 2015 · 14 comments

Comments

@squaremo
Copy link
Contributor

Not assigning an IP because a container is not running breaks the Docker plugin, because it cannot supply a container ID, only an endpoint ID.

@rade
Copy link
Member

rade commented Jun 21, 2015

Simply feed ipam an id that syntactically isn't a full container id. As we do with weave:expose.

@squaremo
Copy link
Contributor Author

Yes, I came to that conclusion. That seems pretty grubby.

@squaremo
Copy link
Contributor Author

(not to mention undocumented?)

@rade
Copy link
Member

rade commented Jun 21, 2015

None of the IPAM APIs are documented.

@squaremo
Copy link
Contributor Author

How does that even work? If not by coincidence.

@rade
Copy link
Member

rade commented Jun 21, 2015

where's the coincidence?

@squaremo
Copy link
Contributor Author

OK I take it back. Just a grubby hack, not a coincidence.

@rade
Copy link
Member

rade commented Jun 21, 2015

Suggestions for non-grubby non-hacks welcome. Feel free to repurpose this issue for that; otherwise close it.

@squaremo squaremo changed the title Checking that containers are running in IPAM breaks the plugin Checking IDs against containers should be explicitly optional Jun 21, 2015
@squaremo
Copy link
Contributor Author

Repurposing: Instead of assuming anything that looks like a container ID (e.g., a libnetwork endpoint ID) is in fact a container ID, and refusing to issue an IP for it, IPAM should be told when it should do this and when not. The option could be supplied, for example, as a form field in the POST (or a querystring param, or either).

For the purposes of being a general API, it would make more sense if the default was not to check; but it is more convenient for the weave script (and not a terrible burden elsewise) if it defaults the other way.

@rade
Copy link
Member

rade commented Jul 3, 2015

NB: this issue also applies to DNS.

@tomwilkie
Copy link
Contributor

For DNS, can I assume all non-empty containerids are intended to be
containerids? Or is the intention to pass endpoint ids to DNS also?

On Friday, 3 July 2015, Matthias Radestock [email protected] wrote:

NB: this issue also applies to DNS.


Reply to this email directly or view it on GitHub
#971 (comment).

@squaremo
Copy link
Contributor Author

squaremo commented Jul 3, 2015

Or is the intention to pass endpoint ids to DNS also?

At the minute the plugin has to watch docker for containers coming and going, so it uses container IDs. If service discovery is opened up as an extension point, it'll be an endpoint ID (or some other). I recommend the approach given above rather than trying to guess from the name, which was after all the original complaint.

@rade
Copy link
Member

rade commented Jul 3, 2015

For DNS, can I assume all non-empty containerids are intended to be containerids?

No, you cannot assume that. And the current DNS code doesn't.

@rade
Copy link
Member

rade commented Jul 3, 2015

I recommend the approach given above rather than trying to guess from the name

That is this issue. For #826, let us keep whatever API is on master.

@rade rade self-assigned this Jul 4, 2015
rade added a commit that referenced this issue Jul 4, 2015
...rather than conducting it implicitly when an ID "looks like" a
container ID.

Closes #971
@rade rade modified the milestones: current, 1.1.0 Jul 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants