Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stage_executor: reset platform in systemcontext for every stage. #5971

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+47 −17
Open

stage_executor: reset platform in systemcontext for every stage. #5971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions imagebuildah/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,9 +372,12 @@ func newExecutor(logger *logrus.Logger, logPrefix string, store storage.Store, o
// startStage creates a new stage executor that will be referenced whenever a
// COPY or ADD statement uses a --from=NAME flag.
func (b *Executor) startStage(ctx context.Context, stage *imagebuilder.Stage, stages imagebuilder.Stages, output string) *StageExecutor {
// create a copy of systemContext for each stage executor.
systemContext := *b.systemContext
stageExec := &StageExecutor{
ctx: ctx,
executor: b,
systemContext: &systemContext,
log: b.log,
index: stage.Position,
stages: stages,
Expand Down
36 changes: 19 additions & 17 deletions imagebuildah/stage_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import (
// name to the image that it produces.
type StageExecutor struct {
ctx context.Context
systemContext *types.SystemContext
executor *Executor
log func(format string, args ...interface{})
index int
Expand Down Expand Up @@ -584,8 +585,8 @@ func (s *StageExecutor) performCopy(excludes []string, copies ...imagebuilder.Co
// The values for these next two fields are ultimately
// based on command line flags with names that sound
// much more generic.
CertPath: s.executor.systemContext.DockerCertPath,
InsecureSkipTLSVerify: s.executor.systemContext.DockerInsecureSkipTLSVerify,
CertPath: s.systemContext.DockerCertPath,
InsecureSkipTLSVerify: s.systemContext.DockerInsecureSkipTLSVerify,
MaxRetries: s.executor.maxPullPushRetries,
RetryDelay: s.executor.retryPullPushDelay,
Parents: copy.Parents,
Expand Down Expand Up @@ -841,7 +842,7 @@ func (s *StageExecutor) Run(run imagebuilder.Run, config docker.Config) error {
Stderr: s.executor.err,
Stdin: stdin,
Stdout: s.executor.out,
SystemContext: s.executor.systemContext,
SystemContext: s.systemContext,
Terminal: buildah.WithoutTerminal,
User: config.User,
WorkingDir: config.WorkingDir,
Expand Down Expand Up @@ -966,19 +967,20 @@ func (s *StageExecutor) prepare(ctx context.Context, from string, initializeIBCo
}
}

builderSystemContext := s.executor.systemContext
// get platform string from stage
if stage.Builder.Platform != "" {
os, arch, variant, err := parse.Platform(stage.Builder.Platform)
// In a multi-stage build where `FROM --platform=<>` was used then we must
// reset context for new stages so that new stages don't inherit unexpected
// `--platform` from prior stages.
if stage.Builder.Platform != "" || (len(s.stages) > 1 && ((s.systemContext.ArchitectureChoice == "" && s.systemContext.VariantChoice == "" && s.systemContext.OSChoice == "") || (s.executor.skipUnusedStages == types.OptionalBoolFalse))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current revision, s.executor.skipUnusedStages doesn't appear to matter and can likely be dropped, as the current revision ensures that OSChoice/ArchitectureChoice/VariantChoice is explicitly specified for every stage of a multi-stage build (earlier revisions only did so for the second and later stages), either to values given in the --platform flag for the FROM instruction, values passed in through our API, or the current system's defaults.

imageOS, imageArch, imageVariant, err := parse.Platform(stage.Builder.Platform)
if err != nil {
return nil, fmt.Errorf("unable to parse platform %q: %w", stage.Builder.Platform, err)
}
if arch != "" || variant != "" {
builderSystemContext.ArchitectureChoice = arch
builderSystemContext.VariantChoice = variant
if imageArch != "" || imageVariant != "" {
s.systemContext.ArchitectureChoice = imageArch
s.systemContext.VariantChoice = imageVariant
}
if os != "" {
builderSystemContext.OSChoice = os
if imageOS != "" {
s.systemContext.OSChoice = imageOS
}
}

Expand All @@ -992,7 +994,7 @@ func (s *StageExecutor) prepare(ctx context.Context, from string, initializeIBCo
BlobDirectory: s.executor.blobDirectory,
SignaturePolicyPath: s.executor.signaturePolicyPath,
ReportWriter: s.executor.reportWriter,
SystemContext: builderSystemContext,
SystemContext: s.systemContext,
Isolation: s.executor.isolation,
NamespaceOptions: s.executor.namespaceOptions,
ConfigureNetwork: s.executor.configureNetwork,
Expand Down Expand Up @@ -2058,7 +2060,7 @@ func (s *StageExecutor) tagExistingImage(ctx context.Context, cacheID, output st
return "", nil, err
}

policyContext, err := util.GetPolicyContext(s.executor.systemContext)
policyContext, err := util.GetPolicyContext(s.systemContext)
if err != nil {
return "", nil, err
}
Expand Down Expand Up @@ -2171,7 +2173,7 @@ func (s *StageExecutor) pushCache(ctx context.Context, src, cacheKey string) err
Compression: s.executor.compression,
SignaturePolicyPath: s.executor.signaturePolicyPath,
Store: s.executor.store,
SystemContext: s.executor.systemContext,
SystemContext: s.systemContext,
BlobDirectory: s.executor.blobDirectory,
SignBy: s.executor.signBy,
MaxRetries: s.executor.maxPullPushRetries,
Expand Down Expand Up @@ -2209,7 +2211,7 @@ func (s *StageExecutor) pullCache(ctx context.Context, cacheKey string) (referen
options := buildah.PullOptions{
SignaturePolicyPath: s.executor.signaturePolicyPath,
Store: s.executor.store,
SystemContext: s.executor.systemContext,
SystemContext: s.systemContext,
BlobDirectory: s.executor.blobDirectory,
MaxRetries: s.executor.maxPullPushRetries,
RetryDelay: s.executor.retryPullPushDelay,
Expand Down Expand Up @@ -2431,7 +2433,7 @@ func (s *StageExecutor) commit(ctx context.Context, createdBy string, emptyLayer
SignaturePolicyPath: s.executor.signaturePolicyPath,
ReportWriter: writer,
PreferredManifestType: s.executor.outputFormat,
SystemContext: s.executor.systemContext,
SystemContext: s.systemContext,
Squash: squash,
OmitHistory: s.executor.commonBuildOptions.OmitHistory,
EmptyLayer: emptyLayer,
Expand Down
15 changes: 15 additions & 0 deletions tests/bud.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6573,6 +6573,21 @@ _EOF
done
}

@test "build must reset platform for stages if needed" {
run_buildah info --format '{{.host.arch}}'
myarch="$output"
otherarch="arm64"
# just make sure that other arch is not equivalent to host arch
if [[ "$otherarch" == "$myarch" ]]; then
otherarch="amd64"
fi
run_buildah build $WITH_POLICY_JSON --build-arg FOREIGNARCH=$otherarch -f $BUDFILES/multiarch/Containerfile.reset-platform $BUDFILES/multiarch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also test a case where the the TARGETPLATFORM build-arg is set, so that we don't regress on that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test which sets TARGETPLATFORM using --platform, should that cover what you meant by this comment ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean literally setting --build-arg TARGETPLATFORM=, which has been known to be done instead of using the --platform flag. This had to be fixed before in #5731.

run_buildah build $WITH_POLICY_JSON --build-arg TARGETPLATFORM=linux/$myarch --build-arg FOREIGNARCH=$otherarch -f $BUDFILES/multiarch/Containerfile.reset-platform $BUDFILES/multiarch
# Following line here attempts to cover edge case discussed here https://github.com/containers/buildah/pull/5971/files#r1970277844 but it's hard to reproduce this in a certain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This URL no longer turns up a discussion.

# way since it can happen only when unused stage is picked in a particular order, following line ensures if this thing breaks then we luckily catch it in our CI.
run_buildah build $WITH_POLICY_JSON --jobs=2 --skip-unused-stages=true --build-arg FOREIGNARCH=$otherarch -f $BUDFILES/multiarch/Containerfile.unused-stage $BUDFILES/multiarch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this run_buildah invocation testing, exactly? The image that's produced isn't examined, and the build run's success only depends on the base image existing.

}

# * Performs multi-stage build with label1=value1 and verifies
# * Relabels build with label1=value2 and verifies
# * Rebuild with label1=value1 and makes sure everything is used from cache
Expand Down
7 changes: 7 additions & 0 deletions tests/bud/multiarch/Containerfile.reset-platform
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
ARG FOREIGNARCH
FROM --platform=linux/$FOREIGNARCH busybox AS foreign
FROM busybox
COPY --from=foreign /bin/busybox /bin/busybox.foreign
RUN arch
RUN ls -l /bin/busybox /bin/busybox.foreign
RUN ! cmp /bin/busybox /bin/busybox.foreign
3 changes: 3 additions & 0 deletions tests/bud/multiarch/Containerfile.unused-stage
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
FROM busybox
RUN arch
FROM --platform=linux/$FOREIGNARCH busybox AS foreign