Skip to content

fix: httproute should enqueue requests when gateways are deleted #660

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.

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
merged 1 commit into from
Jun 20, 2025

Conversation

jonstacks
Copy link
Collaborator

What

While working on #658, I found a bug where we weren't triggering a reconcile for httproutes when one of their parentRef gateways changed. When the gateway was deleted, it wasn't reflecting in the http route status conditions properly.

How

  • In the HTTPRoute reconciler, create a watch on gateways and when one changes, find all http routes that reference that gateway and trigger a reconcile for them.
  • Reduce the timeout in the httproute test. This was originally bumped while I couldn't figure out why the test was flakey. Now that we correctly reconcile on gateway deletions, we can move the timeout back down to a lower value.

Breaking Changes

No

@jonstacks jonstacks self-assigned this Jun 20, 2025
@jonstacks jonstacks requested a review from a team as a code owner June 20, 2025 20:47
@github-actions github-actions bot added the area/controller Issues dealing with the controller label Jun 20, 2025
@jonstacks jonstacks requested a review from Copilot June 20, 2025 20:52
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures HTTPRoutes are reconciled when their parent Gateways change (including deletions) and restores a lower, consistent timeout in tests.

  • Added a watch on Gateway resources to enqueue affected HTTPRoutes via findHTTPRouteForGateway.
  • Introduced findHTTPRouteForGateway mapping function that lists HTTPRoutes referencing the changed Gateway.
  • Updated the HTTPRoute controller test to use the shared timeout variable instead of a hardcoded 30s.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
internal/controller/gateway/httproute_controller.go Added Gateway watch & mapping, adjusted validation error handling
internal/controller/gateway/httproute_controller_test.go Switched test from hardcoded 30s to shared timeout variable

Copy link

codecov bot commented Jun 20, 2025

Codecov Report

Attention: Patch coverage is 47.54098% with 32 lines in your changes missing coverage. Please review.

Project coverage is 30.85%. Comparing base (cbe6362) to head (09fd888).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nternal/controller/gateway/httproute_controller.go 47.54% 28 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #660      +/-   ##
==========================================
+ Coverage   30.83%   30.85%   +0.01%     
==========================================
  Files         101      101              
  Lines       15925    15984      +59     
==========================================
+ Hits         4911     4932      +21     
- Misses      10729    10762      +33     
- Partials      285      290       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonstacks jonstacks added this pull request to the merge queue Jun 20, 2025
Merged via the queue into ngrok:main with commit e98a4fc Jun 20, 2025
10 checks passed
@jonstacks jonstacks deleted the fix-httproute-watch branch June 20, 2025 21:07
@jonstacks jonstacks added this to the controller-0.17.0 milestone Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Issues dealing with the controller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants