Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parse arg commands at the top of dockerfiles #404

Merged
merged 12 commits into from
Nov 6, 2018
Merged
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

# Bump these on release
VERSION_MAJOR ?= 0
VERSION_MINOR ?= 3
VERSION_MINOR ?= 5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this!! Must have missed it...many times... 😅

VERSION_BUILD ?= 0

VERSION ?= v$(VERSION_MAJOR).$(VERSION_MINOR).$(VERSION_BUILD)
Expand Down
17 changes: 17 additions & 0 deletions integration/dockerfiles/Dockerfile_test_meta_arg
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
ARG REGISTRY=gcr.io
ARG REPO=google-appengine
ARG WORD=hello
ARG W0RD2=hey

FROM ${REGISTRY}/${REPO}/debian9 as stage1

# Should evaluate WORD and create /tmp/hello
ARG WORD
RUN touch /${WORD}

FROM ${REGISTRY}/${REPO}/debian9

COPY --from=stage1 /hello /tmp

# /tmp/hey should not get created without the ARG statement
RUN touch /tmp/${WORD2}
6 changes: 6 additions & 0 deletions pkg/commands/arg.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ func (r *ArgCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bui
return err
}
resolvedValue = &value
} else {
meta := buildArgs.GetAllMeta()
if value, ok := meta[resolvedKey]; ok {
resolvedValue = &value
}
}

buildArgs.AddArg(resolvedKey, resolvedValue)
return nil
}
Expand Down
8 changes: 8 additions & 0 deletions pkg/dockerfile/buildargs.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,11 @@ func (b *BuildArgs) ReplacementEnvs(envs []string) []string {
filtered := b.FilterAllowed(envs)
return append(envs, filtered...)
}

// AddMetaArgs adds the supplied args map to b's allowedMetaArgs
func (b *BuildArgs) AddMetaArgs(metaArgs map[string]string) {
for key, value := range metaArgs {
v := value
b.AddMetaArg(key, &v)
}
}
25 changes: 13 additions & 12 deletions pkg/dockerfile/dockerfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,25 @@ import (
)

