diff --git a/internal/controller/externalip_feedback_controller.go b/internal/controller/externalip_feedback_controller.go index 8a413a3d..e692e65c 100644 --- a/internal/controller/externalip_feedback_controller.go +++ b/internal/controller/externalip_feedback_controller.go @@ -24,36 +24,70 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" clnt "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ctrllog "sigs.k8s.io/controller-runtime/pkg/log" mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" "github.com/osac-project/osac-operator/api/v1alpha1" privatev1 "github.com/osac-project/osac-operator/internal/api/osac/private/v1" + "github.com/osac-project/osac-operator/internal/controller/feedback" ) var ErrExternalIPNotFound = errors.New("external IP not found in fulfillment service") // ExternalIPFeedbackReconciler sends updates to the fulfillment service. type ExternalIPFeedbackReconciler struct { - hubClient clnt.Client - externalIPsClient privatev1.ExternalIPsClient + bridge *feedback.Bridge[*v1alpha1.ExternalIP, *privatev1.ExternalIP] networkingNamespace string } -type externalIPFeedbackReconcilerTask struct { - r *ExternalIPFeedbackReconciler - object *v1alpha1.ExternalIP - externalIP *privatev1.ExternalIP -} - // NewExternalIPFeedbackReconciler creates a reconciler that sends to the fulfillment service updates about external IPs. func NewExternalIPFeedbackReconciler(hubClient clnt.Client, grpcConn *grpc.ClientConn, networkingNamespace string) *ExternalIPFeedbackReconciler { - return &ExternalIPFeedbackReconciler{ - hubClient: hubClient, - externalIPsClient: privatev1.NewExternalIPsClient(grpcConn), - networkingNamespace: networkingNamespace, + eipClient := privatev1.NewExternalIPsClient(grpcConn) + r := &ExternalIPFeedbackReconciler{networkingNamespace: networkingNamespace} + r.bridge = &feedback.Bridge[*v1alpha1.ExternalIP, *privatev1.ExternalIP]{ + Client: hubClient, + Finalizer: osacExternalIPFeedbackFinalizer, + IDLabel: osacExternalIPIDLabel, + Kind: "ExternalIP", + IDKey: "externalIPID", + NewObject: func() *v1alpha1.ExternalIP { return &v1alpha1.ExternalIP{} }, + Fetch: func(ctx context.Context, id string) (*privatev1.ExternalIP, error) { + response, err := eipClient.Get(ctx, privatev1.ExternalIPsGetRequest_builder{Id: id}.Build()) + if err != nil { + if status.Code(err) == codes.NotFound { + return nil, fmt.Errorf("%w: %w", ErrExternalIPNotFound, err) + } + return nil, err + } + eip := response.GetObject() + if eip == nil { + return nil, fmt.Errorf("%w: response contained nil object", ErrExternalIPNotFound) + } + if !eip.HasSpec() { + eip.SetSpec(&privatev1.ExternalIPSpec{}) + } + if !eip.HasStatus() { + eip.SetStatus(&privatev1.ExternalIPStatus{}) + } + return eip, nil + }, + Save: func(ctx context.Context, remote *privatev1.ExternalIP) error { + _, err := eipClient.Update(ctx, privatev1.ExternalIPsUpdateRequest_builder{ + Object: remote, + }.Build()) + return err + }, + Signal: func(ctx context.Context, id string) error { + _, err := eipClient.Signal(ctx, privatev1.ExternalIPsSignalRequest_builder{ + Id: id, + }.Build()) + return err + }, + SyncUpdate: syncExternalIPUpdate, + SyncDelete: syncExternalIPDelete, + IsNotFound: func(err error) bool { return errors.Is(err, ErrExternalIPNotFound) }, } + return r } // SetupWithManager adds the reconciler to the controller manager. @@ -69,178 +103,42 @@ func (r *ExternalIPFeedbackReconciler) SetupWithManager(mgr mcmanager.Manager) e Complete(r) } -// Reconcile is the implementation of the reconciler interface. +// Reconcile delegates to the shared feedback Bridge. func (r *ExternalIPFeedbackReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { - log := ctrllog.FromContext(ctx) - - // Step 1: Fetch the object to reconcile, and do nothing if it no longer exists: - object := &v1alpha1.ExternalIP{} - if err := r.hubClient.Get(ctx, request.NamespacedName, object); err != nil { - return ctrl.Result{}, clnt.IgnoreNotFound(err) - } - - // Step 2: Get the identifier of the external IP from the labels. If this isn't present it means that the object - // wasn't created by the fulfillment service, so we ignore it. - externalIPID, ok := object.Labels[osacExternalIPIDLabel] - if !ok { - if !object.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(object, osacExternalIPFeedbackFinalizer) { - log.Info("CR without external IP ID label is being deleted, removing feedback finalizer") - if controllerutil.RemoveFinalizer(object, osacExternalIPFeedbackFinalizer) { - return ctrl.Result{}, r.hubClient.Update(ctx, object) - } - } - log.Info( - "There is no label containing the external IP identifier, will ignore it", - "label", osacExternalIPIDLabel, - ) - return ctrl.Result{}, nil - } - - // Step 3: Fetch the external IP from the fulfillment service so we can compare before/after. - externalIP, err := r.fetchExternalIP(ctx, externalIPID) - if err != nil { - if !object.DeletionTimestamp.IsZero() && errors.Is(err, ErrExternalIPNotFound) { - log.Info("ExternalIP record not found during deletion, removing feedback finalizer", "externalIPID", externalIPID) - if controllerutil.RemoveFinalizer(object, osacExternalIPFeedbackFinalizer) { - return ctrl.Result{}, r.hubClient.Update(ctx, object) - } - return ctrl.Result{}, nil - } - return ctrl.Result{}, err - } - - // Create a task to do the rest of the job, but using copies of the objects, so that we can later compare the - // before and after values and save only the objects that have changed. - t := &externalIPFeedbackReconcilerTask{ - r: r, - object: object, - externalIP: clone(externalIP), - } - - // Step 4: Sync CR state to the fulfillment service record. - if object.DeletionTimestamp.IsZero() { - if err := t.handleUpdate(ctx); err != nil { - return ctrl.Result{}, err - } - } else { - t.handleDelete() - } - - // Step 5: Persist synced state to the fulfillment service. - if err := r.saveExternalIP(ctx, externalIP, t.externalIP); err != nil { - return ctrl.Result{}, err - } - - // Step 6: Handle finalizer removal and signal for deletions. - if !object.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(object, osacExternalIPFeedbackFinalizer) { - if len(object.GetFinalizers()) == 1 { - log.Info( - "Feedback finalizer is last remaining, removing finalizer and signaling", - "externalIPID", externalIPID, - ) - if controllerutil.RemoveFinalizer(object, osacExternalIPFeedbackFinalizer) { - if err := r.hubClient.Update(ctx, object); err != nil { - return ctrl.Result{}, err - } - } - _, signalErr := r.externalIPsClient.Signal(ctx, privatev1.ExternalIPsSignalRequest_builder{ - Id: externalIPID, - }.Build()) - if signalErr != nil { - log.Error( - signalErr, - "Failed to signal fulfillment service, periodic sync will handle cleanup", - "externalIPID", externalIPID, - ) - } - } else { - log.Info( - "Other finalizers still present, waiting", - "finalizers", object.GetFinalizers(), - ) - } - } - - return ctrl.Result{}, nil -} - -func (r *ExternalIPFeedbackReconciler) fetchExternalIP(ctx context.Context, id string) (*privatev1.ExternalIP, error) { - response, err := r.externalIPsClient.Get(ctx, privatev1.ExternalIPsGetRequest_builder{ - Id: id, - }.Build()) - if err != nil { - if status.Code(err) == codes.NotFound { - return nil, fmt.Errorf("%w: %w", ErrExternalIPNotFound, err) - } - return nil, err - } - externalIP := response.GetObject() - if externalIP == nil { - return nil, fmt.Errorf("%w: response contained nil object", ErrExternalIPNotFound) - } - if !externalIP.HasSpec() { - externalIP.SetSpec(&privatev1.ExternalIPSpec{}) - } - if !externalIP.HasStatus() { - externalIP.SetStatus(&privatev1.ExternalIPStatus{}) - } - return externalIP, nil + return r.bridge.Reconcile(ctx, request) } -func (r *ExternalIPFeedbackReconciler) saveExternalIP(ctx context.Context, before, after *privatev1.ExternalIP) error { - log := ctrllog.FromContext(ctx) - - if !equal(after, before) { - log.Info( - "Updating external IP", - "before", before, - "after", after, - ) - _, err := r.externalIPsClient.Update(ctx, privatev1.ExternalIPsUpdateRequest_builder{ - Object: after, - }.Build()) - if err != nil { - return err - } - } +func syncExternalIPUpdate(ctx context.Context, obj *v1alpha1.ExternalIP, remote *privatev1.ExternalIP) error { + syncExternalIPState(ctx, obj, remote) + syncExternalIPAddress(obj, remote) return nil } -func (t *externalIPFeedbackReconcilerTask) handleUpdate(ctx context.Context) error { - if controllerutil.AddFinalizer(t.object, osacExternalIPFeedbackFinalizer) { - if err := t.r.hubClient.Update(ctx, t.object); err != nil { - return err - } +func syncExternalIPDelete(_ context.Context, obj *v1alpha1.ExternalIP, remote *privatev1.ExternalIP) error { + if obj.Status.State == v1alpha1.ExternalIPStateFailed { + remote.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_FAILED) + return nil } - t.syncState(ctx) - t.syncAddress() + remote.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_DELETING) return nil } -func (t *externalIPFeedbackReconcilerTask) handleDelete() { - if t.object.Status.State == v1alpha1.ExternalIPStateFailed { - t.externalIP.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_FAILED) - return - } - t.externalIP.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_DELETING) -} - -func (t *externalIPFeedbackReconcilerTask) syncState(ctx context.Context) { - switch t.object.Status.State { +func syncExternalIPState(ctx context.Context, obj *v1alpha1.ExternalIP, remote *privatev1.ExternalIP) { + switch obj.Status.State { case v1alpha1.ExternalIPStatePending: - t.externalIP.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_PENDING) + remote.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_PENDING) case v1alpha1.ExternalIPStateAllocated: - t.externalIP.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_ALLOCATED) + remote.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_ALLOCATED) case v1alpha1.ExternalIPStateFailed: - t.externalIP.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_FAILED) + remote.GetStatus().SetState(privatev1.ExternalIPState_EXTERNAL_IP_STATE_FAILED) default: log := ctrllog.FromContext(ctx) - log.Info("Unknown state, will ignore it", "state", t.object.Status.State) + log.Info("Unknown state, will ignore it", "state", obj.Status.State) } } -func (t *externalIPFeedbackReconcilerTask) syncAddress() { - if t.object.Status.Address != "" { - t.externalIP.GetStatus().SetAddress(t.object.Status.Address) +func syncExternalIPAddress(obj *v1alpha1.ExternalIP, remote *privatev1.ExternalIP) { + if obj.Status.Address != "" { + remote.GetStatus().SetAddress(obj.Status.Address) } } diff --git a/internal/controller/externalipattachment_feedback_controller.go b/internal/controller/externalipattachment_feedback_controller.go index 94d4de2a..137d8a42 100644 --- a/internal/controller/externalipattachment_feedback_controller.go +++ b/internal/controller/externalipattachment_feedback_controller.go @@ -24,36 +24,70 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" clnt "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ctrllog "sigs.k8s.io/controller-runtime/pkg/log" mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" "github.com/osac-project/osac-operator/api/v1alpha1" privatev1 "github.com/osac-project/osac-operator/internal/api/osac/private/v1" + "github.com/osac-project/osac-operator/internal/controller/feedback" ) var ErrExternalIPAttachmentNotFound = errors.New("external IP attachment not found in fulfillment service") type ExternalIPAttachmentFeedbackReconciler struct { - hubClient clnt.Client - externalIPAttachmentsClient privatev1.ExternalIPAttachmentsClient - externalIPsClient privatev1.ExternalIPsClient - networkingNamespace string -} - -type externalIPAttachmentFeedbackReconcilerTask struct { - r *ExternalIPAttachmentFeedbackReconciler - object *v1alpha1.ExternalIPAttachment - externalIPAttachment *privatev1.ExternalIPAttachment + bridge *feedback.Bridge[*v1alpha1.ExternalIPAttachment, *privatev1.ExternalIPAttachment] + networkingNamespace string } func NewExternalIPAttachmentFeedbackReconciler(hubClient clnt.Client, grpcConn *grpc.ClientConn, networkingNamespace string) *ExternalIPAttachmentFeedbackReconciler { - return &ExternalIPAttachmentFeedbackReconciler{ - hubClient: hubClient, - externalIPAttachmentsClient: privatev1.NewExternalIPAttachmentsClient(grpcConn), - externalIPsClient: privatev1.NewExternalIPsClient(grpcConn), - networkingNamespace: networkingNamespace, + attachClient := privatev1.NewExternalIPAttachmentsClient(grpcConn) + eipClient := privatev1.NewExternalIPsClient(grpcConn) + r := &ExternalIPAttachmentFeedbackReconciler{networkingNamespace: networkingNamespace} + r.bridge = &feedback.Bridge[*v1alpha1.ExternalIPAttachment, *privatev1.ExternalIPAttachment]{ + Client: hubClient, + Finalizer: osacExternalIPAttachmentFeedbackFinalizer, + IDLabel: osacExternalIPAttachmentIDLabel, + Kind: "ExternalIPAttachment", + IDKey: "attachmentID", + NewObject: func() *v1alpha1.ExternalIPAttachment { return &v1alpha1.ExternalIPAttachment{} }, + Fetch: func(ctx context.Context, id string) (*privatev1.ExternalIPAttachment, error) { + response, err := attachClient.Get(ctx, privatev1.ExternalIPAttachmentsGetRequest_builder{Id: id}.Build()) + if err != nil { + if status.Code(err) == codes.NotFound { + return nil, fmt.Errorf("%w: %w", ErrExternalIPAttachmentNotFound, err) + } + return nil, err + } + obj := response.GetObject() + if obj == nil { + return nil, fmt.Errorf("%w: response contained nil object", ErrExternalIPAttachmentNotFound) + } + if !obj.HasSpec() { + obj.SetSpec(&privatev1.ExternalIPAttachmentSpec{}) + } + if !obj.HasStatus() { + obj.SetStatus(&privatev1.ExternalIPAttachmentStatus{}) + } + return obj, nil + }, + Save: func(ctx context.Context, remote *privatev1.ExternalIPAttachment) error { + _, err := attachClient.Update(ctx, privatev1.ExternalIPAttachmentsUpdateRequest_builder{ + Object: remote, + }.Build()) + return err + }, + Signal: func(ctx context.Context, id string) error { + _, err := attachClient.Signal(ctx, privatev1.ExternalIPAttachmentsSignalRequest_builder{ + Id: id, + }.Build()) + return err + }, + SyncUpdate: newExternalIPAttachmentSyncUpdate(eipClient), + SyncDelete: syncExternalIPAttachmentDelete, + PostSaveOnDelete: newExternalIPAttachmentPostSaveOnDelete(eipClient), + IsNotFound: func(err error) bool { return errors.Is(err, ErrExternalIPAttachmentNotFound) }, } + return r } func (r *ExternalIPAttachmentFeedbackReconciler) SetupWithManager(mgr mcmanager.Manager) error { @@ -68,187 +102,71 @@ func (r *ExternalIPAttachmentFeedbackReconciler) SetupWithManager(mgr mcmanager. Complete(r) } +// Reconcile delegates to the shared feedback Bridge. func (r *ExternalIPAttachmentFeedbackReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { - log := ctrllog.FromContext(ctx) - - object := &v1alpha1.ExternalIPAttachment{} - if err := r.hubClient.Get(ctx, request.NamespacedName, object); err != nil { - return ctrl.Result{}, clnt.IgnoreNotFound(err) - } - - attachmentID, ok := object.Labels[osacExternalIPAttachmentIDLabel] - if !ok { - if !object.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(object, osacExternalIPAttachmentFeedbackFinalizer) { - log.Info("CR without external IP attachment ID label is being deleted, removing feedback finalizer") - if controllerutil.RemoveFinalizer(object, osacExternalIPAttachmentFeedbackFinalizer) { - return ctrl.Result{}, r.hubClient.Update(ctx, object) - } - } - log.Info( - "There is no label containing the external IP attachment identifier, will ignore it", - "label", osacExternalIPAttachmentIDLabel, - ) - return ctrl.Result{}, nil - } - - externalIPAttachment, err := r.fetchExternalIPAttachment(ctx, attachmentID) - if err != nil { - if !object.DeletionTimestamp.IsZero() && errors.Is(err, ErrExternalIPAttachmentNotFound) { - log.Info("ExternalIPAttachment record not found during deletion, removing feedback finalizer", "attachmentID", attachmentID) - if controllerutil.RemoveFinalizer(object, osacExternalIPAttachmentFeedbackFinalizer) { - return ctrl.Result{}, r.hubClient.Update(ctx, object) - } - return ctrl.Result{}, nil - } - return ctrl.Result{}, err - } - - t := &externalIPAttachmentFeedbackReconcilerTask{ - r: r, - object: object, - externalIPAttachment: clone(externalIPAttachment), - } - - if object.DeletionTimestamp.IsZero() { - if err := t.handleUpdate(ctx); err != nil { - return ctrl.Result{}, err - } - } else { - t.handleDelete() - } - - if err := r.saveExternalIPAttachment(ctx, externalIPAttachment, t.externalIPAttachment); err != nil { - return ctrl.Result{}, err - } - - if !object.DeletionTimestamp.IsZero() { - if err := t.syncAttachedOnParentExternalIP(ctx, false); err != nil { - log.Error(err, "Failed to clear attached on parent ExternalIP, will retry") - return ctrl.Result{}, err - } - } - - if !object.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(object, osacExternalIPAttachmentFeedbackFinalizer) { - if len(object.GetFinalizers()) == 1 { - log.Info( - "Feedback finalizer is last remaining, removing finalizer and signaling", - "attachmentID", attachmentID, - ) - if controllerutil.RemoveFinalizer(object, osacExternalIPAttachmentFeedbackFinalizer) { - if err := r.hubClient.Update(ctx, object); err != nil { - return ctrl.Result{}, err - } - } - _, signalErr := r.externalIPAttachmentsClient.Signal(ctx, privatev1.ExternalIPAttachmentsSignalRequest_builder{ - Id: attachmentID, - }.Build()) - if signalErr != nil { - log.Error( - signalErr, - "Failed to signal fulfillment service, periodic sync will handle cleanup", - "attachmentID", attachmentID, - ) - } - } else { - log.Info( - "Other finalizers still present, waiting", - "finalizers", object.GetFinalizers(), - ) - } - } - - return ctrl.Result{}, nil + return r.bridge.Reconcile(ctx, request) } -func (r *ExternalIPAttachmentFeedbackReconciler) fetchExternalIPAttachment(ctx context.Context, id string) (*privatev1.ExternalIPAttachment, error) { - response, err := r.externalIPAttachmentsClient.Get(ctx, privatev1.ExternalIPAttachmentsGetRequest_builder{ - Id: id, - }.Build()) - if err != nil { - if status.Code(err) == codes.NotFound { - return nil, fmt.Errorf("%w: %w", ErrExternalIPAttachmentNotFound, err) +// newExternalIPAttachmentSyncUpdate returns a SyncUpdate that captures eipClient +// for setting attached=true on the parent ExternalIP when Ready, and for syncing +// the parent's address to the attachment. +func newExternalIPAttachmentSyncUpdate(eipClient privatev1.ExternalIPsClient) func(context.Context, *v1alpha1.ExternalIPAttachment, *privatev1.ExternalIPAttachment) error { + return func(ctx context.Context, obj *v1alpha1.ExternalIPAttachment, remote *privatev1.ExternalIPAttachment) error { + syncExternalIPAttachmentState(ctx, obj, remote) + syncExternalIPAttachmentAddress(ctx, eipClient, remote) + + if obj.Status.Phase == v1alpha1.ExternalIPAttachmentPhaseReady { + if err := syncAttachedOnParentExternalIP(ctx, eipClient, remote, true); err != nil { + ctrllog.FromContext(ctx).Error(err, "Failed to set attached on parent ExternalIP, will retry") + return err + } } - return nil, err - } - obj := response.GetObject() - if obj == nil { - return nil, fmt.Errorf("%w: response contained nil object", ErrExternalIPAttachmentNotFound) - } - if !obj.HasSpec() { - obj.SetSpec(&privatev1.ExternalIPAttachmentSpec{}) - } - if !obj.HasStatus() { - obj.SetStatus(&privatev1.ExternalIPAttachmentStatus{}) + return nil } - return obj, nil } -func (r *ExternalIPAttachmentFeedbackReconciler) saveExternalIPAttachment(ctx context.Context, before, after *privatev1.ExternalIPAttachment) error { - log := ctrllog.FromContext(ctx) - - if !equal(after, before) { - log.Info( - "Updating external IP attachment", - "before", before, - "after", after, - ) - _, err := r.externalIPAttachmentsClient.Update(ctx, privatev1.ExternalIPAttachmentsUpdateRequest_builder{ - Object: after, - }.Build()) - if err != nil { - return err - } +func syncExternalIPAttachmentDelete(_ context.Context, obj *v1alpha1.ExternalIPAttachment, remote *privatev1.ExternalIPAttachment) error { + if obj.Status.Phase == v1alpha1.ExternalIPAttachmentPhaseFailed { + remote.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_FAILED) + return nil } + remote.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_DELETING) return nil } -func (t *externalIPAttachmentFeedbackReconcilerTask) handleUpdate(ctx context.Context) error { - if controllerutil.AddFinalizer(t.object, osacExternalIPAttachmentFeedbackFinalizer) { - if err := t.r.hubClient.Update(ctx, t.object); err != nil { - return err - } - } - t.syncState(ctx) - t.syncAddress(ctx) - - if t.object.Status.Phase == v1alpha1.ExternalIPAttachmentPhaseReady { - if err := t.syncAttachedOnParentExternalIP(ctx, true); err != nil { - ctrllog.FromContext(ctx).Error(err, "Failed to set attached on parent ExternalIP, will retry") +// newExternalIPAttachmentPostSaveOnDelete returns a PostSaveOnDelete that clears +// the attached flag on the parent ExternalIP after the attachment's DELETING +// state is persisted. +func newExternalIPAttachmentPostSaveOnDelete(eipClient privatev1.ExternalIPsClient) func(context.Context, *v1alpha1.ExternalIPAttachment, *privatev1.ExternalIPAttachment) error { + return func(ctx context.Context, _ *v1alpha1.ExternalIPAttachment, remote *privatev1.ExternalIPAttachment) error { + if err := syncAttachedOnParentExternalIP(ctx, eipClient, remote, false); err != nil { + ctrllog.FromContext(ctx).Error(err, "Failed to clear attached on parent ExternalIP, will retry") return err } + return nil } - - return nil -} - -func (t *externalIPAttachmentFeedbackReconcilerTask) handleDelete() { - if t.object.Status.Phase == v1alpha1.ExternalIPAttachmentPhaseFailed { - t.externalIPAttachment.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_FAILED) - return - } - t.externalIPAttachment.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_DELETING) } -func (t *externalIPAttachmentFeedbackReconcilerTask) syncState(ctx context.Context) { - switch t.object.Status.Phase { +func syncExternalIPAttachmentState(ctx context.Context, obj *v1alpha1.ExternalIPAttachment, remote *privatev1.ExternalIPAttachment) { + switch obj.Status.Phase { case v1alpha1.ExternalIPAttachmentPhaseProgressing: - t.externalIPAttachment.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_PENDING) + remote.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_PENDING) case v1alpha1.ExternalIPAttachmentPhaseReady: - t.externalIPAttachment.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_READY) + remote.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_READY) case v1alpha1.ExternalIPAttachmentPhaseFailed: - t.externalIPAttachment.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_FAILED) + remote.GetStatus().SetState(privatev1.ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE_FAILED) default: log := ctrllog.FromContext(ctx) - log.Info("Unknown phase, will ignore it", "phase", t.object.Status.Phase) + log.Info("Unknown phase, will ignore it", "phase", obj.Status.Phase) } } -func (t *externalIPAttachmentFeedbackReconcilerTask) syncAddress(ctx context.Context) { - externalIPID := t.externalIPAttachment.GetSpec().GetExternalIp() +func syncExternalIPAttachmentAddress(ctx context.Context, eipClient privatev1.ExternalIPsClient, remote *privatev1.ExternalIPAttachment) { + externalIPID := remote.GetSpec().GetExternalIp() if externalIPID == "" { return } - response, err := t.r.externalIPsClient.Get(ctx, privatev1.ExternalIPsGetRequest_builder{ + response, err := eipClient.Get(ctx, privatev1.ExternalIPsGetRequest_builder{ Id: externalIPID, }.Build()) if err != nil { @@ -260,17 +178,17 @@ func (t *externalIPAttachmentFeedbackReconcilerTask) syncAddress(ctx context.Con return } if addr := obj.GetStatus().GetAddress(); addr != "" { - t.externalIPAttachment.GetStatus().SetExternalIpAddress(addr) + remote.GetStatus().SetExternalIpAddress(addr) } } -func (t *externalIPAttachmentFeedbackReconcilerTask) syncAttachedOnParentExternalIP(ctx context.Context, attached bool) error { - externalIPID := t.externalIPAttachment.GetSpec().GetExternalIp() +func syncAttachedOnParentExternalIP(ctx context.Context, eipClient privatev1.ExternalIPsClient, remote *privatev1.ExternalIPAttachment, attached bool) error { + externalIPID := remote.GetSpec().GetExternalIp() if externalIPID == "" { return nil } - response, err := t.r.externalIPsClient.Get(ctx, privatev1.ExternalIPsGetRequest_builder{ + response, err := eipClient.Get(ctx, privatev1.ExternalIPsGetRequest_builder{ Id: externalIPID, }.Build()) if err != nil { @@ -294,7 +212,7 @@ func (t *externalIPAttachmentFeedbackReconcilerTask) syncAttachedOnParentExterna } externalIP.GetStatus().SetAttached(attached) - _, err = t.r.externalIPsClient.Update(ctx, privatev1.ExternalIPsUpdateRequest_builder{ + _, err = eipClient.Update(ctx, privatev1.ExternalIPsUpdateRequest_builder{ Object: externalIP, }.Build()) return err diff --git a/internal/controller/feedback/bridge.go b/internal/controller/feedback/bridge.go index a4b71e8a..d31a4578 100644 --- a/internal/controller/feedback/bridge.go +++ b/internal/controller/feedback/bridge.go @@ -66,9 +66,11 @@ type Bridge[O clnt.Object, R proto.Message] struct { // PostSaveOnDelete is an optional callback invoked after Save completes // on the delete path, before finalizer removal. Use it for cross-resource // side effects that must happen after this resource's state is persisted - // (e.g. clearing a parent resource's "attached" flag). + // (e.g. clearing a parent resource's "attached" flag). The remote record + // is provided for callbacks that need to reference it (e.g. reading a + // parent resource ID from the proto spec). // If nil, this step is skipped. - PostSaveOnDelete func(ctx context.Context, obj O) error + PostSaveOnDelete func(ctx context.Context, obj O, remote R) error // IsNotFound determines whether a Fetch error means the remote record // does not exist. If nil, defaults to gRPC codes.NotFound. @@ -153,7 +155,7 @@ func (b *Bridge[O, R]) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re // Run post-save side effects on the delete path (e.g. cross-resource cleanup). if !object.GetDeletionTimestamp().IsZero() && b.PostSaveOnDelete != nil { - if err := b.PostSaveOnDelete(ctx, object); err != nil { + if err := b.PostSaveOnDelete(ctx, object, remote); err != nil { return result, err } } diff --git a/internal/controller/feedback/bridge_test.go b/internal/controller/feedback/bridge_test.go index a3cabe49..a4ad8501 100644 --- a/internal/controller/feedback/bridge_test.go +++ b/internal/controller/feedback/bridge_test.go @@ -405,8 +405,10 @@ var _ = Describe("Bridge", func() { postSaveCalled := false k8sClient := newFakeClient(cr) bridge := newBridge(k8sClient, trk) - bridge.PostSaveOnDelete = func(_ context.Context, _ *v1alpha1.Subnet) error { + bridge.PostSaveOnDelete = func(_ context.Context, _ *v1alpha1.Subnet, remote *privatev1.Subnet) error { Expect(trk.saveCalls).To(Equal(1)) + Expect(remote).NotTo(BeNil()) + Expect(remote.GetStatus().GetState()).To(Equal(privatev1.SubnetState_SUBNET_STATE_DELETING)) inHook := &v1alpha1.Subnet{} Expect(k8sClient.Get(ctx, types.NamespacedName{Name: testName, Namespace: testNamespace}, inHook)).To(Succeed()) Expect(controllerutil.ContainsFinalizer(inHook, testFinalizer)).To(BeTrue()) @@ -434,7 +436,7 @@ var _ = Describe("Bridge", func() { trk := newTracker() k8sClient := newFakeClient(cr) bridge := newBridge(k8sClient, trk) - bridge.PostSaveOnDelete = func(_ context.Context, _ *v1alpha1.Subnet) error { + bridge.PostSaveOnDelete = func(_ context.Context, _ *v1alpha1.Subnet, _ *privatev1.Subnet) error { return errors.New("cross-resource cleanup failed") } @@ -457,7 +459,7 @@ var _ = Describe("Bridge", func() { trk := newTracker() k8sClient := newFakeClient(cr) bridge := newBridge(k8sClient, trk) - bridge.PostSaveOnDelete = func(_ context.Context, _ *v1alpha1.Subnet) error { + bridge.PostSaveOnDelete = func(_ context.Context, _ *v1alpha1.Subnet, _ *privatev1.Subnet) error { Fail("PostSaveOnDelete should not be called on update path") return nil } diff --git a/internal/controller/feedback_controller.go b/internal/controller/feedback_controller.go index 87349e76..fd4f1c42 100644 --- a/internal/controller/feedback_controller.go +++ b/internal/controller/feedback_controller.go @@ -20,7 +20,6 @@ import ( hypershiftv1beta1 "github.com/openshift/hypershift/api/hypershift/v1beta1" "google.golang.org/grpc" - "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -301,11 +300,3 @@ func findClusterCondition(remote *privatev1.Cluster, kind privatev1.ClusterCondi remote.Status.Conditions = append(remote.Status.Conditions, condition) return condition } - -func clone[M proto.Message](message M) M { - return proto.Clone(message).(M) -} - -func equal[M proto.Message](x, y M) bool { - return proto.Equal(x, y) -}