Add is_primary_key_optional config option for resources with no single primary key - #736
Conversation
|
/retest |
1 similar comment
|
/retest |
| // mutually-exclusive fields (for example, a policy keyed by name OR by | ||
| // resource ARN) so adoption succeeds with whichever field(s) the user | ||
| // supplies. | ||
| IsPrimaryKeyOptional bool `json:"is_primary_key_optional"` |
There was a problem hiding this comment.
nit: is_primary_key_optional: true is a silent no-op unless a field is also marked is_primary_key: true
There was a problem hiding this comment.
Hmm that's a good call. This could also be applied to auto-discovered primary fields.
There was a problem hiding this comment.
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.
|
Removing the required-field guard leaves nothing asserting that any identifier was supplied. Observedcloudwatchlogs-controller#76 built from source and deployed to an EKS cluster, with three Every CR below intends to adopt
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: Note also that two CRs were simultaneously bound to the same AWS policy, each believing it owned it. Why it does not fail safelyThe generated 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 This is specific to DeletionThe controller ran with the default 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 |
@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 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gustavodiaz7722, knottnt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sapphirew
left a comment
sapphirew
left a comment
There was a problem hiding this comment.
Reviewed the adoption path. One substantive concern, on the case where the annotation supplies neither identifier. Details inline.
Verified along the way that the premise holds: PutResourcePolicy rejects policyName + resourceArn together, and a resource-scoped policy carries no policyName, so the current hard requirement really does block adoption of resource-scoped policies.
| if r.IsPrimaryKeyOptional() { | ||
| // The primary key is optional for adoption: set it when the | ||
| // annotation supplies it, but do not require it. | ||
| primaryKeyOut += optionalFieldGuardConstructor("primaryKey", sourceVarName, primaryField.Names.CamelLower, indentLevel) | ||
| primaryKeyOut += setResourceIdentifierPrimaryIdentifierAnn( | ||
| "&primaryKey", | ||
| primaryField, | ||
| targetVarPath, | ||
| indentLevel+1, | ||
| ) | ||
| primaryKeyOut += fmt.Sprintf("%s}\n", indent) |
There was a problem hiding this comment.
The feature is well motivated. I checked against the API and both halves hold up: a resource-scoped policy has no policyName at all (PutResourcePolicy with only resourceArn returns policyScope: RESOURCE with no name field), and passing both is rejected outright:
InvalidParameterException: Both policy name and resource arn cannot be specified at the same time.
So requiring policyName in the annotation really does block adoption of resource-scoped policies today. Worth noting that neither constraint appears in the API docs, which mark both fields Required: No.
My concern is the case where the annotation supplies neither identifier — an empty annotation, or a misspelled key like policyname. With adoption-policy: adopt that doesn't fail. It succeeds against an arbitrary resource:
PopulateResourceFromAnnotationsets nothing and returnsnil.handlePopulationonly stops on an error, so the runtime treats this as success (runtime v0.62.0 reconciler.go:511-518).resolved = populated(reconciler.go:634-647), so the spec is now empty.sdkFindcallsDescribeResourcePolicies, which has nopolicyNamefilter, so with an empty spec there is nothing to filter on and it lists everything.- The generated matcher only compares a field when the spec value is non-nil. Nothing gets compared, so
foundis set on the first policy returned.
setResourceManagedAndAdopted then adds the finalizer and adopted: true, and the CR owns that policy. Nothing looks wrong. With deletionPolicy: delete, deleting the CR deletes a policy the user never meant to manage.
Before this change the if !ok { return terminal } guard made step 1 impossible. That's the property I'd like to keep.
cloudwatchlogs-controller#76 does handle this via customCheckRequiredFieldsMissing, but that makes sdkFind return NotFound. It stops the mis-adoption, but the user gets an indefinite requeue instead of being told an identifier is missing, so the actionable terminal error from #613 is gone either way. And nothing in code-generator requires that pairing, so the next service to set this flag gets the mis-adoption with no protection at all.
Could we generate the check instead? Since the two fields are mutually exclusive, the real constraint is exactly one:
policyName, hasPolicyName := fields["policyName"]
resourceARN, hasResourceARN := fields["resourceARN"]
if hasPolicyName == hasResourceARN {
return ackerrors.NewTerminalError(fmt.Errorf(
"adoption requires exactly one of: policyName, resourceARN"))
}A len(fields) == 0 check would not be enough, since a misspelled key still gives a non-empty map. To emit the above, the generator needs to know which fields are the alternatives, and a resource-level bool can't tell it. Something like mutually_exclusive_identifiers: [PolicyName, ResourceARN] would, and it describes the resource more accurately than "the primary key is optional."
If you'd rather keep the check in the controller, that's reasonable, but then ValidateConfig should reject is_primary_key_optional when custom_check_required_fields_missing_method is absent, so it isn't left to each service to remember.
There was a problem hiding this comment.
Hmm, I think this is reasonable. I can look into implementing a mutually_exclusive_identifiers to provide a better generated adoption check. Might be able to generate the required_field_missing check from this as well.
| if isPrimaryIdentifier && r.IsPrimaryKeyOptional() { | ||
| // The auto-discovered primary key is optional for adoption: set | ||
| // it when the annotation supplies it, but do not require it. | ||
| // This mirrors the explicit is_primary_key handling above. | ||
| // (Note: is_primary_key_optional has no effect for ARN primary | ||
| // keys, which return early and always require the ARN.) | ||
| primaryKeyOut += optionalFieldGuardConstructor(requiredFieldVarName, sourceVarName, targetField.Names.CamelLower, indentLevel) | ||
| primaryKeyOut += setResourceIdentifierPrimaryIdentifierAnn( | ||
| fmt.Sprintf("&%s", requiredFieldVarName), | ||
| targetField, | ||
| sourceVarPath, | ||
| indentLevel+1, | ||
| ) | ||
| primaryKeyOut += fmt.Sprintf("%s}\n", indent) |
There was a problem hiding this comment.
Same request as above for this branch — whatever check we add should cover the auto-discovered case too.
One thing specific to here: the condition only looks at isPrimaryIdentifier and ignores the inputShape.IsRequired(memberName) half of the if on line 1546. So when a field is both the primary identifier and marked required by the read operation's input shape, the guard is dropped for a field the API cannot work without.
The new test is that case. name is required on DescribeClusterRequest in pkg/testdata/codegen/sdk-codegen/aws-models/eks.json, so TestSetResource_EKS_Cluster_OptionalAutoDiscoveredPrimaryKey_PopulateResourceFromAnnotation asserts the relaxed behavior for an identifier that isn't really optional.
I don't think this branch has to change if we add the "exactly one identifier" check, since that would catch the empty case first. But it's worth deciding whether the flag should apply to fields the read op marks required — if not, adding !inputShape.IsRequired(memberName) here would say so. Either way, a test against a resource whose identifiers are genuinely optional would show the intent better; DescribeResourcePolicies has no required members at all, which is exactly the shape this flag is for.
3 participants
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 (
policyNamefor account-wide andresourceARNfor resource scoped). See aws-controllers-k8s/cloudwatchlogs-controller#76By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.