diff --git a/api/v1beta1/hetznerbaremetalhost_webhook.go b/api/v1beta1/hetznerbaremetalhost_webhook.go index 8fdbf474f..b6379682a 100644 --- a/api/v1beta1/hetznerbaremetalhost_webhook.go +++ b/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 } diff --git a/controllers/hetznerbaremetalhost_controller_test.go b/controllers/hetznerbaremetalhost_controller_test.go index 2ac03a401..47348ffbf 100644 --- a/controllers/hetznerbaremetalhost_controller_test.go +++ b/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) diff --git a/controllers/hetznerbaremetalmachine_controller_test.go b/controllers/hetznerbaremetalmachine_controller_test.go index a30bf216c..023f5c260 100644 --- a/controllers/hetznerbaremetalmachine_controller_test.go +++ b/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) diff --git a/main.go b/main.go index efbe22841..ba5fba695 100644 --- a/main.go +++ b/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) } diff --git a/pkg/services/baremetal/host/host_suite_test.go b/pkg/services/baremetal/host/host_suite_test.go index 2bb281f00..db1e9dce1 100644 --- a/pkg/services/baremetal/host/host_suite_test.go +++ b/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" diff --git a/pkg/services/baremetal/host/host_test.go b/pkg/services/baremetal/host/host_test.go index 14bab7cfa..6ec908619 100644 --- a/pkg/services/baremetal/host/host_test.go +++ b/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) diff --git a/test/helpers/defaults.go b/test/helpers/defaults.go index 3481a8543..5bc1a8222 100644 --- a/test/helpers/defaults.go +++ b/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 { diff --git a/test/helpers/envtest.go b/test/helpers/envtest.go index 274db0d01..54bd853b6 100644 --- a/test/helpers/envtest.go +++ b/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 {