Set Multus ConfigMap name so that it matches DaemonSet

[micke]

When trying to configure multus using the configmap by setting manifests.configMap to true i noticed that the name of the generated ConfigMap doesn't match the name used in the DaemonSet.

I'm deploying in K3S using the built-in helm controller.

I'm using the following config:

apiVersion: helm.cattle.io/v1
kind: HelmChart
metadata:
  name: multus
  namespace: kube-system
spec:
  repo: https://rke2-charts.rancher.io
  chart: rke2-multus
  bootstrap: true
  version: 4.1.1
  targetNamespace: kube-system
  valuesContent: |-
    config:
      cni_conf:
        multusConfFile: /tmp/multus-conf/00-multus.conf.template
        cniVersion: 0.3.1
        name: multus-cni-network
        type: multus
        confDir: /var/lib/rancher/k3s/agent/etc/cni/net.d
        binDir: /var/lib/rancher/k3s/data/current/bin/
        kubeconfig: /var/lib/rancher/k3s/agent/etc/cni/net.d/multus.d/multus.kubeconfig
        delegates:
          - type: cilium-cni
            name: cilium
            cniVersion: 0.3.1
            delegate:
              isDefaultGateway: true
    manifests:
      dhcpDaemonSet: true
      configMap: true

[brandond]

Wouldn't this be a breaking change for existing chart users? Why not just deploy it using the name it expects:

apiVersion: helm.cattle.io/v1
kind: HelmChart
metadata:
  name: rke2-multus
  namespace: kube-system
spec:
  repo: https://rke2-charts.rancher.io
  chart: rke2-multus

Or am I misunderstanding, and the configmap is broken regardless of what name the chart is deployed as?

[micke]

@brandond I tried deploying it with the name rke2-multus as that was my first thought too, but then the daemonset will look for a config map named rke2-multus-rke2-multus-v4.1.001-config while the configmaps actual name is rke2-multus-v4.1.001-config.

As far as i can see, the manifests.configMap option have never worked in the rke2 version of the chart.
The name of the configmap was correct in the original chart here: https://github.com/k8snetworkplumbingwg/helm-charts/blob/ca7c0a7549952660eab8f4b12e7ec7be133b381c/multus/templates/configMap.yaml#L19
But even when this repository used that as it's base, it patched the name here: 5d8f7f1#diff-817938548802781dfb15402b649db54551ccc3ead8e1f427c96e5da01f15fc66L9
without patching it in the daemonset here: 5d8f7f1#diff-5a4221204c9e47e3f6a99c2b613c9da3d02ac58e795d985d8ba62898fc67bb40

Once the chart was "localized" to this repository, the brokenness was brought with it: 5d8f7f1

I also noticed an issue with the filename the configmap is mounted with and had to push commit e8c2dee to fix it.
As you can see, the command on the daemonset in the original chart copied the mounted configmap here: https://github.com/k8snetworkplumbingwg/helm-charts/blob/master/multus/templates/daemonSet.yaml#L63

My guess is that noone has used the manifests.configMap option before me, so the issue was never discovered 🤷

[brandond]

thanks!

[brandond]

Oh actually I think we need to bump the packageVersion here:
https://github.com/rancher/rke2-charts/blob/main-source/packages/rke2-multus/package.yaml#L3

cc @manuelbuil - should we bump the package version, or the chart version?

[manuelbuil]

Oh actually I think we need to bump the packageVersion here: https://github.com/rancher/rke2-charts/blob/main-source/packages/rke2-multus/package.yaml#L3

cc @manuelbuil - should we bump the package version, or the chart version?

The package version

[mgfritch]

Could you also squash these into a single commit (with a sign-off)?
otherwise, lgtm!

[micke]

@mgfritch Of course, it's done!

[mgfritch]

@micke Thanks for the contribution!

[micke]

Thank you for your time and attention!