I still would not have expected duplicates... are we sure we want to ignore this and stomp schemas?

Is this related to #109275?

@liggitt: This is for CRD merging, not merging between all api resources. Between CRDs, things like ObjectMeta, etc are shared and should be safe for merging since they'll all be the same version served from the same apiextensions server. I don't think we'll run into any conflicts here? The v2 code assumes conflicts are ignored as well. https://github.com/Jefftree/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go#L63

The issue you linked is around merging between built-ins and CRDs in which conflicts may occur and should be a separate issue

around merging between built-ins and CRDs

Why would merging built-ins and CRDs would conflict?!

I'm not sure what merging is happening, but shouldn't we successfully identify metadata types in the same process as identical and combine them rather than see them as duplicates?

I'm not sure what merging is happening, but shouldn't we successfully identify metadata types in the same process as identical and combine them rather than see them as duplicates?

Yes. That's for solving #109275. This PR only tackles the merging between CRDs which should not have any conflicts (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go#L63).

Why would merging built-ins and CRDs would conflict?!

Sorry I mean merging between built-ins, CRDs and aggregated apiservers. I think we may have conflicts between merging between aggregated apiservers if there is version skew AND a resource that both apiservers depend on changes between versions. We should check if the resources are identical before creating a duplicate _v2 type in the merging process for that (OpenAPI V2 specific), so +1 to Jordan's comment. That's outside the scope of this PR though

/assign @apelisse @liggitt
/triage accepted

if we have 2 CRDs defined in the same group/version, are we merging two identical sets of metadata type definitions?

if so, comments like this are either wrong or confusing:

// For V3, the static spec is never merged with the individual CRD specs so no conflict resolution is necessary
func MergeSpecsV3(crdSpecs ...*spec3.OpenAPI) (*spec3.OpenAPI, error) {

rather than simply skipping or stomping, this should be more principled... some possibilities are:

• define the specific types this server can assume are identical and doesn't need to merge (e.g. TypeMeta, ObjectMeta, autoscaling.Scale, etc), and error if unexpected duplicates other than those types are encountered
• handle merging

this also needs to add tests for scenarios that could lead to duplicate types being encountered:

• multiple CRDs in the same group/version
• multiple CRDs defining scale subresources in the same group/version
• multiple CRDs defining status subresources in the same group/version

define the specific types this server can assume are identical and doesn't need to merge (e.g. TypeMeta, ObjectMeta, autoscaling.Scale, etc), and error if unexpected duplicates other than those types are encountered

This is sort of the approach I have gone with. I'm not sure if we need to list every single type that could conflict and opted to just look at the group version for meta.v1 and autoscaling.v1. I've added unit tests and am still looking into integration tests to properly create CRDs with Scale and Status subresources.

Once #109275 is fixed, we could probably look to change the CRD merging to also use the proper merge code implemented by the fix

Updated, PTAL @liggitt

/lgtm
/approve

thanks, please pick to release-1.24 as well

Thanks!

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jefftree, liggitt

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

The pull request process is described here