extension: add test case by lambdai · Pull Request #13915 · envoyproxy/envoy

lambdai · 2020-11-05T10:12:58Z

Commit Message:
Add extra test case to guard that cluster upstream extension support header rewrite.

Additional Description:

Risk Level: LOW
Testing: Integration test
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

TODO: http request verifier, host data Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

snowp

Thanks for the test, I left a few comments to get this started

test/integration/cluster_upstream_extension_integration_test.cc

test/integration/upstreams/per_host_upstream_request.h

test/integration/upstreams/per_host_upstream_request.cc

test/integration/upstreams/per_host_upstream_config.cc

test/integration/upstreams/BUILD

source/extensions/upstreams/http/http/BUILD

lambdai

@snowp

Thanks for the review! I will fix and update the PR later today

source/extensions/upstreams/http/http/BUILD

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai

@snowp I am seeing checkformat failure on my new "test/integration/upstream/BUILD"

The auto fix is to add envoy_package(), but that fix actually fail the entire build. Any suggestion?

source/extensions/upstreams/http/http/BUILD

test/integration/cluster_upstream_extension_integration_test.cc

test/integration/upstreams/BUILD

test/integration/upstreams/per_host_upstream_config.cc

test/integration/upstreams/per_host_upstream_request.cc

test/integration/upstreams/per_host_upstream_request.h

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2020-11-10T17:20:17Z

@snowp I am seeing checkformat failure on my new "test/integration/upstream/BUILD"

The auto fix is to add envoy_package(), but that fix actually fail the entire build. Any suggestion?

The root cause: envoy_build_fixer.py likes either envoy_package or envoy_extension_package(sole in source/extensions).
The above make sense.

Now that the newly add extension is only used by integration test, I choose to declare in in envoy_package.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

snowp · 2020-11-10T18:46:34Z

Seems like CI is broken, can you fix and then I'll take a look?

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2020-11-10T19:48:42Z

@snowp Thank you for the reminder
Yeah, I reproduce locally when I merged the latest master. Look like linux_x64 release is passing and I expect the rest will pass soon

snowp

Thanks this looks better, just a few comments.

Would appreciate @alyssawilk's thoughts as well, in particular on the general usefulness of this test: part of me feels like this is pretty guaranteed to continue to work no matter what happens in the router and elsewhere.

snowp · 2020-11-10T21:45:35Z

test/integration/upstreams/per_host_upstream_request.h

+    Envoy::Http::HeaderMapImpl::copyFrom(*dup, headers);
+    dup->setCopy(Envoy::Http::LowerCaseString("X-foo"), "foo-common");
+    addHeader(*dup, "X-cluster-foo", host_->cluster().metadata(), "foo", "bar");
+    if (host_->metadata() != nullptr) {
+      addHeader(*dup, "X-host-foo", *host_->metadata(), "foo", "bar");
+    }


If you're only trying to verify that you can modify headers in the upstream extension why the complexity here? Why not just a single header?

The idea is that I don't want to add test case because
"Envoy can do this in this current implementation" but
"This is real world requirement"
Also this can acts an example for user who want to rewrite header base on per upstream attribute like #13897

Does it make sense?

snowp · 2020-11-10T21:46:51Z

test/integration/cluster_upstream_extension_integration_test.cc

+// This test case should be rewritten once upstream http
+// filters(https://github.com/envoyproxy/envoy/issues/10455) is landed.


If this is something that we want to support and validate via a test it seems like we should keep this test for perpetuity - just because you'd rewrite your extension to be a HTTP filter instead it doesn't mean everyone else would.

As I explained in the other comment: This is an unfortunate constraint。
I am happy to refactor my upstream extension if upstream HTTP filter could achieve the goal providing alternative api.
So consider this as soft requirement when you are working on it

lambdai · 2020-11-12T17:46:50Z

gentle ping @alyssawilk

alyssawilk

I agree that it feels clear that this functionality will stick around. I will say given how often Envoy APIs shift around it isn't terrible to have a canonical example, so that if API / factory changes happen there's also a canonical transformation PR, if that makes sense? Basically don't know if it's necessary but I'm not averse.

