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.

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

run, create, connect: add support for gw-priority #5664

Merged
merged 1 commit into from
Dec 6, 2024
Merged

run, create, connect: add support for gw-priority #5664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cli/command/container/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,9 @@ func parseNetworkAttachmentOpt(ep opts.NetworkAttachmentOpts) (*networktypes.End
}
}

epConfig := &networktypes.EndpointSettings{}
epConfig := &networktypes.EndpointSettings{
GwPriority: ep.GwPriority,
}
epConfig.Aliases = append(epConfig.Aliases, ep.Aliases...)
if len(ep.DriverOpts) > 0 {
epConfig.DriverOpts = make(map[string]string)
Expand Down
3 changes: 3 additions & 0 deletions cli/command/network/connect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type connectOptions struct {
aliases []string
linklocalips []string
driverOpts []string
gwPriority int
Copy link
Member

Choose a reason for hiding this comment

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

I'm probably a bit late realising, but should this have been a uint, or do we expect negative values to be ok? 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an int on purpose as it allows to set the priority to a negative value when some existing endpoints have the default priority 0 set. @robmry wrote about that here: moby/moby#48936 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

But wasn't higher higher priority, so if I want a new network to become the default, I need to give it a higher priority?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely. But if one of the current network give you a default gateway, and you want to stick with it, you can connect new networks with a negative priority.

Copy link
Member

Choose a reason for hiding this comment

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

(mostly considering if it's zero, then I did not consider it a priority (and alphabetic order is used), and only if I would explicitly want a network to take priority, I'd probably set a non-zero value; any value lower (including zero) would not take priority in that case)

Copy link
Member Author

Choose a reason for hiding this comment

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

You might consider priority of existing endpoints only when you connect a new network. In that case, having the ability to set a negative priority gives you a way to not touch the existing endpoint(s), but still get the behavior you want.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm good with this for now; perhaps we should brainstorm next week to see if there's still things to be done here.

  • default network (say ddd) without gw_priority; this becomes the default gateway
  • connect to other network (say ccc) without gw_priority; this sorts before ddd, so now takes over default gateway
  • ☝️ this is current situation, and potentially problematic, as it's implicit / unexpected
  • 👉 so, say, I don't want that to happen, I now set gw_priority=-999 to make sure it doesn't capture the gateway
  • But now I connect to another network, aaa. I don't want that to take over the default gateway, so I set gw_priority=-999.
  • But -999 was already set for ccc, so those are now "same priority", except for name.
  • ⚠️ if I would to connect another network (say bbb), but do NOT set a gw_priority, it would takeover the default gateway from ddd (as those both have 0 priority)

So, yes, there's some more controls here, but it's also shifting the goal-posts, and we may now end up with something similar to z-index or oomscore-adjust; "let's pick a high enough value" or "low enough value" that probably no other network has picked. Or at least, these numbers will become rather complicated to use, without either

  • "pick a low or high enough value" hoping no other network was given that value
  • or knowing what values were given to other networks and pick that value "+1" or "-1"
  • or keep track of values used, and continue incrementing or decrementing

I think at least we should consider setting a limit (which could be same as oom-score-adjust, e.g. -999 - 999); not sure if we need the whole range of +/- MAXINT32.

But perhaps we need to look at some other logic as well; I think in most cases, I'd expect the gateway to not change unless I want it to (i.e., once it's set, connecting another network should not change it, unless I want that new one to take over); almost considering if some of the enumerating could be handled daemon-side ("put me first, shift others one position down"), but that would mostly be for connecting to an existing container (for "create container with multiple networks", I could see a more declarative aproach being good).

}

func newConnectCommand(dockerCLI command.Cli) *cobra.Command {
Expand Down Expand Up @@ -55,6 +56,7 @@ func newConnectCommand(dockerCLI command.Cli) *cobra.Command {
flags.StringSliceVar(&options.aliases, "alias", []string{}, "Add network-scoped alias for the container")
flags.StringSliceVar(&options.linklocalips, "link-local-ip", []string{}, "Add a link-local address for the container")
flags.StringSliceVar(&options.driverOpts, "driver-opt", []string{}, "driver options for the network")
flags.IntVar(&options.gwPriority, "gw-priority", 0, "Highest gw-priority provides the default gateway. Accepts positive and negative values.")
return cmd
}

Expand All @@ -73,6 +75,7 @@ func runConnect(ctx context.Context, apiClient client.NetworkAPIClient, options
Links: options.links.GetAll(),
Aliases: options.aliases,
DriverOpts: driverOpts,
GwPriority: options.gwPriority,
})
}

