[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r473251486
##
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:
Oh good catch. I didn't even notice that initially over the whole, why
not use the SDK thing. If the absolute path is necessary, which I doubt it is,
then use CMake `find_program` and inject. It should be sufficient to just make
sure docker is in the PATH or add it to the PATH in the CTest definition.
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:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r473244538 ## File path: clicache/src/PoolFactory.hpp ## @@ -279,6 +279,20 @@ namespace Apache /// PoolFactory^ AddServer(String^ host, Int32 port); +/// +/// Set proxy info for SNI connection. +/// +/// +/// Used for connecting via SNI proxy. +/// +/// +/// host the host name or ip address that the server is listening on. +/// +/// +/// port the port that the server is listening on. +/// +PoolFactory^ SetSniProxy(String^ hostname, Int32 port); Review comment: Offline conversation: The team is comfortable with the limitations of this API in the effort to get something released and are comfortable with potentially deprecating this API in favor of something more flexible and in line with the RFC when and if expansion of this feature is necessary. 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: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r473107131 ## File path: cppcache/integration/test/sni-test-config/geode-config/truststore_sni.pem ## @@ -0,0 +1,68 @@ +-BEGIN CERTIFICATE- Review comment: Is there a technical reason we can’t do it for the client too? 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: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r473106328 ## File path: clicache/src/PoolFactory.hpp ## @@ -279,6 +279,20 @@ namespace Apache /// PoolFactory^ AddServer(String^ host, Int32 port); +/// +/// Set proxy info for SNI connection. +/// +/// +/// Used for connecting via SNI proxy. +/// +/// +/// host the host name or ip address that the server is listening on. +/// +/// +/// port the port that the server is listening on. +/// +PoolFactory^ SetSniProxy(String^ hostname, Int32 port); Review comment: So why not provide, like the Java API, an interface that can take pre-canned `SocketFactory` classes. At this point the only deviation would be the ability for users to provide their own factories? 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: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r473106328 ## File path: clicache/src/PoolFactory.hpp ## @@ -279,6 +279,20 @@ namespace Apache /// PoolFactory^ AddServer(String^ host, Int32 port); +/// +/// Set proxy info for SNI connection. +/// +/// +/// Used for connecting via SNI proxy. +/// +/// +/// host the host name or ip address that the server is listening on. +/// +/// +/// port the port that the server is listening on. +/// +PoolFactory^ SetSniProxy(String^ hostname, Int32 port); Review comment: So why not provide, like the Java API, and interface that can take pre-canned `SocketFactory` classes. At this point the only deviation would be the ability for users to provide their own factories? 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: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634: URL: https://github.com/apache/geode-native/pull/634#discussion_r472527125 ## File path: clicache/src/PoolFactory.hpp ## @@ -279,6 +279,20 @@ namespace Apache /// PoolFactory^ AddServer(String^ host, Int32 port); +/// +/// Set proxy info for SNI connection. +/// +/// +/// Used for connecting via SNI proxy. +/// +/// +/// host the host name or ip address that the server is listening on. +/// +/// +/// port the port that the server is listening on. +/// +PoolFactory^ SetSniProxy(String^ hostname, Int32 port); Review comment: A difference in API is a pretty big surprise to some. Especially when you consider that .NET does expose a Socket concept in the SDK similar to Java. Having a discussion beforehand as to why one API would deviate from another established in an RFC helps reviewers understand the rational for the deviation. I can only make the assumption that the deviation is a result of the C++ layer not wanting to leak `ACE::Socket` outside the internals. There are alternative ways to do that such that you have an API similar to the Java API that is extensible for future proxy implementations or parameters to the proxy connections itself without exposing the internals of the socket implementation. By not having this conversation we have implemented something that solves today's issue but will need to be deprecated or augmented to support future changes to the proxy strategy. 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: us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472525063
##
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("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:
I am running Docker on Windows in GCP just fine. Can I help?
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,
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
pivotal-jbarrett commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472524195
##
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:
https://github.com/lasote/docker_client
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:
us...@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #634: GEODE-8398: Add SNI support to .NET API
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("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),
