Add support for host.containers.internal in the /etc/hosts #3586

rhatdan · 2021-10-12T19:48:04Z

Also switch default hostname from truncated ContainerID to Container
name.

It makes more sense that a user would know the name of the container
versus the generated id, so we should use this as a default.

Fixes: #3509

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci · 2021-10-12T19:48:06Z

[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-10-12T19:48:57Z

@nalind @ashley-cui @flouthoc @TomSweeneyRedHat @umohnani8 @vrothberg PTAL

tests/run.bats

Luap99

host.containers.internal has to point to the host ip and not the loopback ip.

rhatdan · 2021-10-13T18:13:28Z

run_linux.go

 	if hostname != "" {
 		hosts.Write([]byte(fmt.Sprintf("127.0.0.1   %s\n", hostname)))
 		hosts.Write([]byte(fmt.Sprintf("::1         %s\n", hostname)))
 	}
+
+	// getLocalIP returns the non loopback local IP of the host
+	getLocalIP := func() string {


@nalind @Luap99 do we have any better way then this to get the hosts ip?

A host can validly have multiple network addresses, so this can return at most one address, or even no address. What does docker build provide?

Docker runs with in a network namespace with its own IP.

Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?

# docker build /tmp/test/ Sending build context to Docker daemon 2.048kB Step 1/2 : from alpine latest: Pulling from library/alpine a0d0a0d46f8b: Pull complete Digest: sha256:e1c082e3d3c45cccac829840a25941e679c25d438cc8412c2fa221cf1a824e6a Status: Downloaded newer image for alpine:latest ---> 14119a10abf4 Step 2/2 : run cat /etc/hosts ---> Running in 0a834f95dd73 127.0.0.1 localhost ::1 localhost ip6-localhost ip6-loopback fe00::0 ip6-localnet ff00::0 ip6-mcastprefix ff02::1 ip6-allnodes ff02::2 ip6-allrouters 172.17.0.2 0a834f95dd73 Removing intermediate container 0a834f95dd73 ---> ccb58e146963 Successfully built ccb58e146963 # docker run alpine cat /etc/hosts 127.0.0.1 localhost ::1 localhost ip6-localhost ip6-loopback fe00::0 ip6-localnet ff00::0 ip6-mcastprefix ff02::1 ip6-allnodes ff02::2 ip6-allrouters 172.17.0.2 1adf4c1f38b5

Docker does not seem to have this internal hostname, although I thought we added this for compatibility.

Buildkit does:

#5 [2/3] RUN cat /etc/hosts #5 0.346 127.0.0.1 localhost buildkitsandbox #5 0.346 ::1 localhost ip6-localhost ip6-loopback

For docker you have to use --add-host=host.docker.internal:host-gateway AFAIK and they use the default docker0 bridge ip address.
I think this solution is the best for buildah. For podman we use the CNI or slirp4netns gateway but this is also difficult to get.

Luap99 · 2021-10-13T18:46:32Z

run_linux.go

+		}
+		return ""
+	}
+	hosts.Write([]byte(fmt.Sprintf("%s         %s\n", getLocalIP(), "host.containers.internal")))


this should only be done when getLocalIP() is not empty

TomSweeneyRedHat · 2021-10-14T00:21:01Z

All kinds of red test unhappiness @rhatdan

rhatdan · 2021-10-14T16:57:46Z

@TomSweeneyRedHat @nalind @Luap99 @vrothberg @giuseppe @ashley-cui @umohnani8 @flouthoc PTAL

Luap99

LGTM

nalind · 2021-10-14T17:06:30Z

In the current version, generateHosts() only writes entries for 127.0.0.1 and ::1 if hostname is set, which looks like an error if it's also trying to add b.Container as a name for those addresses.

Also switch default hostname from truncated ContainerID to Container name. It makes more sense that a user would know the name of the container versus the generated id, so we should use this as a default. Fixes: containers#3509 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

flouthoc · 2021-10-14T17:25:44Z

In the current version, generateHosts() only writes entries for 127.0.0.1 and ::1 if hostname is set, which looks like an error if it's also trying to add b.Container as a name for those addresses.

@nalind Shouldn't this look like

diff --git a/run_linux.go b/run_linux.go
index e0922c91..47e867fc 100644
--- a/run_linux.go
+++ b/run_linux.go
@@ -717,10 +717,9 @@ func (b *Builder) generateHosts(rdir, hostname string, addHosts []string, chownO
                hosts.Write([]byte(fmt.Sprintf("%s\t%s\n", values[1], values[0])))
        }
 
-       if hostname != "" {
-               hosts.Write([]byte(fmt.Sprintf("127.0.0.1   %s\n", hostname)))
-               hosts.Write([]byte(fmt.Sprintf("::1         %s\n", hostname)))
-       }
+       hosts.Write([]byte(fmt.Sprintf("127.0.0.1 localhost %s %s\n", b.Container, hostname)))
+       hosts.Write([]byte(fmt.Sprintf("::1       localhost %s %s\n", b.Container, hostname)))
+

flouthoc · 2021-10-14T17:31:48Z

LGTM

flouthoc · 2021-10-14T17:37:58Z

