fix: Add mergo.WithAppendSlice for VaultConfigurerPodSpec

[mikeluttikhuis]

Overview

Hi team,

We want to be able to override some fields of the bank-vaults container in the vault-configurer deployment. Currently, the operator does not allow merging of the 'containers' array into the predefined PodSpec.

Fixes

I've added the mergo.WithAppendSlice argument this mergo.Merge function so you are able to override all the fields of the container.

---

I used the following code snipped to test locally using mergo, it may come in handy for you to validate my suggested change:

package main

import (
	"fmt"
	"log"

	"dario.cat/mergo"
	"gopkg.in/yaml.v2"
	corev1 "k8s.io/api/core/v1"
)

func main() {
	podSpec := corev1.PodSpec{
		Containers: []corev1.Container{
			{
				Name: "bank-vaults",
			},
		},
	}
	mergeSpec := corev1.PodSpec{
		Containers: []corev1.Container{
			{
				Name: "bank-vaults",
				SecurityContext: &corev1.SecurityContext{
					RunAsUser:  new(int64), // New value
					Privileged: new(bool),  // New value
				},
			},
		},
	}
	// Use mergo with the WithAppendSlice option to merge slices
	if err := mergo.Merge(&podSpec, mergeSpec, mergo.WithAppendSlice); err != nil {
		log.Fatalf("Error merging specs: %v", err)
	}
	yamlData, err := yaml.Marshal(podSpec)
	if err != nil {
		log.Fatalf("Error marshaling to YAML: %v", err)
	}

	// Print the YAML to stdout
	fmt.Println(string(yamlData))
}

[fstr]

I'm also interested in this patch. I want to mount an additional volume to the vault-configurer containing my root CA certificate. I was relieved when I found the vaultConfigurerPodSpec but the vaultConfigurerPodSpec.volumes property has no effect (without this patch).

With this patch, the vaultConfigurerPodSpec.volumes work. But there's still a problem that I can't solve this patch, which is the volumeMounts on the vault-configurer container.

  vaultConfigurerPodSpec:
    # volumes work with this patch
    volumes:
      - name: cache-volume
        emptyDir:
          sizeLimit: 500Mi
    # I can't get the containers volumeMounts merged though
    containers:
      - name: bank-vaults
        # does not work
        volumeMounts:
          - mountPath: /var/run/secrets/root-cert-bundle
            name: cache-volume
            readOnly: true

In comparison, for the vault StatefulSet the volumes property works out of the box, because it's implemented in a different way. Example from my kind: Vault manifest, this is how I configured the vault StatefulSet. For the volume mounts, a special property was implemented.

  # works
  bankVaultsVolumeMounts:
    - mountPath: /var/run/secrets/root-cert-bundle
      name: root-cert-bundle
      readOnly: true
  # works
  volumes:
    - name: root-cert-bundle
      secret:
        secretName: root-cert-bundle
        defaultMode: 420
        items:
          - key: root-cert-bundle.pem
            path: root-cert-bundle.pem

If mergo is not able to merge the containers volumeMounts, I believe we should change the approach and add a dedicated property vaultConfigurerVolumeMounts, as it was done for the vault StatefulSet. Adding an additional volume mount seems like a useful feature that other people will benefit from as well.

[ramizpolic]

Thanks for addressing this @mikeluttikhuis . Could you please add a test case for this behaviour so that we can iterate over this in case of any future feature requests/issues. Thanks!!

[github-actions]

Thank you for your contribution! This PR has been automatically marked as stale because it has no recent activity in the last 60 days. It will be closed in 20 days, if no further activity occurs. If this pull request is still relevant, please leave a comment to let us know, and the stale label will be automatically removed.

[github-actions]

This PR has been marked stale for 20 days, and is now closed due to inactivity. If this contribution is still relevant, please re-open this PR or file a new one. Thank you!