Add podman play kube --no-hosts options #11707

rhatdan · 2021-09-22T22:35:48Z

This option will setup the containers to not modify their /etc/hosts
file and just use the one from the image.

Fixes: #9500

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

openshift-ci · 2021-09-22T22:35:52Z

[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

TomSweeneyRedHat · 2021-09-23T00:04:29Z

cmd/podman/play/kube.go

@@ -78,6 +78,7 @@ func init() {
 	flags.StringVar(&kubeOptions.LogDriver, logDriverFlagName, "", "Logging driver for the container")
 	_ = kubeCmd.RegisterFlagCompletionFunc(logDriverFlagName, common.AutocompleteLogDriver)

+	flags.BoolVar(&kubeOptions.NoHosts, "no-hosts", false, "Do not create /etc/hosts within the containers, instead use the version from the image")


I think...

Suggested change

flags.BoolVar(&kubeOptions.NoHosts, "no-hosts", false, "Do not create /etc/hosts within the containers, instead use the version from the image")

flags.BoolVar(&kubeOptions.NoHosts, "no-hosts", false, "Do not create /etc/hosts within the container, instead use the version from the image")

TomSweeneyRedHat · 2021-09-23T00:05:43Z

docs/source/markdown/podman-play-kube.1.md

@@ -138,6 +138,10 @@ Valid _mode_ values are:
  Note: Rootlesskit changes the source IP address of incoming packets to a IP address in the container network namespace, usually `10.0.2.100`. If your application requires the real source IP address, e.g. web server logs, use the slirp4netns port handler. The rootlesskit port handler is also used for rootless containers when connected to user-defined networks.
  - **port_handler=slirp4netns**: Use the slirp4netns port forwarding, it is slower than rootlesskit but preserves the correct source IP address. This port handler cannot be used for user-defined networks.

+#### **--no-hosts**
+
+Do not create /etc/hosts within the containers, instead use the version from the image


if you go singular above, go singular here too. Also add an ending period (.)

It should go to all containers in Pod

mheon · 2021-09-23T15:45:40Z

test/e2e/play_kube_test.go

@@ -1137,6 +1137,30 @@ var _ = Describe("Podman play kube", func() {
 		Expect(infraContainerImage).To(Equal(config.DefaultInfraImage))
 	})

+	It("podman play kube should use default infra_image", func() {


Is this actually a test of the default infra image? It looks like it's testing no-hosts only

No cut and paste, fixed.

rhatdan · 2021-09-24T10:24:25Z

@vrothberg @giuseppe @flouthoc @Luap99 PTAL
@containers/podman-maintainers @TomSweeneyRedHat PTAL

vrothberg

LGTM

flouthoc · 2021-09-24T11:23:02Z

@rhatdan If i am correct this will prevent inheriting /etc/hosts from host machine but would also silently ignore hostAliases from pod spec or would spit error. So users will only have a single method to populate /etc/hosts i.e from baked images.

I am not sure but i think use-case should support populating /etc/hosts from hostAliases and --no-hosts should only ignore inheriting from hostmachine.

Just to clarify this i have also commented on the original issue itself.

Apart from this point LGTM

mheon · 2021-09-24T15:26:53Z

Ignoring hostAliases would be most correct, but we should print a warning message (or outright error) on the two being set simultaneously (ideally, libpod would be able to detect this, as hosts are being added and noHosts is being set - this may actually be the case already.

mheon · 2021-09-24T15:30:10Z

@rhatdan Can we get a test with hostAliases set and --no-hosts

flouthoc · 2021-09-24T15:47:26Z

ah if it is expected behavior then i can see error is generated when both are set.

LGTM

cdoern · 2021-09-24T16:36:13Z

pkg/domain/infra/abi/play.go

@@ -182,6 +182,9 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY
 	}

 	podOpt := entities.PodCreateOptions{Infra: true, Net: &entities.NetOptions{StaticIP: &net.IP{}, StaticMAC: &net.HardwareAddr{}}}
+	podOpt.Net = &entities.NetOptions{


move this above into the line with eh already declared net options. If I recall correctly StaticIP and StaticMAC need to be initialized or a nil pointer deref can happen

Good catch.

rhatdan · 2021-09-24T20:14:28Z

@mheon I added thests.

mheon · 2021-09-24T20:41:04Z

LGTM

rhatdan · 2021-09-24T20:56:52Z

/hold

TomSweeneyRedHat · 2021-09-25T16:20:52Z

Tests aren't hip @rhatdan

openshift-ci · 2021-09-27T19:57:57Z

New changes are detected. LGTM label has been removed.

This option will setup the containers to not modify their /etc/hosts file and just use the one from the image. Fixes: containers#9500 Signed-off-by: Daniel J Walsh <[email protected]>

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

rhatdan force-pushed the play branch 2 times, most recently from 3ac7c05 to ec31b01 Compare September 22, 2021 22:41

TomSweeneyRedHat reviewed Sep 23, 2021

View reviewed changes

mheon reviewed Sep 23, 2021

View reviewed changes

rhatdan force-pushed the play branch 2 times, most recently from 795e72d to a3bb7b5 Compare September 23, 2021 18:19

vrothberg reviewed Sep 24, 2021

View reviewed changes

cdoern reviewed Sep 24, 2021

View reviewed changes

rhatdan force-pushed the play branch from a3bb7b5 to 4b1a082 Compare September 24, 2021 20:14

rhatdan added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2021

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

rhatdan force-pushed the play branch from 4b1a082 to 7bdbb4c Compare September 27, 2021 19:57

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021

rhatdan force-pushed the play branch from 7bdbb4c to 63a1852 Compare September 30, 2021 19:09

Add podman play kube --no-hosts options

641f0cc

This option will setup the containers to not modify their /etc/hosts file and just use the one from the image. Fixes: containers#9500 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the play branch from 63a1852 to 641f0cc Compare October 1, 2021 14:53

rhatdan removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2021

rhatdan added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2021

openshift-merge-robot merged commit 1b88b67 into containers:main Oct 1, 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

Add podman play kube --no-hosts options #11707

Add podman play kube --no-hosts options #11707

rhatdan commented Sep 22, 2021

openshift-ci bot commented Sep 22, 2021

TomSweeneyRedHat Sep 23, 2021

TomSweeneyRedHat Sep 23, 2021

rhatdan Sep 23, 2021

mheon Sep 23, 2021

rhatdan Sep 23, 2021

rhatdan commented Sep 24, 2021

vrothberg left a comment

flouthoc commented Sep 24, 2021

mheon commented Sep 24, 2021

mheon commented Sep 24, 2021

flouthoc commented Sep 24, 2021

cdoern Sep 24, 2021

rhatdan Sep 24, 2021

rhatdan commented Sep 24, 2021

mheon commented Sep 24, 2021

rhatdan commented Sep 24, 2021

TomSweeneyRedHat commented Sep 25, 2021

openshift-ci bot commented Sep 27, 2021

	flags.BoolVar(&kubeOptions.NoHosts, "no-hosts", false, "Do not create /etc/hosts within the containers, instead use the version from the image")
	flags.BoolVar(&kubeOptions.NoHosts, "no-hosts", false, "Do not create /etc/hosts within the container, instead use the version from the image")

Add podman play kube --no-hosts options #11707

Add podman play kube --no-hosts options #11707

Conversation

rhatdan commented Sep 22, 2021

openshift-ci bot commented Sep 22, 2021

TomSweeneyRedHat Sep 23, 2021

Choose a reason for hiding this comment

TomSweeneyRedHat Sep 23, 2021

Choose a reason for hiding this comment

rhatdan Sep 23, 2021

Choose a reason for hiding this comment

mheon Sep 23, 2021

Choose a reason for hiding this comment

rhatdan Sep 23, 2021

Choose a reason for hiding this comment

rhatdan commented Sep 24, 2021

vrothberg left a comment

Choose a reason for hiding this comment

flouthoc commented Sep 24, 2021

mheon commented Sep 24, 2021

mheon commented Sep 24, 2021

flouthoc commented Sep 24, 2021

cdoern Sep 24, 2021

Choose a reason for hiding this comment

rhatdan Sep 24, 2021

Choose a reason for hiding this comment

rhatdan commented Sep 24, 2021

mheon commented Sep 24, 2021

rhatdan commented Sep 24, 2021

TomSweeneyRedHat commented Sep 25, 2021

openshift-ci bot commented Sep 27, 2021