🐛 Dedupe equal Default values in flattenAllOfInto

[ravisastryk]

Adds an explicit case "Default": so two *apiextensionsv1.JSON pointers with equal raw bytes are deduped instead of hoisted into a fresh allOf, and adds regression tests for the equal-bytes and differing-bytes cases.

Follow-up to #1027 / #1035.

What does this do, and why do we need it?

Closes the // TODO(directxman12): Default -- use field? in pkg/crd/flatten.go by giving Default an explicit merge case in flattenAllOfInto.

Today the Default field of JSONSchemaProps is *apiextensionsv1.JSON. That type is Comparable in reflect's sense, but == on it compares pointer addresses, not pointee bytes. The equality check at flatten.go:107:

if fldTyp.Comparable() && srcInt == dstInt {
    continue // same value
}

returns false for two distinct allocations that both wrap "TCP". With no explicit case, the field falls into the default: branch which hoists both values into a fresh allOf, producing structural-schema-violating output:

protocol:
  allOf:
  - default: TCP
  - default: TCP
  type: string

The API server rejects this with:

properties[protocol].allOf[0].default: Forbidden: must be undefined to be structural
properties[protocol].allOf[1].default: Forbidden: must be undefined to be structural

How this relates to #1035

#1035 fixed the only known producer of this shape by removing the hard-coded corev1.Protocol schema from pkg/crd/known_types.go once k8s.io/api started shipping the +default="TCP" marker upstream. That fix is preserved here — including the regression test on CronJobSpec.Protocol in pkg/crd/testdata/cronjob_types.go, which now also serves as a higher-level integration check for this PR.

This PR closes the merge-logic half of the problem so the same YAML can't recur from a future KnownPackages override, a +default=-carrying alias type, or any other code path that puts a default on a referenced type schema.

Behavior change

flattenAllOfInto now handles Default explicitly:

• Equal raw bytes → dedupe, keep one, no allOf.
• Differing bytes → keep dst (parent / field-level value), drop src. This matches how XPreserveUnknownFields and XMapType are already merged: the field-level value wins over the type-level value, since users add +default= on a field precisely to override anything the type provides. No error is recorded — silently honoring the override is the less surprising behavior. If reviewers prefer an error here, swapping the final continue for errRec.AddError(...) is a one-line change.

No public API changes. No marker changes. No testdata bumps. Behavior change is limited to schemas that previously produced default: ... inside allOf, which were already broken at apply-time, so this can only fix CRDs, not break them.

Test plan

go test ./pkg/crd/...

Two new specs in pkg/crd/flatten_all_of_test.go:

1. Two *JSON defaults with equal raw bytes (the Error applying a CRD generated by controller-tools that contains the ContainerPort struct #1027 shape) — expects no allOf and a single preserved default.
2. Two *JSON defaults with different raw bytes — expects the dst (field-level) value to win and no allOf.

The existing It("should leave properties not in an AllOf branch ... alone") spec still passes, confirming we haven't regressed the "no merge needed" path.

The Protocol corev1.Protocol field added to the cronjob testdata in #1035 continues to flatten to a single default: TCP (verified against pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml).

What issue does this fix

Fixes #65

Follow-up to / completes #1027 (originally fixed by #1035).

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

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

[k8s-ci-robot]

Welcome @ravisastryk!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @ravisastryk. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

[ravisastryk]

@sbueringer @alvaroaleman Please review when you get a chance

[camilamacedo86]

/ok-to-test

[camilamacedo86]

I have minor suggestions. This PR sort out:

Example

Before PR

  protocol:                                                                                     
    allOf:                                                                                      
    - default: "TCP"                                                                            
    - default: "TCP"  # duplicate!                                                              
    type: string                                            

Very similar to the issue fixed in the PR as well: #1398
Also, see that we have lint issues to sort out, that is why I reduced the complexity by creating its own func in: #1398

It might be easier if we get merged: #1398, then you do that on top.

Beyond the nits, it seems great !!!.
Could you please address those, then I am happy to LGTM.