From a675dc3bd1fe1a5e355d4f7cdee8de8e9e8305c4 Mon Sep 17 00:00:00 2001 From: Natalie Arellano Date: Thu, 3 Nov 2022 10:08:00 -0400 Subject: [PATCH] Fix a few bugs in lifecycle execution Fix (restore): when gid is provided, it shouldn't override other flags Fix (analyze): when cache image is used, flags should not also contain -cache-dir Fix (restore): when cache image is used, flags should not also contain -cache-dir - Variable rename (ops for operations) Fix (restore): provide registry credentials when using a cache image Signed-off-by: Natalie Arellano --- internal/build/lifecycle_execution.go | 108 +++++++++++---------- internal/build/lifecycle_execution_test.go | 26 ++--- 2 files changed, 66 insertions(+), 68 deletions(-) diff --git a/internal/build/lifecycle_execution.go b/internal/build/lifecycle_execution.go index ddb2c5783..379a43113 100644 --- 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 index a04ab3888..812b6bb9b 100644 --- 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,9 +1308,6 @@ 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") @@ -1326,6 +1315,11 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { 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={}") }) })