Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions pkg/config/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,17 @@ type ResourceConfig struct {
// IsARNPrimaryKey determines whether the CRD uses the ARN as the primary
// identifier in the ReadOne operations.
IsARNPrimaryKey bool `json:"is_arn_primary_key"`
// MutuallyExclusiveIdentifiers lists the resource's identifier fields (by
// their configuration/original names, e.g. PolicyName, ResourceArn) when a
// resource has no single mandatory identifier but is instead identified by
// exactly one of several mutually-exclusive fields. When set, adoption
// treats each listed field as optional (populating whichever the user
// supplies) but requires that exactly one of them is present:
// PopulateResourceFromAnnotation returns a terminal error if none or more
// than one is supplied. This prevents an empty or misspelled adoption
// annotation from silently matching an arbitrary resource. Requires at
// least two fields and is incompatible with is_arn_primary_key.
MutuallyExclusiveIdentifiers []string `json:"mutually_exclusive_identifiers,omitempty"`
// TagConfig contains instructions for the code generator to generate
// custom code for ensuring tags
TagConfig *TagConfig `json:"tags,omitempty"`
Expand Down Expand Up @@ -512,6 +523,20 @@ func (c *Config) ResourceIsAdoptable(resourceName string) bool {
return *rConfig.IsAdoptable
}

// ResourceMutuallyExclusiveIdentifiers returns the list of mutually-exclusive
// identifier field names configured for the resource (see
// mutually_exclusive_identifiers), or nil if none are configured.
func (c *Config) ResourceMutuallyExclusiveIdentifiers(resourceName string) []string {
if c == nil {
return nil
}
rConfig, ok := c.Resources[resourceName]
if !ok {
return nil
}
return rConfig.MutuallyExclusiveIdentifiers
}

// ResourceContainsAttributesMap returns true if the underlying API has
// Get{Resource}Attributes/Set{Resource}Attributes API calls that map real,
// schema'd fields to a raw `map[string]*string` for a given resource name (see SNS and
Expand Down
29 changes: 29 additions & 0 deletions pkg/config/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,39 @@ func ValidateConfig(

errs = append(errs, validateRenameOperations(cfg, sdkOperations)...)
errs = append(errs, validateIgnoredOperations(cfg, sdkOperations)...)
errs = append(errs, validateMutuallyExclusiveIdentifiers(cfg)...)

return errs
}

// validateMutuallyExclusiveIdentifiers checks that a resource's
// mutually_exclusive_identifiers configuration is internally consistent: it
// must list at least two fields (a single identifier is not mutually exclusive
// with anything) and cannot be combined with is_arn_primary_key, since an
// ARN-primary resource always requires its ARN.
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
Comment on lines +57 to +77

Copy link
Copy Markdown
Contributor

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:

  1. [PolicyName, policyName] passes len < 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.
  2. Field names aren't verified. A typo fails generation only via PopulateResourceFromAnnotation, without the resources.<R>.mutually_exclusive_identifiers: context. ValidateConfig has no model, but pkg/model/model.go has a post-processFields block doing this for resources.<R>.fields.<F> — likely the right home.
  3. custom_check_required_fields_missing_method makes 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.
  4. Declaring identifiers the read op marks required (deliberate, per the comment in set_resource.go) turns the ReadOne check from || into &&. With only Type set it passes, GetSecurityPolicy gets Name=nil, and fails with a ValidationException instead of NotFound → create. The only test for that branch is the opensearchserverless fixture, which says itself that Name/Type aren't really exclusive. Either allow it with a real fixture, or reject it here.
  5. This option alone doesn't make adoption safe — each identifier also needs a read-request filter or a list_operation.match_fields entry, or the List is unfiltered and sdkFind takes the first result. Patch the correct observed state after a rm.ReadOne call #76 sets match_fields: [PolicyName]; the fixture here doesn't, and nothing warns.

}

// validateRenameOperations checks that operation names referenced in
// resources[R].renames.operations[OpName] exist in the SDK.
func validateRenameOperations(
Expand Down
58 changes: 58 additions & 0 deletions pkg/config/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,64 @@ func TestValidateConfig_ErrorMessageIncludesAvailable(t *testing.T) {
}
}

func TestValidateMutuallyExclusiveIdentifiers(t *testing.T) {
testCases := []struct {
name string
resourceCfg ResourceConfig
expectedErrs int
}{
{
name: "valid two identifiers",
resourceCfg: ResourceConfig{
MutuallyExclusiveIdentifiers: []string{"PolicyName", "ResourceArn"},
},
expectedErrs: 0,
},
{
name: "unset is allowed",
resourceCfg: ResourceConfig{},
expectedErrs: 0,
},
{
name: "single identifier is invalid",
resourceCfg: ResourceConfig{
MutuallyExclusiveIdentifiers: []string{"PolicyName"},
},
expectedErrs: 1,
},
{
name: "incompatible with is_arn_primary_key",
resourceCfg: ResourceConfig{
MutuallyExclusiveIdentifiers: []string{"PolicyName", "ResourceArn"},
IsARNPrimaryKey: true,
},
expectedErrs: 1,
},
{
name: "single identifier and arn primary key reports both",
resourceCfg: ResourceConfig{
MutuallyExclusiveIdentifiers: []string{"PolicyName"},
IsARNPrimaryKey: true,
},
expectedErrs: 2,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
cfg := &Config{
Resources: map[string]ResourceConfig{
"ResourcePolicy": tc.resourceCfg,
},
}
errs := validateMutuallyExclusiveIdentifiers(cfg)
if len(errs) != tc.expectedErrs {
t.Errorf("expected %d errors, got %d: %v", tc.expectedErrs, len(errs), errs)
}
})
}
}

func TestFormatAvailableTruncated(t *testing.T) {
items := []string{"A", "B", "C", "D", "E"}
got := formatAvailableTruncated(items, 3)
Expand Down
78 changes: 78 additions & 0 deletions pkg/generate/code/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
}

Expand Down Expand Up @@ -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, " || ")
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

reqIdentifier, _ := FindPluralizedIdentifiersInShape(r, shape, op)
resVarPath, err := r.GetSanitizedMemberPath(reqIdentifier, op, koVarName)
if err != nil {
Expand Down
59 changes: 59 additions & 0 deletions pkg/generate/code/check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,65 @@ func TestCheckRequiredFields_StatusField_ReadMany_EgressOnlyIGW(t *testing.T) {
)
}

func TestCheckRequiredFields_MutuallyExclusiveIdentifiers_ReadOne(t *testing.T) {
assert := assert.New(t)
require := require.New(t)

g := testutil.NewModelForServiceWithOptions(t, "opensearchserverless", &testutil.TestingModelOptions{
GeneratorConfigFile: "generator-with-mutually-exclusive-identifiers.yaml",
})

crd := testutil.GetCRDByName(t, g, "SecurityPolicy")
require.NotNil(crd)
require.True(crd.HasMutuallyExclusiveIdentifiers())

// GetSecurityPolicy (ReadOne) marks both `name` and `type` required.
// Declaring them mutually exclusive drops each from the per-field `||` list
// and collapses them into a single grouped condition that is true only when
// neither identifier is set.
expRequiredFieldsCode := `
return (r.ko.Spec.Name == nil && r.ko.Spec.Type == nil)
`
gotCode, err := code.CheckRequiredFieldsMissingFromShape(
crd, model.OpTypeGet, "r.ko", 1,
)
require.NoError(err)
assert.Equal(
strings.TrimSpace(expRequiredFieldsCode),
strings.TrimSpace(gotCode),
)
}

func TestCheckRequiredFields_MutuallyExclusiveIdentifiers_ReadMany(t *testing.T) {
assert := assert.New(t)
require := require.New(t)

g := testutil.NewModelForServiceWithOptions(t, "cloudwatch-logs", &testutil.TestingModelOptions{
GeneratorConfigFile: "generator.yaml",
})

crd := testutil.GetCRDByName(t, g, "ResourcePolicy")
require.NotNil(crd)
require.True(crd.HasMutuallyExclusiveIdentifiers())

// DescribeResourcePolicies has no required input members, so without
// mutually_exclusive_identifiers this would generate `return false` and let
// sdkFind match an arbitrary policy. Instead, the read input is treated as
// incomplete unless at least one of the declared identifiers is set, which
// is what the controller previously had to supply via a custom method.
expRequiredFieldsCode := `
return r.ko.Spec.PolicyName == nil && r.ko.Spec.ResourceARN == nil
`
gotCode, err := code.CheckRequiredFieldsMissingFromShape(
crd, model.OpTypeList, "r.ko", 1,
)
require.NoError(err)
assert.Equal(
strings.TrimSpace(expRequiredFieldsCode),
strings.TrimSpace(gotCode),
)
}

func TestCheckNilFieldPath(t *testing.T) {
// Empty FieldPath
field := model.Field{Path: ""}
Expand Down
Loading