Make new stat regex backwards compatible (istio#52031)

* Make new stat regex backwards compatible Signed-off-by: Keith Mattix II <keithmattix@microsoft.com> * Small optimize Signed-off-by: Keith Mattix II <keithmattix@microsoft.com> * One more cleanup Signed-off-by: Keith Mattix II <keithmattix@microsoft.com> --------- Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
MorrisLaw · Jul 18, 2024 · a1d8add · a1d8add
1 parent d030756
commit a1d8add
Show file tree

Hide file tree

Showing 32 changed files with 128 additions and 95 deletions.
diff --git a/pilot/pkg/model/context.go b/pilot/pkg/model/context.go
@@ -391,8 +391,6 @@ type Proxy struct {
 
 type WatchedResource = xds.WatchedResource
 
-var istioVersionRegexp = regexp.MustCompile(`^([1-9]+)\.([0-9]+)(\.([0-9]+))?`)
-
 // GetView returns a restricted view of the mesh for this proxy. The view can be
 // restricted by network (via ISTIO_META_REQUESTED_NETWORK_VIEW).
 // If not set, we assume that the proxy wants to see endpoints in any network.
@@ -427,6 +425,33 @@ func (node *Proxy) IsAmbient() bool {
 	return node.IsWaypointProxy() || node.IsZTunnel()
 }
 
+var NodeTypes = [...]NodeType{SidecarProxy, Router, Waypoint, Ztunnel}
+
+// SetSidecarScope identifies the sidecar scope object associated with this
+// proxy and updates the proxy Node. This is a convenience hack so that
+// callers can simply call push.Services(node) while the implementation of
+// push.Services can return the set of services from the proxyNode's
+// sidecar scope or from the push context's set of global services. Similar
+// logic applies to push.VirtualServices and push.DestinationRule. The
+// short cut here is useful only for CDS and parts of RDS generation code.
+//
+// Listener generation code will still use the SidecarScope object directly
+// as it needs the set of services for each listener port.
+func (node *Proxy) SetSidecarScope(ps *PushContext) {
+	sidecarScope := node.SidecarScope
+
+	switch node.Type {
+	case SidecarProxy:
+		node.SidecarScope = ps.getSidecarScope(node, node.Labels)
+	case Router, Waypoint:
+		// Gateways should just have a default scope with egress: */*
+		node.SidecarScope = ps.getSidecarScope(node, nil)
+	}
+	node.PrevSidecarScope = sidecarScope
+}
+
+var istioVersionRegexp = regexp.MustCompile(`^([1-9]+)\.([0-9]+)(\.([0-9]+))?`)
+
 // IstioVersion encodes the Istio version of the proxy. This is a low key way to
 // do semver style comparisons and generate the appropriate envoy config
 type IstioVersion struct {
@@ -437,6 +462,32 @@ type IstioVersion struct {
 
 var MaxIstioVersion = &IstioVersion{Major: 65535, Minor: 65535, Patch: 65535}
 
+// ParseIstioVersion parses a version string and returns IstioVersion struct
+func ParseIstioVersion(ver string) *IstioVersion {
+	// strip the release- prefix if any and extract the version string
+	ver = istioVersionRegexp.FindString(strings.TrimPrefix(ver, "release-"))
+
+	if ver == "" {
+		// return very large values assuming latest version
+		return MaxIstioVersion
+	}
+
+	parts := strings.Split(ver, ".")
+	// we are guaranteed to have at least major and minor based on the regex
+	major, _ := strconv.Atoi(parts[0])
+	minor, _ := strconv.Atoi(parts[1])
+	// Assume very large patch release if not set
+	patch := 65535
+	if len(parts) > 2 {
+		patch, _ = strconv.Atoi(parts[2])
+	}
+	return &IstioVersion{Major: major, Minor: minor, Patch: patch}
+}
+
+func (pversion *IstioVersion) String() string {
+	return fmt.Sprintf("%d.%d.%d", pversion.Major, pversion.Minor, pversion.Patch)
+}
+
 // Compare returns -1/0/1 if version is less than, equal or greater than inv
 // To compare only on major, call this function with { X, -1, -1}.
 // to compare only on major & minor, call this function with {X, Y, -1}.
@@ -472,32 +523,7 @@ func compareVersion(ov, nv int) int {
 	return 1
 }
 
-var NodeTypes = [...]NodeType{SidecarProxy, Router, Waypoint, Ztunnel}
-
-// SetSidecarScope identifies the sidecar scope object associated with this
-// proxy and updates the proxy Node. This is a convenience hack so that
-// callers can simply call push.Services(node) while the implementation of
-// push.Services can return the set of services from the proxyNode's
-// sidecar scope or from the push context's set of global services. Similar
-// logic applies to push.VirtualServices and push.DestinationRule. The
-// short cut here is useful only for CDS and parts of RDS generation code.
-//
-// Listener generation code will still use the SidecarScope object directly
-// as it needs the set of services for each listener port.
-func (node *Proxy) SetSidecarScope(ps *PushContext) {
-	sidecarScope := node.SidecarScope
-
-	switch node.Type {
-	case SidecarProxy:
-		node.SidecarScope = ps.getSidecarScope(node, node.Labels)
-	case Router, Waypoint:
-		// Gateways should just have a default scope with egress: */*
-		node.SidecarScope = ps.getSidecarScope(node, nil)
-	}
-	node.PrevSidecarScope = sidecarScope
-}
-
-func (node *Proxy) VersionGreaterOrEqual(inv *IstioVersion) bool {
+func (node *Proxy) VersionGreaterAndEqual(inv *IstioVersion) bool {
 	if inv == nil {
 		return true
 	}
@@ -668,28 +694,6 @@ func ParseServiceNodeWithMetadata(nodeID string, metadata *NodeMetadata) (*Proxy
 	return out, nil
 }
 
-// ParseIstioVersion parses a version string and returns IstioVersion struct
-func ParseIstioVersion(ver string) *IstioVersion {
-	// strip the release- prefix if any and extract the version string
-	ver = istioVersionRegexp.FindString(strings.TrimPrefix(ver, "release-"))
-
-	if ver == "" {
-		// return very large values assuming latest version
-		return MaxIstioVersion
-	}
-
-	parts := strings.Split(ver, ".")
-	// we are guaranteed to have at least major and minor based on the regex
-	major, _ := strconv.Atoi(parts[0])
-	minor, _ := strconv.Atoi(parts[1])
-	// Assume very large patch release if not set
-	patch := 65535
-	if len(parts) > 2 {
-		patch, _ = strconv.Atoi(parts[2])
-	}
-	return &IstioVersion{Major: major, Minor: minor, Patch: patch}
-}
-
 // GetOrDefault returns either the value, or the default if the value is empty. Useful when retrieving node metadata fields.
 func GetOrDefault(s string, def string) string {
 	return pm.GetOrDefault(s, def)

diff --git a/pilot/pkg/networking/core/cluster.go b/pilot/pkg/networking/core/cluster.go
@@ -225,7 +225,7 @@ func (configgen *ConfigGeneratorImpl) buildClusters(proxy *model.Proxy, req *mod
 		inboundPatcher := clusterPatcher{efw: envoyFilterPatches, pctx: networking.EnvoyFilter_SIDECAR_INBOUND}
 		clusters = append(clusters, configgen.buildInboundClusters(cb, proxy, instances, inboundPatcher)...)
 		if proxy.EnableHBONEListen() {
-			clusters = append(clusters, configgen.buildInboundHBONEClusters())
+			clusters = append(clusters, configgen.buildInboundHBONEClusters(proxy.IstioVersion))
 		}
 		// Pass through clusters for inbound traffic. These cluster bind loopback-ish src address to access node local service.
 		clusters = inboundPatcher.conditionallyAppend(clusters, nil, cb.buildInboundPassthroughCluster())

diff --git a/pilot/pkg/networking/core/cluster_builder.go b/pilot/pkg/networking/core/cluster_builder.go
@@ -89,7 +89,7 @@ type ClusterBuilder struct {
 	metadataCerts      *metadataCerts        // Client certificates specified in metadata.
 	clusterID          string                // Cluster in which proxy is running.
 	proxyID            string                // Identifier that uniquely identifies a proxy.
-	proxyVersion       string                // Version of Proxy.
+	proxyVersion       *model.IstioVersion   // Version of Proxy.
 	proxyType          model.NodeType        // Indicates whether the proxy is sidecar or gateway.
 	sidecarScope       *model.SidecarScope   // Computed sidecar for the proxy.
 	passThroughBindIPs []string              // Passthrough IPs to be used while building clusters.
@@ -113,7 +113,7 @@ func NewClusterBuilder(proxy *model.Proxy, req *model.PushRequest, cache model.X
 		serviceTargets:     proxy.ServiceTargets,
 		proxyID:            proxy.ID,
 		proxyType:          proxy.Type,
-		proxyVersion:       proxy.Metadata.IstioVersion,
+		proxyVersion:       model.ParseIstioVersion(proxy.Metadata.IstioVersion),
 		sidecarScope:       proxy.SidecarScope,
 		passThroughBindIPs: getPassthroughBindIPs(proxy.GetIPMode()),
 		supportsIPv4:       proxy.SupportsIPv4(),
@@ -293,10 +293,14 @@ func (cb *ClusterBuilder) buildCluster(name string, discoveryType cluster.Cluste
 ) *clusterWrapper {
 	c := &cluster.Cluster{
 		Name:                 name,
-		AltStatName:          name + constants.ClusterAltStatNameDelimeter,
 		ClusterDiscoveryType: &cluster.Cluster_Type{Type: discoveryType},
 		CommonLbConfig:       &cluster.Cluster_CommonLbConfig{},
 	}
+
+	if util.IsIstioVersionGE123(cb.proxyVersion) {
+		c.AltStatName = name + constants.ClusterAltStatNameDelimeter
+	}
+
 	switch discoveryType {
 	case cluster.Cluster_STRICT_DNS, cluster.Cluster_LOGICAL_DNS:
 		if networkutil.AllIPv4(cb.proxyIPAddresses) {
@@ -351,7 +355,9 @@ func (cb *ClusterBuilder) buildCluster(name string, discoveryType cluster.Cluste
 			statPrefix := telemetry.BuildStatPrefix(cb.req.Push.Mesh.OutboundClusterStatName,
 				string(service.Hostname), subset, port, 0, &service.Attributes)
 
-			if statPrefix[len(statPrefix)-1:] != constants.ClusterAltStatNameDelimeter {
+			// 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
 			}
 			ec.cluster.AltStatName = statPrefix
@@ -385,7 +391,8 @@ func (cb *ClusterBuilder) buildInboundCluster(clusterPort int, bind string,
 			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 {
+		if statPrefix[len(statPrefix)-1:] != constants.ClusterAltStatNameDelimeter &&
+			util.IsIstioVersionGE123(cb.proxyVersion) {
 			statPrefix += constants.ClusterAltStatNameDelimeter
 		}
 		localCluster.cluster.AltStatName = statPrefix
@@ -510,11 +517,13 @@ func (cb *ClusterBuilder) buildInboundPassthroughCluster() *cluster.Cluster {
 func (cb *ClusterBuilder) buildBlackHoleCluster() *cluster.Cluster {
 	c := &cluster.Cluster{
 		Name:                 util.BlackHoleCluster,
-		AltStatName:          util.BlackHoleCluster + constants.ClusterAltStatNameDelimeter,
 		ClusterDiscoveryType: &cluster.Cluster_Type{Type: cluster.Cluster_STATIC},
 		ConnectTimeout:       proto.Clone(cb.req.Push.Mesh.ConnectTimeout).(*durationpb.Duration),
 		LbPolicy:             cluster.Cluster_ROUND_ROBIN,
 	}
+	if util.IsIstioVersionGE123(cb.proxyVersion) {
+		c.AltStatName = util.BlackHoleCluster + constants.ClusterAltStatNameDelimeter
+	}
 	return c
 }
 
@@ -523,14 +532,16 @@ func (cb *ClusterBuilder) buildBlackHoleCluster() *cluster.Cluster {
 func (cb *ClusterBuilder) buildDefaultPassthroughCluster() *cluster.Cluster {
 	cluster := &cluster.Cluster{
 		Name:                 util.PassthroughCluster,
-		AltStatName:          util.PassthroughCluster + constants.ClusterAltStatNameDelimeter,
 		ClusterDiscoveryType: &cluster.Cluster_Type{Type: cluster.Cluster_ORIGINAL_DST},
 		ConnectTimeout:       proto.Clone(cb.req.Push.Mesh.ConnectTimeout).(*durationpb.Duration),
 		LbPolicy:             cluster.Cluster_CLUSTER_PROVIDED,
 		TypedExtensionProtocolOptions: map[string]*anypb.Any{
 			v3.HttpProtocolOptionsType: passthroughHttpProtocolOptions,
 		},
 	}
+	if util.IsIstioVersionGE123(cb.proxyVersion) {
+		cluster.AltStatName = util.PassthroughCluster + constants.ClusterAltStatNameDelimeter
+	}
 	cb.applyConnectionPool(cb.req.Push.Mesh, newClusterWrapper(cluster), &networking.ConnectionPoolSettings{})
 	cb.applyMetadataExchange(cluster)
 	return cluster
@@ -730,7 +741,6 @@ func (cb *ClusterBuilder) buildExternalSDSCluster(addr string) *cluster.Cluster
 	}
 	c := &cluster.Cluster{
 		Name:                 security.SDSExternalClusterName,
-		AltStatName:          security.SDSExternalClusterName + constants.ClusterAltStatNameDelimeter,
 		ClusterDiscoveryType: &cluster.Cluster_Type{Type: cluster.Cluster_STATIC},
 		ConnectTimeout:       proto.Clone(cb.req.Push.Mesh.ConnectTimeout).(*durationpb.Duration),
 		LoadAssignment: &endpoint.ClusterLoadAssignment{
@@ -745,6 +755,9 @@ func (cb *ClusterBuilder) buildExternalSDSCluster(addr string) *cluster.Cluster
 			v3.HttpProtocolOptionsType: protoconv.MessageToAny(options),
 		},
 	}
+	if util.IsIstioVersionGE123(cb.proxyVersion) {
+		c.AltStatName = security.SDSExternalClusterName + constants.ClusterAltStatNameDelimeter
+	}
 	return c
 }
 

diff --git a/pilot/pkg/networking/core/cluster_cache.go b/pilot/pkg/networking/core/cluster_cache.go
@@ -189,7 +189,7 @@ func buildClusterKey(service *model.Service, port *model.Port, cb *ClusterBuilde
 	}
 	return clusterCache{
 		clusterName:     clusterName,
-		proxyVersion:    cb.proxyVersion,
+		proxyVersion:    cb.proxyVersion.String(),
 		locality:        cb.locality,
 		proxyClusterID:  cb.clusterID,
 		proxySidecar:    cb.sidecarProxy(),

diff --git a/pilot/pkg/networking/core/cluster_waypoint.go b/pilot/pkg/networking/core/cluster_waypoint.go
@@ -49,10 +49,9 @@ import (
 )
 
 // buildInternalUpstreamCluster builds a single endpoint cluster to the internal listener.
-func buildInternalUpstreamCluster(name string, internalListener string) *cluster.Cluster {
-	return &cluster.Cluster{
+func buildInternalUpstreamCluster(proxyVersion *model.IstioVersion, name string, internalListener string) *cluster.Cluster {
+	c := &cluster.Cluster{
 		Name:                 name,
-		AltStatName:          name + constants.ClusterAltStatNameDelimeter,
 		ClusterDiscoveryType: &cluster.Cluster_Type{Type: cluster.Cluster_STATIC},
 		LoadAssignment: &endpoint.ClusterLoadAssignment{
 			ClusterName: name,
@@ -63,15 +62,26 @@ func buildInternalUpstreamCluster(name string, internalListener string) *cluster
 			v3.HttpProtocolOptionsType: passthroughHttpProtocolOptions,
 		},
 	}
+
+	if proxyVersion != nil && proxyVersion.Minor >= 23 {
+		c.AltStatName = name + constants.ClusterAltStatNameDelimeter
+	}
+
+	return c
 }
 
 var (
-	MainInternalCluster = buildInternalUpstreamCluster(MainInternalName, MainInternalName)
-	EncapCluster        = buildInternalUpstreamCluster(EncapClusterName, ConnectOriginate)
+	GetMainInternalCluster = func(v *model.IstioVersion) *cluster.Cluster {
+		return buildInternalUpstreamCluster(v, MainInternalName, MainInternalName)
+	}
+
+	GetEncapCluster = func(v *model.IstioVersion) *cluster.Cluster {
+		return buildInternalUpstreamCluster(v, EncapClusterName, ConnectOriginate)
+	}
 )
 
-func (configgen *ConfigGeneratorImpl) buildInboundHBONEClusters() *cluster.Cluster {
-	return MainInternalCluster
+func (configgen *ConfigGeneratorImpl) buildInboundHBONEClusters(v *model.IstioVersion) *cluster.Cluster {
+	return GetMainInternalCluster(v)
 }
 
 func (configgen *ConfigGeneratorImpl) buildWaypointInboundClusters(
@@ -83,7 +93,7 @@ func (configgen *ConfigGeneratorImpl) buildWaypointInboundClusters(
 	clusters := make([]*cluster.Cluster, 0)
 	// Creates "main_internal" cluster to route to the main internal listener.
 	// Creates "encap" cluster to route to the encap listener.
-	clusters = append(clusters, MainInternalCluster, EncapCluster)
+	clusters = append(clusters, GetMainInternalCluster(proxy.IstioVersion), GetEncapCluster(proxy.IstioVersion))
 	// Creates per-VIP load balancing upstreams.
 	clusters = append(clusters, cb.buildWaypointInboundVIP(proxy, svcs, push.Mesh)...)
 	// Upstream of the "encap" listener.

diff --git a/pilot/pkg/networking/core/route/route.go b/pilot/pkg/networking/core/route/route.go
@@ -1176,7 +1176,7 @@ func TranslateCORSPolicy(proxy *model.Proxy, in *networking.CorsPolicy) *cors.Co
 	out := cors.CorsPolicy{}
 	// Start from Envoy 1.30(istio 1.22), cors filter will not forward preflight requests to upstream by default.
 	// Istio start support this feature from 1.23.
-	if proxy.VersionGreaterOrEqual(&model.IstioVersion{Major: 1, Minor: 23, Patch: -1}) {
+	if proxy.VersionGreaterAndEqual(&model.IstioVersion{Major: 1, Minor: 23, Patch: -1}) {
 		out.ForwardNotMatchingPreflights = forwardNotMatchingPreflights(in)
 	}
 

diff --git a/pilot/pkg/networking/util/util.go b/pilot/pkg/networking/util/util.go
@@ -452,6 +452,12 @@ func MergeAnyWithAny(dst *anypb.Any, src *anypb.Any) (*anypb.Any, error) {
 	return retVal, nil
 }
 
+// IsIstioVersionGE123 checks whether the given Istio version is greater than or equals 1.23.
+func IsIstioVersionGE123(version *model.IstioVersion) bool {
+	return version == nil ||
+		version.Compare(&model.IstioVersion{Major: 1, Minor: 23, Patch: -1}) >= 0
+}
+
 // AppendLbEndpointMetadata adds metadata values to a lb endpoint using the passed in metadata as base.
 func AppendLbEndpointMetadata(istioMetadata *model.EndpointMetadata, envoyMetadata *core.Metadata,
 ) {
@@ -881,5 +887,5 @@ func ShallowCopyTrafficPolicy(original *networking.TrafficPolicy) *networking.Tr
 }
 
 func VersionGreaterOrEqual124(proxy *model.Proxy) bool {
-	return proxy.VersionGreaterOrEqual(&model.IstioVersion{Major: 1, Minor: 24, Patch: -1})
+	return proxy.VersionGreaterAndEqual(&model.IstioVersion{Major: 1, Minor: 24, Patch: -1})
 }
diff --git a/pkg/bootstrap/config.go b/pkg/bootstrap/config.go
@@ -115,7 +115,8 @@ func (cfg Config) toTemplateParams() (map[string]any, error) {
 		option.XdsType(xdsType),
 		option.MetadataDiscovery(bool(metadataDiscovery)),
 		option.MetricsLocalhostAccessOnly(cfg.Metadata.ProxyConfig.ProxyMetadata),
-		option.DeferredClusterCreation(features.EnableDeferredClusterCreation))
+		option.DeferredClusterCreation(features.EnableDeferredClusterCreation),
+	)
 
 	// Add GCPProjectNumber to access in bootstrap template.
 	md := cfg.Metadata.PlatformMetadata

diff --git a/pkg/bootstrap/instance_test.go b/pkg/bootstrap/instance_test.go
@@ -479,7 +479,6 @@ func checkClusterNameTag(t *testing.T, regex string) {
 			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])

diff --git a/pkg/bootstrap/testdata/all_golden.json b/pkg/bootstrap/testdata/all_golden.json
@@ -40,7 +40,7 @@
     "stats_tags": [
       {
         "tag_name": "cluster_name",
-        "regex": "cluster(\\.(.+);)"
+        "regex": "^cluster(\\.(.+);)"
       },
       {
         "tag_name": "tcp_prefix",

diff --git a/pkg/bootstrap/testdata/auth_golden.json b/pkg/bootstrap/testdata/auth_golden.json
@@ -40,7 +40,7 @@
     "stats_tags": [
       {
         "tag_name": "cluster_name",
-        "regex": "cluster(\\.(.+);)"
+        "regex": "^cluster(\\.(.+);)"
       },
       {
         "tag_name": "tcp_prefix",

diff --git a/pkg/bootstrap/testdata/authsds_golden.json b/pkg/bootstrap/testdata/authsds_golden.json
@@ -40,7 +40,7 @@
     "stats_tags": [
       {
         "tag_name": "cluster_name",
-        "regex": "cluster(\\.(.+);)"
+        "regex": "^cluster(\\.(.+);)"
       },
       {
         "tag_name": "tcp_prefix",

diff --git a/pkg/bootstrap/testdata/default_golden.json b/pkg/bootstrap/testdata/default_golden.json
@@ -40,7 +40,7 @@
     "stats_tags": [
       {
         "tag_name": "cluster_name",
-        "regex": "cluster(\\.(.+);)"
+        "regex": "^cluster(\\.(.+);)"
       },
       {
         "tag_name": "tcp_prefix",