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 · 97708a0 · 97708a0
1 parent 4b7e2b3
commit 97708a0
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 5 deletions.
diff --git a/integration/dockerfiles/Dockerfile_test_add b/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
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)
+			}
+		})
+	}
+}