helm: can't upgrade charts with crd changes

cc. @bacongobbler - sorry to tag you explicitly but we have a bunch of pipelines that are now unable to deploy because of this bug. Wondering if you have any ideas how we can work around it?

@caarlos0 Sorry! I wasn’t very clear. I meant the latest Helm 3 version (which is the dev-v3 branch). I am making sure we don’t re-break people when Helm 3 comes out

@thomastaylor312 Thanks a lot for your quick response!

Also big kudos to @karuppiah7890 for sharing the detailed research on current code-base and what was going on under the hood. It took only a little time thanks to your awesome work 🙏

@caarlos0 @benjamin-hg I have found what’s causing the issue and how it works in kubectl. In kubectl code, MergePatchType patch happens based on the IsNotRegistered error, but in helm StrategicMergePatchType patch happens and it errors out when it’s not able to create a patch. This is the code in helm:

https://github.com/helm/helm/blob/0d81d92a01c20a908d045c8067d04e3bb10be443/pkg/kube/client.go#L595-L614

here, the versionedObject is present without errors, and also, another thing I noted here is that the IsNotRegistered error and the err != nil in the switch cases never execute, as the error variables they refer to are json marshal errors which are already caught if there are any, at the time of serializing. To make things similar to kubectl code, I think asVersioned should return an error result too, and it should return “Not Registered Error” for CutomResourceDefinition resource, as currently it doesn’t.

And for me, the “Not Registered Error” for CRD feels weird in kubectl code, I need to read more about why the error occurs even though CRD is present in the APIs under apiextensions.k8s.io/v1beta1.

We just merged @mumoshu’s fix and will be cutting 2.14.3 with this release soon. Thanks to everyone who helped dig into the issue

I didn’t get the time to check more on this 🙈 but one good point that Carlos gave was that it works with kubectl apply. I was checking how it works there but not here. Still checking it, looks like the only current workaround is to do kubectl apply manually. Will try to work on this and post what can be done to mitigate the issue!

Sure @caarlos0 👍 I’ll debug it and let you know. For now looks like the code flow is right. The comment about CRDs and StrategicMergePatchType - looks like what they meant in comments was about resources of type xyz which is a CRD, but here we are trying to upgrade a resource of type CustomResourceDefinition which is a known object and has structure which is clear from the code as isUnstructured is false and on adding more logs, it also shows it’s type. Something’s going wrong with the merging alone. Still figuring it out with logs and data and code. Will keep this thread updated when I find something!

hey! thanks for looking into this!

here is the CRD:

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  creationTimestamp: null
  labels:
    controller-tools.k8s.io: "1.0"
  name: builds.apps.example.com
spec:
  additionalPrinterColumns:
  - JSONPath: .status.status
    name: Status
    type: string
  - JSONPath: .status.start_time
    name: Started
    type: string
  - JSONPath: .status.completion_time
    name: Completed
    type: string
  group: apps.example.com
  names:
    kind: Build
    plural: builds
  scope: Namespaced
  subresources:
    status: {}
  validation:
    openAPIV3Schema:
      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:
          properties:
            args:
              description: Docker build args in the form of a list of key=value elements
              items:
                type: string
              type: array
            cache:
              description: Wether to cache build image layers
              type: boolean
            cpu:
              description: How much CPU is required
              type: string
            cwd:
              description: CWD is the working directory to be used inside the source.
              type: string
            dockerfile:
              description: Dockerfile path inside cwd
              type: string
            env:
              description: Environment variables
              type: object
            memory:
              description: How much memory is required
              type: string
            source:
              description: Source spec
              properties:
                branch:
                  description: If the type is git, the branch or tag to clone
                  type: string
                path:
                  description: Path to the source, either a git repo or the pvc name.
                  type: string
                type:
                  description: Type of the source, either pvc or git
                  enum:
                  - git
                  - pvc
                  type: string
              required:
              - type
              - path
              type: object
            target:
              description: Target spec
              properties:
                image:
                  description: Image that should be generated
                  type: string
              required:
              - image
              type: object
          required:
          - source
          - target
          - cwd
          - dockerfile
          type: object
        status:
          properties:
            status:
              type: string
          required:
          - status
          type: object
  version: v1beta1
status:
  acceptedNames:
    kind: ""
    plural: ""
  conditions: []
  storedVersions: []

The field added is the spec.cache.

Let me know if you need any more info.

PS: this CRD was generated by kubebuilder, if it helps with anything…