// Stages parses a Dockerfile and returns an array of KanikoStage
func Stages(opts *config.KanikoOptions) ([]config.KanikoStage, error) {
func Stages(opts *config.KanikoOptions) ([]config.KanikoStage, []instructions.ArgCommand, error) {
d, err := ioutil.ReadFile(opts.DockerfilePath)
if err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("reading dockerfile at path %s", opts.DockerfilePath))
return nil, nil, errors.Wrap(err, fmt.Sprintf("reading dockerfile at path %s", opts.DockerfilePath))
}
stages, err := Parse(d)
stages, metaArgs, err := Parse(d)
if err != nil {
return nil, errors.Wrap(err, "parsing dockerfile")
return nil, nil, errors.Wrap(err, "parsing dockerfile")
}
targetStage, err := targetStage(stages, opts.Target)
if err != nil {
return nil, err
return nil, nil, err
}
resolveStages(stages)
var kanikoStages []config.KanikoStage
for index, stage := range stages {
resolvedBaseName, err := util.ResolveEnvironmentReplacement(stage.BaseName, opts.BuildArgs, false)
if err != nil {
return nil, errors.Wrap(err, "resolving base name")
return nil, nil, errors.Wrap(err, "resolving base name")
}
stage.Name = resolvedBaseName
kanikoStages = append(kanikoStages, config.KanikoStage{
Expand All @@ -65,7 +65,8 @@ func Stages(opts *config.KanikoOptions) ([]config.KanikoStage, error) {
break
}
}
return kanikoStages, nil

return kanikoStages, metaArgs, nil
}

// baseImageIndex returns the index of the stage the current stage is built off
Expand All @@ -83,16 +84,16 @@ func baseImageIndex(currentStage int, stages []instructions.Stage) int {
}

// Parse parses the contents of a Dockerfile and returns a list of commands
func Parse(b []byte) ([]instructions.Stage, error) {
func Parse(b []byte) ([]instructions.Stage, []instructions.ArgCommand, error) {
p, err := parser.Parse(bytes.NewReader(b))
if err != nil {
return nil, err
return nil, nil, err
}
stages, _, err := instructions.Parse(p.AST)
stages, metaArgs, err := instructions.Parse(p.AST)
if err != nil {
return nil, err
return nil, nil, err
}
return stages, err
return stages, metaArgs, err
}

// targetStage returns the index of the target stage kaniko is trying to build
Expand Down
8 changes: 4 additions & 4 deletions pkg/dockerfile/dockerfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func Test_resolveStages(t *testing.T) {
FROM scratch
COPY --from=second /hi2 /hi3
`
stages, err := Parse([]byte(dockerfile))
stages, _, err := Parse([]byte(dockerfile))
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -63,7 +63,7 @@ func Test_targetStage(t *testing.T) {
FROM scratch
COPY --from=second /hi2 /hi3
`
stages, err := Parse([]byte(dockerfile))
stages, _, err := Parse([]byte(dockerfile))
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -142,7 +142,7 @@ func Test_SaveStage(t *testing.T) {
expected: false,
},
}
stages, err := Parse([]byte(testutil.Dockerfile))
stages, _, err := Parse([]byte(testutil.Dockerfile))
if err != nil {
t.Fatalf("couldn't retrieve stages from Dockerfile: %v", err)
}
Expand Down Expand Up @@ -177,7 +177,7 @@ func Test_baseImageIndex(t *testing.T) {
},
}

stages, err := Parse([]byte(testutil.Dockerfile))
stages, _, err := Parse([]byte(testutil.Dockerfile))
if err != nil {
t.Fatalf("couldn't retrieve stages from Dockerfile: %v", err)
}
Expand Down
20 changes: 16 additions & 4 deletions pkg/executor/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,12 @@ type stageBuilder struct {
snapshotter *snapshot.Snapshotter
baseImageDigest string
opts *config.KanikoOptions
metaArgs map[string]string
}

// newStageBuilder returns a new type stageBuilder which contains all the information required to build the stage
func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage) (*stageBuilder, error) {
sourceImage, err := util.RetrieveSourceImage(stage, opts.BuildArgs, opts)
func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage, metaArgs map[string]string) (*stageBuilder, error) {
sourceImage, err := util.RetrieveSourceImage(stage, opts, metaArgs)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -83,6 +84,7 @@ func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage) (*sta
snapshotter: snapshotter,
baseImageDigest: digest.String(),
opts: opts,
metaArgs: metaArgs,
}, nil
}

Expand Down Expand Up @@ -136,6 +138,7 @@ func (s *stageBuilder) build() error {
}

