Improve the helm values autodetection logic

.github/in-cluster-test-scripts/external-workloads-install.sh

@@ -9,7 +9,7 @@ cilium install \
   --set cluster.name="${CLUSTER_NAME}" \
   --set bpf.monitorAggregation=none \
   --datapath-mode=tunnel \
-  --set kubeProxyReplacement=strict \
+  --set kubeProxyReplacement=true \
   --set loadBalancer.l7.backend=envoy \
   --set tls.secretsBackend=k8s \
   --set ipv4NativeRoutingCIDR="${CLUSTER_CIDR}"

.github/workflows/kind.yaml

@@ -141,7 +141,7 @@ jobs:
           --nodes-without-cilium="${NODES_WITHOUT_CILIUM}" \
           --set encryption.enabled=true \
           --set encryption.type=ipsec \
-          --set kubeProxyReplacement=disabled
+          --set kubeProxyReplacement=false
 
       - name: Enable Relay
         run: |

connectivity/check/features.go

@@ -147,10 +147,10 @@ func (ct *ConnectivityTest) extractFeaturesFromCiliumStatus(ctx context.Context,
 	}
 
 	// Kube-Proxy Replacement
-	mode = "Disabled"
+	mode = "false"
 	if kpr := st.KubeProxyReplacement; kpr != nil {
-		mode = kpr.Mode
-		if f := kpr.Features; kpr.Mode != "Disabled" && f != nil {
+		mode = strings.ToLower(kpr.Mode)
+		if f := kpr.Features; f != nil {
 			if f.ExternalIPs != nil {
 				result[features.KPRExternalIPs] = features.Status{Enabled: f.ExternalIPs.Enabled}
 			}
@@ -172,7 +172,7 @@ func (ct *ConnectivityTest) extractFeaturesFromCiliumStatus(ctx context.Context,
 		}
 	}
 	result[features.KPRMode] = features.Status{
-		Enabled: mode != "Disabled",
+		Enabled: mode != "false" && mode != "disabled",
 		Mode:    mode,
 	}
 

connectivity/tests/service.go

@@ -286,7 +286,8 @@ func (s *outsideToNodePort) Run(ctx context.Context, t *check.Test) {
 	// With kube-proxy doing N/S LB it is not possible to see the original client
 	// IP, as iptables rules do the LB SNAT/DNAT before the packet hits any
 	// of Cilium's datapath BPF progs. So, skip the flow validation in that case.
-	_, validateFlows := t.Context().Feature(features.KPRNodePort)
+	status, ok := t.Context().Feature(features.KPRNodePort)
+	validateFlows := ok && status.Enabled
 
 	for _, svc := range t.Context().EchoServices() {
 		for _, node := range t.Context().Nodes() {

hubble/hubble.go

@@ -48,7 +48,7 @@ func EnableWithHelm(ctx context.Context, k8sClient *k8s.Client, params Parameter
 			fmt.Sprintf("hubble.ui.enabled=%t", params.UI),
 		},
 	}
-	vals, err := helm.MergeVals(options, nil, nil, nil)
+	vals, err := helm.MergeVals(options, nil)
 	if err != nil {
 		return err
 	}
@@ -67,7 +67,7 @@ func DisableWithHelm(ctx context.Context, k8sClient *k8s.Client, params Paramete
 	options := values.Options{
 		Values: []string{"hubble.relay.enabled=false", "hubble.ui.enabled=false"},
 	}
-	vals, err := helm.MergeVals(options, nil, nil, nil)
+	vals, err := helm.MergeVals(options, nil)
 	if err != nil {
 		return err
 	}

install/autodetect.go

@@ -11,7 +11,9 @@ import (
 
 	"github.com/cilium/cilium-cli/k8s"
 
+	"github.com/cilium/cilium/pkg/versioncheck"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 type validationCheck interface {
@@ -39,25 +41,13 @@ func (p Parameters) checkDisabled(name string) bool {
 	return false
 }
 
-func (k *K8sInstaller) detectDatapathMode() error {
+func (k *K8sInstaller) detectDatapathMode(helmValues map[string]interface{}) error {
 	if k.params.DatapathMode != "" {
 		k.Log("ℹ️  Custom datapath mode: %s", k.params.DatapathMode)
 		return nil
 	}
 
-	vals, err := k.getHelmValues()
-	if err != nil {
-		return err
-	}
-
-	routingMode := ""
-	for _, val := range vals {
-		val, ok := val.(string)
-		if ok && strings.HasPrefix(val, "routingMode") {
-			routingMode = strings.Split(val, "=")[1]
-		}
-
-	}
+	routingMode, _, _ := unstructured.NestedString(helmValues, "routingMode")
 	if routingMode == "native" {
 		k.params.DatapathMode = DatapathNative
 		return nil
@@ -105,14 +95,7 @@ func (k *K8sInstaller) autodetect(ctx context.Context) {
 }
 
 func getClusterName(helmValues map[string]interface{}) string {
-	cluster, ok := helmValues["cluster"].(map[string]interface{})
-	if !ok {
-		return ""
-	}
-	clusterName, ok := cluster["name"].(string)
-	if !ok {
-		return ""
-	}
+	clusterName, _, _ := unstructured.NestedString(helmValues, "cluster", "name")
 	return clusterName
 }
 
@@ -152,15 +135,15 @@ func (k *K8sInstaller) autodetectAndValidate(ctx context.Context, helmValues map
 		k.Log("ℹ️  Using cluster name %q", k.params.ClusterName)
 	}
 
-	if err := k.detectDatapathMode(); err != nil {
+	if err := k.detectDatapathMode(helmValues); err != nil {
 		return err
 	}
 
-	k.autodetectKubeProxy(ctx)
-	return k.autoEnableBPFMasq()
+	k.autodetectKubeProxy(ctx, helmValues)
+	return nil
 }
 
-func (k *K8sInstaller) autodetectKubeProxy(ctx context.Context) error {
+func (k *K8sInstaller) autodetectKubeProxy(ctx context.Context, helmValues map[string]interface{}) error {
 	if k.flavor.Kind == k8s.KindK3s {
 		return nil
 	}
@@ -215,49 +198,25 @@ func (k *K8sInstaller) autodetectKubeProxy(ctx context.Context) error {
 	if apiServerHost != "" && apiServerPort != "" {
 		k.Log("🔮 Auto-detected kube-proxy has not been installed")
 		k.Log("ℹ️  Cilium will fully replace all functionalities of kube-proxy")
-		// Use HelmOpts to set auto kube-proxy installation
-		k.params.HelmOpts.Values = append(k.params.HelmOpts.Values,
-			"kubeProxyReplacement=strict",
-			fmt.Sprintf("k8sServiceHost=%s", apiServerHost),
-			fmt.Sprintf("k8sServicePort=%s", apiServerPort))
-	}
-
-	return nil
-}
 
-func (k *K8sInstaller) autoEnableBPFMasq() error {
-	vals, err := k.getHelmValues()
-	if err != nil {
-		return err
-	}
-
-	// Auto-enable BPF masquerading if KPR=strict and IPv6=disabled
-	foundKPRStrict := false
-	foundMasq := false
-	enabledIPv6 := false
-	for _, param := range vals {
-		param, ok := param.(string)
-		if !ok {
-			continue
+		setIfUnset := func(key, value string) {
+			_, found, _ := unstructured.NestedFieldNoCopy(helmValues, key)
+			if !found {
+				k.params.HelmOpts.Values = append(k.params.HelmOpts.Values,
+					fmt.Sprintf("%s=%s", key, value))
+			}
 		}
 
-		if !foundKPRStrict && param == "kubeProxyReplacement=strict" {
-			foundKPRStrict = true
-			continue
-		}
-		if strings.HasPrefix(param, "bpf.masquerade") {
-			foundMasq = true
-			break
-		}
-		if strings.HasPrefix(param, "ipv6.enabled=true") {
-			enabledIPv6 = true
-			break
-		}
-	}
+		// Use HelmOpts to set auto kube-proxy installation
+		setIfUnset("kubeProxyReplacement", func() string {
+			if !versioncheck.MustCompile(">=1.14.0")(k.chartVersion) {
+				return "true"
+			}
+			return "strict"
+		}())
 
-	if foundKPRStrict && !foundMasq && !enabledIPv6 {
-		k.params.HelmOpts.Values = append(k.params.HelmOpts.Values,
-			"bpf.masquerade=true")
+		setIfUnset("k8sServiceHost", apiServerHost)
+		setIfUnset("k8sServicePort", apiServerPort)
 	}
 
 	return nil

install/azure.go

@@ -9,6 +9,7 @@ import (
 
 	"helm.sh/helm/v3/pkg/cli"
 	"helm.sh/helm/v3/pkg/getter"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 type accountInfo struct {
@@ -83,14 +84,8 @@ func (k *K8sInstaller) setAzureResourceGroupFromHelmValue() error {
 	if err != nil {
 		return err
 	}
-	azure, ok := values["azure"].(map[string]interface{})
-	if !ok {
-		return nil
-	}
-	resourceGroupName, ok := azure["resourceGroup"].(string)
-	if !ok {
-		return nil
-	}
+
+	resourceGroupName, _, _ := unstructured.NestedString(values, "azure", "resourceGroup")
 	if resourceGroupName != "" {
 		k.params.Azure.ResourceGroupName = resourceGroupName
 	}

install/helm.go

@@ -17,13 +17,9 @@ import (
 
 func (k *K8sInstaller) getHelmValues() (map[string]interface{}, error) {
 	helmMapOpts := map[string]string{}
-	deprecatedCfgOpts := map[string]string{}
 
 	switch {
-	// It's likely that certain helm options have changed since 1.9.0
-	// These were tested for the >=1.11.0. In case something breaks for versions
-	// older than 1.11.0 we will fix it afterwards.
-	case versioncheck.MustCompile(">=1.9.0")(k.chartVersion):
+	case versioncheck.MustCompile(">=1.12.0")(k.chartVersion):
 		// TODO(aanm) to keep the previous behavior unchanged we will set the number
 		// of the operator replicas to 1. Ideally this should be the default in the helm chart
 		helmMapOpts["operator.replicas"] = "1"
@@ -92,25 +88,13 @@ func (k *K8sInstaller) getHelmValues() (map[string]interface{}, error) {
 				// Can be removed once we drop support for <1.14.0
 				helmMapOpts["tunnel"] = tunnelDisabled
 			}
-			switch {
-			case versioncheck.MustCompile(">=1.10.0")(k.chartVersion):
-				helmMapOpts["bpf.masquerade"] = "false"
-				helmMapOpts["enableIPv4Masquerade"] = "false"
-				helmMapOpts["enableIPv6Masquerade"] = "false"
-			case versioncheck.MustCompile(">=1.9.0")(k.chartVersion):
-				helmMapOpts["masquerade"] = "false"
-			}
+
+			helmMapOpts["bpf.masquerade"] = "false"
+			helmMapOpts["enableIPv4Masquerade"] = "false"
+			helmMapOpts["enableIPv6Masquerade"] = "false"
 
 		case DatapathAKSBYOCNI:
-			switch {
-			case versioncheck.MustCompile(">=1.12.0")(k.chartVersion):
-				helmMapOpts["aksbyocni.enabled"] = "true"
-			case versioncheck.MustCompile(">=1.9.0")(k.chartVersion):
-				// Manually configure the same ConfigMap options as the new
-				// `aksbyocni` mode does, so that it works transparently.
-				helmMapOpts["ipam.mode"] = ipamClusterPool
-				helmMapOpts["tunnel"] = tunnelVxlan
-			}
+			helmMapOpts["aksbyocni.enabled"] = "true"
 		}
 
 		if k.params.ClusterName != "" {
@@ -120,14 +104,7 @@ func (k *K8sInstaller) getHelmValues() (map[string]interface{}, error) {
 		// TODO: remove when removing "ipv4-native-routing-cidr" flag (marked as
 		// deprecated), kept for backwards compatibility
 		if k.params.IPv4NativeRoutingCIDR != "" {
-			// NOTE: Cilium v1.11 replaced --native-routing-cidr by
-			// --ipv4-native-routing-cidr
-			switch {
-			case versioncheck.MustCompile(">=1.11.0")(k.chartVersion):
-				helmMapOpts["ipv4NativeRoutingCIDR"] = k.params.IPv4NativeRoutingCIDR
-			case versioncheck.MustCompile(">=1.9.0")(k.chartVersion):
-				helmMapOpts["nativeRoutingCIDR"] = k.params.IPv4NativeRoutingCIDR
-			}
+			helmMapOpts["ipv4NativeRoutingCIDR"] = k.params.IPv4NativeRoutingCIDR
 		}
 
 	default:
@@ -142,11 +119,5 @@ func (k *K8sInstaller) getHelmValues() (map[string]interface{}, error) {
 		k.params.HelmOpts.StringValues = append(k.params.HelmOpts.StringValues, defaults.SpireAgentScheduleAffinity...)
 	}
 
-	// Store all the options passed by --config into helm extraConfig
-	extraConfigMap := map[string]interface{}{}
-	for k, v := range deprecatedCfgOpts {
-		extraConfigMap[k] = v
-	}
-
-	return helm.MergeVals(k.params.HelmOpts, helmMapOpts, nil, extraConfigMap)
+	return helm.MergeVals(k.params.HelmOpts, helmMapOpts)
 }

install/install.go

@@ -18,6 +18,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/yaml"
 
@@ -176,14 +177,7 @@ func (k *K8sInstaller) listVersions() error {
 }
 
 func getChainingMode(values map[string]interface{}) string {
-	cni, ok := values["cni"].(map[string]interface{})
-	if !ok {
-		return ""
-	}
-	chainingMode, ok := cni["chainingMode"].(string)
-	if !ok {
-		return ""
-	}
+	chainingMode, _, _ := unstructured.NestedString(values, "cni", "chainingMode")
 	return chainingMode
 }
 

internal/helm/helm.go

@@ -23,7 +23,6 @@ import (
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
-	"helm.sh/helm/v3/pkg/chartutil"
 	"helm.sh/helm/v3/pkg/cli"
 	"helm.sh/helm/v3/pkg/cli/values"
 	"helm.sh/helm/v3/pkg/getter"
@@ -132,17 +131,13 @@ func ciliumCacheDir() (string, error) {
 	return res, nil
 }
 
-// MergeVals merges all values from flag options ('helmFlagOpts'),
+// MergeVals merges all values from flag options ('helmFlagOpts') and
 // auto-generated helm options based on environment ('helmMapOpts'),
-// helm values from a previous installation ('helmValues'),
-// extra options that are not defined as helm flags ('extraConfigMapOpts')
 // and returns a single map with all of these options merged.
-// Both 'helmMapOpts', 'helmValues', 'extraConfigMapOpts', can be nil.
+// 'helmMapOpts' can be nil.
 func MergeVals(
 	helmFlagOpts values.Options,
 	helmMapOpts map[string]string,
-	helmValues,
-	extraConfigMapOpts chartutil.Values,
 ) (map[string]interface{}, error) {
 
 	// Create helm values from helmMapOpts
@@ -153,9 +148,7 @@ func MergeVals(
 
 	helmOptsStr := strings.Join(helmOpts, ",")
 
-	if helmValues == nil {
-		helmValues = map[string]interface{}{}
-	}
+	helmValues := map[string]interface{}{}
 	err := strvals.ParseInto(helmOptsStr, helmValues)
 	if err != nil {
 		return nil, fmt.Errorf("error parsing helm options %q: %w", helmOptsStr, err)
@@ -169,19 +162,7 @@ func MergeVals(
 	}
 
 	// User-defined helm options will overwrite the default cilium-cli helm options
-	userVals = mergeMaps(helmValues, userVals)
-
-	// Merge the user-defined helm options into the `--config` map. This
-	// effectively means that any --set=extraConfig.<key> will overwrite
-	// the values of --config <key>
-	extraConfig := map[string]interface{}{}
-	if len(extraConfigMapOpts) != 0 {
-		extraConfig["extraConfig"] = extraConfigMapOpts
-	}
-
-	vals := mergeMaps(extraConfig, userVals)
-
-	return vals, nil
+	return mergeMaps(helmValues, userVals), nil
 }
 
 // ParseVals takes a slice of Helm values of the form