support for starttls inner socket in UpstreamProxyProtocolSocket#26593
support for starttls inner socket in UpstreamProxyProtocolSocket#26593VishalDamgude wants to merge 4 commits intoenvoyproxy:mainfrom
Conversation
UpstreamProxyProtocolSocket Signed-off-by: VishalDamgude <vishal.damgude@freshworks.com>
|
Hi @VishalDamgude, 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. |
Signed-off-by: VishalDamgude <vishal.damgude@freshworks.com>
|
Huh, my initial pass was going to be to ask we add this directly to the passthrough base class but there's a comment that should not be done. |
|
It would be good to add an integration test that validates this plumbing end to end. |
|
After reviewing the code, I think the call to Also, integration tests should be added as mentioned above. |
cpakulski
left a comment
cpakulski
left a comment
There was a problem hiding this comment.
Please move startSecureTransport to base class.
| config.set_version(ProxyProtocolConfig_Version::ProxyProtocolConfig_Version_V2); | ||
| initialize(config, nullptr); | ||
|
|
||
| EXPECT_CALL(*inner_socket_, startSecureTransport()).WillOnce(Return(false)); |
There was a problem hiding this comment.
Return(false) is not required here. Test only cares that the method is invoked.
There was a problem hiding this comment.
sure, will address this.
|
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
Bot
added
the
stale
Signed-off-by: VishalDamgude <vishal.damgude@freshworks.com>
Signed-off-by: VishalDamgude <vishal.damgude@freshworks.com>
github-actions
Bot
removed
the
stale
|
@alyssawilk Review SLO ping. |
|
assignee is @lizan? |
|
Yet to add integration tests in this PR. |
|
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
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
/reopen |
github-actions
Bot
removed
the
stale
|
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
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
7 participants
Commit Message: support for starttls inner socket in UpstreamProxyProtocolSocket
Additional Description: When starttls socket is configured as inner socket in UpstreamProxyProtocolSocket for the cluster, we are unable to switch to TLS. When an L7 filter (e.g SMTP) invokes startSecureTransport() on UpstreamProxyProtocolSocket as a result of STARTTLS command, the api returns false due to missing implementation.
Risk Level: Low
Testing: Unit Tests, Local tests with SMTP filter.
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #26592
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]