argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
17.65k stars 5.38k forks source link

Helm template appends api/versions to compiled in api/versions instead of replacing #7291

Open sathieu opened 3 years ago

sathieu commented 3 years ago

Describe the bug

There are several bugs about .Capabilities.APIVersions.

Here is my understanding of the current behavior, with identified filled issues:

To Reproduce

With a given chart, repo-server will log:

time="2021-09-08T07:55:59Z"
level=info
msg=Trace
args="[helm template . --name-template gatekeeper --namespace gatekeeper --kube-version 1.20 --values values.yaml --api-versions acme.cert-manager.io/v1 --api-versions acme.cert-manager.io/v1alpha2 --api-versions acme.cert-manager.io/v1alpha3 --api-versions acme.cert-manager.io/v1beta1 --api-versions admissionregistration.k8s.io/v1 --api-versions admissionregistration.k8s.io/v1beta1 --api-versions apiextensions.k8s.io/v1 --api-versions apiextensions.k8s.io/v1beta1 --api-versions apiregistration.k8s.io/v1 --api-versions apiregistration.k8s.io/v1beta1 --api-versions apps/v1 --api-versions argoproj.io/v1alpha1 --api-versions authentication.k8s.io/v1 --api-versions authentication.k8s.io/v1beta1 --api-versions authorization.k8s.io/v1 --api-versions authorization.k8s.io/v1beta1 --api-versions autoscaling/v1 --api-versions autoscaling/v2beta1 --api-versions autoscaling/v2beta2 --api-versions batch/v1 --api-versions batch/v1beta1 --api-versions bitnami.com/v1alpha1 --api-versions ceph.rook.io/v1 --api-versions cert-manager.io/v1 --api-versions cert-manager.io/v1alpha2 --api-versions cert-manager.io/v1alpha3 --api-versions cert-manager.io/v1beta1 --api-versions certificates.k8s.io/v1 --api-versions certificates.k8s.io/v1beta1 --api-versions config.gatekeeper.sh/v1alpha1 --api-versions coordination.k8s.io/v1 --api-versions coordination.k8s.io/v1beta1 --api-versions crd.projectcalico.org/v1 --api-versions discovery.k8s.io/v1beta1 --api-versions events.k8s.io/v1 --api-versions events.k8s.io/v1beta1 --api-versions extensions/v1beta1 --api-versions flowcontrol.apiserver.k8s.io/v1beta1 --api-versions install.istio.io/v1alpha1 --api-versions kubitus-project.gitlab.io/v1alpha1 --api-versions metacontroller.k8s.io/v1alpha1 --api-versions metrics.k8s.io/v1beta1 --api-versions monitoring.coreos.com/v1 --api-versions monitoring.coreos.com/v1alpha1 --api-versions mutations.gatekeeper.sh/v1alpha1 --api-versions networking.istio.io/v1alpha3 --api-versions networking.istio.io/v1beta1 --api-versions networking.k8s.io/v1 --api-versions networking.k8s.io/v1beta1 --api-versions node.k8s.io/v1 --api-versions node.k8s.io/v1beta1 --api-versions objectbucket.io/v1alpha1 --api-versions operators.coreos.com/v1 --api-versions operators.coreos.com/v1alpha1 --api-versions operators.coreos.com/v1alpha2 --api-versions operators.coreos.com/v2 --api-versions packages.operators.coreos.com/v1 --api-versions policy/v1beta1 --api-versions rbac.authorization.k8s.io/v1 --api-versions rbac.authorization.k8s.io/v1beta1 --api-versions replication.storage.openshift.io/v1alpha1 --api-versions rook.io/v1alpha2 --api-versions scheduling.k8s.io/v1 --api-versions scheduling.k8s.io/v1beta1 --api-versions security.istio.io/v1beta1 --api-versions status.gatekeeper.sh/v1beta1 --api-versions storage.k8s.io/v1 --api-versions storage.k8s.io/v1beta1 --api-versions telemetry.istio.io/v1alpha1 --api-versions templates.gatekeeper.sh/v1 --api-versions templates.gatekeeper.sh/v1alpha1 --api-versions templates.gatekeeper.sh/v1beta1 --api-versions v1 --include-crds]"
dir=/tmp/https___gitlab.example.org_gitops_infra/gatekeeper
operation_name="exec helm"
time_ms=265.434214

ArgoCD is correctly passing k8s version and api/versions, (but not api/version/kinds - problem 2), however while only policy/v1beta1 is passed, policy/v1 will also be in .Capabilities.APIVersions, because Helm was compiled with k8s 1.21. As such, a template will have the following evaluated incorrectly:

{{- if .Capabilities.APIVersions.Has "policy/v1" }}
apiVersion: policy/v1
{{ else }}
apiVersion: policy/v1beta1
{{ end -}}

As a workaround, and thanks to problem 4, changing the condition will evaluate correctly:

{{- if .Capabilities.APIVersions.Has "policy/v1/PodDisruptionBudget" }}
apiVersion: policy/v1
{{ else }}
apiVersion: policy/v1beta1
{{ end -}}

Expected behavior

.Capabilities.APIVersions to not be prefilled with values depending on the k8s version used during the Helm build.

Some cooperation is needed between ArgoCD devs and Helm devs about this, we can work on the implementation.

Proposals:

  1. Add --strict-api-versions option to Helm (https://github.com/helm/helm/pull/10108)
  2. Only add StrictAPIVersions in the struct, and change ArgoCD to build against Helm (see here)
  3. Hardcode api/version/kind for each k8s version (rejected by Helm maintainer)
  4. Allow helm to access k8s API to fill .Capabilities.APIVersions.
  5. ... other ideas?

Version

argocd: v2.1.2+7af9dfb
  BuildDate: 2021-09-02T18:05:23Z
  GitCommit: 7af9dfb3524c13e941ab604e36e49a617fe47d2e
  GitTreeState: clean
  GoVersion: go1.16.5
  Compiler: gc
  Platform: linux/amd64
jannfis commented 3 years ago

@rbreeze Are you sure about cherry-picking the change of this behavior into 2.1? Frankly, I think this is not a bug, but rather an enhancement to existing functionality. I agree with the v2.2 milestone, but I'd oppose cherry-picking a behavioral change like a possible fix for this into 2.1.

alexmt commented 2 years ago

I confused @rbreeze and asked to add cherry-pick label. Removing it

alexmt commented 2 years ago

Thanks a lot for the investigation and preparing the summary @sathieu ! It helps a lot.

I think problem 1 is solved already. The https://github.com/argoproj/argo-cd/issues/3594 was reported for v1.5.4 which indeed did not have support for --api-versions

We can fix problems 2 and 4 in v2.2! Working on pull request.

sathieu commented 2 years ago

Thanks @alexmt. Looking forward for those fixes!

pjaak commented 2 years ago

We are facing the same issues at the moment when we try to update our ingress API versions. Looking forward to the changes aswell @alexmt

gdsoumya commented 2 years ago

@alexmt @sathieu can you confirm if this is what is expected for issue #3594 - PR #8371

sathieu commented 2 years ago

@gdsoumya Don't know. I'm more interested in problem 3.