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

[jstewart612]

Signed-off-by: John Stewart jstewart@rentpath.com

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• I have bumped the chart version according to versioning
• I have updated the documentation according to documentation
• I have updated the chart changelog with all the changes that come with this pull request according to changelog.
• Any new values are backwards compatible and/or have sensible default.
• I have signed off all my commits as required by DCO.
• My build is green (troubleshooting builds).

Changes are automatically published when merged to main. They are not published on branches.

[pdrastil]

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

[jstewart612]

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.

[pdrastil]

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.

[jstewart612]

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).

[pdrastil]

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.

[pdrastil]

Thank you for explaining. Now I see where this is heading. I've actually discussed usage of Reloader with @mkilchhofer as we are aware of problems with automatic restarts of pods when some externally managed secrets or configmaps change and I haven't immediately recognised that this PR is related to Reloader.

Use cases for this PR will be to add support for reloading all running pods when configmap or secret changes like this:

global:
  deploymentAnnotations:
    reloader.stakater.com/auto: "true"
  statefulsetAnnotations:
    reloader.stakater.com/auto: "true"

Or in some cases it might be selective reload like this:

# Reload resources with match
global:
  deploymentAnnotations:
    reloader.stakater.com/search: "true"
  statefulsetAnnotations:
    reloader.stakater.com/search: "true"

configs:
  params:
    annotations:
      reloader.stakater.com/match: "true"

# Reloads specific secret
controller:
  annotations:
    secret.reloader.stakater.com/reload: "argocd-repo-server-tls"

repoServer:
  annotations:
    secret.reloader.stakater.com/reload: "argocd-repo-server-tls"

To get approval:

1. Please change deploymentAnnotations to annotations on individual component level
2. I would keep global defaults as they are now. It's kinda copy & paste of same annotation into 2 places but It's possible that we might want to introduce common annotations for every resource or default annotations for specific resources if we see need for that. @mkilchhofer @yu-croco @jmeridth wdyt?

Edit maybe just deploymentAnnotations in global section would be enough to avoid copy & paste even if it's not accurate for STS.

[jmeridth]

@pdrastil there is controller.deploymentAnnotations in the argo-workflows chart currently. We use that at my day job for reloader annotations currently. argo-cd chart may have other annotations that could be "centralized" but we may want to think about this across all charts maybe? I'm a fan of specific but can be outvoted. @mkilchhofer @yu-croco

[pdrastil]

@pdrastil If you mean all charts then it's about naming things in global section more than component level as it's common to group multiple deployments under single umbrella chart.

[mkilchhofer]

@pdrastil If you mean all charts then it's about naming things in global section more than component level as it's common to group multiple deployments under single umbrella chart.

Sure? Do you like the setup where multiple Argo components are deployed together with an umbrella chart? I always try to avoid this to have smaller piece of potential breaking stuff at once :-D

[mkilchhofer]

LGTM