diff --git a/api/v1alpha1/accesslogging_types.go b/api/v1alpha1/accesslogging_types.go index 765fa4de7bc4..24272564488c 100644 --- 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 index 4eac0e11c294..adbd302b9571 100644 --- 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 index 3d9f52018dcc..4f6c7b369239 100644 --- 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 index 5802c511e97c..4161575dd8ab 100644 --- 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 index f62207ce230c..43505266ec0d 100644 --- 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 index 9462c96658ca..86b83fb93a73 100644 --- 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 index 535c49edb0eb..0f523fb1e9c8 100644 --- a/internal/ir/zz_generated.deepcopy.go +++ b/internal/ir/zz_generated.deepcopy.go @@ -19,6 +19,11 @@ import ( // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ALSAccessLog) DeepCopyInto(out *ALSAccessLog) { *out = *in + if in.CELMatches != nil { + in, out := &in.CELMatches, &out.CELMatches + *out = make([]string, len(*in)) + copy(*out, *in) + } in.Destination.DeepCopyInto(&out.Destination) if in.Text != nil { in, out := &in.Text, &out.Text @@ -82,11 +87,6 @@ func (in *ALSAccessLogHTTP) DeepCopy() *ALSAccessLogHTTP { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AccessLog) DeepCopyInto(out *AccessLog) { *out = *in - if in.CELMatches != nil { - in, out := &in.CELMatches, &out.CELMatches - *out = make([]string, len(*in)) - copy(*out, *in) - } if in.Text != nil { in, out := &in.Text, &out.Text *out = make([]*TextAccessLog, len(*in)) @@ -1517,6 +1517,11 @@ func (in *InfraMetadata) DeepCopy() *InfraMetadata { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *JSONAccessLog) DeepCopyInto(out *JSONAccessLog) { *out = *in + if in.CELMatches != nil { + in, out := &in.CELMatches, &out.CELMatches + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.JSON != nil { in, out := &in.JSON, &out.JSON *out = make(map[string]string, len(*in)) @@ -1765,6 +1770,11 @@ func (in *OIDC) DeepCopy() *OIDC { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OpenTelemetryAccessLog) DeepCopyInto(out *OpenTelemetryAccessLog) { *out = *in + if in.CELMatches != nil { + in, out := &in.CELMatches, &out.CELMatches + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.Text != nil { in, out := &in.Text, &out.Text *out = new(string) @@ -2729,6 +2739,11 @@ func (in *TLSUpstreamConfig) DeepCopy() *TLSUpstreamConfig { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TextAccessLog) DeepCopyInto(out *TextAccessLog) { *out = *in + if in.CELMatches != nil { + in, out := &in.CELMatches, &out.CELMatches + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.Format != nil { in, out := &in.Format, &out.Format *out = new(string) diff --git a/internal/xds/translator/accesslog.go b/internal/xds/translator/accesslog.go index 85151eaca3fa..01c448b65e96 100644 --- a/internal/xds/translator/accesslog.go +++ b/internal/xds/translator/accesslog.go @@ -131,6 +131,7 @@ func buildXdsAccessLog(al *ir.AccessLog, forListener bool) []*accesslog.AccessLo ConfigType: &accesslog.AccessLog_TypedConfig{ TypedConfig: accesslogAny, }, + Filter: buildAccessLogFilter(text.CELMatches, forListener), }) } // handle json file access logs @@ -173,6 +174,7 @@ func buildXdsAccessLog(al *ir.AccessLog, forListener bool) []*accesslog.AccessLo ConfigType: &accesslog.AccessLog_TypedConfig{ TypedConfig: accesslogAny, }, + Filter: buildAccessLogFilter(json.CELMatches, forListener), }) } // handle ALS access logs @@ -207,6 +209,7 @@ func buildXdsAccessLog(al *ir.AccessLog, forListener bool) []*accesslog.AccessLo ConfigType: &accesslog.AccessLog_TypedConfig{ TypedConfig: accesslogAny, }, + Filter: buildAccessLogFilter(als.CELMatches, forListener), }) case egv1a1.ALSEnvoyProxyAccessLogTypeTCP: alCfg := &grpcaccesslog.TcpGrpcAccessLogConfig{ @@ -219,6 +222,7 @@ func buildXdsAccessLog(al *ir.AccessLog, forListener bool) []*accesslog.AccessLo ConfigType: &accesslog.AccessLog_TypedConfig{ TypedConfig: accesslogAny, }, + Filter: buildAccessLogFilter(als.CELMatches, forListener), }) } } @@ -266,24 +270,10 @@ func buildXdsAccessLog(al *ir.AccessLog, forListener bool) []*accesslog.AccessLo ConfigType: &accesslog.AccessLog_TypedConfig{ TypedConfig: accesslogAny, }, + Filter: buildAccessLogFilter(otel.CELMatches, forListener), }) } - // add filter for access logs - filters := make([]*accesslog.AccessLogFilter, 0) - for _, expr := range al.CELMatches { - filters = append(filters, celAccessLogFilter(expr)) - } - if forListener { - filters = append(filters, listenerAccessLogFilter) - } - - f := buildAccessLogFilter(filters...) - - for _, log := range accessLogs { - log.Filter = f - } - return accessLogs } @@ -302,19 +292,28 @@ func celAccessLogFilter(expr string) *accesslog.AccessLogFilter { } } -func buildAccessLogFilter(f ...*accesslog.AccessLogFilter) *accesslog.AccessLogFilter { - if len(f) == 0 { +func buildAccessLogFilter(exprs []string, forListener bool) *accesslog.AccessLogFilter { + // add filter for access logs + var filters []*accesslog.AccessLogFilter + for _, expr := range exprs { + filters = append(filters, celAccessLogFilter(expr)) + } + if forListener { + filters = append(filters, listenerAccessLogFilter) + } + + if len(filters) == 0 { return nil } - if len(f) == 1 { - return f[0] + if len(filters) == 1 { + return filters[0] } return &accesslog.AccessLogFilter{ FilterSpecifier: &accesslog.AccessLogFilter_AndFilter{ AndFilter: &accesslog.AndFilter{ - Filters: f, + Filters: filters, }, }, } diff --git a/internal/xds/translator/testdata/in/xds-ir/accesslog-cel.yaml b/internal/xds/translator/testdata/in/xds-ir/accesslog-cel.yaml index b0623fd0842c..e9cff901d3de 100644 --- a/internal/xds/translator/testdata/in/xds-ir/accesslog-cel.yaml +++ b/internal/xds/translator/testdata/in/xds-ir/accesslog-cel.yaml @@ -1,13 +1,15 @@ name: "accesslog" accesslog: - celMatches: - - response.code >= 400 text: - path: "/dev/stdout" + celMatches: + - response.code >= 400 format: | [%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %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%" json: - path: "/dev/stdout" + celMatches: + - response.code >= 400 json: start_time: "%START_TIME%" method: "%REQ(:METHOD)%" @@ -22,6 +24,8 @@ accesslog: resources: "cluster_name": "cluster1" authority: "otel-collector.default.svc.cluster.local" + celMatches: + - response.code >= 400 destination: name: "accesslog-0" settings: diff --git a/internal/xds/translator/testdata/in/xds-ir/accesslog-multi-cel.yaml b/internal/xds/translator/testdata/in/xds-ir/accesslog-multi-cel.yaml index 704c19863d6e..fab193fe5644 100644 --- a/internal/xds/translator/testdata/in/xds-ir/accesslog-multi-cel.yaml +++ b/internal/xds/translator/testdata/in/xds-ir/accesslog-multi-cel.yaml @@ -1,14 +1,18 @@ name: "accesslog" accesslog: - celMatches: - - response.code >= 400 - - request.url_path.contains('v1beta3') + text: - path: "/dev/stdout" + celMatches: + - response.code >= 400 + - request.url_path.contains('v1beta3') format: | [%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %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%" json: - path: "/dev/stdout" + celMatches: + - response.code >= 400 + - request.url_path.contains('v1beta3') json: start_time: "%START_TIME%" method: "%REQ(:METHOD)%" @@ -23,6 +27,9 @@ accesslog: resources: "cluster_name": "cluster1" authority: "otel-collector.default.svc.cluster.local" + celMatches: + - response.code >= 400 + - request.url_path.contains('v1beta3') destination: name: "accesslog-0" settings: diff --git a/test/e2e/tests/utils.go b/test/e2e/tests/utils.go index d620a9baa901..c9ebe55f59f5 100644 --- a/test/e2e/tests/utils.go +++ b/test/e2e/tests/utils.go @@ -372,7 +372,7 @@ func RetrieveMetric(url string, name string, timeout time.Duration) (*dto.Metric return mf, nil } - return nil, fmt.Errorf("metric %s not found", name) + return nil, nil } func WaitForLoadBalancerAddress(t *testing.T, client client.Client, timeout time.Duration, nn types.NamespacedName) (string, error) { @@ -411,6 +411,11 @@ func ALSLogCount(suite *suite.ConformanceTestSuite) (int, error) { return -1, err } + // metric not found or empty + if countMetric == nil { + return 0, nil + } + total := 0 for _, m := range countMetric.Metric { if m.Counter != nil && m.Counter.Value != nil { diff --git a/tools/make/kube.mk b/tools/make/kube.mk index 2258aa90f039..4ec344358296 100644 --- a/tools/make/kube.mk +++ b/tools/make/kube.mk @@ -141,17 +141,24 @@ install-ratelimit: tools/hack/deployment-exists.sh "app.kubernetes.io/name=envoy-ratelimit" "envoy-gateway-system" kubectl wait --timeout=5m -n envoy-gateway-system deployment/envoy-ratelimit --for=condition=Available -.PHONY: run-e2e -run-e2e: ## Run e2e tests +.PHONY: e2e-prepare +e2e-prepare: ## Prepare the environment for running e2e tests @$(LOG_TARGET) kubectl wait --timeout=5m -n envoy-gateway-system deployment/envoy-ratelimit --for=condition=Available kubectl wait --timeout=5m -n envoy-gateway-system deployment/envoy-gateway --for=condition=Available kubectl apply -f test/config/gatewayclass.yaml + +.PHONY: run-e2e +run-e2e: e2e-prepare ## Run e2e tests + @$(LOG_TARGET) ifeq ($(E2E_RUN_TEST),) go test $(E2E_TEST_ARGS) ./test/e2e --gateway-class=envoy-gateway --debug=true --cleanup-base-resources=false go test $(E2E_TEST_ARGS) ./test/e2e/merge_gateways --gateway-class=merge-gateways --debug=true --cleanup-base-resources=false go test $(E2E_TEST_ARGS) ./test/e2e/multiple_gc --debug=true --cleanup-base-resources=true go test $(E2E_TEST_ARGS) ./test/e2e/upgrade --gateway-class=upgrade --debug=true --cleanup-base-resources=$(E2E_CLEANUP) +else + go test $(E2E_TEST_ARGS) ./test/e2e --gateway-class=envoy-gateway --debug=true --cleanup-base-resources=$(E2E_CLEANUP) \ + --run-test $(E2E_RUN_TEST) endif .PHONY: run-benchmark