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

Error constructing app status patch: unable to find api field in struct RawExtension for the json field \"autoscaling\" #15126

Closed
1 of 3 tasks
mikebryant opened this issue Aug 21, 2023 · 56 comments · Fixed by #18061, #18515 or #19340
Labels
bug Something isn't working

Comments

@mikebryant
Copy link
Contributor

Checklist:

  • I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
  • I've included steps to reproduce the bug.
  • I've pasted the output of argocd version.

Describe the bug

We've switched some applications to the new valuesObject syntax, and those apps have gotten "stuck". They go into a refreshing status and never finish

To Reproduce

Not clear to us how to reproduce. We deleted the entire .status of an affected application and it seemed to start working again. Not left it long enough yet to determine if it'll break again

Expected behavior

Screenshots

Version

argocd-server: v2.8.0+804d4b8
  BuildDate: 2023-08-07T14:25:33Z
  GitCommit: 804d4b8ca6bc4c2cf02c5c971aa923ec5b8623f0
  GitTreeState: clean
  GoVersion: go1.20.6
  Compiler: gc
  Platform: linux/amd64
  Kustomize Version: v5.1.0 2023-06-19T16:58:18Z
  Helm Version: v3.12.1+gf32a527
  Kubectl Version: v0.24.2
  Jsonnet Version: v0.20.0

Logs

{"application":"argocd/istio-gateway-iap","level":"error","msg":"Error constructing app status patch: unable to find api field in struct RawExtension for the json field \"service\"","time":"2023-08-21T09:01:10Z"}
{"application":"argocd/istio-gateway-public","level":"error","msg":"Error constructing app status patch: unable to find api field in struct RawExtension for the json field \"service\"","time":"2023-08-21T09:01:10Z"}
{"application":"argocd/istio-gateway-vpn","level":"error","msg":"Error constructing app status patch: unable to find api field in struct RawExtension for the json field \"service\"","time":"2023-08-21T09:01:25Z"}
{"application":"argocd/istio-gateway-public","level":"error","msg":"Error constructing app status patch: unable to find api field in struct RawExtension for the json field \"service\"","time":"2023-08-21T09:01:25Z"}
{"application":"argocd/istio-gateway-vpn","level":"error","msg":"Error constructing app status patch: unable to find api field in struct RawExtension for the json field \"service\"","time":"2023-08-21T09:01:39Z"}
{"application":"argocd/istio-gateway-public","level":"error","msg":"Error constructing app status patch: unable to find api field in struct RawExtension for the json field \"service\"","time":"2023-08-21T09:01:40Z"}
@mikebryant mikebryant added the bug Something isn't working label Aug 21, 2023
@mikebryant
Copy link
Contributor Author

Application yaml for one exhibiting the problem:
app.txt

@mikebryant
Copy link
Contributor Author

Found these issues that look relevant:

persistAppStatus uses CreateTwoWayMergePatch uses strategicpatch. Seems like this might not be happy with the unstructured content in valuesObject?

@vladfr
Copy link
Contributor

vladfr commented Aug 22, 2023

I've bumped into this today as well, on 2 applications installing Istio and Mimir. I don't yet see any pattern, it seems to be happening randomly on various fields. I've checked the Application resources, and the status.helm.valuesObject looks good. I've deleted it, and the controller populates it, but still it gives the patching error.

This is Argo 2.8.0.

{
    "Version": "v2.8.0+804d4b8",
    "BuildDate": "2023-08-07T14:25:33Z",
    "GitCommit": "804d4b8ca6bc4c2cf02c5c971aa923ec5b8623f0",
    "GitTreeState": "clean",
    "GoVersion": "go1.20.6",
    "Compiler": "gc",
    "Platform": "linux/amd64",
    "KustomizeVersion": "v5.1.0 2023-06-19T16:58:18Z",
    "HelmVersion": "v3.12.1+gf32a527",
    "KubectlVersion": "v0.24.2",
    "JsonnetVersion": "v0.20.0"
}

@vladfr
Copy link
Contributor

vladfr commented Aug 23, 2023

I managed to reproduce this consistently. This only happens if the status.sync.comparedTo contains an unquoted boolean. This is why the initial install works. But if you have an unquoted bool field, or you edit the Application and add an unquoted bool, then the Sync breaks.

Steps:

  1. Create an app with Helm source and valuesObject.
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: grafana
  namespace: argocd
  finalizers:
    - resources-finalizer.argocd.argoproj.io
spec:
  project: default
  source:
    repoURL: https://grafana.github.io/helm-charts
    targetRevision: 6.58.8
    chart: grafana

    helm:
      valuesObject:
        persistence:
          enabled: true

  destination:
    server: https://kubernetes.default.svc
    namespace: default

  syncPolicy:
    automated:
      prune: true
      selfHeal: true

⚠️ It is important not to quote the boolean value.

  1. After the app is installed and green, edit the file and disable persistence in Grafana:
    helm:
      valuesObject:
        persistence:
          enabled: false

⚠️ Again, it is important not to quote the boolean value.

  1. Apply the file

Expected: App syncs and refreshes correctly.

Actual Result: The app refresh breaks. Go to the UI and hit Refresh. The Refresh button will be greyed out. The error is visible in the controller logs.

I've managed to also reproduce this by running Argo locally.

Strategicmerge shouldn't be used for Unstructured data

This is an older issue also found in Helm, and fixed here. The fix is to use the json merge for Unstructured data, because it seems that the strategicmerge functionality isn't meant to be used on CRDs or other unstructured inputs. I'll work on a PR to address it this week.

Meanwhile, the workaround could be to quote all values, including numbers.

@crenshaw-dev
Copy link
Member

Brilliantly diagnosed. Looking forward to the PR, thanks @vladfr!

@vladfr
Copy link
Contributor

vladfr commented Aug 25, 2023

@crenshaw-dev added a PR, tests are passing, feedback is welcomed!

@Jojoooo1
Copy link

is it going to be available in 2.8.5 ? Would def help a lot to work with valuesObject!

@joggeli34
Copy link

We encounter the problem too. This prevents us from using the valuesObject, which would simplify our deployment-pipelines a lot

@HerrDerb
Copy link

HerrDerb commented Sep 18, 2023

@vladfr We also experience this problem. It would be great to be able to use valueObjects. Any idea when this will be merged?

@crenshaw-dev
Copy link
Member

tbh I'm fairly concerned about changing the patch calculation mechanism in a patch release since it changes how every single reconciliation works.

I wonder if there's an interim solution to avoid the problem for just this field, but leave the rest of the patch calculation untouched. Then we can switch over to the new mechanism entirely in 2.9.

@vladfr
Copy link
Contributor

vladfr commented Sep 18, 2023 via email

vojtechmares added a commit to vojtechmares-archives/gitops that referenced this issue Oct 2, 2023
amkartashov added a commit to amkartashov/gf-k8s that referenced this issue Oct 14, 2023
amkartashov added a commit to amkartashov/gf-k8s that referenced this issue Oct 14, 2023
amkartashov added a commit to amkartashov/gf-k8s that referenced this issue Oct 14, 2023
amkartashov added a commit to amkartashov/gf-k8s that referenced this issue Oct 14, 2023
@mgledi
Copy link

mgledi commented Oct 25, 2023

Is there any progress?

@Jojoooo1
Copy link

Jojoooo1 commented Nov 6, 2023

Any target release planned ?

@Jrc356
Copy link

Jrc356 commented Dec 1, 2023

currently running on 2.8.5 and seeing this. Also seems to lock up refreshing and messes with application health status. I see that #15227 hasn't been merged yet so I imagine this is still an issue in 2.9.X? Are there any plans on getting that merged?

@sitilge
Copy link

sitilge commented Dec 4, 2023

I'm running app version v2.9.3+6eba5be, still the same issue. Nearly every app is stuck with "Refresh".

@Zeratoxx
Copy link

Zeratoxx commented Dec 6, 2023

I suppose there will be patch versions for 2.8 and 2.9 ?

@hau21um
Copy link

hau21um commented Dec 11, 2023

Using application sets to generate applications, but even quoting in the applicationset template all the valuesObject values is not fix for us, as all the quotes disappear in generated Application manifest. So seems we have to stay with values: only multiline string.

@crenshaw-dev
Copy link
Member

I believe this PR needs to be modified to only calculate json patches (instead of strategic merge patches) for changes to the valuesObject field: #15227

Right now the PR changes the patch calculation logic for every single field, which seems likely excessive.

@diranged
Copy link

diranged commented Feb 2, 2024

We just discoverd this problem by accident .. I just happened to be tailing our logs. Is there any metric that ArgoCD reports when it has an error like this? I couldn't find anything in the argocd application controller metrics. :/

@neuber77
Copy link

neuber77 commented Feb 2, 2024

I hit this problem after upgrading ArgoCD server and the cli from v2.7.2 to v2.9.2.

It seems that, after v2.8.0, the cli generates the yaml with valuesObject instead of values: |. As a workaround, I've downgraded just the cli to v2.7.15 and the problem is solved.

@jetersen
Copy link
Contributor

jetersen commented Feb 3, 2024

Here is a simple search regex that should catch argo application and argo application set. You can use sed or your editor of choice. If the search hits unexpected files you can look at filtering of files or refining the regex.

search:

(\s+)?      valuesObject:

replace:

$1      values: |

Once the fix is out you simply do it in reverse (note the escaping of the pipe)
search:

(\s+)?      values: \|

replace:

$1      valuesObject:

@hshepherd
Copy link

Thank you for the issue and debug efforts! We are also hitting this issue in version 2.9.3 using gitops app-of-apps pattern and valuesObject.

@Kyle-Cooley
Copy link

We use the app of apps pattern with kustomization patches, so the values workaround was not applicable. Our workaround until the fix is merged: change the source helm chart to convert booleans and numbers from strings.

@PaulSonOfLars
Copy link
Contributor

We use the app of apps pattern with kustomization patches, so the values workaround was not applicable. Our workaround until the fix is merged: change the source helm chart to convert booleans and numbers from strings.

I'm confused - this issue should exclusively impact people who use argocd applications with a helm valuesObject field. Kustomize shouldn't be impacted, and changing the upstream helm chart should have nothing to do with it; the error happens at the application syncing level, not the helm level. Are you sure your issue is related to this one?
(Purely trying to I understand here, as I'm hoping to get a PR fix in for this)

@PaulSonOfLars
Copy link
Contributor

PaulSonOfLars commented Mar 29, 2024

On another note; some more debugging of this issue:

While looking into the the kubernetes documentation on RawExtension, which is the type used to define the ValuesObject field, I noticed that it mentions that the data MUST have an apiVersion and a kind field available. Which, of course, helm values files don't have...

So - the reason other operators don't have these issues might be something as simple as the fact that they don't use the RawExtension type.
Given the above, I think we're left with two ways forward:

  • Merging this "magic" JSON patching PR, which admittedly could cause additional maintenance load
  • Changing the type from RawExtension to something more appropriate; maybe simply a map[string]any? Of course, this then ends up as a compile-time breaking change for anyone importing argocd as a library. I could also see it causing some issues with protobufs.

Overall, I do think this requires a proper discussion with some ArgoCD maintainers - would love to hear some thoughts :)

@morfien101
Copy link

I'm pretty new the ArgoCD and was interested and mildly frustrated to find that my apps fail once and then seem to never change state. We also use the ValuesObject to pass config in to a helm chart for just about all our apps.

I'd be happy to help where I can with information.

It makes it rather annoying and pointless to see the state of an app as always red.
I also seem to have some apps (specifically postgresql databases using the cnpg operator) that seem to stay in "out of sync" even though they are fully synced and running.

@davidfrickert
Copy link

I'm pretty new the ArgoCD and was interested and mildly frustrated to find that my apps fail once and then seem to never change state. We also use the ValuesObject to pass config in to a helm chart for just about all our apps.

I'd be happy to help where I can with information.

It makes it rather annoying and pointless to see the state of an app as always red. I also seem to have some apps (specifically postgresql databases using the cnpg operator) that seem to stay in "out of sync" even though they are fully synced and running.

indeed, weird how this issue hasn't been tackled for this long. personally i switched from valuesObject to values: | and no issues since.

@gerchik
Copy link

gerchik commented Apr 2, 2024

valuesObject is still preferable way for my team due to heavy usage of kustomize patches which is more convenient with objects. The issue is still critical.

@Kyle-Cooley
Copy link

