Make the hypervisor spec immutable on termination

It is hard to undo the termination in certain situations and the field
should usually be set by an automation, so uncommiting to terminate a
node is a rather less common scenario. So let's prohibit it for now.

HA events can always happen, so we still need to be able to switch to
that, and then anything goes.

Author: fwiesel

api/v1/hypervisor_types.go

@@ -68,6 +68,7 @@ const (
 )
 
 // HypervisorSpec defines the desired state of Hypervisor
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.maintenance) || oldSelf.maintenance != 'termination' || self.maintenance == 'ha' || self == oldSelf",message="spec is immutable when maintenance is 'termination'; can only change maintenance to 'ha'"
 type HypervisorSpec struct {
 	// +kubebuilder:validation:Optional
 	// OperatingSystemVersion represents the desired operating system version.

api/v1/hypervisor_validation_test.go

@@ -0,0 +1,206 @@
+/*
+SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors
+SPDX-License-Identifier: Apache-2.0
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+// TestHypervisorSpecValidation tests the CEL validation rule for HypervisorSpec
+// The rule: oldSelf.maintenance != 'termination' || self.maintenance == 'ha' || self == oldSelf
+// This ensures that when maintenance is set to 'termination', the spec cannot be modified
+// unless the maintenance field is changed to 'ha'
+var _ = Describe("Hypervisor Spec CEL Validation", func() {
+	var (
+		hypervisor     *Hypervisor
+		hypervisorName types.NamespacedName
+	)
+
+	BeforeEach(func(ctx SpecContext) {
+		hypervisorName = types.NamespacedName{
+			Name: "test-hypervisor",
+		}
+
+		hypervisor = &Hypervisor{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: hypervisorName.Name,
+			},
+			Spec: HypervisorSpec{
+				OperatingSystemVersion: "1.0",
+				LifecycleEnabled:       true,
+				HighAvailability:       true,
+				EvacuateOnReboot:       true,
+				InstallCertificate:     true,
+				Maintenance:            MaintenanceManual,
+			},
+		}
+
+		Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed())
+
+		DeferCleanup(func(ctx SpecContext) {
+			Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, hypervisor))).To(Succeed())
+		})
+	})
+
+	Context("When maintenance is NOT termination", func() {
+		It("should allow changes to any spec fields", func(ctx SpecContext) {
+			// Update version and other fields
+			hypervisor.Spec.OperatingSystemVersion = "2.0"
+			hypervisor.Spec.HighAvailability = false
+			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+
+			// Verify the update was successful
+			updated := &Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
+			Expect(updated.Spec.OperatingSystemVersion).To(Equal("2.0"))
+			Expect(updated.Spec.HighAvailability).To(BeFalse())
+		})
+
+		It("should allow changing maintenance to termination", func(ctx SpecContext) {
+			hypervisor.Spec.Maintenance = MaintenanceTermination
+			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+
+			updated := &Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
+			Expect(updated.Spec.Maintenance).To(Equal(MaintenanceTermination))
+		})
+	})
+
+	Context("When maintenance IS termination", func() {
+		BeforeEach(func(ctx SpecContext) {
+			// Set maintenance to termination
+			hypervisor.Spec.Maintenance = MaintenanceTermination
+			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+
+			// Refresh to get latest version
+			Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
+		})
+
+		It("should reject changes to version field", func(ctx SpecContext) {
+			hypervisor.Spec.OperatingSystemVersion = "2.0"
+			err := k8sClient.Update(ctx, hypervisor)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec is immutable when maintenance is 'termination'"))
+		})
+
+		It("should reject changes to boolean fields", func(ctx SpecContext) {
+			hypervisor.Spec.LifecycleEnabled = false
+			err := k8sClient.Update(ctx, hypervisor)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec is immutable when maintenance is 'termination'"))
+		})
+
+		It("should reject changes to array fields", func(ctx SpecContext) {
+			hypervisor.Spec.CustomTraits = []string{"new-trait"}
+			err := k8sClient.Update(ctx, hypervisor)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec is immutable when maintenance is 'termination'"))
+		})
+
+		It("should reject changing maintenance to manual", func(ctx SpecContext) {
+			hypervisor.Spec.Maintenance = MaintenanceManual
+			err := k8sClient.Update(ctx, hypervisor)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec is immutable when maintenance is 'termination'"))
+		})
+
+		It("should reject changing maintenance to auto", func(ctx SpecContext) {
+			hypervisor.Spec.Maintenance = MaintenanceAuto
+			err := k8sClient.Update(ctx, hypervisor)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec is immutable when maintenance is 'termination'"))
+		})
+
+		It("should reject changing maintenance to empty", func(ctx SpecContext) {
+			hypervisor.Spec.Maintenance = MaintenanceUnset
+			err := k8sClient.Update(ctx, hypervisor)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("spec is immutable when maintenance is 'termination'"))
+		})
+
+		It("should allow changing maintenance to ha", func(ctx SpecContext) {
+			hypervisor.Spec.Maintenance = MaintenanceHA
+			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+
+			updated := &Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
+			Expect(updated.Spec.Maintenance).To(Equal(MaintenanceHA))
+		})
+
+		It("should allow changing maintenance to ha with other spec changes", func(ctx SpecContext) {
+			hypervisor.Spec.Maintenance = MaintenanceHA
+			hypervisor.Spec.OperatingSystemVersion = "2.0"
+			hypervisor.Spec.LifecycleEnabled = false
+			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+
+			updated := &Hypervisor{}
+			Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
+			Expect(updated.Spec.Maintenance).To(Equal(MaintenanceHA))
+			Expect(updated.Spec.OperatingSystemVersion).To(Equal("2.0"))
+			Expect(updated.Spec.LifecycleEnabled).To(BeFalse())
+		})
+
+		It("should allow no-op updates (no changes)", func(ctx SpecContext) {
+			// Update with same values should succeed
+			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+		})
+	})
+
+	Context("When creating a new Hypervisor", func() {
+		It("should allow creation with maintenance set to termination", func(ctx SpecContext) {
+			newHypervisor := &Hypervisor{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "new-test-hypervisor",
+				},
+				Spec: HypervisorSpec{
+					Maintenance:            MaintenanceTermination,
+					OperatingSystemVersion: "1.0",
+					LifecycleEnabled:       true,
+					HighAvailability:       true,
+					EvacuateOnReboot:       true,
+					InstallCertificate:     true,
+				},
+			}
+
+			Expect(k8sClient.Create(ctx, newHypervisor)).To(Succeed())
+
+			DeferCleanup(func(ctx SpecContext) {
+				Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, newHypervisor))).To(Succeed())
+			})
+
+			created := &Hypervisor{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "new-test-hypervisor"}, created)).To(Succeed())
+			Expect(created.Spec.Maintenance).To(Equal(MaintenanceTermination))
+		})
+	})
+})
+
+// TestMaintenanceConstants verifies the maintenance mode constants
+var _ = Describe("Maintenance Constants", func() {
+	It("should have correct constant values", func() {
+		Expect(MaintenanceUnset).To(Equal(""))
+		Expect(MaintenanceManual).To(Equal("manual"))
+		Expect(MaintenanceAuto).To(Equal("auto"))
+		Expect(MaintenanceHA).To(Equal("ha"))
+		Expect(MaintenanceTermination).To(Equal("termination"))
+	})
+})

api/v1/suite_test.go

@@ -0,0 +1,74 @@
+/*
+SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors
+SPDX-License-Identifier: Apache-2.0
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1
+
+import (
+	"fmt"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/envtest"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+)
+
+var cfg *rest.Config
+var k8sClient client.Client
+var testEnv *envtest.Environment
+
+func TestAPIs(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "API v1 Suite")
+}
+
+var _ = BeforeSuite(func() {
+	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
+
+	By("bootstrapping test environment")
+	testEnv = &envtest.Environment{
+		CRDDirectoryPaths:     []string{filepath.Join("..", "..", "config", "crd", "bases")},
+		ErrorIfCRDPathMissing: true,
+		BinaryAssetsDirectory: filepath.Join("..", "..", "bin", "k8s",
+			fmt.Sprintf("1.31.0-%s-%s", runtime.GOOS, runtime.GOARCH)),
+	}
+
+	var err error
+	cfg, err = testEnv.Start()
+	Expect(err).NotTo(HaveOccurred())
+	Expect(cfg).NotTo(BeNil())
+
+	err = AddToScheme(scheme.Scheme)
+	Expect(err).NotTo(HaveOccurred())
+
+	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
+	Expect(err).NotTo(HaveOccurred())
+	Expect(k8sClient).NotTo(BeNil())
+})
+
+var _ = AfterSuite(func() {
+	By("tearing down the test environment")
+	err := testEnv.Stop()
+	Expect(err).NotTo(HaveOccurred())
+})

charts/openstack-hypervisor-operator/crds/hypervisor-crd.yaml

@@ -185,6 +185,11 @@ spec:
             - reboot
             - skipTests
             type: object
+            x-kubernetes-validations:
+            - message: spec is immutable when maintenance is 'termination'; can only
+                change maintenance to 'ha'
+              rule: '!has(oldSelf.maintenance) || oldSelf.maintenance != ''termination''
+                || self.maintenance == ''ha'' || self == oldSelf'
           status:
             description: HypervisorStatus defines the observed state of Hypervisor
             properties:

config/crd/bases/kvm.cloud.sap_hypervisors.yaml

@@ -186,6 +186,11 @@ spec:
             - reboot
             - skipTests
             type: object
+            x-kubernetes-validations:
+            - message: spec is immutable when maintenance is 'termination'; can only
+                change maintenance to 'ha'
+              rule: '!has(oldSelf.maintenance) || oldSelf.maintenance != ''termination''
+                || self.maintenance == ''ha'' || self == oldSelf'
           status:
             description: HypervisorStatus defines the observed state of Hypervisor
             properties: