OpenAPI field should support merging

gruberrichard commented 2 years ago

Describe the bug

We have four directories:

deployment-base - A simple Deployment resource

resources:
- deployment.yaml

deployment-patch - Patch for deployment-base's Deployment resource

resources:
- ../deployment_base

patches:
- deploy_patch.yaml

rollout-base - A Rollout resource with its openapi definition

resources:
- rollout.yaml

openapi:
  path: rollout_cr_schema.json

rollout-patch - Patch for rollout-base's Rollout resource

resources:
- ../rollout_base

patches:
- rollout_patch.yaml

If I run kustomize build deployment-patch it works as expected and it does patch the Deployment resource

If I run kustomize build rollout-patch it works as expected and it does patch the Rollout resource

If I run kustomize build with the following kustomization.yaml then it works as expected (and both resources are patched properly):

resources:
- deployment_patch
- rollout_patch

But when I change the sequence to the following

resources:
- rollout_patch
- deployment_patch

then Rollout resource is still patched properly but at Deployment resource the container definition does not contain properties that are defined only in base.

I have prepared a repo with the details and examples: https://github.com/gruberrichard/kustomize-bug-with-openapi

Expected output

I would expect that the generated manifests are the same (and patching of Deployment resource does not break) if I change the sequence of resource directories

Kustomize version

{Version:kustomize/v4.4.1 GitCommit:b2d65ddc98e09187a8e38adc27c30bab078c1dbf BuildDate:2021-11-11T23:36:27Z GoOs:darwin GoArch:amd64}

Platform

macOS

dan-j commented 2 years ago

I've just ran into this issue with a Deployment's containers object not merging properly with my openapi definition just containing a Knative service schema. Is it not intended behaviour to merge the definitions provided by openapi: with the builtin definitions? Or do I need to commit the whole output of kustomize openapi fetch to my repository and then manually add the CRD?

perenesenko commented 2 years ago

I'm facing issues when trying to patch different resources at a time: rollout, services, etc.

Once I specified the openapi.path field to the rollout schema the other components (services) are not merging as expected anymore. Feeling like there is some built-in schema exists if no openapi.path field is provided. But once you provide your custom one then the build-in schema is not respected anymore.

Is there a way to add/append your custom schema to the existing built-in so all the resources will be patched as expected?

michaelmohamed commented 2 years ago

I am having the same issue where when openapi path is added, my deployments are missing specified resource limits that are added with a patch.

If I remove the openapi path then my deployment limit is patched properly.

zachaller commented 2 years ago

Argo project will have a workaround for this soon with a blog post detailing how we came about it. But you can peak at this as well https://github.com/argoproj/argo-schema-generator

michaelmohamed commented 2 years ago

Thanks @zachaller, I used the https://raw.githubusercontent.com/argoproj/argo-schema-generator/main/schema/argo_all_k8s_kustomize_schema.json contents as my openapi.path and that worked well. My resource limits and requests all rendered correctly.

@dan-j, I also verified that your solution worked. I used the output of kustomize openapi fetch as my openapi.path and everything rendered correctly; however, I think that this assumes that argo has been installed in your cluster previously.

I opted for @zachaller since it seemed clean and also resulted in a smaller schema size.

Thank @zachaller

KnVerey commented 2 years ago

Those who commented that the openapi field requires the full schema, not just additions, are correct. So it isn't a bug, but there are a few related problems:

The docs for the openapi field show it being used to add a schema containing a single definition. While the example works (because its resources only have the type in question), it is not representative of a real use case.
The openapi field needs to be reconciled with the crds field (which is additive) in general: https://github.com/kubernetes-sigs/kustomize/issues/3944
The desired behaviour in this issue is essentially a feature request that @natasha41575 and I agreed to in general terms in the above issue: there should be a way to merge the schemas provided in openapi with the built-ins ( perhaps a behavior: merge|replace field, like ConfigMapGenerator?).

/triage accepted /kind feature /kind documentation /retitle OpenAPI field should support merging

natasha41575 commented 2 years ago

@KnVerey FYI, I have an umbrella issue with some of the things I want to do with openapi: https://github.com/kubernetes-sigs/kustomize/issues/4569

It includes a note that the supplied openapi should be an addition to what's builtin, rather than replacing it all.

zachaller commented 2 years ago

I have a blog post that should be posted this Thursday (8/25/2022) here that will also go into some of these issues as well. But really looking forward to either changes from kustomize or k8s to help with this issue as well.

zachaller commented 2 years ago

For those interested https://blog.argoproj.io/argo-crds-and-kustomize-the-problem-of-patching-lists-5cfc43da288c

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:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

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 active contributors to adequately respond to all issues and PRs.

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

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

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

/lifecycle rotten

k8s-triage-robot commented 1 year ago

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

This bot triages issues according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue with /reopen
Mark this issue as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

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

/close not-planned

k8s-ci-robot commented 1 year ago

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to [this](https://github.com/kubernetes-sigs/kustomize/issues/4613#issuecomment-1399584207): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages issues according to the following rules: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed > >You can: >- Reopen this issue with `/reopen` >- Mark this issue as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

KnVerey commented 1 year ago

/lifecycle frozen

larsks commented 11 months ago

The custom openapi example in the documentation suggests strongly that the openapi keyword merges in the custom schema rather than completely replacing the compiled-in schema.

We should update the example to point out that it is replacing the default schema and with that example, kustomize will no longer correctly handle lists in native resources like pods, deployments, etc.

snuggie12 commented 11 months ago

I agree the docs could be more clear. I noticed under openapi there is no list, just path so I figured all CRDs I want to edit need to be in the same file but didn't imagine it was everything.

I'm going to try and implement a relatively not too complex system for this using kustomize and some text parsing.

If you download all the schemas and store them in their own kustomize directories, attach a random GVK, patch the schemas you want to patch, run a kustomize that calls all the directories, chop off the random GVK and add in definitions at the top you got yourself a patching system for schemas:

$ head rt.openapi.yaml
apiVersion: v12
kind: OpenApi
metadata:
  name: io.solo.gateway.v1.RouteTable
def:
  io.solo.gateway.v1.RouteTable:
    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/sig-architecture/api-conventions.md#resources'
        type: string

$ cat rt.openapi.patch.yaml
apiVersion: v12
kind: OpenApi
metadata:
  name: io.solo.gateway.v1.RouteTable
def:
  io.solo.gateway.v1.RouteTable:
    properties:
      spec:
        properties:
          routes:
            x-kubernetes-patch-merge-key: name
            x-kubernetes-patch-strategy: merge

$ kust build | yq '.def."io.solo.gateway.v1.RouteTable".properties.spec.properties.routes | keys'
- items
- type
- x-kubernetes-patch-merge-key
- x-kubernetes-patch-strategy

larsks commented 11 months ago

@snuggie12 I've implemented something like that here.

kubernetes-sigs / kustomize

OpenAPI field should support merging #4613