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

Simplify regex path match to reduce program size #4197

Merged

Conversation

sunjayBhatia
Copy link
Member

Ingress/HTTPRoute prefix matches now have an anchor to ensure long
prefixes don't increase the program size. Should ensure they are not
rejected by Envoy.

In addition, add an anchor to regex path matches since they at least
have to start with a literal /

Manually ran ingress conformance tests and besides flakes, all passed 👍🏽

Updates: #4191

Ingress/HTTPRoute prefix matches now have an anchor to ensure long
prefixes don't increase the program size. Should ensure they are not
rejected by Envoy.

In addition, add an anchor to regex path matches since they at least
have to start with a literal /

Updates: projectcontour#4191

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia sunjayBhatia requested a review from a team as a code owner November 24, 2021 20:31
@sunjayBhatia sunjayBhatia requested review from tsaarni and skriss and removed request for a team November 24, 2021 20:31
@sunjayBhatia sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Nov 24, 2021
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #4197 (fc27ba6) into main (1673fd1) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4197   +/-   ##
=======================================
  Coverage   75.99%   75.99%           
=======================================
  Files         111      111           
  Lines        9785     9785           
=======================================
  Hits         7436     7436           
  Misses       2181     2181           
  Partials      168      168           
Impacted Files Coverage Δ
internal/envoy/v3/route.go 66.49% <100.00%> (ø)
internal/featuretests/v3/envoy.go 100.00% <100.00%> (ø)

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM (relying on those ingress conformance tests to validate the regex itself).

@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Dec 6, 2021

LGTM (relying on those ingress conformance tests to validate the regex itself).

yeah, need to get those running in CI but theyre pretty flaky as-is, need to add some retries to the requests or something similar

@sunjayBhatia sunjayBhatia merged commit 4027405 into projectcontour:main Dec 6, 2021
@sunjayBhatia sunjayBhatia deleted the ingress-long-prefix-path-regex branch December 6, 2021 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants