[APIP] Traffic-logging fixes

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,47 @@ 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.
+	// 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 {
+			return nil, fmt.Errorf("failed to create gRPC access log config: %w", err)
+		}
+		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 +2708,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
@@ -2721,13 +2748,34 @@ 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

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) {
@@ -2163,7 +2269,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 +2410,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