Skip to content

Conversation

@pradipd
Copy link
Contributor

@pradipd pradipd commented Aug 7, 2019

In windows HNS manages IPAM. If the user does not specify a subnet, HNS will choose one
for them. However, in order for the IPAM to show up in the output of "docker inspect",
we need to update the network IPAMv4Config field.

Signed-off-by: Pradip Dhara pradipd@microsoft.com

@pradipd
Copy link
Contributor Author

pradipd commented Aug 7, 2019

This is a fix for moby/moby#38358. However, this is a fix for only the original issue. I.e. The default docker NAT network is missing.

This root cause is that when the user does not specify a subnet, windows (HNS) will chose one for them. However, we do not show the subnet/gateway in the output of docker inspect. However, the subnet and gateway are present and will work. Unfortunately any scripts that parse the output of "docker inspect nat" will fail.

Another simpler way to repro this issue is:
On windows server 2019.
docker network create -d nat mynat
docker inspect mynat

The output of docker inspect mynat will show
"Config": [ { "Subnet": "0.0.0.0/0" } ]

@pradipd
Copy link
Contributor Author

pradipd commented Aug 7, 2019

Regarding the solution, I investigated @arkodg's suggesting of using docker's ipam and getting rid of the window's ipam plugin (https://github.com/docker/libnetwork/blob/master/ipams/windowsipam/windowsipam.go).
It may be possible to make that work (pradipd/moby@4a8e6fc) but it still requires some more investigation. And, I was told it may not work on server 2016. So, I went with this approach of updating the network object from the driver. I realize that this may not be ideal, however, I don't think we are the first to need the driver to update the network configuration:
https://github.com/pradipd/moby/blob/master/vendor/github.com/docker/libnetwork/controller.go#L836
We could also follow that model. But, I felt the callback was cleaner.

@pradipd
Copy link
Contributor Author

pradipd commented Aug 7, 2019

@arkodg @mavenugo PTAL.

@arkodg
Copy link
Contributor

arkodg commented Aug 13, 2019

@pradipd, would it be possible to add an API to HNS to GET a free subnet or even check if a subnet is free or not (Windows IPAM in libnetwork could pass some random value), then there would be a clear separation between the Driver and the Network

@pradipd
Copy link
Contributor Author

pradipd commented Aug 22, 2019

Sorry for the delayed response. I was OOF.

@pradipd
Copy link
Contributor Author

pradipd commented Aug 22, 2019

@arkodg: Adding an API would require us to backport the change to 2016. I don't think this issue is important enough to backport to 2016. There are workarounds that can unblock users.

@pradipd
Copy link
Contributor Author

pradipd commented Aug 22, 2019

Oops. Sorry. Did not mean to resolve conversation. Will try and reopen

@arkodg
Copy link
Contributor

arkodg commented Aug 22, 2019

@pradipd is it possible to use this CMD or the underlying API - https://docs.microsoft.com/en-us/powershell/module/ipamserver/find-ipamfreeaddress?view=win10-ps

@pradipd
Copy link
Contributor Author

pradipd commented Aug 22, 2019

No. That API is for a different windows server role/feature that is not involved with HNS. I.e. that API is for when you setup your Windows Server as a DHCP server.

@selansen
Copy link
Contributor

Could you pls rebase and submit again?
latest code base fixes the CI failure .

In windows HNS manages IPAM.  If the user does not specify a subnet, HNS will choose one
for them.  However, in order for the IPAM to show up in the output of "docker inspect",
we need to update the network IPAMv4Config field.

Signed-off-by: Pradip Dhara <pradipd@microsoft.com>
@daschott
Copy link

@pradipd @arkodg @selansen is there anything else needed here?

Copy link
Contributor

@selansen selansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@selansen
Copy link
Contributor

@arkodg PTAL .

return nil
}

func (n *network) UpdateIpamConfig(ipV4Data []driverapi.IPAMData) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this generic to ipv4 and ipv6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just undid that change based off feedback from @selansen . Windows wasn't using it, so, we decided we can add it later when we (or some other driver) needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean windows wasn't passing any IPv6 data in using UpdateIpamConfig()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still stick by my comment. if we are not using it, we dont need it. It will confuse everyone who is looking into the code after few months wondering why is this code exists if we are not using it.

@selansen
Copy link
Contributor

Thanks a lot @arkodg @pradipd

@selansen selansen merged commit 45c7102 into moby:master Sep 13, 2019
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Sep 20, 2019
full diff: moby/libnetwork@92d1fbe...96bcc0d

changes included:

- moby/libnetwork#2429 Updating IPAM config with results from HNS create network call
  - addresses moby#38358
- moby/libnetwork#2450 Always configure iptables forward policy
  - related to moby#14041 and moby/libnetwork#1526

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Sep 23, 2019
full diff: moby/libnetwork@92d1fbe...96bcc0d

changes included:

- moby/libnetwork#2429 Updating IPAM config with results from HNS create network call
  - addresses moby/moby#38358
- moby/libnetwork#2450 Always configure iptables forward policy
  - related to moby/moby#14041 and moby/libnetwork#1526

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 75477f0b3c77f2108a6b5586dbc246c52b479941
Component: engine
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Sep 24, 2019
full diff: moby/libnetwork@92d1fbe...96bcc0d

changes included:

- moby/libnetwork#2429 Updating IPAM config with results from HNS create network call
  - addresses moby#38358
- moby/libnetwork#2450 Always configure iptables forward policy
  - related to moby#14041 and moby/libnetwork#1526

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 75477f0)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Sep 25, 2019
full diff: moby/libnetwork@92d1fbe...96bcc0d

changes included:

- moby/libnetwork#2429 Updating IPAM config with results from HNS create network call
  - addresses moby/moby#38358
- moby/libnetwork#2450 Always configure iptables forward policy
  - related to moby/moby#14041 and moby/libnetwork#1526

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 75477f0b3c77f2108a6b5586dbc246c52b479941)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 559be42fc26048f4069de64f84202803a113413a
Component: engine
burnMyDread pushed a commit to burnMyDread/moby that referenced this pull request Oct 21, 2019
full diff: moby/libnetwork@92d1fbe...96bcc0d

changes included:

- moby/libnetwork#2429 Updating IPAM config with results from HNS create network call
  - addresses moby#38358
- moby/libnetwork#2450 Always configure iptables forward policy
  - related to moby#14041 and moby/libnetwork#1526

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: zach <Zachary.Joyner@linux.com>
@subbunori
Copy link

Hi, How to know if this change is release in latest docker enterprise version?

@arkodg
Copy link
Contributor

arkodg commented Oct 31, 2019

@subbunori can you please update to latest 19.03 version and this fix should be present
here are the commits in the 19.03 branch - https://github.com/docker/libnetwork/commits/bump_19.03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants