adopted resource not replaced when using `upgrade --install --force`

[ngraef]

Setup

Given a cluster with an existing resource not installed by Helm and a chart that contains that resource so Helm can take over release management,

1. Following the process added in Adopt resources into release with correct instance and managed-by labels #7649, add label and annotations to the existing resource that was not installed by Helm.
2. Run helm upgrade --install --force <release> <chart>

Expected behavior

Existing resource is adopted into the release (because of the label/annotations) and replaced (because of --force).

Observed behavior

Existing resource is adopted into the release and updated with the default patch strategy.

The install process explicitly sets the force parameter to false when adopting resources:

helm/pkg/action/install.go

Line 375 in 21e694e

if _, err := i.cfg.KubeClient.Update(toBeAdopted, resources, false); err != nil {

Environment

Output of helm version:

version.BuildInfo{Version:"v3.10.2", GitCommit:"50f003e5ee8704ec937a756c646870227d7c8b58", GitTreeState:"clean", GoVersion:"go1.19.3"}

Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"24", GitVersion:"v1.24.4", GitCommit:"95ee5ab382d64cfe6c28967f36b53970b8374491", GitTreeState:"clean", BuildDate:"2022-08-17T18:46:11Z", GoVersion:"go1.19", Compiler:"gc", Platform:"darwin/arm64"}
Kustomize Version: v4.5.4
Server Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.13+k3s1", GitCommit:"66263ac7f0c2c3579f58247718db7df61af92050", GitTreeState:"clean", BuildDate:"2022-10-25T19:05:03Z", GoVersion:"go1.17.13", Compiler:"gc", Platform:"linux/arm64"}

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

---

Related: #9958

[joejulian]

Note: Anybody that picks up this issue will have to ensure that the solution will not change the existing behavior by default, so it'll have to be guarded by some additional flag. The default behavior can then be changed to the new path with the helm 4 release.

[cotej]

@ngraef, do you recall what type of resource you were attempting to adopt + modify when you filed this?

I've recently run into the same or a similar problem, but as far as I can tell this only poses a problem when trying to modify custom or "unstructured" resources, and the scenario you outlined actually works correctly on a normal resource.

Here's an example to demonstrate: adopt-secret.sh.txt
And here's the result:

$ ./adopt-secret.sh 
secret/my-secret created
secret/my-secret patched

--- Before Helm chart install ---

    my-annotation = abc
    my-new-annotation = 

  Release "secret-adoption" does not exist. Installing it now.
NAME: secret-adoption
LAST DEPLOYED: Thu Dec 15 13:08:40 2022
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None

--- After Helm chart install ---

    my-annotation = def
    my-new-annotation = hello

Both without and with --force the chart is able to install, adopt the secret, and modify it as expected.

As far as I can tell the problem is actually with patching - there is a branch of logic that handles patching of "unstructured" resources as a special case:

helm/pkg/kube/client.go

Line 471 in 21e694e

