Adding the client hello sni information at Connection.sniHostName()#58
Adding the client hello sni information at Connection.sniHostName()#58jaylu merged 8 commits into3redronin:masterfrom
Conversation
|
Hi @danielflower please feel free to do a review |
|
Can you make public-facing API changes to the mu3 branch too? The impl can just throw an exception saying "Not implemented". And copy the test in which would fail and mark it as ignored. This is so we don't miss this feature if/when mu3 is released. Thanks |
| import java.util.function.Supplier; | ||
|
|
||
|
|
||
| public class MuSniHandler extends SniHandler { |
There was a problem hiding this comment.
Why is this handler needed? Can the logic just be on the Connection itself?
There was a problem hiding this comment.
when the SniHandler trigger the SniCompletionEvent, the connection haven't been added into the pipeline yet.
The AlpnHandler ( the mu one) can see that event, my initial implementation was put in the AlpnHandler. But later we decide to use the connection attribute, so better to put the SNI things in one place. that's why put it here.
This class also overwrite the newSslHandler() method too, which to cater the existing logic for setting the SslEngine's param.
There was a problem hiding this comment.
Okay, can we not make it a non public class then? Don’t want to expand the public api
Just pushed to the mu3 branch |
| ProxiedConnectionInfoImpl proxyConnectionInfo = ProxiedConnectionInfoImpl.fromNetty(msg); | ||
| ctx.channel().attr(HA_PROXY_INFO).set(proxyConnectionInfo); | ||
| if (!ctx.channel().config().isAutoRead()) { | ||
| ctx.read(); |
There was a problem hiding this comment.
one thing to highlight, for JDK 8, if not doing this extra ctx.read(), it will randomly failed the text - client request timeout.
|
Hi @danielflower , if you are fine, I will do the squash merge |
Okay sure, go for it. Thanks |
2 participants
Adding the client hello sni information at Connection.sniHostName(), below is the implementation details: