More fixes for target compat checking during detect (#1354)

* More fixes for target compat checking during detect - If a buildpack fails to specify os/arch (but specifies distro) still check targets - If the run image fails to specify os/arch (this should not happen actually as we will fail during analyze) still check targets - Fix typo in buildpack descriptor struct so that we actually get stack information - If we get distro information from /etc/os-release, persist this information to later invocations to that the log message printed when errors are encountered will be accurate - Don't override inner `i` in loop (this should not actually affect the outer loop but is confusing) Signed-off-by: Natalie Arellano <narellano@vmware.com> * When stack is "any", don't infer empty target as it is not needed Missing targets is sufficient for wildcard match Signed-off-by: Natalie Arellano <narellano@vmware.com> * Remove backwards compatible glue that actually causes fewer builds to succeed Fixes #1355 Signed-off-by: Natalie Arellano <narellano@vmware.com> * Remove exit (this was added for debugging purposes) Signed-off-by: Natalie Arellano <narellano@vmware.com> --------- Signed-off-by: Natalie Arellano <narellano@vmware.com>
buildpacks · May 14, 2024 · 73f6927 · 73f6927
1 parent 1a1de08
commit 73f6927
Show file tree

Hide file tree

Showing 12 changed files with 226 additions and 72 deletions.
diff --git a/acceptance/testdata/detector/Dockerfile b/acceptance/testdata/detector/Dockerfile
@@ -1,4 +1,4 @@
-FROM ubuntu:bionic
+FROM ubuntu:jammy
 
 ARG cnb_uid=1234
 ARG cnb_gid=1000

diff --git a/...etector/container/cnb/buildpacks/simple_buildpack/simple_buildpack_version/buildpack.toml b/...etector/container/cnb/buildpacks/simple_buildpack/simple_buildpack_version/buildpack.toml
@@ -1,6 +1,12 @@
-api = "0.10"
+api = "0.9"
 
 [buildpack]
-    id = "simple_buildpack"
-    version = "simple_buildpack_version"
-    name = "Simple Buildpack"
+id = "simple_buildpack"
+version = "simple_buildpack_version"
+name = "Simple Buildpack"
+
+[[stacks]]
+id = "io.buildpacks.stacks.bionic"
+
+[[stacks]]
+id = "io.buildpacks.stacks.jammy"
diff --git a/buildpack/bp_descriptor.go b/buildpack/bp_descriptor.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/BurntSushi/toml"
 
+	"github.com/buildpacks/lifecycle/api"
 	"github.com/buildpacks/lifecycle/internal/encoding"
 )
 
@@ -17,7 +18,7 @@ type BpDescriptor struct {
 	Order       Order            `toml:"order"`
 	WithRootDir string           `toml:"-"`
 	Targets     []TargetMetadata `toml:"targets"`
-	Stacks      []StackMetadata  `tome:"stacks"` // just for backwards compat so we can check if it's the bionic stack, which we translate to a target
+	Stacks      []StackMetadata  `toml:"stacks"` // just for backwards compat so we can check if it's the bionic stack, which we translate to a target
 
 }
 