if isUnstructured || isCRD {

I built helm with some added debug output, and attempted a same experiment as attached in my script but using a custom resource as the target to adopt + modify. I'll attach this one as well for good measure: adopt-cr.sh.txt

Here's what I see:

$ ./adopt-cr.sh 
customresourcedefinition.apiextensions.k8s.io/crontabs.stable.example.com unchanged
crontab.stable.example.com/my-new-cron-object created
crontab.stable.example.com/my-new-cron-object patched

--- Before Helm chart install ---

    cronSpec = * * * * */5
    my-custom-annotation = 

  Release "crd-adoption-test" does not exist. Installing it now.
[updateResource] force = false
[createPatch] namespace = default, name = my-new-cron-object, isUnstructured = true, isCRD = false
[createPatch] oldData     = {"apiVersion":"stable.example.com/v1","kind":"CronTab","metadata":{"annotations":{"meta.helm.sh/release-name":"crd-adoption-test","meta.helm.sh/release-namespace":"default","my-custom-annotation":"asdf"},"labels":{"app.kubernetes.io/managed-by":"Helm"},"name":"my-new-cron-object","namespace":"default"},"spec":{"cronSpec":"* * * * */10","image":"my-awesome-cron-image"}}
[createPatch] newData     = {"apiVersion":"stable.example.com/v1","kind":"CronTab","metadata":{"annotations":{"meta.helm.sh/release-name":"crd-adoption-test","meta.helm.sh/release-namespace":"default","my-custom-annotation":"asdf"},"labels":{"app.kubernetes.io/managed-by":"Helm"},"name":"my-new-cron-object","namespace":"default"},"spec":{"cronSpec":"* * * * */10","image":"my-awesome-cron-image"}}
[createPatch] currentData = {"apiVersion":"stable.example.com/v1","kind":"CronTab","metadata":{"annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"stable.example.com/v1\",\"kind\":\"CronTab\",\"metadata\":{\"annotations\":{},\"name\":\"my-new-cron-object\",\"namespace\":\"default\"},\"spec\":{\"cronSpec\":\"* * * * */5\",\"image\":\"my-awesome-cron-image\"}}\n","meta.helm.sh/release-name":"crd-adoption-test","meta.helm.sh/release-namespace":"default"},"creationTimestamp":"2022-12-15T18:13:30Z","generation":1,"labels":{"app.kubernetes.io/managed-by":"Helm"},"managedFields":[{"apiVersion":"stable.example.com/v1","fieldsType":"FieldsV1","fieldsV1":{"f:metadata":{"f:annotations":{".":{},"f:kubectl.kubernetes.io/last-applied-configuration":{}}},"f:spec":{".":{},"f:cronSpec":{},"f:image":{}}},"manager":"kubectl-client-side-apply","operation":"Update","time":"2022-12-15T18:13:30Z"},{"apiVersion":"stable.example.com/v1","fieldsType":"FieldsV1","fieldsV1":{"f:metadata":{"f:annotations":{"f:meta.helm.sh/release-name":{},"f:meta.helm.sh/release-namespace":{}},"f:labels":{".":{},"f:app.kubernetes.io/managed-by":{}}}},"manager":"kubectl-patch","operation":"Update","time":"2022-12-15T18:13:30Z"}],"name":"my-new-cron-object","namespace":"default","resourceVersion":"334105","uid":"49423fa6-02b4-4cab-b242-21786ac3178c"},"spec":{"cronSpec":"* * * * */5","image":"my-awesome-cron-image"}}
Looks like there are no changes for CronTab "my-new-cron-object"
NAME: crd-adoption-test
LAST DEPLOYED: Thu Dec 15 13:13:30 2022
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None

--- After Helm chart install ---

    cronSpec = * * * * */5
    my-custom-annotation = 

Because isUnstructured = true for this resource, the createPatch function is calling jsonpatch.CreateMergePatch(oldData, newData). But we can see from the debug output that oldData and newData hold the same data! They both actually hold the desired state of the resource including the modifications - this seems wrong. Either way, since they are the same, it produces an empty patch and resource does not get modified at all.

Also worth noting that, as originally reported, if force was not hardcoded to false for adopting resources then it may have allowed for a workaround since it would bypass patching entirely, but I don't think force should be necessary here.

[ngraef]

@cotej I saw this issue with both a DaemonSet and a Deployment.

Here's are the files for a simple test:

# Existing resource in cluster
apiVersion: apps/v1
kind: Deployment
metadata:
  name: test-deployment
  namespace: default
  annotations:
    meta.helm.sh/release-name: helm-test
    meta.helm.sh/release-namespace: default
  labels:
    app.kubernetes.io/managed-by: Helm
spec:
  replicas: 1
  selector:
    matchLabels:
      app: test-deployment
  template:
    metadata:
      name: test-deployment
      labels:
        app: test-deployment
    spec:
      containers:
        - name: container-one
          image: postgres
          env:
            - name: POSTGRES_PASSWORD
              value: foo

# Template in Helm chart
apiVersion: apps/v1
kind: Deployment
metadata:
  name: test-deployment
  namespace: default
spec:
  replicas: 1
  selector:
    matchLabels:
      app: test-deployment
  template:
    metadata:
      name: test-deployment
      labels:
        app: test-deployment
    spec:
      containers:
        - name: one
          image: nginx
        - name: two
          image: redis

Running helm upgrade --install --force helm-test <chart> will result in a Deployment with three containers, which is the expected patch behavior when not using --force. (I would argue the patch merge is actually not the expected behavior for many users, but that's a different topic. It is the expected behavior as defined by Helm.) Running the command again does properly replace the resource.

[cotej]

@ngraef Thanks for the info. 👍 Sounds to me then like although our issues are slightly related, they are not duplicates.

[VaibhavSharma-47]

I have raised a PR, please review #11672. As per @joejulian comment do we want to put this behind a feature flag ? Can someone point me to an example where that is already done ?

[gjenkins8]

I've added this issue/PR to the weekly helm dev call notes.