Skip to content

dockerfile: maintain osversion/variant from the base image #5714

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

Merged
tonistiigi merged 1 commit into from
Feb 11, 2025
Merged

dockerfile: maintain osversion/variant from the base image #5714

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
9 changes: 7 additions & 2 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,10 +801,15 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
target.state = target.state.SetMarshalDefaults(defaults...)

if !platformOpt.implicitTarget {
sameOsArch := platformOpt.targetPlatform.OS == target.image.OS && platformOpt.targetPlatform.Architecture == target.image.Architecture
target.image.OS = platformOpt.targetPlatform.OS
target.image.Architecture = platformOpt.targetPlatform.Architecture
target.image.Variant = platformOpt.targetPlatform.Variant
target.image.OSVersion = platformOpt.targetPlatform.OSVersion
if platformOpt.targetPlatform.Variant != "" || !sameOsArch {
Copy link
Member

@cpuguy83 cpuguy83 Feb 11, 2025

Choose a reason for hiding this comment

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

I'm not sure i understand the sameOsArch check here.
Also the test does not cover it.

Copy link
Member Author

@tonistiigi tonistiigi Feb 11, 2025

Choose a reason for hiding this comment

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

The meaning of this section seems to be that if explicit platform was set then it rewrites the one in the loaded base image. I guess it is mainly related to the case where single arch image is loaded with FROM even if it is for a wrong arch. I don't set the variant/osversion in this case to avoid cases where --platform linux/* build would get osversion(or wrong variant) from the base image.

target.image.Variant = platformOpt.targetPlatform.Variant
}
if platformOpt.targetPlatform.OSVersion != "" || !sameOsArch {
target.image.OSVersion = platformOpt.targetPlatform.OSVersion
}
if platformOpt.targetPlatform.OSFeatures != nil {
Copy link
Member Author

@tonistiigi tonistiigi Feb 11, 2025

Choose a reason for hiding this comment

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

This OSFeatures line doesn't make much sense to me either as it can create duplicates and if !sameOsArch then probably should not be merged. Shouldn't be impactful as OSFeatures can't be passed via opt atm. but that will change. I opened containerd/platforms#23 as I think some of it should be handled in upstream platforms pkg.

Copy link
Collaborator

@TBBle TBBle Feb 18, 2025

Choose a reason for hiding this comment

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

This isn't merging the OSFeatures, it's replacing them, so I don't understand where duplicates would come from? It should just be a straight-up copy of platformOpt.targetPlatform.OSFeatures.

It probably should have had the same !sameOsArch check added though. Not particularly impactful, last time I looked into it, OSFeatures was 100% unused and the OCI Working Group that might want it for future use didn't seem interested in it, in favour of a separate artifact that covered its use-cases.

Copy link
Member Author

@tonistiigi tonistiigi Feb 19, 2025

Choose a reason for hiding this comment

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

This isn't merging the OSFeatures, it's replacing them

Ah, yeah. I guess I looked that it was appending to the old OSFeatures, but it is appending to empty array (slices.Clone)

Copy link
Collaborator

@TBBle TBBle Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I had to open-code slices.Clone in the original PR because it wasn't available until Go 1.21 and we were declaring Go 1.20 as supported at that time.

That's no longer the case so we could clean that up for readability now.

target.image.OSFeatures = append([]string{}, platformOpt.targetPlatform.OSFeatures...)
}
Expand Down
114 changes: 114 additions & 0 deletions frontend/dockerfile/dockerfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ var allTests = integration.TestFuncs(
testPowershellInDefaultPathOnWindows,
testOCILayoutMultiname,
testPlatformWithOSVersion,
testMaintainBaseOSVersion,
)

// Tests that depend on the `security.*` entitlements
Expand Down Expand Up @@ -9634,6 +9635,119 @@ EOF
require.Equal(t, p2.OSVersion+"\n", string(dt))
}

func testMaintainBaseOSVersion(t *testing.T, sb integration.Sandbox) {
integration.SkipOnPlatform(t, "windows")
workers.CheckFeatureCompat(t, sb, workers.FeatureDirectPush)

ctx := sb.Context()

c, err := client.New(ctx, sb.Address())
require.NoError(t, err)
defer c.Close()

f := getFrontend(t, sb)

p1 := ocispecs.Platform{
OS: "windows",
OSVersion: "10.20.30",
Architecture: "amd64",
}
p1Str := platforms.FormatAll(p1)

registry, err := sb.NewRegistry()
if errors.Is(err, integration.ErrRequirements) {
t.Skip(err.Error())
}
require.NoError(t, err)
target := registry + "/buildkit/testplatformwithosversion-1:latest"

dockerfile := []byte(`
FROM scratch
ARG TARGETPLATFORM
COPY <<EOF /platform
${TARGETPLATFORM}
EOF
`)

dir := integration.Tmpdir(
t,
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
FrontendAttrs: map[string]string{
"platform": p1Str,
},
Exports: []client.ExportEntry{
{
Type: client.ExporterImage,
Attrs: map[string]string{
"name": target,
"push": "true",
},
},
},

LocalMounts: map[string]fsutil.FS{
dockerui.DefaultLocalNameDockerfile: dir,
dockerui.DefaultLocalNameContext: dir,
},
}, nil)

require.NoError(t, err)

desc, provider, err := contentutil.ProviderFromRef(target)
require.NoError(t, err)

info, err := testutil.ReadImages(ctx, provider, desc)
require.NoError(t, err)
require.Len(t, info.Images, 1)
require.Equal(t, info.Images[0].Img.Platform.OSVersion, p1.OSVersion)

dockerfile = []byte(fmt.Sprintf(`
FROM %s
COPY <<EOF /other
hello
EOF
`, target))

dir = integration.Tmpdir(
t,
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)

target2 := registry + "/buildkit/testplatformwithosversion-2:latest"

_, err = f.Solve(sb.Context(), c, client.SolveOpt{
FrontendAttrs: map[string]string{
"platform": p1.OS + "/" + p1.Architecture,
},
Exports: []client.ExportEntry{
{
Type: client.ExporterImage,
Attrs: map[string]string{
"name": target2,
"push": "true",
},
},
},

LocalMounts: map[string]fsutil.FS{
dockerui.DefaultLocalNameDockerfile: dir,
dockerui.DefaultLocalNameContext: dir,
},
}, nil)
require.NoError(t, err)

desc, provider, err = contentutil.ProviderFromRef(target2)
require.NoError(t, err)

info, err = testutil.ReadImages(ctx, provider, desc)
require.NoError(t, err)
require.Len(t, info.Images, 1)
require.Equal(t, info.Images[0].Img.Platform.OSVersion, p1.OSVersion)
}

func runShell(dir string, cmds ...string) error {
for _, args := range cmds {
var cmd *exec.Cmd
Expand Down