-
Notifications
You must be signed in to change notification settings - Fork 18.9k
IPv6 only: add API option enable/disable IPv4 #48271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Rob Murray <rob.murray@docker.com>
corhere
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Small PRs for the win!
api/swagger.yaml
Outdated
| description: "Optional custom IP scheme for the network." | ||
| $ref: "#/definitions/IPAM" | ||
| EnableIPv4: | ||
| description: "Enable IPv4 on the network." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd great to leave a note about the experimentalness of this property, both to warn API users that its implementation / usage / semantics might evolve (although unlikely), and also to warn that it's not available if dockerd's experimental flag isn't set. For instance, we do something similar for CDISpecDirs.
| return nil, errdefs.InvalidParameter(fmt.Errorf("driver-opt %q is not a valid bool", netlabel.EnableIPv4)) | ||
| } | ||
| } | ||
| if !enableIPv4 && !daemon.config().Experimental && create.ConfigFrom == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to move away from 'experimental' features as it's coarse-grained, to replace it with fine-grained feature flags. Would it make sense to introduce a new feature flag for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR on its own is basically-useless (!). It'll carry on being basically-useless for a few more PRs yet.
I don't think we've decided to release/announce IPv6-only (EnableIPv4:false) as experimental, or behind a feature flag - but, we could do either if we decide to release with it still incomplete. Depending on what that looks like, we might want to feature-flag it for a specific network driver or something like that. Or, if it looks risky and likely to change, we might make the whole thing experimental / feature-flagged until we get some feedback. But, I don't think we'll know until we're at that point.
So, just locking it behind --experimental for now seems like the best approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I won't dwell on that.
akerouanton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Rob Murray <rob.murray@docker.com>
Similar to EnableIPv6:
- Set it if EnableIPv4 is specified in a create request.
- Otherwise, set it if included in `default-network-opts`.
- Apart from in a config-from network, so that it doesn't look
like the API request set the field.
- Include the new field in Network marshalling/unmarshalling test.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>
3d1d1eb to
1f542d5
Compare
|
I forgot to ignore
|
## Description Updates for moby 28.0 networking. ## Related issues or tickets Series of commits ... - Fix description of 'inhibit_ipv4' - Not changed in moby 28.0, updated to clarify difference from (new) IPv6-only networks. - Updates to default bridge address config - moby/moby#48319 - Describe IPv6-only network config - moby/moby#48271 - docker/cli#5599 - Update description of gateway modes - moby/moby#48594 - moby/moby#48596 - moby/moby#48597 - Describe gateway selection in the networking overview. - docker/cli#5664 - Describe gateway mode `isolated` - moby/moby#49262 ## Reviews <!-- Notes for reviewers here --> <!-- List applicable reviews (optionally @tag reviewers) --> - [ ] Technical review - [ ] Editorial review - [ ] Product review --------- Signed-off-by: Rob Murray <rob.murray@docker.com>
- What I did
--ipv4in the CLI, equivalent to--ipv6).com.docker.network.enable_ipv4.default-network-opts.EnableIPv4=false.EnableIPv4=true.\EnableIPv4in a network create request if the API version is less than 1.47.--experimentalto disable IPv4.Follow-up PRs will make the option do-something.
- How I did it
The first commit here, bumping the API version to 1.47, is likely to disappear - as this will be merged after another PR that does the same thing. But, for now, it means there's somewhere to put an API
version-history.mdupdate.The rest is fairly machanical copying of
EnableIPv6behaviour.- How to verify it
Can't disable IPv4 without
--experimental:With
--experimental, can disable IPv4:Default for a new bridge network is
true:Predefined host network shows
EnableIPv4:false, like the existingEnableIPv6:false:Predefined bridge network has
EnableIPv4:true:Marshalling/unmarshalling a
libnetwork.NetworkwithEnableIPv4:trueis covered in an updated unit test.- Description for the changelog