From 3f650402148db9d59b1fade65c2a24cae8d6cf12 Mon Sep 17 00:00:00 2001 From: Dineth Date: Mon, 10 Aug 2026 12:43:11 +0530 Subject: [PATCH 1/3] hardcode ignoring gateway health probe calls --- .../gateway-controller/pkg/xds/translator.go | 24 ++++++++- .../pkg/xds/translator_test.go | 54 ++++++++++++++++++- 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/gateway/gateway-controller/pkg/xds/translator.go b/gateway/gateway-controller/pkg/xds/translator.go index ae27e6832..d409841f3 100644 --- a/gateway/gateway-controller/pkg/xds/translator.go +++ b/gateway/gateway-controller/pkg/xds/translator.go @@ -2721,13 +2721,35 @@ func (t *Translator) createGRPCAccessLog() (*accesslog.AccessLog, error) { return &accesslog.AccessLog{ Name: "envoy.access_loggers.http_grpc", - Filter: buildIgnorePathsAccessLogFilter(t.config.Collector.IgnorePathPrefixes), + Filter: buildAccessLogFilter(t.config.Collector.IgnorePathPrefixes), ConfigType: &accesslog.AccessLog_TypedConfig{ TypedConfig: grpcAccessLogAny, }, }, nil } + +func buildAccessLogFilter(prefixes []string) *accesslog.AccessLogFilter { + filters := []*accesslog.AccessLogFilter{buildReservedHealthPathAccessLogFilter()} + if userFilter := buildIgnorePathsAccessLogFilter(prefixes); userFilter != nil { + filters = append(filters, userFilter) + } + if len(filters) == 1 { + return filters[0] + } + return &accesslog.AccessLogFilter{ + FilterSpecifier: &accesslog.AccessLogFilter_AndFilter{ + AndFilter: &accesslog.AndFilter{Filters: filters}, + }, + } +} + +const envoyPathPseudoHeader = ":path" + +func buildReservedHealthPathAccessLogFilter() *accesslog.AccessLogFilter { + return headerPrefixFilter(envoyPathPseudoHeader, constants.GatewayHealthPathPrefix, true) +} + // envoyOriginalPathHeader is the header Envoy's router sets to the pre-rewrite, // client-facing path whenever a route applies a path rewrite (PrefixRewrite/ // RegexRewrite) — which every proxied API route in this gateway does, for diff --git a/gateway/gateway-controller/pkg/xds/translator_test.go b/gateway/gateway-controller/pkg/xds/translator_test.go index 46321f7cd..e72113b52 100644 --- a/gateway/gateway-controller/pkg/xds/translator_test.go +++ b/gateway/gateway-controller/pkg/xds/translator_test.go @@ -2163,7 +2163,13 @@ func TestTranslator_CreateGRPCAccessLog(t *testing.T) { accessLog, err := translator.createGRPCAccessLog() assert.NoError(t, err) assert.NotNil(t, accessLog) - assert.Nil(t, accessLog.Filter, "no ignore_path_prefixes configured -> no filter") + require.NotNil(t, accessLog.Filter, "reserved health-path suppression filter is always attached") + assert.False(t, evalAccessLogFilter(t, accessLog.Filter, map[string]string{ + ":path": constants.GatewayHealthyPath, + }), "gateway health-check path is suppressed even with no ignore_path_prefixes configured") + assert.True(t, evalAccessLogFilter(t, accessLog.Filter, map[string]string{ + ":path": "/orders", + }), "non-health path is still logged") } func TestTranslator_CreateGRPCAccessLog_WithIgnorePathPrefixes(t *testing.T) { @@ -2298,6 +2304,52 @@ func TestBuildIgnorePathsAccessLogFilter(t *testing.T) { }) } +func TestBuildReservedHealthPathAccessLogFilter(t *testing.T) { + filter := buildReservedHealthPathAccessLogFilter() + require.NotNil(t, filter) + + assert.False(t, evalAccessLogFilter(t, filter, map[string]string{ + ":path": constants.GatewayHealthyPath, + }), "healthy probe path is suppressed") + assert.False(t, evalAccessLogFilter(t, filter, map[string]string{ + ":path": constants.GatewayReadyPath, + }), "ready probe path is suppressed") + assert.True(t, evalAccessLogFilter(t, filter, map[string]string{ + ":path": "/orders", + }), "unrelated path is logged") + assert.True(t, evalAccessLogFilter(t, filter, map[string]string{}), + "no :path header at all is logged (fails open, never suppresses by default)") +} + +func TestBuildAccessLogFilter(t *testing.T) { + t.Run("no ignore prefixes -> health-only suppression", func(t *testing.T) { + filter := buildAccessLogFilter(nil) + require.NotNil(t, filter, "reserved health-path filter is always attached") + assert.False(t, evalAccessLogFilter(t, filter, map[string]string{ + ":path": constants.GatewayHealthyPath, + })) + assert.True(t, evalAccessLogFilter(t, filter, map[string]string{ + ":path": "/orders", + })) + }) + + t.Run("with ignore prefixes -> both health path and configured prefix suppressed", func(t *testing.T) { + filter := buildAccessLogFilter([]string{"/metrics"}) + require.NotNil(t, filter) + assert.False(t, evalAccessLogFilter(t, filter, map[string]string{ + ":path": constants.GatewayHealthyPath, + }), "reserved health path still suppressed alongside a configured prefix") + assert.False(t, evalAccessLogFilter(t, filter, map[string]string{ + "x-envoy-original-path": "/metrics/scrape", + ":path": "/orders", + }), "configured ignore prefix still suppressed") + assert.True(t, evalAccessLogFilter(t, filter, map[string]string{ + "x-envoy-original-path": "/orders", + ":path": "/orders", + }), "unrelated path with neither match is logged") + }) +} + func TestNotEffectivelyMatchesPrefix(t *testing.T) { tests := []struct { name string From 7cc23c5350514a3b8d66569c4851fdfe13483613 Mon Sep 17 00:00:00 2001 From: Dineth Date: Mon, 10 Aug 2026 13:04:33 +0530 Subject: [PATCH 2/3] decouple from router access logs --- .../gateway-controller/pkg/xds/translator.go | 67 +++++++--- .../pkg/xds/translator_test.go | 122 ++++++++++++++++-- 2 files changed, 160 insertions(+), 29 deletions(-) diff --git a/gateway/gateway-controller/pkg/xds/translator.go b/gateway/gateway-controller/pkg/xds/translator.go index d409841f3..82e36821c 100644 --- a/gateway/gateway-controller/pkg/xds/translator.go +++ b/gateway/gateway-controller/pkg/xds/translator.go @@ -1354,8 +1354,12 @@ func (t *Translator) createListener(virtualHosts []*route.VirtualHost, isHTTPS b PathWithEscapedSlashesAction: convertPathWithEscapedSlashesAction(t.routerConfig.HTTPListener.PathWithEscapedSlashesAction), } - // Add access logs if enabled - if t.routerConfig.AccessLogs.Enabled { + // Add access logs if either consumer needs a sink: the operator-facing stdout + // log line (router.access_logs.enabled) or the gRPC ALS stream the collector + // depends on. The collector's sink must not be gated on the stdout toggle — + // ALS is the only data source for traffic logging and analytics, so gating it + // there silently disables both. See createAccessLogConfig. + if t.routerConfig.AccessLogs.Enabled || t.config.IsCollectorEnabled() { accessLogs, err := t.createAccessLogConfig() if err != nil { return nil, nil, fmt.Errorf("failed to create access log config: %w", err) @@ -2609,9 +2613,46 @@ func sanitizeUpstreamDefinitionName(name string) string { return sanitized } -// createAccessLogConfig creates access log configuration based on format (JSON or text) to stdout +// createAccessLogConfig builds the router's access log sinks. Two independent +// consumers can each require a sink, so this is called whenever *either* is +// active (see the call site in createHTTPConnectionManager): +// +// - router.access_logs.enabled — the operator-facing stdout log line +// - the collector (analytics / traffic logging) — the gRPC ALS stream, which is +// the only thing that feeds the policy-engine's analytics pipeline +// +// The two must stay decoupled: turning off the stdout log line is a log-formatting +// choice and must not silently starve traffic logging and analytics of their only +// data source. func (t *Translator) createAccessLogConfig() ([]*accesslog.AccessLog, error) { var accessLogs []*accesslog.AccessLog + + if t.routerConfig.AccessLogs.Enabled { + fileAccessLog, err := t.createFileAccessLog() + if err != nil { + return nil, err + } + accessLogs = append(accessLogs, fileAccessLog) + } + + // If the collector is active, create the gRPC access log config and append to existing access logs + if t.config.IsCollectorEnabled() { + t.logger.Info("Creating gRPC access log configuration") + grpcAccessLog, err := t.createGRPCAccessLog() + if err != nil { + t.logger.Warn("Failed to create gRPC access log config, continuing without it", + slog.Any("error", err)) + } else { + accessLogs = append(accessLogs, grpcAccessLog) + } + } + + return accessLogs, nil +} + +// createFileAccessLog creates the stdout access log sink based on the configured +// format (JSON or text). +func (t *Translator) createFileAccessLog() (*accesslog.AccessLog, error) { var fileAccessLog *fileaccesslog.FileAccessLog if t.routerConfig.AccessLogs.Format == "json" { @@ -2666,27 +2707,12 @@ func (t *Translator) createAccessLogConfig() ([]*accesslog.AccessLog, error) { return nil, fmt.Errorf("failed to marshal access log config: %w", err) } - // Add file access log to slice - accessLogs = append(accessLogs, &accesslog.AccessLog{ + return &accesslog.AccessLog{ Name: "envoy.access_loggers.file", ConfigType: &accesslog.AccessLog_TypedConfig{ TypedConfig: fileAccessLogAny, }, - }) - - // If the collector is active, create the gRPC access log config and append to existing access logs - if t.config.IsCollectorEnabled() { - t.logger.Info("Creating gRPC access log configuration") - grpcAccessLog, err := t.createGRPCAccessLog() - if err != nil { - t.logger.Warn("Failed to create gRPC access log config, continuing without it", - slog.Any("error", err)) - } else { - accessLogs = append(accessLogs, grpcAccessLog) - } - } - - return accessLogs, nil + }, nil } // createGRPCAccessLog creates a gRPC access log configuration for the gateway controller @@ -2728,7 +2754,6 @@ func (t *Translator) createGRPCAccessLog() (*accesslog.AccessLog, error) { }, nil } - func buildAccessLogFilter(prefixes []string) *accesslog.AccessLogFilter { filters := []*accesslog.AccessLogFilter{buildReservedHealthPathAccessLogFilter()} if userFilter := buildIgnorePathsAccessLogFilter(prefixes); userFilter != nil { diff --git a/gateway/gateway-controller/pkg/xds/translator_test.go b/gateway/gateway-controller/pkg/xds/translator_test.go index e72113b52..f8b15847d 100644 --- a/gateway/gateway-controller/pkg/xds/translator_test.go +++ b/gateway/gateway-controller/pkg/xds/translator_test.go @@ -1436,21 +1436,127 @@ func TestTranslator_CreateListener_PathWithEscapedSlashesAction(t *testing.T) { } func TestTranslator_CreateAccessLogConfig_Disabled(t *testing.T) { - // Note: createAccessLogConfig should only be called when access logs are enabled. - // The check for enabled is done at the caller level. When called directly with disabled - // access logs (format defaults to empty, which falls through to text format check), - // it should return an error about missing text_format. + // Both consumers off: no stdout sink, and no ALS sink either. The stdout + // format fields are left unset on purpose — with access logs disabled they + // are never read, so an empty format must not surface as an error. logger := createTestLogger() routerCfg := testRouterConfig() routerCfg.AccessLogs.Enabled = false - // When format is empty, it falls through to text format check cfg := testConfig() + cfg.Router = *routerCfg translator := NewTranslator(logger, routerCfg, nil, cfg) logs, err := translator.createAccessLogConfig() - // Without format configured, it returns error (this is expected behavior) - assert.Error(t, err) - assert.Nil(t, logs) + assert.NoError(t, err) + assert.Empty(t, logs) +} + +// TestTranslator_AccessLogSinks_DecoupledFromStdoutToggle pins the invariant that +// router.access_logs.enabled governs only the operator-facing stdout log line, +// never the gRPC ALS sink. ALS is the sole data source for traffic logging and +// analytics (the policy-engine publishes exclusively on receipt of an ALS entry), +// so gating it on the stdout toggle silently disables both with no error anywhere. +func TestTranslator_AccessLogSinks_DecoupledFromStdoutToggle(t *testing.T) { + const ( + fileSink = "envoy.access_loggers.file" + grpcSink = "envoy.access_loggers.http_grpc" + ) + + tests := []struct { + name string + stdoutEnabled bool + trafficLogging bool + analytics bool + wantSinkNames []string + wantManagerSinks bool + }{ + { + name: "stdout off, traffic logging on -> ALS sink only", + stdoutEnabled: false, + trafficLogging: true, + wantSinkNames: []string{grpcSink}, + wantManagerSinks: true, + }, + { + name: "stdout off, analytics on -> ALS sink only", + stdoutEnabled: false, + analytics: true, + wantSinkNames: []string{grpcSink}, + wantManagerSinks: true, + }, + { + name: "stdout on, traffic logging on -> both sinks", + stdoutEnabled: true, + trafficLogging: true, + wantSinkNames: []string{fileSink, grpcSink}, + wantManagerSinks: true, + }, + { + name: "stdout on, collector off -> stdout sink only", + stdoutEnabled: true, + wantSinkNames: []string{fileSink}, + wantManagerSinks: true, + }, + { + name: "both off -> no sinks at all", + stdoutEnabled: false, + wantSinkNames: nil, + wantManagerSinks: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + logger := createTestLogger() + routerCfg := testRouterConfig() + routerCfg.AccessLogs = config.AccessLogsConfig{ + Enabled: tt.stdoutEnabled, + Format: "text", + TextFormat: "[%START_TIME%] %RESPONSE_CODE%", + } + cfg := testConfig() + cfg.Router = *routerCfg + cfg.TrafficLogging.Enabled = tt.trafficLogging + cfg.Analytics.Enabled = tt.analytics + cfg.Collector.Server = config.GRPCEventServerConfig{ + Mode: "tcp", + BufferFlushInterval: 1000, + BufferSizeBytes: 16384, + GRPCRequestTimeout: 5000, + } + translator := NewTranslator(logger, routerCfg, nil, cfg) + + logs, err := translator.createAccessLogConfig() + require.NoError(t, err) + + gotNames := make([]string, 0, len(logs)) + for _, l := range logs { + gotNames = append(gotNames, l.Name) + } + assert.Equal(t, tt.wantSinkNames, nilIfEmpty(gotNames), "access log sinks") + + // The sinks must actually reach the HCM — the caller's gate is half the fix. + lis, _, err := translator.createListener(nil, false) + require.NoError(t, err) + manager := extractHCM(t, lis) + if !tt.wantManagerSinks { + assert.Empty(t, manager.GetAccessLog(), "no sink should be attached to the HCM") + return + } + managerNames := make([]string, 0, len(manager.GetAccessLog())) + for _, l := range manager.GetAccessLog() { + managerNames = append(managerNames, l.Name) + } + assert.Equal(t, tt.wantSinkNames, managerNames, "access log sinks attached to the HCM") + }) + } +} + +func nilIfEmpty(s []string) []string { + if len(s) == 0 { + return nil + } + return s } func TestTranslator_CreateAccessLogConfig_JSON(t *testing.T) { From b7e1d554b137a5c49f62643fafbcb52b6ee35254 Mon Sep 17 00:00:00 2001 From: Dineth Date: Tue, 11 Aug 2026 11:14:23 +0530 Subject: [PATCH 3/3] make error --- gateway/gateway-controller/pkg/xds/translator.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/gateway/gateway-controller/pkg/xds/translator.go b/gateway/gateway-controller/pkg/xds/translator.go index 82e36821c..bcd4f7598 100644 --- a/gateway/gateway-controller/pkg/xds/translator.go +++ b/gateway/gateway-controller/pkg/xds/translator.go @@ -2635,16 +2635,17 @@ func (t *Translator) createAccessLogConfig() ([]*accesslog.AccessLog, error) { accessLogs = append(accessLogs, fileAccessLog) } - // If the collector is active, create the gRPC access log config and append to existing access logs + // If the collector is active, create the gRPC access log config and append to existing access logs. + // The failure is fatal rather than a warning: ALS is the collector's only data + // source, so continuing without it would push a listener that silently reports + // nothing to traffic logging and analytics while both are enabled in config. if t.config.IsCollectorEnabled() { t.logger.Info("Creating gRPC access log configuration") grpcAccessLog, err := t.createGRPCAccessLog() if err != nil { - t.logger.Warn("Failed to create gRPC access log config, continuing without it", - slog.Any("error", err)) - } else { - accessLogs = append(accessLogs, grpcAccessLog) + return nil, fmt.Errorf("failed to create gRPC access log config: %w", err) } + accessLogs = append(accessLogs, grpcAccessLog) } return accessLogs, nil