diff --git a/keps/sig-cli/kustomize-crd-integration.md b/keps/sig-cli/kustomize-crd-integration.md index f80c14e5d529..a48156e1c7dd 100644 --- a/keps/sig-cli/kustomize-crd-integration.md +++ b/keps/sig-cli/kustomize-crd-integration.md @@ -10,11 +10,12 @@ reviewers: - "@monopole" - "@Liujingfang1" approvers: + - "@monopole" - "@Liujingfang1" editors: - "@jbrette" -creation-date: 2019-09-01 -last-updated: 2019-09-01 +creation-date: 2019-09-15 +last-updated: 2019-09-15 status: provisional see-also: replaces: @@ -25,202 +26,367 @@ superseded-by: # Kustomize CRD Integration ## Table of Contents -* [Table of Contents](#table-of-contents) -* [Summary](#summary) -* [Motivation](#motivation) - * [Goals](#goals) - * [Non-Goals](#non-goals) -* [Proposal](#proposal) - * [Implementation Details/Notes/Constraints](#implementation-detailsnotesconstraints) - * [Risks and Mitigations](#risks-and-mitigations) -* [Graduation Criteria](#graduation-criteria) -* [Implementation History](#implementation-history) -* [Alternatives](#alternatives) +- [Kustomize CRD Integration](#kustomize-crd-integration) + - [Table of Contents](#table-of-contents) + - [Summary](#summary) + - [Motivation](#motivation) + - [Goals](#goals) + - [Non-Goals](#non-goals) + - [Proposal](#proposal) + - [Merge Patch handling](#merge-patch-handling) + - [Simple transformers handling](#simple-transformers-handling) + - [Complex transformers handling](#complex-transformers-handling) + - [Variable Transformer Integration](#variable-transformer-integration) + - [Risks and Mitigations](#risks-and-mitigations) + - [Graduation Criteria](#graduation-criteria) + - [Doc](#doc) + - [Test plan](#test-plan) + - [Version Skew Tests](#version-skew-tests) + - [Implementation History](#implementation-history) + - [Alternatives](#alternatives) [Tools for generating]: https://github.com/ekalinin/github-markdown-toc ## Summary -Kustomize builtin transformers are provided with default configurations describing which -resource fields are subject of a particular transformation. kustomize provides the ability -to add fields to the default configuration but does not let the user adjust that configuration -in order to skip a transformation for a specific resource or resource kind. +CRDs are increasly beeing used in Kubernetes projects. Istio, OpenShift, Argo resources, to name a few, are more and more present. Also most of builtin transformers can be configure (even if it can be quite tedious) to support CRDs, handling of the patches by the strategicMergePatch transformer is different between K8s native objects and CRDs. The first are using the SMP, the later are using JsonMergePatch. We are aiming at addressing those issues: -- Make cluster level kind configurable [#617](https://github.com/kubernetes-sigs/kustomize/issues/617) -- Do not add namespace to cluster scoped CRDs [#552](https://github.com/kubernetes-sigs/kustomize/issues/552) -- Skipping nameprefix for some resource kind [#519](https://github.com/kubernetes-sigs/kustomize/issues/617) -- Adding labels only to metadata.labels [#330](https://github.com/kubernetes-sigs/kustomize/issues/330) -- Skip adding labels to certain paths [#519](https://github.com/kubernetes-sigs/kustomize/issues/519) -- Unable to disable commonLabels injection using transformer config: Error: conflicting fieldspecs [#817](https://github.com/kubernetes-sigs/kustomize/issues/817) +- CRD system for OpenShift resources not interpreted as expected [#1531](https://github.com/kubernetes-sigs/kustomize/issues/1531) +- kustomize handling of CRD inconsistent with k8s handling starting in 1.16 [#1510](https://github.com/kubernetes-sigs/kustomize/issues/1510) ## Motivation - ### Goals -- Provide the ability for the user to skip transformation for specific group/version/kind especially usefull if the default - configuration is using a wild card (see namespace and prefixsuffix transformer configuration). -- Provide the ability of the user to replace or remove an entry in the default configuration which is not matching his needs - (see commonLabel transformer configuration) +Provide the ability for the user to integrate new CRD into kustomize flows: +- Help to register new CRDs into kustomize scheme to leverage SMP instead of just JMP. +- Create/Adjust built-in transformers config such as prefix, suffix, namespaces, annotations, namereference and varreference transformers. +- Adjust kustomize configuration to account for Cluster scoped CRDs. +- Help the user to leverage syntax check transformers such as kubeval. ### Non-Goals -- Provide backward compatibility with the transformer default configurations. -- Provide a way for the user to exclude a kind from a wild card based set as opposed to have to list all the possible kinds/gvks. -- Provide a way for the user to remove a configuration entry from the default configuration if it is not aligned with the needs - of his project. -- Provide a simple method to the transformer developers to select the list of "mutable" fields based on the gvk of the resources. - +- Account for improvments done in kubernetes in CRDs handling, such as go lang annotations to improve patching procedures. +- Keep kustomize build process independant of the CRDs go modules. + ## Proposal -The propsal has three parts: +The proposal is to try to align the behavior of the kustomize with the behavior of kubernetes/kubectl +- The proper handling of patching is done by the ability of a kubernetes operator to register the schema. +- The registration of the CRDs into the system, including the syntax is done through a yaml or json file. -1. Update the FieldSpec definition to add a "skip" field as follow (It is also useful external transformers needing wild card): +### Merge Patch handling + +The POC of solution is using the kustomize v3 plugin concept. The following dummy transformer/register allows +the user to build the following kustomize external plugin (go module) and get the kustomize to load it. ```go -type FieldSpec struct { - gvk.Gvk `json:",inline,omitempty" yaml:",inline,omitempty"` - Path string `json:"path,omitempty" yaml:"path,omitempty"` - CreateIfNotPresent bool `json:"create,omitempty" yaml:"create,omitempty"` - SkipTransformation bool `json:"skip,omitempty" yaml:"skip,omitempty"` +package main + +import ( + "github.com/keleustes/armada-crd/pkg/apis/armada/v1alpha1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/kustomize/v3/pkg/ifc" + "sigs.k8s.io/kustomize/v3/pkg/resmap" +) + +// plugin loads the ArmadaChart CRD scheme into kustomize +type plugin struct { + ldr ifc.Loader + rf *resmap.Factory } -``` - -2. Wrap []FieldSpec into FieldSpecs and add high level method to ease implementation of transformers, during loop accross - resources and fieldspec. +//nolint: golint +//noinspection GoUnusedGlobalVariable +var KustomizePlugin plugin -```go -type FieldSpecs []FieldSpec +func (p *plugin) Config( + ldr ifc.Loader, rf *resmap.Factory, _ []byte) (err error) { + p.ldr = ldr + p.rf = rf -func (s FieldSpecs) ApplicableFieldSpecs(x gvk.Gvk) FieldSpecs { -// .... + // Register the types with the Scheme so the components can map objects to GroupVersionKinds and back + return v1alpha1.SchemeBuilder.AddToScheme(scheme.Scheme) } -``` -3. Wrap FieldSpec into a FieldSpecConfig, to let the user remove and replace configuration entries embedded in the default configuration - provided with kustomize - -```go -type FieldSpecConfig struct { - FieldSpec `json:",inline,omitempty" yaml:",inline,omitempty"` - // Behavior legal values are "", "add", "replace", "remove" - Behavior string `json:"behavior,omitempty" yaml:"behavior,omitempty"` +func (p *plugin) Transform(m resmap.ResMap) error { + return nil } -``` - - -### prefixsuffix transformer example: - -The name prefixsuffix transformer is by default more or less modifying the name of every single resource since it is configured to use the wildcard as a kind criteria. +``` -The default configuration more or less looks like this: +The corresponding transformer.yaml: ```yaml -namePrefix: -- path: metadata/name +apiVersion: armada.airshipit.org/v1alpha1 +kind: ArmadaCRDRegister +metadata: + name: armadacrdregister ``` -The following adjustement, let the transformer indicates to the transformer to perform its tasks normally on the metadata/name field expect for Namespace, MyCRD and Ingress. +The correspoding kustomization.yaml: ```yaml -namePrefix: -- path: metadata/name - version: v1 - kind: Namespace - skip: true -- path: metadata/name - version: v1alpha1 - group: my.org - kind: MyCRD - skip: true -- path: metadata/name - group: extensions - version: v1beta1 - kind: Ingress - skip: true +resources: +- ./resources.yaml + +transformers: +- ./transformer.yaml ``` +This scheme registration is what drive kustomize to use [SMP vs JMP](https://github.com/kubernetes-sigs/kustomize/blob/master/k8sdeps/transformer/patch/conflictdetector.go#L154) -### namespace transformer example: +```go + versionedObj, err := scheme.Scheme.New(toSchemaGvk(id.Gvk)) + if err != nil && !runtime.IsNotRegisteredError(err) { + return nil, err + } + var cd conflictDetector + if err != nil { + cd = newJMPConflictDetector(rf) + } else { + cd, err = newSMPConflictDetector(versionedObj, rf) + if err != nil { + return nil, err + } +``` -In the same way, the namespace transformer is, by default, adding a namespace entry to any object which is not in the internal list of cluster wide resources. This performed by using a wildcard for kind criteria and excluding kind members of predefined cluster wide objects. +This way of loading the scheme should improved and streamlined to be more user friendly. -The default configuration more or less looks like this: +### Simple transformers handling -```yaml -namespace: -- path: metadata/namespace - create: true +When creating a new CRD, generating the yaml file used for kubectl and kustomize is necessary for deployment and quite [straightforward](https://github.com/keleustes/armada-crd/blob/master/Makefile#L45): +```bash +controller-gen crd paths=./pkg/apis/armada/... crd:trivialVersions=true output:crd:dir=./kubectl output:none ``` -The following example describes how indicated to kustomize not to add a metadata/namespace field to cluster wide CRD called MyCRD: +Using the yaml definition of the CustomResourceDefinition, could be done dynamically in the same way kustomize is currently loading the JSON version of those CRDs. This would: +- allow to register the Kind, Group and Version of the new CRD +- allow to register the scope of the CRD (Cluster/Namespaced). +- allow to have access to some part of the syntax (so the fieldSpecs) +- Kustomize could automatically detect those CRD.yaml among the list of files provided in the "resources:" field. + +An offline tool should also be able to scan the CRD.yaml and create the skeleton of configuration for each builtin transformer. User intervention would most likely be needed to trim the transformer config. +This tool would help create most of the fieldSpecs path: + +This would address two issues: +- Creating fieldSpec is quite error prone because some fields such as labels are often located deep inside the specs of the CRD (Path for Deployment is really deep). +- The mutableField method at the core the transformer needs to follow with specific rules to handle slice of maps. ```yaml -namespace: -- path: metadata/namespace +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + creationTimestamp: null + name: armadachartgroups.armada.airshipit.org +spec: + additionalPrinterColumns: + - JSONPath: .status.actualState + description: State + name: State + type: string + - JSONPath: .spec.targetState + description: Target State + name: Target State + type: string + - JSONPath: .status.satisfied + description: Satisfied + name: Satisfied + type: boolean + group: armada.airshipit.org + names: + kind: ArmadaChartGroup + listKind: ArmadaChartGroupList + plural: armadachartgroups + shortNames: + - acg + singular: armadachartgroup + scope: Namespaced + subresources: + status: {} + validation: + openAPIV3Schema: + description: ArmadaChartGroup is the Schema for the armadachartgroups API + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: ArmadaChartGroupSpec defines the desired state of ArmadaChartGroup + properties: + chartRefs: + description: reference to chart document + items: + type: string + type: array + sequenced: + description: enables sequenced chart deployment in a group + type: boolean + targetState: + description: Target state of the ChartGroups + type: string + required: + - charts + - targetState + type: object + status: + description: defines the observed state of ArmadaChartGroup + properties: + actualState: + description: Actual state of the ChartGroups + type: string + conditions: + description: List of conditions and states related to the resource. + items: + description: ResourceCondition represents one current condition of + a resource. + properties: + lastTransitionTime: + format: date-time + type: string + message: + type: string + reason: + type: string + resourceName: + type: string + resourceVersion: + format: int32 + type: integer + status: + description: ResourceConditionStatus represents the current status + of a Condition + type: string + type: + type: string + required: + - status + - type + type: object + type: array + reason: + description: Reason indicates the reason for any related failures. + type: string + satisfied: + description: Satisfied indicates if the actual state satisfies its target + state + type: boolean + required: + - actualState + - satisfied + type: object + type: object version: v1alpha1 - group: my.org - kind: MyCRD - skip: true + versions: + - name: v1alpha1 + served: true + storage: true +status: + acceptedNames: + kind: "" + plural: "" + conditions: [] + storedVersions: [] ``` +### Complex transformers handling + +The above simple CRD definition example also highlights the complexity of dealing with: +- NameReferences Configuration: The `chartRefs` is an array of reference to a 'chart' CRD. The names of those charts are impacted by prefix/suffix. It means the nameReference transformer configuration has to be be updated. + +Note: If you take the example of ArgoRollout CRD which syntax is very similar to the Deployment syntax, most of the kustomize default configuration related to Deployment resources have to be duplicated to create the ArgoRollout entries. + +Kustomize supports creation of some of the internal transformer config by ingesting json matching the CRD, +assuming that the following annotations have been addded to the [JSON file](https://github.com/kubernetes-sigs/kustomize/blob/master/pkg/transformers/config/factorycrd_test.go#L120): +- "x-kubernetes-annotation" +- "x-kubernetes-label-selector" +- "x-kubernetes-identity" +- "x-kubernetes-object-ref-api-version" +- "x-kubernetes-object-ref-kind" +- "x-kubernetes-object-ref-name-key" -### commonlabels transformer example: +The complexity relies in: +- accessing/generating the swagger.json equivalent which goes through the usage of kube-openapi (openapi-gen). + - [airshipit](https://github.com/keleustes/armada-crd/blob/master/Makefile#L65) + - [argoproj](https://github.com/argoproj/argo/blob/master/pkg/apis/workflow/v1alpha1/openapi_generated.go) +- adding the annotations (x-kubernetes-object-ref-kind) to the generated swagger.json seems to be manual. +- the keys important for merging are currently not beeing used by kustomize [x-kubernetes-patch-merge-key](https://github.com/argoproj/argo/blob/master/api/openapi-spec/swagger.json#L754) -Here is an extract of the default configuration for commonLabels +The swagger.json needs to be further refined to be usable by kustomize or by the very useful `kubeval` transformer (https://github.com/keleustes/armada-crd/blob/master/Makefile#L81) + +We need to find a way to streamline all that work, so that kustomize can handle are CRD in the same simple way it can handle a native K8s object. + + +### Variable Transformer Integration + +Almost every single field of a CR can potentially be the target of the variable replacement. +Forcing the user to manually translate the CRD.yaml into the VariableResolutionTransformer configuration is not pratical. In the case, kustomize could leverage the [Automatic Creation of 'vars:' and 'varReferences:' sections #1217](https://github.com/kubernetes-sigs/kustomize/pull/1217) internally add the following entries in the kustomization.yaml + +If the user creates the following entries: + +```yaml +kind: MyCRD1 +metadata: + name: mycrd1 +spec: + somefield: $(MyCRD2.mycrd2.spec.someotherfield) +``` ```yaml -commonLabels: -- path: metadata/labels - create: true -- path: spec/template/spec/affinity/podAffinity/requiredDuringSchedulingIgnoredDuringExecution/labelSelector/matchLabels - create: false - group: apps - kind: Deployment +kind: MyCRD2 +metadata: + name: mycrd2 +spec: + someotherfield: automatic variable resolution ``` -As for the namespace and prefixsuffix handling, the user can add a "skip" entry to exclude the CustomResourceDefinition from the wild card. In the following example, commonLabels will not be added for the metadata/labels section of the CustomResourceDefinition. +Kustomize, when using the PR, internally automatically creates the following entries: + +in the kustomization.yaml: ```yaml - -commonLabels: -- path: metadata/labels - version: v1beta1 - group: apiextensions.k8s.io - kind: CustomResourceDefinition - skip: true +var: +- name: MyCRD2.mycrd2.spec.someotherfield + objref: + kind: MyCRD2 + name: mycrd2 + fieldref: + fieldpath: spec.someotherfield ``` -The remaining case is linked to the handling of a none wild card entry. By default kustomize adds commonLabel of the selectors -of development. Depending on the project, this can be an issue. The `behavior: remove` or `behavior: replace` can be used to -changed the handlig of the field by the transformer. +in the transformer configuration kustomizeconfig.yaml: ```yaml -- path: spec/template/spec/affinity/podAntiAffinity/requiredDuringSchedulingIgnoredDuringExecution/labelSelector/matchLabels - create: false - group: apps - kind: Deployment - behavior: remove +varReference: +- kind: MyCRD1 + path: spec/somefield ``` ### Risks and Mitigations -- Risk: Configuration drifting from Kubernetes best pratices. -- Mitigation: Good documentation and recommendations about how to keep things simple. +- Risk: Using go modules/plugins is quite tricky. If a plugin requires a version of module different from the one used when + kustomize was build (from instance yaml...), the plugin will not load. +- Risk: The "registration" of the new types need to happen before the first merge/mergeconflictdetection is invoked. +- Risk: During the SMP invocation, some of the syntax check is forced before the other transformers and generators are invoked. + This may force the user to write update his patches to be compliant with new rules (similar to be forced to specify the name: attribute when patching the container slice of a Deployment) ## Graduation Criteria Use customer feedback to determine if we should support: -- Needs to be backward compatible with current builtin and external plugins. -- Needs to get feedback on the "skip: true" concept for wild card handling. "skip: true" can be used for external transformers. -- Needs to get feedback on the "behavior: replace" and "behavior: remove". This proposal reuse the concept used for configmapgenerator - and secretgenerator. Concept is all really close from JSONPath behavior. - +- Have a utilitary tool used to be build a plugin/transformers that would excapsulate the transformer configurations for that CRD. +- Ability to get kustomize to load that plugin at runtime. ## Doc @@ -230,10 +396,10 @@ Update Kubectl Book and Kustomize Documentation Unit Tests for: -- [ ] All existing test needs to be successful. -- [ ] Can skip wild card metadata/name, metadata/namespace and metadata/labels in new CRDs. -- [ ] Can remove "selectors" fields for the list of fields mutated by the labels transformer. - +- [ ] Ability to load AirshipIt CRDs. +- [ ] Ability to load OpenShift CRDs +- [ ] Ability to load ArgoProj CRDs. +- [ ] Ability to load Istio CRDs. ### Version Skew Tests @@ -241,74 +407,11 @@ Unit Tests for: The following POC PR have been proposed: -- [feat: skip name prefix/suffix by kind #1485](https://github.com/kubernetes-sigs/kustomize/pull/1485) -- [skip kustomize transformers for paths #1491](https://github.com/kubernetes-sigs/kustomize/pull/1491) +- PR1 +- PR2 ## Alternatives -During the discussion thread related to "skip" feature [here](https://github.com/kubernetes-sigs/kustomize/pull/1485), alternative have been proposed. - -Regardless if the main proposal, alternative 1 or alternative 2 is choosen, those options have in common that a set of wrapper functions and structs needs to be provided by core kustomize packages to ease the selection of the fields to mutate by the transformers. - - -### Alternative 1 - -Creating a new SkipSpec field like follow: - -```go -type SkipSpec struct { - FieldSpec `json:",inline,omitempty" yaml:",inline,omitempty"` - Transformers []string `json:"transformers,omitempty" yaml:"transformers,omitempty"` -} -``` - -The transformer configurations would include a new field skipTransformation: - -```yaml -skipTransformation: - - path: spec/selector - kind: Service - transformers: - - commonlabels - - kind: CustomResourceDefinition - transformers: - - nameprefix - - kind: MyKind - transformers: - - nameprefix - - namespace - - kind: ClusterLeveledCRDs - transformers: - - namespace - ``` - -### Alternative 2 - -The other considered alternate is to create the following struct: - -```go -type SkipSpec struct { - FieldSpec `json:",inline,omitempty" yaml:",inline,omitempty"` - SkipTransformation bool `json:"skip,omitempty" yaml:"skip,omitempty"` -} -``` - -and change the Transformerconfiguration passed to the transformer from: - -```go -type ATransformer struct { - FieldSpecs []config.FieldSpec -} -``` - -to - -```go -type ATransformer struct { - FieldSpecs []config.FieldSpec - SkipFieldSpecs []config.FieldSpec -} -``` - - - +A workaround to not using the +[scheme](https://github.com/kubernetes-sigs/kustomize/blob/master/k8sdeps/transformer/patch/conflictdetector.go#L154) but would force kustomize to reimplement the content of [k8s.io/apimachinery/pkg/util/strategicpatch +](https://github.com/kubernetes-sigs/kustomize/blob/master/k8sdeps/transformer/patch/conflictdetector.go#L133), but would still be possible.