fix: Ignore principals order when comparing policies

AWS returns items in a random order so provider-aws should ignore it to not consider policies changed. Signed-off-by: Max Melentyev <max.melentyev@reddit.com>
crossplane-contrib · May 13, 2024 · 8faf89e · 8faf89e
1 parent bddb183
commit 8faf89e
Show file tree

Hide file tree

Showing 9 changed files with 258 additions and 19 deletions.
diff --git a/pkg/clients/iam/role_test.go b/pkg/clients/iam/role_test.go
@@ -3,6 +3,7 @@ package iam
 import (
 	"net/url"
 	"regexp"
+	"strings"
 	"testing"
 	"time"
 
@@ -66,10 +67,6 @@ 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 {
@@ -423,8 +420,11 @@ func TestIsRoleUpToDate(t *testing.T) {
 					t.Errorf("r: -want, +got:\n%s", diff)
 				}
 			} else {
+				// cmp randomly uses either regular or non-breaking spaces.
+				// Replace them all with regular spaces.
+				compactDiff := strings.Join(strings.Fields(testDiff), " ")
 				for _, wantDiff := range tc.wantDiff {
-					if !wantDiff.MatchString(compactSpaceRegex.ReplaceAllString(testDiff, " ")) {
+					if !wantDiff.MatchString(compactDiff) {
 						t.Errorf("expected:\n%s\nto match:\n%s", testDiff, wantDiff.String())
 					}
 				}

diff --git a/pkg/utils/policy/compare_test.go b/pkg/utils/policy/compare_test.go
@@ -19,6 +19,15 @@ var (
 
 	//go:embed testdata/Issue1892_b_min.json
 	policyIssue1892bMin string
+
+	//go:embed testdata/PrincipalsOrder_a.json
+	policyPrincipalsOrderA string
+
+	//go:embed testdata/PrincipalsOrder_b.json
+	policyPrincipalsOrderB string
+
+	//go:embed testdata/PrincipalsOrder_c.json
+	policyPrincipalsOrderC string
 )
 
 func TestCompareRawPolicies(t *testing.T) {
@@ -51,6 +60,24 @@ func TestCompareRawPolicies(t *testing.T) {
 				equals: true,
 			},
 		},
+		"PrincipalsOrder Equal": {
+			args: args{
+				policyA: policyPrincipalsOrderA,
+				policyB: policyPrincipalsOrderB,
+			},
+			want: want{
+				equals: true,
+			},
+		},
+		"PrincipalsOrder NotEqual": {
+			args: args{
+				policyA: policyPrincipalsOrderA,
+				policyB: policyPrincipalsOrderC,
+			},
+			want: want{
+				equals: false,
+			},
+		},
 	}
 
 	for name, tc := range cases {

diff --git a/pkg/utils/policy/convert.go b/pkg/utils/policy/convert.go
@@ -82,16 +82,16 @@ func convertResourcePolicyPrincipal(p *common.ResourcePrincipal) *Principal {
 	res := Principal{
 		AllowAnon: p.AllowAnon,
 		Federated: p.Federated,
-		Service:   p.Service,
+		Service:   NewStringOrSet(p.Service...),
 	}
 	for _, pr := range p.AWSPrincipals {
 		switch {
 		case pr.AWSAccountID != nil:
-			res.AWSPrincipals = append(res.AWSPrincipals, *pr.AWSAccountID)
+			res.AWSPrincipals = res.AWSPrincipals.Add(*pr.AWSAccountID)
 		case pr.IAMRoleARN != nil:
-			res.AWSPrincipals = append(res.AWSPrincipals, *pr.IAMRoleARN)
+			res.AWSPrincipals = res.AWSPrincipals.Add(*pr.IAMRoleARN)
 		case pr.UserARN != nil:
-			res.AWSPrincipals = append(res.AWSPrincipals, *pr.UserARN)
+			res.AWSPrincipals = res.AWSPrincipals.Add(*pr.UserARN)
 		}
 	}
 	return &res

diff --git a/pkg/utils/policy/parse_test.go b/pkg/utils/policy/parse_test.go
@@ -39,13 +39,13 @@ func TestParsePolicy(t *testing.T) {
 						{
 							SID: ptr.To("AllowPutObjectS3ServerAccessLogsPolicy"),
 							Principal: &Principal{
-								Service: StringOrArray{
+								Service: NewStringOrSet(
 									"logging.s3.amazonaws.com",
-								},
+								),
 								Federated: ptr.To("cognito-identity.amazonaws.com"),
-								AWSPrincipals: StringOrArray{
+								AWSPrincipals: NewStringOrSet(
 									"123456789012",
-								},
+								),
 							},
 							Effect: "Allow",
 							Action: StringOrArray{
@@ -78,15 +78,15 @@ func TestParsePolicy(t *testing.T) {
 						{
 							SID: ptr.To("AllowPutObjectS3ServerAccessLogsPolicy"),
 							Principal: &Principal{
-								Service: StringOrArray{
+								Service: NewStringOrSet(
 									"logging.s3.amazonaws.com",
 									"s3.amazonaws.com",
-								},
+								),
 								Federated: ptr.To("cognito-identity.amazonaws.com"),
-								AWSPrincipals: StringOrArray{
+								AWSPrincipals: NewStringOrSet(
 									"123456789012",
 									"452356421222",
-								},
+								),
 							},
 							Effect: "Allow",
 							Action: StringOrArray{

diff --git a/pkg/utils/policy/testdata/PrincipalsOrder_a.json b/pkg/utils/policy/testdata/PrincipalsOrder_a.json
@@ -0,0 +1,27 @@
+{
+  "Version": "2012-10-17",
+  "Statement": [{
+    "Effect": "Allow",
+    "Principal": {
+      "Service": [
+        "eks.amazonaws.com",
+        "sqs.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:bbb",
+        "arn:aws:iam::123456789012:aaa"
+      ]
+    },
+    "NotPrincipal": {
+      "Service": [
+        "s3.amazonaws.com",
+        "ec2.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:ddd",
+        "arn:aws:iam::123456789012:ccc"
+      ]
+    },
+    "Action": ["sts:AssumeRole"]
+  }]
+}
diff --git a/pkg/utils/policy/testdata/PrincipalsOrder_b.json b/pkg/utils/policy/testdata/PrincipalsOrder_b.json
@@ -0,0 +1,27 @@
+{
+  "Version": "2012-10-17",
+  "Statement": [{
+    "Effect": "Allow",
+    "Principal": {
+      "Service": [
+        "sqs.amazonaws.com",
+        "eks.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:aaa",
+        "arn:aws:iam::123456789012:bbb"
+      ]
+    },
+    "NotPrincipal": {
+      "Service": [
+        "ec2.amazonaws.com",
+        "s3.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:ccc",
+        "arn:aws:iam::123456789012:ddd"
+      ]
+    },
+    "Action": ["sts:AssumeRole"]
+  }]
+}
diff --git a/pkg/utils/policy/testdata/PrincipalsOrder_c.json b/pkg/utils/policy/testdata/PrincipalsOrder_c.json
@@ -0,0 +1,28 @@
+{
+  "Version": "2012-10-17",
+  "Statement": [{
+    "Effect": "Allow",
+    "Principal": {
+      "Service": [
+        "sqs.amazonaws.com",
+        "eks.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:aaa",
+        "arn:aws:iam::123456789012:bbb"
+      ]
+    },
+    "NotPrincipal": {
+      "Service": [
+        "ec2.amazonaws.com",
+        "s3.amazonaws.com"
+      ],
+      "AWS": [
+        "arn:aws:iam::123456789012:ccc",
+        "arn:aws:iam::123456789012:eee",
+        "arn:aws:iam::123456789012:ddd"
+      ]
+    },
+    "Action": ["sts:AssumeRole"]
+  }]
+}
diff --git a/pkg/utils/policy/types.go b/pkg/utils/policy/types.go
@@ -2,6 +2,7 @@ package policy
 
 import (
 	"encoding/json"
+	"sort"
 	"strconv"
 )
 
@@ -98,7 +99,7 @@ type Principal struct {
 	// This list contains the all of the AWS IAM users which are affected
 	// by the policy statement.
 	// +optional
-	AWSPrincipals StringOrArray `json:"AWS,omitempty"`
+	AWSPrincipals StringOrSet `json:"AWS,omitempty"`
 
 	// This string contains the identifier for any federated web identity
 	// provider.
@@ -107,7 +108,7 @@ type Principal struct {
 
 	// Service define the services which can have access to this bucket
 	// +optional
-	Service StringOrArray `json:"Service,omitempty"`
+	Service StringOrSet `json:"Service,omitempty"`
 }
 
 type unmarshalPrinciple Principal
@@ -207,3 +208,53 @@ func (s *StringOrArray) UnmarshalJSON(data []byte) error {
 	*s = list
 	return nil
 }
+
+// StringOrSet is a string array that supports parsing from a single string
+// as a single entry array. Order of elements is not respected when comparing
+// two StringOrSet objects.
+type StringOrSet map[string]struct{}
+
+func NewStringOrSet(values ...string) StringOrSet {
+	set := make(StringOrSet, len(values))
+	for _, s := range values {
+		set[s] = struct{}{}
+	}
+	return set
+}
+
+// Add adds a value to the set.
+func (s StringOrSet) Add(value string) StringOrSet {
+	if s == nil {
+		s = make(StringOrSet, 1)
+	}
+	s[value] = struct{}{}
+	return s
+}
+
+// UnmarshalJSON unmarshals data into s.
+func (s *StringOrSet) UnmarshalJSON(data []byte) error {
+	var single string
+	if err := json.Unmarshal(data, &single); err == nil {
+		*s = NewStringOrSet(single)
+		return nil
+	}
+	list := []string{}
+	if err := json.Unmarshal(data, &list); err != nil {
+		return err
+	}
+	*s = NewStringOrSet(list...)
+	return nil
+}
+
+// MarshalJSON marshals StringOrSet as an array.
+func (s *StringOrSet) MarshalJSON() ([]byte, error) {
+	var array []string
+	if s != nil && *s != nil {
+		array = make([]string, 0, len(*s))
+		for k := range *s {
+			array = append(array, k)
+		}
+		sort.Strings(array)
+	}
+	return json.Marshal(array)
+}
diff --git a/pkg/utils/policy/types_test.go b/pkg/utils/policy/types_test.go
@@ -0,0 +1,79 @@
+package policy
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestStringOrSet_Equal(t *testing.T) {
+	cases := map[string]struct {
+		a, b  StringOrSet
+		equal bool
+	}{
+		"BothNil": {
+			a:     nil,
+			b:     nil,
+			equal: true,
+		},
+		"BothEmpty": {
+			a:     NewStringOrSet(),
+			b:     NewStringOrSet(),
+			equal: true,
+		},
+		"EqualDifferentOrder": {
+			a:     NewStringOrSet("a", "b"),
+			b:     NewStringOrSet("b", "a"),
+			equal: true,
+		},
+		"NotEqual": {
+			a:     NewStringOrSet("a", "b"),
+			b:     NewStringOrSet("a", "c"),
+			equal: false,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			if diff := cmp.Diff(tc.a, tc.b); (diff == "") != tc.equal {
+				t.Errorf("diff: %s", diff)
+			}
+		})
+	}
+}
+
+func TestStringOrSet_MarshalJSON(t *testing.T) {
+	cases := map[string]struct {
+		set  StringOrSet
+		want string
+	}{
+		"nil": {
+			set:  nil,
+			want: "null",
+		},
+		"Empty": {
+			set:  NewStringOrSet(),
+			want: "[]",
+		},
+		"Single": {
+			set:  NewStringOrSet("a"),
+			want: `["a"]`,
+		},
+		"Multiple": {
+			set:  NewStringOrSet("a", "c", "b"),
+			want: `["a","b","c"]`,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			b, err := tc.set.MarshalJSON()
+			if err != nil {
+				t.Fatal(err)
+			}
+			if string(b) != tc.want {
+				t.Errorf("got %s, want %s", b, tc.want)
+			}
+		})
+	}
+}