Environment variables should be replaced in URLs in ADD commands.

We were previously explicitly skipping this for some reason, but Docker seems to expand these in URLs so we should too.
GoogleContainerTools · Feb 22, 2019 · 420b980 · 420b980
1 parent 4b7e2b3
commit 420b980
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 6 deletions.
diff --git a/integration/dockerfiles/Dockerfile_test_add b/integration/dockerfiles/Dockerfile_test_add
@@ -23,5 +23,8 @@ ARG file
 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
+ADD https://github.com/GoogleCloudPlatform/docker-credential-gcr/releases/download/1.4.3/docker-credential-gcr_linux_386-1.4.3.tar.gz context/foo /test/all/
+
+# Test environment replacement in the URL
+ENV VERSION=1.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
diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go
@@ -37,10 +37,6 @@ import (
 func ResolveEnvironmentReplacementList(values, envs []string, isFilepath bool) ([]string, error) {
 	var resolvedValues []string
 	for _, value := range values {
-		if IsSrcRemoteFileURL(value) {
-			resolvedValues = append(resolvedValues, value)
-			continue
-		}
 		resolved, err := ResolveEnvironmentReplacement(value, envs, isFilepath)
 		logrus.Debugf("Resolved %s to %s", value, resolved)
 		if err != nil {

diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"reflect"
 	"sort"
 	"testing"
 
@@ -448,3 +449,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)
+			}
+		})
+	}
+}