Merge pull request #85 from a-hilaly/role-infinite-loop

Fix Role reconciliation logic for `AssumeRolePolicyDocument`

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2023-10-04T06:33:03Z"
-  build_hash: 7445de38211639a12e79992d154adab6e60f01fd
-  go_version: go1.21.0
-  version: v0.27.1-1-g7445de3
+  build_date: "2023-11-21T06:03:03Z"
+  build_hash: 1cc9b5172d3d1676af578a3411e8672698ec29ce
+  go_version: go1.21.1
+  version: v0.27.1-5-g1cc9b51
 api_directory_checksum: 5f836e773a26000be60b198c9166f83ea86405e1
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.93
 generator_config_info:
-  file_checksum: fe29e906cfb41430ae4df1e2dc17ea4a3fb0365f
+  file_checksum: ee030a9c18c7fa34d8f7f4777997df355c029971
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -174,7 +174,7 @@ resources:
   Role:
     hooks:
       delta_pre_compare:
-        code: compareTags(delta, a, b)
+        code: customPreCompare(delta, a, b)
       sdk_read_one_post_set_output:
         template_path: hooks/role/sdk_read_one_post_set_output.go.tpl
       sdk_create_post_set_output:
@@ -232,6 +232,9 @@ resources:
       # policy document.
       InlinePolicies:
         type: map[string]*string
+      AssumeRolePolicyDocument:
+        compare:
+          is_ignored: true
       Tags:
         compare:
           is_ignored: true

generator.yaml

@@ -174,7 +174,7 @@ resources:
   Role:
     hooks:
       delta_pre_compare:
-        code: compareTags(delta, a, b)
+        code: customPreCompare(delta, a, b)
       sdk_read_one_post_set_output:
         template_path: hooks/role/sdk_read_one_post_set_output.go.tpl
       sdk_create_post_set_output:
@@ -232,6 +232,9 @@ resources:
       # policy document.
       InlinePolicies:
         type: map[string]*string
+      AssumeRolePolicyDocument:
+        compare:
+          is_ignored: true
       Tags:
         compare:
           is_ignored: true

go.mod

