search

kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
10.7k stars 2.22k forks source link

Reconcile crds and openapi fields #3944

Open KnVerey opened 3 years ago

KnVerey commented 3 years ago

From a user's perspective, Kustomize is currently asking for custom resource schemas to be provided in two different fields, crds and openapi, in order for those resources to be fully supported (setting aside the legacy configurations field, which is a third way for some things!). We should consolidate this onto the openapi field.

natasha41575 commented 3 years ago

An idea stemming from discussion here: https://github.com/kubernetes-sigs/kustomize/issues/3961#issuecomment-855122175 cc @aodinokov

Alternative: Keep the crds field but have it be slightly different from the openapi field.

The main differences right now are:

  1. crds is an addition to the builtin schema. openapi entirely replaces the builtin schema - the user has to provide all the relevant definitions in order to use it.
  2. crds is used for FieldSpecs, while openapi is used for merge keys.

2 can be made consistent such that both crds and openapi can support specifying FieldSpecs and merge keys, but for backwards compatibility it would be good to keep the distinction described in 1. This way, a user only has to specify their custom resource schemas in one field and can choose whether to specify the whole document or just their CRDs.

madhukar93 commented 3 years ago

I was confused as to why crds did not support merge keys, seems like the feature wasn't requested then?

yhrn commented 3 years ago

I tried adding x-kubernetes-patch-merge-key and x-kubernetes-patch-strategy to a CRD in a cluster so that I could script downloading the OpenAPI schema via Kustomize and then get array merging to work but it turned out that the k8s CRD API does not support those properties so I'm not sure it's feasible for the Kustomize crds feature to support merge keys.

This whole thing also makes the openapi feature way less convenient for the CRD merge key use case since you basically need to download the schema, manually patch it with the merge key stuff, store it and manually keep it in sync with CRD changes. Or maybe I'm missing something here?

aodinokov commented 3 years ago

@natasha41575

thank you for looking into that and your analysis. If we're talking about this:

2 can be made consistent such that both crds and openapi can support specifying FieldSpecs and merge keys, but for backwards compatibility it would be good to keep the distinction described in 1

What will be a desired target state for kustomize? in another words - what will be a right behavior for kustomize? :) I guess we do want to make crds and openapi consistent (just because k8s does it somehow on its end), but if we also want to make this backward compatible - maybe it can be done by some additional option when we specify crd. e.g. setMergeKeys/setFieldSpecs: true|false so it could be configurable, but with proper defaults? :)

KnVerey commented 3 years ago

@yhrn I completely agree with you that the feature's usefulness would be greatly enhanced if these fields were directly supported in CRD. Here is the k/k issue you can follow for progress on making that possible. As you can see, Kustomize is one of many use cases cited, and though open for a long time, the issue was accepted very recently: https://github.com/kubernetes/kubernetes/issues/82942.

@aodinokov My two cents is that we could make what each affects configurable as part of a deprecation process, but that they should be consistent by default in Kustomization v1

marshall007 commented 2 years ago

@KnVerey are you aware of the history behind the extensions listed in https://github.com/kubernetes/kubernetes/issues/82942#issuecomment-854893056? I am still confused as to where these came from and why they are documented in the crds doc despite (AFAIK) not being supported by anything other than kustomize. Per #4095 I have never been able to find anything (including kustomize projects/users) that has actually used them.

Also, following the link @yhrn provided there is reference to a x-kubernetes-embedded-resource extension that appears to be supported:

... but it turned out that the k8s CRD API does not support those properties ...

The description of that property is:

x-kubernetes-embedded-resource defines that the value is an embedded Kubernetes runtime.Object, with TypeMeta and ObjectMeta. The type must be object. It is allowed to further restrict the embedded object. kind, apiVersion and metadata are validated automatically. x-kubernetes-preserve-unknown-fields is allowed to be true, but does not have to be if the object is fully specified (up to kind, apiVersion, metadata).

I can't tell based on this description whether that conflicts with the x-kubernetes-object-ref-* extensions that kustomize claims to support or if x-kubernetes-embedded-resource is only intended to be used when embedding full resource definitions (i.e. not for simple object refs).

@KnVerey I agree with this proposal and my only other question is how do we plan to handle built-in configurations (like namereference.go) going forward? Assuming https://github.com/kubernetes/kubernetes/issues/82942 is implemented and we have some x-kubernetes-* extensions we can look at to generate field specs, would we distribute kustomize with pre-compiled builtins or would users be required to download an openapi schema from their cluster for name references to work for builtin objects?

KnVerey commented 2 years ago

are you aware of the history behind the extensions listed in kubernetes/kubernetes#82942 (comment)?

I don't have additional context on that history unfortunately.

I agree with this proposal and my only other question is how do we plan to handle built-in configurations (like namereference.go) going forward? Assuming kubernetes/kubernetes#82942 is implemented and we have some x-kubernetes-* extensions we can look at to generate field specs, would we distribute kustomize with pre-compiled builtins or would users be required to download an openapi schema from their cluster for name references to work for builtin objects?

User-provided openapi is an override: Kustomize already embeds openAPI for builtin types and will continue to do so. IIRC it is primarily used to determine the correct merge strategy to use at the moment, but as @monopole said in the linked issue, we would love to drive the name references config from it as well. Users who deal exclusively with built-in types should not notice anything if/when that switchover happens.

james-callahan commented 2 years ago

There needs to be some way to add extra strategic merge keys for CRDs: requiring the entire openapi definition isn't going to be composable. What if openapi could take an array of patches to apply on top of the in-built definitions?

natasha41575 commented 2 years ago

/assign /triage accepted

natasha41575 commented 2 years ago

Copying from one of @KnVerey in another issue (#3961):

Instead of getting rid of the crds field, we could swap out the implementation to be driven by the same transformer as openapi, i.e. two different input formats but a consistent underlying feature set / result.

@KnVerey If you still support it, I would like to implement this proposal rather than deprecating crds, where crds takes the input format of CRD schemas while openapi will support openapi schemas, but both will support the same underlying feature set WRT to merge keys and fieldspecs. This will help users who cannot or don't want to apply their CRD to the cluster, run kustomize openapi fetch, and then patch up the returned schema.

What if openapi could take an array of patches to apply on top of the in-built definitions?

IMO both crds and openapi can supplement rather than replace the builtin schema.

natasha41575 commented 2 years ago

/retitle Reconcile crds and openapi fields

KnVerey commented 2 years ago

šŸ‘ I still support accepting both input formats and making the capabilities and results consistent

natasha41575 commented 2 years ago

I've been spending a bit of time trying to design a solution forĀ https://github.com/kubernetes-sigs/kustomize/issues/3418, and I've realized that any solution I propose will very closely interact with all three of these fields (openapi, crds, and configurations). The crds field won't make sense to keep as it is once we have a more generic object reference tracking solution so I'm now more learning towards deprecating and removing it, especially since the proposal I plan to make is to drive object reference config from the openapi field.

My initial reason to support keeping both of the fields was that crds is easier to author in some cases than openapi, but if we (a) make openapi additive, so that users can specify just their crds without needing to specify the entire builtin schema as well, and (b) add some commands to kustomize edit to automatically apply these extensions to the openapi based on an easier-to-author configuration file - there is no need to keep crds in addition to openapi, nor will there be a need to keep configurations.

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

KnVerey commented 1 year ago

/lifecycle frozen

k8s-triage-robot commented 11 months ago

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted