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

[proxy] add --rewrite-inspect flag #1376

Merged
merged 5 commits into from
Sep 4, 2015
Merged

[proxy] add --rewrite-inspect flag #1376

merged 5 commits into from
Sep 4, 2015

Conversation

paulbellamy
Copy link
Contributor

To return the weave ip/mac for docker inspect calls.

Fixes #117, and #1199 (possibly? Anything else outstanding for that?)

@errordeveloper
Copy link
Contributor

I will test this with Kubernetes once again now.

@errordeveloper
Copy link
Contributor

I confirm this is working perfectly well with Kubernetes.

@bboreham bboreham self-assigned this Sep 2, 2015
@@ -104,6 +104,12 @@ Multiple IP addresses and networks can be supplied in the `WEAVE_CIDR`
variable by space-separating them, as in
`WEAVE_CIDR="10.2.1.1/24 10.2.2.1/24"`.

The docker IP will still be returned by `docker inspect`. If you want

This comment was marked as abuse.

@bboreham bboreham assigned paulbellamy and unassigned bboreham Sep 2, 2015
@paulbellamy
Copy link
Contributor Author

Note, docker NetworkSettings includes:

"NetworkSettings": {
    "Bridge": "",
    "Gateway": "",
    "IPAddress": "",
    "IPPrefixLen": 0,
    "MacAddress": "",
    "PortMapping": null,
    "Ports": null
},

This only overwrites the IPAddress, IPPrefixLen, and MacAddress, but not Gateway.

@paulbellamy paulbellamy force-pushed the 1199-proxy-kubernetes branch from ef60bf8 to f5eb8f4 Compare September 2, 2015 13:09
@bboreham
Copy link
Contributor

bboreham commented Sep 2, 2015

Gateway seems to point to the docker bridge IP. I don't think we should leave that in place.

@paulbellamy paulbellamy assigned bboreham and unassigned paulbellamy Sep 2, 2015
@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

Changes look good; we are still leaving gateway in the NetworkSettings - do we want that?

@paulbellamy
Copy link
Contributor Author

Could replace gateway with the IP of the weave router... But I'm not sure how correct that is.

@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

This gateway refers to the route that the container has to the rest of the world, so given that we don't add such a route for ethwe I don't think we should claim a fake one.

I'm not so sure now that leaving it is wrong. That route and gateway still exists. It just isn't associated with the IP or MAC that we are now returning.

@paulbellamy
Copy link
Contributor Author

If we are returning an IP/MAC for an interface with no gateway, it probably makes most sense not to return a gateway.

`docker inspect` to return the weave NetworkSettings instead, then the
proxy must be launced with the `--rewrite-inspect` flag. This will
only substitute in the weave network settings when the container has a
weave IP.

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Sep 3, 2015

Is d7d8b86 also fixing #1371?

@bboreham bboreham assigned paulbellamy and unassigned bboreham Sep 3, 2015
@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

No, it's avoiding making #1371 worse for this specific data. Some of the code will be re-usable when we fix #1371 fully.

@rade
Copy link
Member

rade commented Sep 3, 2015

Hmm. It feels odd to do things completely differently here.

@bboreham
Copy link
Contributor

bboreham commented Sep 3, 2015

It seems much more likely that we will spoil someone's day by disrupting what is returned by docker inspect when Docker change it, than the return values from create, start, restart, etc. Borne out by the fact that only one person managed to trip over this in the real world so far.

@paulbellamy paulbellamy force-pushed the 1199-proxy-kubernetes branch 2 times, most recently from fdddf0d to 75fc999 Compare September 4, 2015 09:41
@rade
Copy link
Member

rade commented Sep 4, 2015

@paulbellamy anything left to do here?

@paulbellamy
Copy link
Contributor Author

I believe all of the comments have been addressed, so if the docs look good, then it should be ready to go.

@paulbellamy paulbellamy removed their assignment Sep 4, 2015
@rade rade force-pushed the 1199-proxy-kubernetes branch from 63703d3 to 633df0c Compare September 4, 2015 12:40
@bboreham
Copy link
Contributor

bboreham commented Sep 4, 2015

LGTM

rade added a commit that referenced this pull request Sep 4, 2015
[proxy] add --rewrite-inspect flag

Fixes #117. Fixes #1199.
@rade rade merged commit 32a82f2 into master Sep 4, 2015
@paulbellamy paulbellamy deleted the 1199-proxy-kubernetes branch September 4, 2015 13:33
@rade rade modified the milestone: 1.1.0 Sep 4, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants