tmp: addressing review comments

- Enhanced TestUnsupportedConfigOverride test for DCM case. - multiple settings can be specified at once. - expected environment state can be different from initial. - initial and expected environment state may have multiple variables. - Removed DCM config override tests from deployment test. - Changed the condition to enable DCM from feature gate and config override to a more readbale one.
openshift · Oct 24, 2024 · 7717e17 · 7717e17
1 parent 75e90cd
commit 7717e17
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 61 deletions.
diff --git a/pkg/operator/controller/ingress/deployment.go b/pkg/operator/controller/ingress/deployment.go
@@ -571,10 +571,17 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController
 		})
 	}
 
+	enableDCM := false
+	if ingressControllerDCMEnabled {
+		enableDCM = true
+	}
 	dynamicConfigOverride := unsupportedConfigOverrides.DynamicConfigManager
-	if v, err := strconv.ParseBool(dynamicConfigOverride); (err == nil && v) || (err != nil && ingressControllerDCMEnabled) {
-		// Enable DCM only if user requested it explicitly via config override
-		// or if ingressControllerDCMEnabled is enabled.
+	if v, err := strconv.ParseBool(dynamicConfigOverride); err == nil {
+		// Config override can still be used to opt out from DCM.
+		enableDCM = v
+	}
+
+	if enableDCM {
 		env = append(env, corev1.EnvVar{
 			Name:  RouterHAProxyConfigManager,
 			Value: "true",

diff --git a/pkg/operator/controller/ingress/deployment_test.go b/pkg/operator/controller/ingress/deployment_test.go
@@ -809,54 +809,6 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) {
 	}
 
 	checkDeploymentHasEnvSorted(t, deployment)
-
-	// Set max dynamic server when DCM feature gate is on.
-	ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
-		Raw: []byte(`{"maxDynamicServers":"7"}`),
-	}
-	deployment, err = desiredRouterDeployment(ic, ingressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil, clusterProxyConfig, false, true)
-	if err != nil {
-		t.Fatalf("invalid router Deployment: %v", err)
-	}
-	tests = []envData{
-		{"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"},
-		{"ROUTER_MAX_DYNAMIC_SERVERS", true, "7"},
-	}
-	if err := checkDeploymentEnvironment(t, deployment, tests); err != nil {
-		t.Error(err)
-	}
-
-	// Set max dynamic server to a wrong value when DCM feature gate is on.
-	ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
-		Raw: []byte(`{"maxDynamicServers":"wrong"}`),
-	}
-	deployment, err = desiredRouterDeployment(ic, ingressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil, clusterProxyConfig, false, true)
-	if err != nil {
-		t.Fatalf("invalid router Deployment: %v", err)
-	}
-	tests = []envData{
-		{"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"},
-		{"ROUTER_MAX_DYNAMIC_SERVERS", true, "1"},
-	}
-	if err := checkDeploymentEnvironment(t, deployment, tests); err != nil {
-		t.Error(err)
-	}
-
-	// Opt out from DCM via unsupportedConfigOverride.
-	ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
-		Raw: []byte(`{"dynamicConfigManager":"false", "maxDynamicServers":"7"}`),
-	}
-	deployment, err = desiredRouterDeployment(ic, ingressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil, clusterProxyConfig, false, true)
-	if err != nil {
-		t.Fatalf("invalid router Deployment: %v", err)
-	}
-	tests = []envData{
-		{"ROUTER_HAPROXY_CONFIG_MANAGER", false, ""},
-		{"ROUTER_MAX_DYNAMIC_SERVERS", false, "0"},
-	}
-	if err := checkDeploymentEnvironment(t, deployment, tests); err != nil {
-		t.Error(err)
-	}
 }
 
 func TestDesiredRouterDeploymentVariety(t *testing.T) {

diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go
@@ -3423,12 +3423,72 @@ func TestLoadBalancingAlgorithmUnsupportedConfigOverride(t *testing.T) {
 func TestUnsupportedConfigOverride(t *testing.T) {
 	t.Parallel()
 
+	type envVar struct {
+		name  string
+		value string
+	}
+
 	var tests = []struct {
-		name, unsupportedConfigOverride, env, defaultValue, value string
+		name                      string
+		initialEnv                []envVar
+		unsupportedConfigOverride string
+		expectedEnv               []envVar
 	}{
-		{"dynamic-config-manager", "dynamicConfigManager", "ROUTER_HAPROXY_CONFIG_MANAGER", "true", "false"},
-		{"contstats", "contStats", "ROUTER_HAPROXY_CONTSTATS", "", "true"},
-		{"max-dynamic-servers", "maxDynamicServers", "ROUTER_MAX_DYNAMIC_SERVERS", "2", "7"},
+		{
+			name: "contstats",
+			initialEnv: []envVar{
+				{
+					name: "ROUTER_HAPROXY_CONTSTATS",
+				},
+			},
+			unsupportedConfigOverride: `{"contStats":"true"}`,
+			expectedEnv: []envVar{
+				{
+					name:  "ROUTER_HAPROXY_CONTSTATS",
+					value: "true",
+				},
+			},
+		},
+		{
+			name:                      "dynamic-config-manager",
+			unsupportedConfigOverride: `{"dynamicConfigManager":"true","maxDynamicServers":"7"}`,
+			expectedEnv: []envVar{
+				{
+					name:  "ROUTER_HAPROXY_CONFIG_MANAGER",
+					value: "true",
+				},
+				{
+					name:  "ROUTER_MAX_DYNAMIC_SERVERS",
+					value: "7",
+				},
+			},
+		},
+		{
+			name:                      "max-dynamic-servers-no-dcm",
+			unsupportedConfigOverride: `{"dynamicConfigManager":"false","maxDynamicServers":"7"}`,
+			expectedEnv: []envVar{
+				{
+					name: "ROUTER_HAPROXY_CONFIG_MANAGER",
+				},
+				{
+					name: "ROUTER_MAX_DYNAMIC_SERVERS",
+				},
+			},
+		},
+		{
+			name:                      "max-dynamic-servers-invalid",
+			unsupportedConfigOverride: `{"dynamicConfigManager":"true","maxDynamicServers":"invalid"}`,
+			expectedEnv: []envVar{
+				{
+					name:  "ROUTER_HAPROXY_CONFIG_MANAGER",
+					value: "true",
+				},
+				{
+					name:  "ROUTER_MAX_DYNAMIC_SERVERS",
+					value: "1",
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -3449,22 +3509,25 @@ func TestUnsupportedConfigOverride(t *testing.T) {
 			if err := kclient.Get(context.TODO(), controller.RouterDeploymentName(ic), deployment); err != nil {
 				t.Fatalf("failed to get ingresscontroller deployment: %v", err)
 			}
-			if err := waitForDeploymentEnvVar(t, kclient, deployment, 30*time.Second, tt.env, tt.defaultValue); err != nil {
-				t.Fatalf("expected initial deployment not to set %s=%s: %v", tt.env, tt.defaultValue, err)
+			for _, env := range tt.initialEnv {
+				if err := waitForDeploymentEnvVar(t, kclient, deployment, 30*time.Second, env.name, env.value); err != nil {
+					t.Fatalf("expected initial deployment not to set %s=%s: %v", env.name, env.value, err)
+				}
 			}
 
 			if err := updateIngressControllerWithRetryOnConflict(t, icName, timeout, func(ic *operatorv1.IngressController) {
 				ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
-					Raw: []byte(fmt.Sprintf(`{"%s":"%s"}`, tt.unsupportedConfigOverride, tt.value)),
+					Raw: []byte(tt.unsupportedConfigOverride),
 				}
 			}); err != nil {
 				t.Fatalf("failed to update ingresscontroller: %v", err)
 			}
-			if err := waitForDeploymentEnvVar(t, kclient, deployment, 1*time.Minute, tt.env, tt.value); err != nil {
-				t.Fatalf("expected updated deployment to set %s=%s: %v", tt.env, tt.value, err)
+			for _, env := range tt.expectedEnv {
+				if err := waitForDeploymentEnvVar(t, kclient, deployment, 1*time.Minute, env.name, env.value); err != nil {
+					t.Fatalf("expected updated deployment not to set %s=%s: %v", env.name, env.value, err)
+				}
 			}
 		})
-
 	}
 }