internal/dag: Add exact path match condition in HttpProxy #5000

arjunsalyan · 2023-01-24T17:44:14Z

This adds support for exact path matching in the HttpProxy along side prefix matching

fixes #4069
Signed-off-by: Arjun Salyan iarjunsalyan@gmail.com

github-actions · 2023-01-24T17:46:17Z

Hi @arjunsalyan! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

arjunsalyan · 2023-01-24T17:50:39Z

This is by no means near completion. Still need to cover some edge cases as I feel I must have missed them, and work on the documentation. Any reviews and feedback would be very helpful. Also, please bear with me as this is my first PR.

design/httpproxy-status-conditions.md

sunjayBhatia

re the rules around merging path conditions in the tree of HTTPProxy includes:

// If the root condition is prefix, and included condition is prefix then after merging we get one prefix condition.
// If the root condition is prefix, and included condition is exact then after merging we get one exact condition.
// If the root condition is exact, and included condition is exact then after merging we get one exact condition.
// If the root condition is exact, and included condition is prefix then after merging we get one prefix condition.

an alternate interpretation of the "exact" match in this context could be that such a match is terminal, i.e. it must come as a leaf of the tree of match conditions, since as it stands above if an exact match is at the root or middle of an include tree it effectively becomes a prefix match. if we want to keep these things distinct/clear that might be a way to go

internal/dag/httpproxy_processor.go

internal/dag/conditions.go

sunjayBhatia · 2023-02-02T00:16:40Z

Thanks for the PR @arjunsalyan I think this is a good start, but we will definitely need some more unit test coverage, we can give pointers where to look to add these if you like

we should already have logic further in our processing logic to sort the generated routes properly so that exact matches are considered by Envoy first, so this should mostly be covered by httpproxy processor/dag tests and some end to end tests

arjunsalyan · 2023-02-04T13:04:14Z

re the rules around merging path conditions in the tree of HTTPProxy includes:
// If the root condition is prefix, and included condition is prefix then after merging we get one prefix condition.
// If the root condition is prefix, and included condition is exact then after merging we get one exact condition.
// If the root condition is exact, and included condition is exact then after merging we get one exact condition.
// If the root condition is exact, and included condition is prefix then after merging we get one prefix condition.
an alternate interpretation of the "exact" match in this context could be that such a match is terminal, i.e. it must come as a leaf of the tree of match conditions, since as it stands above if an exact match is at the root or middle of an include tree it effectively becomes a prefix match. if we want to keep these things distinct/clear that might be a way to go

Agreed @sunjayBhatia, If you see the next line ahead of these comments I have mentioned the interpretation. These comments are just to explain why we reached the conclusion that the leaf decides the path type.

// Hence this can be concluded that the last rule in the delegation chain decides the path type

Will improve the wording of this statement. Let me know if you meant something else.

sunjayBhatia · 2023-02-06T14:50:34Z

re the rules around merging path conditions in the tree of HTTPProxy includes:
// If the root condition is prefix, and included condition is prefix then after merging we get one prefix condition.
// If the root condition is prefix, and included condition is exact then after merging we get one exact condition.
// If the root condition is exact, and included condition is exact then after merging we get one exact condition.
// If the root condition is exact, and included condition is prefix then after merging we get one prefix condition.
an alternate interpretation of the "exact" match in this context could be that such a match is terminal, i.e. it must come as a leaf of the tree of match conditions, since as it stands above if an exact match is at the root or middle of an include tree it effectively becomes a prefix match. if we want to keep these things distinct/clear that might be a way to go
Agreed @sunjayBhatia, If you see the next line ahead of these comments I have mentioned the interpretation. These comments are just to explain why we reached the conclusion that the leaf decides the path type.
// Hence this can be concluded that the last rule in the delegation chain decides the path type
Will improve the wording of this statement. Let me know if you meant something else.

Above I meant that an option we can take is to not allow and mark an HTTPProxy as Invalid if you try to use an exact match in an include match condition

