From 61c0dd922f7a5eee94189c851d94af60745f4611 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Fri, 21 May 2021 16:37:32 -0500 Subject: [PATCH 1/5] Adding --gid flag to override the user's group id on the build and execution image Signed-off-by: Juan Bustamante --- build.go | 12 ++ build_test.go | 56 +++++++ internal/build/lifecycle_execution.go | 15 ++ internal/build/lifecycle_execution_test.go | 180 +++++++++++++++++++++ internal/build/lifecycle_executor.go | 2 + internal/commands/build.go | 11 ++ internal/commands/build_test.go | 40 +++++ 7 files changed, 316 insertions(+) diff --git a/build.go b/build.go index cb04938c7..2fb4ded04 100644 --- a/build.go +++ b/build.go @@ -122,6 +122,9 @@ type BuildOptions struct { // Only trust builders from reputable sources. TrustBuilder bool + // The build and stack user's group id must be override + OverrideGroupID bool + // List of buildpack images or archives to add to a builder. // These buildpacks may overwrite those on the builder if they // share both an ID and Version with a buildpack on the builder. @@ -156,6 +159,9 @@ type BuildOptions struct { // The location at which to mount the AppDir in the build image. Workspace string + + // User's group id used to build the image + GroupID int } // ProxyConfig specifies proxy setting to be set as environment variables in a container. @@ -284,6 +290,10 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error { return err } + if opts.GroupID < 0 { + return errors.New("gid flag must be in the range of 0-2147483647") + } + lifecycleOpts := build.LifecycleOptions{ AppPath: appPath, Image: imageRef, @@ -305,6 +315,8 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error { FileFilter: fileFilter, CacheImage: opts.CacheImage, Workspace: opts.Workspace, + OverrideGID: opts.OverrideGroupID, + GID: opts.GroupID, } lifecycleVersion := ephemeralBuilder.LifecycleDescriptor().Info.Version diff --git a/build_test.go b/build_test.go index 86e6ac4fa..9fcd18611 100644 --- a/build_test.go +++ b/build_test.go @@ -2191,6 +2191,62 @@ func testBuild(t *testing.T, when spec.G, it spec.S) { }) }) }) + + when("GID option", func() { + when("gid is set and override is false", func() { + it("gid override must be false", func() { + h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ + Workspace: "app", + Builder: defaultBuilderName, + Image: "example.com/some/repo:tag", + GroupID: 0, + OverrideGroupID: false, + })) + h.AssertEq(t, fakeLifecycle.Opts.OverrideGID, false) + h.AssertEq(t, fakeLifecycle.Opts.GID, 0) + }) + }) + + when("gid is set and override is true", func() { + it("gid override must be true", func() { + h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ + Workspace: "app", + Builder: defaultBuilderName, + Image: "example.com/some/repo:tag", + GroupID: 1, + OverrideGroupID: true, + })) + h.AssertEq(t, fakeLifecycle.Opts.OverrideGID, true) + h.AssertEq(t, fakeLifecycle.Opts.GID, 1) + }) + }) + + when("gid is negative and override is true", func() { + it("should thrown error", func() { + err := subject.Build(context.TODO(), BuildOptions{ + Workspace: "app", + Builder: defaultBuilderName, + Image: "example.com/some/repo:tag", + GroupID: -1, + OverrideGroupID: true, + }) + h.AssertError(t, err, "gid flag must be in the range of 0-2147483647") + }) + }) + + when("gid is negative and override is false", func() { + it("should thrown error", func() { + err := subject.Build(context.TODO(), BuildOptions{ + Workspace: "app", + Builder: defaultBuilderName, + Image: "example.com/some/repo:tag", + GroupID: -1, + OverrideGroupID: false, + }) + h.AssertError(t, err, "gid flag must be in the range of 0-2147483647") + }) + }) + }) }) } diff --git a/internal/build/lifecycle_execution.go b/internal/build/lifecycle_execution.go index e0d96ebc0..8135a1722 100644 --- a/internal/build/lifecycle_execution.go +++ b/internal/build/lifecycle_execution.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "math/rand" + "strconv" "github.com/buildpacks/lifecycle/api" "github.com/buildpacks/lifecycle/auth" @@ -181,6 +182,10 @@ func (l *LifecycleExecution) Create(ctx context.Context, publish bool, dockerHos flags = append(flags, "-skip-restore") } + if l.opts.OverrideGID { + flags = append(flags, "-gid", strconv.Itoa(l.opts.GID)) + } + processType := determineDefaultProcessType(l.platformAPI, l.opts.DefaultProcessType) if processType != "" { flags = append(flags, "-process-type", processType) @@ -253,6 +258,9 @@ func (l *LifecycleExecution) Restore(ctx context.Context, networkMode string, bu case cache.Volume: cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir())) } + if l.opts.OverrideGID { + flagsOpt = WithFlags("-gid", strconv.Itoa(l.opts.GID)) + } configProvider := NewPhaseConfigProvider( "restorer", @@ -306,6 +314,10 @@ func (l *LifecycleExecution) newAnalyze(repoName, networkMode string, publish bo cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir())) } + if l.opts.OverrideGID { + flagsOpt = WithFlags("-gid", strconv.Itoa(l.opts.GID)) + } + if publish { authConfig, err := auth.BuildEnvVar(authn.DefaultKeychain, repoName) if err != nil { @@ -391,6 +403,9 @@ func (l *LifecycleExecution) newExport(repoName, runImage string, publish bool, if processType != "" { flags = append(flags, "-process-type", processType) } + if l.opts.OverrideGID { + flags = append(flags, "-gid", strconv.Itoa(l.opts.GID)) + } cacheOpt := NullOp() switch buildCache.Type() { diff --git a/internal/build/lifecycle_execution_test.go b/internal/build/lifecycle_execution_test.go index ea26c0a6d..1c7342171 100644 --- a/internal/build/lifecycle_execution_test.go +++ b/internal/build/lifecycle_execution_test.go @@ -714,6 +714,50 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { }) }) }) + + when("override GID", func() { + when("override GID is true", func() { + it("configures the phase with the expected arguments", func() { + verboseLifecycle := newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { + options.OverrideGID = true + options.GID = 0 + }) + fakePhaseFactory := fakes.NewFakePhaseFactory() + + err := verboseLifecycle.Create(context.Background(), false, "", true, "test", "test", "test", fakeBuildCache, fakeLaunchCache, []string{}, []string{}, fakePhaseFactory) + h.AssertNil(t, err) + + lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 + h.AssertNotEq(t, lastCallIndex, -1) + + configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] + h.AssertEq(t, configProvider.Name(), "creator") + h.AssertIncludeAllExpectedPatterns(t, + configProvider.ContainerConfig().Cmd, + []string{"-gid"}, + ) + }) + }) + when("override GID is false", func() { + it("gid is not added to the expected arguments", func() { + verboseLifecycle := newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { + options.OverrideGID = false + options.GID = 0 + }) + fakePhaseFactory := fakes.NewFakePhaseFactory() + + err := verboseLifecycle.Create(context.Background(), false, "", true, "test", "test", "test", fakeBuildCache, fakeLaunchCache, []string{}, []string{}, fakePhaseFactory) + h.AssertNil(t, err) + + lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 + h.AssertNotEq(t, lastCallIndex, -1) + + configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] + h.AssertEq(t, configProvider.Name(), "creator") + h.AssertSliceNotContains(t, configProvider.ContainerConfig().Cmd, "-gid") + }) + }) + }) }) when("#Detect", func() { @@ -1134,6 +1178,51 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { h.AssertSliceContains(t, configProvider.HostConfig().Binds, expectedBind) }) }) + + when("override GID", func() { + var ( + lifecycle *build.LifecycleExecution + fakePhaseFactory *fakes.FakePhaseFactory + ) + fakePhase := &fakes.FakePhase{} + fakePhaseFactory = fakes.NewFakePhaseFactory(fakes.WhichReturnsForNew(fakePhase)) + + when("override GID is true", func() { + it.Before(func() { + lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { + options.OverrideGID = true + options.GID = 0 + }) + }) + it("", func() { + err := lifecycle.Analyze(context.Background(), "test", "test", false, "", false, fakeCache, fakePhaseFactory) + h.AssertNil(t, err) + lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 + h.AssertNotEq(t, lastCallIndex, -1) + configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] + h.AssertIncludeAllExpectedPatterns(t, + configProvider.ContainerConfig().Cmd, + []string{"-gid"}, + ) + }) + }) + when("override GID is false", func() { + it.Before(func() { + lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { + options.OverrideGID = false + options.GID = 0 + }) + }) + it("", func() { + err := lifecycle.Analyze(context.Background(), "test", "test", false, "", false, fakeCache, fakePhaseFactory) + h.AssertNil(t, err) + lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 + h.AssertNotEq(t, lastCallIndex, -1) + configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] + h.AssertSliceNotContains(t, configProvider.ContainerConfig().Cmd, "-gid") + }) + }) + }) }) when("#Restore", func() { @@ -1250,6 +1339,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] h.AssertSliceContains(t, configProvider.HostConfig().Binds, expectedBind) }) + when("using cache image", func() { var ( lifecycle *build.LifecycleExecution @@ -1278,6 +1368,51 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { ) }) }) + + when("override GID", func() { + var ( + lifecycle *build.LifecycleExecution + fakePhaseFactory *fakes.FakePhaseFactory + ) + fakePhase := &fakes.FakePhase{} + fakePhaseFactory = fakes.NewFakePhaseFactory(fakes.WhichReturnsForNew(fakePhase)) + + when("override GID is true", func() { + it.Before(func() { + lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { + options.OverrideGID = true + options.GID = 0 + }) + }) + it("", func() { + err := lifecycle.Restore(context.Background(), "test", fakeCache, fakePhaseFactory) + h.AssertNil(t, err) + lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 + h.AssertNotEq(t, lastCallIndex, -1) + configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] + h.AssertIncludeAllExpectedPatterns(t, + configProvider.ContainerConfig().Cmd, + []string{"-gid"}, + ) + }) + }) + when("override GID is false", func() { + it.Before(func() { + lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { + options.OverrideGID = false + options.GID = 0 + }) + }) + it("", func() { + err := lifecycle.Restore(context.Background(), "test", fakeCache, fakePhaseFactory) + h.AssertNil(t, err) + lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 + h.AssertNotEq(t, lastCallIndex, -1) + configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] + h.AssertSliceNotContains(t, configProvider.ContainerConfig().Cmd, "-gid") + }) + }) + }) }) when("#Build", func() { @@ -1828,6 +1963,51 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { }) }) }) + + when("override GID", func() { + var ( + lifecycle *build.LifecycleExecution + fakePhaseFactory *fakes.FakePhaseFactory + ) + fakePhase := &fakes.FakePhase{} + fakePhaseFactory = fakes.NewFakePhaseFactory(fakes.WhichReturnsForNew(fakePhase)) + + when("override GID is true", func() { + it.Before(func() { + lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { + options.OverrideGID = true + options.GID = 0 + }) + }) + it("", func() { + err := lifecycle.Export(context.Background(), "test", "test", false, "", "test", fakeBuildCache, fakeLaunchCache, []string{}, fakePhaseFactory) + h.AssertNil(t, err) + lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 + h.AssertNotEq(t, lastCallIndex, -1) + configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] + h.AssertIncludeAllExpectedPatterns(t, + configProvider.ContainerConfig().Cmd, + []string{"-gid"}, + ) + }) + }) + when("override GID is false", func() { + it.Before(func() { + lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { + options.OverrideGID = false + options.GID = 0 + }) + }) + it("", func() { + err := lifecycle.Export(context.Background(), "test", "test", false, "", "test", fakeBuildCache, fakeLaunchCache, []string{}, fakePhaseFactory) + h.AssertNil(t, err) + lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 + h.AssertNotEq(t, lastCallIndex, -1) + configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] + h.AssertSliceNotContains(t, configProvider.ContainerConfig().Cmd, "-gid") + }) + }) + }) }) } diff --git a/internal/build/lifecycle_executor.go b/internal/build/lifecycle_executor.go index 7344f9bac..8bf30ceef 100644 --- a/internal/build/lifecycle_executor.go +++ b/internal/build/lifecycle_executor.go @@ -58,6 +58,7 @@ type LifecycleOptions struct { Publish bool TrustBuilder bool UseCreator bool + OverrideGID bool DockerHost string CacheImage string HTTPProxy string @@ -69,6 +70,7 @@ type LifecycleOptions struct { DefaultProcessType string FileFilter func(string) bool Workspace string + GID int } func NewLifecycleExecutor(logger logging.Logger, docker client.CommonAPIClient) *LifecycleExecutor { diff --git a/internal/commands/build.go b/internal/commands/build.go index 3a45864bc..a37cef673 100644 --- a/internal/commands/build.go +++ b/internal/commands/build.go @@ -41,6 +41,7 @@ type BuildFlags struct { Volumes []string AdditionalTags []string Workspace string + GID int } // Build an image from source code @@ -121,6 +122,10 @@ func Build(logger logging.Logger, cfg config.Config, packClient PackClient) *cob } lifecycleImage = ref.Name() } + var overrideGID = false + if cmd.Flags().Changed("gid") { + overrideGID = true + } if err := packClient.Build(cmd.Context(), pack.BuildOptions{ AppPath: flags.AppPath, Builder: builder, @@ -146,6 +151,8 @@ func Build(logger logging.Logger, cfg config.Config, packClient PackClient) *cob CacheImage: flags.CacheImage, Workspace: flags.Workspace, LifecycleImage: lifecycleImage, + GroupID: flags.GID, + OverrideGroupID: overrideGID, }); err != nil { return errors.Wrap(err, "failed to build") } @@ -184,6 +191,7 @@ This option may set DOCKER_HOST environment variable for the build container if cmd.Flags().BoolVar(&buildFlags.TrustBuilder, "trust-builder", false, "Trust the provided builder\nAll lifecycle phases will be run in a single container (if supported by the lifecycle).") cmd.Flags().StringArrayVar(&buildFlags.Volumes, "volume", nil, "Mount host volume into the build container, in the form ':[:]'.\n- 'host path': Name of the volume or absolute directory path to mount.\n- 'target path': The path where the file or directory is available in the container.\n- 'options' (default \"ro\"): An optional comma separated list of mount options.\n - \"ro\", volume contents are read-only.\n - \"rw\", volume contents are readable and writeable.\n - \"volume-opt==\", can be specified more than once, takes a key-value pair consisting of the option name and its value."+multiValueHelp("volume")) cmd.Flags().StringVar(&buildFlags.Workspace, "workspace", "", "Location at which to mount the app dir in the build image") + cmd.Flags().IntVar(&buildFlags.GID, "gid", 0, `Override GID of user's group in the stack's build and run images, it must be a value in the range of 0-2147483647`) } func validateBuildFlags(flags *BuildFlags, cfg config.Config, packClient PackClient, logger logging.Logger) error { @@ -195,6 +203,9 @@ func validateBuildFlags(flags *BuildFlags, cfg config.Config, packClient PackCli return errors.New("cache-image flag requires the publish flag") } + if flags.GID < 0 { + return errors.New("gid flag must be in the range of 0-2147483647") + } return nil } diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 9cc6457eb..592936716 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -678,6 +678,37 @@ builder = "my-builder" h.AssertNil(t, command.Execute()) }) }) + + when("gid flag is provided", func() { + when("--gid is a valid value", func() { + it("override build option should be set to true", func() { + mockClient.EXPECT(). + Build(gomock.Any(), EqBuildOptionsWithOverrideGroupID(true, 1)). + Return(nil) + + command.SetArgs([]string{"--builder", "my-builder", "image", "--gid", "1"}) + h.AssertNil(t, command.Execute()) + }) + }) + when("--gid is an invalid value", func() { + it("error must be thrown", func() { + command.SetArgs([]string{"--builder", "my-builder", "image", "--gid", "-1"}) + err := command.Execute() + h.AssertError(t, err, "gid flag must be in the range of 0-2147483647") + }) + }) + }) + + when("gid flag is not provided", func() { + it("override build option should be set to false", func() { + mockClient.EXPECT(). + Build(gomock.Any(), EqBuildOptionsWithOverrideGroupID(false, 0)). + Return(nil) + + command.SetArgs([]string{"--builder", "my-builder", "image"}) + h.AssertNil(t, command.Execute()) + }) + }) }) } @@ -799,6 +830,15 @@ func EqBuildOptionsWithEnv(env map[string]string) gomock.Matcher { } } +func EqBuildOptionsWithOverrideGroupID(override bool, gid int) gomock.Matcher { + return buildOptionsMatcher{ + description: fmt.Sprintf("override=%t and GID=%d", override, gid), + equals: func(o pack.BuildOptions) bool { + return o.OverrideGroupID == override && o.GroupID == gid + }, + } +} + type buildOptionsMatcher struct { equals func(pack.BuildOptions) bool description string From 1178980e8a71e5742aef88aa92d66973fe2d6d4c Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Mon, 24 May 2021 11:58:34 -0500 Subject: [PATCH 2/5] Fixing some review feedback from Javier Romero Signed-off-by: Juan Bustamante --- build.go | 8 ---- build_test.go | 44 +++++++--------------- internal/build/lifecycle_execution.go | 8 ++-- internal/build/lifecycle_execution_test.go | 38 ++++++++----------- internal/build/lifecycle_executor.go | 1 - internal/commands/build.go | 9 ++--- internal/commands/build_test.go | 2 +- 7 files changed, 37 insertions(+), 73 deletions(-) diff --git a/build.go b/build.go index 2fb4ded04..55477977e 100644 --- a/build.go +++ b/build.go @@ -122,9 +122,6 @@ type BuildOptions struct { // Only trust builders from reputable sources. TrustBuilder bool - // The build and stack user's group id must be override - OverrideGroupID bool - // List of buildpack images or archives to add to a builder. // These buildpacks may overwrite those on the builder if they // share both an ID and Version with a buildpack on the builder. @@ -290,10 +287,6 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error { return err } - if opts.GroupID < 0 { - return errors.New("gid flag must be in the range of 0-2147483647") - } - lifecycleOpts := build.LifecycleOptions{ AppPath: appPath, Image: imageRef, @@ -315,7 +308,6 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error { FileFilter: fileFilter, CacheImage: opts.CacheImage, Workspace: opts.Workspace, - OverrideGID: opts.OverrideGroupID, GID: opts.GroupID, } diff --git a/build_test.go b/build_test.go index 9fcd18611..a8677a473 100644 --- a/build_test.go +++ b/build_test.go @@ -2192,58 +2192,40 @@ func testBuild(t *testing.T, when spec.G, it spec.S) { }) }) - when("GID option", func() { - when("gid is set and override is false", func() { - it("gid override must be false", func() { + when("gid option", func() { + when("gid is equal to zero", func() { + it("gid is passthroughs to lifecycle", func() { h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ Workspace: "app", Builder: defaultBuilderName, Image: "example.com/some/repo:tag", GroupID: 0, - OverrideGroupID: false, })) - h.AssertEq(t, fakeLifecycle.Opts.OverrideGID, false) h.AssertEq(t, fakeLifecycle.Opts.GID, 0) }) }) - when("gid is set and override is true", func() { - it("gid override must be true", func() { + when("gid is less than zero", func() { + it("gid is passthroughs to lifecycle", func() { h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ - Workspace: "app", - Builder: defaultBuilderName, - Image: "example.com/some/repo:tag", - GroupID: 1, - OverrideGroupID: true, - })) - h.AssertEq(t, fakeLifecycle.Opts.OverrideGID, true) - h.AssertEq(t, fakeLifecycle.Opts.GID, 1) - }) - }) - - when("gid is negative and override is true", func() { - it("should thrown error", func() { - err := subject.Build(context.TODO(), BuildOptions{ Workspace: "app", Builder: defaultBuilderName, Image: "example.com/some/repo:tag", GroupID: -1, - OverrideGroupID: true, - }) - h.AssertError(t, err, "gid flag must be in the range of 0-2147483647") + })) + h.AssertEq(t, fakeLifecycle.Opts.GID, -1) }) }) - when("gid is negative and override is false", func() { - it("should thrown error", func() { - err := subject.Build(context.TODO(), BuildOptions{ + when("gid is greater than zero", func() { + it("gid is passthroughs to lifecycle", func() { + h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ Workspace: "app", Builder: defaultBuilderName, Image: "example.com/some/repo:tag", - GroupID: -1, - OverrideGroupID: false, - }) - h.AssertError(t, err, "gid flag must be in the range of 0-2147483647") + GroupID: 2, + })) + h.AssertEq(t, fakeLifecycle.Opts.GID, 2) }) }) }) diff --git a/internal/build/lifecycle_execution.go b/internal/build/lifecycle_execution.go index 8135a1722..8bced8d2e 100644 --- a/internal/build/lifecycle_execution.go +++ b/internal/build/lifecycle_execution.go @@ -182,7 +182,7 @@ func (l *LifecycleExecution) Create(ctx context.Context, publish bool, dockerHos flags = append(flags, "-skip-restore") } - if l.opts.OverrideGID { + if l.opts.GID >= 0 { flags = append(flags, "-gid", strconv.Itoa(l.opts.GID)) } @@ -258,7 +258,7 @@ func (l *LifecycleExecution) Restore(ctx context.Context, networkMode string, bu case cache.Volume: cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir())) } - if l.opts.OverrideGID { + if l.opts.GID >= 0 { flagsOpt = WithFlags("-gid", strconv.Itoa(l.opts.GID)) } @@ -314,7 +314,7 @@ func (l *LifecycleExecution) newAnalyze(repoName, networkMode string, publish bo cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir())) } - if l.opts.OverrideGID { + if l.opts.GID >= 0 { flagsOpt = WithFlags("-gid", strconv.Itoa(l.opts.GID)) } @@ -403,7 +403,7 @@ func (l *LifecycleExecution) newExport(repoName, runImage string, publish bool, if processType != "" { flags = append(flags, "-process-type", processType) } - if l.opts.OverrideGID { + if l.opts.GID >= 0 { flags = append(flags, "-gid", strconv.Itoa(l.opts.GID)) } diff --git a/internal/build/lifecycle_execution_test.go b/internal/build/lifecycle_execution_test.go index 1c7342171..b1687b154 100644 --- a/internal/build/lifecycle_execution_test.go +++ b/internal/build/lifecycle_execution_test.go @@ -716,10 +716,9 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { }) when("override GID", func() { - when("override GID is true", func() { + when("override GID is provided", func() { it("configures the phase with the expected arguments", func() { verboseLifecycle := newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.OverrideGID = true options.GID = 0 }) fakePhaseFactory := fakes.NewFakePhaseFactory() @@ -738,10 +737,9 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { ) }) }) - when("override GID is false", func() { + when("override GID is not provided", func() { it("gid is not added to the expected arguments", func() { verboseLifecycle := newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.OverrideGID = false options.GID = 0 }) fakePhaseFactory := fakes.NewFakePhaseFactory() @@ -1187,14 +1185,13 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { fakePhase := &fakes.FakePhase{} fakePhaseFactory = fakes.NewFakePhaseFactory(fakes.WhichReturnsForNew(fakePhase)) - when("override GID is true", func() { + when("override GID is provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.OverrideGID = true options.GID = 0 }) }) - it("", func() { + it("configures the phase with the expected arguments", func() { err := lifecycle.Analyze(context.Background(), "test", "test", false, "", false, fakeCache, fakePhaseFactory) h.AssertNil(t, err) lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 @@ -1206,14 +1203,13 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { ) }) }) - when("override GID is false", func() { + when("override GID is not provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.OverrideGID = false options.GID = 0 }) }) - it("", func() { + it("gid is not added to the expected arguments", func() { err := lifecycle.Analyze(context.Background(), "test", "test", false, "", false, fakeCache, fakePhaseFactory) h.AssertNil(t, err) lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 @@ -1377,14 +1373,13 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { fakePhase := &fakes.FakePhase{} fakePhaseFactory = fakes.NewFakePhaseFactory(fakes.WhichReturnsForNew(fakePhase)) - when("override GID is true", func() { + when("override GID is provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.OverrideGID = true options.GID = 0 }) }) - it("", func() { + it("configures the phase with the expected arguments", func() { err := lifecycle.Restore(context.Background(), "test", fakeCache, fakePhaseFactory) h.AssertNil(t, err) lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 @@ -1396,14 +1391,13 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { ) }) }) - when("override GID is false", func() { + when("override GID is not provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.OverrideGID = false options.GID = 0 }) }) - it("", func() { + it("gid is not added to the expected arguments", func() { err := lifecycle.Restore(context.Background(), "test", fakeCache, fakePhaseFactory) h.AssertNil(t, err) lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 @@ -1972,14 +1966,13 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { fakePhase := &fakes.FakePhase{} fakePhaseFactory = fakes.NewFakePhaseFactory(fakes.WhichReturnsForNew(fakePhase)) - when("override GID is true", func() { + when("override GID is provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.OverrideGID = true options.GID = 0 }) }) - it("", func() { + it("configures the phase with the expected arguments", func() { err := lifecycle.Export(context.Background(), "test", "test", false, "", "test", fakeBuildCache, fakeLaunchCache, []string{}, fakePhaseFactory) h.AssertNil(t, err) lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 @@ -1991,14 +1984,13 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { ) }) }) - when("override GID is false", func() { + when("override GID is not provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.OverrideGID = false - options.GID = 0 + options.GID = -1 }) }) - it("", func() { + it("gid is not added to the expected arguments", func() { err := lifecycle.Export(context.Background(), "test", "test", false, "", "test", fakeBuildCache, fakeLaunchCache, []string{}, fakePhaseFactory) h.AssertNil(t, err) lastCallIndex := len(fakePhaseFactory.NewCalledWithProvider) - 1 diff --git a/internal/build/lifecycle_executor.go b/internal/build/lifecycle_executor.go index 8bf30ceef..0532c5fd5 100644 --- a/internal/build/lifecycle_executor.go +++ b/internal/build/lifecycle_executor.go @@ -58,7 +58,6 @@ type LifecycleOptions struct { Publish bool TrustBuilder bool UseCreator bool - OverrideGID bool DockerHost string CacheImage string HTTPProxy string diff --git a/internal/commands/build.go b/internal/commands/build.go index a37cef673..340250a2a 100644 --- a/internal/commands/build.go +++ b/internal/commands/build.go @@ -122,9 +122,9 @@ func Build(logger logging.Logger, cfg config.Config, packClient PackClient) *cob } lifecycleImage = ref.Name() } - var overrideGID = false + var gid = -1 if cmd.Flags().Changed("gid") { - overrideGID = true + gid = flags.GID } if err := packClient.Build(cmd.Context(), pack.BuildOptions{ AppPath: flags.AppPath, @@ -151,8 +151,7 @@ func Build(logger logging.Logger, cfg config.Config, packClient PackClient) *cob CacheImage: flags.CacheImage, Workspace: flags.Workspace, LifecycleImage: lifecycleImage, - GroupID: flags.GID, - OverrideGroupID: overrideGID, + GroupID: gid, }); err != nil { return errors.Wrap(err, "failed to build") } @@ -191,7 +190,7 @@ This option may set DOCKER_HOST environment variable for the build container if cmd.Flags().BoolVar(&buildFlags.TrustBuilder, "trust-builder", false, "Trust the provided builder\nAll lifecycle phases will be run in a single container (if supported by the lifecycle).") cmd.Flags().StringArrayVar(&buildFlags.Volumes, "volume", nil, "Mount host volume into the build container, in the form ':[:]'.\n- 'host path': Name of the volume or absolute directory path to mount.\n- 'target path': The path where the file or directory is available in the container.\n- 'options' (default \"ro\"): An optional comma separated list of mount options.\n - \"ro\", volume contents are read-only.\n - \"rw\", volume contents are readable and writeable.\n - \"volume-opt==\", can be specified more than once, takes a key-value pair consisting of the option name and its value."+multiValueHelp("volume")) cmd.Flags().StringVar(&buildFlags.Workspace, "workspace", "", "Location at which to mount the app dir in the build image") - cmd.Flags().IntVar(&buildFlags.GID, "gid", 0, `Override GID of user's group in the stack's build and run images, it must be a value in the range of 0-2147483647`) + cmd.Flags().IntVar(&buildFlags.GID, "gid", 0, `Override GID of user's group in the stack's build and run images`) } func validateBuildFlags(flags *BuildFlags, cfg config.Config, packClient PackClient, logger logging.Logger) error { diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 592936716..6114d50de 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -834,7 +834,7 @@ func EqBuildOptionsWithOverrideGroupID(override bool, gid int) gomock.Matcher { return buildOptionsMatcher{ description: fmt.Sprintf("override=%t and GID=%d", override, gid), equals: func(o pack.BuildOptions) bool { - return o.OverrideGroupID == override && o.GroupID == gid + return o.GroupID == gid }, } } From 8978cafbd11e79cb68557d67663b40779f2c7484 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Mon, 24 May 2021 14:51:55 -0500 Subject: [PATCH 3/5] Externalizing GID value to constant, also some minor fixes in unit testing Signed-off-by: Juan Bustamante --- build_test.go | 42 +++++----------------- internal/build/lifecycle_execution.go | 9 ++--- internal/build/lifecycle_execution_test.go | 34 ++++++++++-------- internal/commands/build.go | 2 +- 4 files changed, 34 insertions(+), 53 deletions(-) diff --git a/build_test.go b/build_test.go index a8677a473..cb9ea739b 100644 --- a/build_test.go +++ b/build_test.go @@ -2193,40 +2193,14 @@ func testBuild(t *testing.T, when spec.G, it spec.S) { }) when("gid option", func() { - when("gid is equal to zero", func() { - it("gid is passthroughs to lifecycle", func() { - h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ - Workspace: "app", - Builder: defaultBuilderName, - Image: "example.com/some/repo:tag", - GroupID: 0, - })) - h.AssertEq(t, fakeLifecycle.Opts.GID, 0) - }) - }) - - when("gid is less than zero", func() { - it("gid is passthroughs to lifecycle", func() { - h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ - Workspace: "app", - Builder: defaultBuilderName, - Image: "example.com/some/repo:tag", - GroupID: -1, - })) - h.AssertEq(t, fakeLifecycle.Opts.GID, -1) - }) - }) - - when("gid is greater than zero", func() { - it("gid is passthroughs to lifecycle", func() { - h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ - Workspace: "app", - Builder: defaultBuilderName, - Image: "example.com/some/repo:tag", - GroupID: 2, - })) - h.AssertEq(t, fakeLifecycle.Opts.GID, 2) - }) + it("gid is passthroughs to lifecycle", func() { + h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ + Workspace: "app", + Builder: defaultBuilderName, + Image: "example.com/some/repo:tag", + GroupID: 2, + })) + h.AssertEq(t, fakeLifecycle.Opts.GID, 2) }) }) }) diff --git a/internal/build/lifecycle_execution.go b/internal/build/lifecycle_execution.go index 8bced8d2e..7ba904c91 100644 --- a/internal/build/lifecycle_execution.go +++ b/internal/build/lifecycle_execution.go @@ -22,6 +22,7 @@ import ( const ( defaultProcessType = "web" + overrideGID = 0 ) type LifecycleExecution struct { @@ -182,7 +183,7 @@ func (l *LifecycleExecution) Create(ctx context.Context, publish bool, dockerHos flags = append(flags, "-skip-restore") } - if l.opts.GID >= 0 { + if l.opts.GID >= overrideGID { flags = append(flags, "-gid", strconv.Itoa(l.opts.GID)) } @@ -258,7 +259,7 @@ func (l *LifecycleExecution) Restore(ctx context.Context, networkMode string, bu case cache.Volume: cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir())) } - if l.opts.GID >= 0 { + if l.opts.GID >= overrideGID { flagsOpt = WithFlags("-gid", strconv.Itoa(l.opts.GID)) } @@ -314,7 +315,7 @@ func (l *LifecycleExecution) newAnalyze(repoName, networkMode string, publish bo cacheOpt = WithBinds(fmt.Sprintf("%s:%s", buildCache.Name(), l.mountPaths.cacheDir())) } - if l.opts.GID >= 0 { + if l.opts.GID >= overrideGID { flagsOpt = WithFlags("-gid", strconv.Itoa(l.opts.GID)) } @@ -403,7 +404,7 @@ func (l *LifecycleExecution) newExport(repoName, runImage string, publish bool, if processType != "" { flags = append(flags, "-process-type", processType) } - if l.opts.GID >= 0 { + if l.opts.GID >= overrideGID { flags = append(flags, "-gid", strconv.Itoa(l.opts.GID)) } diff --git a/internal/build/lifecycle_execution_test.go b/internal/build/lifecycle_execution_test.go index b1687b154..81d1f1ff6 100644 --- a/internal/build/lifecycle_execution_test.go +++ b/internal/build/lifecycle_execution_test.go @@ -719,7 +719,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { when("override GID is provided", func() { it("configures the phase with the expected arguments", func() { verboseLifecycle := newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.GID = 0 + options.GID = 2 }) fakePhaseFactory := fakes.NewFakePhaseFactory() @@ -733,14 +733,14 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { h.AssertEq(t, configProvider.Name(), "creator") h.AssertIncludeAllExpectedPatterns(t, configProvider.ContainerConfig().Cmd, - []string{"-gid"}, + []string{"-gid", "2"}, ) }) }) when("override GID is not provided", func() { it("gid is not added to the expected arguments", func() { verboseLifecycle := newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.GID = 0 + options.GID = -1 }) fakePhaseFactory := fakes.NewFakePhaseFactory() @@ -891,7 +891,9 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { fakeCache.ReturnForType = cache.Image fakeCache.ReturnForName = "some-cache-image" - lifecycle = newTestLifecycleExec(t, false) + lifecycle = newTestLifecycleExec(t, false, func(options *build.LifecycleOptions) { + options.GID = -1 + }) fakePhaseFactory = fakes.NewFakePhaseFactory() }) it("configures the phase with a build cache images", func() { @@ -1036,7 +1038,9 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { }) it("configures the phase with a build cache images", func() { - lifecycle := newTestLifecycleExec(t, false) + lifecycle := newTestLifecycleExec(t, false, func(options *build.LifecycleOptions) { + options.GID = -1 + }) fakePhaseFactory := fakes.NewFakePhaseFactory() expectedRepoName := "some-repo-name" @@ -1188,7 +1192,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { when("override GID is provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.GID = 0 + options.GID = 2 }) }) it("configures the phase with the expected arguments", func() { @@ -1199,14 +1203,14 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] h.AssertIncludeAllExpectedPatterns(t, configProvider.ContainerConfig().Cmd, - []string{"-gid"}, + []string{"-gid", "2"}, ) }) }) when("override GID is not provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.GID = 0 + options.GID = -1 }) }) it("gid is not added to the expected arguments", func() { @@ -1346,7 +1350,9 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { fakeCache.ReturnForType = cache.Image fakeCache.ReturnForName = "some-cache-image" - lifecycle = newTestLifecycleExec(t, false) + lifecycle = newTestLifecycleExec(t, false, func(options *build.LifecycleOptions) { + options.GID = -1 + }) fakePhaseFactory = fakes.NewFakePhaseFactory() }) it("configures the phase with a cache image", func() { @@ -1376,7 +1382,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { when("override GID is provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.GID = 0 + options.GID = 2 }) }) it("configures the phase with the expected arguments", func() { @@ -1387,14 +1393,14 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] h.AssertIncludeAllExpectedPatterns(t, configProvider.ContainerConfig().Cmd, - []string{"-gid"}, + []string{"-gid", "2"}, ) }) }) when("override GID is not provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.GID = 0 + options.GID = -1 }) }) it("gid is not added to the expected arguments", func() { @@ -1969,7 +1975,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { when("override GID is provided", func() { it.Before(func() { lifecycle = newTestLifecycleExec(t, true, func(options *build.LifecycleOptions) { - options.GID = 0 + options.GID = 2 }) }) it("configures the phase with the expected arguments", func() { @@ -1980,7 +1986,7 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] h.AssertIncludeAllExpectedPatterns(t, configProvider.ContainerConfig().Cmd, - []string{"-gid"}, + []string{"-gid", "2"}, ) }) }) diff --git a/internal/commands/build.go b/internal/commands/build.go index 340250a2a..c877c74d5 100644 --- a/internal/commands/build.go +++ b/internal/commands/build.go @@ -190,7 +190,7 @@ This option may set DOCKER_HOST environment variable for the build container if cmd.Flags().BoolVar(&buildFlags.TrustBuilder, "trust-builder", false, "Trust the provided builder\nAll lifecycle phases will be run in a single container (if supported by the lifecycle).") cmd.Flags().StringArrayVar(&buildFlags.Volumes, "volume", nil, "Mount host volume into the build container, in the form ':[:]'.\n- 'host path': Name of the volume or absolute directory path to mount.\n- 'target path': The path where the file or directory is available in the container.\n- 'options' (default \"ro\"): An optional comma separated list of mount options.\n - \"ro\", volume contents are read-only.\n - \"rw\", volume contents are readable and writeable.\n - \"volume-opt==\", can be specified more than once, takes a key-value pair consisting of the option name and its value."+multiValueHelp("volume")) cmd.Flags().StringVar(&buildFlags.Workspace, "workspace", "", "Location at which to mount the app dir in the build image") - cmd.Flags().IntVar(&buildFlags.GID, "gid", 0, `Override GID of user's group in the stack's build and run images`) + cmd.Flags().IntVar(&buildFlags.GID, "gid", 0, `Override GID of user's group in the stack's build and run images. The provided value must be a positive number`) } func validateBuildFlags(flags *BuildFlags, cfg config.Config, packClient PackClient, logger logging.Logger) error { From 458353725aca5c50a891760740a7f27b6ac7467f Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Mon, 24 May 2021 14:52:53 -0500 Subject: [PATCH 4/5] Formatting issues Signed-off-by: Juan Bustamante --- build_test.go | 8 ++++---- internal/build/lifecycle_execution.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/build_test.go b/build_test.go index cb9ea739b..ef23049c4 100644 --- a/build_test.go +++ b/build_test.go @@ -2195,10 +2195,10 @@ func testBuild(t *testing.T, when spec.G, it spec.S) { when("gid option", func() { it("gid is passthroughs to lifecycle", func() { h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{ - Workspace: "app", - Builder: defaultBuilderName, - Image: "example.com/some/repo:tag", - GroupID: 2, + Workspace: "app", + Builder: defaultBuilderName, + Image: "example.com/some/repo:tag", + GroupID: 2, })) h.AssertEq(t, fakeLifecycle.Opts.GID, 2) }) diff --git a/internal/build/lifecycle_execution.go b/internal/build/lifecycle_execution.go index 7ba904c91..38cb1f777 100644 --- a/internal/build/lifecycle_execution.go +++ b/internal/build/lifecycle_execution.go @@ -22,7 +22,7 @@ import ( const ( defaultProcessType = "web" - overrideGID = 0 + overrideGID = 0 ) type LifecycleExecution struct { From 94fdbc551090cd4c1b22fa763de12fce2f8f177f Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Wed, 26 May 2021 14:18:21 -0500 Subject: [PATCH 5/5] Fixing failing unit tests Signed-off-by: Juan Bustamante --- internal/commands/build_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/commands/build_test.go b/internal/commands/build_test.go index 6114d50de..1e2fa190a 100644 --- a/internal/commands/build_test.go +++ b/internal/commands/build_test.go @@ -683,7 +683,7 @@ builder = "my-builder" when("--gid is a valid value", func() { it("override build option should be set to true", func() { mockClient.EXPECT(). - Build(gomock.Any(), EqBuildOptionsWithOverrideGroupID(true, 1)). + Build(gomock.Any(), EqBuildOptionsWithOverrideGroupID(1)). Return(nil) command.SetArgs([]string{"--builder", "my-builder", "image", "--gid", "1"}) @@ -702,7 +702,7 @@ builder = "my-builder" when("gid flag is not provided", func() { it("override build option should be set to false", func() { mockClient.EXPECT(). - Build(gomock.Any(), EqBuildOptionsWithOverrideGroupID(false, 0)). + Build(gomock.Any(), EqBuildOptionsWithOverrideGroupID(-1)). Return(nil) command.SetArgs([]string{"--builder", "my-builder", "image"}) @@ -830,9 +830,9 @@ func EqBuildOptionsWithEnv(env map[string]string) gomock.Matcher { } } -func EqBuildOptionsWithOverrideGroupID(override bool, gid int) gomock.Matcher { +func EqBuildOptionsWithOverrideGroupID(gid int) gomock.Matcher { return buildOptionsMatcher{ - description: fmt.Sprintf("override=%t and GID=%d", override, gid), + description: fmt.Sprintf("GID=%d", gid), equals: func(o pack.BuildOptions) bool { return o.GroupID == gid },