-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
network create: make --ipv6 optional #5126
Conversation
- api: Make EnableIPv6 optional full diff: moby/moby@c6aaabc...00f18ef Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5126 +/- ##
==========================================
- Coverage 61.37% 61.34% -0.03%
==========================================
Files 298 295 -3
Lines 20717 20714 -3
==========================================
- Hits 12715 12707 -8
- Misses 7102 7104 +2
- Partials 900 903 +3 |
cli/command/network/create.go
Outdated
@@ -61,7 +69,7 @@ func newCreateCommand(dockerCli command.Cli) *cobra.Command { | |||
flags.VarP(&options.driverOpts, "opt", "o", "Set driver specific options") | |||
flags.Var(&options.labels, "label", "Set metadata on a network") | |||
flags.BoolVar(&options.internal, "internal", false, "Restrict external access to the network") | |||
flags.BoolVar(&options.ipv6, "ipv6", false, "Enable IPv6 networking") | |||
flags.BoolVar(options.ipv6, "ipv6", false, "Enable IPv6 networking") |
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.
LOL perhaps we need to create our own "optional bool" so that we could have nil
as default. 🤔
Perhaps the flag-description should be updated though; something like "auto" (or "determined by daemon"?);
flags.BoolVar(options.ipv6, "ipv6", false, "Enable IPv6 networking") | |
flags.BoolVar(options.ipv6, "ipv6", false, "Enable or disable IPv6 networking (default is auto)") |
or should we consider the default to be "true"?
flags.BoolVar(options.ipv6, "ipv6", false, "Enable IPv6 networking") | |
flags.BoolVar(options.ipv6, "ipv6", true, "Enable or disable IPv6 networking (default is enabled, if supported)") |
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.
or should we consider the default to be "true"?
I think we shouldn't do that, at least for now. One reason is the discussion we had regarding ip6tables. Another reason, IPv6 isn't supported by all drivers currently (eg. overlay).
If we want to enable IPv6 by default, I think it'd need to be done at the API level, in a new API version.
cli/command/network/create.go
Outdated
f := cmd.Flag("ipv6") | ||
if !f.Changed { |
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.
f := cmd.Flag("ipv6") | |
if !f.Changed { | |
if !cmd.Flag("ipv6").Changed { |
or (not sure which one is more performant, but probably won't matter much)
f := cmd.Flag("ipv6") | |
if !f.Changed { | |
if !cmd.Flags().Changed("ipv6") { |
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.
Actually; would the reverse work? I see you have the var ipv6 bool
added; that one could be set on the flag;
And this code could then do;
if cmd.Flag("ipv6").Changed {
options.ipv6 = &ipv6
}
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.
I chose cmd.Flag("ipv6").Changed
instead of cmd.Flags().Changed("ipv6")
as the former will trigger a panic if the flag is renamed / removed, whereas the latter will silently fail (ie. return false
).
f67415b
to
5ae1360
Compare
The API field `EnableIPv6` was marked as optional in our Swagger docs, and its default value in the Go client came from that field being a bool, thus defaulting to its zero value. That's not the case anymore. This field is now a `*bool` as to let daemon's config define the default value. IPv6 can still be enabled / disabled by explicitly specifying the `--ipv6` flag when doing `docker network create`. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
5ae1360
to
db2672e
Compare
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
The only thing I'm wondering is if we need to add more details about what the default is, but we can do in a follow-up. |
@thaJeztah Sorry for the 1st version of this patch, it was crap. I should stop multi-tasking sometimes 😓
I think we need to update the documentation to explain how to set the default daemon-wide, how to disable ipv6 for a specific network, etc... but that's pretty much it. Do you have anything else in mind? FWIW, we discussed docs updates a bit with @robmry and we decided to wait for at least the rc1 to start working on that -- we'd like to get every PR ready first. |
- What I did
The API field
EnableIPv6
was marked as optional in our Swagger docs, and its default value in the Go client came from that field being a bool, thus defaulting to its zero value. That's not the case anymore.This field is now a
*bool
as to let daemon's config define the default value. IPv6 can still be enabled / disabled by explicitly specifying the--ipv6
flag when doingdocker network create
.- How to verify it
Start a daemon from moby/moby master branch:
Then restart the daemon with
--default-network-opt=bridge=com.docker.network.enable_ipv6=true
:- A picture of a cute animal (not mandatory but encouraged)