From c97ce60ae4eb7f21bd82f90894a1b02c82425c48 Mon Sep 17 00:00:00 2001
From: Gaurav Ghildiyal <gauravkg@google.com>
Date: Mon, 3 Apr 2023 11:33:09 -0700
Subject: [PATCH] Set SampleRate to 0 when LogConfig is disabled for
 BackendService

---
 pkg/backends/features/logging.go      |  8 ++---
 pkg/backends/features/logging_test.go | 47 +++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/pkg/backends/features/logging.go b/pkg/backends/features/logging.go
index 528570b2b0..be8bcf4ce1 100644
--- a/pkg/backends/features/logging.go
+++ b/pkg/backends/features/logging.go
@@ -32,7 +32,7 @@ func EnsureLogging(sp utils.ServicePort, be *composite.BackendService) bool {
 		return false
 	}
 	svcKey := fmt.Sprintf("%s/%s", sp.ID.Service.Namespace, sp.ID.Service.Name)
-	if be.LogConfig != nil && !be.LogConfig.Enable && !sp.BackendConfig.Spec.Logging.Enable {
+	if be.LogConfig != nil && !be.LogConfig.Enable && !sp.BackendConfig.Spec.Logging.Enable && be.LogConfig.SampleRate == 0.0 {
 		klog.V(3).Infof("Logging continues to stay disabled for service %s (port %d), skipping update", svcKey, sp.Port)
 		return false
 	}
@@ -44,8 +44,7 @@ func EnsureLogging(sp utils.ServicePort, be *composite.BackendService) bool {
 		}
 	}
 	ensureBackendServiceLogConfig(sp, be)
-	if existingLogConfig == nil || existingLogConfig.Enable != be.LogConfig.Enable ||
-		existingLogConfig.SampleRate != be.LogConfig.SampleRate {
+	if existingLogConfig == nil || existingLogConfig.Enable != be.LogConfig.Enable || existingLogConfig.SampleRate != be.LogConfig.SampleRate {
 		klog.V(2).Infof("Updated Logging settings for service %s (port %d) to (Enable: %t, SampleRate: %f)", svcKey, sp.Port, be.LogConfig.Enable, be.LogConfig.SampleRate)
 		return true
 	}
@@ -61,8 +60,9 @@ func ensureBackendServiceLogConfig(sp utils.ServicePort, be *composite.BackendSe
 		be.LogConfig = &composite.BackendServiceLogConfig{}
 	}
 	be.LogConfig.Enable = sp.BackendConfig.Spec.Logging.Enable
-	// Ignore sample rate if logging is not enabled.
+	// SampleRate should be set to 0.0 if logging is not enabled.
 	if !sp.BackendConfig.Spec.Logging.Enable {
+		be.LogConfig.SampleRate = 0.0
 		return
 	}
 	// Existing sample rate is retained if not specified.
diff --git a/pkg/backends/features/logging_test.go b/pkg/backends/features/logging_test.go
index 31397334c4..d4d9e27927 100644
--- a/pkg/backends/features/logging_test.go
+++ b/pkg/backends/features/logging_test.go
@@ -102,11 +102,31 @@ func TestEnsureLogging(t *testing.T) {
 			be: &composite.BackendService{
 				LogConfig: &composite.BackendServiceLogConfig{
 					Enable:     false,
-					SampleRate: 1.0,
+					SampleRate: 0.0,
 				},
 			},
 			expectUpdate: false,
 		},
+		{
+			desc: "logging remains disabled but has incorrect sample rate, update needed",
+			sp: utils.ServicePort{
+				BackendConfig: &backendconfigv1.BackendConfig{
+					Spec: backendconfigv1.BackendConfigSpec{
+						Logging: &backendconfigv1.LogConfig{
+							Enable:     false,
+							SampleRate: testutils.Float64ToPtr(0.4),
+						},
+					},
+				},
+			},
+			be: &composite.BackendService{
+				LogConfig: &composite.BackendServiceLogConfig{
+					Enable:     false,
+					SampleRate: 0.7,
+				},
+			},
+			expectUpdate: true,
+		},
 		{
 			desc: "no existing settings, update needed",
 			sp: utils.ServicePort{
@@ -272,7 +292,7 @@ func TestEnsureBackendServiceLogConfig(t *testing.T) {
 			},
 		},
 		{
-			desc: "logging stays disabled, sample rate retained",
+			desc: "logging is disabled, sample rate defaults to 0.0",
 			sp: utils.ServicePort{
 				BackendConfig: &backendconfigv1.BackendConfig{
 					Spec: backendconfigv1.BackendConfigSpec{
@@ -284,13 +304,34 @@ func TestEnsureBackendServiceLogConfig(t *testing.T) {
 				},
 			},
 			logConfig: &composite.BackendServiceLogConfig{
-				Enable:     false,
+				Enable:     true,
 				SampleRate: 0.4,
 			},
 			expectLogConfig: &composite.BackendServiceLogConfig{
+				Enable:     false,
+				SampleRate: 0.0,
+			},
+		},
+		{
+			desc: "logging stays disabled, sample rate defaults to 0.0",
+			sp: utils.ServicePort{
+				BackendConfig: &backendconfigv1.BackendConfig{
+					Spec: backendconfigv1.BackendConfigSpec{
+						Logging: &backendconfigv1.LogConfig{
+							Enable:     false,
+							SampleRate: testutils.Float64ToPtr(0.5),
+						},
+					},
+				},
+			},
+			logConfig: &composite.BackendServiceLogConfig{
 				Enable:     false,
 				SampleRate: 0.4,
 			},
+			expectLogConfig: &composite.BackendServiceLogConfig{
+				Enable:     false,
+				SampleRate: 0.0,
+			},
 		},
 		{
 			desc: "logging enabled, sample rate retained",