Skip to content

Commit

Permalink
Merge pull request #2055 from max-melentyev/role-policy
Browse files Browse the repository at this point in the history 
Use new policy checker for iam.roles
  • Loading branch information
@MisterMX
MisterMX authored May 2, 2024
2 parents fbdd828 + b38aeed commit a49ef55
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 a49ef55

Please sign in to comment.