Skip to content
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

api: Make EnableIPv6 optional (impl #2 - Option-based) #47872

Closed

Conversation

akerouanton
Copy link
Member

@akerouanton akerouanton commented May 29, 2024

- What I did

Currently, starting dockerd with --default-network-opt=bridge=com.docker.network.enable_ipv6=true has no effect as NetworkCreateRequest.EnableIPv6 is a basic bool.

This change uses a Rust-like Option type to mark the field as optional. If clients don't specify it, the default-network-opt will be applied if it's specified, otherwise it defaults to false.

- How to verify it

CI -- a new integration test has been added but will fail until #47853 is merged.

- Description for the changelog

- IPv6 can now be enabled by default on all custom networks using `dockerd --default-network-opt=bridge=com.docker.network.enable_ipv6=true` (and the matching json option).

- A picture of a cute animal (not mandatory but encouraged)

@akerouanton akerouanton added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking area/networking/ipv6 Issues related to ipv6 labels May 29, 2024
@akerouanton akerouanton added this to the 27.0.0 milestone May 29, 2024
@akerouanton akerouanton self-assigned this May 29, 2024
@akerouanton akerouanton marked this pull request as ready for review June 4, 2024 14:10
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

This is way more straightforward than #47867! No worrying about accidentally aliasing variables; it's just a value with value semantics.

@@ -451,7 +452,7 @@ type NetworkCreate struct {
CheckDuplicate bool `json:",omitempty"`
Driver string // Driver is the driver-name used to create the network (e.g. `bridge`, `overlay`)
Scope string // Scope describes the level at which the network exists (e.g. `swarm` for cluster-wide or `local` for machine level).
EnableIPv6 bool // EnableIPv6 represents whether to enable IPv6.
EnableIPv6 optional.Option[bool] // EnableIPv6 represents whether to enable IPv6.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EnableIPv6 optional.Option[bool] // EnableIPv6 represents whether to enable IPv6.
EnableIPv6 optional.Option[bool] `json:",omitempty"` // EnableIPv6 represents whether to enable IPv6.

to suppress marhsalling {"EnableIPv6": null}.

daemon/network.go Outdated Show resolved Hide resolved
Currently, starting dockerd with
`--default-network-opt=bridge=com.docker.network.enable_ipv6=true` has
no effect as `NetworkCreateRequest.EnableIPv6` is a basic bool.

This change uses a Rust-like `Option` type to mark the field as
optional. If clients don't specify it, the default-network-opt will be
applied if it's specified, otherwise it defaults to false.

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

Discussed during today's maintainers call and internally. Overall, everyone agrees Option[T] is a good thing as compared to pointer-based optional fields. However, there's some pushback as this would make the API inconsistent (ie. vs what we already do for other fields), and introducing a new dependency which isn't widely picked by the community isn't great.

We might still do that change at a latter time, but for all optional fields at once. And the 2nd problem could be overcomed by the use of an interface to not force a specific implementation.

Since #47867 has been merged, let's close this one.

@akerouanton akerouanton closed this Jun 6, 2024
@akerouanton akerouanton deleted the api-EnableIPv6-override-v2 branch June 6, 2024 18:30
@corhere
Copy link
Contributor

corhere commented Jun 6, 2024

And the 2nd problem could be overcomed by the use of an interface to not force a specific implementation.

Interfaces are reference types, which have the same aliasing issues as plain pointers. I suggested adding a concrete facade type to the github.com/docker/docker/api package to allow us to swap out the underlying implementation without it being a breaking change. E.g.:

type Option[T] struct { v optional.Option[T] }

func (o Option[T]) IsSome() bool { return o.v.IsSome() }
func (o option[T]) IsNone() bool { return o.v.IsNone() }
// etc...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking/ipv6 Issues related to ipv6 area/networking kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants