diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index bc2f3a4088a8..f94a24925fcb 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -435,7 +436,7 @@ func (webhook *Cluster) validateTopologyVersion(ctx context.Context, fldPath *fi } // minor version cannot be increased if MachineDeployments are upgrading or not yet on the current version - if err := validateTopologyMachineDeploymentVersions(ctx, fldPath, webhook.Client, oldCluster); err != nil { + if err := validateTopologyMachineDeploymentVersions(ctx, fldPath, webhook.Client, oldCluster, oldVersion); err != nil { allErrs = append(allErrs, field.Invalid( fldPath, fldValue, @@ -444,7 +445,7 @@ func (webhook *Cluster) validateTopologyVersion(ctx context.Context, fldPath *fi } // minor version cannot be increased if MachinePools are upgrading or not yet on the current version - if err := validateTopologyMachinePoolVersions(ctx, fldPath, webhook.Client, webhook.Tracker, oldCluster); err != nil { + if err := validateTopologyMachinePoolVersions(ctx, fldPath, webhook.Client, webhook.Tracker, oldCluster, oldVersion); err != nil { allErrs = append(allErrs, field.Invalid( fldPath, fldValue, @@ -499,7 +500,7 @@ func validateTopologyControlPlaneVersion(ctx context.Context, fldPath *field.Pat return nil } -func validateTopologyMachineDeploymentVersions(ctx context.Context, fldPath *field.Path, ctrlClient client.Reader, oldCluster *clusterv1.Cluster) error { +func validateTopologyMachineDeploymentVersions(ctx context.Context, fldPath *field.Path, ctrlClient client.Reader, oldCluster *clusterv1.Cluster, oldVersion semver.Version) error { // FROM: desired_state.go getCurrentMachineDeploymentState // List all the machine deployments in the current cluster and in a managed topology. @@ -524,6 +525,17 @@ func validateTopologyMachineDeploymentVersions(ctx context.Context, fldPath *fie for i := range mds.Items { md := &mds.Items[i] + mdVersion, err := semver.ParseTolerant(*md.Spec.Template.Spec.Version) + if err != nil { + // NOTE: this should never happen. Nevertheless, handling this for extra caution. + return field.InternalError(fldPath, errors.Wrapf(err, "failed to parse MachineDeployment's %q version %q", klog.KObj(md), *md.Spec.Template.Spec.Version)) + } + + if mdVersion.NE(oldVersion) { + mdUpgradingNames = append(mdUpgradingNames, md.Name) + continue + } + upgrading, err := check.IsMachineDeploymentUpgrading(ctx, ctrlClient, md) if err != nil { return errors.Wrap(err, "failed to list upgrading MachinePools") @@ -540,7 +552,7 @@ func validateTopologyMachineDeploymentVersions(ctx context.Context, fldPath *fie return nil } -func validateTopologyMachinePoolVersions(ctx context.Context, fldPath *field.Path, ctrlClient client.Reader, tracker ClusterCacheTrackerReader, oldCluster *clusterv1.Cluster) error { +func validateTopologyMachinePoolVersions(ctx context.Context, fldPath *field.Path, ctrlClient client.Reader, tracker ClusterCacheTrackerReader, oldCluster *clusterv1.Cluster, oldVersion semver.Version) error { // FROM: desired_state.go getCurrentMachinePoolState // List all the machine pools in the current cluster and in a managed topology. @@ -571,6 +583,17 @@ func validateTopologyMachinePoolVersions(ctx context.Context, fldPath *field.Pat for i := range mps.Items { mp := &mps.Items[i] + mpVersion, err := semver.ParseTolerant(*mp.Spec.Template.Spec.Version) + if err != nil { + // NOTE: this should never happen. Nevertheless, handling this for extra caution. + return field.InternalError(fldPath, errors.Wrapf(err, "failed to parse MachinePool's %q version %q", klog.KObj(mp), *mp.Spec.Template.Spec.Version)) + } + + if mpVersion.NE(oldVersion) { + mpUpgradingNames = append(mpUpgradingNames, mp.Name) + continue + } + upgrading, err := check.IsMachinePoolUpgrading(ctx, wlClient, mp) if err != nil { return errors.Wrap(err, "failed to list upgrading MachinePools") diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 2fcbaebdef9d..c15d14ec350a 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -1255,157 +1255,10 @@ func TestClusterTopologyValidation(t *testing.T) { Build(), }, { - name: "should return error when topology does not have class", - expectErr: true, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(&clusterv1.Topology{}). - Build(), - }, - { - name: "should return error when topology does not have valid version", - expectErr: true, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("invalid").Build()). - Build(), - }, - { - name: "should return error when downgrading topology version - major", - expectErr: true, - old: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v2.2.3"). - Build()). - Build(), - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.3"). - Build()). - Build(), - }, - { - name: "should return error when downgrading topology version - minor", - expectErr: true, - old: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.3"). - Build()). - Build(), - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.1.3"). - Build()). - Build(), - }, - { - name: "should return error when downgrading topology version - patch", - expectErr: true, - old: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.3"). - Build()). - Build(), - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.2"). - Build()). - Build(), - }, - { - name: "should return error when downgrading topology version - pre-release", - expectErr: true, - old: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.3-xyz.2"). - Build()). - Build(), - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.3-xyz.1"). - Build()). - Build(), - }, - { - name: "should return error when downgrading topology version - build tag", - expectErr: true, - old: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.3+xyz.2"). - Build()). - Build(), - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.3+xyz.1"). - Build()). - Build(), - }, - { - name: "should return error when upgrading +2 minor version", - expectErr: true, - old: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.2.3"). - Build()). - Build(), - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.4.0"). - Build()). - Build(), - }, - { - name: "should return error when duplicated MachineDeployments names exists in a Topology", - expectErr: true, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.1"). - WithMachineDeployment( - builder.MachineDeploymentTopology("workers1"). - WithClass("aa"). - Build()). - WithMachineDeployment( - builder.MachineDeploymentTopology("workers1"). - WithClass("bb"). - Build()). - Build()). - Build(), - }, - { - name: "should pass when MachineDeployments names in a Topology are unique", - expectErr: false, - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.1"). - WithMachineDeployment( - builder.MachineDeploymentTopology("workers1"). - WithClass("aa"). - Build()). - WithMachineDeployment( - builder.MachineDeploymentTopology("workers2"). - WithClass("bb"). - Build()). - Build()). - Build(), - }, - { - name: "should update", + name: "should update if cluster is fully upgraded and up to date", expectErr: false, old: builder.Cluster("fooboo", "cluster1"). + WithControlPlane(builder.ControlPlane("fooboo", "cluster1-cp").Build()). WithTopology(builder.ClusterTopology(). WithClass("foo"). WithVersion("v1.19.1"). @@ -1413,82 +1266,42 @@ func TestClusterTopologyValidation(t *testing.T) { builder.MachineDeploymentTopology("workers1"). WithClass("aa"). Build()). - WithMachineDeployment( - builder.MachineDeploymentTopology("workers2"). - WithClass("bb"). - Build()). - Build()). - Build(), - in: builder.Cluster("fooboo", "cluster1"). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.2"). - WithMachineDeployment( - builder.MachineDeploymentTopology("workers1"). + WithMachinePool( + builder.MachinePoolTopology("pool1"). WithClass("aa"). Build()). - WithMachineDeployment( - builder.MachineDeploymentTopology("workers2"). - WithClass("bb"). - Build()). Build()). Build(), - }, - { - name: "should return error when upgrade concurrency annotation value is < 1", - expectErr: true, in: builder.Cluster("fooboo", "cluster1"). - WithAnnotations(map[string]string{ - clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation: "-1", - }). WithTopology(builder.ClusterTopology(). WithClass("foo"). - WithVersion("v1.19.2"). + WithVersion("v1.20.2"). Build()). Build(), + additionalObjects: []client.Object{ + builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1"). + WithStatusFields(map[string]interface{}{"status.version": "v1.19.1"}). + Build(), + builder.MachineDeployment("fooboo", "cluster1-workers1").WithLabels(map[string]string{ + clusterv1.ClusterNameLabel: "cluster1", + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterTopologyMachineDeploymentNameLabel: "workers1", + }).WithVersion("v1.19.1").Build(), + builder.MachinePool("fooboo", "cluster1-pool1").WithLabels(map[string]string{ + clusterv1.ClusterNameLabel: "cluster1", + clusterv1.ClusterTopologyOwnedLabel: "", + clusterv1.ClusterTopologyMachinePoolNameLabel: "pool1", + }).WithVersion("v1.19.1").Build(), + }, }, { - name: "should return error when upgrade concurrency annotation value is not numeric", + name: "should block update if cluster kcp is not yet provisioned", expectErr: true, - in: builder.Cluster("fooboo", "cluster1"). - WithAnnotations(map[string]string{ - clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation: "abc", - }). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.2"). - Build()). - Build(), - }, - { - name: "should pass upgrade concurrency annotation value is >= 1", - expectErr: false, - in: builder.Cluster("fooboo", "cluster1"). - WithAnnotations(map[string]string{ - clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation: "2", - }). - WithTopology(builder.ClusterTopology(). - WithClass("foo"). - WithVersion("v1.19.2"). - Build()). - Build(), - }, - { - name: "should update if cluster is fully upgraded and up to date", - expectErr: false, old: builder.Cluster("fooboo", "cluster1"). WithControlPlane(builder.ControlPlane("fooboo", "cluster1-cp").Build()). WithTopology(builder.ClusterTopology(). WithClass("foo"). WithVersion("v1.19.1"). - WithMachineDeployment( - builder.MachineDeploymentTopology("workers1"). - WithClass("aa"). - Build()). - WithMachinePool( - builder.MachinePoolTopology("pool1"). - WithClass("aa"). - Build()). Build()). Build(), in: builder.Cluster("fooboo", "cluster1"). @@ -1498,17 +1311,7 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), additionalObjects: []client.Object{ - builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1").Build(), - builder.MachineDeployment("fooboo", "cluster1-workers1").WithLabels(map[string]string{ - clusterv1.ClusterNameLabel: "cluster1", - clusterv1.ClusterTopologyOwnedLabel: "", - clusterv1.ClusterTopologyMachineDeploymentNameLabel: "workers1", - }).WithVersion("v1.19.1").Build(), - builder.MachinePool("fooboo", "cluster1-pool1").WithLabels(map[string]string{ - clusterv1.ClusterNameLabel: "cluster1", - clusterv1.ClusterTopologyOwnedLabel: "", - clusterv1.ClusterTopologyMachinePoolNameLabel: "pool1", - }).WithVersion("v1.19.1").Build(), + builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.18.1").Build(), }, }, { @@ -1528,7 +1331,9 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), additionalObjects: []client.Object{ - builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.18.1").Build(), + builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.18.1"). + WithStatusFields(map[string]interface{}{"status.version": "v1.18.1"}). + Build(), }, }, { @@ -1552,7 +1357,9 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), additionalObjects: []client.Object{ - builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1").Build(), + builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1"). + WithStatusFields(map[string]interface{}{"status.version": "v1.19.1"}). + Build(), builder.MachineDeployment("fooboo", "cluster1-workers1").WithLabels(map[string]string{ clusterv1.ClusterNameLabel: "cluster1", clusterv1.ClusterTopologyOwnedLabel: "", @@ -1581,7 +1388,9 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), additionalObjects: []client.Object{ - builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1").Build(), + builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1"). + WithStatusFields(map[string]interface{}{"status.version": "v1.18.1"}). + Build(), builder.MachineDeployment("fooboo", "cluster1-workers1").WithLabels(map[string]string{ clusterv1.ClusterNameLabel: "cluster1", clusterv1.ClusterTopologyOwnedLabel: "", @@ -1616,7 +1425,9 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), additionalObjects: []client.Object{ - builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1").Build(), + builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1"). + WithStatusFields(map[string]interface{}{"status.version": "v1.18.1"}). + Build(), builder.MachinePool("fooboo", "cluster1-pool1").WithLabels(map[string]string{ clusterv1.ClusterNameLabel: "cluster1", clusterv1.ClusterTopologyOwnedLabel: "", @@ -1645,7 +1456,9 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), additionalObjects: []client.Object{ - builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1").Build(), + builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1"). + WithStatusFields(map[string]interface{}{"status.version": "v1.19.1"}). + Build(), builder.MachinePool("fooboo", "cluster1-pool1").WithLabels(map[string]string{ clusterv1.ClusterNameLabel: "cluster1", clusterv1.ClusterTopologyOwnedLabel: "",