Request: remove --helm-set-string

[joestringer]

It's complicated to understand when --helm-set vs. --helm-set-string is needed in order to correctly configure Cilium. It would be nice to remove --helm-set-string and figure it out properly or fix the upstream Helm charts so that there is no ambiguity.

[michi-covalent]

@tklauser what do you think? personally i only use --set and -f, --values. maybe we can mark the other ones hidden to make the whole thing less confusing? 💭

[michi-covalent]

fix the upstream Helm charts so that there is no ambiguity.

yeah i guess the first step is to figure out if there are any settings right now that need --set-string instead of --set. grepping in the main branch i see a bunch of --set-string:

% git grep set-string
.github/actions/cilium-config/action.yml:            --helm-set-string=kubeProxyReplacement=${{ inputs.kpr }} \
.github/actions/cilium-config/action.yml:          TUNNEL="--helm-set-string=tunnelProtocol=${{ inputs.tunnel }}"
.github/actions/cilium-config/action.yml:            TUNNEL="--helm-set-string=routingMode=native --helm-set-string=autoDirectNodeRoutes=true --helm-set-string=ipv4NativeRoutingCIDR=10.244.0.0/16 --helm-set-string=tunnel=disabled"
.github/actions/cilium-config/action.yml:            TUNNEL="${TUNNEL} --helm-set-string=ipv6NativeRoutingCIDR=fd00:10:244::/56"
.github/actions/cilium-config/action.yml:            LB_MODE="--helm-set-string=loadBalancer.mode=${{ inputs.lb-mode }}"
.github/actions/cilium-config/action.yml:            ENDPOINT_ROUTES="--helm-set-string=endpointRoutes.enabled=true"
.github/workflows/conformance-clustermesh.yaml:            CILIUM_INSTALL_TUNNEL="--helm-set-string=routingMode=native \
.github/workflows/conformance-ipsec-e2e.yaml:            --helm-set-string=kubeProxyReplacement=${{ matrix.kpr }}"
.github/workflows/conformance-ipsec-e2e.yaml:          TUNNEL="--helm-set-string=tunnelProtocol=${{ matrix.tunnel }}"
.github/workflows/conformance-ipsec-e2e.yaml:            TUNNEL="--helm-set-string=routingMode=native --helm-set-string=autoDirectNodeRoutes=true --helm-set-string=ipv4NativeRoutingCIDR=10.244.0.0/16 --helm-set-string=tunnel=disabled"
.github/workflows/conformance-ipsec-e2e.yaml:            TUNNEL="${TUNNEL} --helm-set-string=ipv6NativeRoutingCIDR=fd00:10:244::/56"
.github/workflows/conformance-ipsec-e2e.yaml:            LB_MODE="--helm-set-string=loadBalancer.mode=${{ matrix.lb-mode }}"
.github/workflows/conformance-ipsec-e2e.yaml:            ENDPOINT_ROUTES="--helm-set-string=endpointRoutes.enabled=true"
.github/workflows/conformance-k8s-kind.yaml:            --helm-set-string=kubeProxyReplacement=strict \
.github/workflows/conformance-kind-proxy-daemonset.yaml:            --helm-set-string=kubeProxyReplacement=strict \
Documentation/contributing/testing/e2e.rst:        --helm-set-string=tunnel=vxlan \
Documentation/contributing/testing/e2e.rst:        --helm-set-string=tunnel=vxlan \
Documentation/network/servicemesh/installation.rst:                --set-string extraConfig.enable-envoy-config=true
Documentation/network/servicemesh/installation.rst:                --set-string extraConfig.enable-envoy-config=true
Documentation/network/servicemesh/installation.rst:                --set-string extraConfig.enable-envoy-config=true \

some of these should work fine with --set like --set=tunnel=vxlan 💭

[joestringer]

Can we swap those to use values files?

[michi-covalent]

yeah values file will always work i think

[joestringer]

Oh one other thing, I remember one of the set-string->set conversions might actually break something like kube-proxy-replacement because Cilium charts don't treat true and "true" the same :( -- we may need some upstream work on the Helm charts if we want to switch everything over to set. I don't have the PR handy but I remember reading that on someone's Helm PR in cilium/cilium.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[github-actions[bot]]

This issue has not seen any activity since it was marked stale. Closing.