Fix Role reconciliation logic for AssumeRolePolicyDocument

Addressed a bug in the Role reconciliation mechanism related to
the configuration of a multi-line `AssumeRolePolicyDocument` for
the `Role` resources. The problem was observed as an infinite loop
of requeues and update atempts by the controller. The root cause
was traced back to the behavior of the IAM api, which silently
removed spaces and newline character from the
`AssumeRolePolicyDocument`, leading to faulty comparisons during
the reconciliation process

This commit introduce a solution by removing all instances of
space `(' ')` and newline `('\n')` characters from the targeted
`AssumeRolePolicyDocument`. As aa result, the reconciliation process
now accurately validates and updates the field, eliminating the
previous issue

Use @a-hilaly fork of github.com/micahhausler/aws-iam-policy while waiting for a proper patch/PR

Signed-off-by: Amine Hilaly <[email protected]>

Author: a-hilaly

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/delta.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

pkg/resource/role/hooks.go

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