From b38aeedbbd5b6a05a4d585a69ebf45bbfa4e8e8d Mon Sep 17 00:00:00 2001 From: Max Melentyev Date: Wed, 1 May 2024 19:51:16 -0400 Subject: [PATCH] Use new policy checker for iam.roles To not trigger update event and update role in AWS when AWS changes formatting in policy document. Signed-off-by: Max Melentyev --- pkg/clients/iam/role.go | 61 +++--------- pkg/clients/iam/role_test.go | 108 ++++++++++++++------- pkg/controller/iam/role/controller_test.go | 8 +- pkg/utils/policy/compare.go | 14 +++ 4 files changed, 104 insertions(+), 87 deletions(-) diff --git a/pkg/clients/iam/role.go b/pkg/clients/iam/role.go index 5c6f7e2296..879acf39ce 100644 --- a/pkg/clients/iam/role.go +++ b/pkg/clients/iam/role.go @@ -12,19 +12,13 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/pkg/errors" + "k8s.io/utils/ptr" "github.com/crossplane-contrib/provider-aws/apis/iam/v1beta1" "github.com/crossplane-contrib/provider-aws/pkg/clients/iam/convert" "github.com/crossplane-contrib/provider-aws/pkg/utils/jsonpatch" "github.com/crossplane-contrib/provider-aws/pkg/utils/pointer" - legacypolicy "github.com/crossplane-contrib/provider-aws/pkg/utils/policy/old" -) - -const ( - errCheckUpToDate = "unable to determine if external resource is up to date" - errPolicyJSONEscape = "malformed AssumeRolePolicyDocument JSON" - errPolicyJSONUnescape = "malformed AssumeRolePolicyDocument escaping" + "github.com/crossplane-contrib/provider-aws/pkg/utils/policy" ) // RoleClient is the external client used for Role Custom Resource @@ -89,14 +83,18 @@ func GenerateRoleObservation(role iamtypes.Role) v1beta1.RoleExternalStatus { // GenerateRole assigns the in RoleParamters to role. func GenerateRole(in v1beta1.RoleParameters, role *iamtypes.Role) error { - if in.AssumeRolePolicyDocument != "" { - s, err := legacypolicy.CompactAndEscapeJSON(in.AssumeRolePolicyDocument) - if err != nil { - return errors.Wrap(err, errPolicyJSONEscape) + // iamtypes.Role has url-encoded policy document, while RoleParameters has plain. + // Assign policy from `in` only if it is different from the one in `role`. + if escapedPolicyDoc := role.AssumeRolePolicyDocument; escapedPolicyDoc != nil { + policyDoc, err := url.QueryUnescape(*escapedPolicyDoc) + if err != nil || !policy.ArePolicyDocumentsEqual(policyDoc, in.AssumeRolePolicyDocument) { + role.AssumeRolePolicyDocument = nil } - - role.AssumeRolePolicyDocument = &s } + if role.AssumeRolePolicyDocument == nil && in.AssumeRolePolicyDocument != "" { + role.AssumeRolePolicyDocument = ptr.To(url.QueryEscape(in.AssumeRolePolicyDocument)) + } + role.Description = in.Description role.MaxSessionDuration = in.MaxSessionDuration role.Path = in.Path @@ -158,24 +156,6 @@ func CreatePatch(in *iamtypes.Role, target *v1beta1.RoleParameters) (*v1beta1.Ro return patch, nil } -func isAssumeRolePolicyUpToDate(a, b *string) (bool, error) { - if a == nil || b == nil { - return a == b, nil - } - - jsonA, err := url.QueryUnescape(*a) - if err != nil { - return false, errors.Wrap(err, errPolicyJSONUnescape) - } - - jsonB, err := url.QueryUnescape(*b) - if err != nil { - return false, errors.Wrap(err, errPolicyJSONUnescape) - } - - return legacypolicy.IsPolicyUpToDate(&jsonA, &jsonB), nil -} - // IsRoleUpToDate checks whether there is a change in any of the modifiable fields in role. func IsRoleUpToDate(in v1beta1.RoleParameters, observed iamtypes.Role) (bool, string, error) { desired := (&convert.ConverterImpl{}).DeepCopyAWSRole(&observed) @@ -183,28 +163,15 @@ func IsRoleUpToDate(in v1beta1.RoleParameters, observed iamtypes.Role) (bool, st return false, "", err } - policyUpToDate, err := isAssumeRolePolicyUpToDate(desired.AssumeRolePolicyDocument, observed.AssumeRolePolicyDocument) - if err != nil { - return false, "", err - } - diff := cmp.Diff(desired, &observed, cmpopts.IgnoreInterfaces(struct{ resource.AttributeReferencer }{}), - cmpopts.IgnoreFields(observed, "AssumeRolePolicyDocument", "CreateDate", "PermissionsBoundary.PermissionsBoundaryType", "RoleLastUsed"), + cmpopts.IgnoreFields(observed, "CreateDate", "PermissionsBoundary.PermissionsBoundaryType", "RoleLastUsed"), cmpopts.IgnoreTypes(document.NoSerde{}), cmpopts.SortSlices(lessTag)) - if diff == "" && policyUpToDate { + if diff == "" { return true, diff, nil } diff = "Found observed difference in IAM role\n" + diff - - // Add extra logging for AssumeRolePolicyDocument because cmp.Diff doesn't show the full difference - if !policyUpToDate { - diff += "\ndesired assume role policy: " - diff += *desired.AssumeRolePolicyDocument - diff += "\nobserved assume role policy: " - diff += *observed.AssumeRolePolicyDocument - } return false, diff, nil } diff --git a/pkg/clients/iam/role_test.go b/pkg/clients/iam/role_test.go index bca91c926a..8ca2eecde2 100644 --- a/pkg/clients/iam/role_test.go +++ b/pkg/clients/iam/role_test.go @@ -1,7 +1,8 @@ package iam import ( - "strings" + "net/url" + "regexp" "testing" "time" @@ -10,10 +11,10 @@ import ( "github.com/aws/smithy-go/document" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "k8s.io/utils/ptr" "github.com/crossplane-contrib/provider-aws/apis/iam/v1beta1" "github.com/crossplane-contrib/provider-aws/pkg/utils/pointer" - legacypolicy "github.com/crossplane-contrib/provider-aws/pkg/utils/policy/old" ) var ( @@ -31,8 +32,19 @@ var ( } ] }` + assumeRolePolicyDocumentWithArrays = `{ + "Statement": [ + { + "Effect": "Allow", + "Principal": { + "Service": ["eks.amazonaws.com"] + }, + "Action": ["sts:AssumeRole"] + } + ], + "Version": "2012-10-17" + }` assumeRolePolicyDocument2 = `{ - "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", @@ -44,8 +56,9 @@ var ( "StringEquals": {"foo": "bar"} } } - ] - }` + ], + "Version": "2012-10-17" + }` roleID = "some Id" roleName = "some name" tagKey = "key" @@ -53,6 +66,10 @@ var ( permissionBoundary = "arn:aws:iam::111111111111:policy/permission-boundary" createDate = time.Now() region = "us-east-1" + // There are flaky failures when \s+ is used in line matchers to match diff lines. + // Instead, this regex collapses all whitespaces into a single space, + // and line matchers use single space. + compactSpaceRegex = regexp.MustCompile(`\s+`) ) func roleParams(m ...func(*v1beta1.RoleParameters)) *v1beta1.RoleParameters { @@ -69,14 +86,6 @@ func roleParams(m ...func(*v1beta1.RoleParameters)) *v1beta1.RoleParameters { return o } -func escapedPolicyJSON() *string { - p, err := legacypolicy.CompactAndEscapeJSON(assumeRolePolicyDocument) - if err == nil { - return &p - } - return nil -} - func role(m ...func(*iamtypes.Role)) *iamtypes.Role { o := &iamtypes.Role{ Description: &description, @@ -256,12 +265,12 @@ func TestIsRoleUpToDate(t *testing.T) { cases := map[string]struct { args args want bool - wantDiff string + wantDiff []*regexp.Regexp }{ "SameFields": { args: args{ role: iamtypes.Role{ - AssumeRolePolicyDocument: escapedPolicyJSON(), + AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocument)), Description: &description, MaxSessionDuration: pointer.ToIntAsInt32(1), Path: pointer.ToOrNilIfZeroValue("/"), @@ -282,12 +291,38 @@ func TestIsRoleUpToDate(t *testing.T) { }, }, want: true, - wantDiff: "", + wantDiff: nil, + }, + "SameFieldsWithDifferentPolicyFormat": { + args: args{ + role: iamtypes.Role{ + AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocumentWithArrays)), + Description: &description, + MaxSessionDuration: pointer.ToIntAsInt32(1), + Path: pointer.ToOrNilIfZeroValue("/"), + Tags: []iamtypes.Tag{{ + Key: pointer.ToOrNilIfZeroValue("key1"), + Value: pointer.ToOrNilIfZeroValue("value1"), + }}, + }, + p: v1beta1.RoleParameters{ + Description: &description, + AssumeRolePolicyDocument: assumeRolePolicyDocument, + MaxSessionDuration: pointer.ToIntAsInt32(1), + Path: pointer.ToOrNilIfZeroValue("/"), + Tags: []v1beta1.Tag{{ + Key: "key1", + Value: "value1", + }}, + }, + }, + want: true, + wantDiff: nil, }, "AWSInitializedFields": { args: args{ role: iamtypes.Role{ - AssumeRolePolicyDocument: escapedPolicyJSON(), + AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocument)), CreateDate: &createDate, Description: &description, MaxSessionDuration: pointer.ToIntAsInt32(1), @@ -318,12 +353,12 @@ func TestIsRoleUpToDate(t *testing.T) { }, }, want: true, - wantDiff: "", + wantDiff: nil, }, "DifferentPolicy": { args: args{ role: iamtypes.Role{ - AssumeRolePolicyDocument: escapedPolicyJSON(), + AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocument)), Description: &description, MaxSessionDuration: pointer.ToIntAsInt32(1), Path: pointer.ToOrNilIfZeroValue("/"), @@ -336,15 +371,16 @@ func TestIsRoleUpToDate(t *testing.T) { }, }, want: false, - wantDiff: `Found observed difference in IAM role - -desired assume role policy: %7B%22Version%22%3A%222012-10-17%22%2C%22Statement%22%3A%5B%7B%22Effect%22%3A%22Allow%22%2C%22Principal%22%3A%7B%22Service%22%3A%22eks.amazonaws.com%22%7D%2C%22Action%22%3A%22sts%3AAssumeRole%22%2C%22Condition%22%3A%7B%22StringEquals%22%3A%7B%22foo%22%3A%22bar%22%7D%7D%7D%5D%7D -observed assume role policy: %7B%22Version%22%3A%222012-10-17%22%2C%22Statement%22%3A%5B%7B%22Effect%22%3A%22Allow%22%2C%22Principal%22%3A%7B%22Service%22%3A%22eks.amazonaws.com%22%7D%2C%22Action%22%3A%22sts%3AAssumeRole%22%7D%5D%7D`, + wantDiff: []*regexp.Regexp{ + regexp.MustCompile("Found observed difference in IAM role"), + regexp.MustCompile(`- AssumeRolePolicyDocument: &"(%\w\w)+Statement`), + regexp.MustCompile(`\+ AssumeRolePolicyDocument: &"(%\w\w)+Version`), + }, }, "DifferentFields": { args: args{ role: iamtypes.Role{ - AssumeRolePolicyDocument: &assumeRolePolicyDocument, + AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocument)), Description: &description, MaxSessionDuration: pointer.ToIntAsInt32(1), Path: pointer.ToOrNilIfZeroValue("//"), @@ -364,8 +400,12 @@ observed assume role policy: %7B%22Version%22%3A%222012-10-17%22%2C%22Statement% }}, }, }, - want: false, - wantDiff: "Found observed difference in IAM role", + want: false, + wantDiff: []*regexp.Regexp{ + regexp.MustCompile("Found observed difference in IAM role"), + regexp.MustCompile(`- Path: &"/"`), + regexp.MustCompile(`\+ Path: &"//"`), + }, }, } @@ -378,15 +418,15 @@ observed assume role policy: %7B%22Version%22%3A%222012-10-17%22%2C%22Statement% if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("r: -want, +got:\n%s", diff) } - if tc.wantDiff == "" { - if tc.wantDiff != testDiff { - t.Errorf("r: -want, +got:\n%s", testDiff) + if tc.wantDiff == nil { + if diff := cmp.Diff("", testDiff); diff != "" { + t.Errorf("r: -want, +got:\n%s", diff) } - } - - if tc.wantDiff == "Found observed difference in IAM role" { - if !strings.Contains(testDiff, tc.wantDiff) { - t.Errorf("r: -want, +got:\n%s", testDiff) + } else { + for _, wantDiff := range tc.wantDiff { + if !wantDiff.MatchString(compactSpaceRegex.ReplaceAllString(testDiff, " ")) { + t.Errorf("expected:\n%s\nto match:\n%s", testDiff, wantDiff.String()) + } } } }) diff --git a/pkg/controller/iam/role/controller_test.go b/pkg/controller/iam/role/controller_test.go index 2d394da74b..64d70eca7b 100644 --- a/pkg/controller/iam/role/controller_test.go +++ b/pkg/controller/iam/role/controller_test.go @@ -18,6 +18,7 @@ package role import ( "context" + "net/url" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -36,7 +37,6 @@ import ( "github.com/crossplane-contrib/provider-aws/pkg/clients/iam/fake" errorutils "github.com/crossplane-contrib/provider-aws/pkg/utils/errors" "github.com/crossplane-contrib/provider-aws/pkg/utils/pointer" - legacypolicy "github.com/crossplane-contrib/provider-aws/pkg/utils/policy/old" ) var ( @@ -82,11 +82,7 @@ func withArn(s string) roleModifier { func withPolicy() roleModifier { return func(r *v1beta1.Role) { - p, err := legacypolicy.CompactAndEscapeJSON(policy) - if err != nil { - return - } - r.Spec.ForProvider.AssumeRolePolicyDocument = p + r.Spec.ForProvider.AssumeRolePolicyDocument = url.QueryEscape(policy) } } diff --git a/pkg/utils/policy/compare.go b/pkg/utils/policy/compare.go index 36fe495712..762f6b428c 100644 --- a/pkg/utils/policy/compare.go +++ b/pkg/utils/policy/compare.go @@ -10,3 +10,17 @@ func ArePoliciesEqal(a, b *Policy) (equal bool, diff string) { diff = cmp.Diff(a, b) return diff == "", diff } + +// ArePolicyDocumentsEqual determines if the two policy documents can be considered equal. +func ArePolicyDocumentsEqual(a, b string) bool { + policyA, err := ParsePolicyString(a) + if err != nil { + return a == b + } + policyB, err := ParsePolicyString(b) + if err != nil { + return false + } + eq, _ := ArePoliciesEqal(&policyA, &policyB) + return eq +}