Merge pull request #14151 from olemarkus/plug-iam-role-leak

Plug the IAM role leak

pkg/resources/aws/aws.go

@@ -23,6 +23,7 @@ import (
 	"sync"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/autoscaling"
 	"github.com/aws/aws-sdk-go/service/cloudformation"
 	"github.com/aws/aws-sdk-go/service/ec2"
@@ -1998,15 +1999,18 @@ func ListIAMRoles(cloud fi.Cloud, clusterName string) ([]*resources.Resource, er
 		err := c.IAM().ListRolesPages(request, func(p *iam.ListRolesOutput, lastPage bool) bool {
 			for _, r := range p.Roles {
 				name := aws.StringValue(r.RoleName)
-				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
+					if awserror, ok := err.(awserr.RequestFailure); ok && awserror.StatusCode() == 403 {
+						klog.Warningf("failed to determine ownership of %q: %v", *r.RoleName, awserror)
+
+						return true
+					} else {
+						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" {
@@ -2080,9 +2084,6 @@ func ListIAMInstanceProfiles(cloud fi.Cloud, clusterName string) ([]*resources.R
 	err := c.IAM().ListInstanceProfilesPages(request, func(p *iam.ListInstanceProfilesOutput, lastPage bool) bool {
 		for _, p := range p.InstanceProfiles {
 			name := aws.StringValue(p.InstanceProfileName)
-			if !strings.HasSuffix(name, "."+clusterName) {
-				continue
-			}
 
 			getRequest := &iam.GetInstanceProfileInput{InstanceProfileName: p.InstanceProfileName}
 			profileOutput, err := c.IAM().GetInstanceProfile(getRequest)

pkg/resources/aws/aws_test.go

@@ -24,7 +24,9 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/elb"
+	"github.com/aws/aws-sdk-go/service/iam"
 	"k8s.io/kops/cloudmock/aws/mockec2"
+	"k8s.io/kops/cloudmock/aws/mockiam"
 	"k8s.io/kops/pkg/resources"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
@@ -92,6 +94,72 @@ func TestAddUntaggedRouteTables(t *testing.T) {
 	}
 }
 
+func TestListIAMRoles(t *testing.T) {
+	cloud := awsup.BuildMockAWSCloud("us-east-1", "abc")
+	// resources := make(map[string]*Resource)
+	clusterName := "me.example.com"
+	ownershipTagKey := "kubernetes.io/cluster/" + clusterName
+
+	c := &mockiam.MockIAM{
+		Roles: make(map[string]*iam.Role),
+	}
+	cloud.MockIAM = c
+
+	tags := []*iam.Tag{
+		{
+			Key:   &ownershipTagKey,
+			Value: fi.String("owned"),
+		},
+	}
+
+	{
+		name := "prefixed." + clusterName
+
+		c.Roles[name] = &iam.Role{
+			RoleName: &name,
+			Tags:     tags,
+		}
+	}
+	{
+
+		name := clusterName + ".not-prefixed"
+
+		c.Roles[name] = &iam.Role{
+			RoleName: &name,
+			Tags:     tags,
+		}
+	}
+	{
+		name := "prefixed2." + clusterName
+		owner := "kubernetes.io/cluster/foo." + clusterName
+		c.Roles[name] = &iam.Role{
+			RoleName: &name,
+			Tags: []*iam.Tag{
+				{
+					Key:   &owner,
+					Value: fi.String("owned"),
+				},
+			},
+		}
+	}
+
+	{
+		name := "prefixed3." + clusterName
+		c.Roles[name] = &iam.Role{
+			RoleName: &name,
+		}
+	}
+
+	resourceTrackers, err := ListIAMRoles(cloud, clusterName)
+	if err != nil {
+		t.Fatalf("error listing IAM roles: %v", err)
+	}
+
+	if len(resourceTrackers) != 2 {
+		t.Errorf("Unexpected number of resources to delete. Expected 2, got %d", len(resourceTrackers))
+	}
+}
+
 func TestListRouteTables(t *testing.T) {
 	cloud := awsup.BuildMockAWSCloud("us-east-1", "abc")
 	// resources := make(map[string]*Resource)