Merge pull request #2213 from sawsa307/cherry-pick-1.23

Cherry pick 1.23
kubernetes · Jul 29, 2023 · 229cb90 · 229cb90
2 parents 2092c7c + 30a670b
commit 229cb90
Show file tree

Hide file tree

Showing 5 changed files with 175 additions and 43 deletions.
diff --git a/pkg/backends/features/securitypolicy.go b/pkg/backends/features/securitypolicy.go
@@ -31,30 +31,52 @@ 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 {
-	var desiredPolicyName string
-	if sp.BackendConfig.Spec.SecurityPolicy != nil {
-		desiredPolicyName = sp.BackendConfig.Spec.SecurityPolicy.Name
-	} else {
-		desiredPolicyName = ""
+	// It is too dangerous to remove user's security policy that may have been
+	// configured via the UI or gcloud directly rather than via Kubernetes.
+	// Treat nil security policy -> ignored
+	// Treat empty string security policy name -> remove
+	if sp.BackendConfig.Spec.SecurityPolicy == nil {
+		klog.V(2).Infof("Ignoring nil Security Policy on backend service %s (%s:%s)", be.Name, sp.ID.Service.String(), sp.ID.Port.String())
+		return nil
+	}
+
+	if be.Scope != meta.Global {
+		err := fmt.Errorf("cloud armor security policies not supported for %s backend service %s", be.Scope, be.Name)
+		klog.Errorf("EnsureSecurityPolicy() = %v", err)
+		return err
 	}
 
 	existingPolicyName, err := utils.KeyName(be.SecurityPolicy)
 	// The parser returns error for empty values.
 	if be.SecurityPolicy != "" && err != nil {
+		err := fmt.Errorf("failed to parse existing security policy name %q: %v", existingPolicyName, err)
+		klog.Errorf("EnsureSecurityPolicy() = %v", err)
 		return err
 	}
 
+	desiredPolicyName := sp.BackendConfig.Spec.SecurityPolicy.Name
+	klog.V(2).Infof("Current security policy: %q, desired security policy: %q", existingPolicyName, desiredPolicyName)
 	if existingPolicyName == desiredPolicyName {
+		klog.V(2).Infof("SecurityPolicy on backend service is not changed %s (%s:%s): %q", be.Name, sp.ID.Service.String(), sp.ID.Port.String(), 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)
+	if desiredPolicyName != "" {
+		klog.V(2).Infof("Set security policy in backend service %s (%s:%s) from %q to %q", be.Name, sp.ID.Service.String(), sp.ID.Port.String(), existingPolicyName, desiredPolicyName)
+		if err := composite.SetSecurityPolicy(cloud, be, desiredPolicyName); err != nil {
+			err := fmt.Errorf("failed to set security policy from %q to %q for backend service %s (%s:%s): %v", existingPolicyName, desiredPolicyName, be.Name, sp.ID.Service.String(), sp.ID.Port.String(), err)
+			klog.Errorf("SetSecurityPolicy() = %v", err)
+			return err
+		}
+		klog.V(2).Infof("Successfully set security policy in backend service %s (%s:%s) from %q to %q", be.Name, sp.ID.Service.String(), sp.ID.Port.String(), existingPolicyName, desiredPolicyName)
+		return nil
 	}
-
-	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)
+	klog.V(2).Infof("Removing security policy %q in backend service %s (%s:%s)", existingPolicyName, be.Name, sp.ID.Service.String(), sp.ID.Port.String())
 	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)
+		err := fmt.Errorf("failed to remove security policy %q for backend service %s (%s:%s): %v", existingPolicyName, be.Name, sp.ID.Service.String(), sp.ID.Port.String(), err)
+		klog.Errorf("SetSecurityPolicy() = %v", err)
+		return err
 	}
