[
https://issues.apache.org/jira/browse/GEODE-8398?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17179255#comment-17179255
]
ASF GitHub Bot commented on GEODE-8398:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r471800583
##########
File path: clicache/integration-test2/SNITests.cs
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Diagnostics;
+using System.IO;
+using Xunit;
+using PdxTests;
+using System.Collections;
+using System.Collections.Generic;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+ [Trait("Category", "Integration")]
+ public class SNITests : TestBase, IDisposable
+ {
+ string currentWorkingDirectory;
+ Process dockerProcess;
+ private readonly Cache cache_;
+
+ public SNITests(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+ {
+ currentWorkingDirectory = Directory.GetCurrentDirectory();
+ var clientTruststore = Config.SslClientKeyPath +
@"/truststore_sni.pem";
+
+
+
+ var cacheFactory = new CacheFactory();
+ cacheFactory.Set("log-level", "none");
+ cacheFactory.Set("log-file", "c:/temp/SNITest-csharp.log");
+ cacheFactory.Set("ssl-enabled", "true");
+ cacheFactory.Set("ssl-truststore", clientTruststore);
+
+ cache_ = cacheFactory.Create();
+
+ var dc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "-f " +
Config.SniConfigPath + "/docker-compose.yml" + " up -d");
+ dc.WaitForExit();
+
+ var d = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "exec -t geode gfsh run
--file=/geode/scripts/geode-starter.gfsh");
+ d.WaitForExit();
+ }
+
+ public void Dispose()
+ {
+
+ var dockerComposeProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "stop");
+ dockerComposeProc.WaitForExit();
+
+ var dockerProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "container prune -f");
+ dockerProc.WaitForExit();
+
+ }
+
+ private string RunDockerCommand(string dockerCommand)
+ {
+ ProcessStartInfo startInfo = new ProcessStartInfo();
+ startInfo.RedirectStandardOutput = true;
+ startInfo.UseShellExecute = false;
+ startInfo.FileName = @"C:\Program
Files\Docker\Docker\resources\bin\docker.exe";
+ startInfo.Arguments = dockerCommand;
+ dockerProcess = Process.Start(startInfo);
+ String rVal = dockerProcess.StandardOutput.ReadToEnd();
+ dockerProcess.WaitForExit();
+ return rVal;
+ }
+
+ private int ParseProxyPort(string proxyString)
+ {
+ int colonPosition = proxyString.IndexOf(":");
+ string portNumberString = proxyString.Substring(colonPosition + 1);
+ return Int32.Parse(portNumberString);
+ }
+
+ [Fact (Skip = "Docker nut supported in Windows CI")]
+ public void ConnectViaProxy()
+ {
+ var portString = RunDockerCommand("port haproxy");
+ var portNumber = ParseProxyPort(portString);
+
+ cache_.GetPoolManager()
+ .CreateFactory()
+ .SetSniProxy("localhost", portNumber)
+ .AddLocator("locator-maeve", 10334)
+ .Create("pool");
+
+ var region = cache_.CreateRegionFactory(RegionShortcut.PROXY)
+ .SetPoolName("pool")
+ .Create<string, string>("jellyfish");
+
+ region.Put("1", "one");
+ var value = region.Get("1");
+
+ Assert.Equal("one", value);
+ cache_.Close();
+ }
+
+ [Fact (Skip = "Docker nut supported in Windows CI")]
Review comment:
Perhaps you mean "not" rather than "nut". Also that statement is false,
Docker is fully supported in Windows so why are we not skipping it?
##########
File path: cppcache/src/PoolAttributes.cpp
##########
@@ -44,46 +44,13 @@ PoolAttributes::PoolAttributes()
m_subsEnabled(PoolFactory::DEFAULT_SUBSCRIPTION_ENABLED),
m_multiuserSecurityMode(PoolFactory::DEFAULT_MULTIUSER_SECURE_MODE),
m_isPRSingleHopEnabled(PoolFactory::DEFAULT_PR_SINGLE_HOP_ENABLED),
- m_serverGrp(PoolFactory::DEFAULT_SERVER_GROUP) {}
+ m_serverGrp(PoolFactory::DEFAULT_SERVER_GROUP),
+ m_sniProxyPort(0) {}
+
std::shared_ptr<PoolAttributes> PoolAttributes::clone() {
return std::make_shared<PoolAttributes>(*this);
}
-/** Return true if all the attributes are equal to those of other. */
-bool PoolAttributes::operator==(const PoolAttributes& other) const {
Review comment:
Why did we get rid of this method to compare two pool attributes?
##########
File path: cppcache/include/geode/PoolFactory.hpp
##########
@@ -426,6 +426,11 @@ class APACHE_GEODE_EXPORT PoolFactory {
*/
PoolFactory& addServer(const std::string& host, int port);
+ /**
+ * Set proxy info for SNI connection. Used for connecting via SNI proxy.
+ */
+ PoolFactory& setSniProxy(const std::string& hostname, const int port);
Review comment:
Deviates from [SNI Proxy
RFC](https://cwiki.apache.org/confluence/display/GEODE/Client+side+configuration+for+a+SNI+proxy).
##########
File path: clicache/src/PoolFactory.hpp
##########
@@ -279,6 +279,20 @@ namespace Apache
/// </exception>
PoolFactory^ AddServer(String^ host, Int32 port);
+ /// <summary>
+ /// Set proxy info for SNI connection.
+ /// </summary>
+ /// <remarks>
+ /// Used for connecting via SNI proxy.
+ /// </remarks>
+ /// <param>
+ /// host the host name or ip address that the server is listening on.
+ /// </param>
+ /// <param>
+ /// port the port that the server is listening on.
+ /// </param>
+ PoolFactory^ SetSniProxy(String^ hostname, Int32 port);
Review comment:
Deviates from [SNI Proxy
RFC](https://cwiki.apache.org/confluence/display/GEODE/Client+side+configuration+for+a+SNI+proxy).
##########
File path: clicache/integration-test2/SNITests.cs
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Diagnostics;
+using System.IO;
+using Xunit;
+using PdxTests;
+using System.Collections;
+using System.Collections.Generic;
+using Xunit.Abstractions;
+
+namespace Apache.Geode.Client.IntegrationTests
+{
+ [Trait("Category", "Integration")]
+ public class SNITests : TestBase, IDisposable
+ {
+ string currentWorkingDirectory;
+ Process dockerProcess;
+ private readonly Cache cache_;
+
+ public SNITests(ITestOutputHelper testOutputHelper) :
base(testOutputHelper)
+ {
+ currentWorkingDirectory = Directory.GetCurrentDirectory();
+ var clientTruststore = Config.SslClientKeyPath +
@"/truststore_sni.pem";
+
+
+
+ var cacheFactory = new CacheFactory();
+ cacheFactory.Set("log-level", "none");
+ cacheFactory.Set("log-file", "c:/temp/SNITest-csharp.log");
+ cacheFactory.Set("ssl-enabled", "true");
+ cacheFactory.Set("ssl-truststore", clientTruststore);
+
+ cache_ = cacheFactory.Create();
+
+ var dc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "-f " +
Config.SniConfigPath + "/docker-compose.yml" + " up -d");
+ dc.WaitForExit();
+
+ var d = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "exec -t geode gfsh run
--file=/geode/scripts/geode-starter.gfsh");
+ d.WaitForExit();
+ }
+
+ public void Dispose()
+ {
+
+ var dockerComposeProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker-compose.exe", "stop");
+ dockerComposeProc.WaitForExit();
+
+ var dockerProc = Process.Start(@"C:\Program
Files\docker\docker\resources\bin\docker.exe", "container prune -f");
Review comment:
There is a Docker C++ SDK we could use rather than forking an executable.
##########
File path: cppcache/integration/test/SNITest.cpp
##########
@@ -108,41 +112,51 @@ class SNITest : public ::testing::Test {
}
std::string certificatePassword;
+ boost::filesystem::path clientSslKeysDir;
boost::filesystem::path currentWorkingDirectory;
+ boost::filesystem::path sniConfigPath;
};
-TEST_F(SNITest, DISABLED_connectViaProxyTest) {
+#if defined(_WIN32)
+TEST_F(SNITest, DISABLE_connectViaProxyTest) {
+#else
+TEST_F(SNITest, connectViaProxyTest) {
+#endif
const auto clientTruststore =
- (currentWorkingDirectory /
- boost::filesystem::path("sni-test-config/geode-config/truststore.jks"));
+ (clientSslKeysDir / boost::filesystem::path("/truststore_sni.pem"));
auto cache = CacheFactory()
- .set("log-level", "DEBUG")
+ .set("log-level", "debug")
+ .set("log-file", "SNITest.log")
.set("ssl-enabled", "true")
.set("ssl-truststore", clientTruststore.string())
.create();
- auto portString = makeItSo("docker port haproxy");
+ auto portString = runDockerCommand("docker port haproxy");
auto portNumber = parseProxyPort(portString);
cache.getPoolManager()
.createFactory()
- .addLocator("localhost", portNumber)
+ .setSniProxy("localhost", portNumber)
+ .addLocator("locator-maeve", 10334)
.create("pool");
auto region = cache.createRegionFactory(RegionShortcut::PROXY)
.setPoolName("pool")
- .create("region");
+ .create("jellyfish");
region->put("1", "one");
cache.close();
}
-TEST_F(SNITest, connectionFailsTest) {
+#if defined(_WIN32)
+TEST_F(SNITest, DISABLE_connectWithoutProxyFails) {
Review comment:
Why is this test disabled on Windows?
##########
File path: cppcache/src/TcpSslConn.cpp
##########
@@ -87,15 +97,13 @@ void TcpSslConn::connect() {
ACE_OS::signal(SIGPIPE, SIG_IGN); // Ignore broken pipe
- // m_ssl->init();
-
std::chrono::microseconds waitMicroSeconds = m_waitMilliSeconds;
LOGDEBUG("Connecting SSL socket stream to %s:%d waiting %s micro sec",
m_addr.get_host_name(), m_addr.get_port_number(),
to_string(waitMicroSeconds).c_str());
- int32_t retVal = m_ssl->connect(m_addr, waitMicroSeconds);
+ int32_t retVal = m_ssl->connect(m_addr, m_sniHostname, waitMicroSeconds);
Review comment:
`auto`
##########
File path: cppcache/src/ThinClientLocatorHelper.hpp
##########
@@ -71,6 +74,8 @@ class ThinClientLocatorHelper {
const ThinClientPoolDM* m_poolDM;
ThinClientLocatorHelper(const ThinClientLocatorHelper&);
ThinClientLocatorHelper& operator=(const ThinClientLocatorHelper&);
+ std::string m_sniProxyHost;
Review comment:
If the set of member variable is small perhaps its time to make them
conform to the style guide. (optional)
##########
File path: cppcache/src/TcpSslConn.hpp
##########
@@ -67,14 +81,18 @@ class TcpSslConn : public TcpConn {
const char* privkeyfile, const char* pemPassword)
: TcpConn(ipaddr, waitSeconds, maxBuffSizePool),
m_ssl(nullptr),
+ m_sniPort(0),
+ m_sniHostname(""),
Review comment:
Initializing a `std::string` with `""` is redundant.
##########
File path: sni-test-config/docker-compose.yml
##########
@@ -0,0 +1,43 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+version: '3'
+services:
+ geode:
+ container_name: 'geode'
+ image: 'apachegeode/geode'
+ expose:
+ - '10334'
+ - '40404'
+ entrypoint: 'sh'
+ command: ["-c", "while true; do sleep 600; done"]
Review comment:
If you aren't using the forever script then why include the script in
the check in?
##########
File path:
cppcache/integration/test/sni-test-config/geode-config/truststore_sni.pem
##########
@@ -0,0 +1,68 @@
+-----BEGIN CERTIFICATE-----
Review comment:
Why do we use a pre-baked key and certificates here? I though all the
SSL tests generated certs on the fly but maybe I am wrong. In the past this has
caused issues with hostname validation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> extend C# (existing, ACE sockets) PoolFactoryAPI to support SNI proxy
> ---------------------------------------------------------------------
>
> Key: GEODE-8398
> URL: https://issues.apache.org/jira/browse/GEODE-8398
> Project: Geode
> Issue Type: New Feature
> Components: native client
> Reporter: Ernest Burghardt
> Priority: Major
> Labels: pull-request-available
>
> clicache
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
