Repository: containous/maesh (go)

Issue:
Kubernetes Minor version parsing could fail
## Bug Report

As said in the Kubernetes code the `Minor` version returned by `.Discovery().ServerVersion().Major` could potentially contain a `+`. Therefore, the parsing of the `Minor` version will fail and the topology annotations will not be added, even if the Kubernetes cluster is supporting it.

Maesh should compare the Kubernetes version by using the `semver` string or by removing the `+`.

Pull request description:
Use Kubernetes semver to add topology keys if supported
This PR:

- Adds [go-version](https://github.com/hashicorp/go-version#readme) dependency.
- Uses the Kubernetes server `semver` to check if topology keys are supported.
- Fixes a parsing error when the Kubernetes server `minor` version contains a `+`, see https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/version/base.go#L43

Fixes #678.

### How to test it

* make test

Patch:
diff --git a/go.mod b/go.mod
index 316add8..0c562a3 100644
--- a/go.mod
+++ b/go.mod
@@ -9,6 +9,7 @@ require (
 	github.com/go-check/check v0.0.0-20180628173108-788fd7840127
 	github.com/google/uuid v1.1.1
 	github.com/gorilla/mux v1.7.3
+	github.com/hashicorp/go-version v1.2.1
 	github.com/pmezard/go-difflib v1.0.0
 	github.com/servicemeshinterface/smi-sdk-go v0.3.0
 	github.com/sirupsen/logrus v1.6.0
diff --git a/go.sum b/go.sum
index b6d7d4b..5fbea19 100644
--- a/go.sum
+++ b/go.sum
@@ -401,6 +401,8 @@ github.com/hashicorp/go-uuid v1.0.1 h1:fv1ep09latC32wFoVwnqcnKJGnMSdBanPczbHAYm1
 github.com/hashicorp/go-uuid v1.0.1/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro=
 github.com/hashicorp/go-version v1.2.0 h1:3vNe/fWF5CBgRIguda1meWhsZHy3m8gCJ5wx+dIzX/E=
 github.com/hashicorp/go-version v1.2.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
+github.com/hashicorp/go-version v1.2.1 h1:zEfKbn2+PDgroKdiOzqiE8rsmLqU2uwi5PB5pBJ3TkI=
+github.com/hashicorp/go-version v1.2.1/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
 github.com/hashicorp/go.net v0.0.1/go.mod h1:hjKkEWcCURg++eb33jQU7oqQcI9XDCnUzHA0oac0k90=
 github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
 github.com/hashicorp/golang-lru v0.5.1 h1:0hERBMJE1eitiLkihrMvRVBYAkpHzc/J3QdDN+dAcgU=
diff --git a/pkg/controller/service.go b/pkg/controller/service.go
index b8028bb..f02da48 100644
--- a/pkg/controller/service.go
+++ b/pkg/controller/service.go
@@ -3,9 +3,9 @@ package controller
 import (
 	"errors"
 	"fmt"
-	"strconv"
 
 	"github.com/containous/maesh/pkg/annotations"
+	"github.com/hashicorp/go-version"
 	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
@@ -15,6 +15,8 @@ import (
 	listers "k8s.io/client-go/listers/core/v1"
 )
 
+var versionTopologyKeys = version.Must(version.NewVersion("1.17"))
+
 // PortMapper is capable of storing and retrieving a port mapping for a given service.
 type PortMapper interface {
 	Find(namespace, name string, port int32) (int32, bool)
@@ -87,7 +89,12 @@ func (s *ShadowServiceManager) CreateOrUpdate(svc *corev1.Service) (*corev1.Serv
 	}
 
 	// If the kubernetes server version is 1.17+, then use the topology key.
-	if major, minor := parseKubernetesServerVersion(s.kubeClient); major == 1 && minor >= 17 {
+	serverVersion, err := getServerVersion(s.kubeClient)
+	if err != nil {
+		s.logger.Errorf("Unable to get server version: %v", err)
+	}
+
+	if err == nil && serverVersion.GreaterThanOrEqual(versionTopologyKeys) {
 		newShadowSvc.Spec.TopologyKeys = []string{
 			"kubernetes.io/hostname",
 			"topology.kubernetes.io/zone",
@@ -106,6 +113,7 @@ func (s *ShadowServiceManager) CreateOrUpdate(svc *corev1.Service) (*corev1.Serv
 
 	shadowSvc = shadowSvc.DeepCopy()
 	shadowSvc.Spec.Ports = newShadowSvc.Spec.Ports
+	shadowSvc.Spec.TopologyKeys = newShadowSvc.Spec.TopologyKeys
 
 	return s.kubeClient.CoreV1().Services(s.namespace).Update(shadowSvc)
 }
@@ -276,23 +284,15 @@ func isPortSuitable(trafficType string, sp corev1.ServicePort) bool {
 	return false
 }
 
-func parseKubernetesServerVersion(kubeClient kubernetes.Interface) (major, minor int) {
-	kubeVersion, err := kubeClient.Discovery().ServerVersion()
-	if err != nil {
-		return 0, 0
-	}
-
-	major, err = strconv.Atoi(kubeVersion.Major)
-	if err != nil {
-		return 0, 0
-	}
-
-	minor, err = strconv.Atoi(kubeVersion.Minor)
+// getServerVersion returns the parsed version of the Kubernetes server semver.
+// see https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-base/version/base.go#L58
+func getServerVersion(kubeClient kubernetes.Interface) (*version.Version, error) {
+	serverVersion, err := kubeClient.Discovery().ServerVersion()
 	if err != nil {
-		return 0, 0
+		return nil, fmt.Errorf("unable to get server version: %w", err)
 	}
 
-	return major, minor
+	return version.NewVersion(serverVersion.GitVersion)
 }
 
 // containsPort returns true if a service port with the same port and protocol value exist in the given port list, false otherwise.


Test patch:
diff --git a/pkg/controller/service_test.go b/pkg/controller/service_test.go
index 39bcc4a..ca4302a 100644
--- a/pkg/controller/service_test.go
+++ b/pkg/controller/service_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 	"time"
 
+	goversion "github.com/hashicorp/go-version"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -13,6 +14,8 @@ import (
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/version"
+	fakediscovery "k8s.io/client-go/discovery/fake"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
 	listers "k8s.io/client-go/listers/core/v1"
@@ -53,6 +56,7 @@ func TestShadowServiceManager_CreateOrUpdate(t *testing.T) {
 		desc              string
 		defaultMode       string
 		svc               *corev1.Service
+		serverVersion     string
 		currentShadowSvc  *corev1.Service
 		expectedShadowSvc *corev1.Service
 	}{
@@ -89,6 +93,40 @@ func TestShadowServiceManager_CreateOrUpdate(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc:          "should create a shadow service without the topology keys",
+			defaultMode:   "tcp",
+			serverVersion: "v1.16.6-beta.0",
+			svc: &corev1.Service{
+				ObjectMeta: v1.ObjectMeta{
+					Name:      "foo",
+					Namespace: "bar",
+				},
+				Spec: corev1.ServiceSpec{
+					Ports: []corev1.ServicePort{
+						{
+							Protocol: corev1.ProtocolTCP,
+							Port:     8080,
+						},
+					},
+				},
+			},
+			expectedShadowSvc: &corev1.Service{
+				ObjectMeta: v1.ObjectMeta{
+					Name:      "maesh-foo-6d61657368-bar",
+					Namespace: "maesh",
+				},
+				Spec: corev1.ServiceSpec{
+					Ports: []corev1.ServicePort{
+						{
+							Protocol:   corev1.ProtocolTCP,
+							Port:       8080,
+							TargetPort: intstr.FromInt(10000),
+						},
+					},
+				},
+			},
+		},
 		{
 			desc:        "should update the existing shadow service and remove the unused port",
 			defaultMode: "tcp",
@@ -198,7 +236,12 @@ func TestShadowServiceManager_CreateOrUpdate(t *testing.T) {
 				currentShadowServices = append(currentShadowServices, test.currentShadowSvc)
 			}
 
-			client, lister := newFakeClient(currentShadowServices...)
+			serverVersionStr := "v1.17"
+			if test.serverVersion != "" {
+				serverVersionStr = test.serverVersion
+			}
+
+			client, lister := newFakeClient(serverVersionStr, currentShadowServices...)
 
 			tcpPortMapperMock := portMapperMock{
 				findFunc: func(namespace, name string, port int32) (int32, bool) {
@@ -225,7 +268,6 @@ func TestShadowServiceManager_CreateOrUpdate(t *testing.T) {
 			)
 
 			shadowSvc, err := shadowServiceManager.CreateOrUpdate(test.svc)
-
 			require.NoError(t, err)
 
 			assert.Equal(t, test.expectedShadowSvc.Name, shadowSvc.Name)
@@ -234,6 +276,13 @@ func TestShadowServiceManager_CreateOrUpdate(t *testing.T) {
 			for i, port := range test.expectedShadowSvc.Spec.Ports {
 				assert.Equal(t, port, shadowSvc.Spec.Ports[i])
 			}
+
+			serverVersion, err := goversion.NewVersion(serverVersionStr)
+			require.NoError(t, err)
+
+			if serverVersion.GreaterThanOrEqual(versionTopologyKeys) {
+				assert.True(t, len(shadowSvc.Spec.TopologyKeys) > 0)
+			}
 		})
 	}
 }
@@ -345,7 +394,7 @@ func TestShadowServiceManager_Delete(t *testing.T) {
 				currentShadowServices = append(currentShadowServices, test.currentShadowSvc)
 			}
 
-			client, lister := newFakeClient(currentShadowServices...)
+			client, lister := newFakeClient("v1.17", currentShadowServices...)
 
 			shadowServiceManager := NewShadowServiceManager(
 				log,
@@ -399,7 +448,7 @@ func TestShadowServiceManager_getShadowServiceName(t *testing.T) {
 	log.SetOutput(os.Stdout)
 	log.SetLevel(logrus.DebugLevel)
 
-	client, lister := newFakeClient()
+	client, lister := newFakeClient("v1.17")
 
 	shadowServiceManager := NewShadowServiceManager(
 		log,
@@ -443,7 +492,7 @@ func TestShadowServiceManager_getHTTPPort(t *testing.T) {
 			log.SetOutput(os.Stdout)
 			log.SetLevel(logrus.DebugLevel)
 
-			client, lister := newFakeClient()
+			client, lister := newFakeClient("v1.17")
 
 			minHTTPPort := int32(5000)
 			maxHTTPPort := int32(5002)
@@ -630,9 +679,14 @@ func TestShadowServiceManager_needsCleanup(t *testing.T) {
 	}
 }
 
-func newFakeClient(objects ...runtime.Object) (*fake.Clientset, listers.ServiceLister) {
+func newFakeClient(serverVersion string, objects ...runtime.Object) (*fake.Clientset, listers.ServiceLister) {
 	client := fake.NewSimpleClientset(objects...)
 
+	discovery, _ := client.Discovery().(*fakediscovery.FakeDiscovery)
+	discovery.FakedServerVersion = &version.Info{
+		GitVersion: serverVersion,
+	}
+
 	informerFactory := informers.NewSharedInformerFactory(client, 5*time.Minute)
 	lister := informerFactory.Core().V1().Services().Lister()