podman inspect list network when using --net=host or none #17386

rhatdan · 2023-02-06T21:11:42Z

This will match Docker behaviour.

Signed-off-by: Daniel J Walsh [email protected]

Does this PR introduce a user-facing change?

podman inspect lists networks on all network modes.

openshift-ci · 2023-02-06T21:11:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2023-02-06T21:11:55Z

@Luap99 PTAL

Luap99

I think this is the right approach just need to take a close look at special cases.

Also reference to #16092 which is the same just for network list.

Luap99 · 2023-02-07T14:31:12Z

libpod/networking_common.go

@@ -262,6 +262,12 @@ func (c *Container) getContainerNetworkInfo() (*define.InspectNetworkSettings, e
 				cniNet.Aliases = opts.Aliases
 				settings.Networks[net] = cniNet
 			}
+		} else {
+			settings.Networks = make(map[string]*define.InspectAdditionalNetwork, 1)
+			name := c.NetworkMode()


Problem is that will not only return host and none. This can also result in slirp4netns which should be fine but ns:/some/path and container:ID might not be right. I don't know what docker does in this case, we should check this first.

# cid=$(./bin/podman create alpine echo hi) # ./bin/podman create --network=container:$cid alpine echo hi1 1a66aeb6e0bd6814c7a7dff9ef90847de88d85ccd4109d3e5ef2795eaffde82d # ./bin/podman inspect --format '{{ .NetworkSettings.Networks }}' -l map[podman:0xc0007059e0]

Versus

# docker create alpine echo hi 45410ef61a0d02a2433eab8f48beb2638be457fd92d3145c86829194471542f5 # docker create --network container:45410e alpine echo hi1 81820ea9f50c0894d39e48d9ca7d538cd584491ebcde8cd7af21b21ca5b751c8 # docker inspect --format '{{ .NetworkSettings.Networks }}' 81820ea9f map[]

But this is not changed by my code.

yes it is, this is not what we showed before and not what docker does.
container: and ns: should not populate the map!

Docker populates NS:
If I join a container to another containers network, why shouldn't the second container show the network of the first container?

Luap99 · 2023-02-07T14:31:48Z

test/system/500-networking.bats

+@test "podman inspect list networks " {
+    for network in "host" "none"; do
+        run_podman create --network=$network $IMAGE
+	cid=${output}
+	run_podman inspect --format '{{ .NetworkSettings.Networks }}' $cid
+	is "$output" "map\[$network:.*" "NeworkSettincs should container one network named $network"
+	run_podman rm $cid
+    done
+    run_podman create $IMAGE
+    cid=${output}
+    run_podman inspect --format '{{ .NetworkSettings.Networks }}' $cid
+    if is_rootless; then
+	is "$output" "map\[slirp4netns:.*" "NeworkSettings should container one network named slirp4netns"
+    else
+	is "$output" "map\[podman:.*" "NeworkSettings should container one network named podman"
+    fi
+    run_podman rm $cid
+}


indentation looks wrong

Not sure what you mean?

look at the change in github, the indentation is off

I don't see it.

This does not look right, I assume mixed tabs vs spaces. Ideally we would use a common formatter for these files they are always a mess to work with IMO cc @edsantiago

Luap99 · 2023-02-16T14:39:15Z

test/system/500-networking.bats

+    # Check with ns:/PATH
+    if ! is_rootless; then
+	netns=netns$(random_string)
+	ip netns add $netns
+	run_podman create --network=ns:/var/run/netns/$netns $IMAGE
+	cid=${output}
+	run_podman inspect --format '{{ .NetworkSettings.Networks }}' $cid
+	is "$output" "map\[ns:/var/run/netns/$netns.*" "NeworkSettings should contain path to netns"
+    fi
+    run_podman rm $cid


again this is not correct both container: and ns: should not be added to the map, they are not a network!
If we want to match docker we should do so correctly.

With this change podman and docker return the same content.

# ip netns add dan # podman create --network=ns:/var/run/netns/dan alpine e0ae397eb613e6baf5994c35b6e15ff15768e6c7592a2e2cb63e1f8eff8f9a3e # docker create --network=ns:/var/run/netns/dan alpine d064be722af294c40631ec3ed1a07128af2805a8649b1f62282b966b094c051f # ./bin/podman inspect --format '{{ .NetworkSettings.Networks }}' e0ae397eb613e6baf5994c35b6e15ff15768e6c7592a2e2cb63e1f8eff8f9a3e map[ns:/var/run/netns/dan:0xc00089e000] # docker inspect --format '{{ .NetworkSettings.Networks }}' d064be722af294c40631ec3ed1a07128af2805a8649b1f62282b966b094c051f map[ns:/var/run/netns/dan:0xc00036c000]

What docker version are you using. I don't see that? Are you sure your docker is not linked to podman?

AFAIK docker does not even support the ns: syntax only the contianer: one, at least on my version 20.10.23

# docker create --network=ns:/var/run/netns/dan alpine 67ba122e06560ada92dc8fffa7cd0686af46500c11e424908c6c68003ea2236a # docker inspect --format '{{ .NetworkSettings.Networks }}' 67ba122e06560ada92dc8fffa7cd0686af46500c11e424908c6c68003ea2236a map[ns:/var/run/netns/dan:0xc0004b7980] # docker -v Docker version 20.10.22, build 3a2c30b

20.10.23 is not available in docker-ce, as far as I see.

docker -v is showing you the client information, try docker version/info.

docker version Client: Docker Engine - Community Version: 20.10.22 API version: 1.41 Go version: go1.18.9 Git commit: 3a2c30b Built: Thu Dec 15 22:28:45 2022 OS/Arch: linux/amd64 Context: default Experimental: true Server: Docker Engine - Community Engine: Version: 20.10.22 API version: 1.41 (minimum version 1.12) Go version: go1.18.9 Git commit: 42c8b31 Built: Thu Dec 15 22:26:25 2022 OS/Arch: linux/amd64 Experimental: false containerd: Version: 1.6.15 GitCommit: 5b842e528e99d4d4c1686467debf2bd4b88ecd86 runc: Version: 1.1.4 GitCommit: v1.1.4-0-g5fd4c4d docker-init: Version: 0.19.0 GitCommit: de40ad0

I am not sure why you want to hide this information from the users. Seems like a good thing for users to be able to see which networks their container is attached to.

It is not hidden, all info is already in the basic struct.
The map you are adding uses the network name as key. Neither ns: nor contianer: is considered a network, they are a network mode.

I am confused because https://docs.docker.com/engine/reference/run/#network-settings does not mention the ns: key and AFAIK docker does not support it.

rhatdan · 2023-03-08T18:05:42Z

@Luap99 PTAL

Luap99 · 2023-03-08T18:18:13Z

test/e2e/run_networking_test.go

@@ -876,7 +876,7 @@ EXPOSE 2004-2005/tcp`, ALPINE)

 		inspectOut := podmanTest.InspectContainer(name)
 		Expect(inspectOut[0].NetworkSettings).To(HaveField("IPAddress", ""))
-		Expect(inspectOut[0].NetworkSettings.Networks).To(BeEmpty())
+		Expect(fmt.Sprintf("%v", inspectOut[0].NetworkSettings.Networks)).To(ContainSubstring("ns:/run/netns/" + networkNSName))


shouldn't this still check for empty because ns:... should not show up in the map

I am bit confused because your system test checks for the empty map and both pass? How is this possible?

Luap99 · 2023-03-08T18:18:20Z

test/system/500-networking.bats

+    # Check with ns:/PATH
+    if ! is_rootless; then
+        netns=netns$(random_string)
+        ip netns add $netns


This should also be removed after the test.

Luap99 · 2023-03-08T18:18:46Z

test/apiv2/20-containers.at

@@ -51,7 +51,7 @@ t GET libpod/containers/json?all=true 200 \
  .[0].IsInfra=false

 # Test compat API for Network Settings (.Network is N/A when rootless)


need to change the comment to reflect the change

This will match Docker behaviour. Fixes: containers#17385 Signed-off-by: Daniel J Walsh <[email protected]>

Luap99 · 2023-03-09T13:12:36Z

restarted flakes
/lgtm
/hold

rhatdan · 2023-03-09T19:02:01Z

/hold cancel

openshift-ci bot added the release-note label Feb 6, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2023

Luap99 reviewed Feb 7, 2023

View reviewed changes

rhatdan force-pushed the network branch from 39d3bd8 to e8f792f Compare February 15, 2023 04:54

edsantiago mentioned this pull request Feb 15, 2023

system tests: prevent leading tabs #17518

Merged

rhatdan force-pushed the network branch 2 times, most recently from fdcb893 to 2d41ae1 Compare February 16, 2023 05:04

Luap99 reviewed Feb 16, 2023

View reviewed changes

rhatdan force-pushed the network branch 3 times, most recently from 5e88bbe to 3a32790 Compare February 25, 2023 11:33

rhatdan force-pushed the network branch from 3a32790 to 1df7ce0 Compare March 8, 2023 15:33

Luap99 reviewed Mar 8, 2023

View reviewed changes

podman inspect list network when using --net=host or none

2165170

This will match Docker behaviour. Fixes: containers#17385 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the network branch from 1df7ce0 to 2165170 Compare March 8, 2023 22:27

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2023

openshift-ci bot assigned Luap99 Mar 9, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2023

openshift-merge-robot merged commit 615d80e into containers:main Mar 9, 2023

edsantiago mentioned this pull request Mar 29, 2023

build: COPY hardlinks #17979

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

podman inspect list network when using --net=host or none #17386

podman inspect list network when using --net=host or none #17386

rhatdan commented Feb 6, 2023

openshift-ci bot commented Feb 6, 2023

rhatdan commented Feb 6, 2023

Luap99 left a comment

Luap99 Feb 7, 2023

rhatdan Feb 15, 2023

rhatdan Feb 15, 2023

Luap99 Feb 15, 2023

rhatdan Feb 25, 2023

Luap99 Feb 7, 2023

rhatdan Feb 15, 2023

Luap99 Feb 15, 2023

rhatdan Feb 15, 2023

Luap99 Feb 15, 2023

Luap99 Feb 16, 2023

rhatdan Feb 24, 2023

Luap99 Feb 27, 2023

rhatdan Feb 27, 2023 •

edited

Loading

rhatdan Feb 27, 2023

Luap99 Feb 27, 2023

rhatdan Feb 27, 2023

rhatdan Feb 27, 2023

Luap99 Feb 27, 2023

Luap99 Feb 27, 2023

rhatdan commented Mar 8, 2023

Luap99 Mar 8, 2023

Luap99 Mar 8, 2023

Luap99 Mar 8, 2023

Luap99 commented Mar 9, 2023

rhatdan commented Mar 9, 2023

		@@ -51,7 +51,7 @@ t GET libpod/containers/json?all=true 200 \
		.[0].IsInfra=false

		# Test compat API for Network Settings (.Network is N/A when rootless)

podman inspect list network when using --net=host or none #17386

podman inspect list network when using --net=host or none #17386

Conversation

rhatdan commented Feb 6, 2023

Does this PR introduce a user-facing change?

openshift-ci bot commented Feb 6, 2023

rhatdan commented Feb 6, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented Mar 9, 2023

rhatdan commented Mar 9, 2023

rhatdan Feb 27, 2023 •

edited

Loading