feat(logging): enhance L4LBConfig and LoggingConfig with improved validation rules for optionalFields

Author: 08volt

cmd/glbc/main.go

@@ -192,6 +192,7 @@ func main() {
 	var l4LBConfigClient l4lbconfigclient.Interface
 	if flags.F.ManageL4LBLogging && (flags.F.RunL4Controller || flags.F.RunL4NetLBController) {
 		l4LBConfigCRDMeta := l4lbconfig.CRDMeta()
+		klog.V(0).Info("Ensuring L4LBConfig CRD exists")
 		if _, err := crdHandler.EnsureCRD(l4LBConfigCRDMeta, true); err != nil {
 			klog.Fatalf("Failed to ensure L4LBConfig CRD: %v", err)
 		}

docs/deploy/resources/rbac.yaml

@@ -48,7 +48,7 @@ rules:
   resources: ["nodes", "namespaces", "endpoints", "pods"]
   verbs: ["get", "list", "watch"]
 - apiGroups: ["networking.gke.io"]
-  resources: ["managedcertificates", "frontendconfigs", "servicenetworkendpointgroups", "gcpingressparams", "serviceattachments", "gkenetworkparamsets", "networks", "gcpfirewalls"]
+  resources: ["managedcertificates", "frontendconfigs", "servicenetworkendpointgroups", "gcpingressparams", "serviceattachments", "gkenetworkparamsets", "networks", "gcpfirewalls", "l4lbconfigs"]
   verbs: ["*"]
 - apiGroups: ["networking.gke.io"]
   resources: ["nodetopologies"]

pkg/apis/l4lbconfig/v1/types.go

@@ -37,6 +37,8 @@ type L4LBConfig struct {
 // +k8s:openapi-gen=true
 type L4LBConfigSpec struct {
 	// Logging defines the telemetry and flow logging configuration for the L4 Load Balancer.
+	// +k8s:validation:cel[0]:rule="(self.optionalMode == 'CUSTOM' && has(self.optionalFields) && size(self.optionalFields) > 0) || (self.optionalMode != 'CUSTOM' && (!has(self.optionalFields) || size(self.optionalFields) == 0))"
+	// +k8s:validation:cel[0]:message="optionalFields can only be set when optionalMode is 'CUSTOM', and must be set when optionalMode is 'CUSTOM'"
 	// +optional
 	Logging *LoggingConfig `json:"logging,omitempty"`
 }
@@ -57,7 +59,6 @@ type L4LBConfigList struct {
 }
 
 // LoggingConfig defines the parameters for LB logging.
-// +kubebuilder:validation:XValidation:rule="has(self.optionalFields) && size(self.optionalFields) > 0 ? self.optionalMode == 'CUSTOM' : true",message="optionalFields can only be set when optionalMode is 'CUSTOM'"
 // +k8s:openapi-gen=true
 type LoggingConfig struct {
 	// Enabled allows toggling of Cloud Logging.
@@ -66,20 +67,29 @@ type LoggingConfig struct {
 
 	// SampleRate is the percentage of flows to log, from 0 to 1000000.
 	// 1000000 means 100% of packets are logged.
-	// +kubebuilder:validation:Minimum=0
-	// +kubebuilder:validation:Maximum=1000000
+	// +k8s:validation:maximum=1000000
+	// +k8s:validation:minimum=0
 	// +optional
 	SampleRate *int32 `json:"sampleRate,omitempty"`
 
 	// OptionalMode defines which metadata fields to include in the logs.
 	// Options: INCLUDE_ALL_OPTIONAL, EXCLUDE_ALL_OPTIONAL, CUSTOM.
-	// +kubebuilder:validation:Enum=INCLUDE_ALL_OPTIONAL;EXCLUDE_ALL_OPTIONAL;CUSTOM
 	// +optional
-	OptionalMode string `json:"optionalMode,omitempty"`
+	OptionalMode LoggingOptionalMode `json:"optionalMode,omitempty"`
 
 	// OptionalFields is a list of additional metadata fields to include.
 	// Only valid when optionalMode is set to 'CUSTOM'.
 	// +listType=set
 	// +optional
 	OptionalFields []string `json:"optionalFields,omitempty"`
 }
+
+// +k8s:openapi-gen=true
+// +enum
+type LoggingOptionalMode string
+
+const (
+	LoggingOptionalModeIncludeAllOptional = LoggingOptionalMode("INCLUDE_ALL_OPTIONAL")
+	LoggingOptionalModeExcludeAllOptional = LoggingOptionalMode("EXCLUDE_ALL_OPTIONAL")
+	LoggingOptionalModeCustom             = LoggingOptionalMode("CUSTOM")
+)

pkg/apis/l4lbconfig/v1/zz_generated.openapi.go

@@ -28,14 +28,13 @@ import (
 	"k8s.io/ingress-gce/pkg/crd"
 	"k8s.io/ingress-gce/pkg/flags"
 	"k8s.io/ingress-gce/pkg/l4annotations"
-	common "k8s.io/kube-openapi/pkg/common"
 )
 
 var (
-	ErrL4LBConfigDoesNotExist = errors.New("no L4LBConfig for service port exists.")
-	ErrL4LBConfigFailedToGet  = errors.New("client had error getting L4LBConfig for service.")
-	ErrL4LBConfigInvalidMode  = errors.New("invalid OptionalMode in L4LBConfig for service.")
-	allowedOptionalModes      = []interface{}{"INCLUDE_ALL_OPTIONAL", "EXCLUDE_ALL_OPTIONAL", "CUSTOM"}
+	ErrL4LBConfigDoesNotExist      = errors.New("no L4LBConfig for service port exists.")
+	ErrL4LBConfigFailedToGet       = errors.New("client had error getting L4LBConfig for service.")
+	ErrL4LBConfigInvalidMode       = errors.New("invalid OptionalMode in L4LBConfig for service.")
+	ErrL4LBConfigInvalidSampleRate = errors.New("invalid SampleRate in L4LBConfig for service.")
 )
 
 const (
@@ -52,6 +51,8 @@ const (
 	minSampleRate = 0.0
 	// defaultSampleRate is the default value for composite.BackendServiceLogConfig.SampleRate when not specified.
 	defaultSampleRate = 1.0
+	// maxResolvedSampleRate is the maximum allowed value for the resolved SampleRate after conversion (100% as a decimal).
+	maxResolvedSampleRate = 1.0
 )
 
 func CRDMeta() *crd.CRDMeta {
@@ -62,37 +63,12 @@ func CRDMeta() *crd.CRDMeta {
 		"l4lbconfig",
 		"l4lbconfigs",
 		[]*crd.Version{
-			crd.NewVersion("v1", "k8s.io/ingress-gce/pkg/apis/l4lbconfig/v1.L4LBConfig", getOpenAPIDefinitions, false),
+			crd.NewVersion("v1", "k8s.io/ingress-gce/pkg/apis/l4lbconfig/v1.L4LBConfig", l4lbconfigv1.GetOpenAPIDefinitions, false),
 		},
 	)
 	return meta
 }
 
-func getOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenAPIDefinition {
-	defs := l4lbconfigv1.GetOpenAPIDefinitions(ref)
-
-	if def, ok := defs["k8s.io/ingress-gce/pkg/apis/l4lbconfig/v1.LoggingConfig"]; ok {
-		// Patch SampleRate
-		if prop, ok := def.Schema.SchemaProps.Properties["sampleRate"]; ok {
-			min := minSampleRate
-			max := maxSampleRate
-			prop.SchemaProps.Minimum = &min
-			prop.SchemaProps.Maximum = &max
-			def.Schema.SchemaProps.Properties["sampleRate"] = prop
-		}
-
-		// Patch OptionalMode (Enum)
-		if prop, ok := def.Schema.SchemaProps.Properties["optionalMode"]; ok {
-			prop.SchemaProps.Enum = allowedOptionalModes
-			def.Schema.SchemaProps.Properties["optionalMode"] = prop
-		}
-
-		defs["k8s.io/ingress-gce/pkg/apis/l4lbconfig/v1.LoggingConfig"] = def
-	}
-
-	return defs
-}
-
 // GetL4LBConfigForService returns the corresponding L4LBConfig for
 // the given Service if specified.
 func GetL4LBConfigForService(l4lbConfigLister cache.Store, svc *corev1.Service) (*l4lbconfigv1.L4LBConfig, error) {
@@ -152,18 +128,22 @@ func DetermineL4LoggingConfig(
 		OptionalFields: slc.OptionalFields,
 	}
 
-	resolvedConfig.OptionalMode = slc.OptionalMode
+	resolvedConfig.OptionalMode = string(slc.OptionalMode)
 	if resolvedConfig.OptionalMode == "" { // Set default if not specified
 		resolvedConfig.OptionalMode = "EXCLUDE_ALL_OPTIONAL"
 	}
 
+	// Double-check OptionalMode validity before proceeding with OptionalFields checks
+	// Validation already occurs at the API level, but this serves as a safeguard against any unexpected values.
 	switch resolvedConfig.OptionalMode {
 	case "EXCLUDE_ALL_OPTIONAL", "INCLUDE_ALL_OPTIONAL", "CUSTOM":
 		// Valid
 	default:
 		return nil, NewConditionLoggingInvalid(ErrL4LBConfigInvalidMode), ErrL4LBConfigInvalidMode
 	}
 
+	// Double-check consistency between OptionalMode and OptionalFields
+	// Validation already occurs at the API level, but this serves as a safeguard against any unexpected states.
 	if resolvedConfig.OptionalMode != "CUSTOM" && len(resolvedConfig.OptionalFields) > 0 {
 		return nil, NewConditionLoggingInvalid(ErrL4LBConfigInvalidMode), ErrL4LBConfigInvalidMode
 	}
@@ -178,6 +158,12 @@ func DetermineL4LoggingConfig(
 		resolvedConfig.SampleRate = defaultSampleRate
 	}
 
+	// Doube-check SampleRate validity before returning the resolved config
+	// Validation already occurs at the API level, but this serves as a safeguard against any unexpected values.
+	if resolvedConfig.SampleRate < minSampleRate || resolvedConfig.SampleRate > maxResolvedSampleRate {
+		return nil, NewConditionLoggingInvalid(ErrL4LBConfigInvalidSampleRate), ErrL4LBConfigInvalidSampleRate
+	}
+
 	return resolvedConfig, NewConditionLoggingReconciled(), nil
 }