Skip to content

transport sockets: support passthrough state for internal connections#19435

Merged
lizan merged 67 commits intoenvoyproxy:mainfrom
kyessenov:internal_address_metadata
Jun 23, 2022
Merged

transport sockets: support passthrough state for internal connections#19435
lizan merged 67 commits intoenvoyproxy:mainfrom
kyessenov:internal_address_metadata

Conversation

@kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Jan 6, 2022

Signed-off-by: Kuat Yessenov kuat@google.com

Implements a special transport socket for transferring state (metadata and filter state) over the internal connection.
This transport socket captures a subset of endpoint metadata, cluster metadata, and stream filter state in the user space socket. When an internal listener accepts a user space socket connection, it immediately merges this passthrough state into the connection stream info. Because the state can be transferred from HTTP stream to TCP stream, this transport socket also participates in the hashing decisions in the HTTP connection pools.

Commit Message: Add passhtrough state over internal connection.
Risk Level: low, new extension
Testing: WIP
Docs Changes: yes
Release Notes: yes
Platform Specific Features:
Fixes: #19274

Signed-off-by: Kuat Yessenov <kuat@google.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #19435 was opened by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@lambdai
Copy link
Contributor

lambdai commented Jan 7, 2022

@kyessenov Should we add the target internal listener address as the first field of this transport socket?

@kyessenov
Copy link
Contributor Author

@lambdai I am not sure since we already have that as lb_endpoint address. I think having physical lb_endpoint address differ from the internal address in transport socket might be confusing.

@kyessenov
Copy link
Contributor Author

CC @alyssawilk in case you are interested. This is bare minimum needed to propagate policies from the first upstream to the internal listener. but also could be used to propagate the actual destination.

@alyssawilk
Copy link
Contributor

@adisuissa ping!

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.
I suggest maybe setting the proto as WIP, and add #not-implemented-hide, but up to you.

/lgtm api

@wrowe
Copy link
Contributor

wrowe commented Jan 24, 2022

I suggest maybe setting the proto as WIP, and add #not-implemented-hide

@kyessenov - your thought? In any case, merge master may hopefully clear coverage?

@kyessenov
Copy link
Contributor Author

Thanks, marked as WIP. Implementation likely needs #19467 first to land.

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov kyessenov changed the title internal listener: add metadata passthrough internal listener: add metadata passthrough (WIP) Jan 26, 2022
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

/retest
(timeout)

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19435 (comment) was created by @kyessenov.

see: more, trace.

@wrowe
Copy link
Contributor

wrowe commented Jun 22, 2022

Build looks clean, @lizan - are your concerns addressed?

lizan
lizan previously approved these changes Jun 22, 2022
@ggreenway
Copy link
Member

@kyessenov there's a merge conflict to resolve.

/wait

Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

Thanks, fixed the conflict.

@lizan lizan enabled auto-merge (squash) June 22, 2022 21:21
@wrowe
Copy link
Contributor

wrowe commented Jun 22, 2022

/retest

to determine whether this was a fluke;
[ RUN ] GrpcAccessLoggerCacheImplTest.LoggerCreationResourceAttributes
unknown file: Failure
C++ exception with description "Protobuf message (type opentelemetry.proto.collector.logs.v1.ExportLogsServiceRequest reason INVALID_ARGUMENT:(resource_logs.instrumentation_library_logs[0]) logs: Cannot find field.) has unknown fields" thrown in the test body.
test/extensions/access_loggers/open_telemetry/grpc_access_log_impl_test.cc:51: Failure
Actual function call count doesn't match EXPECT_CALL(*async_client, startRaw(_, _, _, _))...
Expected: to be called once
Actual: never called - unsatisfied and active

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19435 (comment) was created by @wrowe.

see: more, trace.

@kyessenov
Copy link
Contributor Author

/retest
(possible flake //test/extensions/access_loggers/open_telemetry:grpc_access_log_impl_test)

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19435 (comment) was created by @kyessenov.

see: more, trace.

@kyessenov
Copy link
Contributor Author

It's a new flake caused by f7a8db0.
Locally it passes reliably before that PR.

@phlax
Copy link
Member

phlax commented Jun 22, 2022

It's a new flake caused by f7a8db0.

apologies, addressing this now

@kyessenov
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19435 (comment) was created by @kyessenov.

see: more, trace.

@lizan lizan merged commit 637a92a into envoyproxy:main Jun 23, 2022
Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022
…envoyproxy#19435)

Implements a special transport socket for transferring state (metadata and filter state) over the internal connection.
This transport socket captures a subset of endpoint metadata, cluster metadata, and stream filter state in the user space socket. When an internal listener accepts a user space socket connection, it immediately merges this passthrough state into the connection stream info. Because the state can be transferred from HTTP stream to TCP stream, this transport socket also participates in the hashing decisions in the HTTP connection pools.

Commit Message: Add passhtrough state over internal connection.
Risk Level: low, new extension
Testing: WIP
Docs Changes: yes
Release Notes: yes
Platform Specific Features:
Fixes: envoyproxy#19274

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
kyessenov added a commit that referenced this pull request Jul 6, 2022
Commit Message: Adds a new flag for filter state objects that indicates the intent to share with the upstream.
Additional Description: Follow-up to #19809. There have been multiple reports of unexpected lifecycle changes for the filter state objects because they are stored in the transport socket options. This PR addresses this issue by introducing a new mark for filter state that explicitly changes the usage of filter state objects:

marked objects always participate in the connection pool hashing (generalizing and simplifying transport sockets: support passthrough state for internal connections #19435);
marked objects are copied by reference to the upstream info - this allows sharing state between downstream and upstream (and further down the chain, the internal listeners).
Risk Level: medium, revert to the original behavior prior to #19809
Testing: yes
Docs Changes: yes
Release Notes: yes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transport socket for internal listeners

10 participants