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

Case sensitive route match #19647

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Conversation

Lord-Y
Copy link
Contributor

@Lord-Y Lord-Y commented Nov 15, 2023

Description

Sometimes we need route to be case insensitive when using prefix in order to apply some rules later.
Envoy is supporting this feature since a while and can be found here all the way down to version 1.22.x

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@Lord-Y Lord-Y requested a review from a team as a code owner November 15, 2023 12:32
@github-actions github-actions bot added type/docs Documentation needs to be created/updated/clarified theme/api Relating to the HTTP API interface theme/envoy/xds Related to Envoy support labels Nov 15, 2023
@Lord-Y Lord-Y requested a review from a team as a code owner December 14, 2023 15:29
@Lord-Y Lord-Y requested a review from a team December 14, 2023 15:29
@Lord-Y Lord-Y requested a review from a team as a code owner December 14, 2023 15:29
@Lord-Y Lord-Y requested review from sarahethompson and emilymianeil and removed request for a team December 14, 2023 15:29
@Lord-Y
Copy link
Contributor Author

Lord-Y commented Dec 14, 2023

@david-yu Can you give me a help on this PR? As we have a Consul license + support with my enterprise we tried with our account owner/CSM but no luck. You can find it in my Zendesk ticket 131683.

@jkirschner-hashicorp
Copy link
Contributor

@Lord-Y : Acknowledging that I saw your message. Thank you for updating this PR against main. @david-yu / I will follow-up on this.

Copy link
Member

@hashi-derek hashi-derek left a comment

Choose a reason for hiding this comment

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

This looks great so far -- thanks for the contribution. I think the main question here is about whether there's an incompatibility between the new field and regex, but otherwise, I don't see any issues.

@Lord-Y
Copy link
Contributor Author

Lord-Y commented Jan 12, 2024

@hashi-derek @jkirschner-hashicorp let me know if I need to change something after my last commits.

@hashi-derek
Copy link
Member

@Lord-Y Hey, sorry for the delay. Yeah, it looks like the CICD is complaining about a missing import during the linting step, which is preventing some tests from running.

  Error: could not import github.com/hashicorp/consul/agent/xds (-: # github.com/hashicorp/consul/agent/xds
  Error: agent/xds/routes.go:863:22: undefined: wrapperspb) (typecheck)
  Error: "github.com/hashicorp/consul/agent/proxycfg-sources/catalog" imported as catalogproxycfg and not used (typecheck)
  Error: : # github.com/hashicorp/consul/agent/xds [github.com/hashicorp/consul/agent/xds.test]
  Error: agent/xds/routes.go:863:22: undefined: wrapperspb (typecheck)
  Error: could not import github.com/hashicorp/consul/agent/xds (-: # github.com/hashicorp/consul/agent/xds
  Error: agent/xds/routes.go:863:22: undefined: wrapperspb) (typecheck)
  Error: could not import github.com/hashicorp/consul/agent/xds (-: # github.com/hashicorp/consul/agent/xds
  Error: agent/xds/routes.go:863:22: undefined: wrapperspb) (typecheck)

@Lord-Y
Copy link
Contributor Author

Lord-Y commented Jan 18, 2024

@Lord-Y Hey, sorry for the delay. Yeah, it looks like the CICD is complaining about a missing import during the linting step, which is preventing some tests from running.

  Error: could not import github.com/hashicorp/consul/agent/xds (-: # github.com/hashicorp/consul/agent/xds
  Error: agent/xds/routes.go:863:22: undefined: wrapperspb) (typecheck)
  Error: "github.com/hashicorp/consul/agent/proxycfg-sources/catalog" imported as catalogproxycfg and not used (typecheck)
  Error: : # github.com/hashicorp/consul/agent/xds [github.com/hashicorp/consul/agent/xds.test]
  Error: agent/xds/routes.go:863:22: undefined: wrapperspb (typecheck)
  Error: could not import github.com/hashicorp/consul/agent/xds (-: # github.com/hashicorp/consul/agent/xds
  Error: agent/xds/routes.go:863:22: undefined: wrapperspb) (typecheck)
  Error: could not import github.com/hashicorp/consul/agent/xds (-: # github.com/hashicorp/consul/agent/xds
  Error: agent/xds/routes.go:863:22: undefined: wrapperspb) (typecheck)

I don't know this happened as I didn't do anything special. I updated the code with last commits. Let's see how it's going to be.

@hashi-derek
Copy link
Member

@Lord-Y I pushed a commit to your repo to fix the import issue and added in a changelog. All the tests passed, so thank you for the contribution.

@david-yu
Copy link
Contributor

@Lord-Y What version of Consul enterprise are you using? We may need to consider backporting this but would like to check.

@rrondeau
Copy link
Contributor

Hi
We are using 1.17 since last week in production.
@Lord-Y and i work together.
Thanks for fixing the test suite.

@hashi-derek hashi-derek added backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. and removed pr/no-backport labels Jan 18, 2024
@hashi-derek
Copy link
Member

This PR needs to be squashed for backporting purposes, and I will have to rebase the commits so that code from main doesn't sneak into 1.17 (due to the several merge commits in the middle of it). I will address this tomorrow, but figured I should point this out in case you see some activity in @Lord-Y 's branch.

This commit adds in a new feature that allows service routers to specify that
paths and path prefixes should ignore upper / lower casing when matching URLs.

Co-authored-by: Derek Menteer <105233703+hashi-derek@users.noreply.github.com>
@hashi-derek hashi-derek merged commit 758ddf8 into hashicorp:main Jan 22, 2024
87 of 89 checks passed
@david-yu
Copy link
Contributor

@Lord-Y It looks like this just missed the cut for 1.17.2 which is coming out this week, this will be delivered as part of 1.17.3. Sorry for the delay here, but I hope this ok timing wise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.17 This release series is no longer active on CE. Use backport/ent/1.17. theme/api Relating to the HTTP API interface theme/envoy/xds Related to Envoy support type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants