Switch from istio_proxy_version to istio_version (istio#15886)

* nuke proxy version Signed-off-by: Shriram Rajagopalan <[email protected]> * lint Signed-off-by: Shriram Rajagopalan <[email protected]> * bump makefile version Signed-off-by: Shriram Rajagopalan <[email protected]> * sem ver check Signed-off-by: Shriram Rajagopalan <[email protected]> * fixes Signed-off-by: Shriram Rajagopalan <[email protected]> * fixes Signed-off-by: Shriram Rajagopalan <[email protected]> * fix test Signed-off-by: Shriram Rajagopalan <[email protected]>
mfilipiak-ccg · Aug 1, 2019 · 1bc2241 · 1bc2241
1 parent 93df5e4
commit 1bc2241
Show file tree

Hide file tree

Showing 28 changed files with 251 additions and 168 deletions.
diff --git a/Makefile b/Makefile
@@ -20,7 +20,7 @@ export ISTIO_GO
 SHELL := /bin/bash -o pipefail
 
 # Current version, updated after a release.
-VERSION ?= 1.0-dev
+VERSION ?= 1.3-dev
 
 # locations where artifacts are stored
 ISTIO_DOCKER_HUB ?= docker.io/istio

diff --git a/istioctl/cmd/testdata/auth/productpage_config_dump.json b/istioctl/cmd/testdata/auth/productpage_config_dump.json
@@ -16,7 +16,6 @@
       "INSTANCE_IPS": "10.52.2.21,fe80::54f8:2cff:fe2b:2dd3",
       "POD_NAME": "productpage-v1-5cb458d74f-56sx7",
       "istio": "sidecar",
-      "ISTIO_PROXY_VERSION": "1.1.0",
       "ISTIO_PROXY_SHA": "istio-proxy:55c80965eab994e6bfa2227e3942fa89928d0d70",
       "app": "productpage"
      },

diff --git a/pilot/docker/Dockerfile.proxy_debug b/pilot/docker/Dockerfile.proxy_debug
@@ -11,8 +11,6 @@ LABEL istio-api-vcs-ref=$ISTIO_API_SHA \
 # Install Envoy.
 COPY envoy /usr/local/bin/envoy
 
-# Environment variables indicating this proxy's version/capabilities as opaque string
-ENV ISTIO_META_ISTIO_PROXY_VERSION "1.1.3"
 # Environment variable indicating the exact proxy sha - for debugging or version-specific configs 
 ENV ISTIO_META_ISTIO_PROXY_SHA $proxy_version
 # Environment variable indicating the exact build, for debugging

diff --git a/pilot/docker/Dockerfile.proxytproxy b/pilot/docker/Dockerfile.proxytproxy
@@ -8,8 +8,6 @@ ARG ENVOY_SHA=unknown
 LABEL istio-api-vcs-ref=$ISTIO_API_SHA \
     envoy-vcs-ref=$ENVOY_SHA
 
-# Environment variables indicating this proxy's version/capabilities as opaque string
-ENV ISTIO_META_ISTIO_PROXY_VERSION "1.1.3"
 # Environment variable indicating the exact proxy sha - for debugging or version-specific configs
 ENV ISTIO_META_ISTIO_PROXY_SHA $proxy_version
 # Environment variable indicating the exact build, for debugging

diff --git a/pilot/docker/Dockerfile.proxyv2 b/pilot/docker/Dockerfile.proxyv2
@@ -43,8 +43,6 @@ LABEL istio-api-vcs-ref=$ISTIO_API_SHA \
 # Install Envoy.
 COPY envoy /usr/local/bin/envoy
 
-# Environment variables indicating this proxy's version/capabilities as opaque string
-ENV ISTIO_META_ISTIO_PROXY_VERSION "1.1.3"
 # Environment variable indicating the exact proxy sha - for debugging or version-specific configs 
 ENV ISTIO_META_ISTIO_PROXY_SHA $proxy_version
 # Environment variable indicating the exact build, for debugging

diff --git a/pilot/pkg/model/context.go b/pilot/pkg/model/context.go
@@ -17,6 +17,7 @@ package model
 import (
 	"fmt"
 	"net"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -119,6 +120,54 @@ type Proxy struct {
 
 	// labels associated with the workload
 	WorkloadLabels config.LabelsCollection
+
+	// Istio version associated with the Proxy
+	IstioVersion *IstioVersion
+}
+
+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 {
+	Major int
+	Minor int
+	Patch int
+}
+
+var (
+	MaxIstioVersion = &IstioVersion{Major: 65535, Minor: 65535, Patch: 65535}
+)
+
+// 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}.
+func (pversion *IstioVersion) Compare(inv *IstioVersion) int {
+	if pversion.Major > inv.Major {
+		return 1
+	} else if pversion.Major < inv.Major {
+		return -1
+	}
+
+	// check minors
+	if inv.Minor > -1 {
+		if pversion.Minor > inv.Minor {
+			return 1
+		} else if pversion.Minor < inv.Minor {
+			return -1
+		}
+		// check patch
+		if inv.Patch > -1 {
+			if pversion.Patch > inv.Patch {
+				return 1
+			} else if pversion.Patch < inv.Patch {
+				return -1
+			}
+		}
+	}
+	return 0
 }
 
 // NodeType decides the responsibility of the proxy serves in the mesh
@@ -157,12 +206,6 @@ func (node *Proxy) ServiceNode() string {
 
 }
 
-// GetProxyVersion returns the proxy version string identifier, and whether it is present.
-func (node *Proxy) GetProxyVersion() (string, bool) {
-	version, found := node.Metadata[NodeMetadataIstioProxyVersion]
-	return version, found
-}
-
 // GetIstioVersion returns the Istio version of the proxy, and whether it is present
 func (node *Proxy) GetIstioVersion() (string, bool) {
 	version, found := node.Metadata[NodeMetadataIstioVersion]
@@ -318,9 +361,36 @@ func ParseServiceNodeWithMetadata(s string, metadata map[string]string) (*Proxy,
 
 	out.ID = parts[2]
 	out.DNSDomain = parts[3]
+	out.IstioVersion = ParseIstioVersion(metadata[NodeMetadataIstioVersion])
 	return out, nil
 }
 
+// ParseIstioVersion parses a version string and returns IstioVersion struct
+func ParseIstioVersion(ver string) *IstioVersion {
+	if strings.HasPrefix(ver, "master-") {
+		// This proxy is from a master branch build. Assume latest version
+		return MaxIstioVersion
+	}
+
+	// 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 atleast major and minor based on the regex
+	major, _ := strconv.Atoi(parts[0])
+	minor, _ := strconv.Atoi(parts[1])
+	patch := 0
+	if len(parts) > 2 {
+		patch, _ = strconv.Atoi(parts[2])
+	}
+	return &IstioVersion{Major: major, Minor: minor, Patch: patch}
+}
+
 // GetOrDefaultFromMap returns either the value found for key or the default value if the map is nil
 // or does not contain the key. Useful when retrieving node metadata fields.
 func GetOrDefaultFromMap(stringMap map[string]string, key, defaultVal string) string {
@@ -393,9 +463,6 @@ func isValidIPAddress(ip string) bool {
 
 // Pile all node metadata constants here
 const (
-	// NodeMetadataIstioProxyVersion specifies the Envoy version associated with the proxy
-	NodeMetadataIstioProxyVersion = "ISTIO_PROXY_VERSION"
-
 	// NodeMetadataIstioVersion specifies the Istio version associated with the proxy
 	NodeMetadataIstioVersion = "ISTIO_VERSION"
 

diff --git a/pilot/pkg/model/context_test.go b/pilot/pkg/model/context_test.go
@@ -25,7 +25,7 @@ import (
 )
 
 func TestServiceNode(t *testing.T) {
-	nodes := []struct {
+	cases := []struct {
 		in  *model.Proxy
 		out string
 	}{
@@ -35,10 +35,11 @@ func TestServiceNode(t *testing.T) {
 		},
 		{
 			in: &model.Proxy{
-				Type:        model.Router,
-				ID:          "random",
-				IPAddresses: []string{"10.3.3.3"},
-				DNSDomain:   "local",
+				Type:         model.Router,
+				ID:           "random",
+				IPAddresses:  []string{"10.3.3.3"},
+				DNSDomain:    "local",
+				IstioVersion: model.MaxIstioVersion,
 			},
 			out: "router~10.3.3.3~random~local",
 		},
@@ -51,12 +52,13 @@ func TestServiceNode(t *testing.T) {
 				Metadata: map[string]string{
 					"INSTANCE_IPS": "10.3.3.3,10.4.4.4,10.5.5.5,10.6.6.6",
 				},
+				IstioVersion: model.MaxIstioVersion,
 			},
 			out: "sidecar~10.3.3.3~random~local",
 		},
 	}
 
-	for _, node := range nodes {
+	for _, node := range cases {
 		out := node.in.ServiceNode()
 		if out != node.out {
 			t.Errorf("%#v.ServiceNode() => Got %s, want %s", node.in, out, node.out)
@@ -87,3 +89,100 @@ func TestGetOrDefaultFromMap(t *testing.T) {
 	assert.Equal(t, "expectedDefaultKey2Value", model.GetOrDefaultFromMap(meta, "key2", "expectedDefaultKey2Value"))
 	assert.Equal(t, "expectedDefaultFromNilMap", model.GetOrDefaultFromMap(nil, "key", "expectedDefaultFromNilMap"))
 }
+
+func TestProxyVersion_Compare(t *testing.T) {
+	type fields struct {
+		Major int
+		Minor int
+		Patch int
+	}
+	type args struct {
+		inv *model.IstioVersion
+	}
+	tests := []struct {
+		name   string
+		fields fields
+		args   args
+		want   int
+	}{
+		{
+			name:   "greater major",
+			fields: fields{Major: 2, Minor: 1, Patch: 1},
+			args:   args{&model.IstioVersion{Major: 1, Minor: 2, Patch: 1}},
+			want:   1,
+		},
+		{
+			name:   "equal at minor",
+			fields: fields{Major: 2, Minor: 1, Patch: 1},
+			args:   args{&model.IstioVersion{Major: 2, Minor: 1, Patch: -1}},
+			want:   0,
+		},
+		{
+			name:   "less at patch",
+			fields: fields{Major: 2, Minor: 1, Patch: 0},
+			args:   args{&model.IstioVersion{Major: 2, Minor: 1, Patch: 1}},
+			want:   -1,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			pversion := &model.IstioVersion{
+				Major: tt.fields.Major,
+				Minor: tt.fields.Minor,
+				Patch: tt.fields.Patch,
+			}
+			if got := pversion.Compare(tt.args.inv); got != tt.want {
+				t.Errorf("ProxyVersion.Compare() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_parseIstioVersion(t *testing.T) {
+	type args struct {
+		ver string
+	}
+	tests := []struct {
+		name string
+		args args
+		want *model.IstioVersion
+	}{
+		{
+			name: "major.minor.patch",
+			args: args{ver: "1.2.3"},
+			want: &model.IstioVersion{Major: 1, Minor: 2, Patch: 3},
+		},
+		{
+			name: "major.minor",
+			args: args{ver: "1.2"},
+			want: &model.IstioVersion{Major: 1, Minor: 2, Patch: 0},
+		},
+		{
+			name: "release-major.minor-date",
+			args: args{ver: "release-1.2-123214234"},
+			want: &model.IstioVersion{Major: 1, Minor: 2, Patch: 0},
+		},
+		{
+			name: "master-date",
+			args: args{ver: "master-123214234"},
+			want: model.MaxIstioVersion,
+		},
+		{
+			name: "junk-major.minor.patch",
+			args: args{ver: "junk-1.2.3214234"},
+			want: model.MaxIstioVersion,
+		},
+		{
+			name: "junk-garbage",
+			args: args{ver: "junk-garbage"},
+			want: model.MaxIstioVersion,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := model.ParseIstioVersion(tt.args.ver); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("parseIstioVersion() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/pilot/pkg/networking/core/v1alpha3/gateway.go b/pilot/pkg/networking/core/v1alpha3/gateway.go
@@ -371,9 +371,9 @@ func (configgen *ConfigGeneratorImpl) createGatewayHTTPFilterChainOpts(
 	// We know that this is a HTTPS server because this function is called only for ports of type HTTP/HTTPS
 	// where HTTPS server's TLS mode is not passthrough and not nil
 	enableIngressSdsAgent := false
-	// If proxy version is over 1.1, and proxy sends metadata USER_SDS, then create SDS config for
+	// If proxy sends metadata USER_SDS, then create SDS config for
 	// gateway listener.
-	if enableSds, found := node.Metadata["USER_SDS"]; found && util.IsProxyVersionGE11(node) {
+	if enableSds, found := node.Metadata["USER_SDS"]; found {
 		enableIngressSdsAgent, _ = strconv.ParseBool(enableSds)
 	}
 	return &filterChainOpts{
@@ -533,7 +533,7 @@ func (configgen *ConfigGeneratorImpl) createGatewayTCPFilterChainOpts(
 			enableIngressSdsAgent := false
 			// If proxy version is over 1.1, and proxy sends metadata USER_SDS, then create SDS config for
 			// gateway listener.
-			if enableSds, found := node.Metadata["USER_SDS"]; found && util.IsProxyVersionGE11(node) {
+			if enableSds, found := node.Metadata["USER_SDS"]; found {
 				enableIngressSdsAgent, _ = strconv.ParseBool(enableSds)
 			}
 			return []*filterChainOpts{

diff --git a/pilot/pkg/networking/core/v1alpha3/listener.go b/pilot/pkg/networking/core/v1alpha3/listener.go
@@ -1515,9 +1515,7 @@ func buildHTTPConnectionManager(node *model.Proxy, env *model.Environment, httpO
 			Name: xdsutil.FileAccessLog,
 		}
 
-		if util.IsProxyVersionGE11(node) {
-			buildAccessLog(fl, env)
-		}
+		buildAccessLog(fl, env)
 
 		if util.IsXDSMarshalingToAnyEnabled(node) {
 			acc.ConfigType = &accesslog.AccessLog_TypedConfig{TypedConfig: util.MessageToAny(fl)}

diff --git a/pilot/pkg/networking/core/v1alpha3/listener_builder_test.go b/pilot/pkg/networking/core/v1alpha3/listener_builder_test.go
@@ -42,7 +42,7 @@ func getDefaultProxy() model.Proxy {
 		DNSDomain:   "default.example.org",
 		Metadata: map[string]string{
 			model.NodeMetadataConfigNamespace: "not-default",
-			"ISTIO_PROXY_VERSION":             "1.1",
+			"ISTIO_VERSION":                   "1.3",
 		},
 		ConfigNamespace: "not-default",
 	}

diff --git a/pilot/pkg/networking/core/v1alpha3/listener_test.go b/pilot/pkg/networking/core/v1alpha3/listener_test.go
@@ -54,7 +54,7 @@ var (
 		DNSDomain:   "default.example.org",
 		Metadata: map[string]string{
 			model.NodeMetadataConfigNamespace: "not-default",
-			"ISTIO_PROXY_VERSION":             "1.1",
+			"ISTIO_VERSION":                   "1.3",
 		},
 		ConfigNamespace: "not-default",
 	}
@@ -65,7 +65,7 @@ var (
 		DNSDomain:   "default.example.org",
 		Metadata: map[string]string{
 			model.NodeMetadataConfigNamespace: "not-default",
-			"ISTIO_PROXY_VERSION":             "1.1",
+			"ISTIO_VERSION":                   "1.3",
 			model.NodeMetadataHTTP10:          "1",
 		},
 		ConfigNamespace: "not-default",