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

Make it possible to set release name different from argcd app name. #1066

Closed
peterbosalliandercom opened this issue Jan 29, 2019 · 25 comments · Fixed by #1682
Closed

Make it possible to set release name different from argcd app name. #1066

peterbosalliandercom opened this issue Jan 29, 2019 · 25 comments · Fixed by #1682
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@peterbosalliandercom
Copy link

Now the argocd app name is used as release name in the helm templating. It would be great if it is possible to use a seperate app name and release name like this:

argocd create app artifactory-prd --release-name artifactory ....
This way we can create apps for production cluster and test cluster with the same release name.

Reason we need this is that not all helm charts have the possibility to use the nameOverride parameter.

@jessesuen
Copy link
Member

We briefly considered this, but I think there may have been issue about the combination of:

  1. our decision to inject app.kubernetes.io/instance to be the app name
  2. using app name as helm release name (i.e. helm template --name <appname>)
  3. modern helm charts (created after helm 2.12?) also using release name in app.kubernetes.io/instance label.

I think the error scenario is:

  1. modern Helm charts created using helm create (v2.12) will auto-generate labels, including app.kubernetes.io/instance = <release name>
  2. Argo CD overrides this label with app.kubernetes.io/instance = <argocd app name>
  3. Since the two would be different, it may cause problems for the helm chart.

@jessesuen
Copy link
Member

Reason we need this is that not all helm charts have the possibility to use the nameOverride parameter.

That said, I do think this is a valid request. We are also facing similar issue about wanting to set release name in helm charts where app.kubernetes.io/instance is not used by the chart at all. I think one possibility is to detect and possibly error when we detect that user set a release name, and the chart is wanting to use release name in app.kubernetes.io/instance.

@igaskin
Copy link
Member

igaskin commented Feb 8, 2019

This requests seems especially important when managing helm charts installed from a helm repository, instead of installing the chart from source. As a workaround I removed all references to {{ .Release.Name }} in my local helm chart, and substituted it with a consistent name. Keep in mind that if the release name is used as a label selector, it cannot be changed once set: kubernetes/kubernetes#50808

@jessesuen jessesuen added help wanted Extra attention is needed good first issue Good for newcomers labels Mar 2, 2019
@jessesuen
Copy link
Member

I think it's fine to support releaseName, but we need to detect and error if the helm chart is using app.kubernetes.io/instance: <release-name>, and Argo CD wants to do app.kubernetes.io/instance: <application-name>.

For example, this the following situation:

Application Spec:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: foo
spec:
  source:
    helm:
      releaseName: bar

Helm template:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: {{ include "helm-test.fullname" . }}
  labels:
    app.kubernetes.io/name: {{ include "helm-test.name" . }}
    helm.sh/chart: {{ include "helm-test.chart" . }}
    app.kubernetes.io/instance: {{ .Release.Name }}
    app.kubernetes.io/managed-by: {{ .Release.Service }}
spec:
  replicas: {{ .Values.replicaCount }}
  selector:
    matchLabels:
      app.kubernetes.io/name: {{ include "helm-test.name" . }}
      app.kubernetes.io/instance: {{ .Release.Name }}
  template:
    metadata:
      labels:
        app.kubernetes.io/name: {{ include "helm-test.name" . }}
        app.kubernetes.io/instance: {{ .Release.Name }}

@stale
Copy link

stale bot commented May 1, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label May 1, 2019
@stale stale bot closed this as completed May 8, 2019
@alexmt alexmt reopened this May 8, 2019
@stale stale bot removed the wontfix This will not be worked on label May 8, 2019
@eupestov
Copy link

This feature can help to solve the issue with consistently naming resources across multiple clusters or namespaces, but only for helm applications. And indeed many newer charts do use .Release.name in app.kubernetes.io/instance.

A more general fix would be to decouple application resource name from the deployed application name and make the latter scoped with destination server and namespace values.

@lcostea
Copy link
Member

lcostea commented May 16, 2019

@jessesuen I am a little out of the loop on why argocd needs/wants to use app.kubernetes.io/instance: <application-name> but shouldn't it introduce its own labels, like helm does for helm.sh/chart: {{ include "helm-test.chart" . }}
This way there will be no clash and you can still have meta info that it was installed with argocd and the app it belongs to

@lcostea
Copy link
Member

lcostea commented May 20, 2019

@jessesuen So will it make sense to use another label, specific to ArgoCD in order to save the application name on the deployment (and other resources)?
This would allow to decouple the application name from the release name.
Also I see that the release name overriding feature was there, but was removed, so it will not be too complicated to get it back. I could help on that. Just to figure out the labelling.

@muenchdo
Copy link
Contributor

muenchdo commented Jun 4, 2019

I originally thought this was an issue too, but most Helm Charts actually have the option to override the name of the resources created, generally called something like nameOverride or fullnameOverride. Often this is not documented explicitly, but can be found in the Chart's helper templates, e.g. for RabbitMQ:

...
{{/*
Expand the name of the chart.
*/}}
{{- define "rabbitmq.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}}
{{- end -}}

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "rabbitmq.fullname" -}}
{{- if .Values.fullnameOverride -}}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}
{{- else -}}
...

Does this maybe solve your problem already?

@lcostea
Copy link
Member

lcostea commented Jun 4, 2019

