Skip to content

Commit

Permalink
Environment variables should be replaced in URLs in ADD commands.
Browse files Browse the repository at this point in the history 
We were previously explicitly skipping this for some reason, but Docker
seems to expand these in URLs so we should too.
  • Loading branch information
@dlorenc
dlorenc committed Feb 22, 2019
1 parent 4b7e2b3 commit 77b6c3f
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 26 deletions.
5 changes: 4 additions & 1 deletion integration/dockerfiles/Dockerfile_test_add
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,7 @@ COPY $file /arg

# Finally, test adding a remote URL, concurrently with a normal file
ADD https://github.com/GoogleCloudPlatform/docker-credential-gcr/releases/download/v1.4.3/docker-credential-gcr_linux_386-1.4.3.tar.gz context/foo /test/all/
ADD https://github.com/GoogleCloudPlatform/docker-credential-gcr/releases/download/v1.4.3-static/docker-credential-gcr_linux_amd64-1.4.3.tar.gz /destination

# Test environment replacement in the URL
ENV VERSION=v1.4.3
ADD https://github.com/GoogleCloudPlatform/docker-credential-gcr/releases/download/${VERSION}-static/docker-credential-gcr_linux_amd64-1.4.3.tar.gz /destination
5 changes: 4 additions & 1 deletion pkg/commands/add.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ func (a *AddCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bui
for _, src := range srcs {
fullPath := filepath.Join(a.buildcontext, src)
if util.IsSrcRemoteFileURL(src) {
urlDest := util.URLDestinationFilepath(src, dest, config.WorkingDir)
urlDest, err := util.URLDestinationFilepath(src, dest, config.WorkingDir, replacementEnvs)
if err != nil {
return err
}
logrus.Infof("Adding remote URL %s to %s", src, urlDest)
if err := util.DownloadFileToDest(src, urlDest); err != nil {
return err
Expand Down
20 changes: 13 additions & 7 deletions pkg/util/command_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,13 @@ import (
func ResolveEnvironmentReplacementList(values, envs []string, isFilepath bool) ([]string, error) {
var resolvedValues []string
for _, value := range values {
var resolved string
var err error
if IsSrcRemoteFileURL(value) {
resolvedValues = append(resolvedValues, value)
continue
resolved, err = ResolveEnvironmentReplacement(value, envs, false)
} else {
resolved, err = ResolveEnvironmentReplacement(value, envs, isFilepath)
}
resolved, err := ResolveEnvironmentReplacement(value, envs, isFilepath)
logrus.Debugf("Resolved %s to %s", value, resolved)
if err != nil {
return nil, err
Expand Down Expand Up @@ -165,20 +167,24 @@ func DestinationFilepath(src, dest, cwd string) (string, error) {
}

// URLDestinationFilepath gives the destination a file from a remote URL should be saved to
func URLDestinationFilepath(rawurl, dest, cwd string) string {
func URLDestinationFilepath(rawurl, dest, cwd string, envs []string) (string, error) {
if !IsDestDir(dest) {
if !filepath.IsAbs(dest) {
return filepath.Join(cwd, dest)
return filepath.Join(cwd, dest), nil
}
return dest
return dest, nil
}
urlBase := filepath.Base(rawurl)
urlBase, err := ResolveEnvironmentReplacement(urlBase, envs, true)
if err != nil {
return "", err
}
destPath := filepath.Join(dest, urlBase)

if !filepath.IsAbs(dest) {
destPath = filepath.Join(cwd, destPath)
}
return destPath
return destPath, nil
}

func IsSrcsValid(srcsAndDest instructions.SourcesAndDest, resolvedSources []string, root string) error {
Expand Down
96 changes: 79 additions & 17 deletions pkg/util/command_util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package util

import (
"reflect"
"sort"
"testing"

Expand All @@ -27,32 +28,28 @@ var testURL = "https://github.com/GoogleContainerTools/runtimes-common/blob/mast

var testEnvReplacement = []struct {
path string
command string
envs []string
isFilepath bool
expectedPath string
}{
{
path: "/simple/path",
command: "WORKDIR /simple/path",
path: "/simple/path",
envs: []string{
"simple=/path/",
},
isFilepath: true,
expectedPath: "/simple/path",
},
{
path: "/simple/path/",
command: "WORKDIR /simple/path/",
path: "/simple/path/",
envs: []string{
"simple=/path/",
},
isFilepath: true,
expectedPath: "/simple/path/",
},
{
path: "${a}/b",
command: "WORKDIR ${a}/b",
path: "${a}/b",
envs: []string{
"a=/path/",
"b=/path2/",
Expand All @@ -61,8 +58,7 @@ var testEnvReplacement = []struct {
expectedPath: "/path/b",
},
{
path: "/$a/b",
command: "COPY ${a}/b /c/",
path: "/$a/b",
envs: []string{
"a=/path/",
"b=/path2/",
Expand All @@ -71,8 +67,7 @@ var testEnvReplacement = []struct {
expectedPath: "/path/b",
},
{
path: "/$a/b/",
command: "COPY /${a}/b /c/",
path: "/$a/b/",
envs: []string{
"a=/path/",
"b=/path2/",
Expand All @@ -81,17 +76,22 @@ var testEnvReplacement = []struct {
expectedPath: "/path/b/",
},
{
path: "\\$foo",
command: "COPY \\$foo /quux",
path: "\\$foo",
envs: []string{
"foo=/path/",
},
isFilepath: true,
expectedPath: "$foo",
},
{
path: "8080/$protocol",
command: "EXPOSE 8080/$protocol",
path: "8080/$protocol",
envs: []string{
"protocol=udp",
},
expectedPath: "8080/udp",
},
{
path: "8080/$protocol",
envs: []string{
"protocol=udp",
},
Expand Down Expand Up @@ -183,6 +183,7 @@ var urlDestFilepathTests = []struct {
cwd string
dest string
expectedDest string
envs []string
}{
{
url: "https://something/something",
Expand All @@ -202,12 +203,19 @@ var urlDestFilepathTests = []struct {
dest: "/dest/",
expectedDest: "/dest/something",
},
{
url: "https://something/$foo.tar.gz",
cwd: "/test",
dest: "/foo/",
expectedDest: "/foo/bar.tar.gz",
envs: []string{"foo=bar"},
},
}

func Test_UrlDestFilepath(t *testing.T) {
for _, test := range urlDestFilepathTests {
actualDest := URLDestinationFilepath(test.url, test.dest, test.cwd)
testutil.CheckErrorAndDeepEqual(t, false, nil, test.expectedDest, actualDest)
actualDest, err := URLDestinationFilepath(test.url, test.dest, test.cwd, test.envs)
testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedDest, actualDest)
}
}

Expand Down Expand Up @@ -448,3 +456,57 @@ func Test_RemoteUrls(t *testing.T) {
}

}

func TestResolveEnvironmentReplacementList(t *testing.T) {
type args struct {
values []string
envs []string
isFilepath bool
}
tests := []struct {
name string
args args
want []string
wantErr bool
}{
{
name: "url",
args: args{
values: []string{
"https://google.com/$foo", "$bar",
},
envs: []string{
"foo=baz",
"bar=bat",
},
},
want: []string{"https://google.com/baz", "bat"},
},
{
name: "mixed",
args: args{
values: []string{
"$foo", "$bar$baz", "baz",
},
envs: []string{
"foo=FOO",
"bar=BAR",
"baz=BAZ",
},
},
want: []string{"FOO", "BARBAZ", "baz"},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := ResolveEnvironmentReplacementList(tt.args.values, tt.args.envs, tt.args.isFilepath)
if (err != nil) != tt.wantErr {
t.Errorf("ResolveEnvironmentReplacementList() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("ResolveEnvironmentReplacementList() = %v, want %v", got, tt.want)
}
})
}
}

0 comments on commit 77b6c3f

Please sign in to comment.