Use new policy checker for iam.roles

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,53 +156,22 @@ 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)
  if err := GenerateRole(in, desired); err != nil {
  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
 }
 

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,15 +56,20 @@ var (
  "StringEquals": {"foo": "bar"}
  }
  }
- ]
- }`
+ ],
+ "Version": "2012-10-17"
+ }`
  roleID = "some Id"
  roleName = "some name"
  tagKey = "key"
  tagValue = "value"
  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())
+ }
  }
  }
  })

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)
  }
 }
 

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
+}