Skip to content
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

reverse_conn: envoy contrib changes for reverse_connection custom cluster #37932

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

basundhara-c
Copy link

Commit Message: This PR defines a new custom cluster type "envoy.clusters.reverse_connection" which enables requests to be sent over reverse connections instead of forward connections. This PR is strictly tied with #37368.

Additional Description: In order to differentiate between requests that need to be sent via a reverse connection vs those to be sent through forward connections, a new cluster type has been defined. The downstream requests to be sent over reverse connections are expected to contain either of the HTTP headers defined in the cluster's typed config, to enable upstream envoy to figure out which downstream node to send the request to,

The Cluster Manager checks whether the connection is intended for a reverse connection cluster, and if so, calls a custom HTTP/2 conn pool to send the request over the reverse connection.
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Copy link

Hi @basundhara-c, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #37932 was opened by basundhara-c.

see: more, trace.

Signed-off-by: Basundhara Chakrabarty <[email protected]>
@jmarantz
Copy link
Contributor

/retest

@jmarantz
Copy link
Contributor

@botengyao anything blocking moving forward with a review?

@phlax
Copy link
Member

phlax commented Jan 22, 2025

ping @botengyao

Copy link
Member

@botengyao botengyao 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 the contribution!
waiting for the core PR changes but took a quick look for this extension.

/wait

}

Upstream::HostSharedPtr RevConCluster::checkAndCreateHost(const std::string host_id) {
host_map_lock_.ReaderLock();
Copy link
Member

Choose a reason for hiding this comment

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

usually we don't need a lock for the cluster since Envoy will maintain the thread-local cluster for each worker, but I think this cluster could be shared with all worker threads, and it is hard for me to understand how it is used, could you add more info to the class comments? It is fine not include thorough tests in contrib, could you point me maybe any integration/e2e test that involves this component but could not in this PR for me to understand, please?

Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 21, 2025
Signed-off-by: Basundhara Chakrabarty <[email protected]>
Signed-off-by: Basundhara Chakrabarty <[email protected]>
@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 23, 2025
Signed-off-by: Basundhara Chakrabarty <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants