diff --git a/pkg/cmd/generate/cluster.go b/pkg/cmd/generate/cluster.go index b34b5f3..75820ce 100644 --- a/pkg/cmd/generate/cluster.go +++ b/pkg/cmd/generate/cluster.go @@ -1,7 +1,11 @@ package generate import ( + "errors" + "strings" + "github.com/kubesaw/ksctl/pkg/configuration" + "k8s.io/apimachinery/pkg/util/validation" ) type clusterContext struct { @@ -49,6 +53,9 @@ func ensureUsers(ctx *clusterContext, objsCache objectsCache) error { if user.Selector.ShouldBeSkippedForMember(ctx.specificKMemberName) { continue } + if err := validateUserName(user.Name); err != nil { + return err + } m := &permissionsManager{ objectsCache: objsCache, createSubject: ensureUserIdentityAndGroups(user.ID, user.Groups), @@ -67,3 +74,27 @@ func ensureUsers(ctx *clusterContext, objsCache objectsCache) error { return nil } + +func validateUserName(userName string) error { + // check if the userName matches the format for k8s resource names + validationErrors := validation.IsDNS1123Subdomain(userName) + if len(validationErrors) == 0 { + // check if the userName matches the format for k8s label values + validationErrors = validation.IsValidLabelValue(userName) + if len(validationErrors) == 0 { + return nil + } + } + errs := make([]error, len(validationErrors)) + for i := 0; i < len(validationErrors); i++ { + // the returned messages from the validators contain strings mentioning + // labels (in the case of label value validators), and subdomains (in the + // case of resource name validators). This can be confusing so let's replace + // these two strings with "User name" so it refers to the name defined in + // kubesaw-admins.yaml file. + message := strings.ReplaceAll(validationErrors[i], "label", "User name") + message = strings.ReplaceAll(message, "subdomain", "User name") + errs[i] = errors.New(message) + } + return errors.Join(errs...) +} diff --git a/pkg/cmd/generate/cluster_test.go b/pkg/cmd/generate/cluster_test.go index 43afe19..50b0623 100644 --- a/pkg/cmd/generate/cluster_test.go +++ b/pkg/cmd/generate/cluster_test.go @@ -9,6 +9,7 @@ import ( "github.com/kubesaw/ksctl/pkg/assets" "github.com/kubesaw/ksctl/pkg/configuration" . "github.com/kubesaw/ksctl/pkg/test" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -230,6 +231,26 @@ func TestUsers(t *testing.T) { hasNsClusterRole("toolchain-member-operator", "view", configuration.Member.AsSuffix("clusterrole-view-bob-crtadmin")). hasClusterRoleBinding("cluster-monitoring-view", configuration.Member.AsSuffix("clusterrole-cluster-monitoring-view-bob-crtadmin")) }) + + t.Run("returns error for invalid username", func(t *testing.T) { + for _, username := range invalidUserNames { + t.Run("username: "+username, func(t *testing.T) { + // given + kubeSawAdmins := newKubeSawAdminsWithDefaultClusters( + ServiceAccounts(), + Users(User("john+user", []string{"12345"}, true, "", permissionsForAllNamespaces...)), + ) + ctx := newAdminManifestsContextWithDefaultFiles(t, kubeSawAdmins) + clusterCtx := newFakeClusterContext(ctx, configuration.Host) + + // when + err := ensureUsers(clusterCtx, objectsCache{}) + + // then + require.Error(t, err) + }) + } + }) } func newKubeSawAdminsWithDefaultClusters(serviceAccounts []assets.ServiceAccount, users []assets.User) *assets.KubeSawAdmins { @@ -238,3 +259,36 @@ func newKubeSawAdminsWithDefaultClusters(serviceAccounts []assets.ServiceAccount serviceAccounts, users) } + +var invalidUserNames = []string{ + "special#-name", "special:-name", "-name", "name-", "special_name", "specialName", + "some-very-very-very-very-very-very-very-very-very-very-very-long-name", +} + +func TestSanitizeUserName(t *testing.T) { + t.Run("valid usernames", func(t *testing.T) { + require.NoError(t, validateUserName("special-name")) + require.NoError(t, validateUserName("special.name")) + }) + t.Run("invalid usernames", func(t *testing.T) { + for _, username := range invalidUserNames { + t.Run("username: "+username, func(t *testing.T) { + // when + err := validateUserName(username) + + // then + // Let's not hard-code the actual expected errors here. It's something that we don't own, + // and it would be just matter of copy-pasting what is in the apimachinery library. Apart + // from that, the error are not short which would make the code really messy. + require.Error(t, err) + // the returned messages from the k8s validators contain strings mentioning + // labels (in the case of label value validators), and subdomains (in the + // case of resource name validators). The validateUserName function replaced + // these words to not make the final error message confusing. + // Let's verify that the final error doesn't contain these words. + assert.NotContains(t, err.Error(), "label") + assert.NotContains(t, err.Error(), "subdomain") + }) + } + }) +}