@@ -6,6 +6,8 @@ require (
 	github.com/aws-controllers-k8s/runtime v0.27.1
 	github.com/aws/aws-sdk-go v1.44.93
 	github.com/go-logr/logr v1.2.3
+	github.com/google/go-cmp v0.5.9
+	github.com/micahhausler/aws-iam-policy v0.4.2
 	github.com/samber/lo v1.37.0
 	github.com/spf13/pflag v1.0.5
 	k8s.io/api v0.26.8
@@ -14,6 +16,10 @@ require (
 	sigs.k8s.io/controller-runtime v0.14.5
 )
 
+// Temporary fix for github.com/micahhausler/aws-iam-policy. Awaiting for a-hilaly to send
+// a PR to micahhausler/aws-iam-policy to build Equal() method for PolicyDocument struct.
+replace github.com/micahhausler/aws-iam-policy => github.com/a-hilaly/aws-iam-policy v0.0.0-20231121054900-2c56e839ca53
+
 require (
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/cenkalti/backoff/v4 v4.1.3 // indirect
@@ -30,7 +36,6 @@ require (
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.2 // indirect
 	github.com/google/gnostic v0.5.7-v3refs // indirect
-	github.com/google/go-cmp v0.5.9 // indirect
 	github.com/google/gofuzz v1.1.0 // indirect
 	github.com/google/uuid v1.3.0 // indirect
 	github.com/imdario/mergo v0.3.12 // indirect

go.sum

@@ -33,6 +33,8 @@ cloud.google.com/go/storage v1.10.0/go.mod h1:FLPqc6j+Ki4BU591ie1oL6qBQGu2Bl/tZ9
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo=
+github.com/a-hilaly/aws-iam-policy v0.0.0-20231121054900-2c56e839ca53 h1:2uNM0nR2WUDN88EYFxjEaroH+PZJ6k/h9kl+KO0dWVc=
+github.com/a-hilaly/aws-iam-policy v0.0.0-20231121054900-2c56e839ca53/go.mod h1:Ojgst9ZFn+VEEJpqtuw/LxVGqEf2+hwWBlkYWvF/XWM=
 github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
 github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
 github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=

pkg/resource/role/hooks.go

@@ -15,12 +15,15 @@ package role
 
 import (
 	"context"
+	"encoding/json"
 	"net/url"
+	"reflect"
 
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
 	ackutil "github.com/aws-controllers-k8s/runtime/pkg/util"
 	svcsdk "github.com/aws/aws-sdk-go/service/iam"
+	awsiampolicy "github.com/micahhausler/aws-iam-policy/policy"
 	"github.com/samber/lo"
 
 	svcapitypes "github.com/aws-controllers-k8s/iam-controller/apis/v1alpha1"
@@ -366,6 +369,17 @@ func (rm *resourceManager) putAssumeRolePolicy(
 	return err
 }
 
+// customPreCompare contains logic that help compare two iam Roles. This
+// function is injected in newResourceDelta function.
+func customPreCompare(
+	delta *ackcompare.Delta,
+	a *resource,
+	b *resource,
+) {
+	compareTags(delta, a, b)
+	compareAssumeRolePolicyDocument(delta, a, b)
+}
+
 // compareTags is a custom comparison function for comparing lists of Tag
 // structs where the order of the structs in the list is not important.
 func compareTags(
@@ -382,6 +396,42 @@ func compareTags(
 	}
 }
 
+// compareAssumeRolePolicyDocument is a custom comparison function for
+// assumeRolePolicyDocuments. The reason why we need a custom function for
+// this fields is the API logic that trims all the trailing while spaces
+// string of provided documents.
+func compareAssumeRolePolicyDocument(
+	delta *ackcompare.Delta,
+	a *resource,
+	b *resource,
+) {
+	// To handle the variability in shapes of JSON objects representing IAM policies,
+	// especially when it comes to statements, actions, and other fields, we need
+	// a custom json.Unmarshaller approach crafted to our specific needs. Luckily,
+	// it happens that @micahhausler buildta library dedicated to this very special
+	// need: github.com/micahhausler/aws-iam-policy.
+	//
+	// NOTE(a-hilaly): I'm pretty aware that there is an error that should be handled.
+	// However, unfortunetly, the `newResourceDelta` cannot return errors (for now),
+	// leaving us with only two solutions, panicking or ignoring the error. The first
+	// solution is an overkill as it will interrupt all the goroutines from functioning
+	// and causing the controller to enter in a 'CrashLoopBackOff' state, which is not
+	// fair, given that it's also is responsible of managing multiple objects of other
+	// different resources.
+	//
+	// TOOD(a-hilaly): To address this issue, concider changing the delta signature
+	// to return an error or take a context.Context to use the runtime logger. Both
+	// of these changes require runtime/code-generator changes.
+	var policyDocumentA awsiampolicy.Policy
+	_ = json.Unmarshal([]byte(*a.ko.Spec.AssumeRolePolicyDocument), &policyDocumentA)
+	var policyDocumentB awsiampolicy.Policy
+	_ = json.Unmarshal([]byte(*b.ko.Spec.AssumeRolePolicyDocument), &policyDocumentB)
+
+	if !reflect.DeepEqual(policyDocumentA, policyDocumentB) {
+		delta.Add("Spec.AssumeRolePolicyDocument", a.ko.Spec.AssumeRolePolicyDocument, b.ko.Spec.AssumeRolePolicyDocument)
+	}
+}
+
 // syncTags examines the Tags in the supplied Role and calls the ListRoleTags,
 // TagRole and UntagRole APIs to ensure that the set of associated Tags  stays
 // in sync with the Role.Spec.Tags

test/e2e/tests/test_role.py

@@ -293,6 +293,9 @@ def test_crud(self, simple_role):
         k8s_assume_role_policy = json.loads(cr['spec']['assumeRolePolicyDocument'])
         assert assume_role_policy_as_obj == k8s_assume_role_policy
 
+        # make sure the resource is not in an "update infinite loop"
+        condition.assert_synced(ref)
+
         assume_role_policy_to_deny_doc = '''{
     "Version": "2012-10-17",
     "Statement": [
@@ -329,3 +332,6 @@ def test_crud(self, simple_role):
         assert latest_assume_role_policy_doc['Statement'][0]['Effect'] == k8s_assume_role_policy_deny['Statement'][0]['Effect']
 
         # Assume role policies cannot be entirely deleted, so CRU is tested here.
+
+        # make sure the resource is not in an "update infinite loop"
+        condition.assert_synced(ref)