Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Revert "daemon: automatically set network EnableIPv6 if needed" #47306

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

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

akerouanton
Copy link
Member

- What I did

Turns out some users expect a network created with an IPv6 subnet and EnableIPv6=false to actually have no IPv6 connectivity. This PR restores that behavior.

- How to verify it

The unit test TestNetworkWithInvalidIPAM has been updated to make sure IPv6 subnets are ignored by ValidateIPAM when EnableIPv6=false.

- Description for the changelog

  • Make sure networks' parameter EnableIPv6 isn't ignored.

This reverts commit 5d5eeac.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Commit 4f47013 introduced a new validation step to make sure no
IPv6 subnet is configured on a network which has EnableIPv6=false.

Commit 5d5eeac then removed that validation step and automatically
enabled IPv6 for networks with a v6 subnet. But this specific commit
was reverted in c59e93a and now the error introduced by 4f47013
is re-introduced.

But it turns out some users expect a network created with an IPv6
subnet and EnableIPv6=false to actually have no IPv6 connectivity.
This restores that behavior.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@thaJeztah
Copy link
Member

I thought I commented here but ETOOMANYTHREADS; this is what we were discussing in our Slack;

The problem here (again) looks like our API doesn’t use a pointer for the “enable IPv6” option, so we don’t have an option to detect difference between “not set (auto)“, “true” (enable), or “false” (explicitly disabled by user)

I wonder if we can transition;

  • old clients / API versions won’t have a pointer (need to check), and if it doesn’t have omitempty, may be defaulting to false
  • on the daemon side we could use tri-state (pointer)
  • default to be auto (not set)
  • true to be MUST have IPv6 (if not supported this could be an error condition -> make API call fail)
  • false to be MUST have IPv6 disabled

(This is why I hate booleans, but the alternative would be a new API field (enum with auto, enabled, disabled))

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 to fix the issue (let's open a new ticket for looking at follow-ups?)

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

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.