Just one doubt since we are removing check for if hostname != "" should we still have a line to set localhost if hostname is actually empty.

Since I am not sure why this check was there in the first place.

rhatdan · 2021-10-14T17:39:51Z

The check was there in case their was no entry. You would have had "::1 "
Now we are at least guaranteed to have container name.

rhatdan · 2021-10-14T17:40:04Z

/hold

TomSweeneyRedHat · 2021-10-14T17:41:45Z

run_linux.go

+			return ""
+		}
+		for _, address := range addrs {
+			// check the address type and if it is not a loopback the display it


nit

Suggested change

// check the address type and if it is not a loopback the display it

// check the address type and if it is not a loopback then return it

TomSweeneyRedHat · 2021-10-14T17:47:54Z

LGTM

flouthoc · 2021-10-14T17:49:00Z

The check was there in case their was no entry. You would have had "::1 " Now we are at least guaranteed to have container name.

Thanks got it.

rhatdan · 2021-10-14T23:08:16Z

/hold cancel

openshift-ci bot added the approved label Oct 12, 2021

nalind reviewed Oct 12, 2021

View reviewed changes

tests/run.bats Outdated Show resolved Hide resolved

rhatdan force-pushed the docs branch from 704e851 to 6171241 Compare October 12, 2021 20:26

Luap99 requested changes Oct 12, 2021

View reviewed changes

rhatdan force-pushed the docs branch from 6171241 to f843cca Compare October 13, 2021 18:12

rhatdan commented Oct 13, 2021

View reviewed changes

rhatdan force-pushed the docs branch from f843cca to b72a7a9 Compare October 13, 2021 18:20

Luap99 reviewed Oct 13, 2021

View reviewed changes

rhatdan force-pushed the docs branch from b72a7a9 to d2d4838 Compare October 13, 2021 19:01

rhatdan force-pushed the docs branch from d2d4838 to 1cb3414 Compare October 14, 2021 13:06

Luap99 reviewed Oct 14, 2021

View reviewed changes

rhatdan force-pushed the docs branch from 1cb3414 to f8c1526 Compare October 14, 2021 17:21

openshift-ci bot added the do-not-merge/hold label Oct 14, 2021

rhatdan added the lgtm label Oct 14, 2021

TomSweeneyRedHat reviewed Oct 14, 2021

View reviewed changes

openshift-ci bot removed the do-not-merge/hold label Oct 14, 2021

openshift-merge-robot merged commit e4a4f2c into containers:main Oct 14, 2021

github-actions bot added the locked - please file new issue/PR label Sep 15, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for host.containers.internal in the /etc/hosts #3586

Add support for host.containers.internal in the /etc/hosts #3586

rhatdan commented Oct 12, 2021

openshift-ci bot commented Oct 12, 2021

rhatdan commented Oct 12, 2021

Luap99 left a comment

rhatdan Oct 13, 2021

nalind Oct 13, 2021

rhatdan Oct 13, 2021

rhatdan Oct 13, 2021

Luap99 Oct 13, 2021

Luap99 Oct 13, 2021

TomSweeneyRedHat commented Oct 14, 2021

rhatdan commented Oct 14, 2021

Luap99 left a comment

nalind commented Oct 14, 2021

flouthoc commented Oct 14, 2021

flouthoc commented Oct 14, 2021

flouthoc commented Oct 14, 2021

rhatdan commented Oct 14, 2021

rhatdan commented Oct 14, 2021

TomSweeneyRedHat Oct 14, 2021

TomSweeneyRedHat commented Oct 14, 2021

flouthoc commented Oct 14, 2021

rhatdan commented Oct 14, 2021

	// check the address type and if it is not a loopback the display it
	// check the address type and if it is not a loopback then return it

Add support for host.containers.internal in the /etc/hosts #3586

Add support for host.containers.internal in the /etc/hosts #3586

Conversation

rhatdan commented Oct 12, 2021

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci bot commented Oct 12, 2021

rhatdan commented Oct 12, 2021

Luap99 left a comment

Choose a reason for hiding this comment

rhatdan Oct 13, 2021

Choose a reason for hiding this comment

nalind Oct 13, 2021

Choose a reason for hiding this comment

rhatdan Oct 13, 2021

Choose a reason for hiding this comment

rhatdan Oct 13, 2021

Choose a reason for hiding this comment

Luap99 Oct 13, 2021

Choose a reason for hiding this comment

Luap99 Oct 13, 2021

Choose a reason for hiding this comment

TomSweeneyRedHat commented Oct 14, 2021

rhatdan commented Oct 14, 2021

Luap99 left a comment

Choose a reason for hiding this comment

nalind commented Oct 14, 2021

flouthoc commented Oct 14, 2021

flouthoc commented Oct 14, 2021

flouthoc commented Oct 14, 2021

rhatdan commented Oct 14, 2021

rhatdan commented Oct 14, 2021

TomSweeneyRedHat Oct 14, 2021

Choose a reason for hiding this comment

TomSweeneyRedHat commented Oct 14, 2021

flouthoc commented Oct 14, 2021

rhatdan commented Oct 14, 2021