Skip to content

Commit

Permalink
Use new policy checker for iam.roles
Browse files Browse the repository at this point in the history 
To not trigger update event and update role in AWS
when AWS changes formatting in policy document.

Signed-off-by: Max Melentyev <max.melentyev@reddit.com>
  • Loading branch information
@max-melentyev
max-melentyev committed May 2, 2024
1 parent a91527e commit b38aeed
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 87 deletions.
61 changes: 14 additions & 47 deletions pkg/clients/iam/role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
108 changes: 74 additions & 34 deletions pkg/clients/iam/role_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package iam

import (
"strings"
"net/url"
"regexp"
"testing"
"time"

Expand All @@ -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 (
Expand All @@ -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",
Expand All @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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("/"),
Expand All @@ -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),
Expand Down Expand Up @@ -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("/"),
Expand All @@ -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("//"),
Expand All @@ -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: &"//"`),
},
},
}

Expand All @@ -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())
}
}
}
})
Expand Down
8 changes: 2 additions & 6 deletions pkg/controller/iam/role/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package role

import (
"context"
"net/url"
"testing"

"github.com/aws/aws-sdk-go-v2/aws"
Expand All @@ -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 (
Expand Down Expand Up @@ -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)
}
}

Expand Down
14 changes: 14 additions & 0 deletions pkg/utils/policy/compare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

0 comments on commit b38aeed

Please sign in to comment.