🐛 Prevent XValidation duplication by verifying if the rule already exists

[mcbenjemaa]

Analyse:

When defining a custom type and applying a cel expression on the type level, and if you reuse the same type twice or more in the spec, the XValidation will be duplicated, which causes errors in patching the CRDs:

x-kubernetes-validations: duplicate entries for key [rule=

XValidation is always appended to the schema, without checking if the validation.rule already exists.

This PR adds:

• Verifies if XValidation.Rule already exists in the schema before appending it.
• Simple test case, Add another field with the same type that defines a cel expression.

Closes #1294

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mcbenjemaa
Once this PR has been reviewed and has the lgtm label, please assign sbueringer for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[mcbenjemaa]

/ok-to-test

[JoelSpeed]

I'm not convinced that deduplication is the correct fix for this issue. Clearly we are running the ApplyToSchema twice on this XValidation rule, but why? We should only be applying once

I'm also curious how this issue has only just cropped up. I've checked our APIs and we have plenty of XValidations, none of which have been duplicated.

I think this needs some deeper root cause analysis before we can propose a fix

[mcbenjemaa]

I'm not convinced that deduplication is the correct fix for this issue. Clearly we are running the ApplyToSchema twice on this XValidation rule, but why? We should only be applying once

I'm also curious how this issue has only just cropped up. I've checked our APIs and we have plenty of XValidations, none of which have been duplicated.

I think this needs some deeper root cause analysis before we can propose a fix

The issue happens when XValidation rule is applied on the type level, and used twice or more within the spec.
ApplyToSchema is run twice cause it's called on the field level with the same type, which causes the field that has the XValidation already, to call ApplyToSchema again when adding the second field.

that's my conclusion until now.

but I agree it would be great if we can find a better solution that prevent the XValidation.ApplyToSchema run twice on the same Type.
please do not hesitate, if you have something to share that could help me in analysing the root cause.

[mcbenjemaa]

Deep Analyse

I did another analysis:

The issue seems to happen when a pointer type aliases with validation markers (like type MTU *uint64) cause duplication, while struct types and primitive types don't.

A Pointer type alias, is processed through localNamedToSchema -> schemaFetcher -> cached schema
multiple fields using the alias get pointers to the same cached schema object, which result applyMarkers is called multiple times on the same shared schema. So Validation rules gets duplicated

whereas struct types gets processed differently through structToSchema, in this case each field usage gets its own schema instance and applyMarkers is called on separate schema objects.

Lastly, for primitive types it gets handled by builtinToType and return immediately

The shared cached schema is returned from the schema fetcher NeedSchemaFor()
here:

controller-tools/pkg/crd/parser.go

Line 179 in bf7d6b7

return &props

Proposed fix:

The fix should be in the schema fetcher to deep copy the schema when returning it, ensuring each field gets its own copy:

parser.go#197

return props.DeepCopy()

This will prevent the duplication issue root cause.

/cc @JoelSpeed @sbueringer
Please have a look and let me know if you agree with the outcome.
The PR is not updated yet, once you agree with this approach , i will push the changes.

[JoelSpeed]

Please have a look and let me know if you agree with the outcome.

Yes, providing a new instance of the schema from the cache as a deepcopy seems like a good fix after reviewing your notes. Thanks for taking the time to go deeper on this!