Add mutually_exclusive_identifiers config 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.
…better enforcement of Adoption field requirement - Replace is_primary_key_optional with mutually_exclusive_identifiers - Replace optional primary key logic with mutually exclusive identifier check when generating PopulateResourceFromAnnotation - Add cloudwatchlogs api model to testdata
knottnt
changed the title
|
/test ec2-controller-test |
|
@knottnt: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
sapphirew
left a comment
sapphirew
left a comment
There was a problem hiding this comment.
Re-reviewed after the rename and the generated check. This addresses the earlier concern well — the exactly-one terminal error plus the ReadMany check gives two independent layers, and the config now fails validation for the obvious bad combinations. Old name and fixtures are fully cleaned up, build and unit tests are green.
Nothing here blocks. Three inline notes; the one on check.go is a two-line fix I would suggest taking before merge, since it is the only remaining path that fails open. The other two are follow-up material.
| if r.HasMutuallyExclusiveIdentifiers() { | ||
| identifierFields, meErr := r.GetMutuallyExclusiveIdentifierFields() | ||
| if meErr != nil { | ||
| return "", meErr | ||
| } | ||
| identifierKeys := make([]string, 0, len(identifierFields)) | ||
| for _, identifierField := range identifierFields { | ||
| identifierKeys = append(identifierKeys, identifierField.Names.CamelLower) | ||
| } | ||
| out += mutuallyExclusiveIdentifierGuardConstructor(identifierKeys, sourceVarName, indentLevel) | ||
| } |
There was a problem hiding this comment.
Nice — generating the check is the right call.
One issue: the guard's key list comes from the config, but the code that fills those keys in runs in the member loop over the read op's input shape, and the loop skips fields in several places the guard doesn't know about. So the guard can pass while nothing gets written.
Most likely case: IsPrimaryARNField is true for a field named <ResourceName>Arn (pkg/model/crd.go:403-415), and the loop skips those at line 1565 — exactly the "adopt by name or ARN" case this is for. I generated a config like that: the guard counted resourceARN, nothing populated it. Supplying only the ARN passes the guard, writes nothing, and then the ReadMany check reports incomplete, so adoption returns NotFound forever with nothing pointing at the config.
Same skip for: identifier not in the read op's input (or ignored out), a renames declared on only one op, is_secret, and non-string types — the "must be a scalar type" error comes after the type check at line 1554.
It fails safe, so the feature just stops working rather than adopting the wrong thing. Not a blocker for this PR, since ResourcePolicy isn't affected (ResourceArn doesn't match IsPrimaryARNField and is a member of DescribeResourcePoliciesInput). Worth fixing in one place though: build the key list from the fields the loop actually writes, or error at codegen if a configured identifier was never written.
Two smaller ones:
- Line 1534 returns
primaryKeyConditionalOut + arnOut, droppingout— the guard. Validation catchesis_arn_primary_keybut not this path, andmutually_exclusive_identifiersdoesn't requireis_primary_key. - The guard checks key presence, not value, so
{"policyName": ""}passes and sets a non-nil empty pointer, which also passes the ReadMany check.identifierNameOrIDGuardConstructorchecks== "".
| if r.HasMutuallyExclusiveIdentifiers() { | ||
| exclusiveConditions, _, err := mutuallyExclusiveIdentifierNilConditions(r, koVarName) | ||
| if err != nil { | ||
| return result | ||
| } | ||
| return fmt.Sprintf("%sreturn %s\n", indent, strings.Join(exclusiveConditions, " && ")) |
There was a problem hiding this comment.
This drops the error and returns return false — the behavior the comment above calls out as letting "sdkFind list every resource and match an arbitrary one." The ReadOne version returns the error.
A typo'd config name makes mutuallyExclusiveIdentifierNilConditions error (pkg/model/crd.go:485); I confirmed this then emits return false with no error at the call site. It doesn't reach a real build today only because the same typo also fails PopulateResourceFromAnnotation and aborts generation. That's accidental, and this is the function meant to close the hole.
Of everything I found this round, this is the only path that fails open, and it's the cheapest to fix — so it's the one I'd suggest doing before merge. Widening the signature to return an error would be best; return true would at least make the fallback safe.
Also, ReadOne parenthesizes the grouped condition and this doesn't — fine today, but this branch ignores shape, so a future || term here would break quietly.
| func validateMutuallyExclusiveIdentifiers(cfg *Config) []error { | ||
| var errs []error | ||
| for resName, resCfg := range cfg.Resources { | ||
| identifiers := resCfg.MutuallyExclusiveIdentifiers | ||
| if len(identifiers) == 0 { | ||
| continue | ||
| } | ||
| if len(identifiers) < 2 { | ||
| errs = append(errs, fmt.Errorf( | ||
| "resources.%s.mutually_exclusive_identifiers: must list at least two fields, got %d", | ||
| resName, len(identifiers), | ||
| )) | ||
| } | ||
| if resCfg.IsARNPrimaryKey { | ||
| errs = append(errs, fmt.Errorf( | ||
| "resources.%s.mutually_exclusive_identifiers: cannot be combined with is_arn_primary_key", | ||
| resName, | ||
| )) | ||
| } | ||
| } | ||
| return errs |
There was a problem hiding this comment.
A few config rules worth adding here, none urgent:
[PolicyName, policyName]passeslen < 2, since both collapse to one field later. The guard then counts the same key twice, so adoption always fails. Check for two distinct fields after normalizing.- Field names aren't verified. A typo fails generation only via
PopulateResourceFromAnnotation, without theresources.<R>.mutually_exclusive_identifiers:context.ValidateConfighas no model, butpkg/model/model.gohas a post-processFieldsblock doing this forresources.<R>.fields.<F>— likely the right home. custom_check_required_fields_missing_methodmakes the read templates skip the generated check entirely. cloudwatchlogs-controller#76 sets it today, so migrating without removing it silently disables the new check. Worth rejecting together.- Declaring identifiers the read op marks
required(deliberate, per the comment inset_resource.go) turns the ReadOne check from||into&&. With onlyTypeset it passes,GetSecurityPolicygetsName=nil, and fails with a ValidationException instead ofNotFound→ create. The only test for that branch is the opensearchserverless fixture, which says itself thatName/Typearen't really exclusive. Either allow it with a real fixture, or reject it here. - This option alone doesn't make adoption safe — each identifier also needs a read-request filter or a
list_operation.match_fieldsentry, or the List is unfiltered andsdkFindtakes the first result. Patch the correct observed state after arm.ReadOnecall #76 setsmatch_fields: [PolicyName]; the fixture here doesn't, and nothing warns.
3 participants
Issue #, if available:
Description of changes:
Some AWS resources have no single mandatory identifier and are instead identified by exactly one of several mutually-exclusive fields (e.g. CloudWatch Logs ResourcePolicy: account-scoped by PolicyName, resource-scoped by ResourceArn). Today the generated adoption code requires one primary key, which blocks adopting the alternately-keyed variant.
This adds a mutually_exclusive_identifiers resource config listing those fields. From that single declaration the generator now emits:
ValidateConfig rejects a single-element list or combining with is_arn_primary_key.
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#76Previous Implementation
Previous iteration of this PR used a
is_primarykey_optionalconfig that only relaxed the requirement that a single field considered the primary key exists and is provided. However, this approach was dropped as it didn't sufficiently provide users feedback when adoption-fields was misconfigured or empty.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.