accesslog: fix different CelMatches on AccessLog (envoyproxy#3885)

* accesslog: fix different CelMatches on AccessLog Signed-off-by: zirain <zirain2009@gmail.com> * lint Signed-off-by: zirain <zirain2009@gmail.com> * text Signed-off-by: zirain <zirain2009@gmail.com> * lint Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> Co-authored-by: Guy Daich <guy.daich@sap.com> Signed-off-by: Karandashov Daniil <karandashovd@gmail.com>
Demacr · Jul 26, 2024 · 9d23ade · 9d23ade
1 parent 2cc91ba
commit 9d23ade
Show file tree

Hide file tree

Showing 12 changed files with 170 additions and 66 deletions.
diff --git a/api/v1alpha1/accesslogging_types.go b/api/v1alpha1/accesslogging_types.go
@@ -20,7 +20,7 @@ type ProxyAccessLogSetting struct {
 	// Format defines the format of accesslog.
 	// This will be ignored if sink type is ALS.
 	// +optional
-	Format *ProxyAccessLogFormat `json:"format"`
+	Format *ProxyAccessLogFormat `json:"format,omitempty"`
 	// Matches defines the match conditions for accesslog in CEL expression.
 	// An accesslog will be emitted only when one or more match conditions are evaluated to true.
 	// Invalid [CEL](https://www.envoyproxy.io/docs/envoy/latest/xds/type/v3/cel.proto.html#common-expression-language-cel-proto) expressions will be ignored.

diff --git a/internal/gatewayapi/listener.go b/internal/gatewayapi/listener.go
@@ -257,6 +257,21 @@ func (t *Translator) processAccessLog(envoyproxy *egv1a1.EnvoyProxy, resources *
 			}
 		}
 
+		var (
+			validExprs []string
+			errs       []error
+		)
+		for _, expr := range accessLog.Matches {
+			if !validCELExpression(expr) {
+				errs = append(errs, fmt.Errorf("invalid CEL expression: %s", expr))
+				continue
+			}
+			validExprs = append(validExprs, expr)
+		}
+		if len(errs) > 0 {
+			return nil, utilerrors.NewAggregate(errs)
+		}
+
 		for j, sink := range accessLog.Sinks {
 			switch sink.Type {
 			case egv1a1.ProxyAccessLogSinkTypeFile:
@@ -267,8 +282,9 @@ func (t *Translator) processAccessLog(envoyproxy *egv1a1.EnvoyProxy, resources *
 				switch format.Type {
 				case egv1a1.ProxyAccessLogFormatTypeText:
 					al := &ir.TextAccessLog{
-						Format: format.Text,
-						Path:   sink.File.Path,
+						Format:     format.Text,
+						Path:       sink.File.Path,
+						CELMatches: validExprs,
 					}
 					irAccessLog.Text = append(irAccessLog.Text, al)
 				case egv1a1.ProxyAccessLogFormatTypeJSON:
@@ -278,8 +294,9 @@ func (t *Translator) processAccessLog(envoyproxy *egv1a1.EnvoyProxy, resources *
 					}
 
 					al := &ir.JSONAccessLog{
-						JSON: format.JSON,
-						Path: sink.File.Path,
+						JSON:       format.JSON,
+						Path:       sink.File.Path,
+						CELMatches: validExprs,
 					}
 					irAccessLog.JSON = append(irAccessLog.JSON, al)
 				}
@@ -307,7 +324,8 @@ func (t *Translator) processAccessLog(envoyproxy *egv1a1.EnvoyProxy, resources *
 						Name:     fmt.Sprintf("accesslog_als_%d_%d", i, j), // TODO: rename this, so that we can share backend with tracing?
 						Settings: ds,
 					},
-					Type: sink.ALS.Type,
+					Type:       sink.ALS.Type,
+					CELMatches: validExprs,
 				}
 
 				if al.Type == egv1a1.ALSEnvoyProxyAccessLogTypeHTTP && sink.ALS.HTTP != nil {
@@ -334,7 +352,8 @@ func (t *Translator) processAccessLog(envoyproxy *egv1a1.EnvoyProxy, resources *
 
 				// TODO: remove support for Host/Port in v1.2
 				al := &ir.OpenTelemetryAccessLog{
-					Resources: sink.OpenTelemetry.Resources,
+					CELMatches: validExprs,
+					Resources:  sink.OpenTelemetry.Resources,
 				}
 
 				// TODO: how to get authority from the backendRefs?
@@ -368,22 +387,6 @@ func (t *Translator) processAccessLog(envoyproxy *egv1a1.EnvoyProxy, resources *
 				irAccessLog.OpenTelemetry = append(irAccessLog.OpenTelemetry, al)
 			}
 		}
-
-		var (
-			validExprs []string
-			errs       []error
-		)
-		for _, expr := range accessLog.Matches {
-			if !validCELExpression(expr) {
-				errs = append(errs, fmt.Errorf("invalid CEL expression: %s", expr))
-				continue
-			}
-			validExprs = append(validExprs, expr)
-		}
-		if len(errs) > 0 {
-			return nil, utilerrors.NewAggregate(errs)
-		}
-		irAccessLog.CELMatches = validExprs
 	}
 
 	return irAccessLog, nil

diff --git a/internal/gatewayapi/testdata/envoyproxy-accesslog-cel.in.yaml b/internal/gatewayapi/testdata/envoyproxy-accesslog-cel.in.yaml
@@ -24,6 +24,14 @@ envoyProxyForGatewayClass:
               port: 4317
               resources:
                 k8s.cluster.name: "cluster-1"
+        - sinks:
+          - type: ALS
+            als:
+              backendRefs:
+              - name: envoy-als
+                namespace: monitoring
+                port: 9000
+              type: TCP
     provider:
       type: Kubernetes
       kubernetes:
@@ -87,3 +95,36 @@ gateways:
       allowedRoutes:
         namespaces:
           from: Same
+services:
+- apiVersion: v1
+  kind: Service
+  metadata:
+    name: envoy-als
+    namespace: monitoring
+  spec:
+    type: ClusterIP
+    ports:
+    - name: grpc
+      port: 9000
+      appProtocol: grpc
+      protocol: TCP
+      targetPort: 9000
+endpointSlices:
+- apiVersion: discovery.k8s.io/v1
+  kind: EndpointSlice
+  metadata:
+    name: endpointslice-envoy-als
+    namespace: monitoring
+    labels:
+      kubernetes.io/service-name: envoy-als
+  addressType: IPv4
+  ports:
+  - name: grpc
+    appProtocol: grpc
+    protocol: TCP
+    port: 9090
+  endpoints:
+  - addresses:
+    - "10.240.0.10"
+    conditions:
+      ready: true
diff --git a/internal/gatewayapi/testdata/envoyproxy-accesslog-cel.out.yaml b/internal/gatewayapi/testdata/envoyproxy-accesslog-cel.out.yaml
@@ -118,6 +118,14 @@ infraIR:
                     resources:
                       k8s.cluster.name: cluster-1
                   type: OpenTelemetry
+              - sinks:
+                - als:
+                    backendRefs:
+                    - name: envoy-als
+                      namespace: monitoring
+                      port: 9000
+                    type: TCP
+                  type: ALS
         status: {}
       listeners:
       - address: null
@@ -135,10 +143,21 @@ infraIR:
 xdsIR:
   envoy-gateway/gateway-1:
     accessLog:
-      celMatches:
-      - response.code >= 400
+      als:
+      - destination:
+          name: accesslog_als_1_0
+          settings:
+          - addressType: IP
+            endpoints:
+            - host: 10.240.0.10
+              port: 9090
+            protocol: GRPC
+        name: envoy-gateway-system/test
+        type: TCP
       openTelemetry:
       - authority: otel-collector.monitoring.svc.cluster.local
+        celMatches:
+        - response.code >= 400
         destination:
           name: accesslog_otel_0_1
           settings:
@@ -152,7 +171,9 @@ xdsIR:
         text: |
           [%START_TIME%] "%REQ(:METHOD)% %PROTOCOL%" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%"\n
       text:
-      - format: |
+      - celMatches:
+        - response.code >= 400
+        format: |
           [%START_TIME%] "%REQ(:METHOD)% %PROTOCOL%" %RESPONSE_CODE% %RESPONSE_FLAGS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%"\n
         path: /dev/stdout
     http:

diff --git a/internal/gatewayapi/testdata/envoyproxy-accesslog-without-format.out.yaml b/internal/gatewayapi/testdata/envoyproxy-accesslog-without-format.out.yaml
@@ -102,8 +102,7 @@ infraIR:
           telemetry:
             accessLog:
               settings:
-              - format: null
-                sinks:
+              - sinks:
                 - file:
                     path: /dev/stdout
                   type: File

diff --git a/internal/ir/xds.go b/internal/ir/xds.go
@@ -1609,7 +1609,6 @@ type RateLimitValue struct {
 // AccessLog holds the access logging configuration.
 // +k8s:deepcopy-gen=true
 type AccessLog struct {
-	CELMatches    []string                  `json:"celMatches,omitempty" yaml:"celMatches,omitempty"`
 	Text          []*TextAccessLog          `json:"text,omitempty" yaml:"text,omitempty"`
 	JSON          []*JSONAccessLog          `json:"json,omitempty" yaml:"json,omitempty"`
 	ALS           []*ALSAccessLog           `json:"als,omitempty" yaml:"als,omitempty"`
@@ -1619,20 +1618,23 @@ type AccessLog struct {
 // TextAccessLog holds the configuration for text access logging.
 // +k8s:deepcopy-gen=true
 type TextAccessLog struct {
-	Format *string `json:"format,omitempty" yaml:"format,omitempty"`
-	Path   string  `json:"path" yaml:"path"`
+	CELMatches []string `json:"celMatches,omitempty" yaml:"celMatches,omitempty"`
+	Format     *string  `json:"format,omitempty" yaml:"format,omitempty"`
+	Path       string   `json:"path" yaml:"path"`
 }
 
 // JSONAccessLog holds the configuration for JSON access logging.
 // +k8s:deepcopy-gen=true
 type JSONAccessLog struct {
-	JSON map[string]string `json:"json,omitempty" yaml:"json,omitempty"`
-	Path string            `json:"path" yaml:"path"`
+	CELMatches []string          `json:"celMatches,omitempty" yaml:"celMatches,omitempty"`
+	JSON       map[string]string `json:"json,omitempty" yaml:"json,omitempty"`
+	Path       string            `json:"path" yaml:"path"`
 }
 
 // ALSAccessLog holds the configuration for gRPC ALS access logging.
 // +k8s:deepcopy-gen=true
 type ALSAccessLog struct {
+	CELMatches  []string                          `json:"celMatches,omitempty" yaml:"celMatches,omitempty"`
 	LogName     string                            `json:"name" yaml:"name"`
 	Destination RouteDestination                  `json:"destination,omitempty" yaml:"destination,omitempty"`
 	Type        egv1a1.ALSEnvoyProxyAccessLogType `json:"type" yaml:"type"`
@@ -1652,6 +1654,7 @@ type ALSAccessLogHTTP struct {
 // OpenTelemetryAccessLog holds the configuration for OpenTelemetry access logging.
 // +k8s:deepcopy-gen=true
 type OpenTelemetryAccessLog struct {
+	CELMatches  []string          `json:"celMatches,omitempty" yaml:"celMatches,omitempty"`
 	Authority   string            `json:"authority,omitempty" yaml:"authority,omitempty"`
 	Text        *string           `json:"text,omitempty" yaml:"text,omitempty"`
 	Attributes  map[string]string `json:"attributes,omitempty" yaml:"attributes,omitempty"`

diff --git a/internal/ir/zz_generated.deepcopy.go b/internal/ir/zz_generated.deepcopy.go