Add is_primary_key_optional config option for resources with no single primary key

[knottnt]

Issue #, if available:

Description of changes:
In some cases an AWS resource may have multiple potential identifiers with no single one serving as the primary. Currently out generate code for PopulateResourceFromAnnotation requires at least one field be identified as a primary key and will throw an error during adoption if it is not provided. For such resources this will either add an unnecessary requirement or actually prevent adoption if they identifiers are mutually exclusive.

The motivating resource for this feature is ResourcePolicy for cloudwatch logs. This resource can identified by two mutually exclusive primary keys (policyName for account-wide and resourceARN for resource scoped). See aws-controllers-k8s/cloudwatchlogs-controller#76

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

[ack-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knottnt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [knottnt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gustavodiaz7722]

/retest

[gustavodiaz7722]

/retest

[gustavodiaz7722]

nit: is_primary_key_optional: true is a silent no-op unless a field is also marked is_primary_key: true

[knottnt]

Hmm that's a good call. This could also be applied to auto-discovered primary fields.

[knottnt]

Good catch — addressed. is_primary_key_optional now also applies to auto-discovered primary keys, not just fields explicitly marked is_primary_key: true. The optional if ok guard is emitted in the FindPrimaryIdentifierFieldNames path too, so the flag is no longer a silent no-op when is_primary_key is unset. (It still has no effect for ARN primary keys, which have no alternate identifier to fall back to — noted in a code comment.) Added a dedicated test + testdata config for the auto-discovered case.

[gustavodiaz7722]

Removing the required-field guard leaves nothing asserting that any identifier was supplied. PopulateResourceFromAnnotation returns nil having populated nothing, and on adoptionPolicy: adopt that does not fail — it adopts an arbitrary unrelated resource and reports success.

Observed

cloudwatchlogs-controller#76 built from source and deployed to an EKS cluster, with three ResourcePolicy CRs differing only in adoption-fields. The account held three account-scoped policies; DescribeResourcePolicies returned them in a stable order across repeated calls, with the AWS-managed AWSLogDeliveryWrite20150319 first:

$ aws logs describe-resource-policies --query 'resourcePolicies[].policyName' --output text
AWSLogDeliveryWrite20150319	ack736-app-log-policy	ack736-audit-log-policy

Every CR below intends to adopt ack736-app-log-policy:

NAME               ADOPTED_POLICY                SYNCED   FINALIZERS
control-correct    ack736-app-log-policy         True     [finalizers.cloudwatchlogs.services.k8s.aws/ResourcePolicy]
empty-annotation   AWSLogDeliveryWrite20150319   True     [finalizers.cloudwatchlogs.services.k8s.aws/ResourcePolicy]
misspelled-key     AWSLogDeliveryWrite20150319   True     [finalizers.cloudwatchlogs.services.k8s.aws/ResourcePolicy]

• control-correct — '{"policyName": "ack736-app-log-policy"}' — adopts the intended policy, so adoption works correctly whenever an identifier is present.
• empty-annotation — '{}' — ResourceSynced=True, no error, bound to the AWS-managed policy.
• misspelled-key — '{"policyname": "ack736-app-log-policy"}' (lowercase n, so the key is never read) — identical outcome.

Both failing CRs had the ACK finalizer attached and the AWS-managed policy's full 10-statement document copied into their spec — statements governing live log delivery for unrelated workloads in the account:

$ kubectl get resourcepolicy empty-annotation -o jsonpath='{.spec.policyDocument}' | jq '.Statement | length'
10
  - test-c01-SummaryWorkflowExpressLogGroup:log-stream:*
  - fluent-bitEKSAddonRelease-AddonReleaseStateMachineLogGroup0D1FCFFD-9sz...
  - /aws/vendedlogs/states/waiter-state-machine-bristol-devstack-test-pdx-...

Note also that two CRs were simultaneously bound to the same AWS policy, each believing it owned it.

Why it does not fail safely

The generated requiredFieldsMissingFromReadManyInput is a constant return false (DescribeResourcePolicies has no required members), so the early NotFound bailout in sdkFind cannot fire and the list call returns unfiltered. The match_fields comparison is then nil-guarded:

if elem.PolicyName != nil {
	if ko.Spec.PolicyName != nil {          // nil -> comparison skipped entirely
		if *elem.PolicyName != *ko.Spec.PolicyName {
			continue
		}
	}
	ko.Spec.PolicyName = elem.PolicyName
}

With Spec.PolicyName nil the loop takes the first element and breaks. Resources whose read op has required members fail safely here as AdoptedResourceNotFound — but is_primary_key_optional exists for resources with no mandatory identifier, which are exactly the ones that cannot.

This is specific to adopt. Under adopt-or-create, reconciler.go:641-643 keeps only the populated status and discards the spec, so the annotation cannot misdirect the lookup there. Worth noting too that since the primary key is auto-discovered, is_primary_key_optional: true alone is enough to reach this — no is_primary_key needed.

Deletion

The controller ran with the default DELETION_POLICY=delete. Deleting a correctly-adopted CR deletes the underlying AWS policy:

BEFORE: AWSLogDeliveryWrite20150319	ack736-app-log-policy	ack736-audit-log-policy
$ kubectl delete resourcepolicy control-correct
AFTER:  AWSLogDeliveryWrite20150319	ack736-audit-log-policy

Combined with the mis-adoption above, deleting a mis-adopted CR would delete the AWS-managed log delivery policy. That last step was deliberately not run — the two mis-adopted CRs were annotated services.k8s.aws/deletion-policy: retain before removal, so the policy survived (AWS resource will not be deleted - deletion policy set to retain). Each link was verified independently rather than chained on a live resource.

[knottnt]

Removing the required-field guard leaves nothing asserting that any identifier was supplied. PopulateResourceFromAnnotation returns nil having populated nothing, and on adoptionPolicy: adopt that does not fail — it adopts an arbitrary unrelated resource and reports success.

Observed

cloudwatchlogs-controller#76 built from source and deployed to an EKS cluster, with three ResourcePolicy CRs differing only in adoption-fields. The account held three account-scoped policies; DescribeResourcePolicies returned them in a stable order across repeated calls, with the AWS-managed AWSLogDeliveryWrite20150319 first:

$ aws logs describe-resource-policies --query 'resourcePolicies[].policyName' --output text
AWSLogDeliveryWrite20150319	ack736-app-log-policy	ack736-audit-log-policy

Every CR below intends to adopt ack736-app-log-policy:

NAME               ADOPTED_POLICY                SYNCED   FINALIZERS
control-correct    ack736-app-log-policy         True     [finalizers.cloudwatchlogs.services.k8s.aws/ResourcePolicy]
empty-annotation   AWSLogDeliveryWrite20150319   True     [finalizers.cloudwatchlogs.services.k8s.aws/ResourcePolicy]
misspelled-key     AWSLogDeliveryWrite20150319   True     [finalizers.cloudwatchlogs.services.k8s.aws/ResourcePolicy]

* `control-correct` — `'{"policyName": "ack736-app-log-policy"}'` — adopts the intended policy, so adoption works correctly whenever an identifier is present.

* `empty-annotation` — `'{}'` — `ResourceSynced=True`, no error, bound to the AWS-managed policy.

* `misspelled-key` — `'{"policyname": "ack736-app-log-policy"}'` (lowercase `n`, so the key is never read) — identical outcome.

Both failing CRs had the ACK finalizer attached and the AWS-managed policy's full 10-statement document copied into their spec — statements governing live log delivery for unrelated workloads in the account:

$ kubectl get resourcepolicy empty-annotation -o jsonpath='{.spec.policyDocument}' | jq '.Statement | length'
10
  - test-c01-SummaryWorkflowExpressLogGroup:log-stream:*
  - fluent-bitEKSAddonRelease-AddonReleaseStateMachineLogGroup0D1FCFFD-9sz...
  - /aws/vendedlogs/states/waiter-state-machine-bristol-devstack-test-pdx-...

Note also that two CRs were simultaneously bound to the same AWS policy, each believing it owned it.

Why it does not fail safely

The generated requiredFieldsMissingFromReadManyInput is a constant return false (DescribeResourcePolicies has no required members), so the early NotFound bailout in sdkFind cannot fire and the list call returns unfiltered. The match_fields comparison is then nil-guarded:

if elem.PolicyName != nil {
	if ko.Spec.PolicyName != nil {          // nil -> comparison skipped entirely
		if *elem.PolicyName != *ko.Spec.PolicyName {
			continue
		}
	}
	ko.Spec.PolicyName = elem.PolicyName
}

With Spec.PolicyName nil the loop takes the first element and breaks. Resources whose read op has required members fail safely here as AdoptedResourceNotFound — but is_primary_key_optional exists for resources with no mandatory identifier, which are exactly the ones that cannot.

This is specific to adopt. Under adopt-or-create, reconciler.go:641-643 keeps only the populated status and discards the spec, so the annotation cannot misdirect the lookup there. Worth noting too that since the primary key is auto-discovered, is_primary_key_optional: true alone is enough to reach this — no is_primary_key needed.

Deletion

The controller ran with the default DELETION_POLICY=delete. Deleting a correctly-adopted CR deletes the underlying AWS policy:

BEFORE: AWSLogDeliveryWrite20150319	ack736-app-log-policy	ack736-audit-log-policy
$ kubectl delete resourcepolicy control-correct
AFTER:  AWSLogDeliveryWrite20150319	ack736-audit-log-policy

Combined with the mis-adoption above, deleting a mis-adopted CR would delete the AWS-managed log delivery policy. That last step was deliberately not run — the two mis-adopted CRs were annotated services.k8s.aws/deletion-policy: retain before removal, so the policy survived (AWS resource will not be deleted - deletion policy set to retain). Each link was verified independently rather than chained on a live resource.

@gustavodiaz7722 This is a good catch. Will need to test this, but I think what might be happening is when neither PolicyName or ResourceARN are set the DescribeResourcePolicies API returns with the default ACCOUNT policy scope. To get around this I believe can add a check to sdkFind to ensure that at least on of PolicyName or ResourceARN are set and return a Terminal error if that is violated.