Skip to content

CNTRLPLANE-3978: Fix CPO finalizer race leaving orphaned Azure Private Endpoint resources #9194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Aug 7, 2026
+441 −22
Merged

CNTRLPLANE-3978: Fix CPO finalizer race leaving orphaned Azure Private Endpoint resources #9194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 116 additions & 20 deletions control-plane-operator/controllers/azureprivatelinkservice/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,22 @@ func (r *AzurePrivateLinkServiceReconciler) Reconcile(ctx context.Context, req c
return ctrl.Result{}, nil
}

// 3. Add CR finalizer if not present
// 3. Look up the HostedControlPlane. This must happen before adding the CR
// finalizer so that during HCP deletion we return early and never re-add a
// per-CR finalizer that reconcileHCPDeletion just removed.
hcp, err := r.getHCPOrCleanupOrphan(ctx, azPLS, log)
if hcp == nil {
return ctrl.Result{}, err
}

// 4. Handle HCP deletion: clean up Azure resources, remove per-CR finalizers
// from all CRs, and remove the shared HCP finalizer. This returns early so
// step 5 (add CR finalizer) is never reached during HCP deletion.
if !hcp.DeletionTimestamp.IsZero() {
return r.reconcileHCPDeletion(ctx, azPLS, hcp, log)
}

