Skip to content

Commit

Permalink
fix(argo-cd): remove secretName for server and applicationSet Certifi…
Browse files Browse the repository at this point in the history 
…cates (#2741)

* Remove Certificate's secretName because expected names by server and applicationset are static

Signed-off-by: Erwan Vallienne <[email protected]>

* Apply suggestions from code review

Signed-off-by: Marco Maurer (-Kilchhofer) <[email protected]>

* Fix lint

Signed-off-by: Erwan Vallienne <[email protected]>

---------

Signed-off-by: Erwan Vallienne <[email protected]>
Signed-off-by: Marco Maurer (-Kilchhofer) <[email protected]>
Signed-off-by: Erwan Vallienne <[email protected]>
Co-authored-by: Marco Maurer (-Kilchhofer) <[email protected]>
  • Loading branch information
@erwanval @mkilchhofer
erwanval and mkilchhofer authored Jun 20, 2024
1 parent e34b45b commit b0d4648
Show file tree
Hide file tree
Showing 5 changed files with 7 additions and 11 deletions.
8 changes: 5 additions & 3 deletions charts/argo-cd/Chart.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ appVersion: v2.11.3
kubeVersion: ">=1.23.0-0"
description: A Helm chart for Argo CD, a declarative, GitOps continuous delivery tool for Kubernetes.
name: argo-cd
version: 7.1.5
version: 7.2.0
home: https://github.com/argoproj/argo-helm
icon: https://argo-cd.readthedocs.io/en/stable/assets/logo.png
sources:
Expand All @@ -26,5 +26,7 @@ annotations:
fingerprint: 2B8F22F57260EFA67BE1C5824B11F800CD9D2252
url: https://argoproj.github.io/argo-helm/pgp_keys.asc
artifacthub.io/changes: |
- kind: added
description: Added secrettemplateAnnotation field for argocd server certificate
- kind: removed
description: Remove `server.certificate.secretName`, as the expected secret name is static (argocd-server-tls)
- kind: removed
description: Remove `applicationSet.certificate.secretName`, as the expected secret name is static (argocd-applicationset-controller-tls)
2 changes: 0 additions & 2 deletions charts/argo-cd/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,6 @@ NAME: my-release
| server.certificate.privateKey.rotationPolicy | string | `"Never"` | Rotation policy of private key when certificate is re-issued. Either: `Never` or `Always` |
| server.certificate.privateKey.size | int | `2048` | Key bit size of the private key. If algorithm is set to `Ed25519`, size is ignored. |
| server.certificate.renewBefore | string | `""` (defaults to 360h = 15d if not specified) | How long before the expiry a certificate should be renewed. |
| server.certificate.secretName | string | `"argocd-server-tls"` | The name of the Secret that will be automatically created and managed by this Certificate resource |
| server.certificate.secretTemplateAnnotations | object | `{}` | Annotations that allow the certificate to be composed from data residing in existing Kubernetes Resources |
| server.certificate.usages | list | `[]` | Usages for the certificate |
| server.certificateSecret.annotations | object | `{}` | Annotations to be added to argocd-server-tls secret |
Expand Down Expand Up @@ -1402,7 +1401,6 @@ If you use an External Redis (See Option 3 above), this Job is not deployed.
| applicationSet.certificate.privateKey.rotationPolicy | string | `"Never"` | Rotation policy of private key when certificate is re-issued. Either: `Never` or `Always` |
| applicationSet.certificate.privateKey.size | int | `2048` | Key bit size of the private key. If algorithm is set to `Ed25519`, size is ignored. |
| applicationSet.certificate.renewBefore | string | `""` (defaults to 360h = 15d if not specified) | How long before the expiry a certificate should be renewed. |
| applicationSet.certificate.secretName | string | `"argocd-applicationset-controller-tls"` | The name of the Secret that will be automatically created and managed by this Certificate resource |
| applicationSet.containerPorts.metrics | int | `8080` | Metrics container port |
| applicationSet.containerPorts.probe | int | `8081` | Probe container port |
| applicationSet.containerPorts.webhook | int | `7000` | Webhook container port |
Expand Down
2 changes: 1 addition & 1 deletion charts/argo-cd/templates/argocd-applicationset/certificate.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ metadata:
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.applicationSet.name "name" .Values.applicationSet.name) | nindent 4 }}
spec:
secretName: {{ .Values.applicationSet.certificate.secretName }}
secretName: argocd-applicationset-controller-tls
commonName: {{ .Values.applicationSet.certificate.domain | default .Values.global.domain }}
dnsNames:
- {{ .Values.applicationSet.certificate.domain | default .Values.global.domain }}
Expand Down
2 changes: 1 addition & 1 deletion charts/argo-cd/templates/argocd-server/certificate.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ spec:
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
secretName: {{ .Values.server.certificate.secretName }}
secretName: argocd-server-tls
commonName: {{ .Values.server.certificate.domain | default .Values.global.domain }}
dnsNames:
- {{ .Values.server.certificate.domain | default .Values.global.domain }}
Expand Down
4 changes: 0 additions & 4 deletions charts/argo-cd/values.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1990,8 +1990,6 @@ server:
certificate:
# -- Deploy a Certificate resource (requires cert-manager)
enabled: false
# -- The name of the Secret that will be automatically created and managed by this Certificate resource
secretName: argocd-server-tls
# -- Certificate primary domain (commonName)
# @default -- `""` (defaults to global.domain)
domain: ""
Expand Down Expand Up @@ -2998,8 +2996,6 @@ applicationSet:
certificate:
# -- Deploy a Certificate resource (requires cert-manager)
enabled: false
# -- The name of the Secret that will be automatically created and managed by this Certificate resource
secretName: argocd-applicationset-controller-tls
# -- Certificate primary domain (commonName)
# @default -- `""` (defaults to global.domain)
domain: ""
Expand Down

