Tap sink typed extension#28808
Merged
mattklein123 merged 23 commits intoenvoyproxy:mainfrom Aug 19, 2023
Merged
Conversation
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
At this point, the relevant context objects are being passed down into TapConfigBaseImpl so that they are available to be passed into TapSinkFactory::createSinkPtr. In the HTTP case, we pass a Server::Configuration::FactoryContext from createFilterFactoryFromProtoTyped; and in the transport socket case, we pass a TransportSocketFactoryContext from createTransportSocketFactory. Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
This test still requires a lot of clean-up, but it works and I'm committing it as a checkpoint to make sure I can come back if things change underneath. Here's what still needs to be done: 1. see if the test should be moved to another file/location (possibly convert it to an HTTP filter test) 2. clean up unused stuff 3. remove debug code Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Originally, the interface was accepting a Server::Configuration::CommonFactoryContext object, but this type of object is exclusively available to HTTP filters and cannot be used from in transport socket contexts. Instead, we just pull out the relevant fields from each of the contexts and apply them that way. It was also considered that we could use `std::variant` (or `absl:variant`), but variant types appear to not support reference objects, so this approach was not pursued. Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
* Remove the fixture (now it's unnecessary) * Remove the fake TapSinkFactoryImpl test object * Remove dependency on the HTTP filter Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
TapConfigBaseImpl cannot accept a common context object because there's no common hierarchy for HTTP and transport socket context objects. However, I think it's probably best to carry these context objects until the TapConfigBaseImpl constructor is called and decompose them at that point so we can have full access to these objects as long as we're traveling down the stack from filter instantiation to the TapConfigBaseImpl constructor... just in case anything else might need to use a different field from these objects. Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Note that I had to pass validation_visitor manually because I can't get the
commented out version to compile. It reports errors such as this:
user@12e5e5390487:/build/envoy$ bazel build -c dbg //source/exe:envoy-static --jobs=$(($(nproc) - 8))
INFO: Invocation ID: 6ce2e3ec-2a4e-4903-ae6a-19f55cb7c645
INFO: Analyzed target //source/exe:envoy-static (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: /build/envoy/source/extensions/common/tap/BUILD:23:17: Compiling source/extensions/common/tap/tap_config_base.cc failed: (Exit 1): clang-14 failed: error executing command (from target //source/extensions/common/tap:tap_config_base) /opt/llvm/bin/clang-14 -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer -g ... (remaining 224 arguments skipped)
Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
source/extensions/common/tap/tap_config_base.cc:105:12: error: variable type 'Envoy::Server::Configuration::TransportSocketFactoryContext' is an abstract class
auto tsf_context = tsf_context_ref.get();
^
source/extensions/common/tap/tap_config_base.cc:116:7: error: variable type 'Envoy::Server::Configuration::FactoryContext' is an abstract class
http_context = http_context_ref.get(); config =
^
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
At issue was the fact that we needed to change the variable type from `auto context` to `auto& context`. D'oh! Additionally, implement the transport socket factory test since that introduces a branch that was not tested before. Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Contributor
Author
|
/retest |
mattklein123
requested changes
Aug 16, 2023
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, thanks. Please add a release note.
/wait
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Contributor
Author
|
@mattklein123 Thanks for the review! Changelog has been added |
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
…nsion Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
nfuden
pushed a commit
to solo-io/envoy-fork
that referenced
this pull request
Nov 30, 2023
Signed-off-by: Ashish Banerjee <ashish.banerjee@solo.io>
nfuden
pushed a commit
to solo-io/envoy-fork
that referenced
this pull request
Nov 30, 2023
Tap sink typed extension (envoyproxy#28808)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message: add TypedExtensionConfig output sink to tap filter
Additional Description: enable Envoy extensions to write custom output sinks by hooking into the filter via the custom_sink output sink object
Risk Level: medium
Testing: in progress
Docs Changes: N/A
Release Notes: to be completed
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]