@@ -69,7 +70,9 @@ func ReadBpDescriptor(path string) (*BpDescriptor, error) {
 	if len(descriptor.Targets) == 0 {
 		for _, stack := range descriptor.Stacks {
 			if stack.ID == "io.buildpacks.stacks.bionic" {
-				descriptor.Targets = append(descriptor.Targets, TargetMetadata{OS: "linux", Arch: "amd64", Distros: []OSDistro{{Name: "ubuntu", Version: "18.04"}}})
+				if api.MustParse(descriptor.API()).AtLeast("0.10") || len(descriptor.Stacks) == 1 {
+					descriptor.Targets = append(descriptor.Targets, TargetMetadata{OS: "linux", Arch: "amd64", Distros: []OSDistro{{Name: "ubuntu", Version: "18.04"}}})
+				}
 			} else if stack.ID == "*" {
 				descriptor.Targets = append(descriptor.Targets, TargetMetadata{}) // matches any
 			}

diff --git a/buildpack/bp_descriptor_test.go b/buildpack/bp_descriptor_test.go
@@ -81,44 +81,140 @@ func testBpDescriptor(t *testing.T, when spec.G, it spec.S) {
 			h.AssertEq(t, descriptor.Targets[0].Distros[0].Version, "V8.4-2L3")
 		})
 
-		it("does translate one special stack value into target values for older apis", func() {
-			path := filepath.Join("testdata", "buildpack", "by-id", "B", "v1", "buildpack.toml")
-			descriptor, err := buildpack.ReadBpDescriptor(path)
-			h.AssertNil(t, err)
-			// common sanity checks
-			h.AssertEq(t, descriptor.WithAPI, "0.7")
-			h.AssertEq(t, descriptor.Buildpack.ID, "B")
-			h.AssertEq(t, descriptor.Buildpack.Name, "Buildpack B")
-			h.AssertEq(t, descriptor.Buildpack.Version, "v1")
-			h.AssertEq(t, descriptor.Buildpack.Homepage, "Buildpack B Homepage")
-			h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
-			// specific behaviors for this test
-			h.AssertEq(t, descriptor.Stacks[0].ID, "io.buildpacks.stacks.bionic")
-			h.AssertEq(t, len(descriptor.Targets), 1)
-			h.AssertEq(t, descriptor.Targets[0].Arch, "amd64")
-			h.AssertEq(t, descriptor.Targets[0].OS, "linux")
-			h.AssertEq(t, descriptor.Targets[0].Distros[0].Name, "ubuntu")
-			h.AssertEq(t, descriptor.Targets[0].Distros[0].Version, "18.04")
-		})
+		when("translating stacks to targets", func() {
+			when("older buildpacks", func() {
+				when("there is only bionic", func() {
+					it("creates a target", func() {
+						path := filepath.Join("testdata", "buildpack", "by-id", "B", "v1", "buildpack.toml")
+						descriptor, err := buildpack.ReadBpDescriptor(path)
+						h.AssertNil(t, err)
+						// common sanity checks
+						h.AssertEq(t, descriptor.WithAPI, "0.7")
+						h.AssertEq(t, descriptor.Buildpack.ID, "B")
+						h.AssertEq(t, descriptor.Buildpack.Name, "Buildpack B")
+						h.AssertEq(t, descriptor.Buildpack.Version, "v1")
+						h.AssertEq(t, descriptor.Buildpack.Homepage, "Buildpack B Homepage")
+						h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
+						// specific behaviors for this test
+						h.AssertEq(t, descriptor.Stacks[0].ID, "io.buildpacks.stacks.bionic")
+						h.AssertEq(t, len(descriptor.Targets), 1)
+						h.AssertEq(t, descriptor.Targets[0].Arch, "amd64")
+						h.AssertEq(t, descriptor.Targets[0].OS, "linux")
+						h.AssertEq(t, descriptor.Targets[0].Distros[0].Name, "ubuntu")
+						h.AssertEq(t, descriptor.Targets[0].Distros[0].Version, "18.04")
+					})
+				})
 
-		it("translates one special stack value into target values", func() {
-			path := filepath.Join("testdata", "buildpack", "by-id", "B", "v2", "buildpack.toml")
-			descriptor, err := buildpack.ReadBpDescriptor(path)
-			h.AssertNil(t, err)
-			// common sanity checks
-			h.AssertEq(t, descriptor.WithAPI, "0.12")
-			h.AssertEq(t, descriptor.Buildpack.ID, "B")
-			h.AssertEq(t, descriptor.Buildpack.Name, "Buildpack B")
-			h.AssertEq(t, descriptor.Buildpack.Version, "v1")
-			h.AssertEq(t, descriptor.Buildpack.Homepage, "Buildpack B Homepage")
-			h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
-			// specific behaviors for this test
-			h.AssertEq(t, descriptor.Stacks[0].ID, "io.buildpacks.stacks.bionic")
-			h.AssertEq(t, len(descriptor.Targets), 1)
-			h.AssertEq(t, descriptor.Targets[0].Arch, "amd64")
-			h.AssertEq(t, descriptor.Targets[0].OS, "linux")
-			h.AssertEq(t, descriptor.Targets[0].Distros[0].Name, "ubuntu")
-			h.AssertEq(t, descriptor.Targets[0].Distros[0].Version, "18.04")
+				when("there are multiple stacks", func() {
+					it("does NOT create a target", func() {
+						path := filepath.Join("testdata", "buildpack", "by-id", "B", "v1.2", "buildpack.toml")
+						descriptor, err := buildpack.ReadBpDescriptor(path)
+						h.AssertNil(t, err)
+						// common sanity checks
+						h.AssertEq(t, descriptor.WithAPI, "0.7")
+						h.AssertEq(t, descriptor.Buildpack.ID, "B")
+						h.AssertEq(t, descriptor.Buildpack.Name, "Buildpack B")
+						h.AssertEq(t, descriptor.Buildpack.Version, "v1.2")
+						h.AssertEq(t, descriptor.Buildpack.Homepage, "Buildpack B Homepage")
+						h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
+						// specific behaviors for this test
+						h.AssertEq(t, descriptor.Stacks[0].ID, "io.buildpacks.stacks.bionic")
+						h.AssertEq(t, len(descriptor.Targets), 0)
+					})
+				})
+
+				when("there is a wildcard stack", func() {
+					it("creates a wildcard target", func() {
+						path := filepath.Join("testdata", "buildpack", "by-id", "B", "v1.star", "buildpack.toml")
+						descriptor, err := buildpack.ReadBpDescriptor(path)
+						h.AssertNil(t, err)
+						// common sanity checks
+						h.AssertEq(t, descriptor.WithAPI, "0.7")
+						h.AssertEq(t, descriptor.Buildpack.ID, "B")
+						h.AssertEq(t, descriptor.Buildpack.Name, "Buildpack B")
+						h.AssertEq(t, descriptor.Buildpack.Version, "v1.star")
+						h.AssertEq(t, descriptor.Buildpack.Homepage, "Buildpack B Homepage")
+						h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
+						// specific behaviors for this test
+						h.AssertEq(t, descriptor.Stacks[0].ID, "*")
+						h.AssertEq(t, len(descriptor.Targets), 1)
+						// a target that is completely empty will always match whatever is the base image target
+						h.AssertEq(t, descriptor.Targets[0].Arch, "")
+						h.AssertEq(t, descriptor.Targets[0].OS, "")
+						h.AssertEq(t, descriptor.Targets[0].ArchVariant, "")
+						h.AssertEq(t, len(descriptor.Targets[0].Distros), 0)
+					})
+				})
+			})
+
+			when("newer buildpacks", func() {
+				when("there is only bionic", func() {
+					it("creates a target", func() {
+						path := filepath.Join("testdata", "buildpack", "by-id", "B", "v2", "buildpack.toml")
+						descriptor, err := buildpack.ReadBpDescriptor(path)
+						h.AssertNil(t, err)
+						// common sanity checks
+						h.AssertEq(t, descriptor.WithAPI, "0.12")
+						h.AssertEq(t, descriptor.Buildpack.ID, "B")
+						h.AssertEq(t, descriptor.Buildpack.Name, "Buildpack B")
+						h.AssertEq(t, descriptor.Buildpack.Version, "v2")
+						h.AssertEq(t, descriptor.Buildpack.Homepage, "Buildpack B Homepage")
+						h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
+						// specific behaviors for this test
+						h.AssertEq(t, descriptor.Stacks[0].ID, "io.buildpacks.stacks.bionic")
+						h.AssertEq(t, len(descriptor.Targets), 1)
+						h.AssertEq(t, descriptor.Targets[0].Arch, "amd64")
+						h.AssertEq(t, descriptor.Targets[0].OS, "linux")
+						h.AssertEq(t, descriptor.Targets[0].Distros[0].Name, "ubuntu")
+						h.AssertEq(t, descriptor.Targets[0].Distros[0].Version, "18.04")
+					})
+				})
+
+				when("there are multiple stacks", func() {
+					it("creates a target", func() {
+						path := filepath.Join("testdata", "buildpack", "by-id", "B", "v2.2", "buildpack.toml")
+						descriptor, err := buildpack.ReadBpDescriptor(path)
+						h.AssertNil(t, err)
+						// common sanity checks
+						h.AssertEq(t, descriptor.WithAPI, "0.12")
+						h.AssertEq(t, descriptor.Buildpack.ID, "B")
+						h.AssertEq(t, descriptor.Buildpack.Name, "Buildpack B")
+						h.AssertEq(t, descriptor.Buildpack.Version, "v2.2")
+						h.AssertEq(t, descriptor.Buildpack.Homepage, "Buildpack B Homepage")
+						h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
+						// specific behaviors for this test
+						h.AssertEq(t, descriptor.Stacks[0].ID, "io.buildpacks.stacks.bionic")
+						h.AssertEq(t, len(descriptor.Targets), 1)
+						h.AssertEq(t, descriptor.Targets[0].Arch, "amd64")
+						h.AssertEq(t, descriptor.Targets[0].OS, "linux")
+						h.AssertEq(t, descriptor.Targets[0].Distros[0].Name, "ubuntu")
+						h.AssertEq(t, descriptor.Targets[0].Distros[0].Version, "18.04")
+					})
+				})
+
+				when("there is a wildcard stack", func() {
+					it("creates a wildcard target", func() {
+						path := filepath.Join("testdata", "buildpack", "by-id", "B", "v2.star", "buildpack.toml")
+						descriptor, err := buildpack.ReadBpDescriptor(path)
+						h.AssertNil(t, err)
+						// common sanity checks
+						h.AssertEq(t, descriptor.WithAPI, "0.12")
+						h.AssertEq(t, descriptor.Buildpack.ID, "B")
+						h.AssertEq(t, descriptor.Buildpack.Name, "Buildpack B")
+						h.AssertEq(t, descriptor.Buildpack.Version, "v2.star")
+						h.AssertEq(t, descriptor.Buildpack.Homepage, "Buildpack B Homepage")
+						h.AssertEq(t, descriptor.Buildpack.SBOM, []string{"application/vnd.cyclonedx+json"})
+						// specific behaviors for this test
+						h.AssertEq(t, descriptor.Stacks[0].ID, "*")
+						h.AssertEq(t, len(descriptor.Targets), 1)
+						// a target that is completely empty will always match whatever is the base image target
+						h.AssertEq(t, descriptor.Targets[0].Arch, "")
+						h.AssertEq(t, descriptor.Targets[0].OS, "")
+						h.AssertEq(t, descriptor.Targets[0].ArchVariant, "")
+						h.AssertEq(t, len(descriptor.Targets[0].Distros), 0)
+					})
+				})
+			})
 		})
 
 		it("does not translate non-special stack values", func() {

diff --git a/buildpack/testdata/buildpack/by-id/B/v1.2/buildpack.toml b/buildpack/testdata/buildpack/by-id/B/v1.2/buildpack.toml
@@ -0,0 +1,14 @@
+api = "0.7"
+
+[buildpack]
+id = "B"
+name = "Buildpack B"
+version = "v1.2"
+homepage = "Buildpack B Homepage"
+sbom-formats = ["application/vnd.cyclonedx+json"]
+
+[[stacks]]
+id = "io.buildpacks.stacks.bionic"
+
+[[stacks]]
+id = "io.buildpacks.stacks.jammy"
diff --git a/buildpack/testdata/buildpack/by-id/B/v1.star/buildpack.toml b/buildpack/testdata/buildpack/by-id/B/v1.star/buildpack.toml
@@ -0,0 +1,11 @@
+api = "0.7"
+
+[buildpack]
+id = "B"
+name = "Buildpack B"
+version = "v1.star"
+homepage = "Buildpack B Homepage"
+sbom-formats = ["application/vnd.cyclonedx+json"]
+
+[[stacks]]
+id = "*"
diff --git a/buildpack/testdata/buildpack/by-id/B/v2.2/buildpack.toml b/buildpack/testdata/buildpack/by-id/B/v2.2/buildpack.toml
@@ -0,0 +1,14 @@
+api = "0.12"
+
+[buildpack]
+id = "B"
+name = "Buildpack B"
+version = "v2.2"
+homepage = "Buildpack B Homepage"
+sbom-formats = ["application/vnd.cyclonedx+json"]
+
+[[stacks]]
+id = "io.buildpacks.stacks.bionic"
+
+[[stacks]]
+id = "io.buildpacks.stacks.jammy"
diff --git a/buildpack/testdata/buildpack/by-id/B/v2.star/buildpack.toml b/buildpack/testdata/buildpack/by-id/B/v2.star/buildpack.toml
@@ -0,0 +1,11 @@
+api = "0.12"
+
+[buildpack]
+id = "B"
+name = "Buildpack B"
+version = "v2.star"
+homepage = "Buildpack B Homepage"
+sbom-formats = ["application/vnd.cyclonedx+json"]
+
+[[stacks]]
+id = "*"
diff --git a/buildpack/testdata/buildpack/by-id/B/v2/buildpack.toml b/buildpack/testdata/buildpack/by-id/B/v2/buildpack.toml
@@ -3,7 +3,7 @@ api = "0.12"
 [buildpack]
 id = "B"
 name = "Buildpack B"
-version = "v1"
+version = "v2"
 homepage = "Buildpack B Homepage"
 sbom-formats = ["application/vnd.cyclonedx+json"]
 

diff --git a/phase/detector.go b/phase/detector.go
@@ -144,26 +144,14 @@ func (d *Detector) detectOrder(order buildpack.Order, done, next []buildpack.Gro
 	return nil, nil, ErrFailedDetection
 }
 
-// isWildcard returns true IFF the Arch and OS are unspecified, meaning that the target arch/os are "any"
-func isWildcard(t files.TargetMetadata) bool {
-	return t.Arch == "" && t.OS == ""
-}
-
-func hasWildcard(ts []buildpack.TargetMetadata) bool {
-	for _, tm := range ts {
-		if tm.OS == "" && tm.Arch == "" {
-			return true
-		}
-	}
-	return false
-}
-
 func (d *Detector) detectGroup(group buildpack.Group, done []buildpack.GroupElement, wg *sync.WaitGroup) ([]buildpack.GroupElement, []files.BuildPlanEntry, error) {
 	// used below to mark each item as "done" by appending it to the done list
 	markDone := func(groupEl buildpack.GroupElement, descriptor buildpack.Descriptor) {
 		done = append(done, groupEl.WithAPI(descriptor.API()).WithHomepage(descriptor.Homepage()))
 	}
 
+	runImageTargetInfo := d.AnalyzeMD.RunImageTarget()
+
 	for i, groupEl := range group.Group {
 		// Continue if element has already been processed.
 		if hasIDForKind(done, groupEl.Kind(), groupEl.ID) {
@@ -175,7 +163,7 @@ func (d *Detector) detectGroup(group buildpack.Group, done []buildpack.GroupElem
 			return d.detectOrder(groupEl.OrderExtensions, done, group.Group[i+1:], true, wg)
 		}
 
-		// Lookup element in store.  <-- "the store" is the directory where all the buildpacks are.
+		// Lookup element in store (the "store" is the directory where all the buildpacks are).
 		var (
 			descriptor buildpack.Descriptor
 			err        error
@@ -192,26 +180,31 @@ func (d *Detector) detectGroup(group buildpack.Group, done []buildpack.GroupElem
 				// FIXME: cyclical references lead to infinite recursion
 				return d.detectOrder(order, done, group.Group[i+1:], groupEl.Optional, wg)
 			}
-			descriptor = bpDescriptor // standardize the type so below we don't have to care whether it was an extension
+			descriptor = bpDescriptor // Standardize the type so below we don't have to care whether it is an extension.
 		} else {
 			descriptor, err = d.DirStore.LookupExt(groupEl.ID, groupEl.Version)
 			if err != nil {
 				return nil, nil, err
 			}
 		}
+
+		// Check target compatibility.
 		if d.PlatformAPI.AtLeast("0.12") {
-			targetMatch := false
-			if isWildcard(d.AnalyzeMD.RunImageTarget()) || hasWildcard(descriptor.TargetsList()) {
+			var targetMatch bool
+			if len(descriptor.TargetsList()) == 0 {
+				// This is actually just for tests. In practice, the lifecycle will infer a target with at least an OS
+				// when targets are missing from buildpack.toml.
 				targetMatch = true
 			} else {
-				for i := range descriptor.TargetsList() {
-					d.Logger.Debugf("Checking for match against descriptor: %s", descriptor.TargetsList()[i])
-					if platform.TargetSatisfiedForBuild(&fsutil.Detect{}, *d.AnalyzeMD.RunImage.TargetMetadata, descriptor.TargetsList()[i], d.Logger) {
+				for _, target := range descriptor.TargetsList() {
+					d.Logger.Debugf("Checking for match against descriptor: %s", target)
+					if platform.TargetSatisfiedForBuild(&fsutil.Detect{}, &runImageTargetInfo, target, d.Logger) {
 						targetMatch = true
 						break
 					}
 				}
 			}
+
 			if !targetMatch && !groupEl.Optional {
 				markDone(groupEl, descriptor)
 				d.Runs.Store(
@@ -220,7 +213,7 @@ func (d *Detector) detectGroup(group buildpack.Group, done []buildpack.GroupElem
 						Code: -1,
 						Err: fmt.Errorf(
 							"unable to satisfy target os/arch constraints; run image: %s, buildpack: %s",
-							encoding.ToJSONMaybe(d.AnalyzeMD.RunImage.TargetMetadata),
+							encoding.ToJSONMaybe(runImageTargetInfo),
 							encoding.ToJSONMaybe(descriptor.TargetsList()),
 						),
 					})
@@ -240,7 +233,7 @@ func (d *Detector) detectGroup(group buildpack.Group, done []buildpack.GroupElem
 					BuildConfigDir: d.BuildConfigDir,
 					PlatformDir:    d.PlatformDir,
 					Env:            env.NewBuildEnv(os.Environ()),
-					TargetEnv:      platform.EnvVarsFor(&fsutil.Detect{}, d.AnalyzeMD.RunImageTarget(), d.Logger),
+					TargetEnv:      platform.EnvVarsFor(&fsutil.Detect{}, runImageTargetInfo, d.Logger),
 				}
 				d.Runs.Store(key, d.Executor.Detect(descriptor, inputs, d.Logger)) // this is where we finally invoke bin/detect
 			}