-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
formatter: support for virtual host metadata #34958
Conversation
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
/retest |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Verify Examples job failure is irrelevant to this eg the same error on main https://github.com/envoyproxy/envoy/actions/runs/9708036595/job/26794181496 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm api
/assign-from @envoyproxy/senior-maintainers |
@envoyproxy/senior-maintainers assignee is @RyanTheOptimist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Is it possible to add an integration test which verifies this behavior?
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
thanks @RyanTheOptimist, added an integration test with access logger in 747b642 |
This is a good test, but I was hoping to see something in Would this be possible? |
@RyanTheOptimist this formatter is an extension so that would require using diff --git a/test/integration/BUILD b/test/integration/BUILD
index 5f643fb68c..4648bede8b 100644
--- a/test/integration/BUILD
+++ b/test/integration/BUILD
@@ -1743,13 +1743,16 @@ envoy_cc_test_library(
hdrs = ["tcp_proxy_integration_test.h"],
)
-envoy_cc_test(
+envoy_extension_cc_test(
name = "tcp_proxy_integration_test",
size = "large",
srcs = ["tcp_proxy_integration_test.cc"],
data = [
"//test/config/integration/certs",
],
+ extension_names = [
+ "envoy.formatter.metadata",
+ ],
tags = [
"cpu:3",
],
@@ -1767,6 +1770,7 @@ envoy_cc_test(
"//source/extensions/access_loggers/file:config",
"//source/extensions/filters/network/common:factory_base_lib",
"//source/extensions/filters/network/tcp_proxy:config",
+ "//source/extensions/formatter/metadata:config",
"//source/extensions/load_balancing_policies/subset:config",
"//source/extensions/transport_sockets/tls:config",
"//test/mocks/runtime:runtime_mocks", |
well this doesn't work as well -
|
so diff --git a/source/extensions/formatter/metadata/BUILD b/source/extensions/formatter/metadata/BUILD
index 8d1bf6bf39..78a8e7d86b 100644
--- a/source/extensions/formatter/metadata/BUILD
+++ b/source/extensions/formatter/metadata/BUILD
@@ -23,6 +23,10 @@ envoy_cc_extension(
name = "config",
srcs = ["config.cc"],
hdrs = ["config.h"],
+ extra_visibility = [
+ "//test:__subpackages__",
+ ],
deps = [
"//envoy/registry",
"//source/extensions/formatter/metadata:metadata_lib",
diff --git a/test/integration/BUILD b/test/integration/BUILD
index 5f643fb68c..d5dd5ebc54 100644
--- a/test/integration/BUILD
+++ b/test/integration/BUILD
@@ -1767,6 +1767,7 @@ envoy_cc_test(
"//source/extensions/access_loggers/file:config",
"//source/extensions/filters/network/common:factory_base_lib",
"//source/extensions/filters/network/tcp_proxy:config",
+ "//source/extensions/formatter/metadata:config",
"//source/extensions/load_balancing_policies/subset:config",
"//source/extensions/transport_sockets/tls:config",
"//test/mocks/runtime:runtime_mocks", ok so this is what we need. is this acceptable? @RyanTheOptimist |
Hm. Good question. I have a vague recollection that we have a policy about this somehow. @alyssawilk can you clarify? |
generally we request folks not add tests of extension functionality into "core" directories but instead have e2e tests in the extension directory. Otherwise we often break the build for folks who don't import a given extension |
thank you for clarifying. So I think my last commit 747b642 is the furthest we can get without adding the extra visibility. edited: we could add |
Doing this in a follow up would be fine. |
/wait |
/retest |
the verify test failing looks irrelevant to this PR. |
/retest |
2 similar comments
/retest |
/retest |
@phlax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Some comments are added.
This won't block this work anyway. |
This reverts commit 747b642. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@wbpcode friendly ping |
Commit Message: formatter: support for virtual host metadata
Additional Description:
This enables the access to the virtual host metadata introduced in #30175 from formatter.
Risk Level: low
Testing: : unit test
Docs Changes: done.
Release Notes:
Platform Specific Features:
[Optional Fixes #Issue] #34900