From 8a95ab0362aa4ac052ed318a681dd0ff9d2df84c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20Cr=C3=A9gut?= Date: Tue, 20 Jan 2026 09:27:15 +0100 Subject: [PATCH 1/2] Update logic for HostClaims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update ensures the synchronization between HostClaim and BareMetalHost. * Copy HostClaim secrets to BareMetalHost namespace * Ensure propagation of reboot annotations * Synchronize hostclaim status (mirrors BareMetalHost conditions) Co-authored-by: Pierre Crégut Co-authored-by: Laurent Roussarie Signed-off-by: Pierre Crégut --- internal/testutil/common.go | 2 +- pkg/hostclaim/hostclaim_manager.go | 311 +++++++++++++++++++++++- pkg/hostclaim/hostclaim_manager_test.go | 235 ++++++++++++++++++ 3 files changed, 544 insertions(+), 4 deletions(-) diff --git a/internal/testutil/common.go b/internal/testutil/common.go index a644f5b953..21ee251a5c 100644 --- a/internal/testutil/common.go +++ b/internal/testutil/common.go @@ -261,7 +261,7 @@ func (hb *HostClaimBuilder) SetMetaData(mdata string) *HostClaimBuilder { return hb } -func (hb *HostClaimBuilder) SetNetworData(ndata string) *HostClaimBuilder { +func (hb *HostClaimBuilder) SetNetworkData(ndata string) *HostClaimBuilder { hb.hostClaim.Spec.NetworkData = &corev1.SecretReference{Name: ndata} return hb } diff --git a/pkg/hostclaim/hostclaim_manager.go b/pkg/hostclaim/hostclaim_manager.go index 9fe42441bd..8329491fae 100644 --- a/pkg/hostclaim/hostclaim_manager.go +++ b/pkg/hostclaim/hostclaim_manager.go @@ -28,15 +28,19 @@ import ( "github.com/go-logr/logr" metal3api "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" + "github.com/metal3-io/baremetal-operator/pkg/secretutils" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" + "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) type HostManager struct { @@ -50,6 +54,8 @@ const ( // PausedAnnotationValue is the value used to mark a BareMetalHost as paused by // a HostClaim. PausedAnnotationValue = "metal3.io/hostclaim" + // rebootDomain is the domain of metal3 reboot annotations. + rebootDomain = "reboot.metal3.io" // Requeueing after 0 is in fact not requeuing. TerminalReueueDelay time.Duration = 0 // Standard delay when waiting for other to settle. @@ -64,8 +70,14 @@ const ( HostClaimKind = "HostClaim" ) -// An error used when there is no BMH satisfying the constraints. -var ErrNoAvailableBMH = errors.New("no available BareMetalHost") +var ( + // An error used when there is no BMH satisfying the constraints. + ErrNoAvailableBMH = errors.New("no available BareMetalHost") + // An error raised when the secret to synchronize does not exists. + ErrNoSecret = errors.New("no Secret found") + // ErrNoBMH raised when the BareMetalHost is not found. + ErrNoBMH = errors.New("no BareMetalHost") +) func NewHostManager(client client.Client, log logr.Logger, claim *metal3api.HostClaim, apireader client.Reader) (*HostManager, error) { return &HostManager{ @@ -85,7 +97,7 @@ func (m *HostManager) SetConditionHostToFalse( conditions.Set(m.HostClaim, metav1.Condition{Type: t, Status: metav1.ConditionFalse, Reason: reason, Message: message}) } -// SetConditionHostToFalse sets Host condition status to False. +// SetConditionHostToTrue sets Host condition status to True. func (m *HostManager) SetConditionHostToTrue( t string, reason string, @@ -157,6 +169,92 @@ func (m *HostManager) Associate(ctx context.Context) error { return nil } +// Update updates a hostclaim and is invoked by the HostClaim Controller. +func (m *HostManager) Update(ctx context.Context) error { + m.Log.V(1).Info("Updating HostClaim") + + bmh, err := m.getBmh(ctx) + if err != nil && !errors.Is(err, ErrNoBMH) { + m.SetConditionHostToFalse( + metal3api.AssociatedCondition, metal3api.MissingBareMetalHostReason, + "Failed to get a BareMetalHost for the Host") + return err + } + if bmh == nil { + m.SetConditionHostToFalse( + metal3api.AssociatedCondition, metal3api.MissingBareMetalHostReason, + "BareMetalHost associated to the claim not found") + return errors.New("BareMetalHost not found") + } + + // ensure that the BMH specs are correctly set. + updated, err := m.setBmhSpec(ctx, bmh) + if err != nil { + return err + } + + if bmh.Annotations == nil { + bmh.Annotations = map[string]string{} + } + + if syncReboot(m.HostClaim.Annotations, bmh.Annotations) { + updated = true + } + if updated { + m.Log.Info("Update the BareMetalHost spec: changes detected.") + err = m.client.Update(ctx, bmh) + } + + if err != nil { + sanitizedErr := hideConflictError(err) + if !errors.As(err, &RequeueAfterError{}) { + m.SetConditionHostToFalse( + metal3api.SynchronizedCondition, metal3api.BareMetalHostNotSynchronizedReason, + "Failed to update BareMetalHost") + } + m.Log.Error(err, "Error while patching the BareMetalHost") + return sanitizedErr + } + + // transient rebootAnnotation was successfully transmitted. We can delete it on HostClaim. + delete(m.HostClaim.Annotations, rebootDomain) + + m.updateHostClaimStatus(bmh) + + m.Log.V(1).Info("Finished updating HostClaim") + return nil +} + +// getBmh gets the associated BareMetalHost by looking for the status of the HostClaim. +// Returns ErrNoBMH if the BareMetalHost is not found. +func (m *HostManager) getBmh(ctx context.Context) (*metal3api.BareMetalHost, error) { + hostClaim := m.HostClaim + bmhRef := hostClaim.Status.BareMetalHost + if bmhRef == nil { + return nil, ErrNoBMH + } + + bmh := metal3api.BareMetalHost{} + key := types.NamespacedName{ + Name: bmhRef.Name, + Namespace: bmhRef.Namespace, + } + err := m.client.Get(ctx, key, &bmh) + if k8serrors.IsNotFound(err) { + m.Log.Info("Linked host not found", "bmh", bmhRef.Name, "bmhNamespace", bmhRef.Namespace) + hostClaim.Status.BareMetalHost = nil + return nil, ErrNoBMH + } else if err != nil { + return nil, err + } + if !consumerRefMatches(bmh.Spec.ConsumerRef, hostClaim) { + m.Log.Info("The consumer ref does not point to the hostClaim", "consumerRef", bmh.Spec.ConsumerRef) + hostClaim.Status.BareMetalHost = nil + return nil, ErrNoBMH + } + return &bmh, nil +} + func hideConflictError(err error) error { var aggr kerrors.Aggregate if ok := errors.As(err, &aggr); ok { @@ -167,6 +265,164 @@ func hideConflictError(err error) error { return err } +// setBmhSpec will ensure the host's Spec is set according to the hostclaim's +// details. +func (m *HostManager) setBmhSpec(ctx context.Context, bmh *metal3api.BareMetalHost) (bool, error) { + updated := false + secretManager := secretutils.NewSecretManager(m.Log, m.client, m.APIReader) + ref, err := m.synchronizeDataSecret(ctx, secretManager, bmh, "userdata", m.HostClaim.Spec.UserData, m.HostClaim.Namespace, m.HostClaim.Name) + if err != nil && !errors.Is(err, ErrNoSecret) { + m.SetConditionHostToFalse( + metal3api.SynchronizedCondition, metal3api.BadUserDataSecretReason, err.Error(), + ) + return false, err + } + if referencesDiffer(bmh.Spec.UserData, ref) { + bmh.Spec.UserData = ref + updated = true + } + ref, err = m.synchronizeDataSecret(ctx, secretManager, bmh, "metadata", m.HostClaim.Spec.MetaData, m.HostClaim.Namespace, m.HostClaim.Name) + if err != nil && !errors.Is(err, ErrNoSecret) { + m.SetConditionHostToFalse( + metal3api.SynchronizedCondition, metal3api.BadMetaDataSecretReason, err.Error(), + ) + return false, err + } + if referencesDiffer(bmh.Spec.MetaData, ref) { + bmh.Spec.MetaData = ref + updated = true + } + ref, err = m.synchronizeDataSecret(ctx, secretManager, bmh, "networkdata", m.HostClaim.Spec.NetworkData, m.HostClaim.Namespace, m.HostClaim.Name) + if err != nil && !errors.Is(err, ErrNoSecret) { + m.SetConditionHostToFalse( + metal3api.SynchronizedCondition, metal3api.BadNetworkDataSecretReason, err.Error(), + ) + return false, err + } + if referencesDiffer(bmh.Spec.NetworkData, ref) { + bmh.Spec.NetworkData = ref + updated = true + } + // A host with an existing image is already provisioned and + // upgrades are not supported at this time. To re-provision a + // host, we must fully deprovision it and then provision it again. + if bmh.Spec.Image == nil && m.HostClaim.Spec.Image != nil { + updated = true + bmh.Spec.Image = m.HostClaim.Spec.Image.DeepCopy() + } else if m.HostClaim.Spec.Image == nil { + if bmh.Spec.Image != nil { + updated = true + bmh.Spec.Image = nil + } + } + + // Propagate custom deploy. + if m.HostClaim.Spec.CustomDeploy == nil { + if bmh.Spec.CustomDeploy != nil { + updated = true + bmh.Spec.CustomDeploy = nil + } + } else { + if bmh.Spec.CustomDeploy == nil { + updated = true + bmh.Spec.CustomDeploy = &metal3api.CustomDeploy{Method: m.HostClaim.Spec.CustomDeploy.Method} + } else if bmh.Spec.CustomDeploy.Method != m.HostClaim.Spec.CustomDeploy.Method { + updated = true + bmh.Spec.CustomDeploy.Method = m.HostClaim.Spec.CustomDeploy.Method + } + } + + // Set automatedCleaningMode to disabled as long as the hostclaim exists + if bmh.Spec.AutomatedCleaningMode != metal3api.CleaningModeDisabled { + updated = true + bmh.Spec.AutomatedCleaningMode = metal3api.CleaningModeDisabled + } + if bmh.Spec.Online != m.HostClaim.Spec.PoweredOn { + updated = true + bmh.Spec.Online = m.HostClaim.Spec.PoweredOn + } + + m.SetConditionHostToTrue(metal3api.SynchronizedCondition, metal3api.ConfigurationSyncedReason) + return updated, nil +} + +func referencesDiffer(ref1, ref2 *corev1.SecretReference) bool { + if ref1 == nil { + return ref2 != nil + } + return ref2 == nil || ref1.Name != ref2.Name +} + +func (m *HostManager) synchronizeDataSecret( + ctx context.Context, + secretManager secretutils.SecretManager, + bmh *metal3api.BareMetalHost, + role string, + sourceRef *corev1.SecretReference, + namespace string, + hostName string, +) (*corev1.SecretReference, error) { + if namespace == bmh.Namespace { + return sourceRef.DeepCopy(), nil + } + log := m.Log.WithValues("hostclaimName", hostName, "hostclaimNamespace", namespace, "secret-type", role) + secretName := bmh.Name + "-" + role + if sourceRef == nil { + key := client.ObjectKey{Name: secretName, Namespace: bmh.Namespace} + targetSecret := corev1.Secret{} + err := m.client.Get(ctx, key, &targetSecret) + if err != nil { + if !k8serrors.IsNotFound(err) { + log.Error(err, "Failed to access secret associated to BareMetalHost", "secretName", secretName, "namespace", bmh.Namespace) + return nil, err + } + return nil, ErrNoSecret + } + log.V(1).Info("no configuration secret in hostclaim: destroying in bmh") + err = m.client.Delete(ctx, &targetSecret) + if err != nil { + log.Error(err, "Failed to delete secret associated to BareMetalHost", "secretName", secretName, "namespace", bmh.Namespace) + } + return nil, err + } + // For the same reason as bmh, we ignore namespace value in the ref. + sourceKey := client.ObjectKey{Name: sourceRef.Name, Namespace: namespace} + sourceSecret, err := secretManager.AcquireSecret(ctx, sourceKey, m.HostClaim, false) + if err != nil { + if k8serrors.IsNotFound(err) { + log.Error(err, "Missing source Secret for synchronization from claim to BMH", "source", sourceKey) + return nil, RequeueAfterError{RequeueAfter: HostClaimRequeueDelay} + } + log.Error(err, "Cannot get source Secret for synchronization from claim to BMH", "source", sourceKey) + return nil, err + } + log.V(1).Info("Updating bmh secret with hostclaim secret content") + targetSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: bmh.Namespace, + }, + } + _, err = controllerutil.CreateOrUpdate(ctx, m.client, &targetSecret, + func() error { + if targetSecret.Labels == nil { + targetSecret.Labels = map[string]string{} + } + targetSecret.Labels[secretutils.LabelEnvironmentName] = secretutils.LabelEnvironmentValue + if err = controllerutil.SetOwnerReference(bmh, &targetSecret, m.client.Scheme()); err != nil { + return err + } + targetSecret.Data = sourceSecret.DeepCopy().Data + return nil + }, + ) + if err != nil { + log.Error(err, "cannot copy/update secret", "source", sourceKey, "target", secretName) + return nil, err + } + return &corev1.SecretReference{Name: secretName, Namespace: bmh.Namespace}, nil +} + // consumerRefMatches returns a boolean based on whether the consumer // reference and bareMetalHost metadata match. func consumerRefMatches(consumer *corev1.ObjectReference, claim *metal3api.HostClaim) bool { @@ -435,6 +691,55 @@ func (m *HostManager) chooseBMH(ctx context.Context) (*metal3api.BareMetalHost, return chosenHost, err } +// updateHostClaimStatus updates the status of the HostClaim with information from BareMetalHost. +func (m *HostManager) updateHostClaimStatus(bmh *metal3api.BareMetalHost) { + hostOld := m.HostClaim.Status.DeepCopy() + + // synchronize power status + m.HostClaim.Status.PoweredOn = bmh.Status.PoweredOn + m.HostClaim.Status.HardwareData = &metal3api.ObjectReference{ + Namespace: bmh.Namespace, + Name: bmh.Name, + } + conditions.SetMirrorCondition(bmh, m.HostClaim, metal3api.AvailableForProvisioningCondition) + conditions.SetMirrorCondition(bmh, m.HostClaim, metal3api.ProvisionedCondition) + m.SetConditionHostToTrue(metal3api.AssociatedCondition, metal3api.BareMetalHostAssociatedReason) + + if !equality.Semantic.DeepEqual(m.HostClaim.Status, hostOld) { + m.Log.Info("Status of HostClaim changed") + now := metav1.Now() + m.HostClaim.Status.LastUpdated = &now + } +} + +// synchronize reboot annotations from hostMap to bmhMap. +func syncReboot(hostMap, bmhMap map[string]string) bool { + updated := false + // We propagate first deletion of reboot annotations on BMH + for key := range bmhMap { + elts := strings.Split(key, "/") + // Propagates down unless it is just reboot and already propagated. + if elts[0] == rebootDomain && len(elts) == 2 { + if _, ok := hostMap[key]; !ok { + updated = true + delete(bmhMap, key) + } + } + } + + // We propagate reboot annotations to the bmh when it appears. + for key, v := range hostMap { + elts := strings.Split(key, "/") + if elts[0] == rebootDomain { + if bmhMap[key] != v { + updated = true + bmhMap[key] = v + } + } + } + return updated +} + type Set[T comparable] = map[T]struct{} func NewSet[T comparable]() Set[T] { diff --git a/pkg/hostclaim/hostclaim_manager_test.go b/pkg/hostclaim/hostclaim_manager_test.go index 536e0a3f73..b96c826997 100644 --- a/pkg/hostclaim/hostclaim_manager_test.go +++ b/pkg/hostclaim/hostclaim_manager_test.go @@ -19,6 +19,9 @@ package hostclaim import ( "context" "errors" + "maps" + "reflect" + "strings" "testing" metal3api "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" @@ -29,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -49,6 +53,12 @@ func setupScheme() *runtime.Scheme { } var _ = Describe("HostClaim manager", func() { + var defaultConsumerRef = corev1.ObjectReference{ + Name: HostclaimName, + Namespace: HostclaimNamespace, + Kind: HostClaimKind, + APIVersion: metal3api.GroupVersion.String(), + } var ( defaultImage = metal3api.Image{URL: "url"} @@ -358,6 +368,231 @@ var _ = Describe("HostClaim manager", func() { ExpectRequeue: true, }), ) + + type testCaseSetBMHSpec struct { + UserData *corev1.Secret + NetworkData *corev1.Secret + MetaData *corev1.Secret + BMHUserData *corev1.Secret + BMHNetworkData *corev1.Secret + BMHMetaData *corev1.Secret + SetImage bool + SetCustomDeploy bool + SetPoweredOn bool + } + + DescribeTable("Test setBMHspec", + func(tc testCaseSetBMHSpec) { + hcBuilder := NewHostclaim(HostclaimName) + ctx := context.TODO() + objects := []client.Object{} + numSecrets := 0 + if tc.UserData != nil { + hcBuilder = hcBuilder.SetUserData(tc.UserData.Name) + if !strings.HasPrefix(tc.UserData.Name, "removed") { + objects = append(objects, tc.UserData) + numSecrets++ + } + } + if tc.MetaData != nil { + hcBuilder = hcBuilder.SetMetaData(tc.MetaData.Name) + if !strings.HasPrefix(tc.MetaData.Name, "removed") { + objects = append(objects, tc.MetaData) + numSecrets++ + } + } + if tc.NetworkData != nil { + hcBuilder = hcBuilder.SetNetworkData(tc.NetworkData.Name) + if !strings.HasPrefix(tc.NetworkData.Name, "removed") { + objects = append(objects, tc.NetworkData) + numSecrets++ + } + } + if tc.SetImage { + hcBuilder = hcBuilder.SetImage(defaultImage) + } + if tc.SetCustomDeploy { + hcBuilder = hcBuilder.SetCustomDeploy("custom") + } + hostClaim := hcBuilder.Build() + bmhBuilder := NewBaremetalhost("bmh", "ns", metal3api.StateAvailable) + if tc.BMHUserData != nil { + bmhBuilder = bmhBuilder.SetUserData(tc.BMHUserData.Name) + objects = append(objects, tc.BMHUserData) + } + if tc.BMHMetaData != nil { + bmhBuilder = bmhBuilder.SetMetaData(tc.BMHMetaData.Name) + objects = append(objects, tc.BMHMetaData) + } + if tc.BMHNetworkData != nil { + bmhBuilder = bmhBuilder.SetNetworkData(tc.BMHNetworkData.Name) + objects = append(objects, tc.BMHNetworkData) + } + bmh := bmhBuilder.Build() + objects = append(objects, hostClaim, bmh) + // Add secrets if they exists + fakeClient := fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(objects...).Build() + hostMgr, err := NewHostManager(fakeClient, GinkgoLogr, hostClaim, fakeClient) + Expect(err).NotTo(HaveOccurred()) + updated, err := hostMgr.setBmhSpec(ctx, bmh) + errorExpected := false + var checkSecret = func(ref *corev1.SecretReference, source *corev1.Secret, message string) { + if source == nil { + Expect(ref).To(BeNil(), message) + } else if strings.HasPrefix(source.Name, "removed") { + Expect(ref).To(BeNil(), message) + errorExpected = true + } else { + Expect(ref).NotTo(BeNil(), message) + sec := &corev1.Secret{} + key := client.ObjectKey{Name: ref.Name, Namespace: "ns"} + err = fakeClient.Get(ctx, key, sec) + Expect(err).NotTo(HaveOccurred(), message) + Expect(reflect.DeepEqual(sec.Data, source.Data)).To(BeTrue(), message) + } + } + checkSecret(bmh.Spec.UserData, tc.UserData, "userdata coherence") + checkSecret(bmh.Spec.MetaData, tc.MetaData, "metadata coherence") + checkSecret(bmh.Spec.NetworkData, tc.NetworkData, "networkdata coherence") + if errorExpected { + Expect(err).To(HaveOccurred()) + } else { + Expect(err).NotTo(HaveOccurred()) + } + secrets := &corev1.SecretList{} + err = fakeClient.List(ctx, secrets, client.InNamespace("ns")) + Expect(err).NotTo(HaveOccurred()) + Expect(secrets.Items).To(HaveLen(numSecrets)) + if tc.SetImage { + Expect(bmh.Spec.Image).NotTo(BeNil()) + Expect(*bmh.Spec.Image).To(Equal(defaultImage)) + } + if tc.SetCustomDeploy { + Expect(bmh.Spec.CustomDeploy).NotTo(BeNil()) + Expect(bmh.Spec.CustomDeploy.Method).To(Equal("custom")) + } + if !errorExpected { + Expect(updated).To(BeTrue()) + updated, _ = hostMgr.setBmhSpec(ctx, bmh) + Expect(updated).To(BeFalse()) + } + }, + Entry("set user-data (initialize)", testCaseSetBMHSpec{ + UserData: NewSecret("s1", HostclaimNamespace).SetData(map[string][]byte{"f": []byte("udt")}).Build(), + }), + Entry("set user-data (override)", testCaseSetBMHSpec{ + UserData: NewSecret("s1", HostclaimNamespace).SetData(map[string][]byte{"f": []byte("udt")}).Build(), + BMHUserData: NewSecret("bmh-userdata", "ns").SetData(map[string][]byte{"f": []byte("other")}).Build(), + }), + Entry("reset user-data (override)", testCaseSetBMHSpec{ + BMHUserData: NewSecret("bmh-userdata", "ns").SetData(map[string][]byte{"f": []byte("other")}).Build(), + }), + Entry("set meta-data/network-data (initialize)", testCaseSetBMHSpec{ + MetaData: NewSecret("s1", HostclaimNamespace).SetData(map[string][]byte{"f": []byte("mdt")}).Build(), + NetworkData: NewSecret("s2", HostclaimNamespace).SetData(map[string][]byte{"f": []byte("nwdt")}).Build(), + }), + Entry("set meta-data/network-data (overide)", testCaseSetBMHSpec{ + MetaData: NewSecret("s1", HostclaimNamespace).SetData(map[string][]byte{"f": []byte("mdt")}).Build(), + NetworkData: NewSecret("s2", HostclaimNamespace).SetData(map[string][]byte{"f": []byte("nwdt")}).Build(), + BMHMetaData: NewSecret("bmh-metadata", "ns").SetData(map[string][]byte{"f": []byte("other")}).Build(), + BMHNetworkData: NewSecret("bmh-networkdata", "ns").SetData(map[string][]byte{"f": []byte("other")}).Build(), + }), + Entry("reset meta-data/network-data (overide)", testCaseSetBMHSpec{ + BMHMetaData: NewSecret("bmh-metadata", "ns").SetData(map[string][]byte{"f": []byte("other")}).Build(), + BMHNetworkData: NewSecret("bmh-networkdata", "ns").SetData(map[string][]byte{"f": []byte("other")}).Build(), + }), + Entry("set meta-data (initialize/not yet available)", testCaseSetBMHSpec{ + MetaData: NewSecret("removed-secret", HostclaimNamespace).Build(), + }), + Entry("set image", testCaseSetBMHSpec{ + SetImage: true, + }), + Entry("set custom deploy", testCaseSetBMHSpec{ + SetCustomDeploy: true, + }), + ) + + It("Test updateHostClaimStatus", + func() { + hostClaim := NewHostclaim(HostclaimName).SetAssociatedBMH("ns", "bmh").Build() + bmh := NewBaremetalhost("bmh", "ns", metal3api.StateAvailable). + SetCondition(metal3api.ProvisionedCondition, false, "reason"). + SetCondition(metal3api.AvailableForProvisioningCondition, true, "reason"). + Build() + bmh.Status.PoweredOn = true + fakeClient := fake.NewClientBuilder().WithScheme(setupScheme()).Build() + hostMgr, err := NewHostManager(fakeClient, GinkgoLogr, hostClaim, fakeClient) + Expect(err).NotTo(HaveOccurred()) + hostMgr.updateHostClaimStatus(bmh) + Expect(hostClaim.Status.PoweredOn).To(BeTrue()) + Expect(hostClaim.Status.HardwareData).NotTo(BeNil()) + Expect(hostClaim.Status.HardwareData.Name).To(Equal("bmh")) + Expect(hostClaim.Status.HardwareData.Namespace).To(Equal("ns")) + Expect(conditions.IsFalse(hostClaim, metal3api.ProvisionedCondition)).To(BeTrue()) + Expect(conditions.IsTrue(hostClaim, metal3api.AvailableForProvisioningCondition)).To(BeTrue()) + }) + + It("test syncReboot", + func() { + opt := map[string]string{"a": "w1"} + saved := maps.Clone(opt) + bmh := NewBaremetalhost("bmh", "ns", metal3api.StateAvailable).SetAnnotations(opt).Build() + annot := rebootDomain + "/test" + hostClaim := NewHostclaim(HostclaimName).SetAnnotations(map[string]string{annot: "v0"}).SetAssociatedBMH("ns", "bmh").Build() + b := syncReboot(hostClaim.Annotations, bmh.Annotations) + Expect(bmh.Annotations[annot]).To(Equal("v0"), "reboot propagated") + Expect(b).To(BeTrue(), "change occurred") + b = syncReboot(hostClaim.Annotations, bmh.Annotations) + Expect(b).To(BeFalse(), "idempotent") + // Remove reboot/stop Annotation + delete(hostClaim.Annotations, annot) + b = syncReboot(hostClaim.Annotations, bmh.Annotations) + Expect(maps.Equal(bmh.Annotations, saved)).To(BeTrue(), "removal propagated") + Expect(b).To(BeTrue(), "change occurred") + b = syncReboot(hostClaim.Annotations, bmh.Annotations) + Expect(b).To(BeFalse(), "idempotent") + // Set transient reboot. + hostClaim.Annotations[rebootDomain] = "v1" + b = syncReboot(hostClaim.Annotations, bmh.Annotations) + Expect(b).To(BeTrue(), "transient reboot propagated") + // Check reboot propagated to save + if saved == nil { + saved = map[string]string{} + } + saved[rebootDomain] = "v1" + Expect(maps.Equal(bmh.Annotations, saved)).To(BeTrue()) + }, + ) + + type testCaseUpdate struct { + HostClaim *metal3api.HostClaim + ExpectFail bool + } + DescribeTable("test Update", + func(tc testCaseUpdate) { + ctx := context.TODO() + hc := tc.HostClaim + bmh := NewBaremetalhost("bmh", "ns", metal3api.StateAvailable).SetConsumerRef(defaultConsumerRef).Build() + objects := []client.Object{ + hc, bmh, + NewHostdeploypolicy("hdp", "ns").AcceptNames([]string{HostclaimNamespace}).Build(), + NewNamespace("hcNs").Build(), NewNamespace("ns").Build(), + } + fakeClient := fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(objects...).Build() + hostMgr, err := NewHostManager(fakeClient, GinkgoLogr, hc, fakeClient) + Expect(err).NotTo(HaveOccurred()) + err = hostMgr.Update(ctx) + if tc.ExpectFail { + Expect(err).To(HaveOccurred()) + } else { + Expect(err).NotTo(HaveOccurred()) + } + }, + Entry("Regular case", testCaseUpdate{HostClaim: NewHostclaim(HostclaimName).SetAssociatedBMH("ns", "bmh").Build()}), + Entry("badly associated case", testCaseUpdate{HostClaim: NewHostclaim("other").SetAssociatedBMH("ns", "bmh").Build(), ExpectFail: true}), + Entry("no bmh", testCaseUpdate{HostClaim: NewHostclaim(HostclaimName).SetAssociatedBMH("ns", "other").Build(), ExpectFail: true}), + ) + }) func TestManagers(t *testing.T) { From b53e9dcf5a8deba5a147e7d2e5cbf685dfe63842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20Cr=C3=A9gut?= Date: Mon, 27 Apr 2026 13:56:05 +0200 Subject: [PATCH 2/2] Handling propagation of Detached Annotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a HostClaim has a detached annotation, we propagate it to the BareMetalHost iff there is a HostDeployPolicy compatible with the claim that allows such a propagation. We add a manager flag in the "arguments" of the annotation. HostClaim do not set it to true and if the annotation does not appear in the HostClaim, it is not removed from the BareMetalHost if the manager field is set to true. This way the administrator can still force detachment. Signed-off-by: Pierre Crégut --- .../metal3.io/v1alpha1/baremetalhost_types.go | 4 + .../v1alpha1/hostdeploypolicy_types.go | 3 + .../bases/metal3.io_hostdeploypolicies.yaml | 5 + config/render/capm3.yaml | 5 + internal/testutil/common.go | 5 + pkg/hostclaim/hostclaim_manager.go | 160 +++++++++++++----- pkg/hostclaim/hostclaim_manager_test.go | 61 +++++++ 7 files changed, 201 insertions(+), 42 deletions(-) diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go index 89525d4373..cf530ed013 100644 --- a/apis/metal3.io/v1alpha1/baremetalhost_types.go +++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go @@ -699,6 +699,10 @@ const ( type DetachedAnnotationArguments struct { // DeleteAction indicates the desired delete logic when the detached annotation is present DeleteAction DetachedDeleteAction `json:"deleteAction,omitempty"` + // Manager is an informal flag that tells that the annotation was set by an administrator. + // HostClaim controller will always set it to false and will never delete an annotation with + // manager set to true. + Manager bool `json:"manager,omitempty"` } // Match compares the saved status information with the name and diff --git a/apis/metal3.io/v1alpha1/hostdeploypolicy_types.go b/apis/metal3.io/v1alpha1/hostdeploypolicy_types.go index 8c789a6e90..b66566498a 100644 --- a/apis/metal3.io/v1alpha1/hostdeploypolicy_types.go +++ b/apis/metal3.io/v1alpha1/hostdeploypolicy_types.go @@ -25,6 +25,9 @@ type HostDeployPolicySpec struct { // HostClaimNamespaces constrains the namespaces of the HostClaims allowed // to bind the BareMetalHosts in the same namespace as the HostDeployPolicy HostClaimNamespaces *HostClaimNamespaces `json:"hostClaimNamespaces,omitempty"` + // AllowsDetachedMode is a boolean flag specifying if the hostClaim can set the + // detached annotation of the BareMetalHost. + AllowsDetachedMode bool `json:"allowsDetachedMode,omitempty"` } type HostClaimNamespaces struct { diff --git a/config/base/crds/bases/metal3.io_hostdeploypolicies.yaml b/config/base/crds/bases/metal3.io_hostdeploypolicies.yaml index 4a48fa64eb..9a0bf65b50 100644 --- a/config/base/crds/bases/metal3.io_hostdeploypolicies.yaml +++ b/config/base/crds/bases/metal3.io_hostdeploypolicies.yaml @@ -39,6 +39,11 @@ spec: spec: description: HostDeployPolicySpec defines the desired state of HostDeployPolicy. properties: + allowsDetachedMode: + description: |- + AllowsDetachedMode is a boolean flag specifying if the hostClaim can set the + detached annotation of the BareMetalHost. + type: boolean hostClaimNamespaces: description: |- HostClaimNamespaces constrains the namespaces of the HostClaims allowed diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml index 06dbf1dea1..5d334ad6ae 100644 --- a/config/render/capm3.yaml +++ b/config/render/capm3.yaml @@ -2248,6 +2248,11 @@ spec: spec: description: HostDeployPolicySpec defines the desired state of HostDeployPolicy. properties: + allowsDetachedMode: + description: |- + AllowsDetachedMode is a boolean flag specifying if the hostClaim can set the + detached annotation of the BareMetalHost. + type: boolean hostClaimNamespaces: description: |- HostClaimNamespaces constrains the namespaces of the HostClaims allowed diff --git a/internal/testutil/common.go b/internal/testutil/common.go index 21ee251a5c..e889641260 100644 --- a/internal/testutil/common.go +++ b/internal/testutil/common.go @@ -360,3 +360,8 @@ func (hb *HostDeployPolicyBuilder) AcceptRegexp(re string) *HostDeployPolicyBuil spec.HostClaimNamespaces.NameMatches = re return hb } + +func (hb *HostDeployPolicyBuilder) AllowsDetachedMode() *HostDeployPolicyBuilder { + hb.hostDeployPolicy.Spec.AllowsDetachedMode = true + return hb +} diff --git a/pkg/hostclaim/hostclaim_manager.go b/pkg/hostclaim/hostclaim_manager.go index 8329491fae..45379c34d3 100644 --- a/pkg/hostclaim/hostclaim_manager.go +++ b/pkg/hostclaim/hostclaim_manager.go @@ -19,6 +19,7 @@ package hostclaim import ( "context" "crypto/rand" + "encoding/json" "errors" "math/big" "regexp" @@ -200,6 +201,11 @@ func (m *HostManager) Update(ctx context.Context) error { if syncReboot(m.HostClaim.Annotations, bmh.Annotations) { updated = true } + if upd, err2 := m.syncDetached(ctx, bmh.Namespace, m.HostClaim.Annotations, bmh.Annotations); err2 != nil { + return err2 + } else if upd { + updated = true + } if updated { m.Log.Info("Update the BareMetalHost spec: changes detected.") err = m.client.Update(ctx, bmh) @@ -535,56 +541,59 @@ func (m *HostManager) acceptableNamespaces(ctx context.Context, namespace string nsLabels = map[string]string{} } namespaces := NewSet[string]() -LOOP_POLICY: for _, hostDeployPolicy := range hostdeploypolicies.Items { - log := m.Log.WithValues("policyNamespace", hostDeployPolicy.Namespace, "policyName", hostDeployPolicy.Name) - if SetContains(namespaces, hostDeployPolicy.Namespace) { - // namespace already added, no reason to check this hdp. - continue + if accept, err := m.checkPolicy(&hostDeployPolicy, hostNs, nsLabels); err != nil { + return nil, err + } else if accept { + AddSet(namespaces, hostDeployPolicy.Namespace) } - constraints := hostDeployPolicy.Spec.HostClaimNamespaces - if constraints == nil { - log.V(1).Info("Ignoring HostDeployPolicy without constraint") - continue + } + m.Log.Info("Acceptable namespaces", "namespaces", namespaces) + return namespaces, nil +} + +func (m *HostManager) checkPolicy(hostDeployPolicy *metal3api.HostDeployPolicy, hostNs string, hostNsLabels map[string]string) (bool, error) { + log := m.Log.WithValues("policyNamespace", hostDeployPolicy.Namespace, "policyName", hostDeployPolicy.Name) + constraints := hostDeployPolicy.Spec.HostClaimNamespaces + if constraints == nil { + m.Log.V(1).Info("Ignoring HostDeployPolicy without constraint") + return false, nil + } + if constraints.Names != nil && !slices.Contains(constraints.Names, hostNs) { + log.V(1).Info("Ignoring HostDeployPolicy because claim namespace not in names", "names", constraints.Names) + return false, nil + } + if constraints.NameMatches != "" { + b, err := regexp.MatchString(constraints.NameMatches, hostNs) + if err != nil { + log.Error( + err, "Error during regexp matching on HostClaim namespace (bad regexp)", + "regexp", constraints.NameMatches) + return false, err } - if constraints.Names != nil && !slices.Contains(constraints.Names, hostNs) { - log.V(1).Info("Ignoring HostDeployPolicy because claim namespace not in names", "names", constraints.Names) - continue + if !b { + log.V(1).Info("Ignoring HostDeployPolicy because claim namespace does not match regex") + return false, nil } - if constraints.NameMatches != "" { - b, err := regexp.MatchString(constraints.NameMatches, hostNs) - if err != nil { - log.Error( - err, "Error during regexp matching on HostClaim namespace (bad regexp)", - "regexp", constraints.NameMatches) - return nil, err - } - if !b { - log.V(1).Info("Ignoring HostDeployPolicy because claim namespace does not match regex") - continue - } - } - // Behaves as a 'forall' on labels - for _, pair := range constraints.HasLabels { - if v, ok := nsLabels[pair.Name]; ok { - if pair.Value != "" && v != pair.Value { - log.V(1).Info( - "Ignoring HostDeployPolicy because claim namespace labels does not have correct value", - "label", pair.Name, "value", v, "expected", pair.Value) - continue LOOP_POLICY - } - } else { + } + // Behaves as a 'forall' on labels + for _, pair := range constraints.HasLabels { + if v, ok := hostNsLabels[pair.Name]; ok { + if pair.Value != "" && v != pair.Value { log.V(1).Info( - "Ignoring HostDeployPolicy because claim namespace labels does not have a required label", - "label", pair.Name) - continue LOOP_POLICY + "Ignoring HostDeployPolicy because claim namespace labels does not have correct value", + "label", pair.Name, "value", v, "expected", pair.Value) + return false, nil } + } else { + log.V(1).Info( + "Ignoring HostDeployPolicy because claim namespace labels does not have a required label", + "label", pair.Name) + return false, nil } - log.V(1).Info("Accepting namespace because of HostDeployPolicy", "namespace", hostDeployPolicy.Namespace) - AddSet(namespaces, hostDeployPolicy.Namespace) } - m.Log.Info("Acceptable namespaces", "namespaces", namespaces) - return namespaces, nil + log.V(1).Info("Accepting namespace because of HostDeployPolicy", "namespace", hostDeployPolicy.Namespace) + return true, nil } func (m *HostManager) hostLabelSelectorForHostClaim() (labels.Selector, error) { @@ -740,6 +749,43 @@ func syncReboot(hostMap, bmhMap map[string]string) bool { return updated } +// synchronize detached annotation from hostMap to bmhMap. +func (m *HostManager) syncDetached(ctx context.Context, bmhNs string, hostMap, bmhMap map[string]string) (bool, error) { + if annotValue, ok := hostMap[metal3api.DetachedAnnotation]; ok { + if _, ok := bmhMap[metal3api.DetachedAnnotation]; ok { + return false, nil + } + if compatiblePolicies, err := m.GetCompatiblePolicies(ctx, bmhNs); err != nil { + return false, err + } else if !slices.ContainsFunc( + compatiblePolicies, + func(hdp *metal3api.HostDeployPolicy) bool { + return hdp.Spec.AllowsDetachedMode + }, + ) { + m.Log.Info("HostClaim not allowed to detach underlying BareMetalHost") + return false, err + } + inputArgs := metal3api.DetachedAnnotationArguments{} + _ = json.Unmarshal([]byte(annotValue), &inputArgs) + args := metal3api.DetachedAnnotationArguments{DeleteAction: inputArgs.DeleteAction} + annot, err := json.Marshal(args) + if err != nil { + return false, err + } + bmhMap[metal3api.DetachedAnnotation] = string(annot) + return true, nil + } else if annotValue, ok := bmhMap[metal3api.DetachedAnnotation]; ok { + args := metal3api.DetachedAnnotationArguments{} + if err := json.Unmarshal([]byte(annotValue), &args); err != nil || args.Manager { + return false, nil //nolint:nilerr // arbitrary value of annotation mean they are not handled by hostclaim + } + delete(bmhMap, metal3api.DetachedAnnotation) + return true, nil + } + return false, nil +} + type Set[T comparable] = map[T]struct{} func NewSet[T comparable]() Set[T] { @@ -754,3 +800,33 @@ func SetContains[T comparable](m Set[T], s T) bool { _, ok := m[s] return ok } + +// GetCompatiblePolicies find all the HostDeployPolicies in the namespace of the bareMetalHost that +// accept the namespace of the current HostClaim. +func (m *HostManager) GetCompatiblePolicies(ctx context.Context, bmhNs string) ([]*metal3api.HostDeployPolicy, error) { + policiesInNs := &metal3api.HostDeployPolicyList{} + if err := m.client.List(ctx, policiesInNs, client.InNamespace(bmhNs)); err != nil { + m.Log.Error(err, "Cannot access HostDeployPolicies", "bmhNamespace", bmhNs) + return nil, err + } + hostNs := m.HostClaim.Namespace + hostNsResource := &corev1.Namespace{} + if err := m.client.Get(ctx, client.ObjectKey{Name: hostNs}, hostNsResource); err != nil { + m.Log.Error(err, "cannot access the namespace of the claim") + return nil, err + } + nsLabels := hostNsResource.Labels + if nsLabels == nil { + nsLabels = map[string]string{} + } + + var compatiblePolicies []*metal3api.HostDeployPolicy + for _, hdp := range policiesInNs.Items { + if accept, err := m.checkPolicy(&hdp, hostNs, nsLabels); err != nil { + return nil, err + } else if accept { + compatiblePolicies = append(compatiblePolicies, &hdp) + } + } + return compatiblePolicies, nil +} diff --git a/pkg/hostclaim/hostclaim_manager_test.go b/pkg/hostclaim/hostclaim_manager_test.go index b96c826997..dc4a1b69c1 100644 --- a/pkg/hostclaim/hostclaim_manager_test.go +++ b/pkg/hostclaim/hostclaim_manager_test.go @@ -564,6 +564,67 @@ var _ = Describe("HostClaim manager", func() { }, ) + type testCaseSyncDetach struct { + AcceptHostClaimNs string + InHostClaimAnnot string + InBmhAnnot string + OutHostClaimAnnot string + OutBmhAnnot string + Updated bool + } + + DescribeTable("test syncDetach", + func(tc testCaseSyncDetach) { + ctx := context.TODO() + ns := NewNamespace(HostclaimNamespace).Build() + + annotHc := map[string]string{} + annotBmh := map[string]string{} + if tc.InHostClaimAnnot != "" { + annotHc = map[string]string{metal3api.DetachedAnnotation: tc.InHostClaimAnnot} + } + if tc.InBmhAnnot != "" { + annotBmh = map[string]string{metal3api.DetachedAnnotation: tc.InBmhAnnot} + } + + bmh := NewBaremetalhost("bmh", "ns", metal3api.StateAvailable).SetAnnotations(annotBmh).SetConsumerRef(defaultConsumerRef).Build() + hostClaim := NewHostclaim(HostclaimName).SetAssociatedBMH("ns", "bmh").SetAnnotations(annotHc).Build() + hdp := NewHostdeploypolicy("hdp", "ns").AcceptNames([]string{tc.AcceptHostClaimNs}).AllowsDetachedMode().Build() + fakeClient := fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(bmh, hostClaim, ns, hdp).Build() + hostMgr, err := NewHostManager(fakeClient, GinkgoLogr, hostClaim, fakeClient) + Expect(err).NotTo(HaveOccurred()) + updated, err := hostMgr.syncDetached(ctx, "ns", hostClaim.Annotations, bmh.Annotations) + Expect(err).NotTo(HaveOccurred()) + Expect(updated).To(Equal(tc.Updated)) + outBmhAnnot := "" + if bmh.Annotations != nil { + outBmhAnnot = bmh.Annotations[metal3api.DetachedAnnotation] + } + Expect(outBmhAnnot).To(Equal(tc.OutBmhAnnot)) + }, + Entry("not allowed", testCaseSyncDetach{AcceptHostClaimNs: "other", InHostClaimAnnot: "{}", OutBmhAnnot: ""}), + Entry("allowed", + testCaseSyncDetach{ + AcceptHostClaimNs: HostclaimNamespace, InHostClaimAnnot: "{}", OutBmhAnnot: "{}", Updated: true}), + Entry("sanitize", + testCaseSyncDetach{ + AcceptHostClaimNs: HostclaimNamespace, + InHostClaimAnnot: "{\"deleteAction\": \"delay\", \"manager\": true}", + OutBmhAnnot: "{\"deleteAction\":\"delay\"}", + Updated: true}), + Entry("no override", + testCaseSyncDetach{AcceptHostClaimNs: HostclaimNamespace, InHostClaimAnnot: "{}", + InBmhAnnot: "zzz", OutBmhAnnot: "zzz"}), + Entry("delete", + testCaseSyncDetach{ + AcceptHostClaimNs: HostclaimNamespace, InHostClaimAnnot: "", + InBmhAnnot: "{}", OutBmhAnnot: "", Updated: true}), + Entry("no delete", + testCaseSyncDetach{ + AcceptHostClaimNs: HostclaimNamespace, InHostClaimAnnot: "", + InBmhAnnot: "{\"manager\": true}", OutBmhAnnot: "{\"manager\": true}"}), + ) + type testCaseUpdate struct { HostClaim *metal3api.HostClaim ExpectFail bool