Either way, sorry for the lag, and here's some thoughts on how we can limit code duplication and tighten it up a bit :-)

test/integration/cluster_upstream_extension_integration_test.cc

test/integration/upstreams/per_host_upstream_config.cc

test/integration/upstreams/per_host_upstream_request.h

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai

Thank you @alyssawilk

Now the extension is a single header file. It is much cleaner for the test.

test/integration/cluster_upstream_extension_integration_test.cc

test/integration/upstreams/per_host_upstream_config.cc

test/integration/upstreams/per_host_upstream_request.h

test/integration/upstreams/per_host_upstream_config.cc

alyssawilk

Cool, much cleaner, thanks!
Just two more comments and you're good to go.

alyssawilk · 2020-11-17T14:29:43Z

test/integration/upstreams/per_host_upstream_config.h

+  void resetStream() override { sub_upstream_.resetStream(); }
+
+private:
+  Extensions::Upstreams::Http::Http::HttpUpstream sub_upstream_;


Oh, I was thinking if you inherit from the HTTP upstream rather than wrap it, you can remove all the wrapper functions (which simply call sub_upstream_.function). WDYT?

Updated. The new shape is less error prone for this task. Thanks!

test/integration/cluster_upstream_extension_integration_test.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

…3915) Add extra test case to guard that cluster upstream extension support header rewrite. Signed-off-by: Yuchen Dai <silentdai@gmail.com> Signed-off-by: Qin Qin <qqin@google.com>

lambdai added 5 commits November 4, 2020 07:42

add per host cluster upstream extension in test

da57203

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Initial integration test

af7f280

TODO: http request verifier, host data Signed-off-by: Yuchen Dai <silentdai@gmail.com>

extract cluster and host metadata

1dca4f8

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

massage registry factory

e77b918

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

clean up

14e14cb

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai requested review from alyssawilk and snowp November 5, 2020 10:12

ggreenway assigned snowp and alyssawilk Nov 5, 2020

snowp suggested changes Nov 6, 2020

View reviewed changes

lambdai commented Nov 7, 2020

View reviewed changes

source/extensions/upstreams/http/http/BUILD Show resolved Hide resolved

address comment

4888b9f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai commented Nov 9, 2020

View reviewed changes

the target in test doesn't need to be cc_extension

c9e5869

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

This was referenced Nov 10, 2020

Upstream HTTP filters #10455

Closed

Add support of additional %UPSTREAM% variables to custom headers #13897

Open

Merge remote-tracking branch 'origin/master' into smallextension

74fce14

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot added the waiting label Nov 10, 2020

merge master and update api

118fe55

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 10, 2020

snowp suggested changes Nov 10, 2020

View reviewed changes

alyssawilk reviewed Nov 16, 2020

View reviewed changes

lambdai added 4 commits November 17, 2020 11:42

address test.cc

e32f973

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

remove config.cc

4898a4c

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

inherit HttpConnPool

06251c1

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

merge merge

4ea3506

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai commented Nov 17, 2020

View reviewed changes

alyssawilk reviewed Nov 17, 2020

View reviewed changes

inherit HttpUpstream to reduce complexity

dd7d5a8

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

alyssawilk approved these changes Nov 17, 2020

View reviewed changes

lambdai requested a review from snowp November 18, 2020 18:07

snowp approved these changes Nov 23, 2020

View reviewed changes

snowp merged commit 4f5b40c into envoyproxy:master Nov 23, 2020

lambdai mentioned this pull request Mar 8, 2021

filter_chain_match does not apply to dynamic_forward_proxy HTTP targets #15348

Closed

lambdai mentioned this pull request Mar 23, 2021

Upstream extension mapping cluster metadata to HTTP headers #15472

Closed

		// This test case should be rewritten once upstream http
		// filters(https://github.com/envoyproxy/envoy/issues/10455) is landed.

Conversation

lambdai commented Nov 5, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai commented Nov 10, 2020

Uh oh!

snowp commented Nov 10, 2020

Uh oh!

lambdai commented Nov 10, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

snowp Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

lambdai Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

snowp Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

lambdai Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

lambdai commented Nov 12, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk Nov 17, 2020

Choose a reason for hiding this comment