-
Notifications
You must be signed in to change notification settings - Fork 745
add MAC, IPv4, IPv6 addresses to nework inspect #4680
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -397,6 +397,64 @@ func TestNetworkInspect(t *testing.T) { | |||||||||||
| } | ||||||||||||
| }, | ||||||||||||
| }, | ||||||||||||
| { | ||||||||||||
| Description: "Test container network details", | ||||||||||||
| Setup: func(data test.Data, helpers test.Helpers) { | ||||||||||||
| helpers.Ensure("network", "create", data.Identifier("test-network")) | ||||||||||||
|
|
||||||||||||
| // See https://github.com/containerd/nerdctl/issues/4322 | ||||||||||||
| if runtime.GOOS == "windows" { | ||||||||||||
| time.Sleep(time.Second) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Create and start a container on this network | ||||||||||||
| helpers.Ensure("run", "-d", "--name", data.Identifier("test-container"), | ||||||||||||
| "--network", data.Identifier("test-network"), | ||||||||||||
| testutil.CommonImage, "sleep", nerdtest.Infinity) | ||||||||||||
|
|
||||||||||||
| // Get container ID for later use | ||||||||||||
| containerID := strings.Trim(helpers.Capture("inspect", data.Identifier("test-container"), "--format", "{{.Id}}"), "\n") | ||||||||||||
| data.Labels().Set("containerID", containerID) | ||||||||||||
| }, | ||||||||||||
| Cleanup: func(data test.Data, helpers test.Helpers) { | ||||||||||||
| helpers.Anyhow("rm", "-f", data.Identifier("test-container")) | ||||||||||||
| helpers.Anyhow("network", "remove", data.Identifier("test-network")) | ||||||||||||
| }, | ||||||||||||
| Command: func(data test.Data, helpers test.Helpers) test.TestableCommand { | ||||||||||||
| return helpers.Command("network", "inspect", data.Identifier("test-network")) | ||||||||||||
| }, | ||||||||||||
| Expected: func(data test.Data, helpers test.Helpers) *test.Expected { | ||||||||||||
| return &test.Expected{ | ||||||||||||
| Output: func(stdout string, t tig.T) { | ||||||||||||
| var dc []dockercompat.Network | ||||||||||||
| err := json.Unmarshal([]byte(stdout), &dc) | ||||||||||||
| assert.NilError(t, err, "Unable to unmarshal output") | ||||||||||||
| assert.Equal(t, 1, len(dc), "Expected exactly one network") | ||||||||||||
|
|
||||||||||||
| network := dc[0] | ||||||||||||
| assert.Equal(t, network.Name, data.Identifier("test-network")) | ||||||||||||
| assert.Equal(t, 1, len(network.Containers), "Expected exactly one container") | ||||||||||||
|
|
||||||||||||
| // Get the container details | ||||||||||||
| containerID := data.Labels().Get("containerID") | ||||||||||||
| container := network.Containers[containerID] | ||||||||||||
|
|
||||||||||||
| // Test container name | ||||||||||||
| assert.Equal(t, container.Name, data.Identifier("test-container")) | ||||||||||||
|
|
||||||||||||
| // Test IPv4Address has CIDR notation (not empty and contains '/') | ||||||||||||
| assert.Assert(t, container.IPv4Address != "", "IPv4Address should not be empty") | ||||||||||||
| assert.Assert(t, strings.Contains(container.IPv4Address, "/"), "IPv4Address should contain CIDR notation with /") | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, it would be better to verify that the container IP is within the subnet, in addition to checking that Example: _, subnet, _ := net.ParseCIDR(dc[0].IPAM.Config[0].Subnet)
containerIP, _, _ := net.ParseCIDR(container.IPv4Address)
assert.Assert(t, subnet.Contains(containerIP), "IPv4Address should be within the network's subnet") |
||||||||||||
|
|
||||||||||||
| // Test MacAddress is present and has valid format | ||||||||||||
| assert.Assert(t, container.MacAddress != "", "MacAddress should not be empty") | ||||||||||||
|
|
||||||||||||
| // Test IPv6Address is empty for IPv4-only network | ||||||||||||
| assert.Equal(t, "", container.IPv6Address, "IPv6Address should be empty for IPv4-only network") | ||||||||||||
|
Comment on lines
+447
to
+453
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When checking CI logs, there are below messages:
This is because nerdctl/pkg/containerinspector/containerinspector_windows.go Lines 25 to 29 in deec874
|
||||||||||||
| }, | ||||||||||||
| } | ||||||||||||
| }, | ||||||||||||
| }, | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| testCase.Run(t) | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -929,9 +929,9 @@ type Network struct { | |
| type EndpointResource struct { | ||
| Name string `json:"Name"` | ||
| // EndpointID string `json:"EndpointID"` | ||
| // MacAddress string `json:"MacAddress"` | ||
| // IPv4Address string `json:"IPv4Address"` | ||
| // IPv6Address string `json:"IPv6Address"` | ||
| MacAddress string `json:"MacAddress"` | ||
| IPv4Address string `json:"IPv4Address"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we are parsing a range of subnets but we are supposed take the first IPV4 and one IPV6 address i think for docker compatibility, how that is decided? |
||
| IPv6Address string `json:"IPv6Address"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: GlobalIPV6Address |
||
| } | ||
|
|
||
| type structuredCNI struct { | ||
|
|
@@ -949,6 +949,88 @@ type MemorySetting struct { | |
| DisableOOMKiller bool `json:"disableOOMKiller"` | ||
| } | ||
|
|
||
| // parseNetworkSubnets extracts and parses subnet configurations from IPAM config | ||
| func parseNetworkSubnets(ipamConfigs []IPAMConfig) []*net.IPNet { | ||
| var subnets []*net.IPNet | ||
| for _, config := range ipamConfigs { | ||
| if config.Subnet != "" { | ||
| _, subnet, err := net.ParseCIDR(config.Subnet) | ||
| if err != nil { | ||
| log.L.WithError(err).Warnf("failed to parse subnet %q", config.Subnet) | ||
| continue | ||
| } | ||
| subnets = append(subnets, subnet) | ||
| } | ||
| } | ||
| return subnets | ||
| } | ||
|
|
||
| // isUsableInterface checks if a network interface is usable (not loopback and up) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: and interface is up. |
||
| func isUsableInterface(iface *native.NetInterface) bool { | ||
| return iface.Interface.Flags&net.FlagLoopback == 0 && | ||
| iface.Interface.Flags&net.FlagUp != 0 | ||
| } | ||
|
|
||
| // setIPAddresses assigns IPv4 or IPv6 addresses from CIDR notation to the endpoint | ||
| func setIPAddresses(endpoint *EndpointResource, cidr string) { | ||
| ip, _, err := net.ParseCIDR(cidr) | ||
| if err != nil { | ||
| return | ||
| } | ||
| if ip.IsLoopback() || ip.IsLinkLocalUnicast() { | ||
| return | ||
| } | ||
|
|
||
| if ip.To4() != nil { | ||
| endpoint.IPv4Address = cidr | ||
| } else if ip.To16() != nil { | ||
| endpoint.IPv6Address = cidr | ||
| } | ||
| } | ||
|
|
||
| // matchInterfaceToSubnets tries to match an interface to network subnets | ||
| func matchInterfaceToSubnets(endpoint *EndpointResource, iface *native.NetInterface, subnets []*net.IPNet) bool { | ||
| for _, addr := range iface.Addrs { | ||
| ip, _, err := net.ParseCIDR(addr) | ||
| if err != nil || ip.IsLoopback() || ip.IsLinkLocalUnicast() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: handle error and other conditional separately. Not important but the purpose is different. |
||
| continue | ||
| } | ||
|
|
||
| for _, subnet := range subnets { | ||
| if subnet.Contains(ip) { | ||
| endpoint.MacAddress = iface.HardwareAddr | ||
| setIPAddresses(endpoint, addr) | ||
| return true | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the current implementation, when a container belongs to a dual-stack network with both sudo nerdctl network create --ipv6 --subnet 10.1.0.0/24 --subnet fd00::/64 test-dual-stack
sudo nerdctl run -d --name test-container --network test-dual-stack alpine sleep infinity
sudo nerdctl network inspect test-dual-stack | jq -r '.[].Containers'
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 here we are returning true but we are parsing a range of subnets. |
||
| } | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // populateEndpointFromNetNS finds and populates endpoint info from network namespace interfaces | ||
| func populateEndpointFromNetNS(endpoint *EndpointResource, interfaces []native.NetInterface, subnets []*net.IPNet) { | ||
| for _, iface := range interfaces { | ||
| if !isUsableInterface(&iface) { | ||
| continue | ||
| } | ||
|
|
||
| if len(subnets) > 0 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What situations can lead to an empty |
||
| if matchInterfaceToSubnets(endpoint, &iface, subnets) { | ||
| return // Found matching interface | ||
| } | ||
| // Continue to next interface if this one doesn't match any subnets | ||
| continue | ||
| } | ||
|
|
||
| // Fallback: use first usable interface (for networks without explicit subnets) | ||
| endpoint.MacAddress = iface.HardwareAddr | ||
| for _, addr := range iface.Addrs { | ||
| setIPAddresses(endpoint, addr) | ||
| } | ||
| return | ||
| } | ||
| } | ||
|
|
||
| func NetworkFromNative(n *native.Network) (*Network, error) { | ||
| var res Network | ||
|
|
||
|
|
@@ -973,15 +1055,20 @@ func NetworkFromNative(n *native.Network) (*Network, error) { | |
| res.Labels = *n.NerdctlLabels | ||
| } | ||
|
|
||
| // Parse network subnets for interface matching | ||
| networkSubnets := parseNetworkSubnets(res.IPAM.Config) | ||
|
|
||
| res.Containers = make(map[string]EndpointResource) | ||
| for _, container := range n.Containers { | ||
| res.Containers[container.ID] = EndpointResource{ | ||
| endpoint := EndpointResource{ | ||
| Name: container.Labels[labels.Name], | ||
| // EndpointID: container.EndpointID, | ||
| // MacAddress: container.MacAddress, | ||
| // IPv4Address: container.IPv4Address, | ||
| // IPv6Address: container.IPv6Address, | ||
| } | ||
|
|
||
| if container.Process != nil && container.Process.NetNS != nil { | ||
| populateEndpointFromNetNS(&endpoint, container.Process.NetNS.Interfaces, networkSubnets) | ||
| } | ||
|
|
||
| res.Containers[container.ID] = endpoint | ||
| } | ||
|
|
||
| return &res, nil | ||
|
|
||
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 are no tests for the items added in this PR's changes. Should add assertions for
IPv4Address,MacAddress, andIPv6Address.