Skip to content

OCPBUGS-74960: prevent resource leak on deletion and handle DependencyViolation #7868

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
Apr 2, 2026
Merged

OCPBUGS-74960: prevent resource leak on deletion and handle DependencyViolation #7868

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
81 changes: 63 additions & 18 deletions control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@ func (r *PrivateServiceObserver) Reconcile(ctx context.Context, req ctrl.Request
return ctrl.Result{}, nil
}

// errDependencyViolation is returned when AWS reports a DependencyViolation,
// indicating the VPC endpoint is still being deleted. The caller translates
// this into a controlled requeue rather than an error-driven requeue.
var errDependencyViolation = errors.New("security group dependency violation")

const (
finalizer = "hypershift.openshift.io/control-plane-operator-finalizer"
endpointServiceDeletionRequeueDuration = 5 * time.Second
Expand All @@ -196,9 +201,22 @@ const (
type AWSEndpointServiceReconciler struct {
client.Client
upsert.CreateOrUpdateProvider
awsClientBuilder clientBuilder
awsClientBuilder awsClientProvider
}

// awsClientProvider abstracts AWS client creation for testability.
//
//go:generate ../../../hack/tools/bin/mockgen -source=awsprivatelink_controller.go -package=awsprivatelink -destination=awsprivatelink_controller_mock.go
type awsClientProvider interface {
getClients(ctx context.Context) (awsapi.EC2API, awsapi.ROUTE53API, error)
initializeWithHCP(log logr.Logger, hcp *hyperv1.HostedControlPlane)
getLocalHostedZoneID() string
setLocalHostedZoneID(zoneID string)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Verify clientBuilder implements awsClientProvider.
var _ awsClientProvider = (*clientBuilder)(nil)

type clientBuilder struct {
mu sync.Mutex
initialized bool
Expand Down Expand Up @@ -316,6 +334,9 @@ func (b *clientBuilder) setFromHCP(hcp *hyperv1.HostedControlPlane) {
}

func (r *AWSEndpointServiceReconciler) SetupWithManager(mgr ctrl.Manager) error {
if r.awsClientBuilder == nil {
r.awsClientBuilder = &clientBuilder{}
}
_, err := ctrl.NewControllerManagedBy(mgr).
For(&hyperv1.AWSEndpointService{}).
WithOptions(controller.Options{
Expand Down Expand Up @@ -391,17 +412,33 @@ func (r *AWSEndpointServiceReconciler) Reconcile(ctx context.Context, req ctrl.R
return ctrl.Result{}, nil
}

// Best-effort initialization for deletion reconciles: after a controller restart
// the clientBuilder is uninitialized because initializeWithHCP is only called in
// the non-deletion path. If the HCP still exists, initialize from it so that
// getClients can succeed and deletion can proceed.
//
// Known issue (SharedVPC): when the HCP is already deleted, the SharedVPC role
// ARNs (needed for cross-account EC2/Route53 access) are lost. Initialization
// cannot happen, getClients will fail, and the finalizer will be preserved until
// the hypershift-operator force-removes it after the grace period — orphaning
// AWS resources in the shared VPC account. A proper fix requires persisting the
// SharedVPC role ARNs in the AWSEndpointService status. See
// TestReconcileDeletionSharedVPC for details.
hcpList := &hyperv1.HostedControlPlaneList{}
if err := r.List(ctx, hcpList, &client.ListOptions{Namespace: req.Namespace}); err == nil && len(hcpList.Items) == 1 {
r.awsClientBuilder.initializeWithHCP(log, &hcpList.Items[0])
}

ec2Client, route53Client, err := r.awsClientBuilder.getClients(ctx)
if err != nil {
log.Error(err, "failed to get AWS client, skipping aws endpoint service cleanup")
} else {
completed, err := r.delete(ctx, awsEndpointService, ec2Client, route53Client)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to delete resource: %w", err)
}
if !completed {
return ctrl.Result{RequeueAfter: endpointServiceDeletionRequeueDuration}, nil
}
return ctrl.Result{}, fmt.Errorf("failed to get AWS clients for endpoint service cleanup: %w", err)
}
completed, err := r.delete(ctx, awsEndpointService, ec2Client, route53Client)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to delete resource: %w", err)
}
if !completed {
return ctrl.Result{RequeueAfter: endpointServiceDeletionRequeueDuration}, nil
}
if controllerutil.ContainsFinalizer(awsEndpointService, finalizer) {
controllerutil.RemoveFinalizer(awsEndpointService, finalizer)
Expand Down Expand Up @@ -1028,6 +1065,10 @@ func (r *AWSEndpointServiceReconciler) delete(ctx context.Context, awsEndpointSe

if awsEndpointService.Status.SecurityGroupID != "" {
if err := r.deleteSecurityGroup(ctx, ec2Client, awsEndpointService.Status.SecurityGroupID); err != nil {
if errors.Is(err, errDependencyViolation) {
log.Info("security group has dependencies, will retry", "id", awsEndpointService.Status.SecurityGroupID)
return false, nil
}
return false, err
}
log.Info("security group deleted", "id", awsEndpointService.Status.SecurityGroupID)
Expand Down Expand Up @@ -1063,7 +1104,6 @@ func (r *AWSEndpointServiceReconciler) delete(ctx context.Context, awsEndpointSe
}

func (r *AWSEndpointServiceReconciler) deleteSecurityGroup(ctx context.Context, ec2Client awsapi.EC2API, sgID string) error {
log := ctrl.LoggerFrom(ctx)
describeSGResult, err := ec2Client.DescribeSecurityGroups(ctx, &ec2.DescribeSecurityGroupsInput{GroupIds: []string{sgID}})
if err != nil {
if supportawsutil.AWSErrorCode(err) == "InvalidGroup.NotFound" {
Expand All @@ -1081,9 +1121,10 @@ func (r *AWSEndpointServiceReconciler) deleteSecurityGroup(ctx context.Context,
GroupId: sg.GroupId,
IpPermissions: sg.IpPermissions,
}); err != nil {
log.Error(err, "failed to revoke security group ingress permissions", "SecurityGroupID", aws.ToString(sg.GroupId), "code", supportawsutil.AWSErrorCode(err))

return fmt.Errorf("failed to revoke security group ingress rules: %s", supportawsutil.AWSErrorCode(err))
if supportawsutil.AWSErrorCode(err) == supportawsutil.DependencyViolation {
return fmt.Errorf("%w: %w", errDependencyViolation, err)
}
return fmt.Errorf("failed to revoke security group %s ingress rules: %w", aws.ToString(sg.GroupId), err)
}
}

Expand All @@ -1092,16 +1133,20 @@ func (r *AWSEndpointServiceReconciler) deleteSecurityGroup(ctx context.Context,
GroupId: sg.GroupId,
IpPermissions: sg.IpPermissionsEgress,
}); err != nil {
log.Error(err, "failed to revoke security group egress permissions", "SecurityGroupID", aws.ToString(sg.GroupId), "code", supportawsutil.AWSErrorCode(err))
return fmt.Errorf("failed to revoke security group egress rules: %s", supportawsutil.AWSErrorCode(err))
if supportawsutil.AWSErrorCode(err) == supportawsutil.DependencyViolation {
return fmt.Errorf("%w: %w", errDependencyViolation, err)
}
return fmt.Errorf("failed to revoke security group %s egress rules: %w", aws.ToString(sg.GroupId), err)
}
}

if _, err = ec2Client.DeleteSecurityGroup(ctx, &ec2.DeleteSecurityGroupInput{
GroupId: sg.GroupId,
}); err != nil {
log.Error(err, "failed to delete security group", "SecurityGroupID", aws.ToString(sg.GroupId), "code", supportawsutil.AWSErrorCode(err))
return fmt.Errorf("failed to delete security group %s: %s", aws.ToString(sg.GroupId), supportawsutil.AWSErrorCode(err))
if supportawsutil.AWSErrorCode(err) == supportawsutil.DependencyViolation {
return fmt.Errorf("%w: %w", errDependencyViolation, err)
}
return fmt.Errorf("failed to delete security group %s: %w", aws.ToString(sg.GroupId), err)
}

return nil
Expand Down
Loading
Loading