GEODE-8398: Add SNI support to .NET API #634

mmartell · 2020-08-17T19:04:56Z

This adds a new .NET API to the nativeclient to support SNI.

Notes:

This ticket usurps GEODE-8156. Since we need the new SetSniProxy API to run the test, we'll use this GEODE-8398 for the pull request.
The SNI tests are currently disabled on Windows platforms due to issues with running docker in the CI.

jake-at-work

I am concerned with the deviation without discussion from the establish API based on the SNI Proxy RFC that was previously approved. Can you please open a discussion thread on dev@geode regarding the reasons for the C++ and .NET APIs deviate from the RFC? I would have expected work like this to have come with it's own RFC if it was going to deviate from previous RFCs.

https://cwiki.apache.org/confluence/display/GEODE/Client+side+configuration+for+a+SNI+proxy

jake-at-work · 2020-08-17T22:03:21Z

clicache/integration-test2/SNITests.cs

+            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");


There is a Docker C++ SDK we could use rather than forking an executable.

not that we have found, unfortunately...

https://github.com/lasote/docker_client

thx! we'll take a look at this for future improvements, current deadline isn't going to allow

Still, it is odd to hard code the docker executable paths.

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.

jake-at-work · 2020-08-17T22:05:12Z

clicache/integration-test2/SNITests.cs

+            cache_.Close();
+        }
+
+        [Fact (Skip = "Docker nut supported in Windows CI")]


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?

its Docker on GCP that is specifically the issue, but thanks for catching the typo

I am running Docker on Windows in GCP just fine. Can I help?

clicache/src/PoolFactory.hpp

cppcache/include/geode/PoolFactory.hpp

jake-at-work · 2020-08-17T22:08:19Z

cppcache/integration/test/SNITest.cpp


  region->put("1", "one");

  cache.close();
 }

-TEST_F(SNITest, connectionFailsTest) {
+#if defined(_WIN32)
+TEST_F(SNITest, DISABLE_connectWithoutProxyFails) {


Why is this test disabled on Windows?

aforementioned Docker issues...

cppcache/src/PoolAttributes.cpp

cppcache/src/TcpSslConn.cpp

cppcache/src/TcpSslConn.hpp

cppcache/src/ThinClientLocatorHelper.hpp

sni-test-config/docker-compose.yml

moleske

There's no instructions on how to run the Fact (Skip = "... tests. I assume you need docker installed but is there anything else. There should be instructions probably in CONTRIBUTING.md because folks will want to run these tests on their own machines.

Also, it is really strange to add "Skip CI" on tests when we don't have a CI that runs these tests. If one is running a private CI pipeline, that sounds like the skipping of tests should be done in your private pipeline, not the public code

moleske · 2020-08-19T18:47:11Z

clicache/integration-test2/SNITests.cs

+            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");


Still, it is odd to hard code the docker executable paths.

jake-at-work · 2020-08-19T18:55:31Z

clicache/integration-test2/SNITests.cs

+            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");


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.

- also fix formatting

- TcpSslConn ctor was getting a string "host:port" instead of just "host", and needed to split off just the hostname

- Remove hard-coded path to Docker binaries - Update "Skip" comment for SNI tests

echobravopapa · 2020-08-19T21:58:00Z

@pivotal-jbarrett latest mergable is up...

mmartell requested review from pdxcodemonkey, echobravopapa and jake-at-work August 17, 2020 19:05

jake-at-work requested changes Aug 17, 2020

View reviewed changes

GEODE-8398: Incorporate SNI proxy support into native client API

b25017f

jake-at-work approved these changes Aug 19, 2020

View reviewed changes

moleske reviewed Aug 19, 2020

View reviewed changes

jake-at-work requested changes Aug 19, 2020

View reviewed changes

pdxcodemonkey added 2 commits August 19, 2020 13:32

GEODE-8398: Remove redundant initialization of string (PR feedback)

f6a3cee

- also fix formatting

GEODE-8398: Fix SNI tests

6f39766

- TcpSslConn ctor was getting a string "host:port" instead of just "host", and needed to split off just the hostname

pdxcodemonkey force-pushed the GEODE-8398-sni-support-dotnet branch from a9b20a7 to 6f39766 Compare August 19, 2020 21:36

GEODE-8398: PR Feedback

21236e7

- Remove hard-coded path to Docker binaries - Update "Skip" comment for SNI tests

echobravopapa requested a review from jake-at-work August 19, 2020 21:58

jake-at-work approved these changes Aug 19, 2020

View reviewed changes

pdxcodemonkey merged commit 557e018 into apache:develop Aug 19, 2020

mmartell deleted the GEODE-8398-sni-support-dotnet branch November 17, 2021 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-8398: Add SNI support to .NET API #634

GEODE-8398: Add SNI support to .NET API #634

mmartell commented Aug 17, 2020

jake-at-work left a comment

jake-at-work Aug 17, 2020

echobravopapa Aug 18, 2020

jake-at-work Aug 18, 2020

echobravopapa Aug 19, 2020

moleske Aug 19, 2020

jake-at-work Aug 19, 2020

jake-at-work Aug 17, 2020

echobravopapa Aug 18, 2020

jake-at-work Aug 18, 2020

jake-at-work Aug 17, 2020

echobravopapa Aug 18, 2020

moleske left a comment

moleske Aug 19, 2020

jake-at-work Aug 19, 2020

echobravopapa commented Aug 19, 2020

GEODE-8398: Add SNI support to .NET API #634

GEODE-8398: Add SNI support to .NET API #634

Conversation

mmartell commented Aug 17, 2020

jake-at-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moleske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echobravopapa commented Aug 19, 2020