Enable TLS Support (DTS - Specific) #515

Vipinofficial11 · 2024-10-10T09:04:27Z

Add SSL/TLS support in Oracle Batch Source Classes

This PR introduces a new field, useSSL, to enable SSL (TLS) connections for Oracle by allowing the specification of protocol (tcp or tcps) in the connection string format.

Key Changes

New field

useSSL (Allows to specify whether SSL (TLS) should be used for connections or not )

If useSSL is set to yes, the protocol in the connection string is set to tcps. Otherwise, it defaults to tcp.

Connection String Format Updated:

For SID-based connections: jdbc:oracle:thin:@tcps or @tcp ://<host>:<port>/<SID>
For Service Name-based connections: : jdbc:oracle:thin:tcps or @tcp ://<host>:<port>/<ServiceName>

Testing Scope

Validated TCP protocol connectivity:
Verified that the connection works as expected when specifying protocol as tcp explicitly in the connection string to ensure backward compatibility with existing behavior.
Validated TCPS protocol connectivity:
Conducted internal tests with tcps to ensure secure connections (SSL/TLS) are correctly established and that the updated connection string format handles this protocol appropriately.

itsankit-google · 2024-10-10T12:57:46Z

oracle-plugin/src/main/java/io/cdap/plugin/oracle/OracleConnectorConfig.java

+  @Name(OracleConstants.USE_SSL)
+  @Description("Turns on SSL encryption. Connection will fail if SSL is not available")
+  @Nullable
+  public String useSSL;


Why not boolean?

When we add something in plugin config, if we don't add a widget json for this. It will show up as a textbox in CDAP UI as well.

If we don't want it to be visible in CDAP UI, please add a config in widgets json which is hidden.

Why not boolean?
There was no specific reason, change it to Boolean.

When we add something in plugin config, if we don't add a widget json for this. It will show up as a textbox in CDAP UI as well. If we don't want it to be visible in CDAP UI, please add a config in widgets json which is hidden.

got it, added it in the config of Batch Source and Batch Sink as only these are using OracleConnectorConfig. [8d62fb9]

There will also be a widget json file for OracleConnector which also need to be modified the same way.

Updated [b68ec93]

itsankit-google · 2024-10-10T14:09:29Z

oracle-plugin/src/main/java/io/cdap/plugin/oracle/OracleConnectorConfig.java

@@ -103,6 +109,10 @@ public String getDatabase() {
    return database;
  }

+  public Boolean getSSlMode() {
+    return useSSL;


For existing pipelines this property will be null, please handle that case.

Done, please have a look if this change would be enough to avoid failures in case of existing pipelines.

[https://github.com//pull/515/commits/b68ec933ff27ff2a1ade2c578727113604803367#diff-afe65e631b8b5b1d360af43ae84dc844e2c55ee83e9179cdb8bd087da954ae06R114]

itsankit-google · 2024-10-10T14:13:14Z

oracle-plugin/src/main/java/io/cdap/plugin/oracle/OracleConstants.java

+   * @param useSSL Whether SSL/TLS is required(YES/NO)
+   * @return Connection String based on the given parameters and connection type.
+   */
+  public static String getConnectionString(String connectionType,


I cannot see any reason of this new method, why can't we reuse the existing one?

This was actually overloaded to reduce the impact of these changes on CDAP connectors, this method will only be used by Source and Sink plugin.

Oracle Action, Post Action and Oracle Connector will still use the old method to generate connection string. When we enable TLS for CDAP Connectors we can reuse the code and remove this method.

If we are using this method here: https://github.com/data-integrations/database-plugins/pull/515/files#diff-bd85394fe9ff353467fd45fd10ad329b508e0fbbc70145fe34de0d54f483a99bR129

It means it does impact CDAP connectors so it does not really help by overriding the method.

got it, removed. [b68ec93]

itsankit-google · 2024-10-15T07:40:37Z

Please check e2e test failures.

itsankit-google · 2024-10-15T07:44:05Z

oracle-plugin/src/main/java/io/cdap/plugin/oracle/OracleConstants.java

-  public static final String ORACLE_CONNECTION_STRING_SID_FORMAT = "jdbc:oracle:thin:@%s:%s:%s";
-  public static final String ORACLE_CONNECTION_STRING_SERVICE_NAME_FORMAT = "jdbc:oracle:thin:@//%s:%s/%s";
+  // Updating connection strings to accept protocol (e.g., jdbc:oracle:thin:@<protocol>://<host>:<port>/<SID>)
+  public static final String ORACLE_CONNECTION_STRING_SID_FORMAT = "jdbc:oracle:thin:@%s:%s:%s/%s";


We can add a constant ORACLE_CONNECTION_STRING_SID_FORMAT_WITH_PROTOCOL instead of modifying the older one and use the new one when useSSL is true otherwise use the old connection string.

This would make the change backward compatible and ensure CDAP connectors are not affected.

got it, updated [b31e53c]

itsankit-google · 2024-10-15T07:44:34Z

oracle-plugin/src/main/java/io/cdap/plugin/oracle/OracleConstants.java

-  public static final String ORACLE_CONNECTION_STRING_SERVICE_NAME_FORMAT = "jdbc:oracle:thin:@//%s:%s/%s";
+  // Updating connection strings to accept protocol (e.g., jdbc:oracle:thin:@<protocol>://<host>:<port>/<SID>)
+  public static final String ORACLE_CONNECTION_STRING_SID_FORMAT = "jdbc:oracle:thin:@%s:%s:%s/%s";
+  public static final String ORACLE_CONNECTION_STRING_SERVICE_NAME_FORMAT = "jdbc:oracle:thin:@%s://%s:%s/%s";


similar change here ORACLE_CONNECTION_STRING_SERVICE_NAME_FORMAT_WITH_PROTOCOL

updated [b31e53c]

Vipinofficial11 · 2024-10-15T09:22:06Z

Please check e2e test failures.

Sure, I was looking at the reports but looks like there is some issue. Can you check if you are able to access these link

itsankit-google · 2024-10-15T09:24:42Z

oracle-plugin/src/main/java/io/cdap/plugin/oracle/OracleConstants.java

+        if (isSSLEnabled) {
+          return String.format(OracleConstants.ORACLE_CONNECTION_STRING_SERVICE_NAME_FORMAT_WITH_PROTOCOL,
+              connectionProtocol, host, port, database);
+        }
+        return String.format(OracleConstants.ORACLE_CONNECTION_STRING_SERVICE_NAME_FORMAT,
+            host, port, database);


can we have two methods which takes host, port, database & connectionProtocol getConnectionStringWithServic & getConnectionStringWithSID?

Added separate methods to get the different type of connection strings. [7dc9a6c]

itsankit-google · 2024-10-15T16:47:19Z

Please check e2e test failures.

Sure, I was looking at the reports but looks like there is some issue. Can you check if you are able to access these link

If you see a step before, I don't even see these files getting uploaded:

Vipinofficial11 · 2024-10-15T17:05:24Z

Please check e2e test failures.

Sure, I was looking at the reports but looks like there is some issue. Can you check if you are able to access these link

If you see a step before, I don't even see these files getting uploaded:

Yes, looks like some issue with reporting plugin for oracle. I might have seen this before, will try to fix it.

I was looking at older reports, even for the plugins where files were uploaded we are still not able to access the public bucket. see this

itsankit-google · 2024-10-15T17:21:30Z

Please check e2e test failures.

Sure, I was looking at the reports but looks like there is some issue. Can you check if you are able to access these link

If you see a step before, I don't even see these files getting uploaded:

Yes, looks like some issue with reporting plugin for oracle. I might have seen this before, will try to fix it.

I was looking at older reports, even for the plugins where files were uploaded we are still not able to access the public bucket. see this

If the runs are 7 days old, the reports get deleted automatically from the storage bucket.

Enable TLS Support (DTS - Specific)

87175f1

Vipinofficial11 requested review from albertshau, itsankit-google and MrRahulSharma October 10, 2024 09:04

Vipinofficial11 added the build label Oct 10, 2024

itsankit-google reviewed Oct 10, 2024

View reviewed changes

Hide useSSL Field from UI and fix unit test failure

8d62fb9

itsankit-google reviewed Oct 10, 2024

View reviewed changes

Handle null case for SSl mode and Update connector json

b68ec93

itsankit-google reviewed Oct 15, 2024

View reviewed changes

Separate Connection String Formats.

b31e53c

itsankit-google reviewed Oct 15, 2024

View reviewed changes

Add separate utility methods to get connection string.

7dc9a6c

Vipinofficial11 force-pushed the enableSSLConnections branch 2 times, most recently from cd5f103 to b9bf75f Compare October 16, 2024 19:36

Run tests with TestRunner.java

ae5d1c9

Vipinofficial11 force-pushed the enableSSLConnections branch from b9bf75f to ae5d1c9 Compare October 16, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable TLS Support (DTS - Specific) #515

Enable TLS Support (DTS - Specific) #515

Vipinofficial11 commented Oct 10, 2024 •

edited

Loading

itsankit-google Oct 10, 2024

Vipinofficial11 Oct 10, 2024 •

edited

Loading

itsankit-google Oct 10, 2024

Vipinofficial11 Oct 11, 2024

itsankit-google Oct 10, 2024

Vipinofficial11 Oct 11, 2024

itsankit-google Oct 10, 2024

Vipinofficial11 Oct 10, 2024 •

edited

Loading

itsankit-google Oct 10, 2024

Vipinofficial11 Oct 11, 2024

itsankit-google commented Oct 15, 2024

itsankit-google Oct 15, 2024 •

edited

Loading

Vipinofficial11 Oct 15, 2024

itsankit-google Oct 15, 2024

Vipinofficial11 Oct 15, 2024

Vipinofficial11 commented Oct 15, 2024 •

edited

Loading

itsankit-google Oct 15, 2024

Vipinofficial11 Oct 15, 2024

itsankit-google commented Oct 15, 2024

Vipinofficial11 commented Oct 15, 2024 •

edited

Loading

itsankit-google commented Oct 15, 2024

Enable TLS Support (DTS - Specific) #515

Are you sure you want to change the base?

Enable TLS Support (DTS - Specific) #515

Conversation

Vipinofficial11 commented Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Vipinofficial11 Oct 10, 2024 • edited Loading

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

Vipinofficial11 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsankit-google commented Oct 15, 2024

itsankit-google Oct 15, 2024 • edited Loading

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

Vipinofficial11 commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsankit-google commented Oct 15, 2024

Vipinofficial11 commented Oct 15, 2024 • edited Loading

itsankit-google commented Oct 15, 2024

Vipinofficial11 commented Oct 10, 2024 •

edited

Loading

Vipinofficial11 Oct 10, 2024 •

edited

Loading

Vipinofficial11 Oct 10, 2024 •

edited

Loading

itsankit-google Oct 15, 2024 •

edited

Loading

Vipinofficial11 commented Oct 15, 2024 •

edited

Loading

Vipinofficial11 commented Oct 15, 2024 •

edited

Loading