Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

can't upgrade charts with crd changes #5853

Closed
caarlos0 opened this issue Jun 6, 2019 · 22 comments

Comments

@caarlos0
Copy link

commented Jun 6, 2019

I have a custom chart for an operator we're writing.

The operator is under development, so it receives several changes... now, we needed to add a new non-required boolean field to the CRD.

When trying to upgrade it, I got this error:

UPGRADE FAILED
Error: failed to create patch: merging an object in json but data type is not struct, instead is: map
Error: UPGRADE FAILED: failed to create patch: merging an object in json but data type is not struct, instead is: map

The only things changed are the new field on the CRD and the image tag.

If I remove the new field from the CRD and upgrade it works.

What can I do to avoid this?


Output of helm version:

Client: &version.Version{SemVer:"v2.14.0", GitCommit:"05811b84a3f93603dd6c2fcfe57944dfa7ab7fd0", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.14.0", GitCommit:"05811b84a3f93603dd6c2fcfe57944dfa7ab7fd0", GitTreeState:"clean"}

Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"14", GitVersion:"v1.14.2", GitCommit:"66049e3b21efe110454d67df4fa62b08ea79a19b", GitTreeState:"clean", BuildDate:"2019-05-16T18:55:03Z", GoVersion:"go1.12.5", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"13+", GitVersion:"v1.13.6-gke.6", GitCommit:"fcbc1d20b6bca1936c0317743055ac75aef608ce", GitTreeState:"clean", BuildDate:"2019-05-23T11:42:40Z", GoVersion:"go1.11.5b4", Compiler:"gc", Platform:"linux/amd64"}

Cloud Provider/Platform (AKS, GKE, Minikube etc.):

GKE

@caarlos0

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

I tried to update the crd with kubectl apply, still helm fails...

@karuppiah7890

This comment has been minimized.

Copy link

commented Jun 8, 2019

@caarlos0 I tried debugging this, looks like more information is needed to debug why exactly this happened. Information about your CRD and the new field that you mentioned.

Let me put here what I found/guessed based on the details in hand - we know it's a CRD and we know the error message :

This is the place where the error occurs:

helm/pkg/kube/client.go

Lines 616 to 620 in 0d81d92

func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, force bool, recreate bool) error {
patch, patchType, err := createPatch(target, currentObj)
if err != nil {
return fmt.Errorf("failed to create patch: %s", err)
}

Getting into createPatch you can see this part:

helm/pkg/kube/client.go

Lines 597 to 613 in 0d81d92

// Unstructured objects, such as CRDs, may not have an not registered error
// returned from ConvertToVersion. Anything that's unstructured should
// use the jsonpatch.CreateMergePatch. Strategic Merge Patch is not supported
// on objects like CRDs.
_, isUnstructured := versionedObject.(runtime.Unstructured)
switch {
case runtime.IsNotRegisteredError(err), isUnstructured:
// fall back to generic JSON merge patch
patch, err := jsonpatch.CreateMergePatch(oldData, newData)
return patch, types.MergePatchType, err
case err != nil:
return nil, types.StrategicMergePatchType, fmt.Errorf("failed to get versionedObject: %s", err)
default:
patch, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, versionedObject)
return patch, types.StrategicMergePatchType, err
}

Reading the comment about CRDs, it's clear that jsonpatch.CreateMergePatch should be used for CRDs and that Strategic Merge Patch is not supported, but looking at the jsonpatch.CreateMergePatch function and the switch cases and all the digging, the code that executes is actually this:

helm/pkg/kube/client.go

Lines 610 to 612 in 0d81d92

default:
patch, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, versionedObject)
return patch, types.StrategicMergePatchType, err

And from there the flow goes like this (to external modules):
https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L94

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L103

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L119

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L139

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L143

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L168

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L212

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L261

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/patch.go#L263

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/meta.go#L73

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/pkg/util/strategicpatch/meta.go#L74

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/third_party/forked/golang/json/fields.go#L29

https://github.com/kubernetes/apimachinery/blob/6a84e37a896db9780c75367af8d2ed2bb944022e/third_party/forked/golang/json/fields.go#L36

So, that's my guess. But if the guess is right, then it all starts from the switch case which says it's structured and has no errors and goes to the default which leads to StrategicMergePatchType when in fact it should only do JSON merge patch for CRDs

@karuppiah7890

This comment has been minimized.

Copy link

commented Jun 8, 2019

I think it will be possible to debug more if we see the CRD and it's structure and the field you mentioned. But I can't say for sure though

@caarlos0

