Skip to content

Fixed validation for VSR exact & regex subroutes #4744

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 12 commits into from
Feb 6, 2024
Merged

Fixed validation for VSR exact & regex subroutes #4744

merged 12 commits into from
Feb 6, 2024

Conversation

jo-carter
Copy link
Contributor

@jo-carter jo-carter commented Dec 4, 2023 •
edited
Loading

Since a VSR may only contain a single subroute when using exact or regex matching skipping route validation here is incorrect.

Correctly configured VSRs for regex or exact will be validated (and terminate) earlier in function, ie. where VS path is regex + only 1 subroute + subroute syntax check passes.

Resolves ticket 4727

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@jo-carter jo-carter requested a review from a team as a code owner December 4, 2023 18:51
@brianehlert brianehlert linked an issue Dec 4, 2023 that may be closed by this pull request
Virtual Server validation does not reject regex subroute paths that do not match virtual server path #4727
Closed
@brianehlert brianehlert added the bug An issue reporting a potential bug label Dec 4, 2023
@haywoodsh
Copy link
Contributor

Thank you @jo-carter for your PR! Could you please add some unit test cases for the validation failures?
https://github.com/nginxinc/kubernetes-ingress/blob/main/pkg/apis/configuration/validation/virtualserver_test.go#L821

@netlify Netlify
Copy link

netlify bot commented Dec 6, 2023 •
edited
Loading

👷 Deploy request for nginx-kubernetes-ingress pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 046ab97

@github-actions github-actions bot removed the bug An issue reporting a potential bug label Dec 6, 2023
@jo-carter
Copy link
Contributor Author

@haywoodsh Sure, I've added a couple of unit tests.

@brianehlert brianehlert added the backlog Pull requests/issues that are backlog items label Dec 6, 2023
@vepatel vepatel removed the backlog Pull requests/issues that are backlog items label Jan 10, 2024
@vepatel vepatel requested review from haywoodsh and vepatel January 10, 2024 13:13
@vepatel
Copy link
Contributor

vepatel commented Jan 10, 2024

@jo-carter this change removes the capability of using regex or exact matches altogether

haywoodsh
haywoodsh reviewed Jan 15, 2024
View reviewed changes
jo-carter and others added 4 commits January 25, 2024 16:42
@jo-carter @haywoodsh
Fixed validation for VSR exact & regex subroutes
341b596 
Since a VSR may only contain a single subroute when using exact or
regex matching skipping route validation here is incorrect.

Correctly configured VSRs for regex or exact will be validated
(and terminate) earlier in function, ie. where VS path is regex +
only 1 subroute + subroute syntax check passes.
@jo-carter @haywoodsh
Added Unit Tests
5ec7ed2
@haywoodsh
Fix validation
9639638 
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
@haywoodsh
Add unit tests and update docs
ac5fb6e 
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
@jo-carter jo-carter requested a review from a team as a code owner January 25, 2024 23:18
@github-actions github-actions bot added documentation Pull requests/issues for documentation and removed documentation Pull requests/issues for documentation labels Jan 25, 2024
@jo-carter
Copy link
Contributor Author

@haywoodsh Do we need to squash / amend any of these commits ? They undo each other in places (the unit tests).

@codecov Codecov
Copy link

codecov bot commented Jan 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8a2c9a9) 52.32% compared to head (4e57469) 52.35%.
Report is 6 commits behind head on main.

❗ Current head 4e57469 differs from pull request most recent head 046ab97. Consider uploading reports for the commit 046ab97 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4744      +/-   ##
==========================================
+ Coverage   52.32%   52.35%   +0.02%     
==========================================
  Files          61       61              
  Lines       17502    17505       +3     
==========================================
+ Hits         9158     9164       +6     
+ Misses       8015     8012       -3     
  Partials      329      329

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

ADubhlaoich
ADubhlaoich reviewed Jan 29, 2024
View reviewed changes
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM for docs changes: not approving as most of the PR content is code-based.

@haywoodsh haywoodsh requested a review from a team February 1, 2024 17:43
@haywoodsh haywoodsh merged commit 3787bb8 into nginx:main Feb 6, 2024
pdabelf5 pushed a commit that referenced this pull request Feb 13, 2024
@jo-carter @haywoodsh
@vepatel @pdabelf5
Fixed validation for VSR exact & regex subroutes (#4744)
e166460 
* Fixed validation for VSR exact & regex subroutes

Since a VSR may only contain a single subroute when using exact or
regex matching skipping route validation here is incorrect.

Correctly configured VSRs for regex or exact will be validated
(and terminate) earlier in function, ie. where VS path is regex +
only 1 subroute + subroute syntax check passes.

* Added Unit Tests

* Fix validation

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>

* Add unit tests and update docs

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>

* Adjusted Unit test messages.

---------

Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
Co-authored-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
Co-authored-by: Venktesh Shivam Patel <ve.patel@f5.com>
pdabelf5 added a commit that referenced this pull request Feb 13, 2024
@pdabelf5
Fixed validation for VSR exact & regex subroutes (#4744) (#5079)
997981f 
* Fixed validation for VSR exact & regex subroutes
@haywoodsh haywoodsh mentioned this pull request Mar 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ADubhlaoich ADubhlaoich ADubhlaoich left review comments

@haywoodsh haywoodsh haywoodsh left review comments

@vepatel vepatel vepatel approved these changes

@jjngx jjngx jjngx approved these changes

Assignees
No one assigned
Labels
documentation Pull requests/issues for documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Virtual Server validation does not reject regex subroute paths that do not match virtual server path
6 participants
@jo-carter @haywoodsh @vepatel @ADubhlaoich @jjngx @brianehlert