router: support prefix wildcards in virtual hosts domains by bartebor · Pull Request #6303 · envoyproxy/envoy

bartebor · 2019-03-16T21:25:29Z

Description: Adds prefix wildcard support (foo.*) in virtual host domains
Risk Level: Low (does not change current behavior)
Testing: Unit tests
Docs Changes: updated domains field documentation in proto file
Release Notes: updated
Fixes #1269

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

venilnoronha

LGTM overall, just one nit.

venilnoronha · 2019-03-16T22:29:59Z

docs/root/intro/version_history.rst

 * router: added reset reason to response body when upstream reset happens. After this change, the response body will be of the form `upstream connect error or disconnect/reset before headers. reset reason:`
 * router: added :ref:`rq_reset_after_downstream_response_started <config_http_filters_router_stats>` counter stat to router stats.
 * router: added per-route configuration of :ref:`internal redirects <envoy_api_field_route.RouteAction.internal_redirect_action>`.
+* router: added support for prefix wildcards in :ref:`virtual hosts domains<envoy_api_field_route.VirtualHost.domains>`


nit: s/hosts/host

Thanks @venilnoronha, I fixed this. I made the same mistake in commit message which would need to be taken care of when squashing (I did not rewrite history).

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

junr03

@bartebor thanks for the PR. Two high level comments:

Is there any reason for the preferential ordering between suffix and prefix domains? i.e why suffix before prefix.
In #1269 (comment) @htuch mentioned using an inverted trie here. Is this something that we want to address in a subsequent PR with a new issue?

htuch · 2019-03-19T19:57:31Z

@junr03 doesn't have to be a trie in this PR due to the fact we only do a list already (Trevor added a trie a while back but then reverted). What's important to me is that it is plausible to implement the semantics being added in a trie (and inverted trie). This is because we want to add scalability in the future, and so need to ensure we're not adding anything to VH matching that would prohibit this (e.g. if we added a regex, I would be opposed to that).

api/envoy/api/v2/route/route.proto

bartebor · 2019-03-20T07:29:08Z

@junr03 I chose the same order nginx uses. I believe the idea behind this order is matching with steps of decreasing selectivity. Prefix wildcards can match any domain, so they are matched last. Nginx additionally supports regexps as a last step, but we probably do not want that for the reasons @htuch mentioned in #6303 (comment).

junr03

@bartebor thanks for answering my questions!

A couple comments, which funny enough don't explicitly have to do with your change.

/wait

junr03 · 2019-03-25T16:30:48Z

source/common/router/config_impl.cc

        wildcard_virtual_host_suffixes_[domain.size() - 1].emplace(domain.substr(1), virtual_host);
+      } else if (domain.size() > 0 && '*' == domain[domain.size() - 1]) {
+        wildcard_virtual_host_prefixes_[domain.size() - 1].emplace(
+            domain.substr(0, domain.size() - 1), virtual_host);


Same with the suffixes above. But I just noticed that we don't detect duplicates (at least afaict here) for suffix or prefix domains, like we do for exact matches below. Is that intentional?

I don't personally think it was intentional. I'll add duplicate checking to make things clear.

junr03 · 2019-03-25T16:33:45Z

source/common/router/config_impl.cc

  // Fast path the case where we only have a default virtual host.
-  if (virtual_hosts_.empty() && wildcard_virtual_host_suffixes_.empty() && default_virtual_host_) {
+  if (virtual_hosts_.empty() && wildcard_virtual_host_suffixes_.empty() &&
+      wildcard_virtual_host_prefixes_.empty() && default_virtual_host_) {


I wonder if we even need the default_virtual_host_ clause here. We end up returning default_virtual_host_.get() at the end anyway.

You're right, clause removed.

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

junr03

lgtm. Thanks for adding the checks, and the new tests.

junr03 · 2019-03-27T18:27:10Z

@htuch can you do a final pass?

@bartebor do you mind fixing the conflict?

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

htuch

Thanks @junr03 for reviewing and @bartebor for this PR. LGTM but I wonder whether we have sufficient tests, see comment.

htuch · 2019-03-28T04:04:07Z

test/common/router/config_impl_test.cc

  EXPECT_EQ("instant-server",
            config.route(genHeaders(".foo.com", "/", "GET"), 0)->routeEntry()->clusterName());
-  EXPECT_EQ("instant-server",
+  EXPECT_EQ("wildcard",


Are these tests strong enough to show that the match order follows the specified one (suffix, then prefix wildcards)?

@htuch I refactored my tests to handle all cases explicitly.

Something bad is going with the tests from the point I merged with master. I don't think failures result from my code changes, but with no access to test artifacts I can't be sure.

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

htuch

The new test is perfect, thanks. I rekicked CI, let's see if that fixes it.

* master: router: support prefix wildcards in virtual hosts domains (envoyproxy#6303) security: update distributor application example to include e-mail. (envoyproxy#6425) Examples: Update gen script of grpc example service (envoyproxy#6372) config: de-templatize source/common/config (envoyproxy#6391) docs: adds information about the Envoy tracer from Instana envoyproxy#6371 (envoyproxy#6416) test: convert ratelimit test configs to v2 YAML (envoyproxy#6411) include required python and go dependencies for grpc-bridge example (envoyproxy#6402) docs: more snapping fixes (envoyproxy#6404) runtime: switching from unordered_map to absl::flat_hash_map (envoyproxy#6389)

router: support prefix wildcards in virtual hosts domains

6a347aa

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

mattklein123 assigned junr03 Mar 17, 2019

venilnoronha previously approved these changes Mar 18, 2019

View reviewed changes

release note fix

4fa80f9

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

bartebor dismissed venilnoronha’s stale review via 4fa80f9 March 18, 2019 19:09

junr03 reviewed Mar 19, 2019

View reviewed changes

htuch reviewed Mar 19, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Show resolved Hide resolved

htuch added the waiting:any label Mar 19, 2019

repokitteh-read-only bot removed the waiting:any label Mar 20, 2019

junr03 reviewed Mar 25, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 25, 2019

route: detecting wildcard domain duplicates

c221739

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

repokitteh-read-only bot removed the waiting label Mar 25, 2019

junr03 reviewed Mar 27, 2019

View reviewed changes

junr03 assigned htuch Mar 27, 2019

bartebor added 3 commits March 27, 2019 20:52

Merge branch 'master' into trailing_wildcards

9778805

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

Fixed coding style

a410693

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

Kick CI

da0ccde

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

htuch suggested changes Mar 28, 2019

View reviewed changes

htuch added the waiting:any label Mar 28, 2019

Refactoring domain matching order tests

4cceb65

Signed-off-by: Bartosz Borkowski <bartebor@wp.pl>

repokitteh-read-only bot removed the waiting:any label Mar 28, 2019

htuch approved these changes Mar 29, 2019

View reviewed changes

htuch merged commit af7c845 into envoyproxy:master Mar 29, 2019

Conversation

bartebor commented Mar 16, 2019

Uh oh!

venilnoronha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Mar 19, 2019

Uh oh!

Uh oh!

bartebor commented Mar 20, 2019

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 commented Mar 27, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants