From 2bfed0d2b10117bc36b741d3b9558318f59be241 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 31 Jan 2017 02:27:01 -0500 Subject: [PATCH 1/2] Remove additional IAM policies that have been removed This uses an explicit deletion approach, where we set the policy to empty, and use that to signal that the policy should be deleted. This is acceptable because IAM policies can't be empty anyway. We probably should use a tag-based "garbage-collection" approach, but IAM objects can't be tagged, so we're pretty much always going to be doing something name based. Fix #1642 --- pkg/model/iam.go | 61 +++++++++++-------- upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go | 35 ++++++++++- 2 files changed, 66 insertions(+), 30 deletions(-) diff --git a/pkg/model/iam.go b/pkg/model/iam.go index 1ea7797e92..7d05759070 100644 --- a/pkg/model/iam.go +++ b/pkg/model/iam.go @@ -112,35 +112,42 @@ func (b *IAMModelBuilder) Build(c *fi.ModelBuilderContext) error { } // Generate additional policies if needed, and attach to existing role - if b.Cluster.Spec.AdditionalPolicies != nil { - roleAsString := reflect.ValueOf(role).String() - additionalPolicies := *(b.Cluster.Spec.AdditionalPolicies) + { + additionalPolicy := "" + if b.Cluster.Spec.AdditionalPolicies != nil { + roleAsString := reflect.ValueOf(role).String() + additionalPolicies := *(b.Cluster.Spec.AdditionalPolicies) - if additionalPolicy, ok := additionalPolicies[strings.ToLower(roleAsString)]; ok { - additionalPolicyName := "additional." + name - - { - p := &iam.IAMPolicy{ - Version: iam.IAMPolicyDefaultVersion, - } - - statements := make([]*iam.IAMStatement, 0) - json.Unmarshal([]byte(additionalPolicy), &statements) - p.Statement = append(p.Statement, statements...) - - policy, err := p.AsJSON() - if err != nil { - return fmt.Errorf("error building IAM policy: %v", err) - } - - t := &awstasks.IAMRolePolicy{ - Name: s(additionalPolicyName), - Role: iamRole, - PolicyDocument: fi.WrapResource(fi.NewStringResource(policy)), - } - c.AddTask(t) - } + additionalPolicy = additionalPolicies[strings.ToLower(roleAsString)] } + + additionalPolicyName := "additional." + name + + t := &awstasks.IAMRolePolicy{ + Name: s(additionalPolicyName), + Role: iamRole, + } + + if additionalPolicy != "" { + p := &iam.IAMPolicy{ + Version: iam.IAMPolicyDefaultVersion, + } + + statements := make([]*iam.IAMStatement, 0) + json.Unmarshal([]byte(additionalPolicy), &statements) + p.Statement = append(p.Statement, statements...) + + policy, err := p.AsJSON() + if err != nil { + return fmt.Errorf("error building IAM policy: %v", err) + } + + t.PolicyDocument = fi.WrapResource(fi.NewStringResource(policy)) + } else { + t.PolicyDocument = fi.WrapResource(fi.NewStringResource("")) + } + + c.AddTask(t) } } diff --git a/upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go b/upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go index 49f935ae54..a8922b97bb 100644 --- a/upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go +++ b/upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go @@ -32,9 +32,12 @@ import ( //go:generate fitask -type=IAMRolePolicy type IAMRolePolicy struct { - ID *string - Name *string - Role *IAMRole + ID *string + Name *string + Role *IAMRole + + // The PolicyDocument to create as an inline policy. + // If the PolicyDocument is empty, the policy will be removed. PolicyDocument *fi.ResourceHolder } @@ -97,6 +100,21 @@ func (_ *IAMRolePolicy) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *IAMRoleP return fmt.Errorf("error rendering PolicyDocument: %v", err) } + if policy == "" { + // A deletion + + request := &iam.DeleteRolePolicyInput{} + request.RoleName = e.Role.Name + request.PolicyName = e.Name + + glog.V(2).Infof("Deleting role policy %s/%s", aws.StringValue(e.Role.Name), aws.StringValue(e.Name)) + _, err = t.Cloud.IAM().DeleteRolePolicy(request) + if err != nil { + return fmt.Errorf("error deleting IAMRolePolicy: %v", err) + } + return nil + } + doPut := false if a == nil { @@ -148,6 +166,17 @@ type terraformIAMRolePolicy struct { } func (_ *IAMRolePolicy) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *IAMRolePolicy) error { + { + policyString, err := e.PolicyDocument.AsString() + if err != nil { + return fmt.Errorf("error rendering PolicyDocument: %v", err) + } + if policyString == "" { + // A deletion; we simply don't render; terraform will observe the removal + return nil + } + } + policy, err := t.AddFile("aws_iam_role_policy", *e.Name, "policy", e.PolicyDocument) if err != nil { return fmt.Errorf("error rendering PolicyDocument: %v", err) From 1ce120136b0649c62b8ceba0da927b43916f9172 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Tue, 31 Jan 2017 13:44:52 -0500 Subject: [PATCH 2/2] Ignore NoSuchEntity deleting IAM policies --- upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go b/upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go index a8922b97bb..c6e44eca87 100644 --- a/upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go +++ b/upup/pkg/fi/cloudup/awstasks/iamrolepolicy.go @@ -110,6 +110,11 @@ func (_ *IAMRolePolicy) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *IAMRoleP glog.V(2).Infof("Deleting role policy %s/%s", aws.StringValue(e.Role.Name), aws.StringValue(e.Name)) _, err = t.Cloud.IAM().DeleteRolePolicy(request) if err != nil { + if awsup.AWSErrorCode(err) == "NoSuchEntity" { + // Already deleted + glog.V(2).Infof("Got NoSuchEntity deleting role policy %s/%s; assuming does not exist", aws.StringValue(e.Role.Name), aws.StringValue(e.Name)) + return nil + } return fmt.Errorf("error deleting IAMRolePolicy: %v", err) } return nil