Apply suggestions from code review

Co-authored-by: John Gardiner Myers <[email protected]>
kubernetes · Apr 26, 2021 · 925df72 · 925df72
1 parent cc444fd
commit 925df72
Show file tree

Hide file tree

Showing 13 changed files with 171 additions and 213 deletions.
diff --git a/k8s/crds/kops.k8s.io_clusters.yaml b/k8s/crds/kops.k8s.io_clusters.yaml
@@ -1146,16 +1146,14 @@ spec:
                     type: boolean
                   permissionsBoundary:
                     type: string
-                required:
-                - legacy
-                type: object
-              iamRolesForServiceAccounts:
-                description: IAMRolesForServiceAccount defines the IRSA configuration.
-                properties:
-                  serviceAccounts:
+                  serviceAccountMappings:
+                    description: ServiceAccountMappings defines the relatinship between
+                      Kubernetes ServiceAccounts and IAM roles.
                     items:
+                      description: ServiceAccountMapping defines the relationship
+                        between a Kubernetes ServiceAccount and an IAM Role.
                       properties:
-                        iamPolicyARN:
+                        iamPolicyARNs:
                           items:
                             type: string
                           type: array
@@ -1170,6 +1168,8 @@ spec:
                       - namespace
                       type: object
                     type: array
+                required:
+                - legacy
                 type: object
               isolateMasters:
                 description: 'IsolateMasters determines whether we should lock down

diff --git a/pkg/apis/kops/cluster.go b/pkg/apis/kops/cluster.go
@@ -205,20 +205,13 @@ type ClusterSpec struct {
 	RollingUpdate *RollingUpdate `json:"rollingUpdate,omitempty"`
 	// ClusterAutoscaler defines the cluster autoscaler configuration.
 	ClusterAutoscaler *ClusterAutoscalerConfig `json:"clusterAutoscaler,omitempty"`
-
-	// IAMRolesForServiceAccount defines the IRSA configuration.
-	IAMRolesForServiceAccounts *IAMRolesForServiceAccountsConfig `json:"iamRolesForServiceAccounts,omitempty"`
-}
-
-// InstanceRoleForServiceAccountConfig defines the IRSA configuration.
-type IAMRolesForServiceAccountsConfig struct {
-	ServiceAccounts []ServiceAccountMapping `json:"serviceAccounts,omitempty"`
 }
 
+// ServiceAccountMapping defines the relationship between a Kubernetes ServiceAccount and an IAM Role.
 type ServiceAccountMapping struct {
 	Name          string   `json:"name"`
 	Namespace     string   `json:"namespace"`
-	IAMPolicyARNs []string `json:"iamPolicyARN,omitempty"`
+	IAMPolicyARNs []string `json:"iamPolicyARNs,omitempty"`
 	InlinePolicy  string   `json:"inlinePolicy,omitempty"`
 }
 
@@ -284,6 +277,8 @@ type IAMSpec struct {
 	Legacy                 bool    `json:"legacy"`
 	AllowContainerRegistry bool    `json:"allowContainerRegistry,omitempty"`
 	PermissionsBoundary    *string `json:"permissionsBoundary,omitempty"`
+	// ServiceAccountMappings defines the relatinship between Kubernetes ServiceAccounts and IAM roles.
+	ServiceAccountMappings []ServiceAccountMapping `json:"serviceAccountMappings,omitempty"`
 }
 
 // HookSpec is a definition hook

diff --git a/pkg/apis/kops/v1alpha2/cluster.go b/pkg/apis/kops/v1alpha2/cluster.go
@@ -204,20 +204,13 @@ type ClusterSpec struct {
 	RollingUpdate *RollingUpdate `json:"rollingUpdate,omitempty"`
 	// ClusterAutoscaler defines the cluaster autoscaler configuration.
 	ClusterAutoscaler *ClusterAutoscalerConfig `json:"clusterAutoscaler,omitempty"`
-
-	// IAMRolesForServiceAccount defines the IRSA configuration.
-	IAMRolesForServiceAccounts *IAMRolesForServiceAccountsConfig `json:"iamRolesForServiceAccounts,omitempty"`
-}
-
-// IAMRoleForServiceAccountConfig defines the IRSA configuration.
-type IAMRolesForServiceAccountsConfig struct {
-	ServiceAccounts []ServiceAccountMapping `json:"serviceAccounts,omitempty"`
 }
 
+// ServiceAccountMapping defines the relationship between a Kubernetes ServiceAccount and an IAM Role.
 type ServiceAccountMapping struct {
 	Name          string   `json:"name"`
 	Namespace     string   `json:"namespace"`
-	IAMPolicyARNs []string `json:"iamPolicyARN,omitempty"`
+	IAMPolicyARNs []string `json:"iamPolicyARNs,omitempty"`
 	InlinePolicy  string   `json:"inlinePolicy,omitempty"`
 }
 