// 5. Add CR finalizer if not present
if !controllerutil.ContainsFinalizer(azPLS, azurePrivateLinkServiceFinalizer) {
controllerutil.AddFinalizer(azPLS, azurePrivateLinkServiceFinalizer)
if err := r.Update(ctx, azPLS); err != nil {
Expand All @@ -288,23 +303,12 @@ func (r *AzurePrivateLinkServiceReconciler) Reconcile(ctx context.Context, req c
return ctrl.Result{}, nil
}

// 4. Wait for PLS alias to be available (populated by HO platform controller)
// 6. Wait for PLS alias to be available (populated by HO platform controller)
if azPLS.Status.PrivateLinkServiceAlias == "" {
log.Info("PLS alias not yet available, waiting")
return ctrl.Result{RequeueAfter: azureutil.PLSRequeueInterval}, nil
}

// 5. Look up the HostedControlPlane for the KAS hostname
hcp, err := r.getHostedControlPlane(ctx, azPLS)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to get HostedControlPlane: %w", err)
}

// 6. Handle HCP deletion: clean up Azure resources and remove HCP finalizer
if !hcp.DeletionTimestamp.IsZero() {
return r.reconcileHCPDeletion(ctx, azPLS, hcp, log)
}

// 7. Add HCP finalizer to block HCP deletion until Azure cleanup is done.
// This is done after the PLS alias is available, which means Azure resources
// are about to be created or already exist. Adding the finalizer at this point
Expand Down Expand Up @@ -396,27 +400,75 @@ func (r *AzurePrivateLinkServiceReconciler) ensureHCPFinalizer(ctx context.Conte
return ctrl.Result{}, nil
}

// reconcileHCPDeletion handles HCP deletion by cleaning up Azure resources and removing
// the HCP finalizer. This ensures credentials remain valid during the cleanup process.
// getHCPOrCleanupOrphan looks up the HostedControlPlane for the given CR. If the HCP
// is gone (NotFound), it removes any orphaned per-CR finalizer so the CR can be
// garbage-collected with the namespace. Returns (nil, nil) when the HCP is gone and
// cleanup succeeded; the caller should return immediately.
func (r *AzurePrivateLinkServiceReconciler) getHCPOrCleanupOrphan(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, log logr.Logger) (*hyperv1.HostedControlPlane, error) {
hcp, err := r.getHostedControlPlane(ctx, azPLS)
if err == nil {
return hcp, nil
}
if !apierrors.IsNotFound(err) {
return nil, fmt.Errorf("failed to get HostedControlPlane: %w", err)
}
if controllerutil.ContainsFinalizer(azPLS, azurePrivateLinkServiceFinalizer) {
log.Info("HostedControlPlane not found, removing orphaned per-CR finalizer", "name", azPLS.Name)
controllerutil.RemoveFinalizer(azPLS, azurePrivateLinkServiceFinalizer)
if updateErr := r.Update(ctx, azPLS); updateErr != nil {
return nil, fmt.Errorf("failed to remove orphaned finalizer: %w", updateErr)
}
}
return nil, nil
}

// reconcileHCPDeletion handles HCP deletion by cleaning up Azure resources, removing
// per-CR finalizers, and removing the shared HCP finalizer. Per-CR finalizers must be
// removed here because once HCP deletion completes and HO deletes the namespace, CPO
// is terminated and can no longer process them. Stuck per-CR finalizers block namespace
// deletion, which blocks HO from removing the HC finalizer, causing a 40-minute timeout.
//
// The flow is:
// 1. If the HCP does not have our finalizer, nothing to do.
// 2. Perform Azure resource cleanup (PE, DNS zone, VNet link, A record).
// 3. Remove the HCP finalizer to unblock HCP deletion.
// 2. Perform Azure resource cleanup for ALL CRs (PE, DNS zone, VNet link, A record).
// 3. Remove per-CR finalizers from ALL CRs so they can be garbage-collected with the namespace.
// 4. Remove the shared HCP finalizer to unblock HCP deletion.
func (r *AzurePrivateLinkServiceReconciler) reconcileHCPDeletion(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, hcp *hyperv1.HostedControlPlane, log logr.Logger) (ctrl.Result, error) {
if !controllerutil.ContainsFinalizer(hcp, hcpAzurePLSFinalizerName) {
return ctrl.Result{}, nil
}

log.Info("HCP is being deleted, cleaning up Azure resources before removing HCP finalizer")

// Perform Azure resource cleanup
if err := r.reconcileDelete(ctx, azPLS, log); err != nil {
// List all AzurePrivateLinkService CRs in the namespace to ensure all are cleaned up
// before removing the shared HCP finalizer. When multiple CRs exist (e.g., private-router
// and oauth-openshift), each must complete Azure resource cleanup while HCP credentials
// are still valid. With MaxConcurrentReconciles: 1, the first CR to reconcile handles
// cleanup for all siblings in a single pass.
var allPLS hyperv1.AzurePrivateLinkServiceList
if err := r.List(ctx, &allPLS, client.InNamespace(azPLS.Namespace)); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to list AzurePrivateLinkService resources: %w", err)
}

if err := r.cleanupAllAzureResources(ctx, allPLS.Items, log); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to clean up Azure resources during HCP deletion: %w", err)

@bryan-cox bryan-cox Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cleanupSiblingAzureResources calls reconcileDelete for each CR, deleteBaseDomainResources calls hasSiblingCR to decide whether to delete the base domain zone. During HCP deletion, neither CR has a DeletionTimestamp set (only the HCP does), so:

  • private-router cleanup: sees oauth-openshift as an active sibling → skips zone deletion
  • oauth-openshift cleanup: sees private-router as an active sibling → skips zone deletion

After both cleanups, per-CR finalizers are removed, CRs are garbage-collected without their own finalizer logic running, and the base domain DNS zone is never deleted.

This is the same class of bug this PR is fixing — just for the base domain zone instead of the PE. Could you either skip the hasSiblingCR check when called from the HCP deletion path, or add a dedicated base domain zone cleanup pass after all CRs are processed?

If resource group deletion is expected to clean this up, a comment documenting that assumption would be helpful.

@Nirshal Nirshal Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this is a real bug. During HCP deletion neither CR has DeletionTimestamp set, so hasSiblingCR returns true for both and the base domain zone is never deleted.

Before fixing, I verified that resource group deletion wouldn't clean this up implicitly. The guest resource group (azPLS.Spec.ResourceGroupName) is only deleted by the CLI path (hypershift destroy cluster azure). During normal HCP-driven teardown, the HO controller does not delete the resource group: AzureCluster is annotated managed-by: external so CAPZ doesn't manage it either, and the HO's delete() function only removes individual resources via finalizers. So the zone would remain orphaned in the customer's subscription.

I considered adding a flag parameter to reconcileDelete (e.g. skipSiblingCheck bool) to bypass hasSiblingCR when called from the HCP deletion path, but that would mean threading a boolean through two levels of calls (reconcileDelete -> deleteBaseDomainResources). As a fan of Uncle Bob Martin's Clean Code, I'd rather avoid flag arguments that change a function's behavior based on a boolean - it's a sign the function is doing two things.

Instead I went with a dedicated deleteBaseDomainDNSZone pass in reconcileHCPDeletion, called after cleanupAllAzureResources. At that point all per-CR A records and VNet links are already cleaned up, so the zone is empty and safe to delete. The call is idempotent: in the single-CR case, deleteBaseDomainResources already deleted the zone (no siblings), and deleteBaseDomainDNSZone gets a NotFound which is handled gracefully.

Added tests for: multi-CR deletion (zone gets deleted), single-CR idempotency, zone deletion failure preserving the HCP finalizer, and a no-BaseDomain assertion in the existing multi-CR test.

}

// Delete the base domain DNS zone explicitly. When multiple CRs share the same
// base domain zone, each CR's reconcileDelete skips zone deletion because
// hasSiblingCR sees the other CR as still active (neither has DeletionTimestamp
// during HCP deletion). The per-CR cleanup above already removed all A records
// and VNet links from the zone, so it is safe to delete here.
if err := r.deleteBaseDomainDNSZone(ctx, allPLS.Items, log); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to delete base domain DNS zone during HCP deletion: %w", err)
}

if err := r.removeAllCRFinalizers(ctx, allPLS.Items, log); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to remove per-CR finalizers during HCP deletion: %w", err)
}

// Remove the HCP finalizer to unblock HCP deletion
log.Info("Azure resource cleanup complete, removing HCP finalizer")
log.Info("Azure resource cleanup complete for all AzurePrivateLinkService CRs, removing HCP finalizer")
originalHCP := hcp.DeepCopy()
controllerutil.RemoveFinalizer(hcp, hcpAzurePLSFinalizerName)
if err := r.Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil {
Expand All @@ -429,6 +481,50 @@ func (r *AzurePrivateLinkServiceReconciler) reconcileHCPDeletion(ctx context.Con
return ctrl.Result{}, nil
}

// deleteBaseDomainDNSZone finds the base domain from the CR list and deletes the
// shared DNS zone. This is called during HCP deletion after all per-CR resources
// (A records, VNet links, PEs) have been cleaned up by cleanupAllAzureResources.
func (r *AzurePrivateLinkServiceReconciler) deleteBaseDomainDNSZone(ctx context.Context, items []hyperv1.AzurePrivateLinkService, log logr.Logger) error {
for i := range items {
if items[i].Spec.BaseDomain != "" {
log.Info("Deleting base domain DNS zone after all-CR cleanup", "zone", items[i].Spec.BaseDomain)
return r.deleteDNSZone(ctx, items[i].Spec.ResourceGroupName, items[i].Spec.BaseDomain, log)
}
}
return nil
}

func (r *AzurePrivateLinkServiceReconciler) cleanupAllAzureResources(ctx context.Context, items []hyperv1.AzurePrivateLinkService, log logr.Logger) error {
var errs []error
for i := range items {
pls := &items[i]
log.Info("Cleaning up Azure resources for AzurePrivateLinkService", "name", pls.Name)
if err := r.reconcileDelete(ctx, pls, log); err != nil {
errs = append(errs, fmt.Errorf("failed to clean up %s: %w", pls.Name, err))
}
}
return utilerrors.NewAggregate(errs)
}

// removeAllCRFinalizers removes per-CR finalizers so the CRs can be deleted
// during namespace cleanup without requiring CPO to still be running.
func (r *AzurePrivateLinkServiceReconciler) removeAllCRFinalizers(ctx context.Context, items []hyperv1.AzurePrivateLinkService, log logr.Logger) error {
var errs []error
for i := range items {
pls := &items[i]
if !controllerutil.ContainsFinalizer(pls, azurePrivateLinkServiceFinalizer) {
continue
}
log.Info("Removing per-CR finalizer from AzurePrivateLinkService", "name", pls.Name)
original := pls.DeepCopy()
controllerutil.RemoveFinalizer(pls, azurePrivateLinkServiceFinalizer)
if err := r.Patch(ctx, pls, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil {
errs = append(errs, fmt.Errorf("failed to remove per-CR finalizer from %s: %w", pls.Name, err))

@bryan-cox bryan-cox Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: This Update uses the object from the List snapshot at the top of reconcileHCPDeletion. If the CR is modified between the List and this Update, it's a last-writer-wins with no optimistic lock. The HCP finalizer removal at line 464 uses MergeFromWithOptimisticLock — consider the same pattern here for consistency. With MaxConcurrentReconciles: 1 and this being a deletion path the risk is low, but it would be more defensive.

@Nirshal Nirshal Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched from r.Update to r.Patch with MergeFromWithOptimisticLock in removeAllCRFinalizers, consistent with how ensureHCPFinalizer and the HCP finalizer removal in reconcileHCPDeletion already work.

}
}
return utilerrors.NewAggregate(errs)
}

// reconcilePrivateEndpoint creates or updates the Private Endpoint in the guest VNet.
func (r *AzurePrivateLinkServiceReconciler) reconcilePrivateEndpoint(ctx context.Context, azPLS *hyperv1.AzurePrivateLinkService, log logr.Logger) (ctrl.Result, error) {
resourceGroup := azPLS.Spec.ResourceGroupName
Expand Down
Loading