5 comments on commit b0d4648

@adamency
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erwanval @mkilchhofer Can you please explain this change:

Remove Certificate's secretName because expected names by server and applicationset are static

This is the first time in any open-source kubernetes application chart that I cannot have a custom secret name for my ingress rule. I don't know the internals of Argo CD, but the secret name for the Ingress rule should be entirely independent from the underlying service as it is only used by the Ingress Controller itself. I have always been able to use a custom name for ALL kubernetes third-party apps I have installed on my cluster, and this is a shocking first.

Please provide more information as to why this change was warranted. Isn't there an overlap (or confusion) between the TLS cert used by the server for communication with the other Argo CD components and the TLS cert simply used to authenticate the domain by the Ingress Controller to the public web clients ?

@erwanval
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamency This change doesn't concern the ingress TLS certificate, but the one exposed by ArgoCD pods.
According to the doc (and my tests following the issue I encountered initially):

Argo CD decides which TLS certificate to use for the endpoint of argocd-server as follows:
- If the argocd-server-tls secret exists and contains a valid key pair in the tls.crt and tls.key keys, this will be used for the certificate of the endpoint of argocd-server.
- Otherwise, if the argocd-secret secret contains a valid key pair in the tls.crt and tls.key keys, this will be used as certificate for the endpoint of argocd-server.
- If no tls.crt and tls.key keys are found in neither of the two mentioned secrets, Argo CD will generate a self-signed certificate and persist it in the argocd-secret secret.

So in summary, the secret MUST have a predefined name, otherwise it's just ignored.
But again, this is only for TLS exposed by the pods themselves, not the ingress.
You can still configure your own TLS secret for ingresses by using server.ingress.extraTls or applicationSet.ingress.extraTls values.

I hope it clarify things for you

@adamency
Copy link

@adamency adamency commented on b0d4648 Jun 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erwanval Thanks a lot for your answer.

