Merge pull request #1027 from tejal29/small_refacto

refactor copy.chown code and add more tests

pkg/commands/copy.go

@@ -32,6 +32,11 @@ import (
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 )
 
+// for testing
+var (
+	getUIDAndGID = util.GetUIDAndGIDFromString
+)
+
 type CopyCommand struct {
 	BaseCommand
 	cmd           *instructions.CopyCommand
@@ -44,23 +49,12 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
 	if c.cmd.From != "" {
 		c.buildcontext = filepath.Join(constants.KanikoDir, c.cmd.From)
 	}
-	var uid, gid int64
-	uid = util.DoNotChangeUID
-	gid = util.DoNotChangeGID
 
 	replacementEnvs := buildArgs.ReplacementEnvs(config.Env)
 
-	if c.cmd.Chown != "" {
-		chown, err := util.ResolveEnvironmentReplacement(c.cmd.Chown, replacementEnvs, false)
-		if err != nil {
-			return err
-		}
-		uid32, gid32, err := util.GetUIDAndGIDFromString(chown, true)
-		uid = int64(uid32)
-		gid = int64(gid32)
-		if err != nil {
-			return err
-		}
+	uid, gid, err := getUserGroup(c.cmd.Chown, replacementEnvs)
+	if err != nil {
+		return err
 	}
 
 	srcs, dest, err := util.ResolveEnvAndWildcards(c.cmd.SourcesAndDest, c.buildcontext, replacementEnvs)
@@ -126,6 +120,21 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
 	return nil
 }
 
+func getUserGroup(chownStr string, env []string) (int64, int64, error) {
+	if chownStr == "" {
+		return util.DoNotChangeUID, util.DoNotChangeGID, nil
+	}
+	chown, err := util.ResolveEnvironmentReplacement(chownStr, env, false)
+	if err != nil {
+		return -1, -1, err
+	}
+	uid32, gid32, err := getUIDAndGID(chown, true)
+	if err != nil {
+		return -1, -1, err
+	}
+	return int64(uid32), int64(gid32), nil
+}
+
 // FilesToSnapshot should return an empty array if still nil; no files were changed
 func (c *CopyCommand) FilesToSnapshot() []string {
 	return c.snapshotFiles

pkg/commands/copy_test.go

@@ -387,3 +387,55 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
 		})
 	}
 }
+
+func TestGetUserGroup(t *testing.T) {
+	tests := []struct {
+		description string
+		chown       string
+		env         []string
+		mock        func(string, bool) (uint32, uint32, error)
+		expectedU   int64
+		expectedG   int64
+		shdErr      bool
+	}{
+		{
+			description: "non empty chown",
+			chown:       "some:some",
+			env:         []string{},
+			mock:        func(string, bool) (uint32, uint32, error) { return 100, 1000, nil },
+			expectedU:   100,
+			expectedG:   1000,
+		},
+		{
+			description: "non empty chown with env replacement",
+			chown:       "some:$foo",
+			env:         []string{"foo=key"},
+			mock: func(c string, t bool) (uint32, uint32, error) {
+				if c == "some:key" {
+					return 10, 100, nil
+				}
+				return 0, 0, fmt.Errorf("did not resolve environment variable")
+			},
+			expectedU: 10,
+			expectedG: 100,
+		},
+		{
+			description: "empty chown string",
+			mock: func(c string, t bool) (uint32, uint32, error) {
+				return 0, 0, fmt.Errorf("should not be called")
+			},
+			expectedU: -1,
+			expectedG: -1,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.description, func(t *testing.T) {
+			original := getUIDAndGID
+			defer func() { getUIDAndGID = original }()
+			getUIDAndGID = tc.mock
+			uid, gid, err := getUserGroup(tc.chown, tc.env)
+			testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, uid, tc.expectedU)
+			testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, gid, tc.expectedG)
+		})
+	}
+}