This comment has been minimized.

Copy link
Author

commented Jun 8, 2019

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...

@karuppiah7890

This comment has been minimized.

Copy link

commented Jun 9, 2019

Okay, I am able to reproduce the issue by installing this chart - issue-5853-chart.zip and then upgrading with this chart - issue-5853-new-chart.zip

$ helm version
Client: &version.Version{SemVer:"v2.14.0", GitCommit:"05811b84a3f93603dd6c2fcfe57944dfa7ab7fd0", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.14.0", GitCommit:"05811b84a3f93603dd6c2fcfe57944dfa7ab7fd0", GitTreeState:"clean"}

And to confirm my guess (the flow of code execution), I later added some logs to the guessed code flow, to see the functions that were called and deployed the modified tiller and saw that my guess was correct -

[tiller] 2019/06/09 05:58:44 getting history for release issue-5853
[storage] 2019/06/09 05:58:44 getting release history for "issue-5853"
[tiller] 2019/06/09 05:58:44 preparing update for issue-5853
[storage] 2019/06/09 05:58:44 getting deployed releases from "issue-5853" history
[storage] 2019/06/09 05:58:44 getting last revision of "issue-5853"
[storage] 2019/06/09 05:58:44 getting release history for "issue-5853"
[tiller] 2019/06/09 05:58:44 rendering issue-5853-chart chart using values
[tiller] 2019/06/09 05:58:44 creating updated release for issue-5853
[storage] 2019/06/09 05:58:44 creating release "issue-5853.v8"
[tiller] 2019/06/09 05:58:44 performing update for issue-5853
[tiller] 2019/06/09 05:58:44 executing 0 pre-upgrade hooks for issue-5853
[tiller] 2019/06/09 05:58:44 hooks complete for pre-upgrade issue-5853
[kube] 2019/06/09 05:58:44 building resources from updated manifest
[kube] 2019/06/09 05:58:44 checking 1 resources for changes
isUnstructured? false
err: <nil>creating two way merge patch
CreateTwoWayMergePatchUsingLookupPatchMeta
CreateTwoWayMergeMapPatchUsingLookupPatchMeta
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
diffMaps
handleMapDiff
LookupPatchMetadataForStruct
LookupPatchMetadataForStruct
erroring out here! this causes issue 5853! hmmm ...
[kube] 2019/06/09 05:58:44 error updating the resource "builds.apps.example.com":
	 failed to create patch: merging an object in json but data type is not struct, instead is: map
[tiller] 2019/06/09 05:58:44 warning: Upgrade "issue-5853" failed: failed to create patch: merging an object in json but data type is not struct, instead is: map
[storage] 2019/06/09 05:58:44 updating release "issue-5853.v1"
[storage] 2019/06/09 05:58:44 updating release "issue-5853.v8"

Next gotta check why this happened

@caarlos0

This comment has been minimized.

Copy link
Author

commented Jun 9, 2019

awesome, thanks @karuppiah7890 .

let me know if I can help in any way

@karuppiah7890

This comment has been minimized.

Copy link

commented Jun 9, 2019

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!

@caarlos0

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

is there a way I can "force fix" this? just so my deploy scripts work again without me deploying manually 🙇‍♂

@benjamin-hg

This comment has been minimized.

Copy link

commented Jun 19, 2019

I'm getting the same error. Any updates on this?

@karuppiah7890

This comment has been minimized.

Copy link

commented Jun 19, 2019

@karuppiah7890

This comment has been minimized.

Copy link

commented Jun 23, 2019

@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:

helm/pkg/kube/client.go

Lines 595 to 614 in 0d81d92

versionedObject := asVersioned(target)
// Unstructured objects, such as CRDs, may not have an not registered error
// returned from ConvertToVersion. Anything that's unstructured should
// use the jsonpatch.CreateMergePatch. Strategic Merge Patch is not supported
// on objects like CRDs.
_, isUnstructured := versionedObject.(runtime.Unstructured)
switch {
case runtime.IsNotRegisteredError(err), isUnstructured:
// fall back to generic JSON merge patch
patch, err := jsonpatch.CreateMergePatch(oldData, newData)
return patch, types.MergePatchType, err
case err != nil:
return nil, types.StrategicMergePatchType, fmt.Errorf("failed to get versionedObject: %s", err)
default:
patch, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, versionedObject)
return patch, types.StrategicMergePatchType, err
}
}

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.

@Stono

This comment has been minimized.

Copy link

commented Jun 25, 2019

Also being hit by this!
Error: failed to create patch: merging an object in json but data type is not struct, instead is: map

