Merge pull request #1542 from buildpacks/fix/few-bugs

Fix a few bugs
buildpacks · Nov 6, 2022 · 8f8cdb3 · 8f8cdb3
2 parents 69fd327 + a675dc3
commit 8f8cdb3
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 68 deletions.
diff --git a/internal/build/lifecycle_execution.go b/internal/build/lifecycle_execution.go
@@ -252,13 +252,13 @@ func (l *LifecycleExecution) Create(ctx context.Context, buildCache, launchCache
 		flags = append(flags, "-process-type", processType)
 	}
 
-	var cacheOpts PhaseConfigProviderOperation
+	var cacheBindOp PhaseConfigProviderOperation
 	switch buildCache.Type() {
 	case cache.Image:
 		flags = append(flags, "-cache-image", buildCache.Name())
-		cacheOpts = WithBinds(l.opts.Volumes...)
+		cacheBindOp = WithBinds(l.opts.Volumes...)
 	case cache.Volume, cache.Bind:
-		cacheOpts = WithBinds(append(l.opts.Volumes, fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir()))...)
+		cacheBindOp = WithBinds(append(l.opts.Volumes, fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir()))...)
 	}
 
 	withEnv := NullOp()
@@ -270,7 +270,7 @@ func (l *LifecycleExecution) Create(ctx context.Context, buildCache, launchCache
 		WithFlags(l.withLogLevel(flags...)...),
 		WithArgs(l.opts.Image.String()),
 		WithNetwork(l.opts.Network),
-		cacheOpts,
+		cacheBindOp,
 		WithContainerOperations(WriteProjectMetadata(l.mountPaths.projectPath(), l.opts.ProjectMetadata, l.os)),
 		WithContainerOperations(CopyDir(l.opts.AppPath, l.mountPaths.appDir(), l.opts.Builder.UID(), l.opts.Builder.GID(), l.os, true, l.opts.FileFilter)),
 		If(l.opts.SBOMDestinationDir != "", WithPostContainerRunOperations(
@@ -326,18 +326,27 @@ func (l *LifecycleExecution) Detect(ctx context.Context, phaseFactory PhaseFacto
 }
 
 func (l *LifecycleExecution) Restore(ctx context.Context, buildCache Cache, phaseFactory PhaseFactory) error {
-	flagsOpt := NullOp()
-	cacheOpt := NullOp()
+	var flags []string
+	cacheBindOp := NullOp()
+	registryOp := NullOp()
 	switch buildCache.Type() {
 	case cache.Image:
-		flagsOpt = WithFlags("-cache-image", buildCache.Name())
+		flags = append(flags, "-cache-image", buildCache.Name())
+		authConfig, err := auth.BuildEnvVar(authn.DefaultKeychain, l.opts.CacheImage)
+		if err != nil {
+			return err
+		}
+		registryOp = WithRegistryAccess(authConfig)
 	case cache.Volume:
-		cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir()))
+		flags = append(flags, "-cache-dir", l.mountPaths.cacheDir())
+		cacheBindOp = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir()))
 	}
 	if l.opts.GID >= overrideGID {
-		flagsOpt = WithFlags("-gid", strconv.Itoa(l.opts.GID))
+		flags = append(flags, "-gid", strconv.Itoa(l.opts.GID))
 	}
 
+	flagsOp := WithFlags(flags...)
+
 	configProvider := NewPhaseConfigProvider(
 		"restorer",
 		l,
@@ -346,13 +355,12 @@ func (l *LifecycleExecution) Restore(ctx context.Context, buildCache Cache, phas
 		WithEnv(fmt.Sprintf("%s=%d", builder.EnvUID, l.opts.Builder.UID()), fmt.Sprintf("%s=%d", builder.EnvGID, l.opts.Builder.GID())),
 		WithRoot(), // remove after platform API 0.2 is no longer supported
 		WithArgs(
-			l.withLogLevel(
-				"-cache-dir", l.mountPaths.cacheDir(),
-			)...,
+			l.withLogLevel()...,
 		),
 		WithNetwork(l.opts.Network),
-		flagsOpt,
-		cacheOpt,
+		flagsOp,
+		cacheBindOp,
+		registryOp,
 	)
 
 	restore := phaseFactory.New(configProvider)
@@ -361,44 +369,37 @@ func (l *LifecycleExecution) Restore(ctx context.Context, buildCache Cache, phas
 }
 
 func (l *LifecycleExecution) Analyze(ctx context.Context, buildCache, launchCache Cache, phaseFactory PhaseFactory) error {
-	args := []string{
-		l.opts.Image.String(),
-	}
+	var flags []string
+	args := []string{l.opts.Image.String()}
 	platformAPILessThan07 := l.platformAPI.LessThan("0.7")
-	if platformAPILessThan07 {
-		if l.opts.ClearCache {
+
+	cacheBindOp := NullOp()
+	if l.opts.ClearCache {
+		if platformAPILessThan07 || l.platformAPI.AtLeast("0.9") {
 			args = prependArg("-skip-layers", args)
-		} else {
-			args = append([]string{"-cache-dir", l.mountPaths.cacheDir()}, args...)
+		}
+	} else {
+		switch buildCache.Type() {
+		case cache.Image:
+			flags = append(flags, "-cache-image", buildCache.Name())
+		case cache.Volume:
+			if platformAPILessThan07 {
+				args = append([]string{"-cache-dir", l.mountPaths.cacheDir()}, args...)
+				cacheBindOp = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir()))
+			}
 		}
 	}
 
-	launchCacheOpt := NullOp()
+	launchCacheBindOp := NullOp()
 	if l.platformAPI.AtLeast("0.9") {
-		if l.opts.ClearCache {
-			args = prependArg("-skip-layers", args)
-		}
 		if !l.opts.Publish {
 			args = append([]string{"-launch-cache", l.mountPaths.launchCacheDir()}, args...)
-			launchCacheOpt = WithBinds(fmt.Sprintf("%s:%s", launchCache.Name(), l.mountPaths.launchCacheDir()))
-		}
-	}
-
-	cacheOpt := NullOp()
-	flagsOpt := NullOp()
-	switch buildCache.Type() {
-	case cache.Image:
-		if !l.opts.ClearCache {
-			flagsOpt = WithFlags("-cache-image", buildCache.Name())
-		}
-	case cache.Volume:
-		if platformAPILessThan07 {
-			cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir()))
+			launchCacheBindOp = WithBinds(fmt.Sprintf("%s:%s", launchCache.Name(), l.mountPaths.launchCacheDir()))
 		}
 	}
 
 	if l.opts.GID >= overrideGID {
-		flagsOpt = WithFlags("-gid", strconv.Itoa(l.opts.GID))
+		flags = append(flags, "-gid", strconv.Itoa(l.opts.GID))
 	}
 
 	if l.opts.PreviousImage != "" {
@@ -428,7 +429,8 @@ func (l *LifecycleExecution) Analyze(ctx context.Context, buildCache, launchCach
 			args = append([]string{"-previous-image", l.opts.PreviousImage}, args...)
 		}
 	}
-	stackOpts := NullOp()
+
+	stackOp := NullOp()
 	if !platformAPILessThan07 {
 		for _, tag := range l.opts.AdditionalTags {
 			args = append([]string{"-tag", tag}, args...)
@@ -437,9 +439,11 @@ func (l *LifecycleExecution) Analyze(ctx context.Context, buildCache, launchCach
 			args = append([]string{"-run-image", l.opts.RunImage}, args...)
 		}
 		args = append([]string{"-stack", l.mountPaths.stackPath()}, args...)
-		stackOpts = WithContainerOperations(WriteStackToml(l.mountPaths.stackPath(), l.opts.Builder.Stack(), l.os))
+		stackOp = WithContainerOperations(WriteStackToml(l.mountPaths.stackPath(), l.opts.Builder.Stack(), l.os))
 	}
 
+	flagsOp := WithFlags(flags...)
+
 	var analyze RunnerCleaner
 	if l.opts.Publish {
 		authConfig, err := auth.BuildEnvVar(authn.DefaultKeychain, l.opts.Image.String(), l.opts.RunImage, l.opts.CacheImage, l.opts.PreviousImage)
@@ -457,9 +461,9 @@ func (l *LifecycleExecution) Analyze(ctx context.Context, buildCache, launchCach
 			WithRoot(),
 			WithArgs(l.withLogLevel(args...)...),
 			WithNetwork(l.opts.Network),
-			flagsOpt,
-			cacheOpt,
-			stackOpts,
+			flagsOp,
+			cacheBindOp,
+			stackOp,
 		)
 
 		analyze = phaseFactory.New(configProvider)
@@ -474,13 +478,13 @@ func (l *LifecycleExecution) Analyze(ctx context.Context, buildCache, launchCach
 				fmt.Sprintf("%s=%d", builder.EnvGID, l.opts.Builder.GID()),
 			),
 			WithDaemonAccess(l.opts.DockerHost),
-			launchCacheOpt,
+			launchCacheBindOp,
 			WithFlags(l.withLogLevel("-daemon")...),
 			WithArgs(args...),
-			flagsOpt,
+			flagsOp,
 			WithNetwork(l.opts.Network),
-			cacheOpt,
-			stackOpts,
+			cacheBindOp,
+			stackOp,
 		)
 
 		analyze = phaseFactory.New(configProvider)
@@ -537,12 +541,12 @@ func (l *LifecycleExecution) Export(ctx context.Context, buildCache, launchCache
 		flags = append(flags, "-gid", strconv.Itoa(l.opts.GID))
 	}
 
-	cacheOpt := NullOp()
+	cacheBindOp := NullOp()
 	switch buildCache.Type() {
 	case cache.Image:
 		flags = append(flags, "-cache-image", buildCache.Name())
 	case cache.Volume:
-		cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir()))
+		cacheBindOp = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir()))
 	}
 
 	withEnv := NullOp()
