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

Fixed validation for VSR exact & regex subroutes #4744

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

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 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

Copy link

netlify bot commented Dec 6, 2023

👷 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
Copy link
Contributor

vepatel commented Jan 10, 2024

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

jo-carter and others added 4 commits January 25, 2024 16:42
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.
Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com>
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).

Copy link

codecov bot commented Jan 26, 2024

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.

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 nginxinc:main Feb 6, 2024
72 checks passed
pdabelf5 pushed a commit that referenced this pull request Feb 13, 2024
* 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
* Fixed validation for VSR exact & regex subroutes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation
Projects
Archived in project
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