Repository: clusternet/clusternet
Pull request #523 (2022-11-07 02:31:56)

bugfix: fix the improper usage of `version` package
Signed-off-by: Bruce Ma <brucema19901024@gmail.com>

<!--  Thanks for sending a pull request!

-->

#### What type of PR is this?
/bugfix

#### What this PR does / why we need it:
fix the improper usage of `version` package, which is causing unexpected results in version comparison.

#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
NONE

#### Special notes for your reviewer:



mars1024: /assign @dixudx 
codecov-commenter: # [Codecov](https://codecov.io/gh/clusternet/clusternet/pull/523?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clusternet) Report
> Merging [#523](https://codecov.io/gh/clusternet/clusternet/pull/523?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clusternet) (662877a) into [main](https://codecov.io/gh/clusternet/clusternet/commit/d0ca5ebbb91753a2914d137b80c26e88e90465d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clusternet) (d0ca5eb) will **not change** coverage.
> The diff coverage is `n/a`.

```diff
@@           Coverage Diff           @@
##             main     #523   +/-   ##
=======================================
  Coverage   13.10%   13.10%           
=======================================
  Files          65       65           
  Lines        7079     7079           
=======================================
  Hits          928      928           
  Misses       6073     6073           
  Partials       78       78           
```


| [Impacted Files](https://codecov.io/gh/clusternet/clusternet/pull/523?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clusternet) | Coverage Δ | |
|---|---|---|
| [...hub.com/clusternet/clusternet/pkg/utils/version.go](https://codecov.io/gh/clusternet/clusternet/pull/523/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clusternet#diff-c3JjL2dpdGh1Yi5jb20vY2x1c3Rlcm5ldC9jbHVzdGVybmV0L3BrZy91dGlscy92ZXJzaW9uLmdv) | `100.00% <0.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clusternet). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=clusternet)


Patch:
diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go
index dfa26d6..e793c45 100644
--- a/pkg/agent/agent.go
+++ b/pkg/agent/agent.go
@@ -147,6 +147,12 @@ func NewAgent(registrationOpts *ClusterRegistrationOptions, controllerOpts *util
 		}
 	}
 
+	// parse saTokenAutoGenerated by server version
+	var saTokenAutoGenerated bool
+	if saTokenAutoGenerated, err = utils.SATokenAutoGenerated(ver.String()); err != nil {
+		return nil, err
+	}
+
 	agent := &Agent{
 		Identity:               identity,
 		childKubeClientSet:     childKubeClientSet,
@@ -165,7 +171,7 @@ func NewAgent(registrationOpts *ClusterRegistrationOptions, controllerOpts *util
 			registrationOpts.ClusterSyncMode,
 			childKubeConfig.Host,
 			controllerOpts.LeaderElection.ResourceNamespace,
-			utils.SATokenAutoGenerated(ver.String())),
+			saTokenAutoGenerated),
 		predictor:    p,
 		seController: mcs.NewSeController(kubeInformerFactory.Discovery().V1beta1().EndpointSlices(), mcsClientSet, mcsInformerFactory),
 	}
diff --git a/pkg/hub/approver/approver.go b/pkg/hub/approver/approver.go
index a318507..a011746 100644
--- a/pkg/hub/approver/approver.go
+++ b/pkg/hub/approver/approver.go
@@ -73,6 +73,11 @@ func NewCRRApprover(kubeclient *kubernetes.Clientset, clusternetclient *clustern
 		return nil, err
 	}
 
+	var saTokenAutoGenerated bool
+	if saTokenAutoGenerated, err = utils.SATokenAutoGenerated(ver.String()); err != nil {
+		return nil, err
+	}
+
 	crrApprover := &CRRApprover{
 		kubeclient:       kubeclient,
 		clusternetclient: clusternetclient,
@@ -81,7 +86,7 @@ func NewCRRApprover(kubeclient *kubernetes.Clientset, clusternetclient *clustern
 		nsLister:         kubeInformerFactory.Core().V1().Namespaces().Lister(),
 		saLister:         kubeInformerFactory.Core().V1().ServiceAccounts().Lister(),
 		socketConnection: socketConnection,
-		saTokenAutoGen:   utils.SATokenAutoGenerated(ver.String()),
+		saTokenAutoGen:   saTokenAutoGenerated,
 	}
 
 	newCRRController, err := clusterregistrationrequest.NewController(clusternetclient,
diff --git a/pkg/utils/version.go b/pkg/utils/version.go
index 6c55ce8..4987e60 100644
--- a/pkg/utils/version.go
+++ b/pkg/utils/version.go
@@ -16,15 +16,21 @@ limitations under the License.
 
 package utils
 
-import "k8s.io/apimachinery/pkg/version"
+import (
+	"k8s.io/apimachinery/pkg/util/version"
+)
 
-const (
-	// KubeV1240 indicates the version of Kubernetes v1.24.0
-	// This should be "v1.24.0-alpha.4", but it does not tolerate v1.24.0.
-	KubeV1240 = "v1.24.0"
+var (
+	// KubeV1240Alpha4 indicates the version of Kubernetes v1.24.0-alpha.4
+	KubeV1240Alpha4 = version.MustParseSemantic("v1.24.0-alpha.4")
 )
 
 // SATokenAutoGenerated is used to indicate whether secret-based service account token can be auto generated
-func SATokenAutoGenerated(kubeServerVersion string) bool {
-	return version.CompareKubeAwareVersionStrings(kubeServerVersion, KubeV1240) > 0
+func SATokenAutoGenerated(kubeServerVersion string) (bool, error) {
+	parsedVersion, err := version.ParseSemantic(kubeServerVersion)
+	if err != nil {
+		return false, err
+	}
+
+	return parsedVersion.LessThan(KubeV1240Alpha4), nil
 }


Test patch:
diff --git a/pkg/utils/version_test.go b/pkg/utils/version_test.go
index 224f0c1..674cfcb 100644
--- a/pkg/utils/version_test.go
+++ b/pkg/utils/version_test.go
@@ -17,6 +17,8 @@ limitations under the License.
 package utils
 
 import (
+	"fmt"
+	"reflect"
 	"testing"
 )
 
@@ -25,10 +27,16 @@ func TestSATokenAutoGenerated(t *testing.T) {
 		name              string
 		kubeServerVersion string
 		want              bool
+		err               error
 	}{
+		// bad versions
+		{
+			kubeServerVersion: "unknown",
+			err:               fmt.Errorf("could not parse \"unknown\" as version"),
+		},
 		// no auto-generation of secret-based service account token
 		{
-			kubeServerVersion: KubeV1240,
+			kubeServerVersion: KubeV1240Alpha4.String(),
 			want:              false,
 		},
 		{
@@ -40,11 +48,23 @@ func TestSATokenAutoGenerated(t *testing.T) {
 			want:              false,
 		},
 		{
-			kubeServerVersion: "v1.24.6+k3s1",
+			kubeServerVersion: "v1.24.0+k3s1",
+			want:              false,
+		},
+		{
+			kubeServerVersion: "v1.24.0-rc2+k3s1",
+			want:              false,
+		},
+		{
+			kubeServerVersion: "v1.24.0-alpha.5",
+			want:              false,
+		},
+		{
+			kubeServerVersion: "v1.24.0-beta.2",
 			want:              false,
 		},
 		{
-			kubeServerVersion: "v1.24.0-rc1+k3s1",
+			kubeServerVersion: "v1.24.0-rc.0",
 			want:              false,
 		},
 		{
@@ -53,16 +73,28 @@ func TestSATokenAutoGenerated(t *testing.T) {
 		},
 
 		// auto-generation of legacy secret-based service account token
+		{
+			kubeServerVersion: "v1.24.0-alpha.3",
+			want:              true,
+		},
+		{
+			kubeServerVersion: "v1.24.0-alpha.2+k3s1",
+			want:              true,
+		},
 		{
 			kubeServerVersion: "v1.23.1",
 			want:              true,
 		},
 		{
-			kubeServerVersion: "v1.23.6+k3s1",
+			kubeServerVersion: "v1.23.1-alpha.0",
 			want:              true,
 		},
 		{
-			kubeServerVersion: "v1.23.12-rc1+k3s1",
+			kubeServerVersion: "v1.23.5-rc.1",
+			want:              true,
+		},
+		{
+			kubeServerVersion: "v1.23.6+k3s1",
 			want:              true,
 		},
 		{
@@ -72,8 +104,12 @@ func TestSATokenAutoGenerated(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := SATokenAutoGenerated(tt.kubeServerVersion); got != tt.want {
-				t.Errorf("kubeServerVersion %s wants %v", tt.kubeServerVersion, tt.want)
+			saTokenAutoGenerated, err := SATokenAutoGenerated(tt.kubeServerVersion)
+			if !reflect.DeepEqual(err, tt.err) {
+				t.Errorf("kubeServerVersion %s, error expected %v but got %v", tt.kubeServerVersion, tt.err, err)
+			}
+			if saTokenAutoGenerated != tt.want {
+				t.Errorf("kubeServerVersion %s, saTokenAutoGenerated expected %v but got %v", tt.kubeServerVersion, tt.want, saTokenAutoGenerated)
 			}
 		})
 	}