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

.Capabilities.APIVersions returns wrong results #1014

Closed
a-hat opened this issue Dec 3, 2019 · 22 comments · Fixed by #1046
Closed

.Capabilities.APIVersions returns wrong results #1014

a-hat opened this issue Dec 3, 2019 · 22 comments · Fixed by #1046

Comments

@a-hat
Copy link
Contributor

a-hat commented Dec 3, 2019

The helm built-in object Capabilities.APIVersions returns incorrect results when running with helmfile in my cluster.

For demonstration purposes, I have the following test.yaml file, which prints the results from APIVersions using incubator/raw:

templates:
- |
  apiVersion: apps/v1
  kind: Deployment
  metadata:
    name: test
    labels:
      test: |
        {{ .Capabilities.APIVersions }}

Here is a helmfile.yaml using this template:

repositories:
- name: incubator
  url: https://kubernetes-charts-incubator.storage.googleapis.com
  
releases:
- name: test
  namespace: monitoring
  chart: incubator/raw
  version: 0.2.3
  values:
  - test.yaml

When I run helm install test incubator/raw -f test.yaml --dry-run I get the correct list of API Versions available in my cluster. helmfile template outputs a different set of Versions with many entries missing.

helmfile version v0.94.1
# kubectl version (AWS EKS)
Client Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.3", GitCommit:"b3cbbae08ec52a7fc73d334838e18d17e8512749", GitTreeState:"clean", BuildDate:"2019-11-13T11:23:11Z", GoVersion:"go1.12.12", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"14+", GitVersion:"v1.14.8-eks-b8860f", GitCommit:"b8860f6c40640897e52c143f1b9f011a503d6e46", GitTreeState:"clean", BuildDate:"2019-11-25T00:55:38Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"linux/amd64"}
# helm version
version.BuildInfo{Version:"v3.0.0", GitCommit:"e29ce2a54e96cd02ccfce88bee4f58bb6e2a28b6", GitTreeState:"clean", GoVersion:"go1.13.4"}
@a-hat
Copy link
Contributor Author

a-hat commented Dec 3, 2019

Upon further investigation, helm template seems to be the culprit.

The documentation of this command says:

Any values that would normally be looked up or retrieved in-cluster will be
faked locally. Additionally, none of the server-side testing of chart validity
(e.g. whether an API is supported) is done.

In my case, I was running helmfile apply, which (if I understand correctly) first executes helm diff to detect if there are any changes, which probably is based on helm template. In the chart I was installing, there is a check on the API Versions, and this check did not return the expected results, so no changes were detected by helmfile. If I run helmfile sync, the chart gets installed with the correctly rendered templates.

Any ideas how the user experience could be improved for this use case?

@a-hat
Copy link
Contributor Author

a-hat commented Dec 6, 2019

I just stumbled upon this again. My helm release is currently installed correctly. But when I run helmfile apply, helmfile says it will apply some changes, based on the incorrect output of helm diff. Those changes are actually not applied, because helm upgrade executes the Capabilities check correctly. This behaviour is really confusing.

@mumoshu
Copy link
Collaborator

mumoshu commented Dec 11, 2019

@a-hat Thanks for the detailed report. What would be the next action? As helm-diff depends on helm template, I believe this can be unavoidable.

A possible workaround would be to provide Capabilities from helmfile/helm-diff side, like, enhancing helmfile to accept capabilities in helmfile.yaml, and it passes that to helm-diff.

Does it make sense?

@a-hat
Copy link
Contributor Author

a-hat commented Dec 12, 2019

@mumoshu There is an issue in the helm repo about adding an option to provide the Capabilities to helm template, however this seems to be stale.

If it is possible to implement this feature in helmfile / helm-diff, this would be a good solution. Maybe it would be possible to add an option to retrieve the actual Capabilities and passing the result to helm template, this would be even better because it avoids having to maintain a list of Capabilities in helmfile.

@mumoshu
Copy link
Collaborator

mumoshu commented Dec 12, 2019

@a-hat Hey!

There is an issue in the helm repo about adding an option to provide the Capabilities to helm template, however this seems to be stale.

No, 3.0.1 does have the flag to set capabilities:

$ helm3 template --help | grep api
  -a, --api-versions stringArray   Kubernetes api versions used for Capabilities.APIVersions

retrieve the actual Capabilities and passing the result to helm template

Hmm, I think we shouldn't do that. Say we have two releases in helmfile.yaml, the first one having a CRD, the second one has Capabilities.APIVersions in the template. When we runhelmfile apply, it can firstly show diff based on the current state of the cluster. But the diff can be wrong as what's actually installed by the second release depends on the first release.

The only way to "correct" the diff in this case would be you define api-versions. Enhancing helmfile.yaml to accept capabilities.apiVersions would be sufficient for that.

@a-hat
Copy link
Contributor Author

a-hat commented Dec 12, 2019

Hi @mumoshu,

So the flag api-versions already exists, thanks for the pointer.

I agree that manually reading the Capabilities from the API server would lead to confusing behaviour in your use case. Then the solution you proposed seems to be the best option 👍

@pavdmyt
Copy link

pavdmyt commented Dec 17, 2019

Having the similar issue trying to install ServiceMonitor from fluent-bit chart using helmfile.

$ helm version 
version.BuildInfo{Version:"v3.0.1", GitCommit:"7c22ef9ce89e0ebeb7125ba2ebf7d421f3e82ffa", GitTreeState:"clean", GoVersion:"go1.13.4"}

$ helmfile -v
helmfile version v0.97.0

Per my understanding, the proposed solution is to support capabilities.apiVersions at helmfile side. Is there any plans/estimates on this feature?

@mumoshu
Copy link
Collaborator

mumoshu commented Dec 17, 2019

Yes, implementing capabiilities.apiVersion for helmfile.yaml would do the job.

No plan yet due to my personal reason but I'll definitely review anyone's PR once submitted!

a-hat added a commit to a-hat/helmfile that referenced this issue Dec 20, 2019
mumoshu pushed a commit that referenced this issue Dec 26, 2019
…1046)

This makes it possible to pass the API Capabilities to helmfile when executing a task that does not render against an actual cluster (diff, template, apply).

Resolves #1014
@travisghansen
Copy link

@a-hat is this only available if declared in the helmfile or can it be specified at the cli at runtime?

helmfile template -a apps/v1

Basically I'm exploring the idea of bridging helmfile with argo-cd. The same helmfile could be getting used on disperse clusters so at runtime I'd like to query the target cluster, that those values, and then pass the data to the helmfile template command dynamically.

In addition to the above, it seems like --no-hooks and --skip-crds may come in handy as well. Not sure if those are already supported or not.

@mumoshu
Copy link
Collaborator

mumoshu commented Jan 5, 2020

@travisghansen Hey! The option is available in your helmfile.yaml that can be used like this:

https://github.com/roboll/helmfile/pull/1046/files#diff-04c6e90faac2675aa89e2176d2eec7d8R278

There's no support for --no-hooks and --skip-crds today but they do worth dedicated feature requests. Probably enhancing helmfile template to accept those flags to be passed thru to helm would be enough.

@travisghansen
Copy link

@mumoshu yeah I'd need apiversions as pass through as well. Want me to create 3 separate issues?

@mumoshu
Copy link
Collaborator

mumoshu commented Jan 6, 2020

You could already use apiVersions in helmfile.yaml for passing through api versions to template. That said I'd be happy with just one issue about extending helmfile template for GitOps. In it you would mention about both no-hooks and skip-crds.

@travisghansen
Copy link

@mumoshu putting that data in helmfile is less than ideal (for me anyway). I intend to use the same helmfile across disperse clusters which may or may not be at the same k8s version.

@mumoshu
Copy link
Collaborator

mumoshu commented Jan 6, 2020

@travisghansen Thanks for clarifying - that makes sense.

To be clear, I'd still recommend specifying it from within helmfile.yaml so that your helmfile.yaml can be the source-of-truth. You'd use helmfile's "environment values" feature or helmfile.yaml template to set different apiVersions per environment.

@travisghansen
Copy link

That's fair. In the use-case I'm currently looking at (integration with argo-cd) the number of clusters could be extremely large (fleet management type stuff). Maintaining that manually in the file directly would not be really feasible at the scale involved. I'm admittedly not using advanced features of helmfile yet however so I could be misunderstanding something. The template env var may be a feasible route especially if it can be made conditional (I think it can). In any case, pass through seems the ideal route for my crazy use case ATM.

Thanks for the consideration and help!

@a-hat
Copy link
Contributor Author

a-hat commented Jan 7, 2020

Hey @mumoshu!

Just in case you missed it, the apiVersions configuration option in helmfile is unfortunately broken, see my comments here: #1046.

I am waiting for databus23/helm-diff#175 to be merged, which will fix it.

@travisghansen
Copy link

Reading through the repo issues over there it appears to have 2 or 3 issues related to helm3 and/or API versions. I'll be keeping an eye on all this for sure.

@carlosrmendes
Copy link

how to fix this now, since apiVersion is not being passed to helm diff? I'm always getting some apiVersions diffs that are not true after each helmfile apply.

@mumoshu
Copy link
Collaborator

mumoshu commented Jan 22, 2022

I thought I've recently introduced an option for helm-diff to let it use helm upgrade --dry-run instead of helm template to render the manifests to be diffed. Maybe it would let helm-diff retrieve the real API versions from the target cluster, so that this becomes no issue.

@raxod502-plaid
Copy link
Contributor

@mumoshu Can you elaborate? I see that when running helm diff upgrade that helm diff upgrade does require network connectivity to the cluster specified in --kube-context, but it apparently does not populate .Capabilities.KubeVersion from this cluster (cluster version is v1.17.17, but .Capabilities.KubeVersion expands to v1.20.0 using Helm 3.5.3, Helmfile 0.139.5, helm-diff 3.1.3).

The full command is generated by Helmfile, along with the temporary files it references:

helm --kube-context k8s-o11y.util.ue1.plaid.io diff upgrade --reset-values --allow-unreleased prometheus-operator ../../../../files/code/prometheus-charts/charts/kube-prometheus-stack --version 23.3.2 --disable-validation --kube-context k8s-o11y.util.ue1.plaid.io --namespace monitoring --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile753538446/monitoring-prometheus-operator-values-76b5c547c7 --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile360978325/monitoring-prometheus-operator-values-755c54d77b --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile960049648/monitoring-prometheus-operator-values-858846cd9d --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile841419919/monitoring-prometheus-operator-values-5d657b8458 --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile813189794/monitoring-prometheus-operator-values-77c5496cc --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile467059097/monitoring-prometheus-operator-values-766996969b --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile361589796/monitoring-prometheus-operator-values-5dcdddfc45 --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile401100083/monitoring-prometheus-operator-values-6975794955 --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile348298486/monitoring-prometheus-operator-values-7c47b44bd8 --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile059864029/monitoring-prometheus-operator-values-6784c5699c --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile945508248/monitoring-prometheus-operator-values-5856569548 --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile458297879/monitoring-prometheus-operator-values-6fd9b87c5c --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile969634954/monitoring-prometheus-operator-values-556b4d778d --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile019384929/monitoring-prometheus-operator-values-6494676977 --values /var/folders/bh/2r569qtj36d_84p5x1dw19g5f0b0ms/T/helmfile292554316/monitoring-prometheus-operator-values-647897bcb8 --set-file 'alertmanager.templateFiles.kubernetes\.tmpl=../resources/kube-prometheus-stack/alertmanager/templates/kubernetes.tmpl' --set-file 'alertmanager.templateFiles.plaid\.tmpl=../resources/kube-prometheus-stack/alertmanager/templates/plaid.tmpl'

Is there some additional flag that needs to get pass to helm-diff to make it use the correct values? And if so is there a way to make helmfile pass that flag? databus23/helm-diff#175 (comment) by @a-hat mentions that helm diff works "correctly" with .Capabilities.APIVersions as of Helm 3.1.0 (we are on Helm 3.5.3), but that does not appear to be the case (or I misunderstand what the expected behavior is).

FWIW, here is what .Capabilities.APIVersions expands to for our cluster according to helm-diff:

[v1 admissionregistration.k8s.io/v1 admissionregistration.k8s.io/v1beta1 internal.apiserver.k8s.io/v1alpha1 apps/v1 apps/v1beta1 apps/v1beta2 authentication.k8s.io/v1 authentication.k8s.io/v1beta1 authorization.k8s.io/v1 authorization.k8s.io/v1beta1 autoscaling/v1 autoscaling/v2beta1 autoscaling/v2beta2 batch/v1 batch/v1beta1 batch/v2alpha1 certificates.k8s.io/v1 certificates.k8s.io/v1beta1 coordination.k8s.io/v1beta1 coordination.k8s.io/v1 discovery.k8s.io/v1alpha1 discovery.k8s.io/v1beta1 events.k8s.io/v1 events.k8s.io/v1beta1 extensions/v1beta1 flowcontrol.apiserver.k8s.io/v1alpha1 flowcontrol.apiserver.k8s.io/v1beta1 networking.k8s.io/v1 networking.k8s.io/v1beta1 node.k8s.io/v1 node.k8s.io/v1alpha1 node.k8s.io/v1beta1 policy/v1beta1 rbac.authorization.k8s.io/v1 rbac.authorization.k8s.io/v1beta1 rbac.authorization.k8s.io/v1alpha1 scheduling.k8s.io/v1alpha1 scheduling.k8s.io/v1beta1 scheduling.k8s.io/v1 storage.k8s.io/v1beta1 storage.k8s.io/v1 storage.k8s.io/v1alpha1 apiextensions.k8s.io/v1beta1 apiextensions.k8s.io/v1]

You mentioned adding something "recently" but I checked the release history and didn't see anything that seemed relevant. I upgraded from helm-diff 3.1.3 to 3.4.1 but no change in observed behavior.

@mumoshu
Copy link
Collaborator

mumoshu commented Jan 24, 2022

@raxod502-plaid Upgrade helm-diff to a very recent version and run helmfile like HELM_DIFF_USE_UPGRADE_DRY_RUN =true helmfile apply. Please also see databus23/helm-diff#253 for the rationale behind this feature.

I don't remember if it populated KubeVersion. AFAIK it has been deprecated in Helm 3 and only recently added back to Helm 3 in Helm 3.7.0(I'm unsure) as helm template --kube-version. So HELM_DIFF_USE_UPGRADE_DRY_RUN doesn't work, you can either update your chart to not use the deprecate KubeVersion and use ApiVersions instead, OR enhance helm-diff(without HELM_DIFF_USE_UPGRADE_DRY_RUN option) and helmfile to correctly pass the added back --kube-version flag down to helm template which is called by helm-diff.

@raxod502-plaid
Copy link
Contributor

It turned out that the issue was actually our fault; another team member had set disableValidation: true in some releases in our helmfiles, but I was unaware of that. After replacing that configuration with the disableValidationOnInstall: true parameter you introduced in #1600 (comment), the incorrect rendering went away. Even with using KubeVersion and without using HELM_DIFF_USE_UPGRADE_DRY_RUN. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants