[bitnami/argo-cd] Add native support for ConfigManagementPlugins

[maxnitze]

Description of the change

Adding support for ArgoCD Config Management Plugins (CMP) to the chart.

Argo CD's "native" config management tools are Helm, Jsonnet, and Kustomize. If you want to use a different config management tools, or if Argo CD's native tool support does not include a feature you need, you might need to turn to a Config Management Plugin (CMP).

Adding those plugins creates a bit of overhead, as there are some volumes that need to be shared between the repo server and the sidecar container of the plugin and the sidecar configuration in general. This is automatically added when the plugins are enabled.

Benefits

ConfigManagementPlugins can be enabled directly from the chart.

Example with the argocd-vault-plugin

Here's an example of how I would be using it for a CMP that adds support for the argocd-vault-plugin.

repoServer:
  extraVolumes:
    - name: vault-configuration
      secret:
        secretName: argo-cd-vault-configuration

  configManagementPlugins:
    enabled: true
    additionalBinaries:
      - name: argocd-vault-plugin
        url: https://github.com/argoproj-labs/argocd-vault-plugin/releases/download/v1.18.1/argocd-vault-plugin_1.18.1_linux_amd64
    plugins:
      - name: avp-helm
        spec:
          init:
            command:
              - /bin/sh
              - -c
              - |
                #!/usr/bin/env bash
                set -Eeuo pipefail
                
                # add all repositories from dependencies of this chart
                for REPO_URL in $(helm dependency list | tail -n+2 | tr -s '[:space:]' | cut -f3)
                do
                  # skip entries that reference local charts
                  if [[ "${REPO_URL}" == file://* ]]; then continue; fi
                  helm repo add $(echo -n "${REPO_URL}" | base64 -w0) "${REPO_URL}"
                done
                
                # finally download the charts dependencies
                helm dependency build
          generate:
            command:
              - /bin/sh
              - -c
              - |
                #!/usr/bin/env bash
                set -Eeuo pipefail
                
                # do some magic with the parameters
                # see https://github.com/argoproj-labs/argocd-vault-plugin/issues/566 for an example
                
                helm template ${HELM_RELEASE_NAME} \
                  --namespace ${ARGOCD_APP_NAMESPACE} \
                  ${HELM_VALUE_FILES} \
                  ${HELM_PARAMETERS} \
                  ${HELM_EXTRA_PARAMETERS} \
                  ./ |
                argocd-vault-plugin generate - -c /run/secrets/vault-configuration/config.yaml
          allowConcurrency: true
          lockRepo: false
        sidecar:
          image:
            repository: alpine/helm
            tag: 3.12.3
          extraEnvVars:
            # root filesystem is read-only, so we need to move the Helm home folders to /tmp
            - name: HELM_CACHE_HOME
              value: /tmp/helm/cache
            - name: HELM_CONFIG_HOME
              value: /tmp/helm/config
            - name: HELM_DATA_HOME
              value: /tmp/helm/data
          extraVolumeMounts:
            - mountPath: /run/secrets/vault-configuration/config.yaml
              name: vault-configuration
              subPath: config.yaml

Possible drawbacks

The change tries to make everything as much configurable as possible. So no drawbacks there (hopefully).

The change in general is opt-in. So no undesired changes to existing installations.

Applicable issues

None

Additional information

argocd.tplvalues.merge-with-precedence Helper

I introduced a new helper for merging values from a list with precedence to later entries.

This was necessary, because the helper common.tplvalues.merge has one major "flaw", that I needed fixed here:

context1:
  enabled: true
  name: 1001
  isRunning: true
context2:
  enabled: false
  name: 1234
  isRunning: false

When I merge these two with include "common.tplvalues.merge" (dict "values" (list .context1 .context2) "context" $), the result is

mergedContext:
  enabled: true
  name: 1234
  isRunning: true

I don't know if this is a bug or a feature (the comment of the helper says, that "precedence is consistent with http://masterminds.github.io/sprig/dicts.html#merge-mustmerge").

Since I was merging container security contexts here, I wanted to be give the option to deactivate the readOnlyRootFilesystem, for example. Or disable the context completely.

The result when using the new helper include "argocd.tplvalues.merge-with-precedence" (dict "values" (list .context1 .context2) "context" $) is

mergedContext:
  enabled: false
  name: 1234
  isRunning: false

Checklist

• Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
• Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
• Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
• All commits signed off and in agreement of Developer Certificate of Origin (DCO)

[maxnitze]

Not sure why the VIB Verify check failed here. But it also failed for other PRs. Maybe somethings wrong with the action?

[andresbono]

Thank you very much for your contribution, this could be a great addition! I left some initial questions. Let's discuss them before we go more in depth!

[andresbono]

Is this new value repoServer.configManagementPlugins.enabled actually needed? Wouldn't it be enough to check if repoServer.configManagementPlugins.plugins has elements or not?

• If it has elements, then we consider the feature activated
• Otherwise, deactivated

[maxnitze]

We could, yeah. This is the pattern we use for most features though, right? I also like the fact, that for debugging purposes you can just disable it here without having to delete the whole plugins config.

[andresbono]

Could we achieve the same by using repoServer.initContainers? And it gives you more flexibility... In addition to that, I would expect that users also customize their container images to include any required binaries baked in the image instead of having to download the binaries every time at runtime. Does it make sense?

[maxnitze]

Yep. In the end having the binaries here will only fill an init container. You can still use your own init container or bake your binaries into your custom image. You just have to make sure, that the binaries are on the path in the sidecar container, that runs your plugin.

The whole point of the change was to get rid of the boilerplate and having a simple way of configuring a CMP. That why I included this. But as stated, this is fully optional.

[maxnitze]

While I was thinking about it: Maybe we can skip the "customScript" part and recommend to use an own init container or custom image in that case. But for the easy cases, where the binary can just be downloaded from a given URL, I would prefre to keep the functionality.

[andresbono]

Yes, at the very least, we should skip the customScript part. Will you update the PR? Once we have that removed, let's see if it makes sense to keep simplifying the implementation. Thanks!

[maxnitze]

Updated the chart and removed the custom script option of the additional binaries.

[andresbono]

@maxnitze, thank you for addressing this. Let me take a look at the changes to get back to you. No worries about the conflicts, I'll take care of resolving them.

[andresbono]

Any container image should be maintained by Bitnami and configured in a standard way from values.yaml. You could use bitnami/os-shell. Look at the volumePermissions.image value to follow a similar pattern.

Do you have any concerns with that?

[maxnitze]

No issues at all! I remember thinking about this when I added the init-container. I think I just forgot about it. Anyway, added it now!

[andresbono]

Hi, I just added some comments to the PR. Could you please check? Thank you!

[andresbono]

I'm not sure what is the value of having this helper function here in argo-cd. It looks like a generic function unrelated to argo-cd itself.

This should go in the common library, although I would try to avoid adding more complexity and try to avoid the usage of this helper if possible.

[andresbono]

Suggested change

	securityContext: {{- omit .Values.repoServer.containerSecurityContext "enabled" | toYaml | nindent 12 }}
	securityContext: {{- include "common.compatibility.renderSecurityContext" (dict "secContext" .Values.repoServer.containerSecurityContext "context" $) | nindent 12 }}

[andresbono]

This should go into its own helper function (see examples for the other images):

{{/*
Return the proper CMP binary downloader image name
*/}}
{{- define "argocd.repoServer.configManagementPlugins.additionalBinaries.image" -}}
{{- include "common.images.image" ( dict "imageRoot" .Values.repoServer.configManagementPlugins.additionalBinaries.image "global" .Values.global ) -}}
{{- end -}}

and be called like:

Suggested change

	image: {{ include "common.images.image" (dict "imageRoot" .image "global" $.Values.global) }}
	image: {{ include "argocd.repoServer.configManagementPlugins.additionalBinaries.image" . }}

[andresbono]

Should we pass the -ec flags to detect errors?

[andresbono]

I think it would be better to simplify all this and let users decide what will be the sidecar preferred structure. It simplify our maintenance at the same time it allows total flexibility for users.

It would be the same approach that we follow in .Values.repoServer.sidecars for example.

[andresbono]

There is a typo here, but in any case, refer to my previous comment about simplifying all this code and allow users to set their sidecars config explicitly.

Suggested change

	{{- else if and $plugin.sidecar.resourcePreset (ne $plugin.sidecar.resourcesPreset "none") }}
	{{- else if and $plugin.sidecar.resourcesPreset (ne $plugin.sidecar.resourcesPreset "none") }}

[andresbono]

Should this image be added to argocd.imagePullSecrets?

[github-actions]

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.