GEODE-7837: add gemfire.security.sni-proxy system property #4758
Conversation
geode-core/src/main/java/org/apache/geode/internal/admin/SSLConfig.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/admin/SSLConfig.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/net/SCAdvancedSocketCreator.java
Outdated
Show resolved
Hide resolved
| sockaddr = sockaddrTemp; | ||
| } | ||
| socket.connect(sockaddr, Math.max(timeout, 0)); | ||
| coreSocketCreator.configureClientSSLSocket(socket, timeout, _ignored -> { |
There was a problem hiding this comment.
Why not provide the SNI regardless of proxy?
There was a problem hiding this comment.
If I understand your question, I chose to pass a lambda to configureClientSSLSocket() rather than passing in a (possibly-null SNI hostname).
There was a problem hiding this comment.
My question is, There is always a host name so why not always set the SNI header?
There was a problem hiding this comment.
Oh I see what you mean. I'd rather not add that header, which is superfluous in the common case i.e. no proxy. But yeah like you, I think functionally that could work.
There was a problem hiding this comment.
I am thinking from the standpoint that everything is the same except for where it can't. So in this case the initialization of the SSL socket is identical regardless of the proxy. Less moving parts.
geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorWithSNIProxyTest.java
Outdated
Show resolved
Hide resolved
|
that IntegrationTestOpenJDK11 is a well-known flaky test outside this PR: https://issues.apache.org/jira/browse/GEODE-7843?jql=text%20~%20%22testConcurrentHSetNX%22 |
Bill
requested review from
bschuchardt and
echobravopapa
and removed request for
bschuchardt and
echobravopapa
|
This PR needs an automated test agin a real SNI proxy. I'm closing the PR until that test is in place. |
GEODE-7837 is an MVP to add SNI support in the Geode Java client. This PR lets you set an SNI proxy (host:port) in a system property. A follow-on PR GEODE-7852 will replace the system property with a first-class connection pool configuration property.
This PR adds some unit tests of the system property parsing but doesn't have an integration test. Integration testing was manual for the MVP. We'll have a follow-on story TBD to add an integration test.
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.