Use hosts public ip address in rootless containers #12375

rhatdan · 2021-11-20T14:10:36Z

Add first non localhost ipv4 of all host interfaces as destination
for host.contaners.internal for rootless containers.

Fixes: #12000

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

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

openshift-ci · 2021-11-20T14:10:38Z

[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 · 2021-11-20T14:10:55Z

@mheon @Luap99 Is this what you intended?

Luap99 · 2021-11-21T12:28:25Z

libpod/container_internal_linux.go

-			} else {
-				hosts += fmt.Sprintf("%s host.containers.internal\n", gatewayIP.String())
+			// getLocalIP returns the non loopback local IP of the host
+			getLocalIP := func() string {


Why is this a local function?

Well it was local in Buildah, I will make it Public in Buildah and then we can use it in both places, but this will take a while.

Luap99 · 2021-11-21T12:31:35Z

libpod/container_internal_linux.go

+				}
+				return ""
+			}
+			if ip := getLocalIP(); ip != "" {


Can you fallback to GetSlirp4netnsGateway() if the ip is empty.

test/system/500-networking.bats

mheon · 2021-11-21T16:52:41Z

LGTM once comments from @Luap99 are addressed

rhatdan · 2021-11-24T00:49:53Z

@edsantiago I will need you to look at the Buildah bud failures.

edsantiago · 2021-11-24T01:01:01Z

The first one: podman build --unsetenv is a NOP (probably need to pass the flag along to buildah).

The second one, hmmm, I'm really confused by. I don't think I'll have time to get deep into it tonight, but basically, there's a buildah test that expects a 128 exit status, and I honestly kind of think that this is a bug in buildah itself: I think this failure should be 125, not 128. I don't know how to fix it for purposes of this PR. Maybe a skip?
https://github.com/containers/buildah/blob/8b79d8b0778fff2db13eb84aacc4d31c22a56872/tests/bud.bats#L778

The third one, no idea, something is expecting manifests to be >1 but it's ==1. I can try to take a closer look later tonight, but more likely tomorrow, sorry.

edsantiago · 2021-11-24T01:38:59Z

Is --all-platforms being passed properly to buildah?

edsantiago · 2021-11-24T02:41:28Z

Yeah... I think the first and third are that --unsetenv and --all-platforms aren't being passed to buildah. And the second, I really think is a bug in buildah or even git. 128 is just not a normal exit status. It makes no sense.

rhatdan · 2021-11-24T12:06:00Z

Not sure where it comes from, but buildah does exit with a 128.

$ buildah bud git:///tmp/no-such-repository
error prepping temporary context directory: exit status 128
ERRO[0000] exit status 128

rhatdan · 2021-11-24T12:08:25Z

The exit code comes from git, which buildah is executing.

$ git clone git:///tmp/no-such-repository
Cloning into 'no-such-repository'...
fatal: unable to look up  (port 9418) (Name or service not known)
buildah (group) $ echo $?
128

edsantiago · 2021-11-24T12:08:46Z

It comes from git. I believe that's a bug in git, but I don't care about git, I care about buildah. I believe that buildah should exit with one of its standard error exit codes.

rhatdan · 2021-11-24T12:12:57Z

$ buildah build  /tmp/no-such-repository
discovering Containerfile: stat /tmp/no-such-repository: no such file or directory
ERRO[0000] exit status 125

It should probably be equivalent. But not sure why 128 shouldn't work correctly in a test.

edsantiago · 2021-11-24T12:17:39Z

UNIX exit statuses are 0-127. When bit 7 is set, it means there was a signal, e.g., 130 = 128 | 2 = SIGINT. So exit status 128 is what: SIGNOTHING??

Currently we are only wiring the logger into run_linux.go Not into the Config section. This PR is needed in order to update vendor in Podman. containers/podman#12375 [NO NEW TESTS NEEDED] Tests will be done in Podman. Signed-off-by: Daniel J Walsh <[email protected]>

nalind · 2021-12-03T15:33:22Z

pkg/api/handlers/compat/images_build.go

+		}
+		if arch == "" {
+			arch = runtime.GOARCH
+		}


This overrides values computed from (possibly multiple) Platform values with the single values from arch and os, which looks wrong. They should only be used in place of the values returned by github.com/containerd/containerd/platforms.DefaultSpec() if no Platform values are present, otherwise they should be ignored.

I don't recall, why I added those, they do not look correct from reading parse. I will remove and see if errors return.

Add first non localhost ipv4 of all host interfaces as destination for host.contaners.internal for rootless containers. Fixes: containers#12000 Signed-off-by: Daniel J Walsh <[email protected]>

mheon · 2021-12-22T15:38:50Z

/lgtm

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2021

Luap99 reviewed Nov 21, 2021

View reviewed changes