args := dockerfile.NewBuildArgs(s.opts.BuildArgs)
args.AddMetaArgs(s.metaArgs)
for index, command := range cmds {
if command == nil {
continue
Expand Down Expand Up @@ -250,12 +253,21 @@ func (s *stageBuilder) saveSnapshot(createdBy string, ck string, contents []byte
// DoBuild executes building the Dockerfile
func DoBuild(opts *config.KanikoOptions) (v1.Image, error) {
// Parse dockerfile and unpack base image to root
stages, err := dockerfile.Stages(opts)
stages, metaArgs, err := dockerfile.Stages(opts)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of storing the metaArgs for each stage in the KanikoStage type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that simplified things considerably. done.

if err != nil {
return nil, err
}
// Parse and apply args declared before any stage.
kanikoMetaArgs := map[string]string{}
for _, arg := range metaArgs {
var v string
if arg.Value != nil {
v = *arg.Value
}
kanikoMetaArgs[arg.Key] = v
}
for index, stage := range stages {
sb, err := newStageBuilder(opts, stage)
sb, err := newStageBuilder(opts, stage, kanikoMetaArgs)
if err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("getting stage builder for stage %d", index))
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/executor/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func Test_reviewConfig(t *testing.T) {
}

func stage(t *testing.T, d string) config.KanikoStage {
stages, err := dockerfile.Parse([]byte(d))
stages, _, err := dockerfile.Parse([]byte(d))
if err != nil {
t.Fatalf("error parsing dockerfile: %v", err)
}
Expand Down
7 changes: 7 additions & 0 deletions pkg/snapshot/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ func (s *Snapshotter) snapShotFS(f io.Writer) (bool, error) {
}
}

addedPaths := []string{}
// Now create the tar.
for path := range memFs {
whitelisted, err := util.CheckWhitelist(path)
Expand All @@ -208,12 +209,18 @@ func (s *Snapshotter) snapShotFS(f io.Writer) (bool, error) {
}
if maybeAdd {
logrus.Debugf("Adding %s to layer, because it was changed.", path)
addedPaths = append(addedPaths, path)
filesAdded = true
if err := t.AddFileToTar(path); err != nil {
return false, err
}
}
}

if filesAdded && len(addedPaths) == 1 && addedPaths[0] == "/" {
logrus.Infof("Only added /, not taking snapshot.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so for a command like RUN touch /${FOO}, where $FOO doesn't resolve to anything, there should just be an empty layer appended to the image, that's what docker does. For some reason, we see that / has changed and add that to the snapshot, resulting in an extra non-empty layer compared to docker.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmm, what if it's like "/foo/${FOO}"? Maybe the check is if the only added path is a directory, rather than root?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, done.

return false, nil
}

return filesAdded, nil
}
9 changes: 8 additions & 1 deletion pkg/util/image_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package util

import (
"crypto/tls"
"fmt"
"net/http"
"path/filepath"
"strconv"
Expand All @@ -44,7 +45,13 @@ var (
)

// RetrieveSourceImage returns the base image of the stage at index
func RetrieveSourceImage(stage config.KanikoStage, buildArgs []string, opts *config.KanikoOptions) (v1.Image, error) {
func RetrieveSourceImage(stage config.KanikoStage, opts *config.KanikoOptions, metaArgs map[string]string) (v1.Image, error) {
buildArgs := opts.BuildArgs
var metaArgsString []string
for key, value := range metaArgs {
metaArgsString = append(metaArgsString, fmt.Sprintf("%s=%s", key, value))
}
buildArgs = append(buildArgs, metaArgsString...)
currentBaseName, err := ResolveEnvironmentReplacement(stage.BaseName, buildArgs, false)
if err != nil {
return nil, err
Expand Down
6 changes: 3 additions & 3 deletions pkg/util/image_util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func Test_StandardImage(t *testing.T) {
retrieveRemoteImage = mock
actual, err := RetrieveSourceImage(config.KanikoStage{
Stage: stages[0],
}, nil, &config.KanikoOptions{})
}, &config.KanikoOptions{}, map[string]string{})
testutil.CheckErrorAndDeepEqual(t, false, err, nil, actual)
}
func Test_ScratchImage(t *testing.T) {
Expand All @@ -67,7 +67,7 @@ func Test_ScratchImage(t *testing.T) {
}
actual, err := RetrieveSourceImage(config.KanikoStage{
Stage: stages[1],
}, nil, &config.KanikoOptions{})
}, &config.KanikoOptions{}, map[string]string{})
expected := empty.Image
testutil.CheckErrorAndDeepEqual(t, false, err, expected, actual)
}
Expand All @@ -89,7 +89,7 @@ func Test_TarImage(t *testing.T) {
BaseImageStoredLocally: true,
BaseImageIndex: 0,
Stage: stages[2],
}, nil, &config.KanikoOptions{})
}, &config.KanikoOptions{}, map[string]string{})
testutil.CheckErrorAndDeepEqual(t, false, err, nil, actual)
}

Expand Down