Merge pull request #1749 from songrx1997/fixSecurityPolicy

Fix BackendConfig.securityPolicy is not removed after config updates
kubernetes · Aug 10, 2022 · c926ab5 · c926ab5
2 parents 74ad470 + 0289885
commit c926ab5
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 17 deletions.
diff --git a/pkg/backends/features/securitypolicy.go b/pkg/backends/features/securitypolicy.go
@@ -31,24 +31,27 @@ import (
 // EnsureSecurityPolicy ensures the security policy link on backend service.
 // TODO(mrhohn): Emit event when attach/detach security policy to backend service.
 func EnsureSecurityPolicy(cloud *gce.Cloud, sp utils.ServicePort, be *composite.BackendService) error {
-	if sp.BackendConfig.Spec.SecurityPolicy == nil {
-		return nil
-	}
-
-	if be.Scope != meta.Global {
-		return fmt.Errorf("cloud armor security policies not supported for %s backend service %s", be.Scope, be.Name)
+	var desiredPolicyName string
+	if sp.BackendConfig.Spec.SecurityPolicy != nil {
+		desiredPolicyName = sp.BackendConfig.Spec.SecurityPolicy.Name
+	} else {
+		desiredPolicyName = ""
 	}
 
 	existingPolicyName, err := utils.KeyName(be.SecurityPolicy)
 	// The parser returns error for empty values.
 	if be.SecurityPolicy != "" && err != nil {
 		return err
 	}
-	desiredPolicyName := sp.BackendConfig.Spec.SecurityPolicy.Name
+
 	if existingPolicyName == desiredPolicyName {
 		return nil
 	}
 
+	if be.Scope != meta.Global {
+		return fmt.Errorf("cloud armor security policies not supported for %s backend service %s", be.Scope, be.Name)
+	}
+
 	klog.V(2).Infof("Set security policy in backend service %s (%s:%s) to %q", be.Name, sp.ID.Service.String(), sp.ID.Port.String(), desiredPolicyName)
 	if err := composite.SetSecurityPolicy(cloud, be, desiredPolicyName); err != nil {
 		return fmt.Errorf("failed to set security policy %q for backend service %s (%s:%s): %v", desiredPolicyName, be.Name, sp.ID.Service.String(), sp.ID.Port.String(), err)

diff --git a/pkg/backends/features/securitypolicy_test.go b/pkg/backends/features/securitypolicy_test.go
@@ -78,7 +78,7 @@ func TestEnsureSecurityPolicy(t *testing.T) {
 			expectSetCall: true,
 		},
 		{
-			desc: "remove-policy",
+			desc: "unset-policy-empty-policy-string",
 			currentBackendService: &composite.BackendService{
 				Scope:          meta.Global,
 				SecurityPolicy: "https://www.googleapis.com/compute/projects/test-project/global/securityPolicies/policy-1",
@@ -92,6 +92,15 @@ func TestEnsureSecurityPolicy(t *testing.T) {
 			},
 			expectSetCall: true,
 		},
+		{
+			desc: "unset-policy-no-specified-policy",
+			currentBackendService: &composite.BackendService{
+				Scope:          meta.Global,
+				SecurityPolicy: "https://www.googleapis.com/compute/projects/test-project/global/securityPolicies/policy-1",
+			},
+			desiredConfig: &backendconfigv1.BackendConfig{},
+			expectSetCall: true,
+		},
 		{
 			desc: "same-policy",
 			currentBackendService: &composite.BackendService{
@@ -111,15 +120,6 @@ func TestEnsureSecurityPolicy(t *testing.T) {
 			currentBackendService: &composite.BackendService{Scope: meta.Global},
 			desiredConfig:         &backendconfigv1.BackendConfig{},
 		},
-		{
-			desc: "no-specified-policy",
-			currentBackendService: &composite.BackendService{
-				Scope:          meta.Global,
-				SecurityPolicy: "https://www.googleapis.com/compute/projects/test-project/global/securityPolicies/policy-1",
-			},
-			desiredConfig: &backendconfigv1.BackendConfig{},
-			expectSetCall: false,
-		},
 		{
 			desc: "regional backend service",
 			currentBackendService: &composite.BackendService{
@@ -135,6 +135,13 @@ func TestEnsureSecurityPolicy(t *testing.T) {
 			expectSetCall: false,
 			expectError:   true,
 		},
+		{
+			desc:                  "regional backend service with no specified security policy",
+			currentBackendService: &composite.BackendService{Scope: meta.Regional},
+			desiredConfig:         &backendconfigv1.BackendConfig{},
+			expectSetCall:         false,
+			expectError:           false,
+		},
 	}
 
 	for i, tc := range testCases {