From bd4d2a7b4ee2a5b733853f1fa23d6bc3e24acff2 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 3 Apr 2024 16:57:23 -0400 Subject: [PATCH 1/3] Process string metadata in CDS for com.google.csm.telemetry_labels --- .../xdsclient/xdsresource/type_cds.go | 3 + .../xdsclient/xdsresource/unmarshal_cds.go | 13 +++ .../xdsresource/unmarshal_cds_test.go | 93 +++++++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/xds/internal/xdsclient/xdsresource/type_cds.go b/xds/internal/xdsclient/xdsresource/type_cds.go index b59eb9c33883..1a9742fb70b1 100644 --- a/xds/internal/xdsclient/xdsresource/type_cds.go +++ b/xds/internal/xdsclient/xdsresource/type_cds.go @@ -85,4 +85,7 @@ type ClusterUpdate struct { // Raw is the resource from the xds response. Raw *anypb.Any + // All the string valued metadata of filter_metadata type + // "com.google.csm.telemetry_labels". + StringMD map[string]string } diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_cds.go b/xds/internal/xdsclient/xdsresource/unmarshal_cds.go index c5b751d4d8a9..625addc15080 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_cds.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_cds.go @@ -39,6 +39,7 @@ import ( "google.golang.org/grpc/xds/internal/xdsclient/xdsresource/version" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/anypb" + "google.golang.org/protobuf/types/known/structpb" ) // ValidateClusterAndConstructClusterUpdateForTesting exports the @@ -81,6 +82,17 @@ const ( ) func validateClusterAndConstructClusterUpdate(cluster *v3clusterpb.Cluster) (ClusterUpdate, error) { + stringMD := make(map[string]string) + if fmd := cluster.GetMetadata().GetFilterMetadata(); fmd != nil { + if val, ok := fmd["com.google.csm.telemetry_labels"]; ok { + for key, val := range val.GetFields() { + if _, ok := val.GetKind().(*structpb.Value_StringValue); ok { + stringMD[key] = val.GetStringValue() + } + } + } + } + var lbPolicy json.RawMessage var err error switch cluster.GetLbPolicy() { @@ -160,6 +172,7 @@ func validateClusterAndConstructClusterUpdate(cluster *v3clusterpb.Cluster) (Clu MaxRequests: circuitBreakersFromCluster(cluster), LBPolicy: lbPolicy, OutlierDetection: od, + StringMD: stringMD, } // Note that this is different from the gRFC (gRFC A47 says to include the diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go b/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go index 638cf91980dc..5a7a4ae8633e 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go @@ -41,6 +41,7 @@ import ( v3matcherpb "github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3" "google.golang.org/protobuf/types/known/anypb" "google.golang.org/protobuf/types/known/durationpb" + "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -1216,6 +1217,69 @@ func (s) TestUnmarshalCluster(t *testing.T) { }, }, }) + + v3ClusterAnyWithTelemetryLabels = testutils.MarshalAny(t, &v3clusterpb.Cluster{ + Name: v3ClusterName, + ClusterDiscoveryType: &v3clusterpb.Cluster_Type{Type: v3clusterpb.Cluster_EDS}, + EdsClusterConfig: &v3clusterpb.Cluster_EdsClusterConfig{ + EdsConfig: &v3corepb.ConfigSource{ + ConfigSourceSpecifier: &v3corepb.ConfigSource_Ads{ + Ads: &v3corepb.AggregatedConfigSource{}, + }, + }, + ServiceName: v3Service, + }, + LbPolicy: v3clusterpb.Cluster_ROUND_ROBIN, + LrsServer: &v3corepb.ConfigSource{ + ConfigSourceSpecifier: &v3corepb.ConfigSource_Self{ + Self: &v3corepb.SelfConfigSource{}, + }, + }, + Metadata: &v3corepb.Metadata{ + FilterMetadata: map[string]*structpb.Struct{ + "com.google.csm.telemetry_labels": { + Fields: map[string]*structpb.Value{ + "service_name": structpb.NewStringValue("grpc-service"), + "service_namespace": structpb.NewStringValue("grpc-service-namespace"), + }, + }, + }, + }, + }) + v3ClusterAnyWithTelemetryLabelsIgnoreSome = testutils.MarshalAny(t, &v3clusterpb.Cluster{ + Name: v3ClusterName, + ClusterDiscoveryType: &v3clusterpb.Cluster_Type{Type: v3clusterpb.Cluster_EDS}, + EdsClusterConfig: &v3clusterpb.Cluster_EdsClusterConfig{ + EdsConfig: &v3corepb.ConfigSource{ + ConfigSourceSpecifier: &v3corepb.ConfigSource_Ads{ + Ads: &v3corepb.AggregatedConfigSource{}, + }, + }, + ServiceName: v3Service, + }, + LbPolicy: v3clusterpb.Cluster_ROUND_ROBIN, + LrsServer: &v3corepb.ConfigSource{ + ConfigSourceSpecifier: &v3corepb.ConfigSource_Self{ + Self: &v3corepb.SelfConfigSource{}, + }, + }, + Metadata: &v3corepb.Metadata{ + FilterMetadata: map[string]*structpb.Struct{ + "com.google.csm.telemetry_labels": { + Fields: map[string]*structpb.Value{ + "string-value-dont-ignore": structpb.NewStringValue("string-val"), + "float-value-ignore": structpb.NewNumberValue(3), + "bool-value-ignore": structpb.NewBoolValue(false), + }, + }, + "ignore-this-metadata": { + Fields: map[string]*structpb.Value{ + "string-value-should-ignore": structpb.NewStringValue("string-val-should-ignore"), + }, + }, + }, + }, + }) ) tests := []struct { @@ -1300,6 +1364,35 @@ func (s) TestUnmarshalCluster(t *testing.T) { Raw: v3ClusterAnyWithEDSConfigSourceSelf, }, }, + { + name: "v3 cluster with telemetry case", + resource: v3ClusterAnyWithTelemetryLabels, + wantName: v3ClusterName, + wantUpdate: ClusterUpdate{ + ClusterName: v3ClusterName, + EDSServiceName: v3Service, + LRSServerConfig: ClusterLRSServerSelf, + Raw: v3ClusterAnyWithTelemetryLabels, + StringMD: map[string]string{ + "service_name": "grpc-service", + "service_namespace": "grpc-service-namespace", + }, + }, + }, + { + name: "v3 metadata ignore other types not string and not com.google.csm.telemetry_labels", + resource: v3ClusterAnyWithTelemetryLabelsIgnoreSome, + wantName: v3ClusterName, + wantUpdate: ClusterUpdate{ + ClusterName: v3ClusterName, + EDSServiceName: v3Service, + LRSServerConfig: ClusterLRSServerSelf, + Raw: v3ClusterAnyWithTelemetryLabelsIgnoreSome, + StringMD: map[string]string{ + "string-value-dont-ignore": "string-val", + }, + }, + }, { name: "xdstp cluster resource with unset EDS service name", resource: testutils.MarshalAny(t, &v3clusterpb.Cluster{ From 20092f494e797989744a0a47c97907f16e8895a0 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 3 Apr 2024 19:20:54 -0400 Subject: [PATCH 2/3] Switch to only parse csm specific string labels --- .../xdsclient/xdsresource/unmarshal_cds.go | 17 ++++++++++++++--- .../xdsclient/xdsresource/unmarshal_cds_test.go | 14 ++++++++------ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_cds.go b/xds/internal/xdsclient/xdsresource/unmarshal_cds.go index 625addc15080..a308589d4e80 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_cds.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_cds.go @@ -85,9 +85,20 @@ func validateClusterAndConstructClusterUpdate(cluster *v3clusterpb.Cluster) (Clu stringMD := make(map[string]string) if fmd := cluster.GetMetadata().GetFilterMetadata(); fmd != nil { if val, ok := fmd["com.google.csm.telemetry_labels"]; ok { - for key, val := range val.GetFields() { - if _, ok := val.GetKind().(*structpb.Value_StringValue); ok { - stringMD[key] = val.GetStringValue() + /* + "service_name" = "", + "service_namespace" = "" + */ + if fields := val.GetFields(); fields != nil { + if val, ok := fields["service_name"]; ok { + if _, isStringVal := val.GetKind().(*structpb.Value_StringValue); isStringVal { + stringMD["service_name"] = val.GetStringValue() + } + } + if val, ok := fields["service_namespace"]; ok { + if _, isStringVal := val.GetKind().(*structpb.Value_StringValue); isStringVal { + stringMD["service_namespace"] = val.GetStringValue() + } } } } diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go b/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go index 5a7a4ae8633e..124220236863 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go @@ -1267,14 +1267,16 @@ func (s) TestUnmarshalCluster(t *testing.T) { FilterMetadata: map[string]*structpb.Struct{ "com.google.csm.telemetry_labels": { Fields: map[string]*structpb.Value{ - "string-value-dont-ignore": structpb.NewStringValue("string-val"), - "float-value-ignore": structpb.NewNumberValue(3), - "bool-value-ignore": structpb.NewBoolValue(false), + "string-value-should-ignore": structpb.NewStringValue("string-val"), + "float-value-ignore": structpb.NewNumberValue(3), + "bool-value-ignore": structpb.NewBoolValue(false), + "service_name": structpb.NewStringValue("grpc-service"), // shouldn't ignore + "service_namespace": structpb.NewNullValue(), // should ignore - wrong type }, }, - "ignore-this-metadata": { + "ignore-this-metadata": { // should ignore this filter_metadata Fields: map[string]*structpb.Value{ - "string-value-should-ignore": structpb.NewStringValue("string-val-should-ignore"), + "service_namespace": structpb.NewStringValue("string-val-should-ignore"), }, }, }, @@ -1389,7 +1391,7 @@ func (s) TestUnmarshalCluster(t *testing.T) { LRSServerConfig: ClusterLRSServerSelf, Raw: v3ClusterAnyWithTelemetryLabelsIgnoreSome, StringMD: map[string]string{ - "string-value-dont-ignore": "string-val", + "service_name": "grpc-service", }, }, }, From dcc290ce05494ac917835f2c280f59d152a63bfc Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 5 Apr 2024 15:51:19 -0400 Subject: [PATCH 3/3] Responded to Doug's comments --- xds/internal/xdsclient/xdsresource/type_cds.go | 7 ++++--- .../xdsclient/xdsresource/unmarshal_cds.go | 16 ++++++---------- .../xdsclient/xdsresource/unmarshal_cds_test.go | 4 ++-- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/xds/internal/xdsclient/xdsresource/type_cds.go b/xds/internal/xdsclient/xdsresource/type_cds.go index 1a9742fb70b1..b782e2455492 100644 --- a/xds/internal/xdsclient/xdsresource/type_cds.go +++ b/xds/internal/xdsclient/xdsresource/type_cds.go @@ -85,7 +85,8 @@ type ClusterUpdate struct { // Raw is the resource from the xds response. Raw *anypb.Any - // All the string valued metadata of filter_metadata type - // "com.google.csm.telemetry_labels". - StringMD map[string]string + // TelemetryLabels are the string valued metadata of filter_metadata type + // "com.google.csm.telemetry_labels" with keys "service_name" or + // "service_namespace". + TelemetryLabels map[string]string } diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_cds.go b/xds/internal/xdsclient/xdsresource/unmarshal_cds.go index a308589d4e80..276bf405e330 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_cds.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_cds.go @@ -82,22 +82,18 @@ const ( ) func validateClusterAndConstructClusterUpdate(cluster *v3clusterpb.Cluster) (ClusterUpdate, error) { - stringMD := make(map[string]string) + telemetryLabels := make(map[string]string) if fmd := cluster.GetMetadata().GetFilterMetadata(); fmd != nil { if val, ok := fmd["com.google.csm.telemetry_labels"]; ok { - /* - "service_name" = "", - "service_namespace" = "" - */ if fields := val.GetFields(); fields != nil { if val, ok := fields["service_name"]; ok { - if _, isStringVal := val.GetKind().(*structpb.Value_StringValue); isStringVal { - stringMD["service_name"] = val.GetStringValue() + if _, ok := val.GetKind().(*structpb.Value_StringValue); ok { + telemetryLabels["service_name"] = val.GetStringValue() } } if val, ok := fields["service_namespace"]; ok { - if _, isStringVal := val.GetKind().(*structpb.Value_StringValue); isStringVal { - stringMD["service_namespace"] = val.GetStringValue() + if _, ok := val.GetKind().(*structpb.Value_StringValue); ok { + telemetryLabels["service_namespace"] = val.GetStringValue() } } } @@ -183,7 +179,7 @@ func validateClusterAndConstructClusterUpdate(cluster *v3clusterpb.Cluster) (Clu MaxRequests: circuitBreakersFromCluster(cluster), LBPolicy: lbPolicy, OutlierDetection: od, - StringMD: stringMD, + TelemetryLabels: telemetryLabels, } // Note that this is different from the gRFC (gRFC A47 says to include the diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go b/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go index 124220236863..a345466961c2 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_cds_test.go @@ -1375,7 +1375,7 @@ func (s) TestUnmarshalCluster(t *testing.T) { EDSServiceName: v3Service, LRSServerConfig: ClusterLRSServerSelf, Raw: v3ClusterAnyWithTelemetryLabels, - StringMD: map[string]string{ + TelemetryLabels: map[string]string{ "service_name": "grpc-service", "service_namespace": "grpc-service-namespace", }, @@ -1390,7 +1390,7 @@ func (s) TestUnmarshalCluster(t *testing.T) { EDSServiceName: v3Service, LRSServerConfig: ClusterLRSServerSelf, Raw: v3ClusterAnyWithTelemetryLabelsIgnoreSome, - StringMD: map[string]string{ + TelemetryLabels: map[string]string{ "service_name": "grpc-service", }, },