I'm confused - this issue should exclusively impact people who use argocd applications with a helm valuesObject field. Kustomize shouldn't be impacted, and changing the upstream helm chart should have nothing to do with it; the error happens at the application syncing level, not the helm level. Are you sure your issue is related to this one?
(Purely trying to I understand here, as I'm hoping to get a PR fix in for this)

Yes, we are using the valuesObject fields for helm applications. We use kustomize to patch subfields in valuesObject based on the environment. If we switched to values from valuesObject, then we would have to define a kustomization patch per application adding the environment specific variables to the base configuration for the helm chart. Given we will eventually have 70 or so applications like this in multiple environments, that was not an appealing option.

@garyiwu
Copy link

garyiwu commented Apr 2, 2024

As a workaround, we ended up putting in a ConfigManagementPlugin that does a string-replace of valuesObject: back into values: | before feeding the resulting YAML to Argo CD for interpretation.

@issmirnov
Copy link
Contributor

@garyiwu just got hit by this bug, can you share the example of your plugin? Our whole cluster is down, so everyone is scrambling to migrate.

@garyiwu
Copy link

garyiwu commented Apr 3, 2024

This works for our application layout. You might need to tailor to suit.

apiVersion: argoproj.io/v1alpha1
kind: ConfigManagementPlugin
metadata:
  name: valuesobjectworkaround
spec:
  init:
    command: ["/bin/true"]
  generate:
    command: ["/bin/bash", "-c"]
    args:
      - find -name "*.yaml" -exec cat {} \; -exec printf "\\n\\n---\\n\\n" \; | sed 's/valuesObject:/values:\ |/g'

@PaulSonOfLars
Copy link
Contributor

@Kyle-Cooley Aha, gotcha - so you use kustomize to deploy your applications, which then deploy helm charts with valuesObjects. And you need the valuesObjects for patches. All clear - thanks for clarifying!

@vladfr
Copy link
Contributor

vladfr commented Apr 3, 2024

indeed, weird how this issue hasn't been tackled for this long. personally i switched from valuesObject to values: | and no issues since.

I get your confusion too. Unfortunately, even if the fix for this issue is simple, the way it integrates with the rest of ArgoCD is not. I'm not aware of any other project that does such complex difffing.

I am still in favor of doing the fix. As shown by other people here, valuesObject really is the way forward, opening up a lot of nice things in managing Argo with GitOps. I hope that maintainers would also agree that it's worth the added complexity.

@issmirnov
Copy link
Contributor

Building on top of what @garyiwu shared, I've added the following two patch files to my kustomize build for argocd.

plugin-patch.yaml, required to inject the plugin container into the deployment

apiVersion: apps/v1
kind: Deployment
metadata:
  name: argocd-repo-server
spec:
  template:
    spec:
      containers:
      - name: my-plugin
        image: busybox
        command: ["/var/run/argocd/argocd-cmp-server"]
        securityContext:
          runAsNonRoot: true
          runAsUser: 999
        volumeMounts:
        - name: var-files
          mountPath: /var/run/argocd
        - name: plugins
          mountPath: /home/argocd/cmp-server/plugins
        - name: my-plugin-config
          mountPath: /home/argocd/cmp-server/config/plugin.yaml
          subPath: plugin.yaml
        - name: cmp-tmp
          mountPath: /tmp
      volumes:
      - name: my-plugin-config
        configMap:
          name: my-plugin-config
      - name: cmp-tmp
        emptyDir: {}

and the config-values-workaround.yaml required to actually add the rewrite rule. Note that the ConfigManagementPlugin may look like a k8s object, but it is not actually a CRD.

# from https://github.com/argoproj/argo-cd/issues/15126#issuecomment-2033317852
# used to patch values since there's some bug with argo.
# See also https://argo-cd.readthedocs.io/en/stable/operator-manual/config-management-plugins/#place-the-plugin-configuration-file-in-the-sidecar
apiVersion: v1
kind: ConfigMap
metadata:
  name: my-plugin-config
data:
  plugin.yaml: |
    apiVersion: argoproj.io/v1alpha1
    kind: ConfigManagementPlugin
    metadata:
      name: valuesobjectworkaround
    spec:
      init:
        command: ["/bin/true"]
      generate:
        command: ["/bin/bash", "-c"]
        args:
          - find -name "*.yaml" -exec cat {} \; -exec printf "\\n\\n---\\n\\n" \; | sed 's/valuesObject:/values:\ |/g'

Once those two files are added to the kustomization.yaml

...
resources:
...
- ./config-values-workaround.yaml
...
patches:
- path: plugin-patch.yaml
...

We can install argo as usual with kustomize build . | kubectl apply -f - -n argocd

@hebestreit
Copy link

I'm also looking forward to use valuesObject to patch multiple Helm applications with different environment settings using kustomize. After closely following this discussion I came up with a work around for the time being which can be easily removed once the bug is fixed.

Thanks a ton to @garyiwu and @issmirnov for sharing your fixes which gave me an idea. Your implementation will replace all occurrences of valuesObject: to values: | but unfortunately doesn't support the flexibility to patch the content.

So instead of using cat to print out the content of each manifest file we can run kustomize build and pass the output to sed. Argo CD won't notice that the initial property was valuesObject as the content will be transformed into a string after patching and uses the property values instead.

Once the bug is fixed you should be able to simply remove the plugin so that the valuesObject is also persisted in the Application resource.

# from https://github.com/argoproj/argo-cd/issues/15126#issuecomment-2033317852
# used to patch values since there's some bug with argo.
# See also https://argo-cd.readthedocs.io/en/stable/operator-manual/config-management-plugins/#place-the-plugin-configuration-file-in-the-sidecar
apiVersion: v1
kind: ConfigMap
metadata:
  name: kustomize-values-object-plugin
data:
  plugin.yaml: |
    apiVersion: argoproj.io/v1alpha1
    kind: ConfigManagementPlugin
    metadata:
      name: kustomize-values-object-plugin
    spec:
      generate:
        command: ["/bin/bash", "-c"]
        args:
          - kubectl kustomize | sed 's/valuesObject:/values:\ |/g'

Also notice that the image: bitnami/kubectl:1.29.3 needs to be changed to match your Kubernetes cluster version.

apiVersion: apps/v1
kind: Deployment
metadata:
  name: argocd-repo-server
spec:
  template:
    spec:
      containers:
        - name: kustomize-values-object-plugin
          image: bitnami/kubectl:1.29.3
          command: [ "/var/run/argocd/argocd-cmp-server" ]
          securityContext:
            runAsNonRoot: true
            runAsUser: 999
          volumeMounts:
            - name: var-files
              mountPath: /var/run/argocd
            - name: plugins
              mountPath: /home/argocd/cmp-server/plugins
            - name: kustomize-values-object-plugin
              mountPath: /home/argocd/cmp-server/config/plugin.yaml
              subPath: plugin.yaml
            - name: cmp-tmp
              mountPath: /tmp
      volumes:
        - name: kustomize-values-object-plugin
          configMap:
            name: kustomize-values-object-plugin
        - name: cmp-tmp
          emptyDir: { }

Lastly add the plugin to your Application resource or extend the above plugin with a discovery rule to automatically enable it based on your needs.

https://argo-cd.readthedocs.io/en/stable/operator-manual/config-management-plugins/#write-discovery-rules-for-your-plugin

apiVersion: argoproj.io/v1alpha1
kind: Application
spec:
  source:
    plugin:
      name: kustomize-values-object-plugin

@ro0NL
Copy link

ro0NL commented Apr 26, 2024

to just not use valuesObject

this is NOT a solution

both cases define the same YAML, and im amazed it leads to different results

valuesObject || yaml.parse(values) should produce the same set of values

@gecube
Copy link

gecube commented Apr 30, 2024

Good day Sirs,

it looks like a critical issue or design flaw in ArgoCD and show-stopper for it's usage. Are any ideas when it would be patched? I am very appreciate any feedback (ofk, for the exception of "bring your PRs")

@lexfrei
Copy link

lexfrei commented Apr 30, 2024

@gecube the PR is almost ready to merge #16602.
As hot fix you can replace your valuesObject: with values: |. It's not great, but works.

@robpearce-flux
Copy link

if you use values: | you cant patch it as part of a crossplane composition, i believe. That's how i ended up using valuesObject to begin with.

@alexmt
Copy link
Collaborator

alexmt commented Jun 5, 2024

Sorry, @PaulSonOfLars, I totally forgot to give you credit for the e2e tests. Fixing authorship in this PR: #18515

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment