formatter: support for virtual host metadata

[mathetake]

Commit Message: formatter: support for virtual host metadata Additional Description: This enables the access to the virtual host metadata introduced in https://github.com/envoyproxy/envoy/pull/30175 from formatter.

Risk Level: low Testing: : unit test Docs Changes: done. Release Notes:
Platform Specific Features: [Optional Fixes #Issue] https://github.com/envoyproxy/envoy/issues/34900

[repokitteh-read-only[bot]]

As a reminder, PRs marked as draft will not be automatically assigned reviewers, or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/34958 was opened by mathetake.

see: more, trace.

[repokitteh-read-only[bot]]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/). envoyproxy/api-shepherds assignee is @abeyad CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/34958 was opened by mathetake.

see: more, trace.

[mathetake]

/retest

[mathetake]

Verify Examples job failure is irrelevant to this eg the same error on main https://github.com/envoyproxy/envoy/actions/runs/9708036595/job/26794181496

[abeyad]

/assign-from @envoyproxy/senior-maintainers

[repokitteh-read-only[bot]]

@envoyproxy/senior-maintainers assignee is @RyanTheOptimist

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/34958#issuecomment-2197469939 was created by @abeyad.

see: more, trace.

[mathetake]

thanks @RyanTheOptimist, added an integration test with access logger in https://github.com/envoyproxy/envoy/pull/34958/commits/747b64282442575299f39e4f53e7848f7f5832a3

[RyanTheOptimist]

thanks @RyanTheOptimist, added an integration test with access logger in 747b642

This is a good test, but I was hoping to see something in test/integration in which we see Envoy process an actual request. This test is TCP-proxy specific, but shows the idea. https://github.com/envoyproxy/envoy/blob/main/test/integration/tcp_proxy_integration_test.cc#L378

Would this be possible?

[mathetake]

@RyanTheOptimist this formatter is an extension so that would require using envoy_extension_cc_test for say tcp_proxy_integration_test. Is that acceptable practice in general? (I am not knowledgeable on the extension visibility policy)

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",

[mathetake]

well this doesn't work as well -

vscode@mathetake-Venus-series:/workspaces/envoy$ bazel test --define wasm=none //test/integration:tcp_proxy_integration_test
ERROR: /workspaces/envoy/test/integration/BUILD:1750:24: in cc_test rule //test/integration:tcp_proxy_integration_test: target '//source/extensions/formatter/metadata:config' is not visible from target '//test/integration:tcp_proxy_integration_test'. Check the visibility declaration of the former target if you think the dependency is legitimate
ERROR: /workspaces/envoy/test/integration/BUILD:1750:24: Analysis of target '//test/integration:tcp_proxy_integration_test' failed
ERROR: Analysis of target '//test/integration:tcp_proxy_integration_test' failed; build aborted: 
INFO: Elapsed time: 0.938s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (605 packages loaded, 35042 targets configured)
ERROR: Couldn't start the build. Unable to run tests

[mathetake]

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

[RyanTheOptimist]

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?

[alyssawilk]

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

[mathetake]

thank you for clarifying. So I think my last commit https://github.com/envoyproxy/envoy/commit/747b64282442575299f39e4f53e7848f7f5832a3 is the furthest we can get without adding the extra visibility.

edited: we could add test/extensions/formatter/metadata/integration_test.cc in the first place, but that would go beyond the scope of this PR since I have to write tests for all metadata types, not only for VIRTUAL_HOST that is added in this PR. I think I would do that in a follow-up PR later. wdyt? @RyanTheOptimist

[RyanTheOptimist]

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 test/extensions/formatter/metadata/integration_test.cc in the first place, but that would go beyond the scope of this PR since I have to write tests for all metadata types, not only for VIRTUAL_HOST that is added in this PR. I think I would do that in a follow-up PR later. wdyt? @RyanTheOptimist

Doing this in a follow up would be fine.

[RyanTheOptimist]

/wait on CI

[mathetake]

/retest

[mathetake]

the verify test failing looks irrelevant to this PR.

[mathetake]

/retest

[mathetake]

/retest

[mathetake]

/retest

[mathetake]

@phlax verify_examples wasm-cc is constantly failing all over the places recently. Anyone is working on fixing it?

[wbpcode]

@phlax verify_examples wasm-cc is constantly failing all over the places recently. Anyone is working on fixing it?

This won't block this work anyway.

[RyanTheOptimist]

@wbpcode friendly ping