controlplane: Normalise image name for kube-proxy

Signed-off-by: Naadir Jeewa <[email protected]>

controlplane/kubeadm/internal/workload_cluster.go

@@ -589,7 +589,7 @@ func (w *Workload) UpdateKubeProxyImageInfo(ctx context.Context, kcp *controlpla
 	if len(ds.Spec.Template.Spec.Containers) == 0 {
 		return nil
 	}
-	newImageName, err := util.ModifyImageTag(ds.Spec.Template.Spec.Containers[0].Image, kcp.Spec.Version)
+	newImageName, err := util.ModifyImageTag(ds.Spec.Template.Spec.Containers[0].Image, util.NormalizedKubernetesBuildVersion(kcp.Spec.Version))
 	if err != nil {
 		return err
 	}

controlplane/kubeadm/internal/workload_cluster_test.go

@@ -26,9 +26,12 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha3"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 func TestCluster_ReconcileKubeletRBACBinding_NoError(t *testing.T) {
@@ -108,8 +111,40 @@ func TestCluster_ReconcileKubeletRBACBinding_Error(t *testing.T) {
 	}
 }
 
+func newKubeProxyDS() appsv1.DaemonSet {
+	return appsv1.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      kubeProxyDaemonSetName,
+			Namespace: metav1.NamespaceSystem,
+		},
+		Spec: appsv1.DaemonSetSpec{
+			Template: corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{{Image: "k8s.gcr.io/kube-proxy:v1.16.2"}},
+				},
+			},
+		},
+	}
+}
+
+func newKubeProxyDSWithImage(image string) appsv1.DaemonSet {
+	ds := newKubeProxyDS()
+	ds.Spec.Template.Spec.Containers[0].Image = image
+	return ds
+}
+
 func TestUpdateKubeProxyImageInfo(t *testing.T) {
+
+	scheme := runtime.NewScheme()
+	if err := appsv1.AddToScheme(scheme); err != nil {
+		t.Fatalf("unable to setup scheme: %s", err)
+	}
+
 	ds := &appsv1.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      kubeProxyDaemonSetName,
+			Namespace: metav1.NamespaceSystem,
+		},
 		Spec: appsv1.DaemonSetSpec{
 			Template: corev1.PodTemplateSpec{
 				Spec: corev1.PodSpec{
@@ -126,57 +161,92 @@ func TestUpdateKubeProxyImageInfo(t *testing.T) {
 	dsImageEmpty.Spec.Template.Spec.Containers[0].Image = ""
 
 	tests := []struct {
-		name      string
-		ds        *appsv1.DaemonSet
-		expectErr bool
-		clientGet map[string]interface{}
-		patchErr  error
+		name        string
+		ds          appsv1.DaemonSet
+		expectErr   bool
+		expectImage string
+		clientGet   map[string]interface{}
+		patchErr    error
+		KCP         *v1alpha3.KubeadmControlPlane
 	}{
 		{
-			name:      "succeeds if patch correctly",
-			ds:        ds,
-			expectErr: false,
-			clientGet: map[string]interface{}{
-				"kube-system/" + kubeProxyDaemonSetName: ds,
-			},
+			name:        "succeeds if patch correctly",
+			ds:          newKubeProxyDS(),
+			expectErr:   false,
+			expectImage: "k8s.gcr.io/kube-proxy:v1.16.3",
+			KCP:         &v1alpha3.KubeadmControlPlane{Spec: v1alpha3.KubeadmControlPlaneSpec{Version: "v1.16.3"}},
 		},
 		{
-			name:      "returns error if image in kube-proxy ds was in digest format",
-			ds:        dsImageInDigestFormat,
-			expectErr: true,
-			clientGet: map[string]interface{}{
-				"kube-system/" + kubeProxyDaemonSetName: dsImageInDigestFormat,
-			},
+			name:        "returns error if image in kube-proxy ds was in digest format",
+			ds:          newKubeProxyDSWithImage("k8s.gcr.io/kube-proxy@sha256:47bfd"),
+			expectErr:   true,
+			expectImage: "k8s.gcr.io/kube-proxy@sha256:47bfd",
+			KCP:         &v1alpha3.KubeadmControlPlane{Spec: v1alpha3.KubeadmControlPlaneSpec{Version: "v1.16.3"}},
+		},
+		{
+			name:        "expects v-prefixed format of tag",
+			ds:          newKubeProxyDS(),
+			expectErr:   false,
+			expectImage: "k8s.gcr.io/kube-proxy:v1.16.3",
+			KCP:         &v1alpha3.KubeadmControlPlane{Spec: v1alpha3.KubeadmControlPlaneSpec{Version: "1.16.3"}},
+		},
+		{
+			name:        "expects OCI compatible format of tag",
+			ds:          newKubeProxyDS(),
+			expectErr:   false,
+			expectImage: "k8s.gcr.io/kube-proxy:v1.16.3_build1",
+			KCP:         &v1alpha3.KubeadmControlPlane{Spec: v1alpha3.KubeadmControlPlaneSpec{Version: "v1.16.3+build1"}},
 		},
 		{
 			name:      "returns error if image in kube-proxy ds was in wrong format",
-			ds:        ds,
+			ds:        newKubeProxyDSWithImage(""),
 			expectErr: true,
-			clientGet: map[string]interface{}{
-				"kube-system/" + kubeProxyDaemonSetName: dsImageEmpty,
-			},
+			KCP:       &v1alpha3.KubeadmControlPlane{Spec: v1alpha3.KubeadmControlPlaneSpec{Version: "v1.16.3"}},
 		},
 	}
 
 	ctx := context.Background()
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-
-			fakeClient := &fakeClient{
-				get: tt.clientGet,
+			objects := []runtime.Object{
+				&tt.ds,
 			}
-			c := &Workload{
+			fakeClient := fake.NewFakeClientWithScheme(scheme, objects...)
+			w := &Workload{
 				Client: fakeClient,
 			}
-			err := c.UpdateKubeProxyImageInfo(ctx, &v1alpha3.KubeadmControlPlane{Spec: v1alpha3.KubeadmControlPlaneSpec{Version: "v1.16.3"}})
+			err := w.UpdateKubeProxyImageInfo(ctx, tt.KCP)
 			if err != nil && !tt.expectErr {
 				t.Fatalf("expected no error, got %s", err)
 			}
 			if err == nil && tt.expectErr {
 				t.Fatal("expected error but got none")
 			}
-
+			proxyImage, err := w.getProxyImageInfo(ctx)
+			if err != nil {
+				t.Fatalf("expected no error, got %s", err)
+			}
+			if proxyImage != tt.expectImage {
+				t.Fatalf("expected proxy image %s, got %s", tt.expectImage, proxyImage)
+			}
 		})
 	}
 }
+
+func (w *Workload) getProxyImageInfo(ctx context.Context) (string, error) {
+	ds := &appsv1.DaemonSet{}
+
+	if err := w.Client.Get(ctx, ctrlclient.ObjectKey{Name: kubeProxyDaemonSetName, Namespace: metav1.NamespaceSystem}, ds); err != nil {
+		if apierrors.IsNotFound(err) {
+			// if kube-proxy is missing, return without errors
+			return "", errors.New("no image found")
+		}
+		return "", errors.New("failed to determine if daemonset already exists")
+	}
+
+	if len(ds.Spec.Template.Spec.Containers) == 0 {
+		return "", errors.New("failed to find container")
+	}
+	return ds.Spec.Template.Spec.Containers[0].Image, nil
+}

util/util.go

@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"math/rand"
+	"regexp"
 	"strings"
 	"time"
 
@@ -51,6 +52,8 @@ var (
 	rnd                          = rand.New(rand.NewSource(time.Now().UnixNano()))
 	ErrNoCluster                 = fmt.Errorf("no %q label present", clusterv1.ClusterLabelName)
 	ErrUnstructuredFieldNotFound = fmt.Errorf("field not found")
+	// taken from k8s.io/cmd/kubeadm/app/util
+	kubeReleaseRegex = regexp.MustCompile(`^v?(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$`)
 )
 
 // RandomString returns a random alphanumeric string.
@@ -64,6 +67,8 @@ func RandomString(n int) string {
 
 // ModifyImageTag takes an imageName (e.g., registry/repo:tag), and returns an image name with updated tag
 func ModifyImageTag(imageName, tagName string) (string, error) {
+	normalisedTagName := SemverToOCIImageTag(tagName)
+
 	namedRef, err := reference.ParseNormalizedNamed(imageName)
 	if err != nil {
 		return "", errors.Wrap(err, "failed to parse image name")
@@ -75,7 +80,7 @@ func ModifyImageTag(imageName, tagName string) (string, error) {
 	}
 
 	// update the image tag with tagName
-	namedTagged, err := reference.WithTag(namedRef, tagName)
+	namedTagged, err := reference.WithTag(namedRef, normalisedTagName)
 	if err != nil {
 		return "", errors.Wrap(err, "failed to update image tag")
 	}
@@ -99,6 +104,31 @@ func GetMachinesForCluster(ctx context.Context, c client.Client, cluster *cluste
 	return &machines, nil
 }
 
+// Internal helper: returns normalized build version (with "v" prefix if needed)
+// If input doesn't match known version pattern, returns empty string.
+// Taken from k8s.io/cmd/kubeadm/app/util
+func NormalizedKubernetesBuildVersion(version string) string {
+	if kubeReleaseRegex.MatchString(version) {
+		if strings.HasPrefix(version, "v") {
+			return version
+		}
+		return "v" + version
+	}
+	return ""
+}
+
+// SemVerToOCIImageTag is helper function that replaces all
+// non-allowed symbols in tag strings with underscores.
+// Image tag can only contain lowercase and uppercase letters, digits,
+// underscores, periods and dashes.
+// Current usage is for CI images where all of symbols except '+' are valid,
+// but function is for generic usage where input can't be always pre-validated.
+// Taken from k8s.io/cmd/kubeadm/app/util
+func SemverToOCIImageTag(version string) string {
+	allowed := regexp.MustCompile(`[^-a-zA-Z0-9_\.]`)
+	return allowed.ReplaceAllString(version, "_")
+}
+
 // GetControlPlaneMachines returns a slice containing control plane machines.
 func GetControlPlaneMachines(machines []*clusterv1.Machine) (res []*clusterv1.Machine) {
 	for _, machine := range machines {

util/util_test.go

@@ -444,6 +444,17 @@ func TestGetMachinesForCluster(t *testing.T) {
 	}
 }
 
+func TestModifyImageTag(t *testing.T) {
+	g := NewGomegaWithT(t)
+	t.Run("should ensure image is a docker compatible tag", func(t *testing.T) {
+		testTag := "v1.17.4+build1"
+		image := "example.com/image:1.17.3"
+		res, err := ModifyImageTag(image, testTag)
+		g.Expect(err).NotTo(HaveOccurred())
+		g.Expect(res).To(Equal("example.com/image:v1.17.4_build1"))
+	})
+}
+
 func TestEnsureOwnerRef(t *testing.T) {
 	g := NewGomegaWithT(t)