From bb840d902f25942038f37f6ce9d02980467e9af5 Mon Sep 17 00:00:00 2001 From: Naadir Jeewa Date: Mon, 16 Mar 2020 14:50:03 +0000 Subject: [PATCH] controlplane: Normalise image name for kube-proxy Signed-off-by: Naadir Jeewa --- .../kubeadm/internal/workload_cluster.go | 2 +- .../kubeadm/internal/workload_cluster_test.go | 124 ++++++++++++++---- util/util.go | 49 ++++++- util/util_test.go | 11 ++ 4 files changed, 157 insertions(+), 29 deletions(-) diff --git a/controlplane/kubeadm/internal/workload_cluster.go b/controlplane/kubeadm/internal/workload_cluster.go index b4638b93fc4f..13e9e40c7ba6 100644 --- a/controlplane/kubeadm/internal/workload_cluster.go +++ b/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 } diff --git a/controlplane/kubeadm/internal/workload_cluster_test.go b/controlplane/kubeadm/internal/workload_cluster_test.go index a670b47f4c46..b9c89f93dbd6 100644 --- a/controlplane/kubeadm/internal/workload_cluster_test.go +++ b/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,35 +161,47 @@ 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"}}, }, } @@ -162,21 +209,44 @@ func TestUpdateKubeProxyImageInfo(t *testing.T) { 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 +} diff --git a/util/util.go b/util/util.go index bff3255505a8..4d40ecb34b1d 100644 --- a/util/util.go +++ b/util/util.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "math/rand" + "regexp" "strings" "time" @@ -51,6 +52,9 @@ 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_\.+]*)?$`) + ociTagAllowedChars = regexp.MustCompile(`[^-a-zA-Z0-9_\.]`) ) // RandomString returns a random alphanumeric string. @@ -64,6 +68,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 +81,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") } @@ -83,6 +89,23 @@ func ModifyImageTag(imageName, tagName string) (string, error) { return reference.FamiliarString(reference.TagNameOnly(namedTagged)), nil } +func KubernetesVersionIsValid(version string) bool { + return kubeReleaseRegex.MatchString(version) +} + +func ImageTagIsValid(tagName string) bool { + return !ociTagAllowedChars.MatchString(tagName) +} + +func ImageNameIsValid(imageName string) (bool, error) { + _, err := reference.ParseNormalizedNamed(imageName) + if err != nil { + return false, errors.Wrap(err, "failed to parse image name") + + } + return true, nil +} + // GetMachinesForCluster returns a list of machines associated with the cluster. func GetMachinesForCluster(ctx context.Context, c client.Client, cluster *clusterv1.Cluster) (*clusterv1.MachineList, error) { var machines clusterv1.MachineList @@ -99,6 +122,30 @@ 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 { + return ociTagAllowedChars.ReplaceAllString(version, "_") +} + // GetControlPlaneMachines returns a slice containing control plane machines. func GetControlPlaneMachines(machines []*clusterv1.Machine) (res []*clusterv1.Machine) { for _, machine := range machines { diff --git a/util/util_test.go b/util/util_test.go index 33d0878af5fc..d2b0271b0a6b 100644 --- a/util/util_test.go +++ b/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)