[release-1.24] fix filterStateObjectsToLog (#53788)

* fix filterStateObjectsToLog * lint --------- Co-authored-by: zirain <[email protected]>
istio · Nov 5, 2024 · 8825a6b · 8825a6b
1 parent 5d5aa1f
commit 8825a6b
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 16 deletions.
diff --git a/pilot/pkg/model/telemetry.go b/pilot/pkg/model/telemetry.go
@@ -269,7 +269,7 @@ func (t *Telemetries) AccessLogging(push *PushContext, proxy *Proxy, class netwo
 			Disabled: v.Disabled,
 		}
 
-		al := telemetryAccessLog(push, fp)
+		al := telemetryAccessLog(push, proxy, fp)
 		if al == nil {
 			// stackdriver will be handled in HTTPFilters/TCPFilters
 			continue

diff --git a/pilot/pkg/model/telemetry_logging.go b/pilot/pkg/model/telemetry_logging.go
@@ -110,6 +110,8 @@ var (
 	// We need to propagate these as part of access log service stream
 	// Logging them by default on the console may be an issue as the base64 encoded string is bound to be a big one.
 	// But end users can certainly configure it on their own via the meshConfig using the %FILTER_STATE% macro.
+	// Related to https://github.com/istio/proxy/pull/5825, start from 1.24, Istio use new filter state key.
+	envoyStateToLog     = []string{"upstream_peer", "downstream_peer"}
 	envoyWasmStateToLog = []string{"wasm.upstream_peer", "wasm.upstream_peer_id", "wasm.downstream_peer", "wasm.downstream_peer_id"}
 
 	// reqWithoutQueryFormatter configures additional formatters needed for some of the format strings like "REQ_WITHOUT_QUERY"
@@ -138,7 +140,7 @@ var (
 // STOP. DO NOT UPDATE THIS WITHOUT UPDATING telemetryAccessLog.
 const telemetryAccessLogHandled = 14
 
-func telemetryAccessLog(push *PushContext, fp *meshconfig.MeshConfig_ExtensionProvider) *accesslog.AccessLog {
+func telemetryAccessLog(push *PushContext, proxy *Proxy, fp *meshconfig.MeshConfig_ExtensionProvider) *accesslog.AccessLog {
 	var al *accesslog.AccessLog
 	switch prov := fp.Provider.(type) {
 	case *meshconfig.MeshConfig_ExtensionProvider_EnvoyFileAccessLog:
@@ -149,11 +151,11 @@ func telemetryAccessLog(push *PushContext, fp *meshconfig.MeshConfig_ExtensionPr
 			al = fileAccessLogFromTelemetry(prov.EnvoyFileAccessLog)
 		}
 	case *meshconfig.MeshConfig_ExtensionProvider_EnvoyHttpAls:
-		al = httpGrpcAccessLogFromTelemetry(push, prov.EnvoyHttpAls)
+		al = httpGrpcAccessLogFromTelemetry(push, proxy, prov.EnvoyHttpAls)
 	case *meshconfig.MeshConfig_ExtensionProvider_EnvoyTcpAls:
-		al = tcpGrpcAccessLogFromTelemetry(push, prov.EnvoyTcpAls)
+		al = tcpGrpcAccessLogFromTelemetry(push, proxy, prov.EnvoyTcpAls)
 	case *meshconfig.MeshConfig_ExtensionProvider_EnvoyOtelAls:
-		al = openTelemetryLog(push, prov.EnvoyOtelAls)
+		al = openTelemetryLog(push, proxy, prov.EnvoyOtelAls)
 	case *meshconfig.MeshConfig_ExtensionProvider_EnvoyExtAuthzHttp,
 		*meshconfig.MeshConfig_ExtensionProvider_EnvoyExtAuthzGrpc,
 		*meshconfig.MeshConfig_ExtensionProvider_Zipkin,
@@ -172,13 +174,23 @@ func telemetryAccessLog(push *PushContext, fp *meshconfig.MeshConfig_ExtensionPr
 	return al
 }
 
-func tcpGrpcAccessLogFromTelemetry(push *PushContext, prov *meshconfig.MeshConfig_ExtensionProvider_EnvoyTcpGrpcV3LogProvider) *accesslog.AccessLog {
+func filterStateObjectsToLog(proxy *Proxy) []string {
+	if proxy.VersionGreaterAndEqual(&IstioVersion{Major: 1, Minor: 24, Patch: 0}) {
+		return envoyStateToLog
+	}
+
+	return envoyWasmStateToLog
+}
+
+func tcpGrpcAccessLogFromTelemetry(push *PushContext, proxy *Proxy,
+	prov *meshconfig.MeshConfig_ExtensionProvider_EnvoyTcpGrpcV3LogProvider,
+) *accesslog.AccessLog {
 	logName := TCPEnvoyAccessLogFriendlyName
 	if prov != nil && prov.LogName != "" {
 		logName = prov.LogName
 	}
 
-	filterObjects := envoyWasmStateToLog
+	filterObjects := filterStateObjectsToLog(proxy)
 	if len(prov.FilterStateObjectsToLog) != 0 {
 		filterObjects = prov.FilterStateObjectsToLog
 	}
@@ -332,13 +344,15 @@ func accessLogTextFormatters(text string) []*core.TypedExtensionConfig {
 	return formatters
 }
 
-func httpGrpcAccessLogFromTelemetry(push *PushContext, prov *meshconfig.MeshConfig_ExtensionProvider_EnvoyHttpGrpcV3LogProvider) *accesslog.AccessLog {
+func httpGrpcAccessLogFromTelemetry(push *PushContext, proxy *Proxy,
+	prov *meshconfig.MeshConfig_ExtensionProvider_EnvoyHttpGrpcV3LogProvider,
+) *accesslog.AccessLog {
 	logName := HTTPEnvoyAccessLogFriendlyName
 	if prov != nil && prov.LogName != "" {
 		logName = prov.LogName
 	}
 
-	filterObjects := envoyWasmStateToLog
+	filterObjects := filterStateObjectsToLog(proxy)
 	if len(prov.FilterStateObjectsToLog) != 0 {
 		filterObjects = prov.FilterStateObjectsToLog
 	}
@@ -443,7 +457,7 @@ func FileAccessLogFromMeshConfig(path string, mesh *meshconfig.MeshConfig) *acce
 	return al
 }
 
-func openTelemetryLog(pushCtx *PushContext,
+func openTelemetryLog(pushCtx *PushContext, proxy *Proxy,
 	provider *meshconfig.MeshConfig_ExtensionProvider_EnvoyOpenTelemetryLogProvider,
 ) *accesslog.AccessLog {
 	hostname, cluster, err := clusterLookupFn(pushCtx, provider.Service, int(provider.Port))
@@ -468,15 +482,17 @@ func openTelemetryLog(pushCtx *PushContext,
 		labels = provider.LogFormat.Labels
 	}
 
-	cfg := buildOpenTelemetryAccessLogConfig(logName, hostname, cluster, f, labels)
+	cfg := buildOpenTelemetryAccessLogConfig(proxy, logName, hostname, cluster, f, labels)
 
 	return &accesslog.AccessLog{
 		Name:       OtelEnvoyALSName,
 		ConfigType: &accesslog.AccessLog_TypedConfig{TypedConfig: protoconv.MessageToAny(cfg)},
 	}
 }
 
-func buildOpenTelemetryAccessLogConfig(logName, hostname, clusterName, format string, labels *structpb.Struct) *otelaccesslog.OpenTelemetryAccessLogConfig {
+func buildOpenTelemetryAccessLogConfig(proxy *Proxy,
+	logName, hostname, clusterName, format string, labels *structpb.Struct,
+) *otelaccesslog.OpenTelemetryAccessLogConfig {
 	cfg := &otelaccesslog.OpenTelemetryAccessLogConfig{
 		CommonConfig: &grpcaccesslog.CommonGrpcAccessLogConfig{
 			LogName: logName,
@@ -489,7 +505,7 @@ func buildOpenTelemetryAccessLogConfig(logName, hostname, clusterName, format st
 				},
 			},
 			TransportApiVersion:     core.ApiVersion_V3,
-			FilterStateObjectsToLog: envoyWasmStateToLog,
+			FilterStateObjectsToLog: filterStateObjectsToLog(proxy),
 		},
 		DisableBuiltinLabels: true,
 	}

diff --git a/pilot/pkg/model/telemetry_logging_test.go b/pilot/pkg/model/telemetry_logging_test.go
@@ -959,6 +959,13 @@ func TestAccessLoggingCache(t *testing.T) {
 }
 
 func TestBuildOpenTelemetryAccessLogConfig(t *testing.T) {
+	sidecar := &Proxy{
+		ConfigNamespace: "default",
+		Labels:          map[string]string{"app": "test"},
+		Metadata:        &NodeMetadata{},
+		IstioVersion:    &IstioVersion{Major: 1, Minor: 23},
+	}
+
 	fakeCluster := "outbound|55680||otel-collector.monitoring.svc.cluster.local"
 	fakeAuthority := "otel-collector.monitoring.svc.cluster.local"
 	for _, tc := range []struct {
@@ -1042,7 +1049,7 @@ func TestBuildOpenTelemetryAccessLogConfig(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			got := buildOpenTelemetryAccessLogConfig(tc.logName, tc.hostname, tc.clusterName, tc.body, tc.labels)
+			got := buildOpenTelemetryAccessLogConfig(sidecar, tc.logName, tc.hostname, tc.clusterName, tc.body, tc.labels)
 			assert.Equal(t, tc.expected, got)
 		})
 	}
@@ -1430,6 +1437,13 @@ func TestTelemetryAccessLog(t *testing.T) {
 		},
 	}
 
+	sidecar := &Proxy{
+		ConfigNamespace: "default",
+		Labels:          map[string]string{"app": "test"},
+		Metadata:        &NodeMetadata{},
+		IstioVersion:    &IstioVersion{Major: 1, Minor: 23},
+	}
+
 	for _, tc := range []struct {
 		name       string
 		ctx        *PushContext
@@ -1597,7 +1611,7 @@ func TestTelemetryAccessLog(t *testing.T) {
 			}
 			push.Mesh = tc.meshConfig
 
-			got := telemetryAccessLog(push, tc.fp)
+			got := telemetryAccessLog(push, sidecar, tc.fp)
 			if got == nil {
 				t.Fatal("get nil accesslog")
 			}
@@ -1912,3 +1926,36 @@ func TestAccessLogFormatters(t *testing.T) {
 		})
 	}
 }
+
+func TestFilterStateObjectsToLog(t *testing.T) {
+	cases := []struct {
+		proxy    *Proxy
+		expected []string
+	}{
+		{
+			proxy: &Proxy{
+				IstioVersion: &IstioVersion{Major: 1, Minor: 23},
+			},
+			expected: []string{"wasm.upstream_peer", "wasm.upstream_peer_id", "wasm.downstream_peer", "wasm.downstream_peer_id"},
+		},
+		{
+			proxy: &Proxy{
+				IstioVersion: &IstioVersion{Major: 1, Minor: 24},
+			},
+			expected: []string{"upstream_peer", "downstream_peer"},
+		},
+		{
+			proxy: &Proxy{
+				IstioVersion: &IstioVersion{Major: 1, Minor: 25},
+			},
+			expected: []string{"upstream_peer", "downstream_peer"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run("", func(t *testing.T) {
+			got := filterStateObjectsToLog(tc.proxy)
+			assert.Equal(t, tc.expected, got)
+		})
+	}
+}
diff --git a/pilot/pkg/networking/core/accesslog.go b/pilot/pkg/networking/core/accesslog.go
@@ -49,7 +49,8 @@ var (
 	// We need to propagate these as part of access log service stream
 	// Logging them by default on the console may be an issue as the base64 encoded string is bound to be a big one.
 	// But end users can certainly configure it on their own via the meshConfig using the %FILTER_STATE% macro.
-	envoyWasmStateToLog = []string{"wasm.upstream_peer", "wasm.upstream_peer_id", "wasm.downstream_peer", "wasm.downstream_peer_id"}
+	envoyWasmStateToLog = []string{"upstream_peer", "downstream_peer", // start from 1.24.0
+		"wasm.upstream_peer", "wasm.upstream_peer_id", "wasm.downstream_peer", "wasm.downstream_peer_id"}
 
 	// accessLogBuilder is used to set accessLog to filters
 	accessLogBuilder = newAccessLogBuilder()

diff --git a/releasenotes/notes/53778.yaml b/releasenotes/notes/53778.yaml
@@ -0,0 +1,6 @@
+apiVersion: release-notes/v2
+kind: bug-fix
+area: telemetry
+releaseNotes:
+  - |
+    **Fixed** an issue that should use `upstream_peer` instead of `wasm.upstream_peer` in `filterStateObjectsToLog`.