@@ -563,7 +567,7 @@ func (l *LifecycleExecution) Export(ctx context.Context, buildCache, launchCache
 		WithArgs(append([]string{l.opts.Image.String()}, l.opts.AdditionalTags...)...),
 		WithRoot(),
 		WithNetwork(l.opts.Network),
-		cacheOpt,
+		cacheBindOp,
 		WithContainerOperations(WriteStackToml(l.mountPaths.stackPath(), l.opts.Builder.Stack(), l.os)),
 		WithContainerOperations(WriteProjectMetadata(l.mountPaths.projectPath(), l.opts.ProjectMetadata, l.os)),
 		If(l.opts.SBOMDestinationDir != "", WithPostContainerRunOperations(

diff --git a/internal/build/lifecycle_execution_test.go b/internal/build/lifecycle_execution_test.go
@@ -55,7 +55,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
 		providedAdditionalTags = []string{"some-additional-tag1", "some-additional-tag2"}
 		providedVolumes        = []string{"some-mount-source:/some-mount-target"}
 
-		platformAPI = build.SupportedPlatformAPIVersions[0]
+		platformAPI = build.SupportedPlatformAPIVersions[0] // TODO: update the tests to target the latest api by default and make earlier apis special cases
 		providedUID = 2222
 		providedGID = 3333
 
@@ -947,10 +947,6 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
 			})
 
 			when("using a cache image", func() {
-				lifecycleOps = append(lifecycleOps, func(options *build.LifecycleOptions) {
-					options.GID = -1 // npa: why is this needed?
-				})
-
 				fakeBuildCache = newFakeImageCache()
 
 				it("configures the phase with a build cache image", func() {
@@ -959,9 +955,9 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
 						configProvider.ContainerConfig().Cmd,
 						[]string{"-cache-image", "some-cache-image"},
 					)
-					h.AssertIncludeAllExpectedPatterns(t,
+					h.AssertSliceNotContains(t,
 						configProvider.ContainerConfig().Cmd,
-						[]string{"-cache-dir", "/cache"},
+						"-cache-dir",
 					)
 				})
 
@@ -1061,19 +1057,15 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
 			when("using a cache image", func() {
 				fakeBuildCache = newFakeImageCache()
 
-				lifecycleOps = append(lifecycleOps, func(options *build.LifecycleOptions) {
-					options.GID = -1
-				})
-
 				it("configures the phase with a build cache images", func() {
 					h.AssertSliceNotContains(t, configProvider.HostConfig().Binds, ":/cache")
 					h.AssertIncludeAllExpectedPatterns(t,
 						configProvider.ContainerConfig().Cmd,
 						[]string{"-cache-image", "some-cache-image"},
 					)
-					h.AssertIncludeAllExpectedPatterns(t,
+					h.AssertSliceNotContains(t,
 						configProvider.ContainerConfig().Cmd,
-						[]string{"-cache-dir", "/cache"}, // npa: is this a mistake?
+						"-cache-dir",
 					)
 				})
 			})
@@ -1316,16 +1308,18 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) {
 
 		when("using cache image", func() {
 			fakeBuildCache = newFakeImageCache()
-			lifecycleOps = append(lifecycleOps, func(options *build.LifecycleOptions) {
-				options.GID = -1
-			})
 
 			it("configures the phase with a cache image", func() {
 				h.AssertSliceNotContains(t, configProvider.HostConfig().Binds, ":/cache")
 				h.AssertIncludeAllExpectedPatterns(t,
 					configProvider.ContainerConfig().Cmd,
 					[]string{"-cache-image", "some-cache-image"},
 				)
+				h.AssertSliceNotContains(t, configProvider.ContainerConfig().Cmd, "-cache-dir")
+			})
+
+			it("configures the phase with registry access", func() {
+				h.AssertSliceContains(t, configProvider.ContainerConfig().Env, "CNB_REGISTRY_AUTH={}")
 			})
 		})