CNTRLPLANE-507: Add HCP finalizer to AWSEndpointService reconciler - #8499
CNTRLPLANE-507: Add HCP finalizer to AWSEndpointService reconciler#8499hypershift-jira-solve-ci[bot] wants to merge 4 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-507 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds an HCP-scoped AWS PrivateLink finalizer, updates HostedControlPlane event handling to enqueue AWSEndpointService reconciliations, and splits reconciliation into normal and HCP-deletion paths. The deletion path initializes AWS clients from the HCP, cleans up AWS resources, removes the AWSEndpointService finalizer, and clears the HCP finalizer after dependent CRs are done. Tests cover finalizer patching, deletion handling, client errors, and mapping behavior. Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
area/control-plane-operator
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8499 +/- ##
==========================================
+ Coverage 43.50% 43.59% +0.08%
==========================================
Files 771 771
Lines 95722 95786 +64
==========================================
+ Hits 41648 41760 +112
+ Misses 51174 51121 -53
- Partials 2900 2905 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Re: #8499 (comment) Thanks for the walkthrough summary — the description and sequence diagram accurately capture the changes. Regarding the docstring coverage warning: noted. This is a new controller flow with several helper functions; docstrings can be added in a follow-up if the team considers it valuable for this area. AI-assisted response via Claude Code |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Staff Engineer Review: Add HCP finalizer to AWSEndpointService reconciler
The core idea of this PR is correct and addresses a real operational problem — preventing AWS PrivateLink resource orphaning when the HCP is deleted before the AWSEndpointService cleanup runs. However, the implementation diverges from the Azure PLS pattern in ways that introduce correctness risks.
Blocking Issues (3)
See inline comments for details on:
- Dual deletion paths compete — existing CR deletion path and new HCP deletion path both remove the CR finalizer
- Multi-CR coordination under concurrency — convergent but produces unnecessary work with
MaxConcurrentReconciles: 10 UpdateFuncmisses HCP deletions on controller restart — defeats the purpose of the PR
Open Questions (2)
- Does the hypershift-operator's force-finalizer-removal logic (10-minute grace) know about this new
aws-private-link-endpoint-cleanupfinalizer? If not, the HCP could get stuck indefinitely. - The finalizer is added for ALL AWS PrivateLink clusters, not just SharedVPC. Is the broader scope intentional?
Praise
Test coverage is excellent — 784 lines of well-structured table-driven tests with gomock and client interceptors covering all new paths. The context.Background() → ctx fix in the handler is a good improvement.
| MaxConcurrentReconciles: 10, | ||
| }). | ||
| Watches(&hyperv1.HostedControlPlane{}, handler.Funcs{UpdateFunc: r.enqueueOnAccessChange(mgr)}). | ||
| Watches(&hyperv1.HostedControlPlane{}, handler.Funcs{UpdateFunc: r.enqueueOnHCPChange(mgr)}). |
There was a problem hiding this comment.
[blocking] UpdateFunc misses HCP deletions on controller restart
Using handler.Funcs{UpdateFunc: ...} means only Update events trigger this handler. If the CPO restarts while an HCP is being deleted (DeletionTimestamp already set), the informer cache sync generates a Create event — not an Update — so this handler never fires.
The Azure PLS controller avoids this by using handler.EnqueueRequestsFromMapFunc(...), which receives all event types (Create, Update, Delete) from the informer. On restart, it gets a Create event for the HCP with DeletionTimestamp set and correctly enqueues the CRs.
With the current approach, if the CPO restarts mid-HCP-deletion, the new handler will NOT fire. The reconciler would fall through to the existing AWSEndpointService CR deletion path — exactly the scenario this PR is trying to fix.
Recommendation: Switch to handler.EnqueueRequestsFromMapFunc(...) to match the Azure PLS pattern.
There was a problem hiding this comment.
Done. Switched from handler.Funcs{UpdateFunc: ...} to handler.EnqueueRequestsFromMapFunc(...) matching the Azure PLS pattern. The new mapHCPToAWSEndpointService() function receives all event types (Create, Update, Delete), so on controller restart the informer cache sync's Create event now correctly triggers cleanup for an HCP with DeletionTimestamp already set.
The EndpointAccess change detection (previously via old/new comparison in UpdateFunc) is dropped from the handler — those changes are picked up by the reconciler's existing 5-minute periodic requeue, which is acceptable since EndpointAccess changes are rare operational events.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Switched from handler.Funcs{UpdateFunc: ...} to handler.EnqueueRequestsFromMapFunc(r.mapHCPToAWSEndpointService()) matching the Azure PLS pattern exactly. The new mapHCPToAWSEndpointService() MapFunc receives all event types (Create, Update, Delete), so on controller restart the informer cache sync's Create event correctly triggers cleanup for an HCP with DeletionTimestamp already set.
The MapFunc filters by finalizer presence (controllerutil.ContainsFinalizer(hcp, hcpAWSPrivateLinkFinalizerName)) to avoid unnecessary reconciliations, matching the Azure PLS approach. EndpointAccess change detection is dropped from the handler — those changes are picked up by the reconciler's existing 5-minute periodic requeue.
Tests updated: replaced TestEnqueueOnHCPChange (which tested the old UpdateFunc) with TestMapHCPToAWSEndpointService (which tests the new MapFunc directly).
AI-assisted response via Claude Code
| // Handle HCP deletion: clean up AWS resources while HCP credentials are still valid. | ||
| if !hcp.DeletionTimestamp.IsZero() { | ||
| return r.reconcileHCPDeletion(ctx, awsEndpointService, hcp, log) | ||
| } |
There was a problem hiding this comment.
[blocking] Dual deletion paths can compete
The existing AWSEndpointService CR deletion path (lines 466-486 in the diff) runs when the CR itself has a DeletionTimestamp and also removes the CR finalizer + calls r.delete(). This new HCP deletion path at line 534 also removes the CR finalizer + calls r.delete().
These two paths can activate simultaneously during namespace deletion or HCP ownership-based cascading. Consider:
- HCP deletion triggers
enqueueOnHCPChange, enqueuing all CRs - Namespace/owner cascade sets DeletionTimestamp on the CRs themselves
- A reconcile fires for a CR that has BOTH its own DeletionTimestamp AND the HCP is being deleted
- The CR enters the existing deletion path (step 1), which initializes from HCP and cleans up
- Another reconcile enters this HCP deletion path
The existing CR deletion path (line 466) does return early before reaching this check, so they are technically exclusive within a single reconcile call. But with MaxConcurrentReconciles: 10, two concurrent reconciles for the same CR could race.
Suggestion: Add an explicit guard here: if !awsEndpointService.DeletionTimestamp.IsZero() { return ctrl.Result{}, nil } to make the exclusion explicit and defend against concurrent reconciles.
There was a problem hiding this comment.
Done. Added explicit guard at the top of reconcileHCPDeletion:
if !awsEndpointService.DeletionTimestamp.IsZero() {
return ctrl.Result{}, nil
}This makes the exclusion between the two deletion paths explicit and defends against concurrent reconciles under MaxConcurrentReconciles: 10. If the CR itself is being deleted, we defer to the existing CR deletion path at the top of Reconcile.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Added explicit guard at the top of reconcileHCPDeletion:
if !awsEndpointService.DeletionTimestamp.IsZero() {
return ctrl.Result{}, nil
}This makes the exclusion between the two deletion paths explicit and defends against concurrent reconciles under MaxConcurrentReconciles: 10. If the CR itself is being deleted, we defer to the existing CR deletion path at the top of Reconcile.
AI-assisted response via Claude Code
| // our finalizer blocks HCP deletion. | ||
| r.awsClientBuilder.initializeWithHCP(log, hcp) | ||
| ec2Client, route53Client, err := r.awsClientBuilder.getClients(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
[blocking] Multi-CR coordination needs documentation or simplification
With MaxConcurrentReconciles: 10 and enqueueOnHCPChange enqueuing ALL CRs, multiple reconcilers race through reconcileHCPDeletion concurrently. Each one cleans up its own CR, then checks if all others are done. The last one to finish removes the HCP finalizer, while earlier finishers return RequeueAfter: 5s and re-enter this path only to find the HCP finalizer already removed.
This convergent pattern is functionally correct, but:
- It produces unnecessary requeues and reconcile loops
- It is not documented, making it hard for future maintainers to reason about
- The Azure PLS controller avoids this entirely because it has
MaxConcurrentReconciles: 1and only one CR per namespace
Suggestion: At minimum, add a comment explaining the convergent behavior. Alternatively, consider having only the CR whose cleanup triggers len(pendingCRs) == 0 remove the HCP finalizer, and have all others simply return ctrl.Result{} after their own cleanup.
There was a problem hiding this comment.
Done. Added comprehensive documentation on the reconcileHCPDeletion function explaining the convergent multi-CR coordination pattern:
- Multiple reconcilers run concurrently (one per AWSEndpointService CR)
- Each cleans up its own CR, then checks if all CRs are done
- Only the last reconciler to finish removes the HCP finalizer
- Earlier finishers see pending CRs, return RequeueAfter, and on re-entry find the finalizer already removed
The comment explains this produces a small number of no-op requeues but is correct and self-healing. This is functionally similar to how the Azure PLS controller works, but documented explicitly because the AWS controller has MaxConcurrentReconciles: 10 and multiple CRs per namespace (unlike Azure PLS's MaxConcurrentReconciles: 1 with one CR per namespace).
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Added comprehensive documentation on reconcileHCPDeletion explaining the convergent multi-CR coordination pattern:
- Multiple reconcilers run concurrently (one per AWSEndpointService CR) under
MaxConcurrentReconciles: 10 - Each cleans up its own CR's AWS resources, removes the CR finalizer, then checks if all CRs are done
- The last reconciler to finish (seeing
len(pendingCRs) == 0) removes the HCP finalizer - Earlier finishers see pending CRs, return
RequeueAfter, and on re-entry find the HCP finalizer already removed
The comment explicitly contrasts this with the Azure PLS controller (MaxConcurrentReconciles: 1, one CR per namespace) to explain why this convergent pattern is necessary for the AWS controller.
AI-assisted response via Claude Code
There was a problem hiding this comment.
One detail on the extra requeue: the r.List at the pending-CRs check reads from the informer cache, which won't have caught up with the r.Patch that just removed the CR finalizer. So the reconciler that finishes last is guaranteed to see itself as still-pending and take one extra requeue cycle. Not a real problem — just worth a brief comment, or you could skip the current AES by name in the pending-CRs loop to avoid it.
There was a problem hiding this comment.
Done. Opted to skip the current AES by name in the pending-CRs loop rather than just adding a comment — this eliminates the guaranteed extra requeue cycle for the last reconciler to finish.
AI-assisted response via Claude Code
| controllerutil.AddFinalizer(hcp, hcpAWSPrivateLinkFinalizerName) | ||
| if err := r.Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { | ||
| if apierrors.IsConflict(err) { | ||
| return ctrl.Result{Requeue: true}, nil |
There was a problem hiding this comment.
[suggestion] Use RequeueAfter: time.Second instead of Requeue: true on conflicts
The Azure PLS equivalent returns ctrl.Result{RequeueAfter: time.Second} on conflict (see controller.go line 371). Using Requeue: true risks a tight retry loop under contention when multiple AWSEndpointService reconcilers are concurrently trying to patch the same HCP.
Same applies to the conflict handling in ensureHCPFinalizer (line 558).
There was a problem hiding this comment.
Done. Changed both conflict-handling sites to ctrl.Result{RequeueAfter: time.Second}:
ensureHCPFinalizer(adding finalizer)reconcileHCPDeletion(removing finalizer)
Both now include a comment explaining the rationale: avoiding tight retry loops when multiple AWSEndpointService reconcilers concurrently try to patch the same HCP.
Note: the Azure PLS controller also uses Requeue: true for conflicts (controller.go line 371), so this change makes the AWS controller stricter than Azure PLS.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Changed both conflict-handling sites to ctrl.Result{RequeueAfter: time.Second}:
ensureHCPFinalizer(adding finalizer)reconcileHCPDeletion(removing finalizer)
Both now include a comment explaining the rationale: avoiding tight retry loops when multiple AWSEndpointService reconcilers concurrently try to patch the same HCP. Tests updated to assert RequeueAfter > 0 instead of Requeue == true.
AI-assisted response via Claude Code
| // TestReconcileDeletionSharedVPC for details. | ||
| // The HCP finalizer (hcpAWSPrivateLinkFinalizerName) added during normal | ||
| // reconciliation ensures the HCP remains available during this cleanup. | ||
| // For SharedVPC clusters, this guarantees the cross-account role ARNs can |
There was a problem hiding this comment.
[suggestion] Comment overstates the guarantee
This comment claims the HCP finalizer "ensures the HCP remains available during this cleanup." That is only true after a successful normal reconciliation has added the finalizer. If a cluster is newly created and the controller has not yet reconciled (e.g., controller was down), the HCP can still be deleted before the AWSEndpointService cleanup runs — the old scenario.
Consider acknowledging this edge case rather than stating the guarantee unconditionally.
There was a problem hiding this comment.
Done. Updated the comment to acknowledge the edge case. The new wording states that the finalizer "when present, blocks HCP deletion" and explicitly notes that it's only added after a successful normal reconciliation — if the controller hasn't reconciled yet (e.g., was down since cluster creation), the HCP may be deleted before the finalizer is placed, and the best-effort initialization is the only protection in that case.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Updated the comment to acknowledge the edge case. The new wording states that the finalizer, "when present, blocks HCP deletion" and explicitly notes that it's only added after a successful normal reconciliation — if the controller hasn't reconciled yet (e.g., was down since cluster creation), the HCP may be deleted before the finalizer is placed, and the best-effort initialization is the only protection in that case.
AI-assisted response via Claude Code
| } | ||
|
|
||
| // Enqueue when EndpointAccess changes (existing behavior). | ||
| if newHCP.Spec.Platform.AWS != nil && oldHCP.Spec.Platform.AWS != nil && newHCP.Spec.Platform.AWS.EndpointAccess != oldHCP.Spec.Platform.AWS.EndpointAccess { |
There was a problem hiding this comment.
[suggestion] Filter deletion trigger to transition only
Once the HCP finalizer is added, ANY HCP update with a DeletionTimestamp will re-enqueue all CRs. During HCP deletion, status updates from other controllers will repeatedly trigger this, producing unnecessary list+enqueue cycles.
Consider adding oldHCP.DeletionTimestamp.IsZero() to the condition so it only fires on the transition to deletion:
if oldHCP.DeletionTimestamp.IsZero() && !newHCP.DeletionTimestamp.IsZero() && controllerutil.ContainsFinalizer(newHCP, hcpAWSPrivateLinkFinalizerName) {There was a problem hiding this comment.
Addressed by switching to `EnqueueRequestsFromMapFunc` (per comment 1). With the new `mapHCPToAWSEndpointService()` MapFunc, we can no longer detect the deletion transition (old vs new DeletionTimestamp) because MapFunc only receives the current object, not old/new.
However, the filtering is achieved differently: the MapFunc only fires when the HCP has our finalizer (`controllerutil.ContainsFinalizer(hcp, hcpAWSPrivateLinkFinalizerName)`). Once all CRs are cleaned up and the HCP finalizer is removed, subsequent HCP updates no longer trigger CR enqueues. During the short deletion window, the repeated enqueues from status updates are harmless since the reconciler is idempotent — CRs that are already cleaned up return early. This matches the Azure PLS pattern exactly.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Addressed by switching to EnqueueRequestsFromMapFunc (per comment 1). With the new mapHCPToAWSEndpointService() MapFunc, we can no longer detect the deletion transition (old vs new DeletionTimestamp) because MapFunc only receives the current object, not old/new.
However, the filtering is achieved differently: the MapFunc only fires when the HCP has our finalizer (controllerutil.ContainsFinalizer(hcp, hcpAWSPrivateLinkFinalizerName)). Once all CRs are cleaned up and the HCP finalizer is removed, subsequent HCP updates no longer trigger CR enqueues. During the short deletion window, the repeated enqueues from status updates are harmless since the reconciler is idempotent — CRs that are already cleaned up return early. This matches the Azure PLS pattern exactly.
AI-assisted response via Claude Code
There was a problem hiding this comment.
One additional thing to consider: after the finalizer is placed, every HCP status update (from other controllers) will trigger reconciliation of all AES CRs, since the MapFunc only filters on finalizer presence. Before the finalizer is placed, EndpointAccess changes aren't detected at all. The comment at line 405-406 acknowledges the deferred detection, and the over-triggering is bounded by the existing MaxConcurrentReconciles: 10 -- but it's a behavioral change from the old selective approach worth keeping in mind for AWS API call volume.
There was a problem hiding this comment.
Acknowledged. The over-triggering is bounded by MaxConcurrentReconciles: 10 and each extra reconcile is cheap (reads from informer cache, checks finalizer state). Agree this is worth keeping in mind for AWS API call volume — the actual AWS API calls only happen in the normal reconcile path which has the 5 * time.Minute requeue guard.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Acknowledged. The over-triggering from HCP status updates is bounded by MaxConcurrentReconciles: 10 and the reconciler returns early for non-deletion events when there are no AWS resource changes. The periodic 5-minute requeue already handles EndpointAccess drift detection.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Acknowledged. The over-triggering is bounded by MaxConcurrentReconciles: 10 and the reconciler returns early at multiple checkpoints (no serviceName, HCP not deleting, etc.) so the additional reconciliations are cheap no-ops. The tradeoff is acceptable given that the alternative (selective HCP field watching) would miss the DeletionTimestamp change on controller restart.
AI-assisted response via Claude Code
|
Re: #8499 (review) @bryan-cox Thank you for the thorough staff engineer review. All blocking issues and suggestions have been addressed — see inline replies for details on each change. Summary of changes
Answers to open questionsQ: Does the hypershift-operator's force-finalizer-removal logic know about this new finalizer? Q: Is the broader scope (all AWS PrivateLink clusters, not just SharedVPC) intentional? AI-assisted response via Claude Code |
|
Re: #8499 (review) Thank you for the thorough staff engineer review, @bryan-cox. All 6 inline comments have been addressed: Blocking Issues (3) — All Fixed
Suggestions (3) — All Fixed
All tests pass. Ready for re-review. AI-assisted response via Claude Code |
|
/test e2e-aws |
e2e-aws evidence that the HCP finalizer is workingJob: pull-ci-openshift-hypershift-main-e2e-aws/2067264178355179520 Finalizer added during reconciliation (CPO logs)Both private cluster tests show the CPO logging TestCreateClusterPrivate — CPO log (grep for {"level":"info","ts":"2026-06-17T17:02:21Z","msg":"Adding HCP finalizer for AWS PrivateLink cleanup","controller":"awsendpointservice","AWSEndpointService":{"name":"kube-apiserver-private","namespace":"e2e-clusters-5w84p-private-xcn67"}}TestCreateClusterPrivateWithRouteKAS — CPO log (grep for {"level":"info","ts":"2026-06-17T17:03:11Z","msg":"Adding HCP finalizer for AWS PrivateLink cleanup","controller":"awsendpointservice","AWSEndpointService":{"name":"private-router","namespace":"e2e-clusters-5jz4x-private-22vkq"}}Finalizer present on HCP resources (YAML dumps)All three HCPs in the dump have
Clean teardown (destroy logs)Both private cluster tests destroyed successfully with no errors — the finalizer did not block teardown:
LimitationThe |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, hypershift-jira-solve-ci[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/lgtm cancel |
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
openshift-ci
Bot
added
the
needs-rebase
3c58530 to
9877433
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/address-review-comments |
|
🤖 Addressing review comments: workflow run |
|
/restructure-commits |
|
🤖 Restructuring commits: workflow run |
eeacd8c to
ef58f03
Compare
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
Overall this is well-designed -- the convergent multi-CR coordination pattern is solid, the flow ordering in Reconcile is carefully thought through, and the test coverage is thorough. One minor test gap noted inline.
| } | ||
| } | ||
|
|
||
| func TestReconcileCRDeletion(t *testing.T) { |
There was a problem hiding this comment.
TestReconcileCRDeletion doesn't exercise the delete() returning (false, nil) requeue path -- the case where AWS resources aren't fully cleaned up yet (e.g., DependencyViolation on security group deletion). TestReconcileHCPDeletion covers this at line 2228 with the DependencyViolation mock pattern. Adding a similar case here (AES with SecurityGroupID in status, DeleteSecurityGroup returning DependencyViolation) would cover the RequeueAfter: endpointServiceDeletionRequeueDuration branch.
There was a problem hiding this comment.
Done. Added a "When AWS resource cleanup is incomplete it should requeue" test case to TestReconcileCRDeletion with SecurityGroupID in status and DeleteSecurityGroup returning DependencyViolation, verifying the RequeueAfter: endpointServiceDeletionRequeueDuration branch.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
The HCP finalizer pattern here is correct and matches the Azure PLS controller. The issue is that deleteAWSEndpointServices() in the HC controller (hostedcluster_controller.go:3834) deletes all AWSEndpointService CRs and waits for them to be GC'd before the HCP gets its DeletionTimestamp (line 3900). By the time the HCP is deleted, no CRs exist for mapHCPToAWSEndpointService to enqueue, reconcileHCPDeletion never runs, and this finalizer is never removed. The HC controller loops at "Waiting for hostedcontrolplane deletion" indefinitely.
Azure PLS doesn't have this problem because the HC controller has no equivalent deleteAzurePrivateLinkServices() — Azure PLS CRs still exist when the HCP gets DeletionTimestamp, so the watch fires and cleanup works as designed.
deleteAWSEndpointServices() was introduced in PR #4740 (OCPBUGS-42107) as a fix for the same underlying problem — the HCP being unavailable during cleanup. It was the right solution at the time (pre-SharedVPC), but the HCP finalizer pattern is the architectural replacement, as described in CNTRLPLANE-507. The two approaches conflict because deleteAWSEndpointServices() reverses the deletion ordering that the HCP finalizer depends on.
I think the right path is to remove deleteAWSEndpointServices() from the HC controller's delete() function (lines 3833-3841) and let the CPO handle everything through reconcileHCPDeletion, aligning AWS with the Azure pattern. The 10-minute force-removal safety net in deleteAWSEndpointServices() would be replaced by the HCP finalizer itself — which is better for SharedVPC since it prevents orphaned resources rather than allowing them after a timeout.
|
@enxebre @bryan-cox -- This one may be a really good example of the fact that these autonomous/semi-autonomous workflows can really be time/token sinks. 😬 |
|
/test address-review-comments |
|
Review agent triggered. View job |
|
Done. Removed Also aligned the CR finalizer addition to use AI-assisted response via Claude Code |
…reconciler
Add a finalizer on the HostedControlPlane to block HCP deletion until
all AWS PrivateLink resources (VPC endpoints, security groups, DNS
records) are cleaned up. Without this finalizer, the controller may
not be able to construct valid AWS clients if the HCP is already
deleted — particularly for SharedVPC clusters where cross-account
role ARNs are sourced from the HCP spec — which would orphan AWS
resources.
Key changes:
- Add hcpAWSPrivateLinkFinalizerName finalizer constant and manage
its lifecycle during reconcile/cleanup
- Replace handler.Funcs{UpdateFunc: ...} with
EnqueueRequestsFromMapFunc so Create/Delete/Update HCP events all
trigger reconciliation — critical for CPO restarts where a
deleting HCP appears as a Create event after cache sync
- Guard reconciliation behind finalizer presence to avoid unnecessary
reconciliations for unrelated HCPs
- Refactor cleanup logic into deleteAWSEndpointService to share
between reconcile and HCP-deletion paths
- Clear localZoneID for non-SharedVPC clusters in setFromHCP
- Add comprehensive unit tests for the new finalizer behavior
Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com>
Commit-Message-Assisted-by: Claude (via Claude Code)
…tion Address review feedback: add test case exercising the delete() returning (false, nil) requeue path in TestReconcileCRDeletion (DependencyViolation on security group deletion). Also add invariant comment at the pending-CRs check in reconcileHCPDeletion documenting that CRs only get the finalizer after serviceName is set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove deleteAWSEndpointServices() from the HC controller's delete() path. It conflicts with the HCP finalizer pattern: it deletes all AWSEndpointService CRs before the HCP gets its DeletionTimestamp, so mapHCPToAWSEndpointService has nothing to enqueue and reconcileHCPDeletion never runs, leaving the HCP finalizer stuck. With this change, the CPO handles all AWS PrivateLink cleanup through reconcileHCPDeletion, aligning AWS with the Azure PLS pattern. The HCP finalizer replaces the 10-minute force-removal safety net. Also aligns CR finalizer addition to use Patch with optimistic locking instead of Update, matching the pattern used for HCP finalizer operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
45fa34f to
a7a53a7
Compare
openshift-ci
Bot
added
the
area/hypershift-operator
|
/test address-review-comments |
|
Now I have all the evidence needed. The failure is clear and well-documented. Let me produce the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is Gangway API rate limiting on the CI infrastructure. Specifically:
Recommendations
Evidence
|
|
/test address-review-comments |
|
Review agent triggered. View job |
With MaxConcurrentReconciles: 10, multiple reconcilers can race past the pending-CRs check and attempt to remove the HCP finalizer. The first succeeds; if the HCP had no other finalizers, k8s GCs it and runner-up reconcilers get NotFound as a hard reconcile error. Handle this gracefully by returning nil, matching the existing Conflict handling pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@hypershift-jira-solve-ci[bot]: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Not blocking, just a note... the need for this seems like a pattern that will continue to emerge across other platforms and resources classes (GCP...) and may be indicative of a design gap. Tracking deletion handling across sprawling finalizers and processes could lead us to inadvertent coupling, ordering issues, too many testing dimensions, etc. Deletion handling can't be reasoned about locally (adding a new one has to consider the behavior around all the others to avoid those issues, etc). Like, what if the main HCP reconciler's deletion path could gate the main finalizer removal on a set of status conditions (e.g. CloudResourcesCleanedUp per platform controller), where each platform controller sets its own condition to true when its cleanup is done, replacing per-controller finalizers with per-controller conditions and a single finalizer that reads them all. Food for thought... edit:
I agree it "matches the Azure PLS controller" but as to whether the "HCP finalizer pattern here is correct" I don't necessarily agree with that assumption (hence my above notes) |
|
/close Closing this PR in favor of a new approach. Based on Dan's design feedback and the follow-up discussion with Cesar and Alberto, the consensus is to use HCP status conditions instead of per-controller finalizers to gate deletion cleanup. This avoids the coupling/ordering issues Dan raised and degrades gracefully via timeouts if a controller can't run. New ticket: CNTRLPLANE-3857 |
|
@bryan-cox: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
5 participants
What this PR does / why we need it:
Adds a finalizer on the HostedControlPlane resource from the AWSEndpointService reconciler to prevent HCP deletion before AWS PrivateLink resources are cleaned up.
Problem: When the CPO restarts during deletion of a SharedVPC cluster, the
clientBuilderis uninitialized and the HCP (with its cross-account role ARNs) may already be deleted. This causes the reconciler to fail creating AWS clients, and after a 10-minute grace period the hypershift-operator force-removes the CPO finalizer — orphaning VPC endpoints, security groups, and DNS records in the shared VPC account.Solution: The new HCP finalizer (
hypershift.openshift.io/aws-private-link-endpoint-cleanup) follows the same pattern used by the Azure PLS controller:enqueueOnHCPChange) to also trigger reconciliation when an HCP is being deleted with the finalizer presentWhich issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-507
Special notes for your reviewer:
enqueueOnHCPChangehandler (renamed fromenqueueOnAccessChange) now triggers on both EndpointAccess changes and HCP deletions with the finalizergetAWSClienthelper, sourcing credentials from the still-available HCP specChecklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-507](https://redhat.atlassian.net/browse/CNTRLPLANE-507)Summary by CodeRabbit
Bug Fixes
Tests