Skip to content

Commit

Permalink
Environment variables should be replaced in URLs in ADD commands. (Go…
Browse files Browse the repository at this point in the history 
…ogleContainerTools#580)

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 25, 2019
1 parent 1d079e6 commit 2abe109
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 2abe109

Please sign in to comment.