Add Inclusion documentation for HTTPProxy #1573

youngnick · 2019-09-25T04:39:36Z

Fixes #1462

Add link to HTTPProxy documentation to upgrading.md, fixes #1476

Signed-off-by: Nick Young [email protected]

davecheney

Thank you for working on this. Just a bunch of nit picking comments but I think this is good to go on the next round.

docs/httpproxy.md

davecheney · 2019-09-25T06:21:16Z

Ok, my mistake.

…

On 25 Sep 2019, at 16:10, Nick Young ***@***.***> wrote: @youngnick commented on this pull request. In docs/httpproxy.md: > +Because the path is not necessarily used as the only key, the route space can be multi-dimensional. + +### Conditions and Inclusion + +Inclusions may be specified with conditions. +These conditions are added to any conditions in the included HTTPProxy. +This process repeated for any further inclusions as well. + +All [Conditions](#conditions) are supported for use with inclusion. +The `prefix` condition is slightly special in that, when used for inclusion, any routes will have the value of `prefix` prepended to *their* prefix value. +Header conditions are added to the conditions array. + +### Configuring inclusion + +Inclusion is a top-level part of the HTTPProxy `spec` element. +It requires two fields, `name` and `namespace`, with the option of including a `conditions` block. It's definitely required at the moment. I don't think it should be, but wanted to document current state. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

stevesloka · 2019-09-25T13:04:03Z

docs/httpproxy.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.
+- Repeated identical `header:` conditions (the same `name`, match type, and `invert` values) are deduplicated to a single `header:` condition.


The invert language is abstracted away from users. They only see exact or notexact (or contains, notcontains) to set that flag.

Headers aren't merged either. We should discuss what should happen in this case. Should there be some sort of merge? Throw an error back to the user? Or just let them all stack ending up in a potential situation where requests won't match since the route is too specific.

For now, I think they need to just be there, and we say that tautological conditions are an error.

Identical headers will end up deduplicated though, right?

I'll change the invert part, yes.

Currently, master the headers are all just smashed together, so if someone duplicates headers, the route must match all the defined headers to route properly.

There's an issue open (#1559), which I thought we would define an error if headers were duplicated. Thoughts on that approach? I'm not sure if use-cases exist where a route should match multiple headers of the same key with different values.

I meant that they are deduplicated at the end when applied to Envoy - since setting a correctly-formed header is idempotent on a route (I assume). I think that contradictory headers should be an error, but exact matches (where the header name, value, and operator are the same)? That doesn't feel like an error to me.

In any case, #1559 is probably the place for this discussion. Can we accept that this is the case for now, and get this one in?

It's fine to merge, just want to be careful that we don't explain functionality that doesn't exist or could change and we confuse folks right at the beginning.

stevesloka · 2019-09-25T13:06:34Z

docs/httpproxy.md

+HTTPProxies can include other HTTPProxy objects in the namespace by specifying the name of the object and its namespace in the top-level `includes` block.
+Note that `includes` is a list, and so it must use the YAML list construct.
+
+In this example, the HTTPProxy `delegation-root` has included the configuration for paths matching `/service2` from the HTTPPRoxy named `service2` in the same namespace as `delegation-root` (the `default` namespace).


I still use "delegation" in speaking about "include", but should we try and stray away? I'm not sure if it's confusing to users who know IngressRoute or not. In a sense, we're performing the same operation but in a different way. This is just me thinking out loud.

I should change the names, so that it's consistent, at least. Thanks.

I think that they're functionally the same, and it's probably okay in conversation, but I was trying to avoid being confusing in this reference documentation.

stevesloka · 2019-09-25T13:07:42Z

docs/httpproxy.md

+Note that `includes` is a list, and so it must use the YAML list construct.
+
+In this example, the HTTPProxy `delegation-root` has included the configuration for paths matching `/service2` from the HTTPPRoxy named `service2` in the same namespace as `delegation-root` (the `default` namespace).
+It's important to note that `service2` HTTPProxy has not defined a `virtualhost` property as it is NOT a root HTTPProxy.


s/service2/www/g ??

Yeah, too much copypasta.

stevesloka · 2019-09-25T13:09:40Z

docs/httpproxy.md

+
+#### Virtualhost aliases
+
+To present the same set of routes under multiple dns entries, for example www.example.com and example.com, including a service with a `prefix` condition of `/` can be used.


Another way to do this so that you don't have to repeat the includes section twice is to have each root include to a single Proxy, then that one includes a child proxy. You end up with an additional Proxy object, but changes to the includes that are passed down are in a single place.

Your example is 100% correct, again just me typing out loud.

I think it's okay to be a bit more verbose in this, since it's a reference guide.

Fixes projectcontour#1462 Update upgrading.md with HTTProxy docs, fixes projectcontour#1476 Signed-off-by: Nick Young <[email protected]>

davecheney

LGTM. Thank you. I'll merge this now.

davecheney suggested changes Sep 25, 2019

View reviewed changes

youngnick force-pushed the 1462-inclusion-docs branch from 591137a to ff528cc Compare September 25, 2019 06:49

youngnick requested a review from davecheney September 25, 2019 06:54

stevesloka suggested changes Sep 25, 2019

View reviewed changes

youngnick force-pushed the 1462-inclusion-docs branch from ff528cc to 3d5063d Compare September 26, 2019 00:16

youngnick requested a review from stevesloka September 26, 2019 00:17

youngnick force-pushed the 1462-inclusion-docs branch 3 times, most recently from 4836a4b to 6dcee5b Compare September 26, 2019 00:24

Add Inclusion documentation for HTTPProxy

b4e862b

Fixes projectcontour#1462 Update upgrading.md with HTTProxy docs, fixes projectcontour#1476 Signed-off-by: Nick Young <[email protected]>

youngnick force-pushed the 1462-inclusion-docs branch from 6dcee5b to b4e862b Compare September 26, 2019 00:34

davecheney approved these changes Sep 26, 2019

View reviewed changes

davecheney merged commit f27bc5b into projectcontour:master Sep 26, 2019

youngnick deleted the 1462-inclusion-docs branch April 27, 2020 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Inclusion documentation for HTTPProxy #1573

Add Inclusion documentation for HTTPProxy #1573

youngnick commented Sep 25, 2019 •

edited

Loading

davecheney left a comment

davecheney commented Sep 25, 2019 via email

stevesloka Sep 25, 2019 •

edited

Loading

youngnick Sep 25, 2019

youngnick Sep 25, 2019

stevesloka Sep 26, 2019

youngnick Sep 26, 2019

youngnick Sep 26, 2019

stevesloka Sep 26, 2019

stevesloka Sep 25, 2019

youngnick Sep 25, 2019

stevesloka Sep 25, 2019

youngnick Sep 25, 2019

stevesloka Sep 26, 2019

stevesloka Sep 25, 2019

stevesloka Sep 25, 2019

youngnick Sep 25, 2019

davecheney left a comment


		#### Virtualhost aliases

		To present the same set of routes under multiple dns entries, for example www.example.com and example.com, including a service with a `prefix` condition of `/` can be used.

Add Inclusion documentation for HTTPProxy #1573

Add Inclusion documentation for HTTPProxy #1573

Conversation

youngnick commented Sep 25, 2019 • edited Loading

davecheney left a comment

Choose a reason for hiding this comment

davecheney commented Sep 25, 2019 via email

stevesloka Sep 25, 2019 • edited Loading

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

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

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

davecheney left a comment

Choose a reason for hiding this comment

youngnick commented Sep 25, 2019 •

edited

Loading

stevesloka Sep 25, 2019 •

edited

Loading