Enhance EKS AccessEntries policy attachements and delta logic (#102)

Prior to this patch, we observed some issues with the controller
behaviour regarding access policy association and delta computations,
specifically:
- False positive deltas were detected at every reconciliation (or
  controller restart)
- When a delta was detected, the controller disociate the latest
  policies, and patch the desired ones. (Even when it's unnecessary).

The root cause was identifier as the usage of `reflect.DeepEqual` for
slices. According to the `reflect` library [documentation](https://pkg.go.dev/reflect#DeepEqual)
`DeepEqual`  returns false if the comapred elements are a nil slice and
a "non-nil empty slice" (e.g `nil` and `[]string{}`).
This combined with the possibility of a user providing a nil
`namespaces` slice, while the controller sets an empty one (based on the
API response), cause behaviour inconsistencies.

Users shouldn't care/worry about the "nil-ity" of their arrays/slices;
it is up to the controller to accurately compute the deltas.

This patch replaces the usage of `reflect.DeepEqual` with a meticulous
custom delta function.

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

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: a-hilaly

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2024-02-08T16:38:43Z"
+  build_date: "2024-02-09T21:18:38Z"
   build_hash: 5b4565ec2712d29988b8123aeeed6a4af57467bf
   go_version: go1.21.5
   version: v0.29.2-4-g5b4565e
 api_directory_checksum: d960a9f06b58cc445e5ab21fb26ee6d92c441374
 api_version: v1alpha1
 aws_sdk_go_version: v1.49.13
 generator_config_info:
-  file_checksum: 89a00ff29a62ca0e86a8c08b275a6e9e708004b0
+  file_checksum: ac8c0c6f258d7b552ccbaf81401e6c511de3373e
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -395,11 +395,15 @@ resources:
       AccessPolicies:
         custom_field:
           list_of: AssociateAccessPolicyInput
+        compare:
+          is_ignored: true
       AccessPolicies.AccessScope.Type:
         go_tag: json:"type,omitempty"
       Type:
         go_tag: json:"type,omitempty"
     hooks:
+      delta_pre_compare:
+        code: customPreCompare(delta, a, b)
       sdk_read_one_post_set_output:
         template_path: hooks/access_entry/sdk_read_one_post_set_output.go.tpl
       sdk_update_pre_build_request:

generator.yaml

@@ -395,11 +395,15 @@ resources:
       AccessPolicies:
         custom_field:
           list_of: AssociateAccessPolicyInput
+        compare:
+          is_ignored: true
       AccessPolicies.AccessScope.Type:
         go_tag: json:"type,omitempty"
       Type:
         go_tag: json:"type,omitempty"
     hooks:
+      delta_pre_compare:
+        code: customPreCompare(delta, a, b)
       sdk_read_one_post_set_output:
         template_path: hooks/access_entry/sdk_read_one_post_set_output.go.tpl
       sdk_update_pre_build_request:

pkg/resource/access_entry/delta.go

@@ -15,9 +15,10 @@ package access_entry
 
 import (
 	"context"
-	"reflect"
 
+	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
+	"github.com/aws/aws-sdk-go/aws"
 	svcsdk "github.com/aws/aws-sdk-go/service/eks"
 
 	"github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
@@ -87,34 +88,21 @@ func (rm *resourceManager) syncAccessPolicies(ctx context.Context, desired, late
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.syncAccessPolicies")
 	defer func() { exit(err) }()
-	toAdd := []*v1alpha1.AssociateAccessPolicyInput{}
-	toDelete := []*string{}
 
 	existingPolicies := latest.ko.Spec.AccessPolicies
+	desiredPolicies := desired.ko.Spec.AccessPolicies
 
-	// find the policies to add
-	for _, p := range desired.ko.Spec.AccessPolicies {
-		if !exactMatchInAccessPolicies(p, existingPolicies) {
-			toAdd = append(toAdd, p)
-		}
-	}
+	toAdd, toDelete := computeAccessPoliciesDelta(desiredPolicies, existingPolicies)
 
-	// find the policies to delete
-	for _, p := range existingPolicies {
-		if !inAccessPolicies(p, desired.ko.Spec.AccessPolicies) {
-			toDelete = append(toDelete, p.PolicyARN)
-		}
-	}
-
-	// manage policies...
+	// remove policies first (to avoid conflicts)
 	for _, p := range toDelete {
-		rlog.Debug("disassociating access policy from role", "policy_arn", *p)
+		rlog.Debug("disassociating access policy from access entry", "policy_arn", *p)
 		if err = rm.disassociateAccessPolicy(ctx, desired, p); err != nil {
 			return err
 		}
 	}
 	for _, p := range toAdd {
-		rlog.Debug("associate access policy to access entry", "policy_arn", *p.PolicyARN)
+		rlog.Debug("associating access policy to access entry", "policy_arn", *p.PolicyARN)
 		if err = rm.associateAccessPolicy(ctx, desired, p); err != nil {
 			return err
 		}
@@ -123,28 +111,6 @@ func (rm *resourceManager) syncAccessPolicies(ctx context.Context, desired, late
 	return nil
 }
 
-// inAccessPolicies returns true if the supplied AccessPolicy ARN exists
-// in the slice of AccessPolicy objects.
-func inAccessPolicies(policy *v1alpha1.AssociateAccessPolicyInput, policies []*v1alpha1.AssociateAccessPolicyInput) bool {
-	for _, p := range policies {
-		if p.PolicyARN == policy.PolicyARN {
-			return false
-		}
-	}
-	return false
-}
-
-// exactMatchInAccessPolicies returns true if the supplied AccessPolicy is in the
-// slice of AccessPolicy objects and the AccessScope is exactly the same.
-func exactMatchInAccessPolicies(policy *v1alpha1.AssociateAccessPolicyInput, policies []*v1alpha1.AssociateAccessPolicyInput) bool {
-	for _, p := range policies {
-		if p.PolicyARN == policy.PolicyARN {
-			return reflect.DeepEqual(p.AccessScope, policy.AccessScope)
-		}
-	}
-	return false
-}
-
 // associateAccessPolicy adds the supplied AccessPolicy to the supplied
 // AccessEntry resource.
 func (rm *resourceManager) associateAccessPolicy(
@@ -153,7 +119,7 @@ func (rm *resourceManager) associateAccessPolicy(
 	entry *v1alpha1.AssociateAccessPolicyInput,
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
-	exit := rlog.Trace("rm.addManagedPolicy")
+	exit := rlog.Trace("rm.associateAccessPolicy")
 	defer func() { exit(err) }()
 
 	input := &svcsdk.AssociateAccessPolicyInput{
@@ -190,3 +156,116 @@ func (rm *resourceManager) disassociateAccessPolicy(
 	rm.metrics.RecordAPICall("UPDATE", "DisassociateAccessPolicy", err)
 	return err
 }
+
+// inAccessPolicies returns true if the supplied AccessPolicy ARN exists
+// in the slice of AccessPolicy objects.
+func inAccessPolicies(policy *v1alpha1.AssociateAccessPolicyInput, policies []*v1alpha1.AssociateAccessPolicyInput) bool {
+	for _, p := range policies {
+		if *p.PolicyARN == *policy.PolicyARN {
+			return true
+		}
+	}
+	return false
+}
+
+// equalAccessScopes returns true if the supplied AccessScope objects are
+// exactly the same.
+func equalAccessScopes(a, b *v1alpha1.AccessScope) bool {
+	if a == nil && b == nil {
+		return true
+	}
+	if a == nil {
+		return equalZeroString(b.Type) && len(b.Namespaces) == 0
+	}
+	if b == nil {
+		return equalZeroString(b.Type) && len(a.Namespaces) == 0
+	}
+	return equalStrings(a.Type, b.Type) && ackcompare.SliceStringPEqual(a.Namespaces, b.Namespaces)
+}
+
+// exactMatchInAccessPolicies returns true if the supplied AccessPolicy is in the
+// slice of AccessPolicy objects and the AccessScope is exactly the same.
+func exactMatchInAccessPolicies(policy *v1alpha1.AssociateAccessPolicyInput, policies []*v1alpha1.AssociateAccessPolicyInput) bool {
+	for _, p := range policies {
+		if p.PolicyARN == nil {
+			continue
+		}
+		if *p.PolicyARN == *policy.PolicyARN {
+			return equalAccessScopes(p.AccessScope, policy.AccessScope)
+		}
+	}
+	return false
+}
+
+// computeAccessPoliciesDelta returns two slices of AccessPolicy objects: one
+// slice of AccessPolicy objects that are in the desired slice but not in the
+// latest slice, and one slice of AccessPolicy objects that are in the latest
+// slice but not in the desired slice.
+func computeAccessPoliciesDelta(desired, latest []*v1alpha1.AssociateAccessPolicyInput) (toAdd []*v1alpha1.AssociateAccessPolicyInput, toDelete []*string) {
+	// useful for the toDelete elements
+	visited := map[string]bool{}
+
+	// First we need to loop through the desired policies and see if they are
+	// in the latest policies. If they are, we need to check if the AccessScope
+	// is the same. If it is, we don't need to do anything. If it's not, we need
+	// to add the policy to the toAdd slice and remove it from the toDelete slice.
+	//
+	// The delete is necessary because the API dosen't allow us to update the
+	// AccessScope of an existing policy. We need to disassociate the policy and
+	// then reassociate it.
+	for _, p := range desired {
+		visited[*p.PolicyARN] = true
+		// If it's an exact match, we don't need to do anything.
+		if exactMatchInAccessPolicies(p, latest) {
+			continue
+		}
+		// If it's in the latest policies, but the AccessScope is different, we
+		// need to remove it from the toDelete slice (update).
+		if inAccessPolicies(p, latest) {
+			toAdd = append(toAdd, p)
+			toDelete = append(toDelete, p.PolicyARN)
+		} else {
+			// If it's not in the latest policies, we need to add it.
+			toAdd = append(toAdd, p)
+		}
+	}
+
+	// Now that we've handled the desired policies, we need to loop through the
+	// latest policies and see if they are in the desired policies. If they are
+	// not, we need to add them to the toDelete slice.
+	for _, p := range latest {
+		// If we've already visited this policy, we don't need to do anything.
+		if visited[*p.PolicyARN] {
+			continue
+		}
+		// If it's not in the desired policies, we need to remove it.
+		if !inAccessPolicies(p, desired) {
+			toDelete = append(toDelete, p.PolicyARN)
+		}
+	}
+	return toAdd, toDelete
+}
+
+func customPreCompare(delta *ackcompare.Delta, a, b *resource) {
+	if len(a.ko.Spec.AccessPolicies) != len(b.ko.Spec.AccessPolicies) {
+		delta.Add("Spec.AccessPolicies", a.ko.Spec.AccessPolicies, b.ko.Spec.AccessPolicies)
+	} else if toAdd, toRemove := computeAccessPoliciesDelta(a.ko.Spec.AccessPolicies, b.ko.Spec.AccessPolicies); len(toAdd) > 0 || len(toRemove) > 0 {
+		delta.Add("Spec.AccessPolicies", a.ko.Spec.AccessPolicies, b.ko.Spec.AccessPolicies)
+	}
+}
+
+// EqualStrings returns true if two strings are equal e.g., both are nil, one is
+// nil and the other is empty string, or both non-zero strings are equal.
+func equalStrings(a, b *string) bool {
+	if a == nil {
+		return b == nil || *b == ""
+	}
+	if b == nil {
+		return *a == ""
+	}
+	return *a == *b
+}
+
+func equalZeroString(a *string) bool {
+	return equalStrings(a, aws.String(""))
+}