Skip to content

feat(argo-cd): Add ability to annotate Deployments and StatefulSets #1608

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.

Sign up for GitHub

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

Merged
mkilchhofer merged 4 commits into from
Nov 30, 2022
Merged

feat(argo-cd): Add ability to annotate Deployments and StatefulSets #1608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0402fb
feat(argo-cd): Add ability to annotate Deployments and StatefulSets
Oct 31, 2022
8363b21
Merge branch 'main' into argo-cd-annotate-addtl-objects
jstewart612 Nov 3, 2022
6e8bd90
Merge branch 'main' into argo-cd-annotate-addtl-objects
mkilchhofer Nov 30, 2022
64b2087
fix: Controller and AppSet controller was mixed
mkilchhofer Nov 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions charts/argo-cd/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ appVersion: v2.5.3
kubeVersion: ">=1.22.0-0"
description: A Helm chart for Argo CD, a declarative, GitOps continuous delivery tool for Kubernetes.
name: argo-cd
version: 5.15.2
version: 5.16.0
home: https://github.com/argoproj/argo-helm
icon: https://argo-cd.readthedocs.io/en/stable/assets/logo.png
sources:
Expand All @@ -23,7 +23,4 @@ dependencies:
condition: redis-ha.enabled
annotations:
artifacthub.io/changes: |
- "[Changed]: ApplicationSet now automatically detects leader election"
- "[Changed]: Simplified ApplicationSet RBAC rules"
- "[Removed]: Configuration option applicationset.args.debug"
- "[Removed]: Configuration option applicationset.args.enableLeaderElection"
- "[Added]: Ability to annotate Deployment and Statefulset objects for all components"
9 changes: 9 additions & 0 deletions charts/argo-cd/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ NAME: my-release
| Key | Type | Default | Description |
|-----|------|---------|-------------|
| global.additionalLabels | object | `{}` | Common labels for the all resources |
| global.deploymentAnnotations | object | `{}` | Annotations for the all deployed Deployments |
| global.hostAliases | list | `[]` | Mapping between IP and hostnames that will be injected as entries in the pod's hosts files |
| global.image.imagePullPolicy | string | `"IfNotPresent"` | If defined, a imagePullPolicy applied to all Argo CD deployments |
| global.image.repository | string | `"quay.io/argoproj/argocd"` | If defined, a repository applied to all Argo CD deployments |
Expand All @@ -389,6 +390,7 @@ NAME: my-release
| global.podLabels | object | `{}` | Labels for the all deployed pods |
| global.revisionHistoryLimit | int | `3` | Number of old deployment ReplicaSets to retain. The rest will be garbage collected. |
| global.securityContext | object | `{}` (See [values.yaml]) | Toggle and define pod-level security context. |
| global.statefulsetAnnotations | object | `{}` | Annotations for the all deployed Statefulsets |

## Argo CD Configs

Expand Down Expand Up @@ -506,6 +508,7 @@ NAME: my-release
| controller.serviceAccount.create | bool | `true` | Create a service account for the application controller |
| controller.serviceAccount.labels | object | `{}` | Labels applied to created service account |
| controller.serviceAccount.name | string | `"argocd-application-controller"` | Service account name |
| controller.statefulsetAnnotations | object | `{}` | Annotations for the application controller StatefulSet |
| controller.tolerations | list | `[]` | [Tolerations] for use with node taints |
| controller.topologySpreadConstraints | list | `[]` | Assign custom [TopologySpreadConstraints] rules to the application controller |
| controller.volumeMounts | list | `[]` | Additional volumeMounts to the application controller main container |
Expand Down Expand Up @@ -533,6 +536,7 @@ NAME: my-release
| repoServer.clusterRoleRules.rules | list | `[]` | List of custom rules for the Repo server's Cluster Role resource |
| repoServer.containerPort | int | `8081` | Configures the repo server port |
| repoServer.containerSecurityContext | object | See [values.yaml] | Repo server container-level security context |
| repoServer.deploymentAnnotations | object | `{}` | Annotations to be added to repo server Deployment |
| repoServer.env | list | `[]` | Environment variables to pass to repo server |
| repoServer.envFrom | list | `[]` (See [values.yaml]) | envFrom to pass to repo server |
| repoServer.extraArgs | list | `[]` | Additional command line arguments to pass to repo server |
Expand Down Expand Up @@ -632,6 +636,7 @@ NAME: my-release
| server.clusterAdminAccess.enabled | bool | `true` | Enable RBAC for local cluster deployments |
| server.containerPort | int | `8080` | Configures the server port |
| server.containerSecurityContext | object | See [values.yaml] | Server container-level security context |
| server.deploymentAnnotations | object | `{}` | Annotations to be added to server Deployment |
| server.env | list | `[]` | Environment variables to pass to Argo CD server |
| server.envFrom | list | `[]` (See [values.yaml]) | envFrom to pass to Argo CD server |
| server.extensions.containerSecurityContext | object | See [values.yaml] | Server UI extensions container-level security context |
Expand Down Expand Up @@ -775,6 +780,7 @@ server:
| dex.containerPortHttp | int | `5556` | Container port for HTTP access |
| dex.containerPortMetrics | int | `5558` | Container port for metrics access |
| dex.containerSecurityContext | object | See [values.yaml] | Dex container-level security context |
| dex.deploymentAnnotations | object | `{}` | Annotations to be added to the Dex server Deployment |
| dex.enabled | bool | `true` | Enable dex |
| dex.env | list | `[]` | Environment variables to pass to the Dex server |
| dex.envFrom | list | `[]` (See [values.yaml]) | envFrom to pass to the Dex server |
Expand Down Expand Up @@ -848,6 +854,7 @@ server:
| redis.affinity | object | `{}` | Assign custom [affinity] rules to the deployment |
| redis.containerPort | int | `6379` | Redis container port |
| redis.containerSecurityContext | object | See [values.yaml] | Redis container-level security context |
| redis.deploymentAnnotations | object | `{}` | Annotations to be added to the Redis server Deployment |
| redis.enabled | bool | `true` | Enable redis |
| redis.env | list | `[]` | Environment variables to pass to the Redis server |
| redis.envFrom | list | `[]` (See [values.yaml]) | envFrom to pass to the Redis server |
Expand Down Expand Up @@ -955,6 +962,7 @@ If you want to use an existing Redis (eg. a managed service from a cloud provide
| applicationSet.args.policy | string | `"sync"` | How application is synced between the generator and the cluster |
| applicationSet.args.probeBindAddr | string | `":8081"` | The default health check port |
| applicationSet.containerSecurityContext | object | See [values.yaml] | ApplicationSet controller container-level security context |
| applicationSet.deploymentAnnotations | object | `{}` | Annotations to be added to ApplicationSet controller Deployment |
| applicationSet.enabled | bool | `true` | Enable ApplicationSet controller |
| applicationSet.extraArgs | list | `[]` | List of extra cli args to add |
| applicationSet.extraContainers | list | `[]` | Additional containers to be added to the applicationset controller pod |
Expand Down Expand Up @@ -1056,6 +1064,7 @@ If you want to use an existing Redis (eg. a managed service from a cloud provide
| notifications.cm.create | bool | `true` | Whether helm chart creates controller config map |
| notifications.containerSecurityContext | object | See [values.yaml] | Notification controller container-level security Context |
| notifications.context | object | `{}` | Define user-defined context |
| notifications.deploymentAnnotations | object | `{}` | Annotations to be applied to the notifications controller Deployment |
| notifications.enabled | bool | `true` | Enable notifications controller |
| notifications.extraArgs | list | `[]` | Extra arguments to provide to the controller |
| notifications.extraEnv | list | `[]` | Additional container environment variables |
Expand Down
8 changes: 7 additions & 1 deletion charts/argo-cd/templates/argocd-application-controller/statefulset.yaml
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
apiVersion: apps/v1
kind: StatefulSet
metadata:
name: {{ include "argo-cd.controller.fullname" . }}
{{- with (mergeOverwrite (deepCopy .Values.global.statefulsetAnnotations) .Values.controller.statefulsetAnnotations) }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
name: {{ template "argo-cd.controller.fullname" . }}
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.controller.name "name" .Values.controller.name) | nindent 4 }}
spec:
Expand Down
6 changes: 6 additions & 0 deletions charts/argo-cd/templates/argocd-applicationset/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
apiVersion: apps/v1
kind: Deployment
metadata:
{{- with (mergeOverwrite (deepCopy .Values.global.deploymentAnnotations) .Values.applicationSet.deploymentAnnotations) }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
name: {{ include "argo-cd.applicationSet.fullname" . }}
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.applicationSet.name "name" .Values.applicationSet.name) | nindent 4 }}
Expand Down
8 changes: 7 additions & 1 deletion charts/argo-cd/templates/argocd-notifications/bots/slack/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: {{ include "argo-cd.notifications.fullname" . }}-bot
{{- with (mergeOverwrite (deepCopy .Values.global.deploymentAnnotations) .Values.notifications.deploymentAnnotations) }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
name: {{ template "argo-cd.notifications.fullname" . }}-bot
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.notifications.bots.slack.name "name" .Values.notifications.bots.slack.name) | nindent 4 }}
spec:
Expand Down
6 changes: 6 additions & 0 deletions charts/argo-cd/templates/argocd-notifications/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
apiVersion: apps/v1
kind: Deployment
metadata:
{{- with (mergeOverwrite (deepCopy .Values.global.deploymentAnnotations) .Values.notifications.deploymentAnnotations) }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
name: {{ include "argo-cd.notifications.fullname" . }}
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.notifications.name "name" .Values.notifications.name) | nindent 4 }}
Expand Down
6 changes: 6 additions & 0 deletions charts/argo-cd/templates/argocd-repo-server/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
apiVersion: apps/v1
kind: Deployment
metadata:
{{- with (mergeOverwrite (deepCopy .Values.global.deploymentAnnotations) .Values.repoServer.deploymentAnnotations) }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
name: {{ template "argo-cd.repoServer.fullname" . }}
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.repoServer.name "name" .Values.repoServer.name) | nindent 4 }}
Expand Down
6 changes: 6 additions & 0 deletions charts/argo-cd/templates/argocd-server/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
apiVersion: apps/v1
kind: Deployment
metadata:
{{- with (mergeOverwrite (deepCopy .Values.global.deploymentAnnotations) .Values.server.deploymentAnnotations) }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
name: {{ template "argo-cd.server.fullname" . }}
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.server.name "name" .Values.server.name) | nindent 4 }}
Expand Down
6 changes: 6 additions & 0 deletions charts/argo-cd/templates/dex/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
apiVersion: apps/v1
kind: Deployment
metadata:
{{- with (mergeOverwrite (deepCopy .Values.global.deploymentAnnotations) .Values.dex.deploymentAnnotations) }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
name: {{ template "argo-cd.dex.fullname" . }}
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.dex.name "name" .Values.dex.name) | nindent 4 }}
Expand Down
6 changes: 6 additions & 0 deletions charts/argo-cd/templates/redis/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
apiVersion: apps/v1
kind: Deployment
metadata:
{{- with (mergeOverwrite (deepCopy .Values.global.deploymentAnnotations) .Values.redis.deploymentAnnotations) }}
annotations:
{{- range $key, $value := . }}
{{ $key }}: {{ $value | quote }}
{{- end }}
{{- end }}
name: {{ include "argo-cd.redis.fullname" . }}
labels:
{{- include "argo-cd.labels" (dict "context" . "component" .Values.redis.name "name" .Values.redis.name) | nindent 4 }}
Expand Down
27 changes: 27 additions & 0 deletions charts/argo-cd/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ global:
# -- Set the global logging level. One of: `debug`, `info`, `warn` or `error`
level: info

