Merge pull request #11497 from johngmyers/cleanup-iam

Cleanup orphaned IAM service account roles in direct render

pkg/model/awsmodel/BUILD.bazel

@@ -35,6 +35,7 @@ go_library(
         "//vendor/github.com/aws/aws-sdk-go/aws:go_default_library",
         "//vendor/github.com/aws/aws-sdk-go/aws/endpoints:go_default_library",
         "//vendor/github.com/aws/aws-sdk-go/service/ec2:go_default_library",
+        "//vendor/github.com/aws/aws-sdk-go/service/iam:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",

pkg/model/awsmodel/iam.go

@@ -22,6 +22,7 @@ import (
 	"strings"
 
 	"github.com/aws/aws-sdk-go/aws/endpoints"
+	awsIam "github.com/aws/aws-sdk-go/service/iam"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2"
 	"k8s.io/kops/pkg/apis/kops"
@@ -31,6 +32,7 @@ import (
 	"k8s.io/kops/pkg/util/stringorslice"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awstasks"
+	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 )
 
 // IAMModelBuilder configures IAM objects
@@ -41,6 +43,7 @@ type IAMModelBuilder struct {
 }
 
 var _ fi.ModelBuilder = &IAMModelBuilder{}
+var _ fi.HasDeletions = &IAMModelBuilder{}
 
 const NodeRolePolicyTemplate = `{
   "Version": "2012-10-17",
@@ -441,3 +444,41 @@ func (b *IAMModelBuilder) buildAWSIAMRolePolicy(role iam.Subject) (fi.Resource,
 
 	return fi.NewStringResource(policy), nil
 }
+
+func (b *IAMModelBuilder) FindDeletions(context *fi.ModelBuilderContext, cloud fi.Cloud) error {
+	iamapi := cloud.(awsup.AWSCloud).IAM()
+	ownershipTag := "kubernetes.io/cluster/" + b.Cluster.ObjectMeta.Name
+	request := &awsIam.ListRolesInput{}
+	var getRoleErr error
+	err := iamapi.ListRolesPages(request, func(p *awsIam.ListRolesOutput, lastPage bool) bool {
+		for _, role := range p.Roles {
+			if !strings.HasSuffix(fi.StringValue(role.RoleName), "."+b.Cluster.ObjectMeta.Name) {
+				continue
+			}
+			getRequest := &awsIam.GetRoleInput{RoleName: role.RoleName}
+			roleOutput, err := iamapi.GetRole(getRequest)
+			if err != nil {
+				getRoleErr = fmt.Errorf("calling IAM GetRole on %s: %w", fi.StringValue(role.RoleName), err)
+				return false
+			}
+			for _, tag := range roleOutput.Role.Tags {
+				if fi.StringValue(tag.Key) == ownershipTag && fi.StringValue(tag.Value) == "owned" {
+					if _, ok := context.Tasks["IAMRole/"+fi.StringValue(role.RoleName)]; !ok {
+						context.AddTask(&awstasks.IAMRole{
+							ID:   role.RoleId,
+							Name: role.RoleName,
+						})
+					}
+				}
+			}
+		}
+		return true
+	})
+	if getRoleErr != nil {
+		return getRoleErr
+	}
+	if err != nil {
+		return fmt.Errorf("listing IAM roles: %w", err)
+	}
+	return nil
+}

pkg/resources/aws/aws.go

@@ -1938,42 +1938,47 @@ func DeleteIAMRole(cloud fi.Cloud, r *resources.Resource) error {
 func ListIAMRoles(cloud fi.Cloud, clusterName string) ([]*resources.Resource, error) {
 	c := cloud.(awsup.AWSCloud)
 
-	remove := make(map[string]bool)
-	remove["masters."+clusterName] = true
-	remove["nodes."+clusterName] = true
-	remove["bastions."+clusterName] = true
-
-	var roles []*iam.Role
-	// Find roles matching remove map
+	var resourceTrackers []*resources.Resource
+	// Find roles owned by the cluster
 	{
+		var getRoleErr error
+		ownershipTag := "kubernetes.io/cluster/" + clusterName
 		request := &iam.ListRolesInput{}
 		err := c.IAM().ListRolesPages(request, func(p *iam.ListRolesOutput, lastPage bool) bool {
 			for _, r := range p.Roles {
 				name := aws.StringValue(r.RoleName)
-				if remove[name] {
-					roles = append(roles, r)
+				if !strings.HasSuffix(name, "."+clusterName) {
+					continue
+				}
+
+				getRequest := &iam.GetRoleInput{RoleName: r.RoleName}
+				roleOutput, err := c.IAM().GetRole(getRequest)
+				if err != nil {
+					getRoleErr = fmt.Errorf("calling IAM GetRole on %s: %w", name, err)
+					return false
+				}
+				for _, tag := range roleOutput.Role.Tags {
+					if fi.StringValue(tag.Key) == ownershipTag && fi.StringValue(tag.Value) == "owned" {
+						resourceTracker := &resources.Resource{
+							Name:    name,
+							ID:      name,
+							Type:    "iam-role",
+							Deleter: DeleteIAMRole,
+						}
+						resourceTrackers = append(resourceTrackers, resourceTracker)
+					}
 				}
 			}
 			return true
 		})
+		if getRoleErr != nil {
+			return nil, getRoleErr
+		}
 		if err != nil {
 			return nil, fmt.Errorf("error listing IAM roles: %v", err)
 		}
 	}
 
-	var resourceTrackers []*resources.Resource
-
-	for _, role := range roles {
-		name := aws.StringValue(role.RoleName)
-		resourceTracker := &resources.Resource{
-			Name:    name,
-			ID:      name,
-			Type:    "iam-role",
-			Deleter: DeleteIAMRole,
-		}
-		resourceTrackers = append(resourceTrackers, resourceTracker)
-	}
-
 	return resourceTrackers, nil
 }
 

upup/pkg/fi/cloudup/apply_cluster.go

@@ -664,13 +664,11 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
 			return fmt.Errorf("unknown cloudprovider %q", cluster.Spec.CloudProvider)
 		}
 	}
-	taskMap, err := l.BuildTasks(assetBuilder, &stageAssetsLifecycle, c.LifecycleOverrides)
+	c.TaskMap, err = l.BuildTasks(assetBuilder, &stageAssetsLifecycle, c.LifecycleOverrides)
 	if err != nil {
 		return fmt.Errorf("error building tasks: %v", err)
 	}
 
-	c.TaskMap = taskMap
-
 	var target fi.Target
 	dryRun := false
 	shouldPrecreateDNS := true
@@ -739,6 +737,19 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
 	}
 	c.Target = target
 
+	if checkExisting {
+		c.TaskMap, err = l.FindDeletions(cloud, c.LifecycleOverrides)
+		if err != nil {
+			return fmt.Errorf("error finding deletions: %w", err)
+		}
+	}
+
+	context, err := fi.NewContext(target, cluster, cloud, keyStore, secretStore, configBase, checkExisting, c.TaskMap)
+	if err != nil {
+		return fmt.Errorf("error building context: %v", err)
+	}
+	defer context.Close()
+
 	if !dryRun {
 		acl, err := acls.GetACL(configBase, cluster)
 		if err != nil {
@@ -770,12 +781,6 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
 		}
 	}
 
-	context, err := fi.NewContext(target, cluster, cloud, keyStore, secretStore, configBase, checkExisting, taskMap)
-	if err != nil {
-		return fmt.Errorf("error building context: %v", err)
-	}
-	defer context.Close()
-
 	var options fi.RunTasksOptions
 	if c.RunTasksOptions != nil {
 		options = *c.RunTasksOptions
@@ -798,7 +803,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
 		}
 	}
 
-	err = target.Finish(taskMap) //This will finish the apply, and print the changes
+	err = target.Finish(c.TaskMap) //This will finish the apply, and print the changes
 	if err != nil {
 		return fmt.Errorf("error closing target: %v", err)
 	}

upup/pkg/fi/cloudup/loader.go

@@ -181,3 +181,19 @@ func (l *Loader) processDeferrals() error {
 
 	return nil
 }
+
+func (l *Loader) FindDeletions(cloud fi.Cloud, lifecycleOverrides map[string]fi.Lifecycle) (map[string]fi.Task, error) {
+	for _, builder := range l.Builders {
+		if hasDeletions, ok := builder.(fi.HasDeletions); ok {
+			context := &fi.ModelBuilderContext{
+				Tasks:              l.tasks,
+				LifecycleOverrides: lifecycleOverrides,
+			}
+			if err := hasDeletions.FindDeletions(context, cloud); err != nil {
+				return nil, err
+			}
+			l.tasks = context.Tasks
+		}
+	}
+	return l.tasks, nil
+}

upup/pkg/fi/task.go

@@ -50,6 +50,14 @@ type ModelBuilder interface {
 	Build(context *ModelBuilderContext) error
 }
 
+// HasDeletions is a ModelBuilder that creates tasks to delete cloud objects that no longer exist in the model.
+type HasDeletions interface {
+	ModelBuilder
+	// FindDeletions finds cloud objects that are owned by the cluster but no longer in the model and creates tasks to delete them.
+	// It is not called for the Terraform or Cloudformation targets.
+	FindDeletions(context *ModelBuilderContext, cloud Cloud) error
+}
+
 // ModelBuilderContext is a context object that holds state we want to pass to ModelBuilder
 type ModelBuilderContext struct {
 	Tasks              map[string]Task