diff --git a/pkg/backends/features/securitypolicy.go b/pkg/backends/features/securitypolicy.go index 0840285dff..8d65906303 100644 --- 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 index e27d4f4bef..7574acb1d4 100644 --- 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 index 1976617433..f852970bd9 100644 --- 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 index f2ae640ac2..d0030f0fb1 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -623,6 +623,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, @@ -653,6 +656,9 @@ func isServiceFeature(ftr feature) bool { servicePort: true, externalServicePort: true, internalServicePort: true, + cloudArmorEmpty: true, + cloudArmorNil: true, + cloudArmorSet: true, } return serviceFeatures[ftr] } diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go index c5def5371b..20f3e2d9d5 100644 --- a/pkg/metrics/metrics_test.go +++ b/pkg/metrics/metrics_test.go @@ -126,6 +126,34 @@ var ( }, }, }, + { + ID: utils.ServicePortID{ + Service: types.NamespacedName{ + Name: "service-with-empty-security-policy", + Namespace: defaultNamespace, + }, + Port: v1.ServiceBackendPort{Number: 80}, + }, + BackendConfig: &backendconfigv1.BackendConfig{ + Spec: backendconfigv1.BackendConfigSpec{ + SecurityPolicy: &backendconfigv1.SecurityPolicyConfig{ + Name: "", + }, + }, + }, + }, + { + ID: utils.ServicePortID{ + Service: types.NamespacedName{ + Name: "service-with-no-security-policy", + Namespace: defaultNamespace, + }, + Port: v1.ServiceBackendPort{Number: 80}, + }, + BackendConfig: &backendconfigv1.BackendConfig{ + Spec: backendconfigv1.BackendConfigSpec{}, + }, + }, } ingressStates = []struct { desc string @@ -186,7 +214,7 @@ var ( []feature{ingress, externalIngress, httpEnabled}, []utils.ServicePort{testServicePorts[0]}, []feature{servicePort, externalServicePort, cloudCDN, - cookieAffinity, cloudArmor, backendConnectionDraining, customHealthChecks}, + cookieAffinity, cloudArmor, cloudArmorSet, backendConnectionDraining, customHealthChecks}, }, { "host rule only", @@ -245,7 +273,7 @@ var ( hostBasedRouting, pathBasedRouting}, []utils.ServicePort{testServicePorts[1]}, []feature{servicePort, externalServicePort, neg, cloudIAP, - clientIPAffinity, backendTimeout, customRequestHeaders}, + clientIPAffinity, cloudArmorNil, backendTimeout, customRequestHeaders}, }, { "default backend and host rule", @@ -292,8 +320,8 @@ var ( hostBasedRouting, pathBasedRouting}, testServicePorts[:2], []feature{servicePort, externalServicePort, cloudCDN, - cookieAffinity, cloudArmor, backendConnectionDraining, customHealthChecks, neg, cloudIAP, - clientIPAffinity, backendTimeout, customRequestHeaders}, + cookieAffinity, cloudArmor, cloudArmorSet, backendConnectionDraining, customHealthChecks, neg, cloudIAP, + clientIPAffinity, cloudArmorNil, backendTimeout, customRequestHeaders}, }, { "tls termination with pre-shared certs", @@ -323,7 +351,7 @@ var ( tlsTermination, preSharedCertsForTLS}, []utils.ServicePort{testServicePorts[0]}, []feature{servicePort, externalServicePort, cloudCDN, - cookieAffinity, cloudArmor, backendConnectionDraining, customHealthChecks}, + cookieAffinity, cloudArmor, cloudArmorSet, backendConnectionDraining, customHealthChecks}, }, { "tls termination with google managed certs", @@ -353,7 +381,7 @@ var ( tlsTermination, managedCertsForTLS}, []utils.ServicePort{testServicePorts[0]}, []feature{servicePort, externalServicePort, cloudCDN, - cookieAffinity, cloudArmor, backendConnectionDraining, customHealthChecks}, + cookieAffinity, cloudArmor, cloudArmorSet, backendConnectionDraining, customHealthChecks}, }, { "tls termination with pre-shared and google managed certs", @@ -384,7 +412,7 @@ var ( tlsTermination, preSharedCertsForTLS, managedCertsForTLS}, []utils.ServicePort{testServicePorts[0]}, []feature{servicePort, externalServicePort, cloudCDN, - cookieAffinity, cloudArmor, backendConnectionDraining, customHealthChecks}, + cookieAffinity, cloudArmor, cloudArmorSet, backendConnectionDraining, customHealthChecks}, }, { "tls termination with pre-shared and secret based certs", @@ -433,7 +461,7 @@ var ( pathBasedRouting, tlsTermination, preSharedCertsForTLS, secretBasedCertsForTLS}, []utils.ServicePort{testServicePorts[1]}, []feature{servicePort, externalServicePort, neg, cloudIAP, - clientIPAffinity, backendTimeout, customRequestHeaders}, + clientIPAffinity, cloudArmorNil, backendTimeout, customRequestHeaders}, }, { "global static ip", @@ -464,7 +492,7 @@ var ( tlsTermination, preSharedCertsForTLS, staticGlobalIP, managedStaticGlobalIP}, []utils.ServicePort{testServicePorts[0]}, []feature{servicePort, externalServicePort, cloudCDN, - cookieAffinity, cloudArmor, backendConnectionDraining, customHealthChecks}, + cookieAffinity, cloudArmor, cloudArmorSet, backendConnectionDraining, customHealthChecks}, }, { "default backend, host rule for internal load-balancer", @@ -515,7 +543,7 @@ var ( hostBasedRouting, pathBasedRouting}, []utils.ServicePort{testServicePorts[2], testServicePorts[3]}, []feature{servicePort, internalServicePort, neg, cloudIAP, - cookieAffinity, backendConnectionDraining}, + cookieAffinity, cloudArmorNil, backendConnectionDraining}, }, { "non-existent pre-shared cert", @@ -734,6 +762,54 @@ var ( []utils.ServicePort{}, nil, }, + { + "backend with empty security policy", + &v1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultNamespace, + Name: "ingress20", + }, + Spec: v1.IngressSpec{ + DefaultBackend: &v1.IngressBackend{ + Service: &v1.IngressServiceBackend{ + Name: "service-with-empty-security-policy", + Port: v1.ServiceBackendPort{ + Number: int32(80), + }, + }, + }, + Rules: []v1.IngressRule{}, + }, + }, + nil, + []feature{ingress, externalIngress, httpEnabled}, + []utils.ServicePort{testServicePorts[4]}, + []feature{servicePort, externalServicePort, cloudArmor, cloudArmorEmpty}, + }, + { + "backend with no security policy", + &v1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: defaultNamespace, + Name: "ingress21", + }, + Spec: v1.IngressSpec{ + DefaultBackend: &v1.IngressBackend{ + Service: &v1.IngressServiceBackend{ + Name: "service-with-no-security-policy", + Port: v1.ServiceBackendPort{ + Number: int32(80), + }, + }, + }, + Rules: []v1.IngressRule{}, + }, + }, + nil, + []feature{ingress, externalIngress, httpEnabled}, + []utils.ServicePort{testServicePorts[5]}, + []feature{servicePort, externalServicePort, cloudArmorNil}, + }, } ) @@ -824,6 +900,9 @@ func TestComputeIngressMetrics(t *testing.T) { backendTimeout: 0, clientIPAffinity: 0, cloudArmor: 0, + cloudArmorSet: 0, + cloudArmorEmpty: 0, + cloudArmorNil: 0, cloudCDN: 0, cloudIAP: 0, cookieAffinity: 0, @@ -873,6 +952,9 @@ func TestComputeIngressMetrics(t *testing.T) { backendTimeout: 0, clientIPAffinity: 0, cloudArmor: 0, + cloudArmorSet: 0, + cloudArmorEmpty: 0, + cloudArmorNil: 1, cloudCDN: 0, cloudIAP: 1, cookieAffinity: 1, @@ -924,6 +1006,9 @@ func TestComputeIngressMetrics(t *testing.T) { backendTimeout: 1, clientIPAffinity: 1, cloudArmor: 1, + cloudArmorSet: 1, + cloudArmorEmpty: 0, + cloudArmorNil: 1, cloudCDN: 1, cloudIAP: 1, cookieAffinity: 1, @@ -955,6 +1040,7 @@ func TestComputeIngressMetrics(t *testing.T) { NewIngressState(ingressStates[14].ing, ingressStates[14].fc, ingressStates[14].svcPorts), NewIngressState(ingressStates[15].ing, nil, ingressStates[15].svcPorts), NewIngressState(ingressStates[16].ing, ingressStates[16].fc, ingressStates[16].svcPorts), + NewIngressState(ingressStates[20].ing, nil, ingressStates[20].svcPorts), }, map[feature]int{ backendConnectionDraining: 7, @@ -966,10 +1052,10 @@ func TestComputeIngressMetrics(t *testing.T) { cookieAffinity: 7, customRequestHeaders: 3, customHealthChecks: 6, - externalIngress: 15, - httpEnabled: 16, + externalIngress: 16, + httpEnabled: 17, hostBasedRouting: 5, - ingress: 17, + ingress: 18, internalIngress: 2, managedCertsForTLS: 2, managedStaticGlobalIP: 1, @@ -989,14 +1075,17 @@ func TestComputeIngressMetrics(t *testing.T) { backendTimeout: 1, clientIPAffinity: 1, cloudArmor: 1, + cloudArmorSet: 1, + cloudArmorEmpty: 0, + cloudArmorNil: 3, cloudCDN: 1, cloudIAP: 2, cookieAffinity: 2, customRequestHeaders: 1, customHealthChecks: 1, internalServicePort: 2, - servicePort: 4, - externalServicePort: 2, + servicePort: 5, + externalServicePort: 3, neg: 3, }, },