We were changing a field on a CRD:

From:

        spec:
          properties:
            config:
              description: Additional configuration options
              type: object
            name:
              description: The name of the topic
              pattern: '^[A-Za-z0-9\-]+$'
              type: string

To:

              pattern: '^[a-zA-Z0-9\._\-]+$'

Are there any work arounds? We can't purge this chart as it'll delete the CRDs and subsequently the CR's so it's quite a mess. We can't edit the resource manually as helm is generating the merge based on the last applied structure rather than whats in the api server.

Helm 2.14.1 btw

@Stono

This comment has been minimized.

Copy link

commented Jun 26, 2019

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?

mumoshu added a commit to mumoshu/helm that referenced this issue Jul 26, 2019

fix: upgrade charts with CRD changes
Probably since K8s 1.13.x, `converter.ConvertToVersion(info.Object, groupVersioner)` which is the body of `asVersioned` doesn't return an error or an "unstructured" object, but `apiextensions/v1beta1.CustomResourceDefinition`.

The result was `helm upgrade` with any changes in CRD consistently failing.

This fixes that by adding an additional case of the conversion result being `v1beta1.CustomResourceDefinition`.

This is a backward-compatible change as it doesn't remove existing switch cases for older K8s versions.

Fixes helm#5853

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu mumoshu referenced this issue Jul 26, 2019
1 of 3 tasks complete

mumoshu added a commit to mumoshu/helm that referenced this issue Jul 26, 2019

fix: upgrade charts with CRD changes
Probably since K8s 1.13.x, `converter.ConvertToVersion(info.Object, groupVersioner)` which is the body of `asVersioned` doesn't return an error or an "unstructured" object, but `apiextensions/v1beta1.CustomResourceDefinition`.

The result was `helm upgrade` with any changes in CRD consistently failing.

This fixes that by adding an additional case of the conversion result being `v1beta1.CustomResourceDefinition`.

This is a backward-compatible change as it doesn't remove existing switch cases for older K8s versions.

Fixes helm#5853

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

At least on K8s 1.15.0 (and probably since K8s 1.13.x as @caarloso has encountered this on K8s 1.13.6), converter.ConvertToVersion(info.Object, groupVersioner) which is the body of asVersioned doesn't return an error or an "unstructured" object, but apiextensions/v1beta1.CustomResourceDefinition.

I've submitted #6092 that fixes the issue by handling that.

mumoshu added a commit to mumoshu/helm that referenced this issue Jul 26, 2019

fix: upgrade with CRD changes
Probably since K8s 1.13.x, `converter.ConvertToVersion(info.Object, groupVersioner)` which is the body of `asVersioned` doesn't return an error or an "unstructured" object, but `apiextensions/v1beta1.CustomResourceDefinition`.

The result was `helm upgrade` with any changes in CRD consistently failing.

This fixes that by adding an additional case of the conversion result being `v1beta1.CustomResourceDefinition`.

This is a backward-compatible change as it doesn't remove existing switch cases for older K8s versions.

Fixes helm#5853

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@thomastaylor312

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

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

@thomastaylor312 thomastaylor312 added this to the 2.14.3 milestone Jul 29, 2019

@mumoshu

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

@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

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

awesome, thanks everyone!

bacongobbler added a commit that referenced this issue Jul 30, 2019

fix: upgrade with CRD changes
Probably since K8s 1.13.x, `converter.ConvertToVersion(info.Object, groupVersioner)` which is the body of `asVersioned` doesn't return an error or an "unstructured" object, but `apiextensions/v1beta1.CustomResourceDefinition`.

The result was `helm upgrade` with any changes in CRD consistently failing.

This fixes that by adding an additional case of the conversion result being `v1beta1.CustomResourceDefinition`.

This is a backward-compatible change as it doesn't remove existing switch cases for older K8s versions.

Fixes #5853

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@thomastaylor312

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

Hey all! Could any of you who were having this issue see if it occurs with the latest dev-v3 code? I was working on porting @mumoshu's fix and I couldn't duplicate the bug now that Helm 3 uses a 3 way merge strategy. Just want to double check and make sure I am not missing anything!

@caarlos0

This comment has been minimized.

Copy link
Author

commented Aug 6, 2019

I just tested with the latest released version and it works fine... can't test master right now :(

@thomastaylor312

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

@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

@hickeyma

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@thomastaylor312 Is there a correlation between this issue and #4697?

@thomastaylor312

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

@hickeyma I don't think so. That is more about ordering

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.