From 956495784e8b14dea3a80b0d028770f5027d38b0 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Fri, 31 Jan 2020 15:19:57 -0800 Subject: [PATCH 1/4] fix group string being always set to uid in case a user has a gid set --- pkg/commands/user.go | 11 ++++++++++- pkg/commands/user_test.go | 20 +++++++++++++++++++- pkg/util/command_util.go | 15 +++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/pkg/commands/user.go b/pkg/commands/user.go index f79e1bd60..35568f74d 100644 --- a/pkg/commands/user.go +++ b/pkg/commands/user.go @@ -26,6 +26,11 @@ import ( "github.com/sirupsen/logrus" ) +// for testing +var ( + Lookup = util.Lookup +) + type UserCommand struct { BaseCommand cmd *instructions.UserCommand @@ -40,7 +45,11 @@ func (r *UserCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu if err != nil { return err } - groupStr := userStr + userObj, err := Lookup(userStr) + if err != nil { + return err + } + groupStr := userObj.Gid if len(userAndGroup) > 1 { groupStr, err = util.ResolveEnvironmentReplacement(userAndGroup[1], replacementEnvs, false) if err != nil { diff --git a/pkg/commands/user_test.go b/pkg/commands/user_test.go index 916f98c0f..a0db5decb 100644 --- a/pkg/commands/user_test.go +++ b/pkg/commands/user_test.go @@ -16,9 +16,11 @@ limitations under the License. package commands import ( + "os/user" "testing" "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" + "github.com/GoogleContainerTools/kaniko/pkg/util" "github.com/GoogleContainerTools/kaniko/testutil" v1 "github.com/google/go-containerregistry/pkg/v1" @@ -27,52 +29,64 @@ import ( var userTests = []struct { user string + userObj user.User expectedUID string expectedGID string }{ { user: "root", + userObj: user.User{Uid: "root", Gid: "root"}, expectedUID: "root:root", }, { user: "root-add", - expectedUID: "root-add:root-add", + userObj: user.User{Uid: "root-add", Gid: "root"}, + expectedUID: "root-add:root", }, { user: "0", + userObj: user.User{Uid: "0", Gid: "0"}, expectedUID: "0:0", }, { user: "fakeUser", + userObj: user.User{Uid: "fakeUser", Gid: "fakeUser"}, expectedUID: "fakeUser:fakeUser", }, { user: "root:root", + userObj: user.User{Uid: "root", Gid: "some"}, expectedUID: "root:root", }, { user: "0:root", + userObj: user.User{Uid: "0"}, expectedUID: "0:root", }, { user: "root:0", + userObj: user.User{Uid: "root"}, expectedUID: "root:0", expectedGID: "f0", }, { user: "0:0", + userObj: user.User{Uid: "0"}, expectedUID: "0:0", }, { user: "$envuser", + userObj: user.User{Uid: "root", Gid: "root"}, expectedUID: "root:root", }, { user: "root:$envgroup", + userObj: user.User{Uid: "root"}, expectedUID: "root:grp", }, { user: "some:grp", + userObj: user.User{Uid: "some"}, expectedUID: "some:grp", }, } @@ -90,6 +104,10 @@ func TestUpdateUser(t *testing.T) { User: test.user, }, } + Lookup = func(_ string) (*user.User, error) { + return &test.userObj, nil + } + defer func() { Lookup = util.Lookup }() buildArgs := dockerfile.NewBuildArgs([]string{}) err := cmd.ExecuteCommand(cfg, buildArgs) testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedUID, cfg.User) diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index 6f391e133..3fdcf7d19 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -363,3 +363,18 @@ func GetUserFromUsername(userStr string, groupStr string) (string, string, error return uid, gid, nil } + +func Lookup(userStr string) (*user.User, error) { + userObj, err := user.Lookup(userStr) + if err != nil { + if _, ok := err.(user.UnknownUserError); !ok { + return nil, err + } + // Lookup by id + userObj, err = user.LookupId(userStr) + if err != nil { + return nil, err + } + } + return userObj, nil +} From 6894be442fefa7d90f6195b4c187fd46ae3f3163 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Fri, 31 Jan 2020 15:32:52 -0800 Subject: [PATCH 2/4] remove duplicate code --- pkg/util/command_util.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index 3fdcf7d19..cadf6e564 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -328,16 +328,9 @@ Loop: func GetUserFromUsername(userStr string, groupStr string) (string, string, error) { // Lookup by username - userObj, err := user.Lookup(userStr) + userObj, err := Lookup(userStr) if err != nil { - if _, ok := err.(user.UnknownUserError); !ok { - return "", "", err - } - // Lookup by id - userObj, err = user.LookupId(userStr) - if err != nil { - return "", "", err - } + return "", "", err } // Same dance with groups From db6f6c5ad3d6c74369df4d3ba9fcdb521db8a44a Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Mon, 3 Feb 2020 10:11:40 -0800 Subject: [PATCH 3/4] more tests and do not error out if user not found --- pkg/commands/user.go | 21 ++++++++++++++------- pkg/commands/user_test.go | 34 +++++++++++++++++++++------------- 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/pkg/commands/user.go b/pkg/commands/user.go index 35568f74d..d1e4ceb94 100644 --- a/pkg/commands/user.go +++ b/pkg/commands/user.go @@ -17,6 +17,8 @@ limitations under the License. package commands import ( + "fmt" + "github.com/pkg/errors" "strings" "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" @@ -43,17 +45,13 @@ func (r *UserCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu replacementEnvs := buildArgs.ReplacementEnvs(config.Env) userStr, err := util.ResolveEnvironmentReplacement(userAndGroup[0], replacementEnvs, false) if err != nil { - return err + return errors.Wrap(err, fmt.Sprintf("resolving user %s", userAndGroup[0])) } - userObj, err := Lookup(userStr) - if err != nil { - return err - } - groupStr := userObj.Gid + var groupStr = setGroupDefault(userStr) if len(userAndGroup) > 1 { groupStr, err = util.ResolveEnvironmentReplacement(userAndGroup[1], replacementEnvs, false) if err != nil { - return err + return errors.Wrap(err, fmt.Sprintf("resolving group %s", userAndGroup[1])) } } @@ -66,3 +64,12 @@ func (r *UserCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu func (r *UserCommand) String() string { return r.cmd.String() } + +func setGroupDefault(userStr string) string { + userObj, err := Lookup(userStr) + if err != nil { + logrus.Debugf("could not lookup user %s. Setting group empty", userStr) + return "" + } + return userObj.Gid +} diff --git a/pkg/commands/user_test.go b/pkg/commands/user_test.go index a0db5decb..fc90e2ca9 100644 --- a/pkg/commands/user_test.go +++ b/pkg/commands/user_test.go @@ -16,6 +16,7 @@ limitations under the License. package commands import ( + "fmt" "os/user" "testing" @@ -29,66 +30,70 @@ import ( var userTests = []struct { user string - userObj user.User + userObj *user.User expectedUID string expectedGID string }{ { user: "root", - userObj: user.User{Uid: "root", Gid: "root"}, + userObj: &user.User{Uid: "root", Gid: "root"}, expectedUID: "root:root", }, { user: "root-add", - userObj: user.User{Uid: "root-add", Gid: "root"}, + userObj: &user.User{Uid: "root-add", Gid: "root"}, expectedUID: "root-add:root", }, { user: "0", - userObj: user.User{Uid: "0", Gid: "0"}, + userObj: &user.User{Uid: "0", Gid: "0"}, expectedUID: "0:0", }, { user: "fakeUser", - userObj: user.User{Uid: "fakeUser", Gid: "fakeUser"}, + userObj: &user.User{Uid: "fakeUser", Gid: "fakeUser"}, expectedUID: "fakeUser:fakeUser", }, { user: "root:root", - userObj: user.User{Uid: "root", Gid: "some"}, + userObj: &user.User{Uid: "root", Gid: "some"}, expectedUID: "root:root", }, { user: "0:root", - userObj: user.User{Uid: "0"}, + userObj: &user.User{Uid: "0"}, expectedUID: "0:root", }, { user: "root:0", - userObj: user.User{Uid: "root"}, + userObj: &user.User{Uid: "root"}, expectedUID: "root:0", expectedGID: "f0", }, { user: "0:0", - userObj: user.User{Uid: "0"}, + userObj: &user.User{Uid: "0"}, expectedUID: "0:0", }, { user: "$envuser", - userObj: user.User{Uid: "root", Gid: "root"}, + userObj: &user.User{Uid: "root", Gid: "root"}, expectedUID: "root:root", }, { user: "root:$envgroup", - userObj: user.User{Uid: "root"}, + userObj: &user.User{Uid: "root"}, expectedUID: "root:grp", }, { user: "some:grp", - userObj: user.User{Uid: "some"}, + userObj: &user.User{Uid: "some"}, expectedUID: "some:grp", }, + { + user: "some", + expectedUID: "some:", + }, } func TestUpdateUser(t *testing.T) { @@ -105,7 +110,10 @@ func TestUpdateUser(t *testing.T) { }, } Lookup = func(_ string) (*user.User, error) { - return &test.userObj, nil + if test.userObj != nil { + return test.userObj, nil + } + return nil, fmt.Errorf("error while looking up user.") } defer func() { Lookup = util.Lookup }() buildArgs := dockerfile.NewBuildArgs([]string{}) From 18fec3ba38d3c64761f2ea4bfc58701d98156c15 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Mon, 3 Feb 2020 11:03:58 -0800 Subject: [PATCH 4/4] fix lint --- pkg/commands/user.go | 2 +- pkg/commands/user_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/commands/user.go b/pkg/commands/user.go index d1e4ceb94..702e5594d 100644 --- a/pkg/commands/user.go +++ b/pkg/commands/user.go @@ -18,13 +18,13 @@ package commands import ( "fmt" - "github.com/pkg/errors" "strings" "github.com/GoogleContainerTools/kaniko/pkg/dockerfile" "github.com/GoogleContainerTools/kaniko/pkg/util" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/moby/buildkit/frontend/dockerfile/instructions" + "github.com/pkg/errors" "github.com/sirupsen/logrus" ) diff --git a/pkg/commands/user_test.go b/pkg/commands/user_test.go index fc90e2ca9..455295f7c 100644 --- a/pkg/commands/user_test.go +++ b/pkg/commands/user_test.go @@ -113,7 +113,7 @@ func TestUpdateUser(t *testing.T) { if test.userObj != nil { return test.userObj, nil } - return nil, fmt.Errorf("error while looking up user.") + return nil, fmt.Errorf("error while looking up user") } defer func() { Lookup = util.Lookup }() buildArgs := dockerfile.NewBuildArgs([]string{})