Ensuring proper resource reconciliation on Repository Creation (#81)

Resolves: aws-controllers-k8s/community#1873

**TLDR:**
- Addressed a bug in the ECR controller where Repository Custom Resources (CRs) failed
  to create `Policy` and `LifecyclePolicy` entries when containing all (possible) fields.
- The ECR controller was missing the post `sdkCreate` hooks; But the resources that 
  were missing few default-able fields we properly reconciled primarily due to late
  initialization, detecting API default values for non-specified fields, causing the controller
  to re queue resources after they have been created.
- Resolved by adding the necessary hooks to the create path.

**Description:**
This fix addresses a bug in the ECR controller. When a user submitted a Repository
Custom Resource (CR) containing all fields, the controller failed to create `Policy`
and `LifecyclePolicy` entries. However, if one of the fields was missing (excluding
`Policy` or `LifecyclePolicy`), the reconciliation process succeeded.

The bug fix was straightforward and intuitive, as it became evident that the controller
lacked the required hook for the create path. However, this raised questions about why
`policy` and `LifecyclePolicy` were missing in some specific cases. Our e2e tests
created CRs with isolated fields, such as `Policy` and `LifecyclePolicy`, and verified
that these policies were indeed applied to the ECR Repository. 

It was evident that the reason these specific cases were successfully reconciled was
due to the correct logic in the update path. This logic ensured that if a `Policy` or
`LifecyclePolicy` was missing, an API call was made to update these fields.

The mystery revolved around why these edge cases were reconciled even when we didn't
explicitly request a second reconciliation or requeue of the resource

**Debugging:**

- Our debugging journey began with a review of the logs and a comparison between the logs
  of a resource created with all fields and one with only the policy. Surprisingly, the
  logs showed a complete aditional reconciliation step, and it wasn't initiated by the controller
  itself (no requeue, no drift remediation, no condition). This was puzzling.

- To gain deeper insights, we connected the controller to a local clone of the runtime and
  added print statements. However, this approach made the logs messier and harder to
  interpret. Consequently, we tried to debugging with a go-delve debugger.

- Our debugging session took us through the ACK runtime, but the issue didn't originate
  there. Instead, it originiated from 'k8s-sigs/controller-runtime.' We cloned it and extended
  our debugging to the `controller-runtime`. There, we discovered that 'c.Do.Reconcile(ctx, req)'
  was causing two reconciliations.

- Further investigation revealed that 'for c.processNextWorkItem(ctx) {}' was called,
  indicating the presence of an extra item in the queue. This item had to be an update
  because the first item in the queue was a "create".

- The delve debugger showed that we submitted two resources one with only few fields
  and the other with all the fields... We then ran a `diff` on `k get repository <repo1/repo2> -o yaml`
  which revealed a crucial clue: 'metadata.generation' was 1 for the policy+name-only
  resource and 2 for the all the fields resource, confirming the update theory.

- The controller initiated the resource update because the ECR API returned all fields,
  even those not explicitly provided. This behavior prompted the ACK runtime to execute
  a patch into the resource spec, incrementing the generation and triggering an update.

Yeah, we know what that late initialisation is a thing, but it completly baited me here...

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: "2023-09-14T23:11:50Z"
-  build_hash: 892f29d00a4c4ad21a2fa32919921de18190979d
-  go_version: go1.21.0
-  version: v0.27.1
+  build_date: "2023-09-29T07:01:53Z"
+  build_hash: 7445de38211639a12e79992d154adab6e60f01fd
+  go_version: go1.21.1
+  version: v0.27.1-1-g7445de3
 api_directory_checksum: e33a65f2b24673bb4e6b614f65b3e16533ceb95c
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.93
 generator_config_info:
-  file_checksum: 0e045292e0fa1ace6f091edb93cab73b1817490c
+  file_checksum: 2c93dfcca987fd568b301811e33e4233523ffee8
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -52,6 +52,8 @@ resources:
         code: customPreCompare(delta, a, b)
       sdk_read_many_post_set_output:
         template_path: hooks/repository/sdk_read_many_post_set_output.go.tpl
+      sdk_create_post_set_output:
+        template_path: hooks/repository/sdk_create_post_set_output.go.tpl
       sdk_delete_post_build_request:
         template_path: hooks/repository/sdk_delete_post_build_request.go.tpl
     update_operation:

generator.yaml

@@ -52,6 +52,8 @@ resources:
         code: customPreCompare(delta, a, b)
       sdk_read_many_post_set_output:
         template_path: hooks/repository/sdk_read_many_post_set_output.go.tpl
+      sdk_create_post_set_output:
+        template_path: hooks/repository/sdk_create_post_set_output.go.tpl
       sdk_delete_post_build_request:
         template_path: hooks/repository/sdk_delete_post_build_request.go.tpl
     update_operation:

pkg/resource/repository/sdk.go

@@ -0,0 +1,12 @@
+    // Set the repository policy
+	if ko.Spec.Policy != nil && *ko.Spec.Policy != ""  {
+		if _, err := rm.updateRepositoryPolicy(ctx, desired); err != nil{
+			return nil, err
+		}
+	}
+    // Set the lifecycle policy
+	if ko.Spec.LifecyclePolicy != nil && *ko.Spec.LifecyclePolicy != "" {
+		if _, err := rm.updateLifecyclePolicy(ctx, desired); err != nil{
+			return nil, err
+		}
+	}

templates/hooks/repository/sdk_create_post_set_output.go.tpl

@@ -0,0 +1,14 @@
+apiVersion: ecr.services.k8s.aws/v1alpha1
+kind: Repository
+metadata:
+  name: $REPOSITORY_NAME
+spec:
+  name: $REPOSITORY_NAME
+  registryID: '$REGISTRY_ID'
+  imageScanningConfiguration:
+    scanOnPush: false
+  imageTagMutability: MUTABLE
+  encryptionConfiguration:
+    encryptionType: AES256
+  policy: '$REPOSITORY_POLICY'
+  lifecyclePolicy: '$REPOSITORY_LIFECYCLE_POLICY'

test/e2e/resources/repository_all_fields.yaml

@@ -21,6 +21,7 @@
 
 from acktest import tags as tagutil
 from acktest.resources import random_suffix_name
+from acktest.aws.identity import get_region, get_account_id
 from acktest.k8s import resource as k8s
 from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_ecr_resource
 from e2e.replacement_values import REPLACEMENT_VALUES
@@ -365,3 +366,56 @@ def test_repository_policy(self, ecr_client):
         # Check ECR repository doesn't exists
         exists = self.repository_exists(ecr_client, resource_name)
         assert not exists
+
+    def test_repository_create_will_all_fields(self, ecr_client):
+        resource_name = random_suffix_name("ecr-repository", 24)
+
+        replacements = REPLACEMENT_VALUES.copy()
+        replacements["REPOSITORY_NAME"] = resource_name
+        replacements["REPOSITORY_POLICY"] = REPOSITORY_POLICY_GET_DOWNLOAD_URL_ALL
+        replacements["REPOSITORY_LIFECYCLE_POLICY"] = LIFECYCLE_POLICY_FILTERING_ON_IMAGE_AGE
+        replacements["REGISTRY_ID"] = get_account_id()
+
+        # Load Repository CR
+        resource_data = load_ecr_resource(
+            "repository_all_fields",
+            additional_replacements=replacements,
+        )
+        logging.debug(resource_data)
+
+        # Create k8s resource
+        ref = k8s.CustomResourceReference(
+            CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
+            resource_name, namespace="default",
+        )
+        k8s.create_custom_resource(ref, resource_data)
+        cr = k8s.wait_resource_consumed_by_controller(ref)
+
+        assert cr is not None
+        assert k8s.get_resource_exists(ref)
+
+        time.sleep(CREATE_WAIT_AFTER_SECONDS)
+
+        # Get latest repository CR
+        cr = k8s.wait_resource_consumed_by_controller(ref)
+
+        # Check ECR repository exists
+        repo = self.get_repository(ecr_client, resource_name)
+        assert repo is not None
+
+        # Check ECR repository policy exists
+        policy = self.get_repository_policy(ecr_client, resource_name, repo["registryId"])
+        assert minify_json_string(policy) == REPOSITORY_POLICY_GET_DOWNLOAD_URL_ALL
+        # Check ECR repository lifecycle policy exists
+        lifecycle_policy = self.get_lifecycle_policy(ecr_client, resource_name, repo["registryId"])
+        assert lifecycle_policy == LIFECYCLE_POLICY_FILTERING_ON_IMAGE_AGE
+
+        # Delete k8s resource
+        _, deleted = k8s.delete_custom_resource(ref)
+        assert deleted is True
+
+        time.sleep(DELETE_WAIT_AFTER_SECONDS)
+
+        # Check ECR repository doesn't exists
+        exists = self.repository_exists(ecr_client, resource_name)
+        assert not exists