Add host.containers.internal entry into container's etc/hosts #9972

bblenard · 2021-04-08T01:10:04Z

This change adds the entry host.containers.internal to the /etc/hosts
file within a new containers filesystem. The ip address is determined by
the containers networking configuration and points to the gateway address
for the containers networking namespace.

Closes #5651

Signed-off-by: Baron Lenardson [email protected]

mheon · 2021-04-08T18:57:46Z

/approve

openshift-ci-robot · 2021-04-08T18:57:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bblenard, mheon

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~~ [mheon]

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

mheon · 2021-04-08T18:59:27Z

libpod/container_internal_linux.go

@@ -1381,23 +1401,12 @@ func (c *Container) makeBindMounts() error {
 			// other container. Unless we're not creating both of
 			// them.
 			var (
-				depCtr  *Container
-				nextCtr string
+				depCtr *Container


Can drop this block and make depCtr, err = c.getRootNetNsDepCtr() a :=

Will be a little cleaner

#9972 (comment)

mheon · 2021-04-08T19:00:01Z

libpod/container_internal_linux.go

@@ -1342,6 +1342,26 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti
 	return c.save()
 }

+// Retrieves a container's "root" net namespace container dependency.


Can you note that this returns nil if there is no dependency?

I don't think this can be null unless there is an error, although I could be wrong. So would you like a note about it returning an nil on error? That being said I am adding some extra error handling in that function based on your comment, but it wont make sense if State.Container interface function can return nil, nil.

mheon · 2021-04-08T19:02:01Z

libpod/networking_linux.go

@@ -36,7 +36,7 @@ import (
 	"golang.org/x/sys/unix"
 )

-const (
+var (


Can you keep this block const and then add a separate var block below for the slirp4netns bit?

I have changed this I just don't want to kick off another CICD run until I get the other things sorted out.

mheon · 2021-04-08T19:04:45Z

I'd like @AkihiroSuda or someone else familiar with slirp to verify the slirp address calculation bits. Overall looks good, few small comments.

TomSweeneyRedHat · 2021-04-08T23:55:45Z

Nothing over @mheon's comments. @giuseppe if you have a spare eyeball for this, that would be great.

AkihiroSuda · 2021-04-09T04:37:54Z

test/system/500-networking.bats

+    run_podman run --network slirp4netns:cidr=192.168.0.0/24,allow_host_loopback=true \
+                $IMAGE grep 192.168.0.2 /etc/hosts
+    is "$output"   "192.168.0.2 host.gateway.internal"   "host.gateway.internal should be the cidr+2 address"
+}


I'm not feeling this behavior is correct.
The CIDR+2 address isn't useful when slirp4netns is launched with --disable-host-loopback.
And --disable-host-loopback should not be removed, as exposing bare 127.0.0.1 to containers might result in container-breakout.

I suggest exposing the "real" host IP regardless to rootful/rootless.

So what would be the alternative? Are you suggesting to derive all interface ip addresses not including lo?

The issue (#5651) was to add behavior that reflects docker's host.docker.internal.

I had a look through some of the docker code and as far as I can tell that is a configuration option given to the daemon so if a user asks for the host gateway address to be added to their container it is added based on that configuration docker-ce reference

So should podman have a similar configuration option?

Are you suggesting to derive all interface ip addresses not including lo?

The first interface (eth0) would be fine

I'm not sure about this solution.

There can be so many different interfaces in so many different configurations that trying to derive an ip address from one of the interfaces seems unstable. I don't necessarily disagree that exposing the loopback interface to the container could result in a container breakout but this change doesn't give that access it simply puts a /etc/hosts entry for that interface.

Like I said above. I think the only viable alternative is to make the interface a configurable option similar to dockerd's --host-gateway-ip launch option.

@AkihiroSuda bump in case you didn't see my last response

Having an option SGTM

rhatdan · 2021-04-16T08:30:58Z

libpod/container_internal_linux.go

+		depCtr, _ = c.getRootNetNsDepCtr()
+	} else if len(c.state.NetworkStatus) != 0 {
+		depCtr = c
+	} else {


This last block is not necessary. depCtr will be nil by default.

I'm not sure syntactically how to get rid of that block 😬

I added it because I wanted to ensure mutual exclusion between shared container network namespace configurations and containers with network namespaces created for them. Basically I didn't want the situation where

var depCtr *Container if c.config.NetNsCtr != "" { // **TRUE** // ignoring the error because there isn't anything to do depCtr, _ = c.getRootNetNsDepCtr() } if len(c.state.NetworkStatus) != 0 { // **TRUE** depCtr = c }

But If that is not possible / something to worry about I can make them both if's and remove the unnecessary block

@rhatdan just in case you didn't see my response.

Yes, that is fine.

rhatdan · 2021-04-16T08:33:48Z

libpod/container_internal_linux.go

+	} else if c.config.NetMode.IsSlirp4netns() {
+		hosts += fmt.Sprintf("%s host.gateway.internal\n", slirp4netnsGateway)
+	} else {
+		logrus.Debug("unexpected network configuration while determining host.gateway.internal address")


Is this unexpected, or would this just be the situation if I run a container with podman run --net=none ...

IE Is this worth logging?

This would be the case for podman run --net=none so I suppose it isn't that unexpected. Should I just quietly continue or change up the debug message?

@rhatdan bump just in case you didn't see my response

rhatdan · 2021-04-16T08:35:25Z

@bblenard Thanks for working on this.

TomSweeneyRedHat · 2021-04-26T23:58:05Z

@rhatdan can you merge this if it LGTY? I think there was one last "is this OK" type of question for you in the review comments on this one.

AkihiroSuda · 2021-04-27T06:02:31Z

Bikeshedding: For consistency with host.docker.internal, the corresponding value for Podman should be host.podman.internal or host.containers.internal rather than host.gateway.internal?

rhatdan · 2021-04-27T13:57:10Z

Yes lets go with host.containers.internal and then we can merge.

bblenard · 2021-04-28T00:52:42Z

I just pushed up some code and assuming the tests all pass there are just a few small things I want to make sure everyone is on the same page about:

this thread. @AkihiroSuda mentioned their concerns about using the slirpns gateway address, we discussed a bit and contemplated using a configuration option to specify an ip address for the host.containers.internal address. @rhatdan / @mheon / @TomSweeneyRedHat thoughts? If that is the route we want to go I'll have to rework most of this code :)
@rhatdan I wasn't sure what "Yes, that is fine." meant exactly so I left the code as it was. I can change it if I mis-interpreted your response though.

AkihiroSuda · 2021-04-28T02:53:47Z

The commit message ( host.gateway.internal ) seems to need to be updated

bblenard · 2021-05-04T23:26:19Z

The commit message ( host.gateway.internal ) seems to need to be updated

Fixed f5e7b40d66646b3e6517d989d8078110172808de

rhatdan · 2021-05-05T14:27:09Z

LGTM

rhatdan · 2021-05-05T18:34:49Z

@containers/podman-maintainers PTAL

Luap99 · 2021-05-05T18:45:49Z

libpod/networking_slirp4netns.go

+			return errors.Wrapf(err, "error calculating expected dns ip for slirp4netns")
+		}
+		// Save off calculated address into globals. I don't like this either
+		slirp4netnsIP = expectedIP.String()


I don't think we should use globals, this will fail when used with the podman service.

Okay, I'll look at moving away from the globals. I think when I initially wrote this those were constant values that I changed to variables so I didn't even think about if they should be globals. Thanks for catching that :)

I think you can add a new field (maybe called slirp4netnsSubnet) to the container struct here

podman/libpod/container.go

Line 97 in 663ea96

type Container struct {

and add methods like GetSlirp4netnsIP, GetSlirp4netnsGateway and GetSlirp4netnsDNS which returns the correct ip calculated from the stored subnet.

Fixed: c0335e77819e23be94bb55d883e18bdd6b742195

I made the helper functions generatl libpod functions that take subnets instead of Container functions that reference internal struct members because I didn't think it was a container specific thing. I also essentially reverted your changes here: f99b7a3 since I had a merge conflict and my changes covered what you did with that commit as far as I could tell.

@Luap99 I'll let you resolve these conversations if you think I properly addressed your comments.

Luap99 · 2021-05-05T18:46:52Z

libpod/networking_slirp4netns.go

@@ -314,9 +314,53 @@ func (r *Runtime) setupSlirp4netns(ctr *Container) error {
 		}
 		return r.setupRootlessPortMappingViaRLK(ctr, netnsPath)
 	}
+
+	// Calculate Slirp addresses now that we are pretty sure the command executed


This block has to be above if havePortMapping...

I'll fix that once I figure out how to move away from globals like you suggested above.

Fixed: c0335e77819e23be94bb55d883e18bdd6b742195

Luap99

Thanks this looks good. One small nit.

Luap99 · 2021-05-12T19:59:01Z

libpod/networking_linux.go

+				if err != nil {
+					return nil, errors.Wrap(err, "failed to determine default slirp4netns DNS address")
+				}
+


The block below here should also use GetSlirp4netnsDNS for consistency.

Fixed: ef6027b2db9d7ebd4a294af002c08c0c93d0e5db

rhatdan · 2021-05-12T20:04:59Z

libpod/container_internal_linux.go

-			hosts += fmt.Sprintf("# used by slirp4netns\n%s\t%s %s\n", slirp4netnsIP, c.Hostname(), c.config.Name)
+			slirp4netnsIP, err := GetSlirp4netnsGateway(c.slirp4netnsSubnet)
+			if err != nil {
+				logrus.Error("failed to determine slirp4netnsIP: ", err.Error())


This should be a warning not an error, If the user can not do anything about it, then it should just warn.

Fixed: ef6027b2db9d7ebd4a294af002c08c0c93d0e5db

Luap99 · 2021-05-12T20:05:34Z

@edsantiago PTAL at the system tests.

rhatdan · 2021-05-12T20:05:38Z

libpod/container_internal_linux.go

-			nameservers = append([]string{slirp4netnsDNS}, nameservers...)
+			slirp4netnsDNS, err := GetSlirp4netnsDNS(c.slirp4netnsSubnet)
+			if err != nil {
+				logrus.Error("failed to determine Slirp4netns DNS: ", err.Error())


Warning not Error.

Fixed: ef6027b2db9d7ebd4a294af002c08c0c93d0e5db

rhatdan · 2021-05-12T20:06:01Z

libpod/container_internal_linux.go

+	} else if c.config.NetMode.IsSlirp4netns() {
+		gatewayIP, err := GetSlirp4netnsGateway(c.slirp4netnsSubnet)
+		if err != nil {
+			logrus.Error("failed to determine gatewayIP: ", err.Error())


Warning not error.

Fixed: ef6027b2db9d7ebd4a294af002c08c0c93d0e5db

rhatdan · 2021-05-12T20:06:58Z

LGTM once Error->Warnin

edsantiago · 2021-05-12T20:14:12Z

test/system/500-networking.bats

@@ -162,6 +162,24 @@ load helpers
    done
 }

+@test "podman run with slirp4ns assigns correct gateway address to host.containers.internal" {
+    run_podman run --network slirp4netns:cidr=192.168.0.0/24 \


Just curious: what problem does random_rfc1918_subnet() have that prevents you from using it?

I wasn't aware of that function. I added it to the tests I wrote 👍

Thanks! One suggestion, though: I find string concatenation easier to read than stripping. Would you consider:

subnet=$(random_rfc1918_subnet) ... :cidr="$subnet.0/24" .... grep "$subnet" ... nameserger "${subnet}.3"

That removes the need for all those complicated fragile %.*, and makes life much easier for future maintainers.

Sounds good. Fixed: c8dfcce

Luap99

LGTM

openshift-ci · 2021-05-17T09:11:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bblenard, Luap99, mheon

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~~ [Luap99,mheon]

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

This change adds the entry `host.containers.internal` to the `/etc/hosts` file within a new containers filesystem. The ip address is determined by the containers networking configuration and points to the gateway address for the containers networking namespace. Closes containers#5651 Signed-off-by: Baron Lenardson <[email protected]>

edsantiago · 2021-05-17T13:41:30Z

test/system/500-networking.bats

@@ -162,6 +162,27 @@ load helpers
    done
 }

+@test "podman run with slirp4ns assigns correct gateway address to host.containers.internal" {
+    CIDR="$(random_rfc1918_subnet)"


Much better, thank you, but I'm kind of uncomfortable now with the variable name: a CIDR is defined as a full subnet, slash, and mask. A future maintainer may find this a stumbling point, and waste time (e.g. on line 169) wondering how a CIDR can have .2 appended. This is about a 3-out-of-10 gripe, I'm not going to insist on you changing it, but I'd like to bring it to the attention of others in case someone else on the team has a strong opinion either way.

rhatdan · 2021-05-17T14:44:00Z

/lgtm

rhatdan · 2021-05-17T14:44:18Z

Thanks @bblenard Nice work.

bblenard · 2021-05-17T14:53:20Z

Thanks @bblenard Nice work.

Thanks! I've been enjoying working on podman :)

bblenard force-pushed the issue-5651-hostname-for-container-gateway branch from 03073cf to 7747541 Compare April 8, 2021 01:52

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021

mheon reviewed Apr 8, 2021

View reviewed changes

AkihiroSuda suggested changes Apr 9, 2021

View reviewed changes

rhatdan reviewed Apr 16, 2021

View reviewed changes

bblenard force-pushed the issue-5651-hostname-for-container-gateway branch from 7747541 to 724078f Compare April 28, 2021 00:21

bblenard force-pushed the issue-5651-hostname-for-container-gateway branch 2 times, most recently from 971e347 to f5e7b40 Compare May 4, 2021 23:25

rhatdan changed the title ~~Add host.gateway.internal entry into container's etc/hosts~~ Add host.containers.internal entry into container's etc/hosts May 5, 2021

Luap99 reviewed May 5, 2021

View reviewed changes

mheon mentioned this pull request May 6, 2021

Support '--add-host=host.docker.internal:host-gateway' #8466

Closed

bblenard mentioned this pull request May 11, 2021

hostname for container gateway #5651

Closed

bblenard force-pushed the issue-5651-hostname-for-container-gateway branch from f5e7b40 to 3ce696e Compare May 12, 2021 15:26

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

bblenard force-pushed the issue-5651-hostname-for-container-gateway branch from 3ce696e to c0335e7 Compare May 12, 2021 16:14

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

Luap99 reviewed May 12, 2021

View reviewed changes

rhatdan reviewed May 12, 2021

View reviewed changes

edsantiago reviewed May 12, 2021

View reviewed changes

bblenard force-pushed the issue-5651-hostname-for-container-gateway branch from c0335e7 to ef6027b Compare May 17, 2021 02:44

Luap99 approved these changes May 17, 2021

View reviewed changes

bblenard force-pushed the issue-5651-hostname-for-container-gateway branch from ef6027b to c8dfcce Compare May 17, 2021 13:21

edsantiago reviewed May 17, 2021

View reviewed changes

openshift-ci bot assigned rhatdan May 17, 2021

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

openshift-merge-robot merged commit 62a7d4b into containers:master May 17, 2021

jszwedko mentioned this pull request May 28, 2021

feat(new source): New fluent source vectordotdev/vector#7548

Merged

rugk mentioned this pull request Jul 8, 2021

Support Docker's host.docker.internal:host-gateway special host #10878

Closed

freundTech mentioned this pull request Jul 14, 2022

Add support for reaching containers using host networking on Podman traefik/traefik#9190

Merged

2 tasks

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

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

Add host.containers.internal entry into container's etc/hosts #9972

Add host.containers.internal entry into container's etc/hosts #9972

Conversation

bblenard commented Apr 8, 2021 • edited by rhatdan Loading

mheon commented Apr 8, 2021

openshift-ci-robot commented Apr 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bblenard Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Apr 8, 2021

TomSweeneyRedHat commented Apr 8, 2021

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 commented Apr 16, 2021

TomSweeneyRedHat commented Apr 26, 2021

AkihiroSuda commented Apr 27, 2021

rhatdan commented Apr 27, 2021

bblenard commented Apr 28, 2021

AkihiroSuda commented Apr 28, 2021

bblenard commented May 4, 2021

rhatdan commented May 5, 2021

rhatdan commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 May 7, 2021 • edited Loading

Choose a reason for hiding this comment

bblenard May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Luap99 May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Luap99 commented May 12, 2021

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 May 12, 2021

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

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 17, 2021

Choose a reason for hiding this comment

rhatdan commented May 17, 2021

rhatdan commented May 17, 2021

bblenard commented May 17, 2021

bblenard commented Apr 8, 2021 •

edited by rhatdan

Loading

bblenard Apr 9, 2021 •

edited

Loading

Luap99 May 7, 2021 •

edited

Loading

bblenard May 12, 2021 •

edited

Loading

Luap99 May 5, 2021 •

edited

Loading