executor: expand args in build logs #6329

flouthoc · 2025-08-09T15:31:58Z

Expand user,builtin and heading args before logging build steps.

Closes: #6327

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?

executor: expand args in build logs

openshift-ci · 2025-08-09T15:32:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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]

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

flouthoc · 2025-08-09T15:32:33Z

@vrothberg @nalind PTAL

vrothberg

Changes and tests LGTM.

I didn't check, but maybe there's a similar behavior with ENV?

Thanks so much for the quick PR 🙏

nalind

This produces some weird corner cases, both with this patch and with docker build:

FROM busybox
ARG CMD=pwd
RUN CMD=ls; echo $CMD

nalind · 2025-08-11T13:28:03Z

imagebuildah/executor.go

+				argsCopy[i], err = imagebuilder.ProcessWord(args[i].(string), userArgs)
+				if err != nil {
+					argsCopy[i] = args[i]
+					fmt.Printf("while replacing arg variables with values for format in logging %q: %w\n", args[i], err)


Why is this error message sent to stdout rather than one of the output destinations that's been specified for logging?

nalind · 2025-08-11T13:29:43Z

imagebuildah/executor.go

+			headingArgs := argsMapToSlice(stage.Builder.HeadingArgs)
+			userArgs := argsMapToSlice(stage.Builder.Args)
+			userArgs = append(builtinArgs, append(userArgs, headingArgs...)...)
+			argsCopy := args


If this is meant to create a copy of args, use slices.Clone(), otherwise they're both still pointing at the same array.

nalind · 2025-08-11T13:36:43Z

tests/bud.bats

+_EOF
+
+  run_buildah build $WITH_POLICY_JSON -t ${target}-undefined -f $contextdir/Containerfile2 $contextdir
+  expect_output --substring "RUN echo "


This would match whether a value was printed or not.

flouthoc · 2025-08-15T21:21:33Z

This produces some weird corner cases, both with this patch and with docker build:
FROM busybox
ARG CMD=pwd
RUN CMD=ls; echo $CMD

Similar behavior exists in buildkit as well I think this is bug in both buildah and docker. Should we fix it here ?

Luap99

This produces some weird corner cases, both with this patch and with docker build:
FROM busybox
ARG CMD=pwd
RUN CMD=ls; echo $CMD
Similar behavior exists in buildkit as well I think this is bug in both buildah and docker. Should we fix it here ?

You don't event need build args to trigger this:
RUN test=1; echo ${test:-abc}

And worse if you just have an unset non existing build arg buildah just removes from the log (uses empty string) which is also super confusing,

The issue states

I would love Buildah to expand the variables as well. I couldn't wrap my head around an issue where the build succeeded but didn't provide the build artifact I expected. Trying docker build helped me debug the issue and correct the Containerfile.

Given that I would argue this change doesn't really improve the situation, it trades one problem for another. But if docker behaves like that and logs incorrectly then maybe matching its bugs makes sense? I don't personally care much either way.

But I think the correct thing is to only replace the vars that buildah actually replaced when executing the command which ties back to my comment about replacing the RUN string in to many different places.

Luap99 · 2025-08-18T10:51:49Z

imagebuildah/executor.go

+			headingArgs := argsMapToSlice(stage.Builder.HeadingArgs)
+			userArgs := argsMapToSlice(stage.Builder.Args)
+			userArgs = append(builtinArgs, append(userArgs, headingArgs...)...)
+			argsCopy := slices.Clone(args)


why is this cloning the full args array if we throw away all values, that is just causing unnecessary memory copies.
To preallocate use make([]string, 0, len(args)).

Luap99 · 2025-08-18T10:57:10Z

imagebuildah/executor.go

+			userArgs = append(builtinArgs, append(userArgs, headingArgs...)...)
+			argsCopy := slices.Clone(args)
+			for i := range args {
+				argsCopy[i], err = imagebuilder.ProcessWord(args[i].(string), userArgs)


casting to string seems unsafe, the function signature accepts any, This means all non strings values are discarded basically.
If the log can only deal with string the function signature should be updated to only accept strings.

In principle I am also quite confused in how many different places ProcessWord() gets called, buildah seems to call this and replace values as one off in may palaces. I think it would be much cleaner to replace and and then keep the replaced values but I don't understand enough about the buildah design here.

Am I looking at the right function? ProcessWord() appears to take a string and a slice of strings, so type assertions shouldn't even be necessary.

I am talking about the log function itself which defines args as []any type. Everything that gets passed which is not a string is basically lost here due this type cast.

Ah, I follow now. The assertion isn't checked, so I think that case would panic. In practice, we only pass it strings.

I am processing only args with string type, I avoided changing function signature as it involves making changes in public API of some structs so will do that in a new PR. Practically all args are strings so this check is just preventing from panic.

Luap99 · 2025-08-18T10:57:35Z

imagebuildah/executor.go

+				argsCopy[i], err = imagebuilder.ProcessWord(args[i].(string), userArgs)
+				if err != nil {
+					argsCopy[i] = args[i]
+					fmt.Fprintf(stageExecutor.executor.err, "while replacing arg variables with values for format in logging %q: %w\n", args[i], err)


you cannot use %w for normal formatting, only in fmt.Errorf()

flouthoc · 2025-08-18T20:15:51Z

This produces some weird corner cases, both with this patch and with docker build:
FROM busybox
ARG CMD=pwd
RUN CMD=ls; echo $CMD
Similar behavior exists in buildkit as well I think this is bug in both buildah and docker. Should we fix it here ?
You don't event need build args to trigger this: RUN test=1; echo ${test:-abc}

And worse if you just have an unset non existing build arg buildah just removes from the log (uses empty string) which is also super confusing,

The issue states

I would love Buildah to expand the variables as well. I couldn't wrap my head around an issue where the build succeeded but didn't provide the build artifact I expected. Trying docker build helped me debug the issue and correct the Containerfile.

Given that I would argue this change doesn't really improve the situation, it trades one problem for another. But if docker behaves like that and logs incorrectly then maybe matching its bugs makes sense? I don't personally care much either way.

But I think the correct thing is to only replace the vars that buildah actually replaced when executing the command which ties back to my comment about replacing the RUN string in to many different places.

I am keeping behavior same as buildkit for consistency and created an upstream issue with moby/buildkit#6139 to track it there.

Expand user,builtin and heading args before logging build steps. Closes: containers#6327 Signed-off-by: flouthoc <[email protected]>

Signed-off-by: flouthoc <[email protected]>

Luap99 · 2025-08-19T10:40:55Z

imagebuildah/executor.go

+				if arg, ok := args[i].(string); ok {
+					if containsVariable(arg) {
+						processedArg, err := imagebuilder.ProcessWord(arg, userArgs)
+						if err != nil {
+							processedArg = arg
+							fmt.Fprintf(stageExecutor.executor.err, "while replacing arg variables with values for format in logging %q: %v\n", arg, err)
+						}
+						processedArgs = append(processedArgs, processedArg)
+					} else {
+						processedArgs = append(processedArgs, arg)
+					}
+				}


That still discards any arg which is not a string, the else block must be one lever out because it needs to keep the non string args otherwise the formatting string will be messed up badly.

Luap99 · 2025-08-19T10:46:15Z

imagebuildah/executor.go

+	re := regexp.MustCompile(`\$\{?[a-zA-Z_][a-zA-Z0-9_]*\}?`)
+	return re.MatchString(arg)


That compiles the regex every single time which is quite inefficient. That should use something like regexp.Delayed() in the global scope.

But is the regex really right? At the very least it ignores escaping rules \$var is not a variable. ProcessWord() has already has a parser so this would need to match exactly what it is doing. I think it would be better to reuse the ProcessWord code and expose some function for this instead, yeah sure more complicated but the only sane way to avoid any differences between the var replacement code (ProcessWord) and this custom regex

github-actions · 2025-09-19T00:14:15Z

A friendly reminder that this PR had no activity for 30 days.

openshift-ci bot added the approved label Aug 9, 2025

flouthoc mentioned this pull request Aug 9, 2025

expand build arguments in log #6327

Open

flouthoc force-pushed the expand-logs branch from f15c476 to 282fd89 Compare August 9, 2025 16:36

vrothberg reviewed Aug 11, 2025

View reviewed changes

nalind reviewed Aug 11, 2025

View reviewed changes

flouthoc force-pushed the expand-logs branch from 282fd89 to 2a6ba38 Compare August 15, 2025 21:19

Luap99 reviewed Aug 18, 2025

View reviewed changes

flouthoc force-pushed the expand-logs branch from 2a6ba38 to 97f584d Compare August 18, 2025 20:11

flouthoc requested a review from Luap99 August 18, 2025 20:16

flouthoc added 2 commits August 18, 2025 13:53

executor: expand args in build logs

b2065ea

Expand user,builtin and heading args before logging build steps. Closes: containers#6327 Signed-off-by: flouthoc <[email protected]>

retrofit tests

90e866e

Signed-off-by: flouthoc <[email protected]>

flouthoc force-pushed the expand-logs branch from 97f584d to 90e866e Compare August 18, 2025 20:54

Luap99 reviewed Aug 19, 2025

View reviewed changes

github-actions bot added the stale-pr label Sep 19, 2025

		re := regexp.MustCompile(`\$\{?[a-zA-Z_][a-zA-Z0-9_]*\}?`)
		return re.MatchString(arg)

executor: expand args in build logs #6329

Are you sure you want to change the base?

executor: expand args in build logs #6329

Uh oh!

Conversation

flouthoc commented Aug 9, 2025

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?

Uh oh!

openshift-ci bot commented Aug 9, 2025

Uh oh!

flouthoc commented Aug 9, 2025

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flouthoc commented Aug 15, 2025

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flouthoc commented Aug 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants