Merge pull request kubernetes-retired#443 from mumoshu/validate-iam-r…

…ole-name-len

Documentation and validation for too long IAM role names

cfnresource/naming.go

@@ -0,0 +1,14 @@
+package cfnresource
+
+import (
+	"fmt"
+)
+
+func ValidateRoleNameLength(clusterName string, nestedStackLogicalName string, managedIAMRoleName string, region string) error {
+	name := fmt.Sprintf("%s-%s-PRK1CVQNY7XZ-%s-%s", clusterName, nestedStackLogicalName, region, managedIAMRoleName)
+	if len(name) > 64 {
+		limit := 64 - len(name) + len(clusterName) + len(nestedStackLogicalName) + len(managedIAMRoleName)
+		return fmt.Errorf("IAM role name(=%s) will be %d characters long. It exceeds the AWS limit of 64 characters: cluster name(=%s) + nested stack name(=%s) + managed iam role name(=%s) should be less than or equal to %d", name, len(name), clusterName, nestedStackLogicalName, managedIAMRoleName, limit)
+	}
+	return nil
+}

cfnresource/naming_test.go

@@ -0,0 +1,16 @@
+package cfnresource
+
+import "testing"
+
+func TestValidateRoleNameLength(t *testing.T) {
+	t.Run("WhenMax", func(t *testing.T) {
+		if e := ValidateRoleNameLength("my-firstcluster", "prodWorkerks", "prod-workers", "us-east-1"); e != nil {
+			t.Errorf("expected validation to succeed but failed: %v", e)
+		}
+	})
+	t.Run("WhenTooLong", func(t *testing.T) {
+		if e := ValidateRoleNameLength("my-secondcluster", "prodWorkerks", "prod-workers", "us-east-1"); e == nil {
+			t.Error("expected validation to fail but succeeded")
+		}
+	})
+}

core/controlplane/config/config.go

@@ -12,6 +12,7 @@ import (
 	"unicode/utf8"
 
 	"github.com/coreos/go-semver/semver"
+	"github.com/coreos/kube-aws/cfnresource"
 	"github.com/coreos/kube-aws/coreos/amiregistry"
 	"github.com/coreos/kube-aws/filereader/userdatatemplate"
 	"github.com/coreos/kube-aws/model"
@@ -846,6 +847,13 @@ func (c Config) InternetGatewayRef() string {
 	}
 }
 
