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: make regex for hosts consistent with APISIX #2065

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Revolyssup
Copy link
Contributor

Type of change:

Fixes #2064
Currently there is a mismatch between the regex that APISIX allowed[1] and what ApisixRoute CRD allowed. This PR makes sure that both are consistent.
More specifically - Both 1.2.3.4 and 1.2.3.4:443 should be considered valid hosts field

  1. "pattern": "^\\*?[0-9a-zA-Z-._\\[\\]:]+$",
  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches
  • Documentation
  • Refactor
  • Chore
  • CI/CD or Tests

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@Revolyssup Revolyssup added this to the 1.8.0 milestone Nov 22, 2023
Signed-off-by: Ashish Tiwari <[email protected]>
@Revolyssup Revolyssup added the bugfix pull requests that fix a bug label Nov 23, 2023
@AlinsRan
Copy link
Contributor

Is there a problem with the admin-api mode?
We need test cases to cover it.

@Revolyssup
Copy link
Contributor Author

Is there a problem with the admin-api mode? We need test cases to cover it.

The admin api is fine. The regex of the same field that admin api expects and what ingress controller expects is different. So this PR makes sure that regex is same for the same field.
Okay I'll add a test case.

Signed-off-by: Ashish Tiwari <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2023

Codecov Report

Attention: Patch coverage is 60.62500% with 63 lines in your changes are missing coverage. Please review.

Project coverage is 37.38%. Comparing base (c111c12) to head (1e9c7dc).
Report is 33 commits behind head on master.

❗ Current head 1e9c7dc differs from pull request most recent head 93afbd5. Consider uploading reports for the commit 93afbd5 to get more accurate results

Files Patch % Lines
pkg/providers/ingress/translation/translator.go 29.82% 38 Missing and 2 partials ⚠️
...gress/translation/annotations/upstream/upstream.go 83.78% 3 Missing and 3 partials ⚠️
pkg/providers/apisix/translation/apisix_route.go 60.00% 3 Missing and 1 partial ⚠️
pkg/providers/utils/insert_map.go 75.00% 2 Missing and 2 partials ⚠️
pkg/providers/apisix/translation/apisix_ssl.go 0.00% 3 Missing ⚠️
pkg/providers/ingress/ingress.go 0.00% 3 Missing ⚠️
cmd/ingress/ingress.go 87.50% 1 Missing and 1 partial ⚠️
...roviders/apisix/translation/apisix_pluginconfig.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2065      +/-   ##
==========================================
+ Coverage   36.81%   37.38%   +0.56%     
==========================================
  Files          93       94       +1     
  Lines        7863     7971     +108     
==========================================
+ Hits         2895     2980      +85     
- Misses       4580     4597      +17     
- Partials      388      394       +6     

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

Signed-off-by: Ashish Tiwari <[email protected]>
Signed-off-by: Ashish Tiwari <[email protected]>
@Revolyssup
Copy link
Contributor Author

This issue needs to be fixed first for the CI to pass here api7/lua-resty-radixtree#138

- name: rule1
match:
hosts:
- httpbin.org
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this use case seem unrelated to PR? What about the port?

@Revolyssup
Copy link
Contributor Author

Waiting on next apisix release with this chaange apache/apisix#10861

@yzeng25 yzeng25 requested a review from AlinsRan May 23, 2024 06:47
@Revolyssup Revolyssup requested review from AlinsRan and shreemaan-abhishek and removed request for AlinsRan September 4, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull requests that fix a bug
Projects
Status: In Progress
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

bug: Host matching cannot carry ports
4 participants