@muenchdo If you follow the rabbitmq.fullname function you will see the outcome is {{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} so the Release.Name is not getting overridden, only the chart name.
So the problem is with the Release.Name being equal to the application name, when using centralised ArgoCD approach.

@alexec
Copy link
Contributor

alexec commented Jun 4, 2019

@lcostea has raised a PR for this feature (#1682) would anyone here fancy taking a look at it?

@alexec
Copy link
Contributor

alexec commented Jun 4, 2019

Ok. Reviewing @jessesuen comments, the key thing we need to consider is what to do if the chart uses app.kubernetes.io/instance. I would wonder is a problem not just for Helm.\

Options:

  1. Warn about this issue is helm.md, and then see if people encounter it.
  2. Report a warning in the logs when generating manifests.
  3. Error and refuse to sync when encountering it.

(1) is a quick doc-only fix. I expect (3) is only marginally more work than (3).

@lcostea @jessesuen - thoughts?

@lcostea
Copy link
Member

lcostea commented Jun 4, 2019

Only no 3. is safe, the others can lead to big problems.
And indeed this can lead to label clashes not only on helm.

What about:
4. Stop using app.kubernetes.io/instance and switch to another label, a custom one for argo-cd

@alexec
Copy link
Contributor

alexec commented Jun 4, 2019

I'd considered (4), but I believe would break any existing installation, and we ought to support our users. Thoughts?

@lcostea
Copy link
Member

lcostea commented Jun 5, 2019

Indeed, it would break current installations, but on the long run seems to be a better choice.
We could have a transition period, where for 2-3 minor versions we just add the extra tag and don't use it to find resources with it, while after that we stop using app.kubernetes.io/instance and switch to the new one completely.
With additional migration docs and warnings in code we could manage the risks.

@jessesuen
Copy link
Member

jessesuen commented Jun 5, 2019

The label can already be configurable to something other than app.kubernetes.io/instance. We actually switched to app.kubernetes.io/instance from applications.argoproj.io/app-name so that we were compliant with kubernetes recommended labels (https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/).

@jessesuen
Copy link
Member

jessesuen commented Jun 5, 2019

Here is how to do it:
https://github.com/argoproj/argo-cd/blob/master/docs/operator-manual/argocd-cm.yaml#L115-L118

NOTE: Once you change the label key, all apps are out of sync until the next sync.

@alexec
Copy link
Contributor

alexec commented Jun 5, 2019

@jessesuen what do you think of the related PR?

@alexec alexec added this to the v1.1 milestone Jun 5, 2019
@lcostea
Copy link
Member

lcostea commented Jun 5, 2019

@jessesuen
I read carefully https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ a couple of times
And I don't think argo-cd should use the shared labels for its tooling, instead it should use custom ones, just like helm does. Being an operator, I see argo-cd at least one layer above the concept of applications described in the article. It can loose the interoperability with other tools when a shared label is overwritten.
On the other hand of course I am subjective and see everything through my context and the way we use argo-cd.

It's great that we can override the app.kubernetes.io/instance label, thanks for sharing that.
So what if I add to the PR #1682 a check to see if there is a try to override the helm release name and the default label was used to throw an error? And add some additional documentation to mention that.

@jessesuen
Copy link
Member

The motivation for using app.kubernetes.io/instance was that we wanted our deployed applications to play well with other tooling (e.g. kubernetes dashboard). Our legacy label, applications.argoproj.io/app-name served the exact same purpose as app.kubernetes.io/instance (resource association), and so we wanted to converge to the standard, and be less opinionated.

On the other hand, I agree there is definitely some friction about the use of these labels and how it is causing issues with other popular deployment tools, namely helm, which is one of the reasons we made this configurable.

So what if I add to the PR #1682 a check to see if there is a try to override the helm release name and the default label was used to throw an error? And add some additional documentation to mention that.

I think that may be too heavy handed. The error needs to be chart dependent. There are many legacy charts which do not use app.kubernetes.io/instance and Argo CD can deploy these charts perfectly fine, even when releaseName is set to something different than the application name. We should not error out in this circumstance. Only the newer charts have started using this convention, and will have issue with the combination of release name, and app.kubernetes.io/instance. I feel that the only time if we should error is under these circumstance:

  • releaseName is used
  • we detect that the rendered chart is also using app.kubernetes.io/instance
  • The chart value for app.kubernetes.io/instance is different the application name

For PR #1682, either we need to add the more sophisticated error logic I described above, or we don't error at all, and leave it to documentation. However this is potential gotcha for anyone using Argo CD with helm.

In retrospect, choosing to use app.kubernetes.io/instance may have been a poor decision, since the most popular Kubernetes packaging tool also is also making use of this label. On the other hand, I also think Helm is way too opinionated about propagating app.kubernetes.io/instance down to deployment label selectors in their default template generation. If they did not propagate this down to selectors, there would be no issue at all.

@jessesuen
Copy link
Member

jessesuen commented Jun 6, 2019

Also, we have thought about using a different way to detect what is considered a related resource of an applcation, which wouldn't involve label injection. Since v0.11, Argo CD no longer required that the resource association to be a label, and I think we even thought about switching to an annotation, or ownership reference.

I believe this was supposed to be: #1482

@alexec
Copy link
Contributor

alexec commented Jun 6, 2019

@warmfusion
Copy link
Contributor

warmfusion commented Jun 24, 2019

We're using Kustomize to build our projects and have stumbled over an issue where our project names are auto-generated based on various attributes but are > 63 characters and thus break the instance label.

Having an option to override the label target doesn't really solve this problem as the label will still be too long.

We are creating a workaround where names are hashed and during the construction of the application.yamls but ArgoCD doesn't appear to check that the dynamically inserted values are valid per Kubernetes spec on labels.

@abdennour
Copy link

/reopen
Highly needed feature

@cra1gg
Copy link

cra1gg commented Aug 22, 2023

Looks like this is possible using the releaseName?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.