[tls] Introduce HandshakerAndConnectionInfo interface.#13081
[tls] Introduce HandshakerAndConnectionInfo interface.#13081ambuc wants to merge 2 commits intoenvoyproxy:masterfrom
Conversation
…namic_pointer_cast. Signed-off-by: James Buckland <jbuckland@google.com>
|
/assign lizan |
Signed-off-by: James Buckland <jbuckland@google.com>
| * which can both perform handshakes and provide connection-specific | ||
| * information. | ||
| */ | ||
| class HandshakerAndConnectionInfo : public Handshaker, public ConnectionInfo { |
There was a problem hiding this comment.
I'm not thrilled with this interface, with extra virtual method. What is the goal of this PR? If we just want to provide an impl, then we might just make some mixin pattern class for ConnectionInfoImpl. If we want make the cast to ConnectionInfo safe, then perhaps make Handshaker return a ConnectionInfo pointer (in the default impl it just return this).
There was a problem hiding this comment.
The goal of this PR is to make the cast safe.
If we have Handshaker return a connectioninfo pointer and save that as info_, it's unclear to me who holds on to the SslHandshaker class for lifetime reasons. Plus info_ also needs socketstate setter/getters, which aren't a part of either the ConnectionInfo or Handshaker interfaces.
But, I agree, I don't love this interface. WDYT?
There was a problem hiding this comment.
I don't like this either. So we're requiring one class to implement both of these interfaces. Why not just combine them into the same interface? Or if that isn't the right solution, some other refactoring appears needed.
@mattklein123 any thoughts (since you left the original comment)?
There was a problem hiding this comment.
My original thinking was to have a connectionInfo() method on the Handshaker interface and then you can handle the lifetime independently, but I don't have a strong opinion either way.
|
@ggreenway are you able to take this over (feel free to re-assign) since Lizan is out? |
|
/wait |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 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! |
stale
bot
added
the
stale
mattklein123
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! |
5 participants
Signed-off-by: James Buckland jbuckland@google.com
Commit Message: Introduce HandshakerAndConnectionInfo interface.
Additional Description: @mattklein123 pointed out in #12658 (comment) that the current design of
dynamic_pointer_cast-ing an extension impl only works if, as in the test, our custom impl extends the real impl. This PR introduces a new interface with the necessary method, which allows us to remove this unsafe cast.Risk Level: Low
Testing: N/a
Docs Changes: N/a
Release Notes: N/a