Skip to content

[feat: gw api] Fix edge cases for lb config finalizers #4225

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

Jump to bottom
Merged
merged 3 commits into from
Jun 11, 2025
Merged

[feat: gw api] Fix edge cases for lb config finalizers #4225

merged 3 commits into from
Jun 11, 2025

Conversation

shraddhabang
Copy link
Collaborator

Description

  1. Fixed few edge cases as mentioned in here.
  • Edge case: If the gwClass C with lbconfig C has two gateways A and B. Finalizer is added on lbcfg C. If both the gateways are deleted, the finalizer will not be removed from lbcfg sinc it is still being referenced by gwclass C. So when we delete gwclass C, it should also trigger the removal of finalizer on lbcfg C if its not in use by any other gateway/gwclas.
  • Edge case: If the gateway A is created with lbconfig A, the finalizer is added on lbcfg A. Now I change the gateway A to reference another lbcfg B. This does not remove finalizer on initial lbcfg A. So when the deletion of lbcfg gets blocked. Hence, if we get a deletion event for lbcfg, we should check whether its being used by any gw/gwclasses currenlty. If not, finalizer should be removed from it.
  1. Moved the shared utils for gateway and lb cfg in their own utils files to avoid cyclic imports.
  2. Added missing rbac permissions to patch resources with finalizers
  3. refactored the code for IsLBConfigInUse to make it modular based on gw and gwclass.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the docs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shraddhabang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 6, 2025
zac-nixon
zac-nixon reviewed Jun 6, 2025
View reviewed changes
zac-nixon
zac-nixon reviewed Jun 6, 2025
View reviewed changes
controllers/gateway/eventhandlers/gatewayclass/load_balancer_configuration_events.go
@@ -46,6 +49,14 @@ func (h *enqueueRequestsForLoadBalancerConfigurationEvent) Create(ctx context.Co
func (h *enqueueRequestsForLoadBalancerConfigurationEvent) Update(ctx context.Context, e event.TypedUpdateEvent[*elbv2gw.LoadBalancerConfiguration], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this going under the Update handler? What should trigger it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When the the lbcfg with finalizers are attempted to delete, it triggers the update event as only the deletiontimestamp changes. hence added here so that if any residual lbfgs with finalizers gets delete attempt we try to clean these for them.

zac-nixon
zac-nixon reviewed Jun 6, 2025
View reviewed changes
controllers/gateway/gateway_class_controller.go Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we have an add / remove finalizer call in the gw class controller?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The finalizers are only added if there are any gateways referenced by gwclass referencing the lbcfg which we do in gateway controller logic. There is no need to add finalizers on lbcfg with just gatewayClass and no gateways.

zac-nixon
zac-nixon reviewed Jun 6, 2025
View reviewed changes
controllers/gateway/eventhandlers/gateway_class_events.go Outdated
@@ -49,6 +53,13 @@ func (h *enqueueRequestsForGatewayClassEvent) Update(ctx context.Context, e even

// Delete is not implemented for this handler as GatewayClass deletion should be finalized and is prevented while referenced by Gateways
func (h *enqueueRequestsForGatewayClassEvent) Delete(ctx context.Context, e event.TypedDeleteEvent[*gatewayv1.GatewayClass], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
gwClass := e.Object
h.logger.V(1).Info("enqueue gatewayclass delete event", "gatewayclass", gwClass.Name)
// remove the load balancer configuration finalizer if there are any referred by this gwclass
Copy link
Collaborator

Choose a reason for hiding this comment

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

this isn't the right place to perform the finalizer deletion. If it fails, it won't get retried.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even if its fails, when the actual deletion would occur for lbcfg, we will handle the removal of finalizers in lbcfg events as well which will succed right so they will be able to delete it eventually.

@zac-nixon zac-nixon mentioned this pull request Jun 9, 2025
Merged
6 tasks
@zac-nixon
Copy link
Collaborator

/lgtm
/approved

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2025
@k8s-ci-robot k8s-ci-robot merged commit 03de3e4 into kubernetes-sigs:main Jun 11, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zac-nixon zac-nixon zac-nixon left review comments

@M00nF1sh M00nF1sh Awaiting requested review from M00nF1sh

@oliviassss oliviassss Awaiting requested review from oliviassss

Assignees

@zac-nixon zac-nixon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shraddhabang @k8s-ci-robot @zac-nixon