diff --git a/controllers/topology/desired_state.go b/controllers/topology/desired_state.go index 98c2c3b9466b..a43981e190b1 100644 --- a/controllers/topology/desired_state.go +++ b/controllers/topology/desired_state.go @@ -19,6 +19,7 @@ package topology import ( "context" "fmt" + "strings" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -26,6 +27,7 @@ import ( "k8s.io/apiserver/pkg/storage/names" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" "sigs.k8s.io/cluster-api/controllers/external" + "sigs.k8s.io/cluster-api/util/version" ) // computeDesiredState computes the desired state of the cluster topology. @@ -136,15 +138,86 @@ func computeControlPlane(class *clusterTopologyClass, current *clusterTopologySt } } + // Computes the desired Kubernetes version for the control plane. + controlPlaneVersion, err := computeControlPlaneVersion(current) + if err != nil { + return nil, errors.Wrap(err, "failed to compute spec.version for the ControlPlane object") + } + // Sets the desired Kubernetes version for the control plane. - // TODO: improve this logic by adding support for version upgrade component by component - if err := unstructured.SetNestedField(controlPlane.UnstructuredContent(), current.cluster.Spec.Topology.Version, "spec", "version"); err != nil { + if err := unstructured.SetNestedField(controlPlane.UnstructuredContent(), controlPlaneVersion, "spec", "version"); err != nil { return nil, errors.Wrap(err, "failed to set spec.version in the ControlPlane object") } return controlPlane, nil } +// computeControlPlaneVersion computes the ControlPlane version based on the current clusterTopologyState. +// TODO: we also have to handle the following cases: +// * ControlPlane.spec.version != MachineDeployment[].spec.template.spec.version, i.e. one of the MachineDeployments has +// a different version then the ControlPlane. +func computeControlPlaneVersion(current *clusterTopologyState) (string, error) { + topologyVersion := current.cluster.Spec.Topology.Version + + // ControlPlane does not exist yet, use topologyVersion for new ControlPlane. + if current.controlPlane.object == nil { + return topologyVersion, nil + } + + controlPlaneVersion, ok, err := getControlPlaneVersion(current.controlPlane.object, "spec", "version") + if err != nil { + return "", err + } + if !ok { + return "", errors.Errorf("failed to get .spec.version from ControlPlane: not found") + } + controlPlaneStatusVersion, statusVersionSet, err := getControlPlaneVersion(current.controlPlane.object, "status", "version") + if err != nil { + return "", err + } + + // Do not trigger another rollout, while a rollout is already in progress. A rollout is still in progress if: + // * .status.version is not set + // * .status.version is set but not equal to .spec.version + if !statusVersionSet || controlPlaneVersion != controlPlaneStatusVersion { + return controlPlaneVersion, nil + } + + // ControlPlane downgrade is not allowed. + if err := isDowngrade(controlPlaneVersion, topologyVersion); err != nil { + return "", err + } + + // Either no-op or ControlPlane will be upgraded. + return topologyVersion, nil +} + +// getControlPlaneVersion gets the version from the specified path of a ControlPlane. +func getControlPlaneVersion(controlPlane *unstructured.Unstructured, path ...string) (string, bool, error) { + version, ok, err := unstructured.NestedString(controlPlane.UnstructuredContent(), path...) + if err != nil { + return "", ok, errors.Wrapf(err, "failed to get %s from ControlPlane", strings.Join(path, ".")) + } + return version, ok, nil +} + +// isDowngrade compares currentVersion and desiredVersion and returns an error if they either cannot be parsed +// or if a downgrade is detected. +func isDowngrade(currentVersion, desiredVersion string) error { + currentVersionParsed, err := version.ParseMajorMinorPatch(currentVersion) + if err != nil { + return errors.Errorf("failed to parse current version %q: %v", currentVersion, err) + } + desiredVersionParsed, err := version.ParseMajorMinorPatch(desiredVersion) + if err != nil { + return errors.Errorf("failed to parse desired version %q: %v", desiredVersion, err) + } + if desiredVersionParsed.LT(currentVersionParsed) { + return errors.Errorf("downgrading from %s to %s is not supported", currentVersion, desiredVersion) + } + return nil +} + // computeCluster computes the desired state for the Cluster object. // NOTE: Some fields of the Cluster’s fields contribute to defining how a Cluster should look like (e.g. Cluster.Spec.Topology), // while some other fields should be managed as part of the actual Cluster (e.g. Cluster.Spec.ControlPlaneRef); in this func diff --git a/controllers/topology/desired_state_test.go b/controllers/topology/desired_state_test.go index 7dd9d930bf45..217d5164f14a 100644 --- a/controllers/topology/desired_state_test.go +++ b/controllers/topology/desired_state_test.go @@ -381,6 +381,118 @@ func TestComputeControlPlane(t *testing.T) { }) } +func TestComputeControlPlaneVersion(t *testing.T) { + tests := []struct { + name string + topologyVersion string + currentControlPlane *unstructured.Unstructured + want string + wantErr bool + }{ + { + name: "ControlPlane does not exist", + topologyVersion: "v1.21.0", + currentControlPlane: nil, + want: "v1.21.0", + }, + { + name: "ControlPlane does exist, but without version", + topologyVersion: "v1.21.0", + currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp").Obj(), + wantErr: true, + }, + { + name: "ControlPlane does exist, with same version (no-op)", + topologyVersion: "v1.21.0", + currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp"). + WithVersion("v1.21.0"). + WithStatusVersion("v1.21.0"). + Obj(), + want: "v1.21.0", + }, + { + name: "ControlPlane does exist, with newer version (downgrade)", + topologyVersion: "v1.21.0", + currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp"). + WithVersion("v1.22.0"). + WithStatusVersion("v1.22.0"). + Obj(), + wantErr: true, + }, + { + name: "ControlPlane does exist, with invalid version", + topologyVersion: "v1.21.0", + currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp"). + WithVersion("invalid-version"). + WithStatusVersion("invalid-version"). + Obj(), + wantErr: true, + }, + { + name: "ControlPlane does exist, with valid version but invalid topology version", + topologyVersion: "invalid-version", + currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp"). + WithVersion("v1.21.0"). + WithStatusVersion("v1.21.0"). + Obj(), + wantErr: true, + }, + { + name: "ControlPlane does exist, with older version (upgrade)", + topologyVersion: "v1.21.0", + currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp"). + WithVersion("v1.20.0"). + WithStatusVersion("v1.20.0"). + Obj(), + want: "v1.21.0", + }, + { + name: "ControlPlane does exist, with older version (upgrade) but another upgrade is already in progress (.status.version has older version)", + topologyVersion: "v1.22.0", + currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp"). + WithVersion("v1.21.0"). + WithStatusVersion("v1.20.0"). + Obj(), + want: "v1.21.0", + }, + { + name: "ControlPlane does exist, with older version (upgrade) but another upgrade is already in progress (.status.version is not set)", + topologyVersion: "v1.22.0", + currentControlPlane: newFakeControlPlane(metav1.NamespaceDefault, "cp"). + WithVersion("v1.21.0"). + Obj(), + want: "v1.21.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + current := &clusterTopologyState{ + cluster: &clusterv1.Cluster{ + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{ + Version: tt.topologyVersion, + }, + }, + }, + controlPlane: controlPlaneTopologyState{ + object: tt.currentControlPlane, + }, + } + + got, err := computeControlPlaneVersion(current) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + g.Expect(got).To(Equal(tt.want)) + }) + } +} + func TestComputeCluster(t *testing.T) { // generated objects infrastructureCluster := newFakeInfrastructureCluster(metav1.NamespaceDefault, "infrastructureCluster1").Obj() diff --git a/controllers/topology/object_builders_test.go b/controllers/topology/object_builders_test.go index d256008e8b31..f6afa6e6d80a 100644 --- a/controllers/topology/object_builders_test.go +++ b/controllers/topology/object_builders_test.go @@ -395,6 +395,8 @@ type fakeControlPlane struct { namespace string name string infrastructureMachineTemplate *unstructured.Unstructured + version *string + statusVersion *string } func newFakeControlPlane(namespace, name string) *fakeControlPlane { @@ -409,6 +411,16 @@ func (f *fakeControlPlane) WithInfrastructureMachineTemplate(t *unstructured.Uns return f } +func (f *fakeControlPlane) WithVersion(version string) *fakeControlPlane { + f.version = &version + return f +} + +func (f *fakeControlPlane) WithStatusVersion(version string) *fakeControlPlane { + f.statusVersion = &version + return f +} + func (f *fakeControlPlane) Obj() *unstructured.Unstructured { obj := &unstructured.Unstructured{} obj.SetAPIVersion(fakeControlPlaneProviderGroupVersion.String()) @@ -428,5 +440,16 @@ func (f *fakeControlPlane) Obj() *unstructured.Unstructured { panic(err) } } + + if f.version != nil { + if err := unstructured.SetNestedField(obj.UnstructuredContent(), *f.version, "spec", "version"); err != nil { + panic(err) + } + } + if f.statusVersion != nil { + if err := unstructured.SetNestedField(obj.UnstructuredContent(), *f.statusVersion, "status", "version"); err != nil { + panic(err) + } + } return obj }