Merge pull request #3 from g-gaston/metadata-cert-manager-fork

🌱 Check for build metadata in cert-manager version

Author: mrajashree

cmd/clusterctl/client/cluster/cert_manager.go

@@ -21,11 +21,11 @@ import (
 	_ "embed"
 	"time"
 
+	"github.com/blang/semver"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/util/version"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -34,6 +34,7 @@ import (
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/util"
 	logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log"
 	utilresource "sigs.k8s.io/cluster-api/util/resource"
+	"sigs.k8s.io/cluster-api/util/version"
 	utilyaml "sigs.k8s.io/cluster-api/util/yaml"
 )
 
@@ -48,10 +49,8 @@ const (
 	certManagerVersionAnnotation = "certmanager.clusterctl.cluster.x-k8s.io/version"
 )
 
-var (
-	//go:embed assets/cert-manager-test-resources.yaml
-	certManagerTestManifest []byte
-)
+//go:embed assets/cert-manager-test-resources.yaml
+var certManagerTestManifest []byte
 
 // CertManagerUpgradePlan defines the upgrade plan if cert-manager needs to be
 // upgraded to a different version.
@@ -300,6 +299,12 @@ func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (st
 		return "", "", false, err
 	}
 
+	desiredVersion := config.Version()
+	desiredSemVersion, err := semver.ParseTolerant(desiredVersion)
+	if err != nil {
+		return "", "", false, errors.Wrapf(err, "failed to parse config version [%s] for cert-manager component", desiredVersion)
+	}
+
 	needUpgrade := false
 	currentVersion := ""
 	for i := range objs {
@@ -322,16 +327,12 @@ func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (st
 			}
 		}
 
-		objSemVersion, err := version.ParseSemantic(objVersion)
+		objSemVersion, err := semver.ParseTolerant(objVersion)
 		if err != nil {
 			return "", "", false, errors.Wrapf(err, "failed to parse version for cert-manager component %s/%s", obj.GetKind(), obj.GetName())
 		}
 
-		c, err := objSemVersion.Compare(config.Version())
-		if err != nil {
-			return "", "", false, errors.Wrapf(err, "failed to compare target version for cert-manager component %s/%s", obj.GetKind(), obj.GetName())
-		}
-
+		c := version.Compare(objSemVersion, desiredSemVersion, version.WithBuildTags())
 		switch {
 		case c < 0:
 			// if version < current, then upgrade
@@ -346,7 +347,7 @@ func (cm *certManagerClient) shouldUpgrade(objs []unstructured.Unstructured) (st
 			break
 		}
 	}
-	return currentVersion, config.Version(), needUpgrade, nil
+	return currentVersion, desiredVersion, needUpgrade, nil
 }
 
 func (cm *certManagerClient) getWaitTimeout() time.Duration {

cmd/clusterctl/client/cluster/cert_manager_test.go

@@ -207,6 +207,7 @@ func Test_shouldUpgrade(t *testing.T) {
 	}
 	tests := []struct {
 		name            string
+		configVersion   string
 		args            args
 		wantFromVersion string
 		wantToVersion   string
@@ -247,6 +248,111 @@ func Test_shouldUpgrade(t *testing.T) {
 			want:            false,
 			wantErr:         false,
 		},
+		{
+			name:          "Version is equal but current version has no build metadata, should upgrade",
+			configVersion: "v1.5.3+h4fd4",
+			args: args{
+				objs: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"metadata": map[string]interface{}{
+								"annotations": map[string]interface{}{
+									clusterctlv1.CertManagerVersionAnnotation: "v1.5.3",
+								},
+							},
+						},
+					},
+				},
+			},
+			wantFromVersion: "v1.5.3",
+			wantToVersion:   "v1.5.3+h4fd4",
+			want:            true,
+			wantErr:         false,
+		},
+		{
+			name:          "Version is equal but different build metadata with hash, should upgrade",
+			configVersion: "v1.5.3+h4fd4",
+			args: args{
+				objs: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"metadata": map[string]interface{}{
+								"annotations": map[string]interface{}{
+									clusterctlv1.CertManagerVersionAnnotation: "v1.5.3+h4fd5",
+								},
+							},
+						},
+					},
+				},
+			},
+			wantFromVersion: "v1.5.3+h4fd5",
+			wantToVersion:   "v1.5.3+h4fd4",
+			want:            true,
+			wantErr:         false,
+		},
+		{
+			name:          "Version is equal and same build metadata with hash, should not upgrade",
+			configVersion: "v1.5.3+h4fd5",
+			args: args{
+				objs: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"metadata": map[string]interface{}{
+								"annotations": map[string]interface{}{
+									clusterctlv1.CertManagerVersionAnnotation: "v1.5.3+h4fd5",
+								},
+							},
+						},
+					},
+				},
+			},
+			wantFromVersion: "v1.5.3+h4fd5",
+			wantToVersion:   "v1.5.3+h4fd5",
+			want:            false,
+			wantErr:         false,
+		},
+		{
+			name:          "Version is equal but older numbered build metadata, should not upgrade",
+			configVersion: "v1.5.3+build.1",
+			args: args{
+				objs: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"metadata": map[string]interface{}{
+								"annotations": map[string]interface{}{
+									clusterctlv1.CertManagerVersionAnnotation: "v1.5.3+build.2",
+								},
+							},
+						},
+					},
+				},
+			},
+			wantFromVersion: "v1.5.3+build.2",
+			wantToVersion:   "v1.5.3+build.1",
+			want:            false,
+			wantErr:         false,
+		},
+		{
+			name:          "Version is equal but newer numbered build metadata, should upgrade",
+			configVersion: "v1.5.3+build.3",
+			args: args{
+				objs: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"metadata": map[string]interface{}{
+								"annotations": map[string]interface{}{
+									clusterctlv1.CertManagerVersionAnnotation: "v1.5.3+build.2",
+								},
+							},
+						},
+					},
+				},
+			},
+			wantFromVersion: "v1.5.3+build.2",
+			wantToVersion:   "v1.5.3+build.3",
+			want:            true,
+			wantErr:         false,
+		},
 		{
 			name: "Version is older, should upgrade",
 			args: args{
@@ -313,7 +419,7 @@ func Test_shouldUpgrade(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 			proxy := test.NewFakeProxy()
-			fakeConfigClient := newFakeConfig()
+			fakeConfigClient := newFakeConfig().WithCertManager("", tt.configVersion, "")
 			pollImmediateWaiter := func(interval, timeout time.Duration, condition wait.ConditionFunc) error {
 				return nil
 			}

util/version/version.go

@@ -162,14 +162,13 @@ func (v buildIdentifier) compare(o buildIdentifier) int {
 			return 1
 		}
 	} else { // both are strings
-		switch {
-		case v.IdentifierStr < o.IdentifierStr:
-			return -1
-		case v.IdentifierStr == o.IdentifierStr:
+		// In order to support random build indentifiers, like commit hashes,
+		// always return -1 when the strings are different to always signal
+		// there is version change
+		if v.IdentifierStr == o.IdentifierStr {
 			return 0
-		default:
-			return 1
 		}
+		return -1
 	}
 }
 

util/version/version_test.go

@@ -156,7 +156,7 @@ func TestCompareWithBuildIdentifiers(t *testing.T) {
 			expected: -1,
 		},
 		{
-			name: "compare with pre release versions and  build identifiers",
+			name: "compare with pre release versions and build identifiers",
 			a: func() semver.Version {
 				v, _ := semver.ParseTolerant("v1.20.1-alpha.1+xyz.1")
 				return v
@@ -240,7 +240,7 @@ func TestCompareWithBuildIdentifiers(t *testing.T) {
 			expected: 1,
 		},
 		{
-			name: "compare with build identifiers - smaller non numeric",
+			name: "compare with build identifiers - different non numeric",
 			a: func() semver.Version {
 				v, _ := semver.ParseTolerant("v1.20.1+xyz.a")
 				return v
@@ -251,6 +251,18 @@ func TestCompareWithBuildIdentifiers(t *testing.T) {
 			}(),
 			expected: -1,
 		},
+		{
+			name: "compare with build identifiers - equal non numeric",
+			a: func() semver.Version {
+				v, _ := semver.ParseTolerant("v1.20.1+xyz.a")
+				return v
+			}(),
+			b: func() semver.Version {
+				v, _ := semver.ParseTolerant("v1.20.1+xyz.a")
+				return v
+			}(),
+			expected: 0,
+		},
 		{
 			name: "compare with build identifiers - smaller - a is numeric b is not",
 			a: func() semver.Version {