From 5d2d2829d05dea9b2bf2457d9449fcb37ddbcc92 Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Mon, 10 Sep 2018 18:08:43 -0700 Subject: [PATCH 1/3] Review config for cmd/entrypoint after building a stage As mentioned in #346, if only ENTRYPOINT is set in a stage then any CMD inherited from a parent should be cleared. If both entrypoint and cmd are set then nothing should change. I added a function and unit test to review the config file after building a stage which clears out config.Cmd if ENTRYPOINT was declared but CMD wasn't. I also added an integration test to make sure this works, which should be tested by the preexisting container-diff --metadata test. --- integration/dockerfiles/Dockerfile_test_cmd | 5 ++ pkg/executor/build.go | 24 +++++++ pkg/executor/build_test.go | 76 +++++++++++++++++++++ 3 files changed, 105 insertions(+) create mode 100644 integration/dockerfiles/Dockerfile_test_cmd create mode 100644 pkg/executor/build_test.go diff --git a/integration/dockerfiles/Dockerfile_test_cmd b/integration/dockerfiles/Dockerfile_test_cmd new file mode 100644 index 0000000000..631b93fd84 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_cmd @@ -0,0 +1,5 @@ +FROM scratch AS first +CMD ["mycmd"] + +FROM first +ENTRYPOINT ["myentrypoint"] # This should clear out CMD in the config metadata diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 27f58d70fd..10cbeca9e8 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -145,6 +145,9 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { return nil, err } } + if err := reviewConfig(stage, &imageConfig.Config); err != nil { + return nil, err + } sourceImage, err = mutate.Config(sourceImage, imageConfig.Config) if err != nil { return nil, err @@ -228,3 +231,24 @@ func resolveOnBuild(stage *config.KanikoStage, config *v1.Config) error { config.OnBuild = nil return nil } + +// reviewConfig makes sure the value of CMD is correct after building the stage +// If ENTRYPOINT was set in this stage but CMD wasn't, then CMD should be cleared out +// See Issue #346 for more info +func reviewConfig(stage config.KanikoStage, config *v1.Config) error { + entrypoint := false + cmd := false + + for _, c := range stage.Commands { + if c.Name() == "cmd" { + cmd = true + } + if c.Name() == "entrypoint" { + entrypoint = true + } + } + if entrypoint && !cmd { + config.Cmd = []string{} + } + return nil +} diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go new file mode 100644 index 0000000000..e1f1204331 --- /dev/null +++ b/pkg/executor/build_test.go @@ -0,0 +1,76 @@ +/* +Copyright 2018 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package executor + +import ( + "testing" + + "github.com/GoogleContainerTools/kaniko/pkg/config" + "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" + "github.com/GoogleContainerTools/kaniko/testutil" + "github.com/google/go-containerregistry/pkg/v1" +) + +func Test_reviewConfig(t *testing.T) { + tests := []struct { + name string + dockerfile string + originalCmd []string + originalEntrypoint []string + expectedCmd []string + }{ + { + name: "entrypoint and cmd declared", + dockerfile: ` + FROM scratch + CMD ["mycmd"] + ENTRYPOINT ["myentrypoint"]`, + originalEntrypoint: []string{"myentrypoint"}, + originalCmd: []string{"mycmd"}, + expectedCmd: []string{"mycmd"}, + }, + { + name: "only entrypoint declared", + dockerfile: ` + FROM scratch + ENTRYPOINT ["myentrypoint"]`, + originalEntrypoint: []string{"myentrypoint"}, + originalCmd: []string{"mycmd"}, + expectedCmd: []string{}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + config := &v1.Config{ + Cmd: test.originalCmd, + Entrypoint: test.originalEntrypoint, + } + err := reviewConfig(stage(t, test.dockerfile), config) + testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedCmd, config.Cmd) + }) + } +} + +func stage(t *testing.T, d string) config.KanikoStage { + stages, err := dockerfile.Parse([]byte(d)) + if err != nil { + t.Fatalf("error parsing dockerfile: %v", err) + } + return config.KanikoStage{ + Stage: stages[0], + } +} From d923d5ef02631aafbe1920830cb740f717b4a4a1 Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Tue, 11 Sep 2018 10:05:15 -0700 Subject: [PATCH 2/3] Fix integration test --- integration/dockerfiles/Dockerfile_test_cmd | 2 +- pkg/commands/cmd.go | 2 -- pkg/commands/entrypoint.go | 2 -- pkg/executor/build.go | 2 +- pkg/executor/build_test.go | 2 +- 5 files changed, 3 insertions(+), 7 deletions(-) diff --git a/integration/dockerfiles/Dockerfile_test_cmd b/integration/dockerfiles/Dockerfile_test_cmd index 631b93fd84..a2f0162b0a 100644 --- a/integration/dockerfiles/Dockerfile_test_cmd +++ b/integration/dockerfiles/Dockerfile_test_cmd @@ -1,4 +1,4 @@ -FROM scratch AS first +FROM gcr.io/distroless/base@sha256:628939ac8bf3f49571d05c6c76b8688cb4a851af6c7088e599388259875bde20 AS first CMD ["mycmd"] FROM first diff --git a/pkg/commands/cmd.go b/pkg/commands/cmd.go index c6965c41ef..3087552002 100644 --- a/pkg/commands/cmd.go +++ b/pkg/commands/cmd.go @@ -23,7 +23,6 @@ import ( "github.com/google/go-containerregistry/pkg/v1" "github.com/moby/buildkit/frontend/dockerfile/instructions" - "github.com/sirupsen/logrus" ) type CmdCommand struct { @@ -48,7 +47,6 @@ func (c *CmdCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bui newCommand = c.cmd.CmdLine } - logrus.Infof("Replacing CMD in config with %v", newCommand) config.Cmd = newCommand config.ArgsEscaped = true return nil diff --git a/pkg/commands/entrypoint.go b/pkg/commands/entrypoint.go index ba8aa0c822..5e403e23d2 100644 --- a/pkg/commands/entrypoint.go +++ b/pkg/commands/entrypoint.go @@ -23,7 +23,6 @@ import ( "github.com/google/go-containerregistry/pkg/v1" "github.com/moby/buildkit/frontend/dockerfile/instructions" - "github.com/sirupsen/logrus" ) type EntrypointCommand struct { @@ -47,7 +46,6 @@ func (e *EntrypointCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerf newCommand = e.cmd.CmdLine } - logrus.Infof("Replacing Entrypoint in config with %v", newCommand) config.Entrypoint = newCommand return nil } diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 10cbeca9e8..c9d08b7611 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -248,7 +248,7 @@ func reviewConfig(stage config.KanikoStage, config *v1.Config) error { } } if entrypoint && !cmd { - config.Cmd = []string{} + config.Cmd = nil } return nil } diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index e1f1204331..51eca6f993 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -50,7 +50,7 @@ func Test_reviewConfig(t *testing.T) { ENTRYPOINT ["myentrypoint"]`, originalEntrypoint: []string{"myentrypoint"}, originalCmd: []string{"mycmd"}, - expectedCmd: []string{}, + expectedCmd: nil, }, } for _, test := range tests { From cd1b957e4327d4b58fcf527d906cf1fbbbb0ae42 Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Mon, 17 Sep 2018 11:11:51 +0100 Subject: [PATCH 3/3] Address code review comments; review unnecessary error check --- pkg/constants/constants.go | 6 +++++- pkg/executor/build.go | 11 ++++------- pkg/executor/build_test.go | 4 ++-- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index bcc658fca6..d8fcc722e9 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -55,9 +55,13 @@ const ( S3BuildContextPrefix = "s3://" LocalDirBuildContextPrefix = "dir://" + HOME = "HOME" // DefaultHOMEValue is the default value Docker sets for $HOME - HOME = "HOME" DefaultHOMEValue = "/root" + + // Docker command names + Cmd = "cmd" + Entrypoint = "entrypoint" ) // KanikoBuildFiles is the list of files required to build kaniko diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 8b9659a762..9237411261 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -145,9 +145,7 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { return nil, err } } - if err := reviewConfig(stage, &imageConfig.Config); err != nil { - return nil, err - } + reviewConfig(stage, &imageConfig.Config) sourceImage, err = mutate.Config(sourceImage, imageConfig.Config) if err != nil { return nil, err @@ -235,20 +233,19 @@ func resolveOnBuild(stage *config.KanikoStage, config *v1.Config) error { // reviewConfig makes sure the value of CMD is correct after building the stage // If ENTRYPOINT was set in this stage but CMD wasn't, then CMD should be cleared out // See Issue #346 for more info -func reviewConfig(stage config.KanikoStage, config *v1.Config) error { +func reviewConfig(stage config.KanikoStage, config *v1.Config) { entrypoint := false cmd := false for _, c := range stage.Commands { - if c.Name() == "cmd" { + if c.Name() == constants.Cmd { cmd = true } - if c.Name() == "entrypoint" { + if c.Name() == constants.Entrypoint { entrypoint = true } } if entrypoint && !cmd { config.Cmd = nil } - return nil } diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 51eca6f993..edf162261a 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -59,8 +59,8 @@ func Test_reviewConfig(t *testing.T) { Cmd: test.originalCmd, Entrypoint: test.originalEntrypoint, } - err := reviewConfig(stage(t, test.dockerfile), config) - testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedCmd, config.Cmd) + reviewConfig(stage(t, test.dockerfile), config) + testutil.CheckErrorAndDeepEqual(t, false, nil, test.expectedCmd, config.Cmd) }) } }