Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 25, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion integration/dockerfiles/Dockerfile_test_add
View file
Open in desktop
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
imjasonh marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 4 additions & 1 deletion pkg/commands/add.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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)
}
})
}
}