Fix http_conn_manager stats (istio#52234)

* Fix http_conn_manager stats Signed-off-by: Keith Mattix II <keithmattix@microsoft.com> * Make gen Signed-off-by: Keith Mattix II <keithmattix@microsoft.com> * Fix lint Signed-off-by: Keith Mattix II <keithmattix@microsoft.com> * Make lint and gen Signed-off-by: Keith Mattix II <keithmattix@microsoft.com> --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
MorrisLaw · Jul 23, 2024 · d7eea93 · d7eea93
1 parent a9786a3
commit d7eea93
Show file tree

Hide file tree

Showing 34 changed files with 200 additions and 60 deletions.
diff --git a/pilot/pkg/networking/core/cluster_builder.go b/pilot/pkg/networking/core/cluster_builder.go
@@ -298,7 +298,7 @@ func (cb *ClusterBuilder) buildCluster(name string, discoveryType cluster.Cluste
 	}
 
 	if util.IsIstioVersionGE123(cb.proxyVersion) {
-		c.AltStatName = name + constants.ClusterAltStatNameDelimeter
+		c.AltStatName = name + constants.StatPrefixDelimiter
 	}
 
 	switch discoveryType {
@@ -352,14 +352,11 @@ func (cb *ClusterBuilder) buildCluster(name string, discoveryType cluster.Cluste
 	if direction == model.TrafficDirectionOutbound {
 		// If stat name is configured, build the alternate stats name.
 		if len(cb.req.Push.Mesh.OutboundClusterStatName) != 0 {
-			statPrefix := telemetry.BuildStatPrefix(cb.req.Push.Mesh.OutboundClusterStatName,
-				string(service.Hostname), subset, port, 0, &service.Attributes)
-
-			// Add the cluster name delimeter if it's not the last character and the proxy version >= 1.23.
-			if statPrefix[len(statPrefix)-1:] != constants.ClusterAltStatNameDelimeter &&
-				util.IsIstioVersionGE123(cb.proxyVersion) {
-				statPrefix += constants.ClusterAltStatNameDelimeter
+			statPrefix := telemetry.BuildStatPrefix(cb.req.Push.Mesh.OutboundClusterStatName, string(service.Hostname), subset, port, 0, &service.Attributes)
+			if util.IsIstioVersionGE123(cb.proxyVersion) && statPrefix[len(statPrefix)-1:] != constants.StatPrefixDelimiter {
+				statPrefix += constants.StatPrefixDelimiter
 			}
+
 			ec.cluster.AltStatName = statPrefix
 		}
 	}
@@ -390,11 +387,10 @@ func (cb *ClusterBuilder) buildInboundCluster(clusterPort int, bind string,
 		statPrefix := telemetry.BuildStatPrefix(cb.req.Push.Mesh.InboundClusterStatName,
 			string(instance.Service.Hostname), "", instance.Port.ServicePort, clusterPort,
 			&instance.Service.Attributes)
-		// Add the cluster name delimeter if it's not the last character.
-		if statPrefix[len(statPrefix)-1:] != constants.ClusterAltStatNameDelimeter &&
-			util.IsIstioVersionGE123(cb.proxyVersion) {
-			statPrefix += constants.ClusterAltStatNameDelimeter
+		if util.IsIstioVersionGE123(cb.proxyVersion) && statPrefix[len(statPrefix)-1:] != constants.StatPrefixDelimiter {
+			statPrefix += constants.StatPrefixDelimiter
 		}
+
 		localCluster.cluster.AltStatName = statPrefix
 	}
 
@@ -522,7 +518,7 @@ func (cb *ClusterBuilder) buildBlackHoleCluster() *cluster.Cluster {
 		LbPolicy:             cluster.Cluster_ROUND_ROBIN,
 	}
 	if util.IsIstioVersionGE123(cb.proxyVersion) {
-		c.AltStatName = util.BlackHoleCluster + constants.ClusterAltStatNameDelimeter
+		c.AltStatName = util.BlackHoleCluster + constants.StatPrefixDelimiter
 	}
 	return c
 }
@@ -540,7 +536,7 @@ func (cb *ClusterBuilder) buildDefaultPassthroughCluster() *cluster.Cluster {
 		},
 	}
 	if util.IsIstioVersionGE123(cb.proxyVersion) {
-		cluster.AltStatName = util.PassthroughCluster + constants.ClusterAltStatNameDelimeter
+		cluster.AltStatName = util.PassthroughCluster + constants.StatPrefixDelimiter
 	}
 	cb.applyConnectionPool(cb.req.Push.Mesh, newClusterWrapper(cluster), &networking.ConnectionPoolSettings{})
 	cb.applyMetadataExchange(cluster)
@@ -756,7 +752,7 @@ func (cb *ClusterBuilder) buildExternalSDSCluster(addr string) *cluster.Cluster
 		},
 	}
 	if util.IsIstioVersionGE123(cb.proxyVersion) {
-		c.AltStatName = security.SDSExternalClusterName + constants.ClusterAltStatNameDelimeter
+		c.AltStatName = security.SDSExternalClusterName + constants.StatPrefixDelimiter
 	}
 	return c
 }

diff --git a/pilot/pkg/networking/core/cluster_waypoint.go b/pilot/pkg/networking/core/cluster_waypoint.go
@@ -63,8 +63,8 @@ func buildInternalUpstreamCluster(proxyVersion *model.IstioVersion, name string,
 		},
 	}
 
-	if proxyVersion != nil && proxyVersion.Minor >= 23 {
-		c.AltStatName = name + constants.ClusterAltStatNameDelimeter
+	if util.IsIstioVersionGE123(proxyVersion) {
+		c.AltStatName = name + constants.StatPrefixDelimiter
 	}
 
 	return c
@@ -299,9 +299,8 @@ func (cb *ClusterBuilder) buildConnectOriginate(proxy *model.Proxy, push *model.
 	}
 	// Compliance for Envoy tunnel upstreams.
 	sec_model.EnforceCompliance(ctx)
-	return &cluster.Cluster{
+	c := &cluster.Cluster{
 		Name:                          ConnectOriginate,
-		AltStatName:                   ConnectOriginate + constants.ClusterAltStatNameDelimeter,
 		ClusterDiscoveryType:          &cluster.Cluster_Type{Type: cluster.Cluster_ORIGINAL_DST},
 		LbPolicy:                      cluster.Cluster_CLUSTER_PROVIDED,
 		ConnectTimeout:                durationpb.New(2 * time.Second),
@@ -330,6 +329,12 @@ func (cb *ClusterBuilder) buildConnectOriginate(proxy *model.Proxy, push *model.
 			})},
 		},
 	}
+
+	if util.IsIstioVersionGE123(proxy.IstioVersion) {
+		c.AltStatName = ConnectOriginate + constants.StatPrefixDelimiter
+	}
+
+	return c
 }
 
 func h2connectUpgrade() map[string]*anypb.Any {

diff --git a/pilot/pkg/networking/core/gateway.go b/pilot/pkg/networking/core/gateway.go
@@ -44,6 +44,7 @@ import (
 	"istio.io/istio/pilot/pkg/networking/util"
 	"istio.io/istio/pilot/pkg/util/protoconv"
 	"istio.io/istio/pkg/config"
+	"istio.io/istio/pkg/config/constants"
 	"istio.io/istio/pkg/config/gateway"
 	"istio.io/istio/pkg/config/host"
 	"istio.io/istio/pkg/config/protocol"
@@ -97,6 +98,9 @@ func (ml *MutableGatewayListener) build(builder *ListenerBuilder, opts gatewayLi
 			// If statPrefix has been set before calling this method, respect that.
 			if len(opt.httpOpts.statPrefix) == 0 {
 				opt.httpOpts.statPrefix = strings.ToLower(ml.Listener.TrafficDirection.String()) + "_" + ml.Listener.Name
+				if util.IsIstioVersionGE123(builder.node.IstioVersion) {
+					opt.httpOpts.statPrefix += constants.StatPrefixDelimiter
+				}
 			}
 			opt.httpOpts.port = opts.port
 			httpConnectionManagers[i] = builder.buildHTTPConnectionManager(opt.httpOpts)

diff --git a/pilot/pkg/networking/core/listener.go b/pilot/pkg/networking/core/listener.go
@@ -613,6 +613,9 @@ func buildListenerFromEntry(builder *ListenerBuilder, le *outboundListenerEntry,
 			chain.Filters = opt.networkFilters
 		} else {
 			opt.httpOpts.statPrefix = strings.ToLower(l.TrafficDirection.String()) + "_" + l.Name
+			if util.IsIstioVersionGE123(builder.node.IstioVersion) {
+				opt.httpOpts.statPrefix += constants.StatPrefixDelimiter
+			}
 			opt.httpOpts.port = le.servicePort.Port
 			hcm := builder.buildHTTPConnectionManager(opt.httpOpts)
 			filter := &listener.Filter{

diff --git a/pilot/pkg/networking/core/listener_inbound.go b/pilot/pkg/networking/core/listener_inbound.go
@@ -39,6 +39,7 @@ import (
 	"istio.io/istio/pilot/pkg/serviceregistry/provider"
 	"istio.io/istio/pilot/pkg/util/protoconv"
 	xdsfilters "istio.io/istio/pilot/pkg/xds/filters"
+	"istio.io/istio/pkg/config/constants"
 	"istio.io/istio/pkg/config/host"
 	"istio.io/istio/pkg/config/protocol"
 	"istio.io/istio/pkg/config/security"
@@ -91,12 +92,20 @@ type inboundChainConfig struct {
 }
 
 // StatPrefix returns the stat prefix for the config
-func (cc inboundChainConfig) StatPrefix() string {
+func (cc inboundChainConfig) StatPrefix(istioVersion *model.IstioVersion) string {
+	var statPrefix string
 	if cc.passthrough {
 		// A bit arbitrary, but for backwards compatibility just use the cluster name
-		return cc.clusterName
+		statPrefix = cc.clusterName
+	} else {
+		statPrefix = "inbound_" + cc.Name(istionetworking.ListenerProtocolHTTP)
+	}
+
+	if util.IsIstioVersionGE123(istioVersion) {
+		statPrefix += constants.StatPrefixDelimiter
 	}
-	return "inbound_" + cc.Name(istionetworking.ListenerProtocolHTTP)
+
+	return statPrefix
 }
 
 // Name determines the name for this chain
@@ -807,7 +816,7 @@ func buildSidecarInboundHTTPOpts(lb *ListenerBuilder, cc inboundChainConfig) *ht
 		protocol:                  cc.port.Protocol,
 		class:                     istionetworking.ListenerClassSidecarInbound,
 		port:                      int(cc.port.TargetPort),
-		statPrefix:                cc.StatPrefix(),
+		statPrefix:                cc.StatPrefix(lb.node.IstioVersion),
 		hbone:                     cc.hbone,
 	}
 	// See https://github.com/grpc/grpc-web/tree/master/net/grpc/gateway/examples/helloworld#configure-the-proxy

diff --git a/pilot/pkg/networking/core/listener_test.go b/pilot/pkg/networking/core/listener_test.go
@@ -49,6 +49,7 @@ import (
 	xdsfilters "istio.io/istio/pilot/pkg/xds/filters"
 	"istio.io/istio/pilot/test/xdstest"
 	"istio.io/istio/pkg/config"
+	"istio.io/istio/pkg/config/constants"
 	"istio.io/istio/pkg/config/host"
 	"istio.io/istio/pkg/config/mesh"
 	"istio.io/istio/pkg/config/protocol"
@@ -985,16 +986,23 @@ func TestInboundHTTPListenerConfig(t *testing.T) {
 	svc := buildService("test.com", wildcardIPv4, protocol.HTTP, tnow)
 	for _, p := range []*model.Proxy{getProxy(), &proxyHTTP10, &dualStackProxy} {
 		cases := []struct {
-			name     string
-			p        *model.Proxy
-			cfg      []config.Config
-			services []*model.Service
+			name                 string
+			p                    *model.Proxy
+			cfg                  []config.Config
+			services             []*model.Service
+			istioVersionOverride *model.IstioVersion
 		}{
 			{
 				name:     "simple",
 				p:        p,
 				services: []*model.Service{svc},
 			},
+			{
+				name:                 "simple (< 1.23)",
+				p:                    p,
+				services:             []*model.Service{svc},
+				istioVersionOverride: &model.IstioVersion{Major: 1, Minor: 22, Patch: 0},
+			},
 			{
 				name:     "sidecar with service",
 				p:        p,
@@ -1011,6 +1019,20 @@ func TestInboundHTTPListenerConfig(t *testing.T) {
 		for _, tt := range cases {
 			t.Run(tt.name, func(t *testing.T) {
 				t.Helper()
+				if tt.istioVersionOverride != nil {
+					// Create a new proxy object with the overridden version
+					p := &model.Proxy{
+						Type:            tt.p.Type,
+						IPAddresses:     tt.p.IPAddresses,
+						ID:              tt.p.ID,
+						DNSDomain:       tt.p.DNSDomain,
+						Metadata:        tt.p.Metadata,
+						ConfigNamespace: tt.p.ConfigNamespace,
+						IstioVersion:    tt.istioVersionOverride,
+					}
+					p.DiscoverIPMode()
+					tt.p = p
+				}
 				listeners := buildListeners(t, TestOptions{
 					Services: tt.services,
 					Configs:  tt.cfg,
@@ -1027,11 +1049,15 @@ func TestInboundHTTPListenerConfig(t *testing.T) {
 							},
 							ValidateHCM: func(t test.Failer, hcm *hcm.HttpConnectionManager) {
 								assert.Equal(t, "istio-envoy", hcm.GetServerName(), "server name")
+								statPrefixDelimeter := constants.StatPrefixDelimiter
+								if tt.istioVersionOverride != nil && !util.IsIstioVersionGE123(tt.istioVersionOverride) {
+									statPrefixDelimeter = ""
+								}
 								if len(tt.cfg) == 0 {
-									assert.Equal(t, "inbound_0.0.0.0_8080", hcm.GetStatPrefix(), "stat prefix")
+									assert.Equal(t, "inbound_0.0.0.0_8080"+statPrefixDelimeter, hcm.GetStatPrefix(), "stat prefix")
 								} else {
 									// Sidecar impacts stat prefix
-									assert.Equal(t, "inbound_1.1.1.1_8080", hcm.GetStatPrefix(), "stat prefix")
+									assert.Equal(t, "inbound_1.1.1.1_8080"+statPrefixDelimeter, hcm.GetStatPrefix(), "stat prefix")
 								}
 								assert.Equal(t, "APPEND_FORWARD", hcm.GetForwardClientCertDetails().String(), "forward client cert details")
 								assert.Equal(t, true, hcm.GetSetCurrentClientCertDetails().GetSubject().GetValue(), "subject")

diff --git a/pilot/pkg/networking/core/listener_waypoint.go b/pilot/pkg/networking/core/listener_waypoint.go
@@ -415,7 +415,7 @@ func (lb *ListenerBuilder) buildWaypointInboundHTTPFilters(svc *model.Service, c
 		suppressEnvoyDebugHeaders: ph.SuppressDebugHeaders,
 		protocol:                  cc.port.Protocol,
 		class:                     istionetworking.ListenerClassSidecarInbound,
-		statPrefix:                cc.StatPrefix(),
+		statPrefix:                cc.StatPrefix(lb.node.IstioVersion),
 		isWaypoint:                true,
 		policySvc:                 svc,
 	}

diff --git a/pilot/pkg/networking/core/tls.go b/pilot/pkg/networking/core/tls.go
@@ -182,7 +182,8 @@ func buildSidecarOutboundTLSFilterChainOpts(node *model.Proxy, push *model.PushC
 		statPrefix := clusterName
 		// If stat name is configured, use it to build the stat prefix.
 		if len(push.Mesh.OutboundClusterStatName) != 0 {
-			statPrefix = telemetry.BuildStatPrefix(push.Mesh.OutboundClusterStatName, string(service.Hostname), "", &model.Port{Port: port}, 0, &service.Attributes)
+			statPrefix = telemetry.BuildStatPrefix(push.Mesh.OutboundClusterStatName, string(service.Hostname),
+				"", &model.Port{Port: port}, 0, &service.Attributes)
 		}
 		// Use the hostname as the SNI value if and only:
 		// 1) if the destination is a CIDR;
@@ -319,7 +320,8 @@ TcpLoop:
 			model.TrafficDirectionOutbound, node, service.Hostname).GetRule())
 		// If stat name is configured, use it to build the stat prefix.
 		if len(push.Mesh.OutboundClusterStatName) != 0 {
-			statPrefix = telemetry.BuildStatPrefix(push.Mesh.OutboundClusterStatName, string(service.Hostname), "", &model.Port{Port: port}, 0, &service.Attributes)
+			statPrefix = telemetry.BuildStatPrefix(push.Mesh.OutboundClusterStatName, string(service.Hostname), "",
+				&model.Port{Port: port}, 0, &service.Attributes)
 		}
 		out = append(out, &filterChainOpts{
 			destinationCIDRs: destinationCIDRs,

diff --git a/pilot/pkg/networking/grpcgen/cds.go b/pilot/pkg/networking/grpcgen/cds.go
@@ -154,7 +154,7 @@ func (b *clusterBuilder) build() []*cluster.Cluster {
 func edsCluster(name string) *cluster.Cluster {
 	return &cluster.Cluster{
 		Name:                 name,
-		AltStatName:          name + constants.ClusterAltStatNameDelimeter,
+		AltStatName:          name + constants.StatPrefixDelimiter,
 		ClusterDiscoveryType: &cluster.Cluster_Type{Type: cluster.Cluster_EDS},
 		EdsClusterConfig: &cluster.Cluster_EdsClusterConfig{
 			ServiceName: name,

diff --git a/pkg/bootstrap/instance_test.go b/pkg/bootstrap/instance_test.go
@@ -423,14 +423,109 @@ func checkOpencensusConfig(t *testing.T, got, want *bootstrap.Bootstrap) {
 func checkStatsTags(t *testing.T, got *bootstrap.Bootstrap) {
 	for _, tag := range got.StatsConfig.StatsTags {
 		// TODO: Add checks for other tags
-		if tag.TagName == "cluster_name" {
+		switch tag.TagName {
+		case "cluster_name":
 			checkClusterNameTag(t, tag.GetRegex())
+		case "http_conn_manager_prefix":
+			checkHTTPConnManagerPrefixTag(t, tag.GetRegex())
 		}
 	}
 }
 
 // Envoy will remove the first capture group and set the tag to the second capture group.
-// We double check that the regex returns what we want here.
+// We double check that all of the regexes we set return what we want here.
+
+func checkHTTPConnManagerPrefixTag(t *testing.T, regex string) {
+	if regex == "" {
+		t.Fatalf("cluster_name tag regex is empty")
+	}
+
+	compiledRegex, err := regexp.Compile(regex)
+	if err != nil {
+		t.Fatalf("invalid regex for cluster_name tag: %v", err)
+	}
+
+	tc := []struct {
+		name               string
+		statPrefix         string
+		firstCaptureGroup  string
+		secondCaptureGroup string
+	}{
+		{
+			name:               "http_conn_manager_prefix stats tag - ipv4 address - single-segment",
+			statPrefix:         "http.0.0.0.0;.downstream_rq_total",
+			firstCaptureGroup:  "0.0.0.0;",
+			secondCaptureGroup: "0.0.0.0",
+		},
+		{
+			name:               "http_conn_manager_prefix stats tag - ipv4 address - multi-segment",
+			statPrefix:         "http.0.0.0.0;.jwt_authn.allowed",
+			firstCaptureGroup:  "0.0.0.0;",
+			secondCaptureGroup: "0.0.0.0",
+		},
+		{
+			name:               "http_conn_manager_prefix stats tag - ipv6 address - single-segment",
+			statPrefix:         "http.2001:0000:130F:0000:0000:09C0:876A:130B;.downstream_rq_total",
+			firstCaptureGroup:  "2001:0000:130F:0000:0000:09C0:876A:130B;",
+			secondCaptureGroup: "2001:0000:130F:0000:0000:09C0:876A:130B",
+		},
+		{
+			name:               "http_conn_manager_prefix stats tag - ipv6 address - multi-segment",
+			statPrefix:         "http.2001:0000:130F:0000:0000:09C0:876A:130B;.jwt_authn.allowed",
+			firstCaptureGroup:  "2001:0000:130F:0000:0000:09C0:876A:130B;",
+			secondCaptureGroup: "2001:0000:130F:0000:0000:09C0:876A:130B",
+		},
+		{
+			name:               "http_conn_manager_prefix stats tag - direction-prefixed ipv4 - single-segment",
+			statPrefix:         "http.inbound_0.0.0.0;.downstream_rq_total",
+			firstCaptureGroup:  "inbound_0.0.0.0;",
+			secondCaptureGroup: "inbound_0.0.0.0",
+		},
+		{
+			name:               "http_conn_manager_prefix stats tag - direction-prefixed ipv4 - multi-segment",
+			statPrefix:         "http.inbound_0.0.0.0;.jwt_authn.allowed",
+			firstCaptureGroup:  "inbound_0.0.0.0;",
+			secondCaptureGroup: "inbound_0.0.0.0",
+		},
+		{
+			name:               "http_conn_manager_prefix stats tag - direction-prefixed ipv6 - single-segment",
+			statPrefix:         "http.inbound_2001:0000:130F:0000:0000:09C0:876A:130B;.downstream_rq_total",
+			firstCaptureGroup:  "inbound_2001:0000:130F:0000:0000:09C0:876A:130B;",
+			secondCaptureGroup: "inbound_2001:0000:130F:0000:0000:09C0:876A:130B",
+		},
+		{
+			name:               "http_conn_manager_prefix stats tag - direction-prefixed ipv6 - multi-segment",
+			statPrefix:         "http.inbound_2001:0000:130F:0000:0000:09C0:876A:130B;.jwt_authn.allowed",
+			firstCaptureGroup:  "inbound_2001:0000:130F:0000:0000:09C0:876A:130B;",
+			secondCaptureGroup: "inbound_2001:0000:130F:0000:0000:09C0:876A:130B",
+		},
+	}
+
+	for _, tt := range tc {
+		t.Run(tt.name, func(t *testing.T) {
+			subMatches := compiledRegex.FindStringSubmatch(tt.statPrefix)
+			if subMatches == nil {
+				t.Fatalf("cluster_name tag regex does not match cluster name %s", tt.statPrefix)
+			}
+
+			// The first index is the whole match followed by N number of capture groups.
+			// There are 2 capture groups we expect in the regex, so we always check for 3
+			if len(subMatches) != 3 {
+				t.Fatalf("unexpected number of capture groups: %d. Submatches: %v", len(subMatches), subMatches)
+			}
+			// Now we examine both of the capture groups (which start at index 1)
+			if subMatches[1] != tt.firstCaptureGroup {
+				t.Fatalf("first capture group does not match %s, got %s", tt.firstCaptureGroup, subMatches[1])
+			}
+
+			// Finally, check the second capture group
+			if subMatches[2] != tt.secondCaptureGroup {
+				t.Fatalf("second capture group does not match %s, got %s", tt.secondCaptureGroup, subMatches[2])
+			}
+		})
+	}
+}
+
 func checkClusterNameTag(t *testing.T, regex string) {
 	if regex == "" {
 		t.Fatalf("cluster_name tag regex is empty")