Expand Down
2 changes: 2 additions & 0 deletions cli/command/network/connect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ func TestNetworkConnectWithFlags(t *testing.T) {
"driveropt1": "optval1,optval2",
"driveropt2": "optval4",
},
GwPriority: 100,
}
cli := test.NewFakeCli(&fakeClient{
networkConnectFunc: func(ctx context.Context, networkID, container string, config *network.EndpointSettings) error {
Expand All @@ -76,6 +77,7 @@ func TestNetworkConnectWithFlags(t *testing.T) {
{"ip6", "fdef:f401:8da0:1234::5678"},
{"link", "otherctr"},
{"link-local-ip", "169.254.42.42"},
{"gw-priority", "100"},
} {
err := cmd.Flags().Set(opt.name, opt.value)
assert.Check(t, err)
Expand Down
19 changes: 10 additions & 9 deletions docs/reference/commandline/container_run.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,15 +749,16 @@ To specify options when connecting to more than one network, use the extended sy
for the `--network` flag. Comma-separated options that can be specified in the extended
`--network` syntax are:

| Option | Top-level Equivalent | Description |
|-----------------|---------------------------------------|-------------------------------------------------|
| `name` | | The name of the network (mandatory) |
| `alias` | `--network-alias` | Add network-scoped alias for the container |
| `ip` | `--ip` | IPv4 address (e.g., 172.30.100.104) |
| `ip6` | `--ip6` | IPv6 address (e.g., 2001:db8::33) |
| `mac-address` | `--mac-address` | Container MAC address (e.g., 92:d0:c6:0a:29:33) |
| `link-local-ip` | `--link-local-ip` | Container IPv4/IPv6 link-local addresses |
| `driver-opt` | `docker network connect --driver-opt` | Network driver options |
| Option | Top-level Equivalent | Description |
|-----------------|---------------------------------------|-----------------------------------------------------------------------------------------|
| `name` | | The name of the network (mandatory) |
| `alias` | `--network-alias` | Add network-scoped alias for the container |
| `ip` | `--ip` | IPv4 address (e.g., 172.30.100.104) |
| `ip6` | `--ip6` | IPv6 address (e.g., 2001:db8::33) |
| `mac-address` | `--mac-address` | Container MAC address (e.g., 92:d0:c6:0a:29:33) |
| `link-local-ip` | `--link-local-ip` | Container IPv4/IPv6 link-local addresses |
| `driver-opt` | `docker network connect --driver-opt` | Network driver options |
| `gw-priority` | | Highest gw-priority provides the default gateway. Accepts positive and negative values. |

```console
$ docker network create --subnet 192.0.2.0/24 my-net1
Expand Down
17 changes: 9 additions & 8 deletions docs/reference/commandline/network_connect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@ Connect a container to a network

### Options

| Name | Type | Default | Description |
|:--------------------|:--------------|:--------|:-------------------------------------------|
| [`--alias`](#alias) | `stringSlice` | | Add network-scoped alias for the container |
| `--driver-opt` | `stringSlice` | | driver options for the network |
| [`--ip`](#ip) | `string` | | IPv4 address (e.g., `172.30.100.104`) |
| `--ip6` | `string` | | IPv6 address (e.g., `2001:db8::33`) |
| [`--link`](#link) | `list` | | Add link to another container |
| `--link-local-ip` | `stringSlice` | | Add a link-local address for the container |
| Name | Type | Default | Description |
|:--------------------|:--------------|:--------|:----------------------------------------------------------------------------------------|
| [`--alias`](#alias) | `stringSlice` | | Add network-scoped alias for the container |
| `--driver-opt` | `stringSlice` | | driver options for the network |
| `--gw-priority` | `int` | `0` | Highest gw-priority provides the default gateway. Accepts positive and negative values. |
| [`--ip`](#ip) | `string` | | IPv4 address (e.g., `172.30.100.104`) |
| `--ip6` | `string` | | IPv6 address (e.g., `2001:db8::33`) |
| [`--link`](#link) | `list` | | Add link to another container |
| `--link-local-ip` | `stringSlice` | | Add a link-local address for the container |


<!---MARKER_GEN_END-->
Expand Down
8 changes: 8 additions & 0 deletions opts/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"regexp"
"strconv"
"strings"
)

Expand All @@ -16,6 +17,7 @@ const (
networkOptMacAddress = "mac-address"
networkOptLinkLocalIP = "link-local-ip"
driverOpt = "driver-opt"
gwPriorityOpt = "gw-priority"
)

// NetworkAttachmentOpts represents the network options for endpoint creation
Expand All @@ -28,6 +30,7 @@ type NetworkAttachmentOpts struct {
IPv6Address string
LinkLocalIPs []string
MacAddress string
GwPriority int
}

// NetworkOpt represents a network config in swarm mode.
Expand Down Expand Up @@ -83,6 +86,11 @@ func (n *NetworkOpt) Set(value string) error { //nolint:gocyclo
netOpt.DriverOpts = make(map[string]string)
}
netOpt.DriverOpts[key] = val
case gwPriorityOpt:
netOpt.GwPriority, err = strconv.Atoi(val)
if err != nil {
return fmt.Errorf("invalid gw-priority: %w", err)
}
default:
return errors.New("invalid field key " + key)
}
Expand Down
10 changes: 10 additions & 0 deletions opts/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,16 @@ func TestNetworkOptAdvancedSyntax(t *testing.T) {
},
},
},
{
value: "name=docknet1,gw-priority=10",
expected: []NetworkAttachmentOpts{
{
Target: "docknet1",
Aliases: []string{},
GwPriority: 10,
},
},
},
}
for _, tc := range testCases {
t.Run(tc.value, func(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion vendor.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ require (
github.com/distribution/reference v0.6.0
github.com/docker/cli-docs-tool v0.8.0
github.com/docker/distribution v2.8.3+incompatible
github.com/docker/docker v27.0.2-0.20241120142749-e5c2b5e10d68+incompatible // master (v-next)
github.com/docker/docker v27.0.2-0.20241202115249-87fbd9cd3b37+incompatible // master (v-next)
github.com/docker/docker-credential-helpers v0.8.2
github.com/docker/go-connections v0.5.0
github.com/docker/go-units v0.5.0
Expand Down
4 changes: 2 additions & 2 deletions vendor.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ github.com/docker/cli-docs-tool v0.8.0/go.mod h1:8TQQ3E7mOXoYUs811LiPdUnAhXrcVsB
github.com/docker/distribution v2.7.1+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w=
github.com/docker/distribution v2.8.3+incompatible h1:AtKxIZ36LoNK51+Z6RpzLpddBirtxJnzDrHLEKxTAYk=
github.com/docker/distribution v2.8.3+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w=
github.com/docker/docker v27.0.2-0.20241120142749-e5c2b5e10d68+incompatible h1:ZWh4HhdUCagAd3S+gsFPOobHbc562obYFSrz3irGSsU=
github.com/docker/docker v27.0.2-0.20241120142749-e5c2b5e10d68+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
github.com/docker/docker v27.0.2-0.20241202115249-87fbd9cd3b37+incompatible h1:Ct0/s+pkUCDPBsQmLVHnBEas8OlTRxNvDXdSa6Y2PfE=
github.com/docker/docker v27.0.2-0.20241202115249-87fbd9cd3b37+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
github.com/docker/docker-credential-helpers v0.8.2 h1:bX3YxiGzFP5sOXWc3bTPEXdEaZSeVMrFgOr3T+zrFAo=
github.com/docker/docker-credential-helpers v0.8.2/go.mod h1:P3ci7E3lwkZg6XiHdRKft1KckHiO9a2rNtyFbZ/ry9M=
github.com/docker/go v1.5.1-1.0.20160303222718-d30aec9fd63c h1:lzqkGL9b3znc+ZUgi7FlLnqjQhcXxkNM/quxIjBVMD0=
Expand Down
11 changes: 11 additions & 0 deletions vendor/github.com/docker/docker/api/swagger.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions vendor/github.com/docker/docker/api/types/network/endpoint.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion vendor/github.com/docker/docker/registry/service.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 0 additions & 4 deletions vendor/github.com/docker/docker/registry/service_v2.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 2 additions & 21 deletions vendor/github.com/docker/docker/registry/types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ github.com/docker/distribution/registry/client/transport
github.com/docker/distribution/registry/storage/cache
github.com/docker/distribution/registry/storage/cache/memory
github.com/docker/distribution/uuid
# github.com/docker/docker v27.0.2-0.20241120142749-e5c2b5e10d68+incompatible
# github.com/docker/docker v27.0.2-0.20241202115249-87fbd9cd3b37+incompatible
## explicit
github.com/docker/docker/api
github.com/docker/docker/api/types
Expand Down
Loading