arjunsalyan · 2023-02-07T08:17:25Z

Above I meant that an option we can take is to not allow and mark an HTTPProxy as Invalid if you try to use an exact match in an include match condition

Got it. I have some subjective thoughts here, especially for the case of delegation of a path to teams. Included routes should have the flexibility to use exact matches. But open to any other views on this.

I have added unit tests and e2e trying to cover most of the things. Will start with documentation once these things come to good shape.

arjunsalyan · 2023-02-10T14:08:02Z

For now I have added the docs considering my initial theory about routes delegation, just to make this PR ready for a review. Although we can revisit it.

sunjayBhatia

I think my interpretation of "Exact" path matching in conjunction with the Inclusion feature is that if you are using Inclusion, you really want a "Prefix" match (because the Included child proxy can then do different matching) so "Exact" matches should be disallowed on the path match conditions on an Include

cc @skriss @tsaarni if you have any thoughts

skriss · 2023-03-01T17:02:01Z

I think my interpretation of "Exact" path matching in conjunction with the Inclusion feature is that if you are using Inclusion, you really want a "Prefix" match (because the Included child proxy can then do different matching) so "Exact" matches should be disallowed on the path match conditions on an Include

cc @skriss @tsaarni if you have any thoughts

Yeah, that is how I had been thinking about it too, should be disallowed as an Include condition.

arjunsalyan · 2023-03-01T20:16:45Z

I think my interpretation of "Exact" path matching in conjunction with the Inclusion feature is that if you are using Inclusion, you really want a "Prefix" match (because the Included child proxy can then do different matching) so "Exact" matches should be disallowed on the path match conditions on an Include
cc @skriss @tsaarni if you have any thoughts

Yeah, that is how I had been thinking about it too, should be disallowed as an Include condition.

Alright, I am starting to see the point here. But before we reach a conclusion, please allow me to put one last statement in the favour of the proposed merging algorithm.

Since we have a strict rule of one fqdn - one httpproxy, inclusion becomes the only way to route traffic to services in different namespaces from the same fqdn. To adopt a general approach, we currently follow a methodology where we create an orphan httpproxy without any rules in all of the namespaces. Then we create a root httpproxy in one of the namespaces and define all the rules in that root httpproxy. Not having the exact condition in include takes away this flexibility.

There are definitely other ways to handle this, but just putting a use case.

Anyways, I am now aligned on not allowing the Exact in the Include condition. Will push the changes soon.

arjunsalyan · 2023-03-15T11:57:54Z

Hi @skriss @sunjayBhatia

The changes for disallowing the exact condition except for the child/leaf httproxy are done.

sunjayBhatia · 2023-04-04T22:56:05Z

test/e2e/httpproxy/include_exact_condition_test.go

+				},
+			},
+		}
+		invalidRootProxy := &contourv1.HTTPProxy{


i think it's ok to omit testing this in e2e tests, should instead get coverage in the dag package tests

sunjayBhatia · 2023-04-04T23:00:54Z

site/content/docs/main/config/request-routing.md

+Paths defined are matched using exact conditions.
+Up to one exact condition may be present in any condition block. Any condition block can
+either have an exact condition or prefix condition, but not both together. Exact conditions are
+not allowed in root httpproxies or in includes blocks.


Suggested change

not allowed in root httpproxies or in includes blocks.

only allowed in route match conditions and not in include match conditions.

sunjayBhatia · 2023-04-04T23:02:09Z

site/content/docs/main/config/inclusion-delegation.md

 To resolve this Contour applies the following logic.

 - `prefix:` conditions are concatenated together in the order they were applied from the root object. For example the conditions, `prefix: /api`, `prefix: /v1` becomes a single `prefix: /api/v1` conditions. Note: Multiple prefixes cannot be supplied on a single set of Route conditions.
+- `exact:` conditions are also concatenated just like `prefix:` conditions, but `exact:` conditions are not allowed in the root httpproxy or in the inlcusion block. If the child httpproxy has `exact:` condition then after concatenation, it becomes a single `exact:` condition. For example, `prefix: /static` and `exact: /main.js` become a single `exact: /static/main.js` condition.


Suggested change

- `exact:` conditions are also concatenated just like `prefix:` conditions, but `exact:` conditions are not allowed in the root httpproxy or in the inlcusion block. If the child httpproxy has `exact:` condition then after concatenation, it becomes a single `exact:` condition. For example, `prefix: /static` and `exact: /main.js` become a single `exact: /static/main.js` condition.

- `exact:` conditions are also concatenated just like `prefix:` conditions, but `exact:` conditions are not allowed in include match conditions. If the child httpproxy has `exact:` condition then after concatenation, it becomes a single `exact:` condition. For example, `prefix: /static` and `exact: /main.js` become a single `exact: /static/main.js` condition.

apis/projectcontour/v1/httpproxy.go

internal/dag/httpproxy_processor.go

arjunsalyan · 2023-04-14T14:56:36Z

@sunjayBhatia Added the generated sources, which were leading to the failed test.

arjunsalyan · 2023-04-14T15:25:01Z

This is strange

Build and Test Pull Request / test-linux (pull_request)
Build and Test Pull Request / test-osx (pull_request)

These tests pass all fine when I run them locally. Looking into it

sunjayBhatia · 2023-04-14T15:45:02Z

This is strange
Build and Test Pull Request / test-linux (pull_request)
Build and Test Pull Request / test-osx (pull_request)
These tests pass all fine when I run them locally. Looking into it

you'll want to merge/rebase on main, and update the port in that test (github is merging the PR with the base branch when checking out the code)

This adds support for exact path matching in the HttpProxy along side prefix matching Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

…ical Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

arjunsalyan · 2023-04-14T15:55:33Z

you'll want to merge/rebase on main, and update the port in that test (github is merging the PR with the base branch when checking out the code)

Ah! The tests were updated. Fixed, thanks.

codecov · 2023-04-14T16:02:25Z

Codecov Report

Merging #5000 (b673ca1) into main (5b4f4eb) will increase coverage by 0.04%.
The diff coverage is 91.80%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5000      +/-   ##
==========================================
+ Coverage   77.74%   77.79%   +0.04%     
==========================================
  Files         139      139              
  Lines       18073    18121      +48     
==========================================
+ Hits        14051    14097      +46     
- Misses       3750     3753       +3     
+ Partials      272      271       -1

Impacted Files	Coverage Δ
internal/dag/httpproxy_processor.go	`92.27% <68.75%> (-0.07%)`	⬇️
internal/dag/conditions.go	`96.60% <100.00%> (+0.43%)`	⬆️
internal/dag/dag.go	`96.71% <100.00%> (+0.06%)`	⬆️

sunjayBhatia · 2023-04-14T19:46:15Z

test/e2e/httpproxy/exact_path_condition_match_test.go

+
+		f.CreateHTTPProxyAndWaitFor(serviceProxy, e2e.HTTPProxyValid)
+
+		cases := map[string]string{


nit: I think this looks correct, but it's not super intuitive to read

not a huge deal but would be nice to either add some comments above some of the cases or use naming of the routes/services that make it clearer

Can agree to that, it is just the nature of a few cases that I could not find better naming. Have tried something with comments, not sure how much value that adds though.

sunjayBhatia · 2023-04-14T19:47:05Z

test/e2e/httpproxy/include_exact_condition_test.go

+		f.CreateHTTPProxyAndWaitFor(baseProxy, e2e.HTTPProxyValid)
+		f.CreateHTTPProxyAndWaitFor(invalidRootProxy, e2e.HTTPProxyInvalid)
+
+		cases := map[string]string{


similar nit to the above on comments/naming to make these test cases super clear (looks correct though)

sunjayBhatia · 2023-04-14T19:50:46Z

internal/dag/builder_test.go

@@ -14958,6 +15090,9 @@ func listeners(ls ...*Listener) []*Listener {
 func prefixString(prefix string) MatchCondition {
 	return &PrefixMatchCondition{Prefix: prefix, PrefixMatchType: PrefixMatchString}
 }
+func exactString(exact string) MatchCondition {


nit: the prefixString was named since it represented a prefix match that would basically match a path using "string prefix" vs. matching path segments, we could change this to just exact here

Makes sense, rather exact() is already there making it completely redundant.

sunjayBhatia

looks good overall, just some tiny nits

approving but will leave for another maintainer to have a pass over as well before merge

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

exactString(exact string) is redundant and does the same as exact(path string) Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

skriss

LGTM, thanks @arjunsalyan!

arjunsalyan requested a review from a team as a code owner January 24, 2023 17:44

arjunsalyan requested review from tsaarni and sunjayBhatia and removed request for a team January 24, 2023 17:44

arjunsalyan mentioned this pull request Jan 24, 2023

Exact path match for HTTPProxy #4069

Closed

sunjayBhatia reviewed Feb 1, 2023

View reviewed changes

design/httpproxy-status-conditions.md Outdated Show resolved Hide resolved

sunjayBhatia reviewed Feb 2, 2023

View reviewed changes

internal/dag/httpproxy_processor.go Outdated Show resolved Hide resolved

sunjayBhatia reviewed Feb 2, 2023

View reviewed changes

internal/dag/conditions.go Show resolved Hide resolved

sunjayBhatia requested a review from skriss February 2, 2023 00:12

arjunsalyan force-pushed the httpproxy/exact-match branch 2 times, most recently from 823e20d to 3d26269 Compare February 7, 2023 08:12

arjunsalyan requested review from sunjayBhatia and removed request for tsaarni and skriss February 10, 2023 14:08

sunjayBhatia requested review from skriss and tsaarni February 13, 2023 14:52

sunjayBhatia reviewed Feb 27, 2023

View reviewed changes

arjunsalyan force-pushed the httpproxy/exact-match branch 2 times, most recently from 1dad0b2 to 29ccfcf Compare March 15, 2023 11:54

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 4, 2023

sunjayBhatia removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 4, 2023

sunjayBhatia reviewed Apr 4, 2023

View reviewed changes

sunjayBhatia self-requested a review April 14, 2023 14:27

arjunsalyan force-pushed the httpproxy/exact-match branch from 0636f94 to d7e30b3 Compare April 14, 2023 14:56

arjunsalyan added 11 commits April 14, 2023 21:18

internal/dag: Add exact path match condition in HttpProxy

3775eeb

This adds support for exact path matching in the HttpProxy along side prefix matching Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

internal/dag: Add unit tests for exact path condition

724ae82

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

test/e2e/httpproxy: Add e2e tests for exact path condition

8bb86cf

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

site/content/docs: Add documentation for exact condition

38ebad5

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

changelogs: Add changelog for exact match condition

ced7fbf

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

internal/dag: Disallow exact match condition in inclusion

de8509f

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

HTTPProxy: Improve comment for exact path match field

99cba51

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

site/content/docs: Improve docs for exact path match condition

0a30cd9

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

internal/dag: Add unit tests for exact path match condition

18f5c0d

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

internal/dag: Remove redundant check from includeMatchConditionsIdent…

065ed97

…ical Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

Exact match: Add generated sources

56fd244

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

arjunsalyan force-pushed the httpproxy/exact-match branch from d7e30b3 to 56fd244 Compare April 14, 2023 15:54

sunjayBhatia reviewed Apr 14, 2023

View reviewed changes

sunjayBhatia approved these changes Apr 14, 2023

View reviewed changes

arjunsalyan added 2 commits April 15, 2023 02:12

e2e/httpproxy: Improve exact match testcases with comments

c1c59fc

Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

internal/dag/builder_test: Remove redundant func

b673ca1

exactString(exact string) is redundant and does the same as exact(path string) Signed-off-by: Arjun Salyan <iarjunsalyan@gmail.com>

skriss approved these changes Apr 26, 2023

View reviewed changes

skriss merged commit 8946933 into projectcontour:main Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/dag: Add exact path match condition in HttpProxy #5000

internal/dag: Add exact path match condition in HttpProxy #5000

arjunsalyan commented Jan 24, 2023

github-actions bot commented Jan 24, 2023

arjunsalyan commented Jan 24, 2023

sunjayBhatia left a comment

sunjayBhatia commented Feb 2, 2023

arjunsalyan commented Feb 4, 2023 •

edited

Loading

sunjayBhatia commented Feb 6, 2023

arjunsalyan commented Feb 7, 2023

arjunsalyan commented Feb 10, 2023

sunjayBhatia left a comment

skriss commented Mar 1, 2023

arjunsalyan commented Mar 1, 2023

arjunsalyan commented Mar 15, 2023

sunjayBhatia Apr 4, 2023

sunjayBhatia Apr 4, 2023

sunjayBhatia Apr 4, 2023

arjunsalyan commented Apr 14, 2023

arjunsalyan commented Apr 14, 2023

sunjayBhatia commented Apr 14, 2023

arjunsalyan commented Apr 14, 2023 •

edited

Loading

codecov bot commented Apr 14, 2023 •

edited

Loading

sunjayBhatia Apr 14, 2023

arjunsalyan Apr 14, 2023

sunjayBhatia Apr 14, 2023

sunjayBhatia Apr 14, 2023

arjunsalyan Apr 14, 2023

sunjayBhatia left a comment

skriss left a comment

	not allowed in root httpproxies or in includes blocks.
	only allowed in route match conditions and not in include match conditions.

	- `exact:` conditions are also concatenated just like `prefix:` conditions, but `exact:` conditions are not allowed in the root httpproxy or in the inlcusion block. If the child httpproxy has `exact:` condition then after concatenation, it becomes a single `exact:` condition. For example, `prefix: /static` and `exact: /main.js` become a single `exact: /static/main.js` condition.
	- `exact:` conditions are also concatenated just like `prefix:` conditions, but `exact:` conditions are not allowed in include match conditions. If the child httpproxy has `exact:` condition then after concatenation, it becomes a single `exact:` condition. For example, `prefix: /static` and `exact: /main.js` become a single `exact: /static/main.js` condition.


		f.CreateHTTPProxyAndWaitFor(serviceProxy, e2e.HTTPProxyValid)

		cases := map[string]string{

internal/dag: Add exact path match condition in HttpProxy #5000

internal/dag: Add exact path match condition in HttpProxy #5000

Conversation

arjunsalyan commented Jan 24, 2023

github-actions bot commented Jan 24, 2023

arjunsalyan commented Jan 24, 2023

sunjayBhatia left a comment

Choose a reason for hiding this comment

sunjayBhatia commented Feb 2, 2023

arjunsalyan commented Feb 4, 2023 • edited Loading

sunjayBhatia commented Feb 6, 2023

arjunsalyan commented Feb 7, 2023

arjunsalyan commented Feb 10, 2023

sunjayBhatia left a comment

Choose a reason for hiding this comment

skriss commented Mar 1, 2023

arjunsalyan commented Mar 1, 2023

arjunsalyan commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjunsalyan commented Apr 14, 2023

arjunsalyan commented Apr 14, 2023

sunjayBhatia commented Apr 14, 2023

arjunsalyan commented Apr 14, 2023 • edited Loading

codecov bot commented Apr 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

arjunsalyan commented Feb 4, 2023 •

edited

Loading

arjunsalyan commented Apr 14, 2023 •

edited

Loading

codecov bot commented Apr 14, 2023 •

edited

Loading