Whenever copying OCI Platform data, include OSVersion and OSFeatures

Trivially created by looking for every reference to .Variant and adding OSVersion and OSFeatures, except the ones related to the string representation of a Platform instance. I then went through and ensured every assignment of OSFeatures used the append-to-nil idiom to avoid sharing the slice storage after copying. Signed-off-by: Paul "TBBle" Hampson <[email protected]>
moby · Nov 2, 2023 · ca6d8c5 · ca6d8c5
1 parent 36c5550
commit ca6d8c5
Show file tree

Hide file tree

Showing 17 changed files with 67 additions and 33 deletions.
diff --git a/client/llb/marshal.go b/client/llb/marshal.go
@@ -101,7 +101,7 @@ func MarshalConstraints(base, override *Constraints) (*pb.Op, *pb.OpMetadata) {
 			Architecture: c.Platform.Architecture,
 			Variant:      c.Platform.Variant,
 			OSVersion:    c.Platform.OSVersion,
-			OSFeatures:   c.Platform.OSFeatures,
+			OSFeatures:   append([]string{}, c.Platform.OSFeatures...),
 		},
 		Constraints: &pb.WorkerConstraints{
 			Filter: c.WorkerConstraints,

diff --git a/client/llb/state.go b/client/llb/state.go
@@ -262,6 +262,8 @@ func (s State) WithImageConfig(c []byte) (State, error) {
 			OS:           img.OS,
 			Architecture: img.Architecture,
 			Variant:      img.Variant,
+			OSVersion:    img.OSVersion,
+			OSFeatures:   append([]string{}, img.OSFeatures...),
 		})
 	}
 	return s, nil

diff --git a/exporter/containerimage/exptypes/parse.go b/exporter/containerimage/exptypes/parse.go
@@ -33,7 +33,7 @@ func ParsePlatforms(meta map[string][]byte) (Platforms, error) {
 				Architecture: img.Architecture,
 				OS:           img.OS,
 				OSVersion:    img.OSVersion,
-				OSFeatures:   img.OSFeatures,
+				OSFeatures:   append([]string{}, img.OSFeatures...),
 				Variant:      img.Variant,
 			}
 		}

diff --git a/exporter/containerimage/writer.go b/exporter/containerimage/writer.go
@@ -598,6 +598,8 @@ func defaultImageConfig() ([]byte, error) {
 	img := ocispecs.Image{}
 	img.Architecture = pl.Architecture
 	img.OS = pl.OS
+	img.OSVersion = pl.OSVersion
+	img.OSFeatures = append([]string{}, pl.OSFeatures...)
 	img.Variant = pl.Variant
 	img.RootFS.Type = "layers"
 	img.Config.WorkingDir = "/"
@@ -611,7 +613,7 @@ func attestationsConfig(layers []ocispecs.Descriptor) ([]byte, error) {
 	img.Architecture = intotoPlatform.Architecture
 	img.OS = intotoPlatform.OS
 	img.OSVersion = intotoPlatform.OSVersion
-	img.OSFeatures = intotoPlatform.OSFeatures
+	img.OSFeatures = append([]string{}, intotoPlatform.OSFeatures...)
 	img.Variant = intotoPlatform.Variant
 	img.RootFS.Type = "layers"
 	for _, layer := range layers {

diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go
@@ -274,6 +274,8 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 							OS:           img.OS,
 							Architecture: img.Architecture,
 							Variant:      img.Variant,
+							OSVersion:    img.OSVersion,
+							OSFeatures:   append([]string{}, img.OSFeatures...),
 						}
 					}
 				}
@@ -595,6 +597,8 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 		target.image.OS = platformOpt.targetPlatform.OS
 		target.image.Architecture = platformOpt.targetPlatform.Architecture
 		target.image.Variant = platformOpt.targetPlatform.Variant
+		target.image.OSVersion = platformOpt.targetPlatform.OSVersion
+		target.image.OSFeatures = append([]string{}, platformOpt.targetPlatform.OSFeatures...)
 	}
 
 	return target, nil

diff --git a/frontend/dockerfile/dockerfile2llb/image.go b/frontend/dockerfile/dockerfile2llb/image.go
@@ -19,6 +19,8 @@ func emptyImage(platform ocispecs.Platform) image.Image {
 	img := image.Image{}
 	img.Architecture = platform.Architecture
 	img.OS = platform.OS
+	img.OSVersion = platform.OSVersion
+	img.OSFeatures = append([]string{}, platform.OSFeatures...)
 	img.Variant = platform.Variant
 	img.RootFS.Type = "layers"
 	img.Config.WorkingDir = "/"

diff --git a/frontend/dockerui/build.go b/frontend/dockerui/build.go
@@ -57,7 +57,7 @@ func (bc *Client) Build(ctx context.Context, fn BuildFunc) (*ResultBuilder, erro
 					p.OSVersion = img.OSVersion
 				}
 				if p.OSFeatures == nil && len(img.OSFeatures) > 0 {
-					p.OSFeatures = img.OSFeatures
+					p.OSFeatures = append([]string{}, img.OSFeatures...)
 				}
 			}
 

diff --git a/frontend/gateway/gateway.go b/frontend/gateway/gateway.go
@@ -566,7 +566,7 @@ func (lbf *llbBridgeForwarder) ResolveImageConfig(ctx context.Context, req *pb.R
 			Architecture: p.Architecture,
 			Variant:      p.Variant,
 			OSVersion:    p.OSVersion,
-			OSFeatures:   p.OSFeatures,
+			OSFeatures:   append([]string{}, p.OSFeatures...),
 		}
 	}
 	ref, dgst, dt, err := lbf.llbBridge.ResolveImageConfig(ctx, req.Ref, llb.ResolveImageConfigOpt{

diff --git a/frontend/gateway/grpcclient/client.go b/frontend/gateway/grpcclient/client.go
@@ -486,7 +486,7 @@ func (c *grpcClient) ResolveImageConfig(ctx context.Context, ref string, opt llb
 			Architecture: platform.Architecture,
 			Variant:      platform.Variant,
 			OSVersion:    platform.OSVersion,
-			OSFeatures:   platform.OSFeatures,
+			OSFeatures:   append([]string{}, platform.OSFeatures...),
 		}
 	}
 

diff --git a/solver/llbsolver/ops/exec.go b/solver/llbsolver/ops/exec.go
@@ -113,6 +113,8 @@ func (e *ExecOp) CacheMap(ctx context.Context, g session.Group, index int) (*sol
 			OS:           e.platform.OS,
 			Architecture: e.platform.Architecture,
 			Variant:      e.platform.Variant,
+			OSVersion:    e.platform.OSVersion,
+			OSFeatures:   append([]string{}, e.platform.OSFeatures...),
 		}
 	}
 
@@ -133,17 +135,21 @@ func (e *ExecOp) CacheMap(ctx context.Context, g session.Group, index int) (*sol
 	}
 
 	dt, err := json.Marshal(struct {
-		Type    string
-		Exec    *pb.ExecOp
-		OS      string
-		Arch    string
-		Variant string `json:",omitempty"`
+		Type       string
+		Exec       *pb.ExecOp
+		OS         string
+		Arch       string
+		Variant    string   `json:",omitempty"`
+		OSVersion  string   `json:",omitempty"`
+		OSFeatures []string `json:",omitempty"`
 	}{
-		Type:    execCacheType,
-		Exec:    &op,
-		OS:      p.OS,
-		Arch:    p.Architecture,
-		Variant: p.Variant,
+		Type:       execCacheType,
+		Exec:       &op,
+		OS:         p.OS,
+		Arch:       p.Architecture,
+		Variant:    p.Variant,
+		OSVersion:  p.OSVersion,
+		OSFeatures: append([]string{}, p.OSFeatures...),
 	})
 	if err != nil {
 		return nil, false, err

diff --git a/solver/llbsolver/ops/exec_binfmt.go b/solver/llbsolver/ops/exec_binfmt.go
@@ -90,6 +90,8 @@ func getEmulator(ctx context.Context, p *pb.Platform, idmap *idtools.IdentityMap
 	pp := platforms.Normalize(ocispecs.Platform{
 		Architecture: p.Architecture,
 		OS:           p.OS,
+		OSVersion:    p.OSVersion,
+		OSFeatures:   append([]string{}, p.OSFeatures...),
 		Variant:      p.Variant,
 	})
 

diff --git a/solver/llbsolver/provenance/capture.go b/solver/llbsolver/provenance/capture.go
@@ -152,7 +152,9 @@ func (c *Capture) AddImage(i ImageSource) {
 				return
 			}
 			if v.Platform != nil && i.Platform != nil {
-				if v.Platform.Architecture == i.Platform.Architecture && v.Platform.OS == i.Platform.OS && v.Platform.Variant == i.Platform.Variant {
+				// NOTE: Deliberately excluding OSFeatures, as there's no extant (or rational) case where a source image is an index and contains images distinguished only by OSFeature
+				// See https://github.com/moby/buildkit/pull/4387#discussion_r1376234241
+				if v.Platform.Architecture == i.Platform.Architecture && v.Platform.OS == i.Platform.OS && v.Platform.OSVersion == i.Platform.OSVersion && v.Platform.Variant == i.Platform.Variant {
 					return
 				}
 			}

diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go
@@ -79,17 +79,27 @@ func NormalizeRuntimePlatforms() LoadOpt {
 					OS:           p.OS,
 					Architecture: p.Architecture,
 					Variant:      p.Variant,
+					OSVersion:    p.OSVersion,
+					OSFeatures:   append([]string{}, p.OSFeatures...),
 				}
 			}
 			op.Platform = defaultPlatform
 		}
-		platform := ocispecs.Platform{OS: op.Platform.OS, Architecture: op.Platform.Architecture, Variant: op.Platform.Variant}
+		platform := ocispecs.Platform{
+			OS:           op.Platform.OS,
+			Architecture: op.Platform.Architecture,
+			Variant:      op.Platform.Variant,
+			OSVersion:    op.Platform.OSVersion,
+			OSFeatures:   append([]string{}, op.Platform.OSFeatures...),
+		}
 		normalizedPlatform := platforms.Normalize(platform)
 
 		op.Platform = &pb.Platform{
 			OS:           normalizedPlatform.OS,
 			Architecture: normalizedPlatform.Architecture,
 			Variant:      normalizedPlatform.Variant,
+			OSVersion:    normalizedPlatform.OSVersion,
+			OSFeatures:   append([]string{}, normalizedPlatform.OSFeatures...),
 		}
 
 		return nil

diff --git a/solver/pb/ops.proto b/solver/pb/ops.proto
@@ -29,7 +29,7 @@ message Platform {
 	string Architecture = 1;
 	string OS = 2;
 	string Variant = 3;
-	string OSVersion = 4; // unused
+	string OSVersion = 4;
 	repeated string OSFeatures = 5; // unused
 }
 

diff --git a/solver/pb/platform.go b/solver/pb/platform.go
@@ -10,7 +10,7 @@ func (p *Platform) Spec() ocispecs.Platform {
 		Architecture: p.Architecture,
 		Variant:      p.Variant,
 		OSVersion:    p.OSVersion,
-		OSFeatures:   p.OSFeatures,
+		OSFeatures:   append([]string{}, p.OSFeatures...),
 	}
 }
 
