Environment variables should be replaced in URLs in ADD commands. (#580)

We were previously explicitly skipping this for some reason, but Docker
seems to expand these in URLs so we should too.

integration/dockerfiles/Dockerfile_test_add

@@ -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

pkg/commands/add.go

@@ -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

pkg/util/command_util.go

@@ -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)
+			resolved, err = ResolveEnvironmentReplacement(value, envs, false)
-			continue
+		} 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
@@ -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 {

pkg/util/command_util_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"reflect"
 	"sort"
 	"testing"
 
@@ -27,14 +28,12 @@ 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",
 		envs: []string{
 			"simple=/path/",
 		},
@@ -43,7 +42,6 @@ var testEnvReplacement = []struct {
 	},
 	{
 		path: "/simple/path/",
-		command: "WORKDIR /simple/path/",
 		envs: []string{
 			"simple=/path/",
 		},
@@ -52,7 +50,6 @@ var testEnvReplacement = []struct {
 	},
 	{
 		path: "${a}/b",
-		command: "WORKDIR ${a}/b",
 		envs: []string{
 			"a=/path/",
 			"b=/path2/",
@@ -62,7 +59,6 @@ var testEnvReplacement = []struct {
 	},
 	{
 		path: "/$a/b",
-		command: "COPY ${a}/b /c/",
 		envs: []string{
 			"a=/path/",
 			"b=/path2/",
@@ -72,7 +68,6 @@ var testEnvReplacement = []struct {
 	},
 	{
 		path: "/$a/b/",
-		command: "COPY /${a}/b /c/",
 		envs: []string{
 			"a=/path/",
 			"b=/path2/",
@@ -82,7 +77,6 @@ var testEnvReplacement = []struct {
 	},
 	{
 		path: "\\$foo",
-		command: "COPY \\$foo /quux",
 		envs: []string{
 			"foo=/path/",
 		},
@@ -91,7 +85,13 @@ var testEnvReplacement = []struct {
 	},
 	{
 		path: "8080/$protocol",
-		command: "EXPOSE 8080/$protocol",
+		envs: []string{
+			"protocol=udp",
+		},
+		expectedPath: "8080/udp",
+	},
+	{
+		path: "8080/$protocol",
 		envs: []string{
 			"protocol=udp",
 		},
@@ -183,6 +183,7 @@ var urlDestFilepathTests = []struct {
 	cwd          string
 	dest         string
 	expectedDest string
+	envs         []string
 }{
 	{
 		url:          "https://something/something",
@@ -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)
+		actualDest, err := URLDestinationFilepath(test.url, test.dest, test.cwd, test.envs)
-		testutil.CheckErrorAndDeepEqual(t, false, nil, test.expectedDest, actualDest)
+		testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedDest, actualDest)
 	}
 }
 
@@ -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)
+			}
+		})
+	}
+}