I guessed so about this being for the TLS between pods, however as I explained, there is overlap with the Ingress as this secretName is the variable that will actually be used in the Ingress rule for the secret name in its TLS section.

And I had already tried using server.ingress.extraTls, this creates a duplicate host in the tls sectionof theIngressresource only with the secret name being different, instead of overwriting the secret name in the originalhost`.

Here is the output of a Helm dry-run with this values override file:

values-override.yaml

global:
  domain: argocd.<mydomain>
server:
  ingress:
    enabled: true
    ingressClassName: nginx
    extraTls:
      - hosts:
        - argocd.<mydomain>
        secretName: argocd-<mydomain>
    annotations:
      cert-manager.io/cluster-issuer: "letsencrypt-prod"
      nginx.ingress.kubernetes.io/backend-protocol: "HTTPS"
    tls: true

helm install --dry-run -f values-override.yaml [...]

[...]
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: argocd-server
  namespace: argocd
  labels:
    helm.sh/chart: argo-cd-7.2.1
    app.kubernetes.io/name: argocd-server
    app.kubernetes.io/instance: argocd
    app.kubernetes.io/component: server
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/part-of: argocd
    app.kubernetes.io/version: "v2.11.3"
  annotations:
    cert-manager.io/cluster-issuer: "letsencrypt-prod"
    nginx.ingress.kubernetes.io/backend-protocol: "HTTPS"
spec:
  ingressClassName: nginx
  rules:
    - host: argocd.<domain>
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: argocd-server
                port:
                  number: 443
  tls:
    - hosts:
      - argocd.<mydomain>
      secretName: argocd-server-tls
    - hosts:
      - argocd.<mydomain>
      secretName: argocd-<mydomain>
[...]

So now, that makes two host definitions with the same hostname, but different secrets. This would confuse Cert-Manager about which certificate name to use (and wouldn't this cause issues overall anyway ?)

But in the end, I still don't understand why the secretName variable should appear in the Ingress resource at all. Correct me if I'm wrong, but can't we simply create the Secret resource argocd-server-tls in the chart and leave the Ingress spec.tls.host.secretName variable outside of it ?

The real question is why a variable supposedly only involved in intra-cluster Pods communication is used in the Ingress resource, which is a resource meant specificallly only for external communication ?

Isn't there a bug in the chart code ? Either that, or there is something I am missing and is not explained in the excerpt you shared.

@erwanval
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamency On my side, I don't have that duplicated tls block on the ingress because my values define server.ingress.tls to false (the default), which prevent the tls block using the static secret name to appear in the first place. Beside that, my values are similar to yours.

That said, I agree with you that the secret name for the ingress TLS should be different than the one for "cluster internal" TLS, so in my opinion this is a bug.
I'm only a contributor, not a maintainer, so I can't say for sure, but my understanding is that server.certificate should be for internal TLS (as the comments in values.yaml suggest).
So based on that assumption, it means server.ingress.tls is only there for people who wants to use the same certificate for both internal and ingress TLS, and not bother about setting a different certificate.
And so, people who wants different certificates for internal and ingress TLS muse define server.ingress.tls to false and server.ingress.extraTls. Which is not really the most obvious, but works.

I'm not sure if a maintainer will see this, as these are comments on a past commit, so maybe an issue should be opened if you want to further investigate and/or improve this.

@adamency
Copy link

@adamency adamency commented on b0d4648 Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erwanval Thank you so much for all these very insightful points, and especially for your workaround in order to get the config I originally wanted.
I entirely agree with everything you said, i.e. that:

  • the solution for classic separate ingress config like we both did is very convoluted and non-obvious
  • and intra-cluster TLS cert and public-facing ingress TLS cert should be separate, at least by default.
  • and finally, that this should be explained more clearly in the docs for the chart

I will try to make an issue explaining all this when I find the time, in order to contact the maintainers about this.

In any case, my sincere thanks once again for helping see much more clearly now the structure of the helm chart :)

Please sign in to comment.