-
Notifications
You must be signed in to change notification settings - Fork 2k
vendor: github.com/moby/moby/api, github.com/moby/moby/client master (takę 2) #6545
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
base: master
Are you sure you want to change the base?
Conversation
NetworkID string `json:",omitempty"` | ||
Addr netip.Prefix `json:",omitempty"` |
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.
docker stack deploy --compose-file=./testdata/full-stack.yml --detach=false test-stack-remove
ParseAddr("10.0.1.2/24"): unexpected character (at "/24")
docker stack ls
ParseAddr("10.0.1.2/24"): unexpected character (at "/24")
It looks like the service returns this;
curl -s --unix-socket /var/run/docker.sock http://localhost/v1.51/services/bmu1cozv5cca | jq .Endpoint
{
"Spec": {
"Mode": "vip"
},
"VirtualIPs": [
{
"NetworkID": "yy6jjster3oudzp0ck77ai0id",
"Addr": "10.0.1.2/24"
}
]
}
So older versions of docker ... returned an address range in CIDR notation?
docker network create --scope=swarm --driver=overlay hello
0k61ecfc29ifqj5lnam9usveu
docker service create --network hello --name foo alpine top
mx0vntfyklgnikrzj59gnmxur
overall progress: 1 out of 1 tasks
1/1: running
verify: Service mx0vntfyklgnikrzj59gnmxur converged
docker service inspect --format '{{json .Endpoint}}' foo
{"Spec":{"Mode":"vip"},"VirtualIPs":[{"NetworkID":"0k61ecfc29ifqj5lnam9usveu","Addr":"10.0.1.2/24"}]}
And looks indeed that .. it does?
https://github.com/moby/moby/blob/b8a4f6534f736bc5223c9ee13d210a55a22481d7/daemon/cluster/executor/container/container.go#L573-L583
Value is set from the Swarm API?
https://github.com/moby/moby/blob/6bbb92df70fb433ce89353e41c2158db67feceb0/daemon/cluster/convert/network.go#L121-L142
8782f4b
to
8f2b791
Compare
Not there yet; task listing also fails; docker stack ps test-stack-remove -f=desired-state=running --format json
ParseAddr("10.0.1.3/24"): unexpected character (at "/24") curl -s --unix-socket /var/run/docker.sock 'http://localhost/v1.51/tasks' | jq . [
{
"ID": "l4rynxbamvixckla83rcdk557",
"Version": {
"Index": 52
},
"CreatedAt": "2025-10-09T07:53:35.684913628Z",
"UpdatedAt": "2025-10-09T07:53:37.761470712Z",
"Labels": {},
"Spec": {
"ContainerSpec": {
"Image": "alpine:latest@sha256:4b7ce07002c69e8f3d704a9c5d6fd3053be500b7f1c69fc0d80990c2ad8dd412",
"Labels": {
"com.docker.stack.namespace": "test-stack-remove"
},
"Args": [
"top"
],
"Privileges": {
"CredentialSpec": null,
"SELinuxContext": null,
"NoNewPrivileges": false
},
"Isolation": "default"
},
"Resources": {},
"Placement": {
"Platforms": [
{
"Architecture": "amd64",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "arm64",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "386",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "ppc64le",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "riscv64",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "s390x",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
}
]
},
"Networks": [
{
"Target": "c1i5g04zw3zkznjvjbbv4iizc",
"Aliases": [
"one"
]
}
],
"ForceUpdate": 0
},
"ServiceID": "kp75t36rdju3q9sys3ir38rd4",
"Slot": 1,
"NodeID": "n1rgi7t220c5lgsvwu2so3krr",
"Status": {
"Timestamp": "2025-10-09T07:53:37.718913879Z",
"State": "running",
"Message": "started",
"ContainerStatus": {
"ContainerID": "a1693df4c760b41d38e3c13027fcdd5a88040b0f267dfcad2ace4a66e29f04d2",
"PID": 69448,
"ExitCode": 0
},
"PortStatus": {}
},
"DesiredState": "running",
"NetworksAttachments": [
{
"Network": {
"ID": "c1i5g04zw3zkznjvjbbv4iizc",
"Version": {
"Index": 38
},
"CreatedAt": "2025-10-09T07:53:33.956330835Z",
"UpdatedAt": "2025-10-09T07:53:33.957696627Z",
"Spec": {
"Name": "test-stack-remove_default",
"Labels": {
"com.docker.stack.namespace": "test-stack-remove"
},
"DriverConfiguration": {
"Name": "overlay"
},
"Scope": "swarm"
},
"DriverState": {
"Name": "overlay",
"Options": {
"com.docker.network.driver.overlay.vxlanid_list": "4098"
}
},
"IPAMOptions": {
"Driver": {
"Name": "default"
},
"Configs": [
{
"Subnet": "10.0.1.0/24",
"Gateway": "10.0.1.1"
}
]
}
},
"Addresses": [
"10.0.1.3/24"
]
}
],
"Volumes": null
},
{
"ID": "to1zvgc2flky65q092g9umvr6",
"Version": {
"Index": 53
},
"CreatedAt": "2025-10-09T07:53:37.485566337Z",
"UpdatedAt": "2025-10-09T07:53:37.964857212Z",
"Labels": {},
"Spec": {
"ContainerSpec": {
"Image": "alpine:latest@sha256:4b7ce07002c69e8f3d704a9c5d6fd3053be500b7f1c69fc0d80990c2ad8dd412",
"Labels": {
"com.docker.stack.namespace": "test-stack-remove"
},
"Args": [
"top"
],
"Privileges": {
"CredentialSpec": null,
"SELinuxContext": null,
"NoNewPrivileges": false
},
"Isolation": "default"
},
"Resources": {},
"Placement": {
"Platforms": [
{
"Architecture": "amd64",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "arm64",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "386",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "ppc64le",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "riscv64",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
},
{
"Architecture": "s390x",
"OS": "linux"
},
{
"Architecture": "unknown",
"OS": "unknown"
}
]
},
"Networks": [
{
"Target": "c1i5g04zw3zkznjvjbbv4iizc",
"Aliases": [
"two"
]
}
],
"ForceUpdate": 0
},
"ServiceID": "o5gzytctv0si0j2mm81cdh58b",
"Slot": 1,
"NodeID": "n1rgi7t220c5lgsvwu2so3krr",
"Status": {
"Timestamp": "2025-10-09T07:53:37.889173462Z",
"State": "running",
"Message": "started",
"ContainerStatus": {
"ContainerID": "1f80279860feedcd36162b757a285de87a687893a679da7d5316d07872bc434c",
"PID": 69504,
"ExitCode": 0
},
"PortStatus": {}
},
"DesiredState": "running",
"NetworksAttachments": [
{
"Network": {
"ID": "c1i5g04zw3zkznjvjbbv4iizc",
"Version": {
"Index": 38
},
"CreatedAt": "2025-10-09T07:53:33.956330835Z",
"UpdatedAt": "2025-10-09T07:53:33.957696627Z",
"Spec": {
"Name": "test-stack-remove_default",
"Labels": {
"com.docker.stack.namespace": "test-stack-remove"
},
"DriverConfiguration": {
"Name": "overlay"
},
"Scope": "swarm"
},
"DriverState": {
"Name": "overlay",
"Options": {
"com.docker.network.driver.overlay.vxlanid_list": "4098"
}
},
"IPAMOptions": {
"Driver": {
"Name": "default"
},
"Configs": [
{
"Subnet": "10.0.1.0/24",
"Gateway": "10.0.1.1"
}
]
}
},
"Addresses": [
"10.0.1.6/24"
]
}
],
"Volumes": null
}
] |
8f2b791
to
10004b9
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Getting close now; remaining failures;
Looks like
|
10004b9
to
8cb5b2c
Compare
Oh! I was wrong there; it did NOT accept that, but previously the cobra command would fail, and now the flag itself invalidates it; it expects an error for that test; cli/cli/command/network/create_test.go Lines 130 to 135 in b1ea4fe
But expected the flag parsing to not fail (as it was a cli/cli/command/network/create_test.go Lines 146 to 148 in b1ea4fe
|
bfdd48d
to
6974f4f
Compare
d7f690c
to
10c859d
Compare
I continued looking at this failure;
And.... LOL... it's just because the test is .. bad? The flag returns the same value twice because they parse to the same 😂 https://go.dev/play/p/r7a0txKjths package main
import (
"fmt"
"net"
"strings"
)
func main() {
for _, ipNetStr := range []string{"192.168.1.0/24", "192.168.1.200/24"} {
_, n, err := net.ParseCIDR(strings.TrimSpace(ipNetStr))
if err != nil {
panic(err)
}
fmt.Println("parsed:", ipNetStr, "->", n.String())
}
} Above outputs;
|
57ce7eb
to
00489b7
Compare
651241f
to
16ca6d4
Compare
Merged moby/moby#51150 |
full diff: moby/moby@4ca8aed...ad72822 Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
16ca6d4
to
8f4ff89
Compare
559bc11
to
9818ee0
Compare
| Name | Type | Default | Description | | ||
|:---------------------------------------|:---------|:--------|:-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| [`-f`](#filter), [`--filter`](#filter) | `filter` | | Filter output based on conditions provided | | ||
| [`-f`](#filter), [`--filter`](#filter) | `filter` | `{}` | Filter output based on conditions provided | |
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.
Wondering if we can hide the default for these (as {}
isn't very informative for the user)
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.
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.
Oh, I could play with that idea. Definitely not the most urgent I guess; also because we need to look at some of those custom options and if they're still needed / critical or if we could use more standard options / flags.
| `--ip` | `ip` | `<nil>` | IPv4 address (e.g., 172.30.100.104) | | ||
| `--ip6` | `ip` | `<nil>` | IPv6 address (e.g., 2001:db8::33) | |
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.
Same for these
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.
A few optional options just have an empty field where the default is empty (--hostname
, --expose
etc)... maybe that'd be best here too?
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.
Yeah, I want to look what's needed to hide this; need to look how the docs-tool gets the information from the flags (possibly the flag itself returns <nil>
as a string for empty values.
c9c98f2
to
4a819bb
Compare
Signed-off-by: Sebastiaan van Stijn <[email protected]>
4a819bb
to
e707d9c
Compare
func completeNames(dockerCLI completion.APIClientProvider, state pluginState) cobra.CompletionFunc { | ||
return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { | ||
f := filters.NewArgs() | ||
f := make(client.Filters) |
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.
Oh, we can't just do var f client.Filters
because the type is just a map...
Not sure if that's the best design tbh 🙈
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.
But yeah, I don't think there's a better option if we want to avoid passing by pointer.
At least f := client.Filters{}
still works 😅
stdlib's http.Header
also seems to have this issue, so I guess that's idiomatic.
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.
Yes, I had the same idea at the time; "can we ... somehow... make it work with the zero/nil value", but don't have good options there; see the discussion here; moby/moby#51115 (comment)
assert.DeepEqual(t, config.MacAddress, tc.expectedCfg.MacAddress) //nolint:staticcheck // ignore SA1019: field is deprecated, but still used on API < v1.44. | ||
assert.DeepEqual(t, hConfig.NetworkMode, tc.expectedHostCfg.NetworkMode) | ||
assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected) | ||
assert.DeepEqual(t, nwConfig.EndpointsConfig, tc.expected, cmpopts.IgnoreUnexported(netip.Addr{})) |
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 think cmpopts.EquateComparable would be better here? (Ditto for other uses.)
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.
Oh! Let me try if that alone works; ISTR it didn't like it, but I made so many changes, that ... maybe it does; I'll give it a try!
if len(ip) == 0 { | ||
return netip.Addr{} | ||
} | ||
if ip4 := ip.To4(); ip4 != nil { | ||
a, _ := netip.AddrFromSlice(ip4) | ||
return a | ||
} | ||
if ip16 := ip.To16(); ip16 != nil { | ||
a, _ := netip.AddrFromSlice(ip16) | ||
return a | ||
} | ||
return netip.Addr{} |
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.
if len(ip) == 0 { | |
return netip.Addr{} | |
} | |
if ip4 := ip.To4(); ip4 != nil { | |
a, _ := netip.AddrFromSlice(ip4) | |
return a | |
} | |
if ip16 := ip.To16(); ip16 != nil { | |
a, _ := netip.AddrFromSlice(ip16) | |
return a | |
} | |
return netip.Addr{} | |
a, _ := netip.AddrFromSlice(ip) | |
return a.Unmap() |
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.
Ha! Yes, that's a lot shorter; I think some of these were me being lazy, and putting AI at work to quickly write a utility, while I was making the other changes; I already made a mental note "is this all needed", but forgot to look at it in depth.
func ipNetToPrefix(n net.IPNet) netip.Prefix { | ||
if n.IP == nil { | ||
return netip.Prefix{} | ||
} | ||
|
||
ip := n.IP.To4() | ||
if ip == nil { | ||
ip = n.IP.To16() | ||
} | ||
if ip == nil { | ||
return netip.Prefix{} | ||
} | ||
|
||
addr, ok := netip.AddrFromSlice(ip) | ||
if !ok { | ||
return netip.Prefix{} | ||
} | ||
|
||
ones, _ := n.Mask.Size() | ||
return netip.PrefixFrom(addr, ones) | ||
} |
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.
Probably best to copy moby's netiputil.ToPrefix
here?
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.
Yes, I was considering doing so (heh. it's how I also ended up opening this PR, because I had that still un-committed in my fork 😂
} | ||
} | ||
iData[s] = &network.IPAMConfig{Subnet: s, AuxAddress: map[string]string{}} | ||
sn, err := parsePrefixOrAddr(s) |
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 don't think it could ever make sense for a subnet to be a single address, "/32" or "/128" ... the old code would have accepted it and the daemon will return an error. But to keep things simple, this might be a case where it's ok for the CLI itself to just reject the bad value?
(The API type is netip.Prefix
because it has to be a prefix, so I don't think there's value in converting a single address into a prefix.)
sn, err := parsePrefixOrAddr(s) | |
sn, err := netip.ParsePrefix(s) |
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.
Good idea; yes, it's fine for the CLI to bail out early for these. (It's the compatibility with responses / requests that's more important). I'll change!
|
||
// Populate non-overlapping subnets into consolidation map | ||
for _, s := range options.subnets { | ||
// TODO(thaJeztah): is all this validation needed on the CLI-side? |
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 think it is ... because options like --subnet 192.0.2.0/24 --subnet 198.51.100.0/24 --subnet 2001:db8:1234::/64 --gateway 198.51.100.0 --ip-range 192.0.2.128/25
need to be collated into three network.IPAMConfig
objects (one per subnet) - and the only way to work out which options belong in which IPAMConfig
is to understand the subnets and addresses.
And, for example ... if there are overlapping subnets the other options may be ambiguous, if there's more than one gateway in a subnet there's no way to represent that in IPAMConfig
, etc.
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.
Ah, yes, you mentioned that, and I guess I didn't really register that. So, yes, looks like we still need this, but perhaps we should have a utility for that in the client; at least once we have functional options, this seems like something ideal to be handled there, so that others don't have to re-invent the exact same logic WDYT?
I guess we could already do that before the functional options 🤔
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.
It DOES make me wonder though if the ipamOptions
struct would've been the better type for the client.NetworkCreateOptions
cli/cli/command/network/create.go
Lines 35 to 42 in 6ddff81
type ipamOptions struct { | |
driver string | |
subnets []string | |
ipRanges []string | |
gateways []string | |
auxAddresses opts.MapOpts | |
driverOpts opts.MapOpts | |
} |
Because ultimately it means that we expand the ipamOptions
into a network.IPAM
struct, so that single options struct into a slice of []network.IPAMConfig
, but all data needed for that is already captured in ipamOptions
?
// IPAM represents IP Address Management | |
type IPAM struct { | |
Driver string | |
Options map[string]string // Per network IPAM driver options | |
Config []IPAMConfig | |
} | |
// IPAMConfig represents IPAM configurations | |
type IPAMConfig struct { | |
Subnet string `json:",omitempty"` | |
IPRange string `json:",omitempty"` | |
Gateway string `json:",omitempty"` | |
AuxAddress map[string]string `json:"AuxiliaryAddresses,omitempty"` | |
} |
Or would there be cases where manually crafting the []network.IPAMConfig
is something that would be done? 🤔
} | ||
|
||
func toNetipAddrSlice(ips []net.IP) []netip.Addr { | ||
netips := make([]netip.Addr, 0, len(ips)) |
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.
Could avoid turning a nil slice into an empty slice with a len(ips) == 0
check?
apiClient := dockerCLI.Client() | ||
|
||
defaultAddrPool := make([]string, 0, len(opts.defaultAddrPools)) | ||
// TODO(thaJeztah): should we change opts.defaultAddrPools to be the right type? What formats does it accept? |
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.
Only prefixes, it's already a []net.IPNet
- so netip.Prefix
will work too ... it can change when we have an option-parsing flag type for it.
| `--ip` | `ip` | `<nil>` | IPv4 address (e.g., 172.30.100.104) | | ||
| `--ip6` | `ip` | `<nil>` | IPv6 address (e.g., 2001:db8::33) | |
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.
A few optional options just have an empty field where the default is empty (--hostname
, --expose
etc)... maybe that'd be best here too?
proto, port := nat.SplitProtoPort(string(portRangeProto)) | ||
portInt, _ := strconv.ParseUint(port, 10, 16) | ||
proto = strings.ToLower(proto) | ||
portProto.Num() |
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.
Stray call to remove?
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.
Yes! Very likely was for debugging, or me using autocomplete to discover "what was the method named again?"
|
||
if err != nil && binding.HostPort != "" { | ||
return nil, fmt.Errorf("invalid hostport binding (%s) for port (%s)", binding.HostPort, port) | ||
return nil, fmt.Errorf("invalid hostport binding (%s) for port (%d)", binding.HostPort, portProto.Num()) |
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.
portProto.Num()
-> portInt
(or remove portInt
?)
full diff: moby/moby@4ca8aed...cfefa33
- What I did
- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)