@@ -282,6 +275,8 @@ type IAMSpec struct {
 	Legacy                 bool    `json:"legacy"`
 	AllowContainerRegistry bool    `json:"allowContainerRegistry,omitempty"`
 	PermissionsBoundary    *string `json:"permissionsBoundary,omitempty"`
+	// ServiceAccountMappings defines the relatinship between Kubernetes ServiceAccounts and IAM roles.
+	ServiceAccountMappings []ServiceAccountMapping `json:"serviceAccountMappings,omitempty"`
 }
 
 // HookSpec is a definition hook

diff --git a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go
diff --git a/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go b/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go
diff --git a/pkg/apis/kops/validation/legacy.go b/pkg/apis/kops/validation/legacy.go
@@ -400,34 +400,6 @@ func ValidateCluster(c *kops.Cluster, strict bool) field.ErrorList {
 		}
 	}
 
-	if c.Spec.IAMRolesForServiceAccounts != nil {
-		allErrs = append(allErrs, validateIRSA(c.Spec.IAMRolesForServiceAccounts, fieldSpec.Child("iamRolesForServiceAccounts"))...)
-	}
-	return allErrs
-}
-
-func validateIRSA(irsa *kops.IAMRolesForServiceAccountsConfig, spec *field.Path) (allErrs field.ErrorList) {
-	if !featureflag.PublicJWKS.Enabled() {
-		allErrs = append(allErrs, field.Forbidden(spec, "IAM roles for ServiceAccounts requires PublicJWKS feature flag to be enabled"))
-	}
-
-	sas := make(map[string]string)
-	for _, sa := range irsa.ServiceAccounts {
-		p := spec.Child("serviceAccounts")
-		key := fmt.Sprintf("%s/%s", sa.Namespace, sa.Name)
-		_, duplicate := sas[key]
-		if duplicate {
-			allErrs = append(allErrs, field.Duplicate(p, key))
-		}
-		sas[key] = ""
-
-		if len(sa.IAMPolicyARNs) == 0 && sa.InlinePolicy == "" {
-			allErrs = append(allErrs, field.Required(p, "inlinePolicy or iamPolicyARN must be set"))
-		}
-		if len(sa.IAMPolicyARNs) > 0 && sa.InlinePolicy != "" {
-			allErrs = append(allErrs, field.Forbidden(p, "cannot set both inlinePolicy and iamPolicyARN"))
-		}
-	}
 	return allErrs
 }
 

diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go
@@ -246,6 +246,44 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie
 		allErrs = append(allErrs, validateCloudConfiguration(spec.CloudConfig, fieldPath.Child("cloudConfig"))...)
 	}
 
+	if spec.IAM != nil {
+		allErrs = append(allErrs, validateSAMappings(spec.IAM.ServiceAccountMappings, fieldPath.Child("iam", "serviceAccountMappings"))...)
+	}
+
+	return allErrs
+}
+
+func validateSAMappings(mappings []kops.ServiceAccountMapping, path *field.Path) (allErrs field.ErrorList) {
+	if len(mappings) == 0 {
+		return allErrs
+	}
+	if !featureflag.PublicJWKS.Enabled() {
+		allErrs = append(allErrs, field.Forbidden(path, "IAM roles for ServiceAccounts requires PublicJWKS feature flag to be enabled"))
+	}
+
+	sas := make(map[string]string)
+	for _, sa := range mappings {
+		key := fmt.Sprintf("%s/%s", sa.Namespace, sa.Name)
+		p := path.Key(key)
+		if sa.Namespace == "" {
+			allErrs = append(allErrs, field.Required(p.Child("namespace"), "namespace cannot be empty"))
+		}
+		if sa.Name == "" {
+			allErrs = append(allErrs, field.Required(p.Child("name"), "name cannot be empty"))
+		}
+		_, duplicate := sas[key]
+		if duplicate {
+			allErrs = append(allErrs, field.Duplicate(p, key))
+		}
+		sas[key] = ""
+
+		if len(sa.IAMPolicyARNs) == 0 && sa.InlinePolicy == "" {
+			allErrs = append(allErrs, field.Required(p, "either inlinePolicy or iamPolicyARN must be set"))
+		}
+		if len(sa.IAMPolicyARNs) > 0 && sa.InlinePolicy != "" {
+			allErrs = append(allErrs, field.Forbidden(p, "cannot set both inlinePolicy and iamPolicyARN"))
+		}
+	}
 	return allErrs
 }