# -- Annotations for the all deployed Statefulsets
statefulsetAnnotations: {}
Copy link
Member

@pdrastil pdrastil Nov 1, 2022

Choose a reason for hiding this comment

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

Is the type important or simple annotations would be enough? Technically all things in this chart are Deployments and only controller component is StatefulSet.

Copy link
Contributor Author

@jstewart612 jstewart612 Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Elsewhere in this chart is the value concept of podAnnotations. As such, since it was accepted, I presumed this was a desired standard of distinguish that which you were annotating. Therefore, I carried forward that convention. Reworking to dissolve that convention is something theoretically doable... but it feels like scope creep against what this PR is trying to solve... no?

If you disagree, perhaps a more general convention could be adopted throughout. Maybe a generalized "annotations" key with subkeys "pod", "deployment", and "statefulset"? Even if only one of the statefulsets are present in the entire helm chart, it is, and it's not a choice I made... and therefore one for which I have a use case that must account that I suspect others may have as well.

Copy link
Member

@pdrastil pdrastil Nov 3, 2022

Choose a reason for hiding this comment

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

  1. I'm asking because If you take a look on every single resource in this chart (services, accounts, secrets, configmaps, etc.) you will see annotations and labels as a standard for metadata section. Yet the PR is using nonstandard controller.deploymentAnnotations for metadata section. This is would also imply that controller with kind: StatefulSet is actually a deployment which is not true.

  2. For global section I'm wondering if globally defined common annotations should be split based on component kind and what is the actual real use case for that.

Copy link
Contributor Author

@jstewart612 jstewart612 Nov 3, 2022 •
edited by mkilchhofer
Loading

Choose a reason for hiding this comment

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

It is inaccurate to say "every single resource in this chart":

| global.podAnnotations | object | `{}` | Annotations for the all deployed pods |
| configs.credentialTemplatesAnnotations | object | `{}` | Annotations to be added to `configs.credentialTemplates` Secret |
| configs.repositoriesAnnotations | object | `{}` | Annotations to be added to `configs.repositories` Secret |
| controller.podAnnotations | object | `{}` | Annotations to be added to application controller pods |
| repoServer.podAnnotations | object | `{}` | Annotations to be added to repo server pods |
| server.podAnnotations | object | `{}` | Annotations to be added to server pods |
| dex.podAnnotations | object | `{}` | Annotations to be added to the Dex server pods |
| redis.podAnnotations | object | `{}` | Annotations to be added to the Redis server pods |
| applicationSet.podAnnotations | object | `{}` | Annotations for the controller pods |
| notifications.podAnnotations | object | `{}` | Annotations to be applied to the controller Pods |

The "actual real use case for that", in this case, is third party software suites that operate based on said annotations that I cannot presently use with your helm chart. For example, Stakater Reloader https://github.com/stakater/Reloader. There are secrets in the helm chart that the software does not reload the pods for when required to do so. As such, our organizational strategy is to leverage this whenever applications have edge cases where Secrets data they use do not reload upon that data's change when required to. Any extra Secret I add to related Deployments or Statefulsets will operate this way. The only one I know of that you auto reload is SSL cert for argocd-server, and we've even had that fail on us once (cert-manager updated the Secret for the TLS cert, argo-cd did not reload it until pod termination and restart... that will be a separate issue with our findings there).

As for global, I'll rephrase my first response on the matter. I wouldn't be opposed to, say:

global.annotations.pod
global.annotations.statefulset
global.annotations.deployment

If we did this, though, global.podAnnotations would then become redundant, and, naturally, for consistency, you'd want to rip that out. Will you require this of me to approve this PR? I was also trying to avoid making other people change things about how they use things (a breaking change).

Copy link
Member

@pdrastil pdrastil Nov 4, 2022

Choose a reason for hiding this comment

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

Thanks for explaining this and you are right that there are some places where this convention isn't followed. I was trying to figure why are new specific definitions for annotations required so we can avoid unnecessary tech debt.


# -- Annotations for the all deployed Deployments
deploymentAnnotations: {}

# -- Annotations for the all deployed pods
podAnnotations: {}

Expand Down Expand Up @@ -540,6 +546,9 @@ controller:
# - secretRef:
# name: secret-name

# -- Annotations for the application controller StatefulSet
statefulsetAnnotations: {}

# -- Annotations to be added to application controller pods
podAnnotations: {}

Expand Down Expand Up @@ -846,6 +855,9 @@ dex:
# -- Certificate data. Must contain SANs of Dex service (ie: argocd-dex-server, argocd-dex-server.argo-cd.svc)
crt: ''

# -- Annotations to be added to the Dex server Deployment
deploymentAnnotations: {}

# -- Annotations to be added to the Dex server pods
podAnnotations: {}

Expand Down Expand Up @@ -1029,6 +1041,9 @@ redis:
# - secretRef:
# name: secret-name

# -- Annotations to be added to the Redis server Deployment
deploymentAnnotations: {}

# -- Annotations to be added to the Redis server pods
podAnnotations: {}

Expand Down Expand Up @@ -1343,6 +1358,9 @@ server:
# @default -- `""` (defaults to global.logging.level)
# logLevel: ""

# -- Annotations to be added to server Deployment
deploymentAnnotations: {}

# -- Annotations to be added to server pods
podAnnotations: {}

Expand Down Expand Up @@ -1898,6 +1916,9 @@ repoServer:
# @default -- `""` (defaults to global.logging.format)
# logLevel: ""

# -- Annotations to be added to repo server Deployment
deploymentAnnotations: {}

# -- Annotations to be added to repo server pods
podAnnotations: {}

Expand Down Expand Up @@ -2227,6 +2248,9 @@ applicationSet:
# If not set and create is true, a name is generated using the fullname template
name: ""

# -- Annotations to be added to ApplicationSet controller Deployment
deploymentAnnotations: {}

# -- Annotations for the controller pods
podAnnotations: {}

Expand Down Expand Up @@ -2517,6 +2541,9 @@ notifications:
# service.slack: |
# token: $slack-token

# -- Annotations to be applied to the notifications controller Deployment
deploymentAnnotations: {}

# -- Annotations to be applied to the controller Pods
podAnnotations: {}

Expand Down