SSL enablement for Cassandra Connector by lithinwxd · Pull Request #23507 · prestodb/presto

lithinwxd · 2024-08-23T12:07:04Z

Description

Cassandra Version Upgrade for enabling SSL

Motivation and Context

Presto uses presto cassandra driver (shaded version of datastax driver ) for connections . While non ssl operations work fine , the same does not hold good for SSL functionalities.

While we try to connect to an SSL instance , we get the below error

com.datastax.driver.core.exceptions.NoHostAvailableException: All host(s) tried for query failed (tried: /52.116.130.195:9042 (com.datastax.driver.core.exceptions.OperationTimedOutException: [/52.116.130.195:9042] Operation timed out))

Presto uses 3.6.0 version of DataStax driver which does not support SSL. Version 3.11.2 or higher version of DataStax driver supports SSL.

#23506

Impact

SSL will be enabled and connections can be secured.

Test Plan

Tested Locally end to end

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Cassandra Connector Changes

Upgrade cassandra-driver-core to 3.11.5 for SSL support :pr:23493

tdcmeehan · 2024-08-26T14:05:11Z

...cassandra/src/test/java/com/facebook/presto/cassandra/TestCassandraIntegrationSmokeTest.java

        session.execute("CREATE TABLE keyspace_4.\"TaBlE_4\" (column_4 bigint PRIMARY KEY)");
        session.execute("CREATE TABLE keyspace_4.\"tAbLe_4\" (column_4 bigint PRIMARY KEY)");

+        Thread.sleep(5000);


Why do we need the sleep?

The sleep is needed for cassandra to refresh its metadata . i have reduced it to 1000ms in the recent commit

What metadata is being refreshed asynchronously? Can you help me explain why the changes in this PR require this new sleep?

Without this delay, the test might run into race conditions where it tries to access or query the newly created schema elements before they are fully registered in the system, leading to test failures.The driver now performs more rigorous checks to ensure schema agreement across nodes before refreshing the metadata. This could introduce a delay, especially if schema changes are not immediately visible across the cluster, potentially causing issues like the one we are facing where the test fails if you don't include a Thread.sleep. The driver waits for all nodes to agree on the schema before completing the operation, which wasn't as strictly enforced in earlier versions.

Doesn't the assertContainsEventually call immediately after this sleep handle this race condition?

No , without the Thread.sleep its failing and after adding the sleep it passes . The sleep was added for the testcase to pass and I checked by printing the values and after adding sleep I was able to retrieve values before which it was empty.

OK. Please add a comment explaining why we need the sleep.

@tdcmeehan I have added the comment . Thanks

tdcmeehan · 2024-08-26T14:05:16Z

...cassandra/src/test/java/com/facebook/presto/cassandra/TestCassandraIntegrationSmokeTest.java


    @Test
    public void testTableNameAmbiguity()
+            throws InterruptedException


Suggested change

throws InterruptedException

throws Exception

tdcmeehan · 2024-08-26T14:05:24Z

...cassandra/src/test/java/com/facebook/presto/cassandra/TestCassandraIntegrationSmokeTest.java

                .row("table_4")
                .row("table_4")
-                .build(), new Duration(1, MINUTES));
+                .build(), new Duration(2, MINUTES));


Why is this being bumped up?

okay this is not required , I shall revert back as 1000ms would do the refreshing

tdcmeehan · 2024-08-26T14:05:49Z

presto-cassandra/pom.xml

        <dependency>
-            <groupId>com.facebook.presto.cassandra</groupId>
-            <artifactId>cassandra-driver</artifactId>
+            <groupId>com.datastax.cassandra</groupId>


Can you put this in the dependency management section of the parent pom?

tdcmeehan · 2024-08-26T14:05:58Z

presto-cassandra/pom.xml

        </dependency>
    </dependencies>

+    <dependencyManagement>


Can you add a comment explaining why this is needed?

This is to fix the upper bound issues arising out of different components

Can you leave a comment in the code to that effect?

added the comment

steveburnett · 2024-08-26T14:20:58Z

Nit formatting suggestion for the release note entry

== RELEASE NOTES ==

Cassandra Connector Changes
* Upgrade cassandra-driver-core to 3.11.5 for SSL support :pr:`23493`

lithinwxd · 2024-08-28T08:48:00Z

Nit formatting suggestion for the release note entry

== RELEASE NOTES ==

Cassandra Connector Changes
* Upgrade cassandra-driver-core to 3.11.5 for SSL support :pr:`23493`

done

tdcmeehan · 2024-09-04T14:47:35Z

Please squash commits

lithinwxd · 2024-09-05T07:54:23Z

Please squash commits

done

lithinwxd marked this pull request as ready for review August 23, 2024 12:09

lithinwxd requested a review from a team as a code owner August 23, 2024 12:09

lithinwxd requested a review from presto-oss August 23, 2024 12:09

tdcmeehan requested changes Aug 26, 2024

View reviewed changes

tdcmeehan self-assigned this Aug 26, 2024

SSL enablement for Cassandra connector

edaaf54

lithinwxd force-pushed the master branch from 7639be9 to edaaf54 Compare September 5, 2024 07:44

tdcmeehan approved these changes Sep 5, 2024

View reviewed changes

tdcmeehan merged commit eb7ef88 into prestodb:master Sep 5, 2024

jaystarshot mentioned this pull request Nov 1, 2024

Add release notes for 0.290 #23936

Merged

25 tasks

Comments

Conversation

lithinwxd commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lithinwxd Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lithinwxd Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lithinwxd Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Aug 26, 2024

Uh oh!

lithinwxd commented Aug 28, 2024

Uh oh!

tdcmeehan commented Sep 4, 2024

Uh oh!

lithinwxd commented Sep 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lithinwxd commented Aug 23, 2024 •

edited

Loading

lithinwxd Aug 29, 2024 •

edited

Loading

lithinwxd Aug 30, 2024 •

edited

Loading

lithinwxd Aug 29, 2024 •

edited

Loading