Merge pull request kubernetes-sigs#4079 from Skarlso/fix-cf-resource-delete-order

bug: order of deleting cloud formation resources matters and fix missing GroupName setting from Bootstrap user

Author: k8s-ci-robot

cmd/clusterawsadm/api/bootstrap/v1beta1/defaults.go

@@ -17,7 +17,7 @@ limitations under the License.
 package v1beta1
 
 import (
-	runtime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/pointer"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
@@ -27,6 +27,8 @@ import (
 const (
 	// DefaultBootstrapUserName is the default bootstrap user name.
 	DefaultBootstrapUserName = "bootstrapper.cluster-api-provider-aws.sigs.k8s.io"
+	// DefaultBootstrapGroupName is the default bootstrap user name.
+	DefaultBootstrapGroupName = "bootstrapper.cluster-api-provider-aws.sigs.k8s.io"
 	// DefaultStackName is the default CloudFormation stack name.
 	DefaultStackName = "cluster-api-provider-aws-sigs-k8s-io"
 	// DefaultPartitionName is the default security partition for AWS ARNs.
@@ -43,8 +45,13 @@ func addDefaultingFuncs(scheme *runtime.Scheme) error {
 
 // SetDefaults_BootstrapUser is used by defaulter-gen.
 func SetDefaults_BootstrapUser(obj *BootstrapUser) { //nolint:golint,stylecheck
-	if obj != nil && obj.UserName == "" {
-		obj.UserName = DefaultBootstrapUserName
+	if obj != nil {
+		if obj.UserName == "" {
+			obj.UserName = DefaultBootstrapUserName
+		}
+		if obj.GroupName == "" {
+			obj.GroupName = DefaultBootstrapGroupName
+		}
 	}
 }
 

cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml

@@ -1,7 +1,8 @@
 AWSTemplateFormatVersion: 2010-09-09
 Resources:
   AWSIAMGroupBootstrapper:
-    Properties: {}
+    Properties:
+      GroupName: bootstrapper.cluster-api-provider-aws.sigs.k8s.io
     Type: AWS::IAM::Group
   AWSIAMInstanceProfileControlPlane:
     Properties:

cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml

@@ -1,7 +1,8 @@
 AWSTemplateFormatVersion: 2010-09-09
 Resources:
   AWSIAMGroupBootstrapper:
-    Properties: {}
+    Properties:
+      GroupName: bootstrapper.cluster-api-provider-aws.sigs.k8s.io
     Type: AWS::IAM::Group
   AWSIAMInstanceProfileControlPlane:
     Properties:

cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml

@@ -1,7 +1,8 @@
 AWSTemplateFormatVersion: 2010-09-09
 Resources:
   AWSIAMGroupBootstrapper:
-    Properties: {}
+    Properties:
+      GroupName: bootstrapper.cluster-api-provider-aws.sigs.k8s.io
     Type: AWS::IAM::Group
   AWSIAMInstanceProfileControlPlane:
     Properties:

test/e2e/shared/aws.go

@@ -469,73 +469,104 @@ func SetMultitenancyEnvVars(prov client.ConfigProvider) error {
 func deleteResourcesInCloudFormation(prov client.ConfigProvider, t *cfn_bootstrap.Template) {
 	iamSvc := iam.New(prov)
 	temp := *renderCustomCloudFormation(t)
+	var (
+		iamRoles         []*cfn_iam.Role
+		instanceProfiles []*cfn_iam.InstanceProfile
+		policies         []*cfn_iam.ManagedPolicy
+		groups           []*cfn_iam.Group
+	)
+	// the deletion order of these resources is important. Policies need to be last,
+	// so they don't have any attached resources which prevents their deletion.
+	// temp.Resources is a map. Traversing that directly results in undetermined order.
 	for _, val := range temp.Resources {
-		By(fmt.Sprintf("deleting the following resource: %s", val.AWSCloudFormationType()))
-		tayp := val.AWSCloudFormationType()
-		if tayp == configservice.ResourceTypeAwsIamRole {
+		switch val.AWSCloudFormationType() {
+		case configservice.ResourceTypeAwsIamRole:
 			role := val.(*cfn_iam.Role)
-			By(fmt.Sprintf("cleanup for role with name '%s'", role.RoleName))
-			// added repeat to not keep flooding the logs.
-			repeat := false
-			Eventually(func(gomega Gomega) bool {
-				_, err := iamSvc.DeleteRole(&iam.DeleteRoleInput{RoleName: aws.String(role.RoleName)})
-				if err != nil && !repeat {
-					By(fmt.Sprintf("failed to delete role '%s'; reason: %+v", role.RoleName, err))
-					repeat = true
-				}
-				code, ok := awserrors.Code(err)
-				return err == nil || (ok && code == iam.ErrCodeNoSuchEntityException)
-			}, 5*time.Minute, 5*time.Second).Should(BeTrue())
-		}
-		if val.AWSCloudFormationType() == "AWS::IAM::InstanceProfile" {
+			iamRoles = append(iamRoles, role)
+		case "AWS::IAM::InstanceProfile":
 			profile := val.(*cfn_iam.InstanceProfile)
-			By(fmt.Sprintf("cleanup for profile with name '%s'", profile.InstanceProfileName))
-			repeat := false
-			Eventually(func(gomega Gomega) bool {
-				_, err := iamSvc.DeleteInstanceProfile(&iam.DeleteInstanceProfileInput{InstanceProfileName: aws.String(profile.InstanceProfileName)})
-				if err != nil && !repeat {
-					By(fmt.Sprintf("failed to delete role '%s'; reason: %+v", profile.InstanceProfileName, err))
-					repeat = true
-				}
-				code, ok := awserrors.Code(err)
-				return err == nil || (ok && code == iam.ErrCodeNoSuchEntityException)
-			}, 5*time.Minute, 5*time.Second).Should(BeTrue())
-		}
-		if val.AWSCloudFormationType() == "AWS::IAM::ManagedPolicy" {
+			instanceProfiles = append(instanceProfiles, profile)
+		case "AWS::IAM::ManagedPolicy":
 			policy := val.(*cfn_iam.ManagedPolicy)
-			policies, err := iamSvc.ListPolicies(&iam.ListPoliciesInput{})
-			Expect(err).NotTo(HaveOccurred())
-			if len(policies.Policies) > 0 {
-				for _, p := range policies.Policies {
-					if aws.StringValue(p.PolicyName) == policy.ManagedPolicyName {
-						By(fmt.Sprintf("cleanup for policy '%s'", p.String()))
-						repeat := false
-						Eventually(func(gomega Gomega) bool {
-							_, err := iamSvc.DeletePolicy(&iam.DeletePolicyInput{PolicyArn: p.Arn})
-							if err != nil && !repeat {
-								By(fmt.Sprintf("failed to delete policy '%s'; reason: %+v", policy.Description, err))
-								repeat = true
-							}
-							code, ok := awserrors.Code(err)
-							return err == nil || (ok && code == iam.ErrCodeNoSuchEntityException)
-						}, 5*time.Minute, 5*time.Second).Should(BeTrue())
-						// TODO: why is there a break here? Don't we want to clean up everything?
-						break
-					}
+			policies = append(policies, policy)
+		case configservice.ResourceTypeAwsIamGroup:
+			group := val.(*cfn_iam.Group)
+			groups = append(groups, group)
+		}
+	}
+	for _, role := range iamRoles {
+		By(fmt.Sprintf("deleting the following role: %s", role.RoleName))
+		repeat := false
+		Eventually(func(gomega Gomega) bool {
+			err := DeleteRole(prov, role.RoleName)
+			if err != nil && !repeat {
+				By(fmt.Sprintf("failed to delete role '%s'; reason: %+v", role.RoleName, err))
+				repeat = true
+			}
+			code, ok := awserrors.Code(err)
+			return err == nil || (ok && code == iam.ErrCodeNoSuchEntityException)
+		}, 5*time.Minute, 5*time.Second).Should(BeTrue())
+	}
+	for _, profile := range instanceProfiles {
+		By(fmt.Sprintf("cleanup for profile with name '%s'", profile.InstanceProfileName))
+		repeat := false
+		Eventually(func(gomega Gomega) bool {
+			_, err := iamSvc.DeleteInstanceProfile(&iam.DeleteInstanceProfileInput{InstanceProfileName: aws.String(profile.InstanceProfileName)})
+			if err != nil && !repeat {
+				By(fmt.Sprintf("failed to delete role '%s'; reason: %+v", profile.InstanceProfileName, err))
+				repeat = true
+			}
+			code, ok := awserrors.Code(err)
+			return err == nil || (ok && code == iam.ErrCodeNoSuchEntityException)
+		}, 5*time.Minute, 5*time.Second).Should(BeTrue())
+	}
+	for _, group := range groups {
+		repeat := false
+		Eventually(func(gomega Gomega) bool {
+			_, err := iamSvc.DeleteGroup(&iam.DeleteGroupInput{GroupName: aws.String(group.GroupName)})
+			if err != nil && !repeat {
+				By(fmt.Sprintf("failed to delete group '%s'; reason: %+v", group.GroupName, err))
+				repeat = true
+			}
+			code, ok := awserrors.Code(err)
+			return err == nil || (ok && code == iam.ErrCodeNoSuchEntityException)
+		}, 5*time.Minute, 5*time.Second).Should(BeTrue())
+	}
+	for _, policy := range policies {
+		policies, err := iamSvc.ListPolicies(&iam.ListPoliciesInput{})
+		Expect(err).NotTo(HaveOccurred())
+		if len(policies.Policies) > 0 {
+			for _, p := range policies.Policies {
+				if aws.StringValue(p.PolicyName) == policy.ManagedPolicyName {
+					By(fmt.Sprintf("cleanup for policy '%s'", p.String()))
+					repeat := false
+					Eventually(func(gomega Gomega) bool {
+						response, err := iamSvc.DeletePolicy(&iam.DeletePolicyInput{
+							PolicyArn: p.Arn,
+						})
+						if err != nil && !repeat {
+							By(fmt.Sprintf("failed to delete policy '%s'; reason: %+v, response: %s", policy.Description, err, response.String()))
+							repeat = true
+						}
+						code, ok := awserrors.Code(err)
+						return err == nil || (ok && code == iam.ErrCodeNoSuchEntityException)
+					}, 5*time.Minute, 5*time.Second).Should(BeTrue())
+					// TODO: why is there a break here? Don't we want to clean up everything?
+					break
 				}
 			}
 		}
-		if val.AWSCloudFormationType() == configservice.ResourceTypeAwsIamGroup {
-			group := val.(*cfn_iam.Group)
-			_, _ = iamSvc.DeleteGroup(&iam.DeleteGroupInput{GroupName: aws.String(group.GroupName)})
-		}
 	}
 }
 
 // TODO: remove once test infra accounts are fixed.
 func deleteMultitenancyRoles(prov client.ConfigProvider) {
-	DeleteRole(prov, "multi-tenancy-role")
-	DeleteRole(prov, "multi-tenancy-nested-role")
+	if err := DeleteRole(prov, "multi-tenancy-role"); err != nil {
+		By(fmt.Sprintf("failed to delete role multi-tenancy-role %s", err))
+	}
+	if err := DeleteRole(prov, "multi-tenancy-nested-role"); err != nil {
+		By(fmt.Sprintf("failed to delete role multi-tenancy-nested-role %s", err))
+	}
 }
 
 // detachAllPoliciesForRole detaches all policies for role.
@@ -564,23 +595,25 @@ func detachAllPoliciesForRole(prov client.ConfigProvider, name string) error {
 }
 
 // DeleteRole deletes roles in a best effort manner.
-func DeleteRole(prov client.ConfigProvider, name string) {
+func DeleteRole(prov client.ConfigProvider, name string) error {
 	iamSvc := iam.New(prov)
 
 	// if role does not exist, return.
 	_, err := iamSvc.GetRole(&iam.GetRoleInput{RoleName: aws.String(name)})
 	if err != nil {
-		return
+		return err
 	}
 
 	if err := detachAllPoliciesForRole(prov, name); err != nil {
-		return
+		return err
 	}
 
 	_, err = iamSvc.DeleteRole(&iam.DeleteRoleInput{RoleName: aws.String(name)})
 	if err != nil {
-		return
+		return err
 	}
+
+	return nil
 }
 
 func GetPolicyArn(prov client.ConfigProvider, name string) string {