Add --no-hostname option to buildah containers #5094

rhatdan · 2023-10-20T15:45:43Z

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?

`buildah build --no-hostname` hostname added to allow for users to not setup /etc/hostname within builds.

rhatdan · 2023-10-20T15:46:09Z

@Luap99 @flouthoc @vrothberg @nalind @umohnani8 PTAL

nalind

Mostly nits.

docs/buildah-build.1.md

run.go

nalind · 2023-10-20T17:19:52Z

run_linux.go

@@ -269,8 +269,7 @@ func (b *Builder) Run(command []string, options RunOptions) error {
 		bindFiles[config.DefaultHostsFile] = hostFile
 	}

-	// generate /etc/hostname if the user intentionally did not override
-	if !(contains(volumes, "/etc/hostname")) {
+	if !options.NoHostname && !(contains(volumes, "/etc/hostname")) {
 		if _, ok := bindFiles["/etc/hostname"]; !ok {


I don't see how ok can ever be true here. Did I miss something?

I think this is for the use case where a user does -v /tmp/hostname:/etc/hostname

I still don't follow. If I'm reading it right, the bindFiles map has, at most, one item in it: a value for the config.DefaultHostsFile key. A -v flag from the command line would show up in options.Volumes or b.CommonBuildOpts.Volumes, or maybe both if we're in build.

contains() probably shouldn't exist when we got rid of for github.com/containers/buildah/util.StringInSlice() in favor of github.com/containers/common/pkg/util.StringInSlice().

That was existing code, so I will remove the check.

nalind · 2023-10-20T17:22:48Z

cmd/buildah/run.go

@@ -137,10 +139,15 @@ func runCmd(c *cobra.Command, args []string, iopts runInputOptions) error {
 		}
 	}

+	if len(iopts.hostname) > 0 && iopts.noHostname {
+		return errors.New("--no-hostname and --hostname conflict, can not be used together")


One's a setting in the UTS namespace, the other's a file. Do they conflict?

I believe --hostname attempts to set the hostname field in /etc/hostname, if you don't think this would confuse the user, then I am fine with removing it.

I'd remove the test

tests/bud.bats

nalind · 2023-10-20T17:25:43Z

tests/bud.bats

+  cat > $mytmpdir/Containerfile << _EOF
+from alpine
+RUN mv /etc /usr/
+RUN ls -l /etc


The content of this file will always be the same. Why are we generating it every time this test runs?

This file has different content then the previous?

Use two static files?

tests/bud.bats

TomSweeneyRedHat · 2023-10-20T20:09:35Z

docs/buildah-build.1.md

+
+By default, Buildah manages _/etc/hostname_, adding the container's own hostname.
+**--no-hostname** disables this, and the image's _/etc/hostname_ will be preserved unmodified if it exists.
+Conflicts with the --hostname option.


If you remove the test above, you should remove this line too.

TomSweeneyRedHat · 2023-10-20T20:10:24Z

docs/buildah-build.1.md

@@ -645,9 +645,17 @@ Valid _mode_ values are:

 Do not use existing cached images for the container build. Build from the start with a new set of cached layers.

+**--no-hostname**
+
+Do not create _/etc/hostname_ for RUN instructions.


Soft suggestion

Suggested change

Do not create _/etc/hostname_ for RUN instructions.

Do not create the _/etc/hostname_ file in the container for RUN instructions.

TomSweeneyRedHat · 2023-10-20T20:11:20Z

docs/buildah-build.1.md

+
+Do not create _/etc/hostname_ for RUN instructions.
+
+By default, Buildah manages _/etc/hostname_, adding the container's own hostname.


I'm on the fence about adding the word file after /etc/hostname here and below.

docs/buildah-run.1.md

rhatdan · 2023-10-24T16:45:29Z

@Luap99 @flouthoc @vrothberg @nalind @umohnani8 @TomSweeneyRedHat PTAL

umohnani8 · 2023-10-24T17:02:06Z

LGTM

@rhatdan I think it needs a rebase though

TomSweeneyRedHat · 2023-10-24T23:02:06Z

LGTM

Fixes: containers#5093 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

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

flouthoc

Restarted flaking test
LGTM
/lgtm
/hold

openshift-ci · 2023-10-25T05:00:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, 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~~ [flouthoc,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-10-25T10:23:32Z

/unhold

vrothberg · 2023-10-26T08:26:48Z

Reminder to also open Podman PRs when adding new build flags. Currently, Podman CI breaks when vendoring Buildah.

openshift-ci bot added the approved label Oct 20, 2023

nalind requested changes Oct 20, 2023

View reviewed changes

rhatdan force-pushed the hostname branch from 2aedb87 to 21b848b Compare October 20, 2023 18:06

TomSweeneyRedHat reviewed Oct 20, 2023

View reviewed changes

docs/buildah-run.1.md Show resolved Hide resolved

rhatdan force-pushed the hostname branch 5 times, most recently from 9d95258 to c6ffdc7 Compare October 23, 2023 18:31

rhatdan added 2 commits October 24, 2023 19:43

Add --no-hostname option to buildah containers

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: A2DF901DABE2C028

Learn about vigilant mode

48a9c43

Fixes: containers#5093 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Use cutil.StringInSlice rather then contains

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: A2DF901DABE2C028

Learn about vigilant mode

03b0042

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

rhatdan force-pushed the hostname branch from c6ffdc7 to 03b0042 Compare October 24, 2023 23:44

flouthoc approved these changes Oct 25, 2023

View reviewed changes

openshift-ci bot assigned flouthoc Oct 25, 2023

openshift-ci bot added do-not-merge/hold lgtm labels Oct 25, 2023

openshift-ci bot removed the do-not-merge/hold label Oct 25, 2023

openshift-ci bot merged commit c9523b3 into containers:main Oct 25, 2023

github-actions bot added the locked - please file new issue/PR label Jan 25, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --no-hostname option to buildah containers #5094

Add --no-hostname option to buildah containers #5094

rhatdan commented Oct 20, 2023

rhatdan commented Oct 20, 2023

nalind left a comment

nalind Oct 20, 2023

rhatdan Oct 20, 2023

nalind Oct 20, 2023

rhatdan Oct 22, 2023

nalind Oct 20, 2023

rhatdan Oct 20, 2023

TomSweeneyRedHat Oct 20, 2023

nalind Oct 20, 2023

rhatdan Oct 20, 2023

nalind Oct 20, 2023

TomSweeneyRedHat Oct 20, 2023

TomSweeneyRedHat Oct 20, 2023

TomSweeneyRedHat Oct 20, 2023

rhatdan commented Oct 24, 2023

umohnani8 commented Oct 24, 2023

TomSweeneyRedHat commented Oct 24, 2023

flouthoc left a comment

openshift-ci bot commented Oct 25, 2023

rhatdan commented Oct 25, 2023

vrothberg commented Oct 26, 2023

	Do not create _/etc/hostname_ for RUN instructions.
	Do not create the _/etc/hostname_ file in the container for RUN instructions.


		Do not create _/etc/hostname_ for RUN instructions.

		By default, Buildah manages _/etc/hostname_, adding the container's own hostname.

Add --no-hostname option to buildah containers #5094

Add --no-hostname option to buildah containers #5094

Conversation

rhatdan commented Oct 20, 2023

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?

rhatdan commented Oct 20, 2023

nalind 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 commented Oct 24, 2023

umohnani8 commented Oct 24, 2023

TomSweeneyRedHat commented Oct 24, 2023

flouthoc left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 25, 2023

rhatdan commented Oct 25, 2023

vrothberg commented Oct 26, 2023