Add dnsnames field & fix alias field - #29227
Conversation
fd925fb to
7f92308
Compare
Luap99
left a comment
There was a problem hiding this comment.
approach looks good to me but tests are failing
| networks := make([]types.NamedPerNetworkOptions, len(opts.Networks)) | ||
| copy(networks, opts.Networks) |
There was a problem hiding this comment.
I don't think we need a copy here, the options we are given can be expected to be modified. This should not cause any negative downsides AFAIK
| networks := make([]types.NamedPerNetworkOptions, len(opts.Networks)) | ||
| copy(networks, opts.Networks) | ||
| for i := range networks { | ||
| networks[i].Aliases = append(slices.Clone(networks[i].Aliases), getExtraNetworkAliases(c)...) |
There was a problem hiding this comment.
This code could use slices.Concat() instead of the append and then could also get rid of that clone as concat will allocate a new slice always
| netInfo := new(define.InspectAdditionalNetwork) | ||
| netInfo.NetworkID = getNetworkID(net.Name) | ||
| netInfo.Aliases = net.Aliases | ||
| netInfo.DNSNames = append(append([]string{c.Name()}, net.Aliases...), getExtraNetworkAliases(c)...) |
There was a problem hiding this comment.
same here and for the other nested appends consider using slices.Concat instead
884e420 to
d519dd4
Compare
|
Hi, Comparing it apart from much better tests, different approach how the
{
"Networks": {
"podman": {
...
"Aliases": [
"844f6d6ce177"
],
"DNSNames": [
"dnstest",
"844f6d6ce177",
"844f6d6ce177"
]
}
}
Thanks. |
|
I think the second difference comes in because in 1.44 dnsnames are added, and in 1.45 alias is reserved exclusively for user-defined names? So in this PR aliases are empty and dont contain the short container ID unless the alias is set by the user using a option. And since the fields are changed in InspectAdditionalNetwork, the cli and libpod api should both be updated |
Yes, I also understand it like you. What I meant was only the duplicate value that would Docker deduplicate. Maybe it's a non-issue, though. I'm not sure about the libpod endpoint: ➜ curl -s http://localhost:8080/v6.0.0/libpod/containers/json | jq .
[
{
"AutoRemove": false,
"Command": [
"top"
],
"Created": "2026-07-31T16:19:20.388670122+02:00",
"CreatedAt": "",
"CIDFile": "",
"Exited": false,
"ExitedAt": -62135596800,
"ExitCode": 0,
"ExposedPorts": null,
"Id": "2c58132268f74f004b6969e10b9998f434850702456bec011d00acfc2a3322cd",
"Image": "docker.io/library/alpine:latest",
"ImageID": "1991bd789d7184290c3cce84fd6af068b8b745e9bddf178661ce7f5ecf68135c",
"IsInfra": false,
"Labels": null,
"Mounts": [],
"Names": [
"dnstest"
],
"Namespaces": {},
"Networks": [
"podman"
],
"Pid": 512040,
"Pod": "",
"PodName": "",
"Ports": null,
"Restarts": 0,
"Size": null,
"StartedAt": 1785507560,
"State": "running",
"Status": ""
}
]For comparison: ➜ curl -s http://localhost:8080/v6.0.0/libpod/containers/json | jq .
[
{
...
"Names": [
"dnstest"
],
"Namespaces": {},
"Networks": [
"podman"
],
"DNSNames": {
"podman": [
"dnstest",
"844f6d6ce177"
]
},
...
}
] |
|
Oh maybe not then. I'll update the libpod endpoint. I don't think we need to worry about dedup? |
Yeah, I think that with your other changes it's less of an issue. You can probably set multiple same aliases (haven't tried that), but it's a completely minor thing. Edit: Yes, you can, but it's on you if you set the same alias multiple times. {
"Networks": {
"podman": {
...
"Aliases": [
"alias",
"alias"
],
...
"DNSNames": [
"dnstest",
"alias",
"alias",
"19c1e70a47f0"
]
}
}
} |
|
Looks like the PR in its current state does have this behavior in the libpod endpoint: Re-pushed for de-dup though @simek-m PTAL, thanks! |
Docker now reserves alias specfically for user-defined aliases, and uses another field, dnsnames, to hold all aliases, including container ID and name. Part of docker v1.45 compat work, but this touches the cli too, since they updated it there too. Signed-off-by: Ashley Cui <acui@redhat.com>
Yes, re-tested and it looks all correct. I was wrong about |
|
@podman-container-tools/podman-maintainers PTAL and merge |
|
LGTM |
Docker now reserves alias specfically for user-defined aliases, and uses another field, dnsnames, to hold all aliases, including container ID and name.
Part of docker v1.45 compat work, but this touches the cli too, since they updated it there too.