Merge pull request #932 from syself/ani/issues/928

✨ Add webhook to check if same serverID baremetal host exist or not

api/v1beta1/hetznerbaremetalhost_webhook.go

@@ -17,19 +17,31 @@ limitations under the License.
 package v1beta1
 
 import (
-	"reflect"
+	"context"
+	"fmt"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
+// HetznerBareMetalHostWebhook implements validating and defaulting webhook for HetznerBareMetalHost.
+// +k8s:deepcopy-gen=false
+type HetznerBareMetalHostWebhook struct {
+	c client.Client
+}
+
 // SetupWebhookWithManager initializes webhook manager for HetznerBareMetalHost.
-func (host *HetznerBareMetalHost) SetupWebhookWithManager(mgr ctrl.Manager) error {
+func (hw *HetznerBareMetalHostWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	hw.c = mgr.GetClient()
+
 	return ctrl.NewWebhookManagedBy(mgr).
-		For(host).
+		For(&HetznerBareMetalHost{}).
+		WithValidator(hw).
 		Complete()
 }
 
@@ -43,28 +55,57 @@ func (host *HetznerBareMetalHost) Default() {
 
 //+kubebuilder:webhook:path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-hetznerbaremetalhost,mutating=false,failurePolicy=fail,sideEffects=None,groups=infrastructure.cluster.x-k8s.io,resources=hetznerbaremetalhosts,verbs=create;update,versions=v1beta1,name=validation.hetznerbaremetalhost.infrastructure.cluster.x-k8s.io,admissionReviewVersions={v1,v1beta1}
 
-var _ webhook.Validator = &HetznerBareMetalHost{}
+var _ webhook.CustomValidator = &HetznerBareMetalHostWebhook{}
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
-func (host *HetznerBareMetalHost) ValidateCreate() (admission.Warnings, error) {
-	return nil, nil
+func (hw *HetznerBareMetalHostWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	host, ok := (obj).(*HetznerBareMetalHost)
+	if !ok {
+		return admission.Warnings{}, apierrors.NewBadRequest(fmt.Sprintf("expected HetznerBareMetalHost, but got %T", host))
+	}
+
+	var allErrs field.ErrorList
+
+	hetznerBareMetalHostList := &HetznerBareMetalHostList{}
+	if err := hw.c.List(ctx, hetznerBareMetalHostList, &client.ListOptions{}); err != nil {
+		return admission.Warnings{fmt.Sprintf("could not verify that the host has a unique serverID: %s", err.Error())}, nil
+	}
+
+	for _, hetznerBareMetalHost := range hetznerBareMetalHostList.Items {
+		if hetznerBareMetalHost.Spec.ServerID == host.Spec.ServerID {
+			allErrs = append(allErrs,
+				field.Invalid(field.NewPath("spec", "ServerID"), host.Spec.ServerID, fmt.Sprintf("%q host exist with same serverID: %d", hetznerBareMetalHost.Name, host.Spec.ServerID)),
+			)
+		}
+	}
+
+	return nil, aggregateObjErrors(hetznerBareMetalHostList.GroupVersionKind().GroupKind(), host.Name, allErrs)
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (host *HetznerBareMetalHost) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
+func (hw *HetznerBareMetalHostWebhook) ValidateUpdate(_ context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+	oldHost, ok := oldObj.(*HetznerBareMetalHost)
+	if !ok {
+		return admission.Warnings{}, apierrors.NewBadRequest(fmt.Sprintf("expected an ClusterStack but got a %T", oldObj))
+	}
+
+	newHost, ok := newObj.(*HetznerBareMetalHost)
+	if !ok {
+		return admission.Warnings{}, apierrors.NewBadRequest(fmt.Sprintf("expected an ClusterStack but got a %T", newHost))
+	}
+
 	var allErrs field.ErrorList
 
-	oldHetznerBareMetalHost := old.(*HetznerBareMetalHost)
-	if !reflect.DeepEqual(host.Spec.ServerID, oldHetznerBareMetalHost.Spec.ServerID) {
+	if newHost.Spec.ServerID != oldHost.Spec.ServerID {
 		allErrs = append(allErrs,
-			field.Invalid(field.NewPath("spec", "serverID"), host.Spec.ServerID, "ServerID is immutable"),
+			field.Invalid(field.NewPath("spec", "serverID"), newHost.Spec.ServerID, "serverID is immutable"),
 		)
 	}
 
-	return nil, aggregateObjErrors(host.GroupVersionKind().GroupKind(), host.Name, allErrs)
+	return nil, aggregateObjErrors(newHost.GroupVersionKind().GroupKind(), newHost.Name, allErrs)
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
-func (host *HetznerBareMetalHost) ValidateDelete() (admission.Warnings, error) {
+func (hw *HetznerBareMetalHostWebhook) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) {
 	return nil, nil
 }

controllers/hetznerbaremetalhost_controller_test.go

@@ -139,7 +139,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 		osSSHClientAfterInstallImage = testEnv.OSSSHClientAfterInstallImage
 		osSSHClientAfterCloudInit = testEnv.OSSSHClientAfterCloudInit
 
-		robotClient.On("GetBMServer", 1).Return(&models.Server{
+		robotClient.On("GetBMServer", mock.Anything).Return(&models.Server{
 			ServerNumber: 1,
 			ServerIP:     "1.2.3.4",
 			Rescue:       true,
@@ -151,12 +151,12 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 				Data:        "my-public-key",
 			},
 		}, nil)
-		robotClient.On("GetReboot", 1).Return(&models.Reset{Type: []string{"hw", "sw"}}, nil)
+		robotClient.On("GetReboot", mock.Anything).Return(&models.Reset{Type: []string{"hw", "sw"}}, nil)
 		robotClient.On("GetBootRescue", 1).Return(&models.Rescue{Active: true}, nil)
-		robotClient.On("SetBootRescue", 1, mock.Anything).Return(&models.Rescue{Active: true}, nil)
-		robotClient.On("DeleteBootRescue", 1).Return(&models.Rescue{Active: false}, nil)
+		robotClient.On("SetBootRescue", mock.Anything, mock.Anything).Return(&models.Rescue{Active: true}, nil)
+		robotClient.On("DeleteBootRescue", mock.Anything).Return(&models.Rescue{Active: false}, nil)
 		robotClient.On("RebootBMServer", mock.Anything, mock.Anything).Return(&models.ResetPost{}, nil)
-		robotClient.On("SetBMServerName", 1, mock.Anything).Return(nil, nil)
+		robotClient.On("SetBMServerName", mock.Anything, mock.Anything).Return(nil, nil)
 
 		configureRescueSSHClient(rescueSSHClient)
 
@@ -202,6 +202,10 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
 			key = client.ObjectKey{Namespace: testNs.Name, Name: host.Name}
 		})
 
+		AfterEach(func() {
+			Expect(testEnv.Cleanup(ctx, host)).To(Succeed())
+		})
+
 		It("creates the host machine", func() {
 			Eventually(func() bool {
 				if err := testEnv.Get(ctx, key, host); err != nil {
@@ -637,7 +641,7 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 		robotClient = testEnv.RobotClient
 		rescueSSHClient = testEnv.RescueSSHClient
 
-		robotClient.On("GetBMServer", 1).Return(&models.Server{
+		robotClient.On("GetBMServer", mock.Anything).Return(&models.Server{
 			ServerNumber: 1,
 			ServerIP:     "1.2.3.4",
 			Rescue:       true,
@@ -649,9 +653,9 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
 				Data:        "my-public-key",
 			},
 		}, nil)
-		robotClient.On("GetReboot", 1).Return(&models.Reset{Type: []string{"hw", "sw"}}, nil)
-		robotClient.On("SetBootRescue", 1, mock.Anything).Return(&models.Rescue{Active: true}, nil)
-		robotClient.On("DeleteBootRescue", 1).Return(&models.Rescue{Active: true}, nil)
+		robotClient.On("GetReboot", mock.Anything).Return(&models.Reset{Type: []string{"hw", "sw"}}, nil)
+		robotClient.On("SetBootRescue", mock.Anything, mock.Anything).Return(&models.Rescue{Active: true}, nil)
+		robotClient.On("DeleteBootRescue", mock.Anything).Return(&models.Rescue{Active: true}, nil)
 		robotClient.On("RebootBMServer", mock.Anything, mock.Anything).Return(&models.ResetPost{}, nil)
 
 		configureRescueSSHClient(rescueSSHClient)

controllers/hetznerbaremetalmachine_controller_test.go

@@ -128,7 +128,7 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 		rescueSSHClient = testEnv.RescueSSHClient
 		osSSHClient = testEnv.OSSSHClientAfterInstallImage
 
-		robotClient.On("GetBMServer", 1).Return(&models.Server{
+		robotClient.On("GetBMServer", mock.Anything).Return(&models.Server{
 			ServerNumber: 1,
 			ServerIP:     "1.2.3.4",
 			Rescue:       true,
@@ -140,12 +140,12 @@ var _ = Describe("HetznerBareMetalMachineReconciler", func() {
 				Data:        "my-public-key",
 			},
 		}, nil)
-		robotClient.On("GetReboot", 1).Return(&models.Reset{Type: []string{"hw", "sw"}}, nil)
+		robotClient.On("GetReboot", mock.Anything).Return(&models.Reset{Type: []string{"hw", "sw"}}, nil)
 		robotClient.On("GetBootRescue", 1).Return(&models.Rescue{Active: true}, nil)
-		robotClient.On("SetBootRescue", 1, mock.Anything).Return(&models.Rescue{Active: true}, nil)
-		robotClient.On("DeleteBootRescue", 1).Return(&models.Rescue{Active: false}, nil)
+		robotClient.On("SetBootRescue", mock.Anything, mock.Anything).Return(&models.Rescue{Active: true}, nil)
+		robotClient.On("DeleteBootRescue", mock.Anything).Return(&models.Rescue{Active: false}, nil)
 		robotClient.On("RebootBMServer", mock.Anything, mock.Anything).Return(&models.ResetPost{}, nil)
-		robotClient.On("SetBMServerName", 1, mock.Anything).Return(nil, nil)
+		robotClient.On("SetBMServerName", mock.Anything, mock.Anything).Return(nil, nil)
 
 		configureRescueSSHClient(rescueSSHClient)
 

main.go

@@ -237,7 +237,7 @@ func setUpWebhookWithManager(mgr ctrl.Manager) {
 		setupLog.Error(err, "unable to create webhook", "webhook", "HCloudMachineTemplate")
 		os.Exit(1)
 	}
-	if err := (&infrastructurev1beta1.HetznerBareMetalHost{}).SetupWebhookWithManager(mgr); err != nil {
+	if err := (&infrastructurev1beta1.HetznerBareMetalHostWebhook{}).SetupWebhookWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create webhook", "webhook", "HetznerBareMetalHost")
 		os.Exit(1)
 	}

pkg/services/baremetal/host/host_suite_test.go

@@ -36,7 +36,6 @@ import (
 )
 
 const (
-	bareMetalHostID  = 1
 	sshFingerprint   = "my-fingerprint"
 	osSSHKeyName     = "os-sshkey"
 	rescueSSHKeyName = "rescue-sshkey"

pkg/services/baremetal/host/host_test.go

@@ -201,9 +201,9 @@ var _ = Describe("handleIncompleteBoot", func() {
 		DescribeTable("hostName = rescue, varying error type and ssh client response - robot client giving all positive results, no timeouts",
 			func(tc testCaseHandleIncompleteBootCorrectHostname) {
 				robotMock := robotmock.Client{}
-				robotMock.On("SetBootRescue", bareMetalHostID, sshFingerprint).Return(nil, nil)
-				robotMock.On("GetBootRescue", bareMetalHostID).Return(&models.Rescue{Active: true}, nil)
-				robotMock.On("RebootBMServer", bareMetalHostID, mock.Anything).Return(nil, nil)
+				robotMock.On("SetBootRescue", mock.Anything, sshFingerprint).Return(nil, nil)
+				robotMock.On("GetBootRescue", mock.Anything).Return(&models.Rescue{Active: true}, nil)
+				robotMock.On("RebootBMServer", mock.Anything, mock.Anything).Return(nil, nil)
 
 				host := helpers.BareMetalHost("test-host", "default",
 					helpers.WithRebootTypes([]infrav1.RebootType{
@@ -295,9 +295,9 @@ var _ = Describe("handleIncompleteBoot", func() {
 		DescribeTable("Different reset types",
 			func(tc testCaseHandleIncompleteBootDifferentResetTypes) {
 				robotMock := robotmock.Client{}
-				robotMock.On("SetBootRescue", bareMetalHostID, sshFingerprint).Return(nil, nil)
-				robotMock.On("GetBootRescue", bareMetalHostID).Return(&models.Rescue{Active: true}, nil)
-				robotMock.On("RebootBMServer", bareMetalHostID, mock.Anything).Return(nil, nil)
+				robotMock.On("SetBootRescue", mock.Anything, sshFingerprint).Return(nil, nil)
+				robotMock.On("GetBootRescue", mock.Anything).Return(&models.Rescue{Active: true}, nil)
+				robotMock.On("RebootBMServer", mock.Anything, mock.Anything).Return(nil, nil)
 
 				host := helpers.BareMetalHost("test-host", "default",
 					helpers.WithSSHSpec(),
@@ -311,9 +311,9 @@ var _ = Describe("handleIncompleteBoot", func() {
 				Expect(service.handleIncompleteBoot(true, tc.isTimeOut, tc.isConnectionRefused)).To(Succeed())
 				Expect(host.Spec.Status.ErrorType).To(Equal(tc.expectedHostErrorType))
 				if tc.expectedRebootType != infrav1.RebootType("") {
-					Expect(robotMock.AssertCalled(GinkgoT(), "RebootBMServer", bareMetalHostID, tc.expectedRebootType)).To(BeTrue())
+					Expect(robotMock.AssertCalled(GinkgoT(), "RebootBMServer", mock.Anything, tc.expectedRebootType)).To(BeTrue())
 				} else {
-					Expect(robotMock.AssertNotCalled(GinkgoT(), "RebootBMServer", bareMetalHostID, mock.Anything)).To(BeTrue())
+					Expect(robotMock.AssertNotCalled(GinkgoT(), "RebootBMServer", mock.Anything, mock.Anything)).To(BeTrue())
 				}
 			},
 			Entry("timeout, no errorType, only hw reset", testCaseHandleIncompleteBootDifferentResetTypes{
@@ -377,9 +377,9 @@ var _ = Describe("handleIncompleteBoot", func() {
 		DescribeTable("Different timeouts",
 			func(tc testCaseHandleIncompleteBootDifferentTimeouts) {
 				robotMock := robotmock.Client{}
-				robotMock.On("SetBootRescue", bareMetalHostID, sshFingerprint).Return(nil, nil)
-				robotMock.On("GetBootRescue", bareMetalHostID).Return(&models.Rescue{Active: true}, nil)
-				robotMock.On("RebootBMServer", bareMetalHostID, mock.Anything).Return(nil, nil)
+				robotMock.On("SetBootRescue", mock.Anything, sshFingerprint).Return(nil, nil)
+				robotMock.On("GetBootRescue", mock.Anything).Return(&models.Rescue{Active: true}, nil)
+				robotMock.On("RebootBMServer", mock.Anything, mock.Anything).Return(nil, nil)
 
 				host := helpers.BareMetalHost("test-host", "default",
 					helpers.WithRebootTypes([]infrav1.RebootType{
@@ -396,9 +396,9 @@ var _ = Describe("handleIncompleteBoot", func() {
 				Expect(service.handleIncompleteBoot(true, true, false)).To(Succeed())
 				Expect(host.Spec.Status.ErrorType).To(Equal(tc.expectedHostErrorType))
 				if tc.expectedRebootType != infrav1.RebootType("") {
-					Expect(robotMock.AssertCalled(GinkgoT(), "RebootBMServer", bareMetalHostID, tc.expectedRebootType)).To(BeTrue())
+					Expect(robotMock.AssertCalled(GinkgoT(), "RebootBMServer", mock.Anything, tc.expectedRebootType)).To(BeTrue())
 				} else {
-					Expect(robotMock.AssertNotCalled(GinkgoT(), "RebootBMServer", bareMetalHostID, mock.Anything)).To(BeTrue())
+					Expect(robotMock.AssertNotCalled(GinkgoT(), "RebootBMServer", mock.Anything, mock.Anything)).To(BeTrue())
 				}
 			},
 			Entry("timed out hw reset", testCaseHandleIncompleteBootDifferentTimeouts{
@@ -440,9 +440,9 @@ var _ = Describe("handleIncompleteBoot", func() {
 		DescribeTable("vary hostname and see whether rescue gets triggered",
 			func(tc testCaseHandleIncompleteBoot) {
 				robotMock := robotmock.Client{}
-				robotMock.On("SetBootRescue", bareMetalHostID, sshFingerprint).Return(nil, nil)
-				robotMock.On("GetBootRescue", bareMetalHostID).Return(&models.Rescue{Active: true}, nil)
-				robotMock.On("RebootBMServer", bareMetalHostID, mock.Anything).Return(nil, nil)
+				robotMock.On("SetBootRescue", mock.Anything, sshFingerprint).Return(nil, nil)
+				robotMock.On("GetBootRescue", mock.Anything).Return(&models.Rescue{Active: true}, nil)
+				robotMock.On("RebootBMServer", mock.Anything, mock.Anything).Return(nil, nil)
 
 				host := helpers.BareMetalHost("test-host", "default",
 					helpers.WithRebootTypes([]infrav1.RebootType{
@@ -463,9 +463,9 @@ var _ = Describe("handleIncompleteBoot", func() {
 				}
 				Expect(host.Spec.Status.ErrorType).To(Equal(tc.expectedHostErrorType))
 				if tc.expectsRescueCall {
-					Expect(robotMock.AssertCalled(GinkgoT(), "GetBootRescue", bareMetalHostID)).To(BeTrue())
+					Expect(robotMock.AssertCalled(GinkgoT(), "GetBootRescue", mock.Anything)).To(BeTrue())
 				} else {
-					Expect(robotMock.AssertNotCalled(GinkgoT(), "GetBootRescue", bareMetalHostID)).To(BeTrue())
+					Expect(robotMock.AssertNotCalled(GinkgoT(), "GetBootRescue", mock.Anything)).To(BeTrue())
 				}
 			},
 			Entry("hostname == rescue", testCaseHandleIncompleteBoot{
@@ -642,7 +642,7 @@ var _ = Describe("analyzeSSHOutputInstallImage", func() {
 			)
 
 			robotMock := robotmock.Client{}
-			robotMock.On("GetBootRescue", bareMetalHostID).Return(&models.Rescue{Active: tc.rescueActive}, nil)
+			robotMock.On("GetBootRescue", mock.Anything).Return(&models.Rescue{Active: tc.rescueActive}, nil)
 
 			service := newTestService(host, &robotMock, nil, nil, nil)
 
@@ -719,7 +719,7 @@ var _ = Describe("analyzeSSHOutputInstallImage", func() {
 			)
 
 			robotMock := robotmock.Client{}
-			robotMock.On("GetBootRescue", bareMetalHostID).Return(&models.Rescue{Active: true}, nil)
+			robotMock.On("GetBootRescue", mock.Anything).Return(&models.Rescue{Active: true}, nil)
 
 			service := newTestService(host, &robotMock, nil, nil, nil)
 
@@ -1141,7 +1141,7 @@ var _ = Describe("actionRegistering", func() {
 			sshMock.On("GetHostName").Return(tc.getHostNameOutput)
 
 			robotMock := robotmock.Client{}
-			robotMock.On("GetBootRescue", bareMetalHostID).Return(&models.Rescue{Active: false}, nil)
+			robotMock.On("GetBootRescue", mock.Anything).Return(&models.Rescue{Active: false}, nil)
 
 			service := newTestService(host, &robotMock, bmmock.NewSSHFactory(sshMock, sshMock, sshMock), nil, helpers.GetDefaultSSHSecret(rescueSSHKeyName, "default"))
 
@@ -1282,7 +1282,7 @@ var _ = Describe("actionEnsureProvisioned", func() {
 			oldSSHMock.On("Reboot").Return(sshclient.Output{})
 
 			robotMock := robotmock.Client{}
-			robotMock.On("SetBMServerName", bareMetalHostID, infrav1.BareMetalHostNamePrefix+host.Spec.ConsumerRef.Name).Return(nil, nil)
+			robotMock.On("SetBMServerName", mock.Anything, infrav1.BareMetalHostNamePrefix+host.Spec.ConsumerRef.Name).Return(nil, nil)
 
 			service := newTestService(host, &robotMock, bmmock.NewSSHFactory(sshMock, oldSSHMock, sshMock), helpers.GetDefaultSSHSecret(osSSHKeyName, "default"), nil)
 

test/helpers/defaults.go

@@ -22,6 +22,7 @@ import (
 	infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kuberand "k8s.io/apimachinery/pkg/util/rand"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 )
 
@@ -47,7 +48,7 @@ func BareMetalHost(name, namespace string, opts ...HostOpts) *infrav1.HetznerBar
 			Namespace: namespace,
 		},
 		Spec: infrav1.HetznerBareMetalHostSpec{
-			ServerID: bareMetalHostID,
+			ServerID: kuberand.Intn(1000),
 		},
 	}
 	for _, o := range opts {

test/helpers/envtest.go

@@ -161,7 +161,7 @@ func NewTestEnvironment() *TestEnvironment {
 	if err := (&infrav1.HetznerBareMetalMachineTemplateWebhook{}).SetupWebhookWithManager(mgr); err != nil {
 		klog.Fatalf("failed to set up webhook with manager for HetznerBareMetalMachineTemplate: %s", err)
 	}
-	if err := (&infrav1.HetznerBareMetalHost{}).SetupWebhookWithManager(mgr); err != nil {
+	if err := (&infrav1.HetznerBareMetalHostWebhook{}).SetupWebhookWithManager(mgr); err != nil {
 		klog.Fatalf("failed to set up webhook with manager for HetznerBareMetalHost: %s", err)
 	}
 	if err := (&infrav1.HetznerBareMetalRemediation{}).SetupWebhookWithManager(mgr); err != nil {