+// NestedStackName returns a sanitized name of this control-plane which is usable as a valid cloudformation nested stack name
+func (c Cluster) NestedStackName() string {
+	// Convert stack name into something valid as a cfn resource name or
+	// we'll end up with cfn errors like "Template format error: Resource name test5-controlplane is non alphanumeric"
+	return strings.Title(strings.Replace(c.StackName(), "-", "", -1))
+}
+
 func (c Cluster) valid() error {
 	validClusterNaming := regexp.MustCompile("^[a-zA-Z0-9-:]+$")
 	if !validClusterNaming.MatchString(c.ClusterName) {
@@ -951,6 +959,10 @@ func (c Cluster) valid() error {
 		fmt.Println(`WARNING: instance types "t2.nano" and "t2.micro" are not recommended. See https://github.com/coreos/kube-aws/issues/258 for more information`)
 	}
 
+	if e := cfnresource.ValidateRoleNameLength(c.ClusterName, c.NestedStackName(), c.Controller.ManagedIamRoleName, c.Region.String()); e != nil {
+		return e
+	}
+
 	return nil
 }
 

core/controlplane/config/templates/cluster.yaml

@@ -63,6 +63,10 @@ kmsKeyArn: "{{.KMSKeyARN}}"
 #  # Role will be created with "Ref": {"AWS::StackName"}-{"AWS::Region"}-yourManagedRole
 #  # to follow the recommendation in AWS documentation  http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html
 #  # This is also intended to be used in combination with .experimental.kube2IamSupport. See #297 for more information.
+#  #
+#  # ATTENTION: Consider limiting number of characters in clusterName and managedIamRoleName to avoid the resulting IAM
+#  # role name's length from exceeding the AWS limit: 64
+#  # See https://github.com/kubernetes-incubator/kube-aws/issues/347
 #  managedIamRoleName: "yourManagedRole"
 #  # If omitted, public subnets are created by kube-aws and used for controller nodes
 #  subnets:
@@ -143,6 +147,10 @@ worker:
 #      # Role will be created with "Ref": {"AWS::StackName"}-{"AWS::Region"}-yourManagedRole
 #      # to follow the recommendation in AWS documentation  http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html
 #      # This is also intended to be used in combination with kube2IamSupport. See #297 for more information.
+#      #
+#      # ATTENTION: Consider limiting number of characters in clusterName, nodePools[].name and managedIamRoleName to
+#      # avoid the resulting IAM role name's length from exceeding the AWS limit: 64
+#      # See https://github.com/kubernetes-incubator/kube-aws/issues/347
 #      managedIamRoleName: "yourManagedRole"
 #
 #      # Additional EBS volumes mounted on the worker

core/nodepool/config/config.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"errors"
+	"github.com/coreos/kube-aws/cfnresource"
 	cfg "github.com/coreos/kube-aws/core/controlplane/config"
 	"github.com/coreos/kube-aws/coreos/amiregistry"
 	"github.com/coreos/kube-aws/filereader/userdatatemplate"
@@ -56,6 +57,13 @@ type StackTemplateOptions struct {
 	SkipWait              bool
 }
 
+// NestedStackName returns a sanitized name of this node pool which is usable as a valid cloudformation nested stack name
+func (c ProvidedConfig) NestedStackName() string {
+	// Convert stack name into something valid as a cfn resource name or
+	// we'll end up with cfn errors like "Template format error: Resource name test5-controlplane is non alphanumeric"
+	return strings.Title(strings.Replace(c.StackName(), "-", "", -1))
+}
+
 func (c ProvidedConfig) StackConfig(opts StackTemplateOptions) (*StackConfig, error) {
 	var err error
 	stackConfig := StackConfig{}
@@ -261,6 +269,10 @@ func (c ProvidedConfig) valid() error {
 		return fmt.Errorf("awsNodeLabels can't be enabled for node pool because the total number of characters in clusterName(=\"%s\") + node pool's name(=\"%s\") exceeds the limit of %d", c.ClusterName, c.NodePoolName, limit)
 	}
 
+	if e := cfnresource.ValidateRoleNameLength(c.ClusterName, c.NestedStackName(), c.ManagedIamRoleName, c.Region.String()); e != nil {
+		return e
+	}
+
 	return nil
 }
 

core/root/template_params.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	controlplane "github.com/coreos/kube-aws/core/controlplane/cluster"
 	nodepool "github.com/coreos/kube-aws/core/nodepool/cluster"
-	"strings"
 )
 
 type TemplateParams struct {
@@ -32,8 +31,7 @@ type controlPlane struct {
 }
 
 func (p controlPlane) Name() string {
-	stackName := p.controlPlane.StackName()
-	return strings.Title(strings.Replace(stackName, "-", "", -1))
+	return p.controlPlane.NestedStackName()
 }
 
 func (p controlPlane) Tags() map[string]string {
@@ -55,10 +53,7 @@ type nodePool struct {
 }
 
 func (p nodePool) Name() string {
-	// Convert stack name into something valid as a cfn resource name or
-	// we'll end up with cfn errors like "Template format error: Resource name test5-controlplane is non alphanumeric"
-	stackName := p.nodePool.StackName()
-	return strings.Title(strings.Replace(stackName, "-", "", -1))
+	return p.nodePool.NestedStackName()
 }
 
 func (p nodePool) Tags() map[string]string {

test/integration/aws_test.go

@@ -99,3 +99,8 @@ func (s kubeAwsSettings) withClusterName(n string) kubeAwsSettings {
 	s.clusterName = n
 	return s
 }
+
+func (s kubeAwsSettings) withRegion(r string) kubeAwsSettings {
+	s.region = r
+	return s
+}

test/integration/maincluster_test.go

@@ -1110,23 +1110,23 @@ worker:
 			context: "WithKube2IamSupport",
 			configYaml: minimalValidConfigYaml + `
 controller:
-  managedIamRoleName: mycontrollerrole
+  managedIamRoleName: myrole1
 experimental:
   kube2IamSupport:
     enabled: true
 worker:
   nodePools:
   - name: pool1
-    managedIamRoleName: myworkerrole
+    managedIamRoleName: myrole2
     kube2IamSupport:
       enabled: true
 `,
 			assertConfig: []ConfigTester{
 				hasDefaultEtcdSettings,
 				asgBasedNodePoolHasWaitSignalEnabled,
 				func(c *config.Config, t *testing.T) {
-					expectedControllerRoleName := "mycontrollerrole"
-					expectedWorkerRoleName := "myworkerrole"
+					expectedControllerRoleName := "myrole1"
+					expectedWorkerRoleName := "myrole2"
 
 					if expectedControllerRoleName != c.Controller.ManagedIamRoleName {
 						t.Errorf("controller's managedIamRoleName didn't match : expected=%v actual=%v", expectedControllerRoleName, c.Controller.ManagedIamRoleName)
@@ -2333,14 +2333,14 @@ routeTableId: rtb-1a2b3c4d
 worker:
   nodePools:
   - name: pool1
-    managedIamRoleName: "yourManagedRole"
+    managedIamRoleName: "myManagedRole"
 `,
 			assertConfig: []ConfigTester{
 				hasDefaultEtcdSettings,
 				hasDefaultExperimentalFeatures,
 				func(c *config.Config, t *testing.T) {
-					if c.NodePools[0].ManagedIamRoleName != "yourManagedRole" {
-						t.Errorf("managedIamRoleName: expected=yourManagedRole actual=%s", c.NodePools[0].ManagedIamRoleName)
+					if c.NodePools[0].ManagedIamRoleName != "myManagedRole" {
+						t.Errorf("managedIamRoleName: expected=myManagedRole actual=%s", c.NodePools[0].ManagedIamRoleName)
 					}
 				},
 			},
@@ -2818,6 +2818,24 @@ worker:
 `,
 			expectedErrorMessage: "unknown keys found in worker.nodePools[0].clusterAutoscaler: baz",
 		},
+		{
+			context: "WithTooLongControllerIAMRoleName",
+			configYaml: kubeAwsSettings.withClusterName("kubeaws-it-main").withRegion("ap-northeast-1").minimumValidClusterYaml() + `
+controller:
+  managedIamRoleName: foobarba
+`,
+			expectedErrorMessage: "IAM role name(=kubeaws-it-main-Controlplane-PRK1CVQNY7XZ-ap-northeast-1-foobarba) will be 65 characters long. It exceeds the AWS limit of 64 characters: cluster name(=kubeaws-it-main) + nested stack name(=Controlplane) + managed iam role name(=foobarba) should be less than or equal to 34",
+		},
+		{
+			context: "WithTooLongWorkerIAMRoleName",
+			configYaml: kubeAwsSettings.withClusterName("kubeaws-it-main").withRegion("ap-northeast-1").minimumValidClusterYaml() + `
+worker:
+  nodePools:
+  - name: pool1
+    managedIamRoleName: foobarbazbaraaa
+`,
+			expectedErrorMessage: "IAM role name(=kubeaws-it-main-Pool1-PRK1CVQNY7XZ-ap-northeast-1-foobarbazbaraaa) will be 65 characters long. It exceeds the AWS limit of 64 characters: cluster name(=kubeaws-it-main) + nested stack name(=Pool1) + managed iam role name(=foobarbazbaraaa) should be less than or equal to 34",
+		},
 	}
 
 	for _, invalidCase := range parseErrorCases {