-
Notifications
You must be signed in to change notification settings - Fork 250
Add mutually_exclusive_identifiers config for resources with no single primary key #736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
bcad676
eb31669
e45a4db
8111453
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,42 @@ func CheckRequiredFieldsMissingFromShape( | |
| ) | ||
| } | ||
|
|
||
| // mutuallyExclusiveIdentifierNilConditions returns, for each of the resource's | ||
| // configured mutually-exclusive identifier fields, a "<path> == nil" condition | ||
| // string, along with the set of CR paths so callers can exclude them from | ||
| // per-field required checks. The resource is uniquely identified by exactly one | ||
| // of these fields, so it is considered incomplete only when all of them are | ||
| // nil. Returns nil slices when the resource has no mutually-exclusive | ||
| // identifiers. | ||
| func mutuallyExclusiveIdentifierNilConditions( | ||
| r *model.CRD, | ||
| koVarName string, | ||
| ) ([]string, map[string]bool, error) { | ||
| if !r.HasMutuallyExclusiveIdentifiers() { | ||
| return nil, nil, nil | ||
| } | ||
| identifierFields, err := r.GetMutuallyExclusiveIdentifierFields() | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| cfg := r.Config() | ||
| conditions := make([]string, 0, len(identifierFields)) | ||
| paths := make(map[string]bool, len(identifierFields)) | ||
| for _, identifierField := range identifierFields { | ||
| memberPath, targetField := findFieldInCR(cfg, r, identifierField.Names.Original) | ||
| if targetField == nil { | ||
| return nil, nil, fmt.Errorf( | ||
| "resource %q: mutually_exclusive_identifiers field %q is not in the CR's Spec or Status", | ||
| r.Names.Original, identifierField.Names.Original, | ||
| ) | ||
| } | ||
| path := fmt.Sprintf("%s%s.%s", koVarName, memberPath, targetField.Path) | ||
| conditions = append(conditions, fmt.Sprintf("%s == nil", path)) | ||
| paths[path] = true | ||
| } | ||
| return conditions, paths, nil | ||
| } | ||
|
|
||
| func checkRequiredFieldsMissingFromShape( | ||
| r *model.CRD, | ||
| koVarName string, | ||
|
|
@@ -104,7 +140,26 @@ func checkRequiredFieldsMissingFromShape( | |
| shape *awssdkmodel.Shape, | ||
| ) (string, error) { | ||
| indent := strings.Repeat("\t", indentLevel) | ||
|
|
||
| // When the resource declares mutually-exclusive identifiers, the resource is | ||
| // uniquely identified by exactly one of the declared fields. Build a single | ||
| // grouped condition that is true only when none of them are set, and collect | ||
| // their CR paths so they are not required individually below. This makes the | ||
| // generated check treat the input as incomplete unless at least one | ||
| // identifier is present, mirroring the ReadMany handling. | ||
| exclusiveConditions, exclusivePaths, err := mutuallyExclusiveIdentifierNilConditions(r, koVarName) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| exclusiveGroupCondition := "" | ||
| if len(exclusiveConditions) > 0 { | ||
| exclusiveGroupCondition = fmt.Sprintf("(%s)", strings.Join(exclusiveConditions, " && ")) | ||
| } | ||
|
|
||
| if shape == nil || len(shape.Required) == 0 { | ||
| if exclusiveGroupCondition != "" { | ||
| return fmt.Sprintf("%sreturn %s\n", indent, exclusiveGroupCondition), nil | ||
| } | ||
| return fmt.Sprintf("%sreturn false", indent), nil | ||
| } | ||
|
|
||
|
|
@@ -144,8 +199,16 @@ func checkRequiredFieldsMissingFromShape( | |
| r.Names.Original, memberName, shape.ShapeName, | ||
| ) | ||
| } | ||
| // Mutually-exclusive identifiers are not required individually; they are | ||
| // covered by the grouped condition appended below. | ||
| if exclusivePaths[resVarPath] { | ||
| continue | ||
| } | ||
| missing = append(missing, fmt.Sprintf("%s == nil", resVarPath)) | ||
| } | ||
| if exclusiveGroupCondition != "" { | ||
| missing = append(missing, exclusiveGroupCondition) | ||
| } | ||
| // Use '||' because if any of the required fields are missing the object | ||
| // is not created yet | ||
| missingCondition := strings.Join(missing, " || ") | ||
|
|
@@ -178,6 +241,21 @@ func checkRequiredFieldsMissingFromShapeReadMany( | |
| indent := strings.Repeat("\t", indentLevel) | ||
| result := fmt.Sprintf("%sreturn false", indent) | ||
|
|
||
| // When the resource declares mutually-exclusive identifiers, the ReadMany | ||
| // input typically has no required members, so the default `return false` | ||
| // would let sdkFind list every resource and match an arbitrary one. Instead, | ||
| // treat the read input as incomplete (returning true so sdkFind bails out | ||
| // with NotFound) unless at least one of the declared identifiers is set on | ||
| // the resource. The mutually_exclusive_identifiers guard in | ||
| // PopulateResourceFromAnnotation still rejects supplying more than one. | ||
| if r.HasMutuallyExclusiveIdentifiers() { | ||
| exclusiveConditions, _, err := mutuallyExclusiveIdentifierNilConditions(r, koVarName) | ||
| if err != nil { | ||
| return result | ||
| } | ||
| return fmt.Sprintf("%sreturn %s\n", indent, strings.Join(exclusiveConditions, " && ")) | ||
|
Comment on lines
+251
to
+256
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This drops the error and returns A typo'd config name makes 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; Also, ReadOne parenthesizes the grouped condition and this doesn't — fine today, but this branch ignores |
||
| } | ||
|
|
||
| reqIdentifier, _ := FindPluralizedIdentifiersInShape(r, shape, op) | ||
| resVarPath, err := r.GetSanitizedMemberPath(reqIdentifier, op, koVarName) | ||
| if err != nil { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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.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.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.