Skip to content

Drop "-o com.docker.network.enable_ipv[46]" if overridden #49866

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 25, 2025
Merged

Drop "-o com.docker.network.enable_ipv[46]" if overridden #49866

merged 1 commit into from
Apr 25, 2025

Conversation

robmry
Copy link
Contributor

@robmry robmry commented Apr 25, 2025 •
edited
Loading

- What I did

When a network is created with -o com.docker.network.enable_ipv4 (including via default-network-opts in daemon config), and EnableIPv4 is present in the API request (including when CLI option --ipv4 is used) - the top-level API value is used and the -o is ignored.

But, the -o still shows up in Options in inspect output, which is confusing if the values are different.

Ditto for enable_ipv6 and option --ipv6.

For example ...

# docker network create --ipv4=false --ipv6 b6
ea36bf0f04afc105a0a88c2bfba83cbc30967a463514f7b75d20c5046cb1ab6e
# docker network inspect b6
[
    {
        "Name": "b6",
        [...]
        "EnableIPv4": false,
        "EnableIPv6": true,
        "IPAM": {
            "Driver": "default",
            "Options": {},
            "Config": [
                {
                    "Subnet": "fd1c:c5b3:150a::/64",
                    "Gateway": "fd1c:c5b3:150a::1"
                }
            ]
        },
        [...]
        "Options": {
            "com.docker.network.enable_ipv4": "true",
            "com.docker.network.enable_ipv6": "false"
        },
        "Labels": {}
    }
]

Docker Desktop v4.42 sets these options in default-network-opts, see docker/cli#2899 (comment).

- How I did it

Drop the "-o" if the corresponding top-level API option is set.

- How to verify it

New integration test.

With the fix, the Options value in the example above becomes {}.

- Human readable description for the release notes

- Do not display network options `com.docker.network.enable_ipv4` or `com.docker.network.enable_ipv6` in inspect output if they have been overridden by `EnableIPv4` or `EnableIPv6` in the network create request.

@robmry
Drop "-o com.docker.network.enable_ipv[46]" if overridden
37725b5 
When a network is created with "-o com.docker.network.enable_ipv4'
(including via "default-network-opts" in daemon config), and
EnableIPv4 is present in the API request (including when CLI option
"--ipv4" is used) - the top-level API value is used and the '-o'
is ignored.

But, the "-o" still shows up in Options in inspect output, which is
confusing if the values are different.

So, drop the "-o" if the top-level API option is set. Ditto IPv6.

Signed-off-by: Rob Murray <[email protected]>
@robmry robmry added area/networking kind/bugfix PR's that fix bugs labels Apr 25, 2025
@robmry robmry self-assigned this Apr 25, 2025
@robmry robmry added this to the 28.2.0 milestone Apr 25, 2025
@robmry robmry requested review from akerouanton and thaJeztah April 25, 2025 10:42
@thaJeztah
Copy link
Member

Thanks! This looks good.

Silly question; can we distinguish between "defaults" and "user passed these options?"

Thinking out loud: do the com.docker.xxxx options provide much value after they have been applied? (thus: reflected in EnableIPv4 and EnableIPv6 ), as they're effectively 2 different mechanisms to set the same field, correct?

Haven't given that a lot of thought, so really just "thinking out loud!"

thaJeztah
thaJeztah approved these changes Apr 25, 2025
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@robmry
Copy link
Contributor Author

robmry commented Apr 25, 2025

Silly question; can we distinguish between "defaults" and "user passed these options?"

Internally when the network's created, yes - the API field is a pointer, nil if not set, and the -o options are just absent unless set.

After that, if the -o option's present, that's where the setting came from. Otherwise, if the value is non-default, it came from the top-level API field. But, if the values are default, we can't tell whether they were set explicitly or just allowed to default.

Thinking out loud: do the com.docker.xxxx options provide much value after they have been applied? (thus: reflected in EnableIPv4 and EnableIPv6 ), as they're effectively 2 different mechanisms to set the same field, correct?

Yes, they're just setting the same thing. The label version is needed to allow defaults to be configured, and the API version is needed because labels are terrible UX!

Haven't given that a lot of thought, so really just "thinking out loud!"

Ok - I'm not quite sure where you're heading ... are you thinking we need to eliminate one or the other, or to be explicit about using defaults?

@thaJeztah
Copy link
Member

Yeah, it's a tricky one; basically, "as a user, where should I look to see what the active configuration is?"

  • Check presence of com.docker.xxxx=value ?
  • Check CorrespondingField ?
  • ☝️ do I need to "pick one?", and is "non-occurence" of one (labels!) mean it was a built-in default, or ..?

So .. we could almost go the reverse direction and update the labels to match the field (if it was set 😂 ), but likely would end up in more trouble.

But I guess this brings us back to the recurring story of "desired" config vs "actual config / state", but also "where did this state come from?";

  • Did I (the user) request this field to be set to value?
  • Was field set to value because of a builtin default?
  • (Or was it set to value because of a daemon-configured default?)
  • ... and then there's fields (in some cases?) that may be "dynamic" (state can change?)
  • FUN!

@robmry
Copy link
Contributor Author

robmry commented Apr 25, 2025

Ah, yes I see. The top level "EnableIPv4"/""EnableIPv6" are the definitive running-state, and labels might go missing, but that's not obvious. The whole label-options mechanism is messy, but there's no straightforward fix.

@robmry robmry merged commit 19e7990 into moby:master Apr 25, 2025
155 of 156 checks passed
@thaJeztah thaJeztah mentioned this pull request Apr 30, 2025
Closed
@robmry robmry deleted the drop_enable_ipv_opt_if_overridden branch May 14, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akerouanton akerouanton akerouanton approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

@robmry robmry

Labels
area/networking impact/changelog kind/bugfix PR's that fix bugs version/27.0
Projects
None yet
Milestone
28.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@robmry @thaJeztah @akerouanton