+	klog.V(2).Infof("Successfully removed security policy %q in backend service %s (%s:%s)", existingPolicyName, be.Name, sp.ID.Service.String(), sp.ID.Port.String())
 	return nil
 }
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: "unset-policy-empty-policy-string",
+			desc: "remove policy with empty string policy name",
 			currentBackendService: &composite.BackendService{
 				Scope:          meta.Global,
 				SecurityPolicy: "https://www.googleapis.com/compute/projects/test-project/global/securityPolicies/policy-1",
@@ -92,15 +92,6 @@ 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{
@@ -116,10 +107,21 @@ func TestEnsureSecurityPolicy(t *testing.T) {
 			},
 		},
 		{
-			desc:                  "empty-policy",
+			desc:                  "nil policy should have no effect with policy not configured",
 			currentBackendService: &composite.BackendService{Scope: meta.Global},
 			desiredConfig:         &backendconfigv1.BackendConfig{},
 		},
+		// This is intentional that nil policy have no effect on current security policy.
+		// Please be careful on this.
+		{
+			desc: "nil policy should have no effect with policy configured",
+			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,13 +137,6 @@ 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 {

diff --git a/pkg/metrics/features.go b/pkg/metrics/features.go
@@ -80,6 +80,9 @@ const (
 	// BackendConfig Features
 	cloudCDN                  = feature("CloudCDN")
 	cloudArmor                = feature("CloudArmor")
+	cloudArmorSet             = feature("CloudArmorSet")
+	cloudArmorEmpty           = feature("CloudArmorEmptyString")
+	cloudArmorNil             = feature("CloudArmorNil")
 	cloudIAP                  = feature("CloudIAP")
 	backendTimeout            = feature("BackendTimeout")
 	backendConnectionDraining = feature("BackendConnectionDraining")
@@ -311,10 +314,27 @@ func featuresForServicePort(sp utils.ServicePort) []feature {
 		}
 		klog.V(6).Infof("Session affinity %s is configured for service port %s", affinityType, svcPortKey)
 	}
+
 	if sp.BackendConfig.Spec.SecurityPolicy != nil {
 		klog.V(6).Infof("Security policy %s is configured for service port %s", sp.BackendConfig.Spec.SecurityPolicy, svcPortKey)
 		features = append(features, cloudArmor)
 	}
+
+	// Detailed metrics about cloud armor.
+	if sp.BackendConfig.Spec.SecurityPolicy != nil {
+		var caFeature feature
+		if sp.BackendConfig.Spec.SecurityPolicy.Name == "" {
+			caFeature = cloudArmorEmpty
+		} else {
+			caFeature = cloudArmorSet
+		}
+		features = append(features, caFeature)
+		klog.V(6).Infof("Security policy %s is configured for service port %s (%s)", sp.BackendConfig.Spec.SecurityPolicy, svcPortKey, caFeature)
+	} else {
+		features = append(features, cloudArmorNil)
+		klog.V(6).Infof("Security policy is configured to nil for service port %s (%s)", sp.BackendConfig.Spec.SecurityPolicy, svcPortKey)
+	}
+
 	if sp.BackendConfig.Spec.TimeoutSec != nil {
 		klog.V(6).Infof("Backend timeout(%v secs) is configured for service port %s", sp.BackendConfig.Spec.TimeoutSec, svcPortKey)
 		features = append(features, backendTimeout)

diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go
@@ -765,6 +765,9 @@ func initializeCounts() (map[feature]int, map[feature]int) {
 			neg:                       0,
 			cloudCDN:                  0,
 			cloudArmor:                0,
+			cloudArmorSet:             0,
+			cloudArmorEmpty:           0,
+			cloudArmorNil:             0,
 			cloudIAP:                  0,
 			backendTimeout:            0,
 			backendConnectionDraining: 0,
@@ -795,6 +798,9 @@ func isServiceFeature(ftr feature) bool {
 		servicePort:         true,
 		externalServicePort: true,
 		internalServicePort: true,
+		cloudArmorEmpty:     true,
+		cloudArmorNil:       true,
+		cloudArmorSet:       true,
 	}
 	return serviceFeatures[ftr]
 }