@@ -20,7 +20,7 @@ func PlatformFromSpec(p ocispecs.Platform) Platform {
 		Architecture: p.Architecture,
 		Variant:      p.Variant,
 		OSVersion:    p.OSVersion,
-		OSFeatures:   p.OSFeatures,
+		OSFeatures:   append([]string{}, p.OSFeatures...),
 	}
 }
 

diff --git a/source/containerimage/pull.go b/source/containerimage/pull.go
@@ -60,17 +60,21 @@ type puller struct {
 
 func mainManifestKey(ctx context.Context, desc ocispecs.Descriptor, platform ocispecs.Platform, layerLimit *int) (digest.Digest, error) {
 	dt, err := json.Marshal(struct {
-		Digest  digest.Digest
-		OS      string
-		Arch    string
-		Variant string `json:",omitempty"`
-		Limit   *int   `json:",omitempty"`
+		Digest     digest.Digest
+		OS         string
+		Arch       string
+		Variant    string   `json:",omitempty"`
+		OSVersion  string   `json:",omitempty"`
+		OSFeatures []string `json:",omitempty"`
+		Limit      *int     `json:",omitempty"`
 	}{
-		Digest:  desc.Digest,
-		OS:      platform.OS,
-		Arch:    platform.Architecture,
-		Variant: platform.Variant,
-		Limit:   layerLimit,
+		Digest:     desc.Digest,
+		OS:         platform.OS,
+		Arch:       platform.Architecture,
+		Variant:    platform.Variant,
+		OSVersion:  platform.OSVersion,
+		OSFeatures: append([]string{}, platform.OSFeatures...),
+		Limit:      layerLimit,
 	})
 	if err != nil {
 		return "", err

diff --git a/source/containerimage/source.go b/source/containerimage/source.go
@@ -202,7 +202,7 @@ func (is *Source) registryIdentifier(ref string, attrs map[string]string, platfo
 			Architecture: platform.Architecture,
 			Variant:      platform.Variant,
 			OSVersion:    platform.OSVersion,
-			OSFeatures:   platform.OSFeatures,
+			OSFeatures:   append([]string{}, platform.OSFeatures...),
 		}
 	}
 
@@ -247,7 +247,7 @@ func (is *Source) ociIdentifier(ref string, attrs map[string]string, platform *p
 			Architecture: platform.Architecture,
 			Variant:      platform.Variant,
 			OSVersion:    platform.OSVersion,
-			OSFeatures:   platform.OSFeatures,
+			OSFeatures:   append([]string{}, platform.OSFeatures...),
 		}
 	}