rhatdan mentioned this pull request Nov 22, 2021

Make LocalIP public function so Podman can use it containers/buildah#3645

Merged

rhatdan force-pushed the hosts branch from c5c07e9 to 63f0be0 Compare November 22, 2021 15:05

rhatdan changed the title ~~Use hosts public ip address in rootless containers~~ [WIP] Use hosts public ip address in rootless containers Nov 22, 2021

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2021

edsantiago mentioned this pull request Nov 22, 2021

[remote] error creating cgroup [...]: dial unix /run/systemd/private: connect: EPERM #12362

Closed

rhatdan force-pushed the hosts branch from 63f0be0 to 1d08a58 Compare November 23, 2021 14:55

rhatdan changed the title ~~[WIP] Use hosts public ip address in rootless containers~~ Use hosts public ip address in rootless containers Nov 23, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2021

rhatdan force-pushed the hosts branch from 1d08a58 to 2242250 Compare November 23, 2021 18:26

rhatdan force-pushed the hosts branch from 2242250 to 6b0dbc8 Compare November 24, 2021 11:57

rhatdan force-pushed the hosts branch from 6b0dbc8 to 4db68e5 Compare November 30, 2021 17:26

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021

rhatdan force-pushed the hosts branch from 4db68e5 to 054a255 Compare December 1, 2021 20:16

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021

rhatdan force-pushed the hosts branch from 054a255 to 2e972c8 Compare December 2, 2021 19:42

rhatdan mentioned this pull request Dec 3, 2021

Wire logger through to config containers/buildah#3663

Merged

rhatdan force-pushed the hosts branch from 2e972c8 to 599cddf Compare December 3, 2021 14:35

rhatdan changed the title ~~Use hosts public ip address in rootless containers~~ [WIP] Use hosts public ip address in rootless containers Dec 3, 2021

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2021

nalind reviewed Dec 3, 2021

View reviewed changes

rhatdan force-pushed the hosts branch 2 times, most recently from f2ee714 to 45da1ce Compare December 6, 2021 11:54

rhatdan changed the title ~~[WIP] Use hosts public ip address in rootless containers~~ Use hosts public ip address in rootless containers Dec 6, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2021

rhatdan force-pushed the hosts branch from 45da1ce to c02b87a Compare December 6, 2021 17:23

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2021

rhatdan force-pushed the hosts branch from c02b87a to 015986a Compare December 8, 2021 12:20

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2021

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2021

Use hosts public ip address in rootless containers

64ce694

Add first non localhost ipv4 of all host interfaces as destination for host.contaners.internal for rootless containers. Fixes: containers#12000 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the hosts branch from 015986a to 64ce694 Compare December 21, 2021 13:50

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2021

openshift-ci bot assigned mheon Dec 22, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2021

openshift-merge-robot merged commit 2aea0a5 into containers:main Dec 22, 2021

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 22, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use hosts public ip address in rootless containers #12375

Use hosts public ip address in rootless containers #12375

rhatdan commented Nov 20, 2021

openshift-ci bot commented Nov 20, 2021

rhatdan commented Nov 20, 2021

Luap99 Nov 21, 2021

rhatdan Nov 22, 2021

Luap99 Nov 21, 2021

mheon commented Nov 21, 2021

rhatdan commented Nov 24, 2021

edsantiago commented Nov 24, 2021

edsantiago commented Nov 24, 2021

edsantiago commented Nov 24, 2021

rhatdan commented Nov 24, 2021

rhatdan commented Nov 24, 2021

edsantiago commented Nov 24, 2021

rhatdan commented Nov 24, 2021

edsantiago commented Nov 24, 2021

nalind Dec 3, 2021

rhatdan Dec 3, 2021

mheon commented Dec 22, 2021

Use hosts public ip address in rootless containers #12375

Use hosts public ip address in rootless containers #12375

Conversation

rhatdan commented Nov 20, 2021

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

openshift-ci bot commented Nov 20, 2021

rhatdan commented Nov 20, 2021

Luap99 Nov 21, 2021

Choose a reason for hiding this comment

rhatdan Nov 22, 2021

Choose a reason for hiding this comment

Luap99 Nov 21, 2021

Choose a reason for hiding this comment

mheon commented Nov 21, 2021

rhatdan commented Nov 24, 2021

edsantiago commented Nov 24, 2021

edsantiago commented Nov 24, 2021

edsantiago commented Nov 24, 2021

rhatdan commented Nov 24, 2021

rhatdan commented Nov 24, 2021

edsantiago commented Nov 24, 2021

rhatdan commented Nov 24, 2021

edsantiago commented Nov 24, 2021

nalind Dec 3, 2021

Choose a reason for hiding this comment

rhatdan Dec 3, 2021

Choose a reason for hiding this comment

mheon commented Dec 22, 2021