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.

Sign up for GitHub

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

Issue #439 Strip out double quotes in ARG value #834

Merged
merged 3 commits into from
Nov 7, 2019
Merged

Issue #439 Strip out double quotes in ARG value #834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79649a1
Issue #439 double quotes in ARG value
cvgw Oct 24, 2019
ec2e770
Issue #439 add additional tests for quotes
cvgw Oct 25, 2019
0ce287d
ISSUE #439 ci lint
cvgw Oct 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions integration/dockerfiles/Dockerfile_test_arg_multi_with_quotes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
ARG FILE_NAME="myFile"

FROM busybox:latest AS builder
ARG FILE_NAME

RUN echo $FILE_NAME && touch /$FILE_NAME.txt && stat /$FILE_NAME.txt;

FROM busybox:latest
ARG FILE_NAME

RUN echo $FILE_NAME && touch /$FILE_NAME.txt && stat /$FILE_NAME.txt;
COPY --from=builder /$FILE_NAME.txt /
24 changes: 24 additions & 0 deletions pkg/dockerfile/dockerfile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,33 @@ func Parse(b []byte) ([]instructions.Stage, []instructions.ArgCommand, error) {
if err != nil {
return nil, nil, err
}

metaArgs = stripEnclosingDoubleQuotes(metaArgs)

return stages, metaArgs, nil
}

// stripEnclosingDoubleQuotes removes double quotes enclosing the value of each instructions.ArgCommand in a slice
func stripEnclosingDoubleQuotes(metaArgs []instructions.ArgCommand) []instructions.ArgCommand {
Copy link
Contributor

@tejal29 tejal29 Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We can add table driven unit tests here for this function only

  1. value like"foo"
  2. For empty value "" ? What happens then?
    maybe we shd keep quotes as is then?
  3. value foo
  4. invalid value "foo shd probably error out?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some other test cases to add would be
5. When metaArgs is empty
6. Multiple mataArgs instructions have "". (All get resolved)

Copy link
Collaborator

Choose a reason for hiding this comment

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

just something to consider, something like \"foo\" would probably need to maintain the quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add table driven unit tests here for this function only

  1. value like"foo"
  2. For empty value "" ? What happens then?
    maybe we shd keep quotes as is then?
  3. value foo
  4. invalid value "foo shd probably error out?
  1. Docker seems to strip out the quotes and treat it as a valid, but blank string
  2. Yup, Docker also seems to error when either quote isn't matched

We also might want to handle single quotes as docker seems to handle those as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just something to consider, something like \"foo\" would probably need to maintain the quotes

Ya, it seems that Docker does not remove the quotes in that case which will result in an error for things like image name

for i := range metaArgs {
arg := metaArgs[i]
v := arg.Value
if v != nil {
val := *v
if val[0] == '"' {
val = val[1:]
}

if val[len(val)-1] == '"' {
val = val[:len(val)-1]
}
arg.Value = &val
metaArgs[i] = arg
}
}
return metaArgs
}

// targetStage returns the index of the target stage kaniko is trying to build
func targetStage(stages []instructions.Stage, target string) (int, error) {
if target == "" {
Expand Down
51 changes: 51 additions & 0 deletions pkg/dockerfile/dockerfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,64 @@ limitations under the License.
package dockerfile

import (
"io/ioutil"
"os"
"strconv"
"testing"

"github.com/GoogleContainerTools/kaniko/pkg/config"
"github.com/GoogleContainerTools/kaniko/testutil"
"github.com/moby/buildkit/frontend/dockerfile/instructions"
)

func TestStagesArgValueWithQuotes(t *testing.T) {
dockerfile := `
ARG IMAGE="ubuntu:16.04"
FROM ${IMAGE}
RUN echo hi > /hi

FROM scratch AS second
COPY --from=0 /hi /hi2

FROM scratch
COPY --from=second /hi2 /hi3
`
tmpfile, err := ioutil.TempFile("", "Dockerfile.test")
if err != nil {
t.Fatal(err)
}

defer os.Remove(tmpfile.Name())

if _, err := tmpfile.Write([]byte(dockerfile)); err != nil {
t.Fatal(err)
}
if err := tmpfile.Close(); err != nil {
t.Fatal(err)
}

stages, err := Stages(&config.KanikoOptions{DockerfilePath: tmpfile.Name()})
if err != nil {
t.Fatal(err)
}

if len(stages) == 0 {
t.Fatal("length of stages expected to be greater than zero, but was zero")

}

if len(stages[0].MetaArgs) == 0 {
t.Fatal("length of stage[0] meta args expected to be greater than zero, but was zero")
}

expectedVal := "ubuntu:16.04"

arg := stages[0].MetaArgs[0]
if arg.ValueString() != expectedVal {
t.Fatalf("expected stages[0].MetaArgs[0] val to be %s but was %s", expectedVal, arg.ValueString())
}
}

func Test_resolveStages(t *testing.T) {
dockerfile := `
FROM scratch
Expand Down