Skip to content
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

fix(controllers) skip routes bound to excluded GWs #5642

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Feb 21, 2024

What this PR does / why we need it:

In single-Gateway mode, properly skip route parent references for other Gateways when determining whether to include a route.

Which issue this PR fixes:

Currently, single-Gateway mode doesn't actually restrict the controller to routes attached to that Gateway. The original work in 2e22a10 doesn't actually filter other routes for two reasons:

  • Although we change the controller map functions (via filters in listHTTPRoutesForGateway() and the like), controllers actually queue all route objects, not just those attached to matching Gateways, so that we can process deletes for routes whose Gateway no longer matches or who no longer have a matching Gateway as a parent.
  • Although the route filter had some code from the earlier commit, it's unclear how it was supposed to actually filter non-matching routes. In practice it would sorta pretend that the assigned Gateway was the parent even when it doesn't

This fix addresses the second item. We don't need to (and arguably can't--I'm pretty sure there's no other way to handle that class of deletes) address both.

Special notes for your reviewer:

Integration can't really test this because it doesn't normally filter Gateways. I used
multi-gw.diff.txt off 6e1dcfe to validate it. https://github.com/Kong/gateway-operator/pull/1480 can test it in operator integration, but that's obviously elsewhere.

I don't think the existing envtest can check this. It's already checking that there's only one reported attached route, which seems wrong. I didn't check the full path, but I think we maybe have an additional filter on reporting causing that?

E2E could check this. IDK if we want to set that up for regression on what I understand as mostly an operator feature. I don't recall when all/if we run operator tests against controller main.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (6e1dcfe) 69.3% compared to head (1f22a3e) 71.8%.
Report is 5 commits behind head on main.

Files Patch % Lines
internal/controllers/gateway/route_utils.go 60.0% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5642     +/-   ##
=======================================
+ Coverage   69.3%   71.8%   +2.4%     
=======================================
  Files        176     176             
  Lines      18159   18164      +5     
=======================================
+ Hits       12589   13044    +455     
+ Misses      4603    4171    -432     
+ Partials     967     949     -18     

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

randmonkey
randmonkey previously approved these changes Feb 21, 2024
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

If we're not adding integration/e2e test (as mentioned in the PR description) can we at least at a UT for that?

I realize that it might require some work to set up but IMHO it's worth the effort.

In single-Gateway mode, properly skip route parent references for other
Gateways when determining whether to include a route.
@rainest
Copy link
Contributor Author

rainest commented Feb 21, 2024

Okay yeah, I got tunnel vision around the end result of the route showing up in config with two actual Gateways, but you can fake the inputs in the util pretty easily.

@pmalek pmalek merged commit 17c6968 into main Feb 22, 2024
48 checks passed
@pmalek pmalek deleted the fix/single-gw-filter branch February 22, 2024 09:33
@mlavacca mlavacca added this to the KIC v3.1.x milestone Feb 27, 2024
@team-k8s-bot
Copy link
Collaborator

The backport to release/3.1.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/3.1.x release/3.1.x
# Navigate to the new working tree
cd .worktrees/backport-release/3.1.x
# Create a new branch
git switch --create backport-5642-to-release/3.1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 17c696835ead7d88db3dfaa99da5872e4f535911
# Push it to GitHub
git push --set-upstream origin backport-5642-to-release/3.1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/3.1.x

Then, create a pull request where the base branch is release/3.1.x and the compare/head branch is backport-5642-to-release/3.1.x.

@pmalek
Copy link
Member

pmalek commented Feb 28, 2024

@mlavacca The rebase PR using the backport bot failed. Do you plan to create it manually?

@pmalek pmalek mentioned this pull request Feb 28, 2024
29 tasks
mlavacca pushed a commit that referenced this pull request Feb 28, 2024
In single-Gateway mode, properly skip route parent references for other
Gateways when determining whether to include a route.
mlavacca added a commit that referenced this pull request Feb 28, 2024
* feat: managed gateways config pushed when accepted (#5662)

Signed-off-by: Mattia Lavacca <[email protected]>

* fix(controllers) skip routes bound to excluded GWs (#5642)

In single-Gateway mode, properly skip route parent references for other
Gateways when determining whether to include a route.

* chore: generate validating webhook config using controller-gen (#5659)

* tests: port KongVault validation webhook tests to envtests (#5605)

Co-authored-by: Patryk Małek <[email protected]>

---------

Signed-off-by: Mattia Lavacca <[email protected]>
Co-authored-by: Travis Raines <[email protected]>
Co-authored-by: Grzegorz Burzyński <[email protected]>
Co-authored-by: Patryk Małek <[email protected]>
mlavacca added a commit that referenced this pull request Feb 29, 2024
* feat: managed gateways config pushed when accepted (#5662)

Signed-off-by: Mattia Lavacca <[email protected]>

* fix(controllers) skip routes bound to excluded GWs (#5642)

In single-Gateway mode, properly skip route parent references for other
Gateways when determining whether to include a route.

* chore: generate validating webhook config using controller-gen (#5659)

* tests: port KongVault validation webhook tests to envtests (#5605)

Co-authored-by: Patryk Małek <[email protected]>

---------

Signed-off-by: Mattia Lavacca <[email protected]>
Co-authored-by: Travis Raines <[email protected]>
Co-authored-by: Grzegorz Burzyński <[email protected]>
Co-authored-by: Patryk Małek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants