Repository: GoogleCloudPlatform/professional-services-data-validator

Issue:
DVT unable to connect to Oracle from within a container when using oracledb thin client
**Describe the bug**
DVT has a [PR 1515](https://github.com/GoogleCloudPlatform/professional-services-data-validator/pull/1515) in-flight to use the `python-oracledb` driver instead of the `cx_Oracle` driver. `cx_Oracle` has been deprecated by Oracle and the new driver is `oracledb`. In thin mode, this driver does not require that the Oracle Client be installed locally, so is the preferred option for DVT.

When using this driver, connections from within a container do not succeed unless `disable_oob=True`. This flag tells the driver to not use out of band break as outlined [here](https://docs.oracle.com/en/database/oracle/oracle-database/19/netrf/parameters-for-the-sqlnet.ora.html#GUID-42E939DC-EF37-49A0-B4F0-14158F0E55FD)

**What version of DVT are you using?**
After PR 1515 is accepted

**What type of connections are you using for source and target?**
Oracle for source or target

**Steps to reproduce the behavior**
Neil reports [here](https://github.com/GoogleCloudPlatform/professional-services-data-validator/pull/1515#issuecomment-2880200466) that connections hang and the resolution is to invoke `connect` as 
```
with oracledb.connect(user='usr', password='pwd', dsn='localhost:1521/pdb1', disable_oob=True) as conn:
```

**Expected behavior**
There should be a straightforward way to set this and other parameters for the connection. In Teradata, users specify `--json-parms` for parameters. DVT can provide similar functionality for Oracle connections, perhaps call it `--connect-parms`. According to the documentation [here](https://python-oracledb.readthedocs.io/en/latest/api_manual/connect_params.html#ConnectParams.set_from_config), we can provide a dictionary called `pyo` and those values can be used to set ConnectParams.

Discussion:
@nj1973: I've been able to connect using the code in https://github.com/GoogleCloudPlatform/professional-services-data-validator/pull/1515 in two different ways.

**Thick client**

I added the following lines to the `sa.create_engine` call:
```
            thick_mode={
                "lib_dir": "/opt/oracle/instantclient_21_13",
                "config_dir": "/opt/oracle/wallet_client",
            },
```

In this example `config_dir` contained the wallets and sqlnet/tnsnames files.

**Thin client**

I added the following lines to the `sa.create_engine` call:
```
            connect_args={
                "disable_oob": True,
                "ssl_server_cert_dn": "CN=oracle-db",
                "wallet_location": "/opt/oracle/wallet_client",
                # "wallet_password": wpw,
            },
```
In this example `wallet_location` contained **only** a PEM file.

We need to decide if we want to support only thin or both thin and thick modes. My preference would be to support both for the extra flexibility.

I propose we allow https://github.com/GoogleCloudPlatform/professional-services-data-validator/pull/1515 to continue without these changes but we **do not** allow a release until these changes have also been made. Including doc updates.@nj1973: I've pushed some changes to the attached branch for Oracle connections `--connect-args`. This gives us thin driver support. I used `--connect-args` as the name to match the Snowflake argument - which also matches the SQL Alchemy argument name.

I have one customer who uses a credential only wallet (no TLS) who will be blocked with these changes because we don't know how to create a `.pem` file from a credential only wallet.@nj1973: While testing I also ran into this: https://python-oracledb.readthedocs.io/en/latest/user_guide/troubleshooting.html#dpy3001

I think we're going to need thick mode support.@nj1973: Issue https://github.com/GoogleCloudPlatform/professional-services-data-validator/issues/1400 might be fixed by this too. We should test it while working on this.@sundar-mudupalli-work: /gcbrun

Solution patch:
diff --git a/data_validation/cli_tools.py b/data_validation/cli_tools.py
index 8e28d3b..6820129 100644
--- a/data_validation/cli_tools.py
+++ b/data_validation/cli_tools.py
@@ -96,9 +96,11 @@ CONNECTION_SOURCE_FIELDS = {
         ["host", "Desired Oracle host"],
         ["port", "Oracle port to connect on"],
         ["user", "User used to connect"],
+        ["thick_mode", "Flag indicating thick_mode"],
         ["password", "Password for supplied user"],
         ["database", "Database to connect to"],
-        ["url", "Oracle SQLAlchemy connection URL"],
+        ["protocol", "Oracle networking protocol (default TPC)"],
+        ["connect_args", "(Optional) Additional connection argument mapping"],
     ],
     consts.SOURCE_TYPE_MSSQL: [
         ["host", "Desired SQL Server host (default localhost)"],
@@ -121,7 +123,7 @@ CONNECTION_SOURCE_FIELDS = {
         ["password", "Password for authentication of user"],
         ["account", "Snowflake account to connect to"],
         ["database", "Database in snowflake to connect to"],
-        ["connect_args", "(Optional) Additional connection arg mapping"],
+        ["connect_args", "(Optional) Additional connection argument mapping"],
     ],
     consts.SOURCE_TYPE_POSTGRES: [
         ["host", "Desired PostgreSQL host."],
@@ -539,7 +541,12 @@ def _configure_database_specific_parsers(parser):
         for field_obj in CONNECTION_SOURCE_FIELDS[database]:
             arg_field = "--" + field_obj[0].replace("_", "-")
             help_txt = field_obj[1]
-            db_parser.add_argument(arg_field, help=help_txt)
+            if (
+                field_obj[0] == "thick_mode" and database == consts.SOURCE_TYPE_ORACLE
+            ):  # flag
+                db_parser.add_argument(arg_field, help=help_txt, action="store_true")
+            else:
+                db_parser.add_argument(arg_field, help=help_txt)
 
 
 def _configure_validate_parser(subparsers):
diff --git a/data_validation/clients.py b/data_validation/clients.py
index 1cd6420..ab478e6 100644
--- a/data_validation/clients.py
+++ b/data_validation/clients.py
@@ -320,6 +320,14 @@ def get_data_client(connection_config):
                 key_path
             )
 
+    # Oracle no longer supports the url option, so check and warn users
+    if (
+        source_type == consts.SOURCE_TYPE_ORACLE
+        and "url" in decrypted_connection_config
+    ):
+        msg = f"Connection Configuration Error: url parameter no longer supported for {source_type}.\nRecreate connection with --connect-args parameter."
+        raise Exception(msg)
+
     if source_type not in CLIENT_LOOKUP:
         msg = 'ConfigurationError: Source type "{source_type}" is not supported'.format(
             source_type=source_type
diff --git a/docs/connections.md b/docs/connections.md
index c7aa389..10be5a2 100644
--- a/docs/connections.md
+++ b/docs/connections.md
@@ -18,37 +18,37 @@ DVT supports [Google Cloud Secret Manager](https://cloud.google.com/secret-manag
 
 If the secret-manager flags are present, any of the remaining connection flags can reference secret names instead of the secret itself.
 
-Example 1: A BigQuery connection referencing a secret with name "dvt-project-secret" stored in project `my-project`:
+Example 1: A BigQuery connection referencing a secret with name PROJECT_SECRET stored in project PROJECT_NAME:
 
 ```sh
 data-validation connections add \
     --secret-manager-type GCP \
-    --secret-manager-project-id my-project \
+    --secret-manager-project-id PROJECT_NAME \
     --connection-name bq BigQuery \
-    --project-id "dvt-project-secret"
+    --project-id PROJECT_SECRET
 ```
 
-Example 2: A PostgreSQL connection referencing a mixture of secrets (for `--host` and `--password`) stored in project `my-project` and simple string tokens:
+Example 2: A PostgreSQL connection referencing a mixture of secrets (for `--host` and `--password`) stored in project PROJECT_NAME and simple string tokens:
 
 ```sh
 data-validation connections add \
     --secret-manager-type GCP \
-    --secret-manager-project-id my-project \
-    --connection-name pg Postgres \
-    --host=pg-host-secret \
-    --user=dvt_user \
-    --password=dvt-password-secret \
-    --database=mydatabase
+    --secret-manager-project-id PROJECT_NAME \
+    --connection-name CONN Postgres \
+    --host=HOST_SECRET \
+    --user=USER_NAME \
+    --password=PASSWORD_SECRET \
+    --database=DATABASE
 ```
 
-Example 3: An entire Oracle URL stored as a secret with name "dvt-url-secret" stored in project `my-project`:
+Example 3: An entire Oracle Credential alias stored as a secret with name CREDENTIAL_SECRET stored in project PROJECT_NAME:
 
 ```sh
 data-validation connections add \
     --secret-manager-type GCP \
-    --secret-manager-project-id my-project \
+    --secret-manager-project-id PROJECT_NAME \
     --connection-name ora_uat Oracle \
-    --url="dvt-url-secret"
+    --connect-args=CREDENTIAL_SECRET
 ```
 
 ## List existing connections
@@ -178,25 +178,27 @@ data-validation connections add
     --password PASSWORD                                 Teradata password
     [--logmech LOGMECH]                                 Teradata logmech, defaults to "TD2"
     [--use-no-lock-tables USE_NO_LOCK_TABLES]           Use access lock for queries, defaults to "False"
-    [--json-params JSON_PARAMS]                         Additional teradatasql JSON string parameters (Optional)
+    [--json-params JSON_PARAMS]                         Additional teradatasql JSON string dict (Optional)
 ```
 
 ## Oracle
 
 Please note the Oracle package is not installed by default. You will need to follow [python-oracledb](https://python-oracledb.readthedocs.io/en/latest/user_guide/installation.html) installation steps.
-Then `pip install oracledb`.
+Then `pip install oracledb`. DVT uses oracledb in Thin Mode by default which permits TLS and mTLS connections. You can also enable thick mode by specifying the `--thick-mode` parameter. Thick mode requires installation of Oracle client libraries. Differences between Thin and Thick mode are [discussed in the docs](https://python-oracledb.readthedocs.io/en/latest/user_guide/appendix_b.html#)
 
 ```
 data-validation connections add
     [--secret-manager-type <None|GCP>]                  Secret Manager type (None, GCP)
     [--secret-manager-project-id SECRET_PROJECT_ID]     Secret Manager project ID
     --connection-name CONN_NAME Oracle                  Connection name
-    --host HOST                                         Oracle host
-    --port PORT                                         Oracle port, defaults to 1521
-    --user USER                                         Oracle user
-    --password PASSWORD                                 Oracle password
-    --database DATABASE                                 Oracle database
-    [--url URL]                                         SQLAlchemy connection URL
+    [--thick-mode]                                      Use Thick mode, requires local Oracle client libraries
+    [--host HOST]                                       Oracle host
+    [--port PORT]                                       Oracle port, defaults to 1521
+    [--user USER]                                       Oracle user, if not specified using credentials stored in wallet (see below)
+    [--protocol PROTOCOL]                               Oracle networking protocol (TPC, TPCS)
+    [--password PASSWORD]                               Oracle password
+    [--database DATABASE]                               Oracle database
+    [--connect-args CONNECT_PARAMS]                     Additional oracledb ConnectParams, JSON String dict
 ```
 
 ### Oracle User permissions to run DVT
@@ -205,16 +207,24 @@ data-validation connections add
 * READ or SELECT on any tables to be validated
 * Optional - Read on SYS.V_$TRANSACTION (required to get isolation level, if privilege is not given then will default to Read Committed, [more_details](https://docs.sqlalchemy.org/en/14/dialects/oracle.html#transaction-isolation-level-autocommit))
 
-### Using an Oracle wallet
+### Additional Connect parameters, using TLS, mTLS connections and running DVT within a container
+oracledb supports a large number of connection parameters documented as [ConnectParams](https://python-oracledb.readthedocs.io/en/latest/api_manual/connect_params.html#ConnectParams.set). Any of these params can be set by providing the `--connect-args` as a python dict. 
 
-After creating an Oracle wallet and supporting configuration you can add the connection using the `--url` option, remembering to set `TNS_ADMIN` correctly before doing so. For example:
+For setting up a TLS connection, specify the configuration directory where `tnsnames.ora` is located, the wallet directory where `ewallet.pem` is located and the distinguished name of the server used when creating the certificate. The protocol, host, port and service_name are best specified in `tnsnames.ora` as they take precedence. For example, the `--connect-args` parameter can be specified as follows:
 
-```sh
-export TNS_ADMIN=/opt/dvt/dvt_tns_admin
+```
+data-validation connections add \
+ --connection-name ora_secure Oracle --user USER --password PASSWORD \
+ --connect-args='{ "wallet_password": PASSWORD, "wallet_location": WALLET_DIR, "config_dir": CONFIG_DIR, "ssl_server_cert_dn": DISTINGUISHED_NAME}'
+```
+When DVT is running in a container, you may need to specify  `"disable_oob": True,` as one of the key value pairs in the `connect-args` dictionary to connect to Oracle.
 
+### Using credentials from a wallet
+When a user name is not specified, credentials (user name and password) are assumed to be in a wallet. Thick mode is automatically used, so Oracle client libraries are required. Only [the name of the credential created with the `mkstore createCredential` command](https://docs.oracle.com/en/database/oracle/oracle-database/23/dbseg/using-the-orapki-utility-to-manage-pki-elements.html#GUID-25509071-ABC0-4A0E-A3DB-4D4F61024F25), the `dsn`, is required. `config_dir` indicating location of `tnsnames.ora` and `sqlnet.ora` if not provided, is assumed from the environment variable `TNS_ADMIN`. Other connection parameters must be specified in `tnsnames.ora`. For example, the following is sufficient
+```
 data-validation connections add \
  --connection-name ora_secure Oracle \
- --url="oracle+cx_oracle://@dvt_prod_db"
+ --connect-args='{"dsn": TNS_ALIAS}'
 ```
 
 ## MSSQL Server
@@ -259,9 +269,9 @@ export PGSSLCERT="/path/to/certs/client-cert.pem" \
 export PGSSLKEY=/path/to/certs/client-key.pem \
 export PGSSLROOTCERT=/path/to/certs/server-ca.pem \
 export PGSSLMODE=verify-ca
-data-validation connections add --connection-name pg_tls_ca Postgres \
---host=10.1.0.2 --user=dvt_user --password=secret-password-123 \
---database=appdb
+data-validation connections add --connection-name CONN_NAME Postgres \
+--host=HOST_NAME --user=USER --password=PASSWORD \
+--database=DATABASE
 ```
 
 ## AlloyDB
@@ -412,10 +422,10 @@ data-validation connections add
     --password PASSWORD                                 Snowflake password
     --account ACCOUNT                                   Snowflake account
     --database DATABASE/SCHEMA                          Snowflake database and schema, separated by a `/`
-    [--connect-args CONNECT_ARGS]                       Additional connection args, default {}
+    [--connect-args CONNECT_ARGS]                       Additional connection args, JSON String dict, default {}
 ```
 
 To connect to Snowflake using key-pair authentication you will need to use the `--connect-args` options. Example content from a connection file is included below for reference:
 ```
-{"source_type": "Snowflake", "secret_manager_type": null, "secret_manager_project_id": null, "user": "dvtuserp8", "password": "", "account": "my-account", "database": "my_dvt_database", "connect_args": {"private_key_file": "/some/path/snowflake/rsa_key.p8", "private_key_file_pwd": "my-passphrase"}}
+{"source_type": "Snowflake", "user": USER_NAME, "password": "", "account": ACCOUNT, "database": DATABASE, "connect_args": '{"private_key_file": PATH_TO_RSA_KEY/RSA_KEY.p8, "private_key_file_pwd": PASSPHRASE}'}
 ```
diff --git a/third_party/ibis/ibis_oracle/__init__.py b/third_party/ibis/ibis_oracle/__init__.py
index c0f8590..fcce368 100644
--- a/third_party/ibis/ibis_oracle/__init__.py
+++ b/third_party/ibis/ibis_oracle/__init__.py
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from typing import Iterable, Literal, Tuple
+from typing import Iterable, Literal, Tuple, Dict, Any
 
 import sqlalchemy as sa
 from sqlalchemy.dialects.oracle.base import (
@@ -26,6 +26,7 @@ import ibis.expr.datatypes as dt
 from ibis.backends.base.sql.alchemy import BaseAlchemyBackend
 from third_party.ibis.ibis_oracle.compiler import OracleCompiler
 from third_party.ibis.ibis_oracle.datatypes import _get_type
+import oracledb
 
 
 EXTRA_RESERVED_WORDS = set(
@@ -99,29 +100,30 @@ class Backend(BaseAlchemyBackend):
         port: int = 1521,
         database: str = None,
         protocol: str = "TCP",
-        url: str = None,
+        thick_mode: bool = False,
         driver: Literal["oracledb"] = "oracledb",
+        connect_args: Dict[str, Any] = None,
     ) -> None:
-        if url is None:
-            if driver != "oracledb":
-                raise NotImplementedError(
-                    "oracledb is currently the only supported driver"
-                )
-            dsn = """(description=(address=(protocol={})(host={})(port={}))(connect_data=(service_name={})))""".format(
-                protocol, host, port, database
+        if driver != "oracledb":
+            raise NotImplementedError("oracledb is currently the only supported driver")
+        if thick_mode or not user:
+            # Configuration explicitly requests thick_mode or user not specified, credentials in wallet - requires thick_mode
+            oracledb.init_oracle_client(config_dir=connect_args.get("config_dir", None))
+        connect_args = {} if not connect_args else connect_args
+        if user:
+            connect_args.update(
+                {
+                    "host": host,
+                    "user": user,
+                    "password": password,
+                    "port": port,
+                    "service_name": database,
+                    "protocol": protocol,
+                }
             )
-            sa_url = sa.engine.url.URL.create(
-                "oracle+oracledb",
-                user,
-                password,
-                dsn,
-            )
-        else:
-            sa_url = sa.engine.url.make_url(url)
 
-        self.database_name = sa_url.database
         engine = sa.create_engine(
-            sa_url,
+            "oracle+oracledb://@",
             poolclass=sa.pool.StaticPool,
             arraysize=self.arraysize,
             # The hardcoding of 128 below is not great but is the simplest way of dealing with:
@@ -134,8 +136,10 @@ class Backend(BaseAlchemyBackend):
             # Therefore the ugly hardcoding of 128 kicks the can down the road and unblocks a customer
             # who is working with Oracle 11g and a max identifier length of 30.
             max_identifier_length=128,
-            # Pessimistic disconnect handling
+            # Pessimistic disconnect handling.
             pool_pre_ping=True,
+            # oracledb connection arguments.
+            connect_args=connect_args,
         )
         try:
             # Identify the session in Oracle as DVT, no-op if this fails.
@@ -151,6 +155,10 @@ class Backend(BaseAlchemyBackend):
                 cur.execute("ALTER SESSION SET NLS_NUMERIC_CHARACTERS='.,'")
 
         super().do_connect(engine)
+        # the database / service name is usually obtained from tnsnames.ora, we fetch it here
+        self.database_name = self.raw_sql(
+            "select sys_context('USERENV', 'SERVICE_NAME') from dual"
+        ).fetchall()[0][0]
 
     def _metadata(self, query) -> Iterable[Tuple[str, dt.DataType]]:
         if (
diff --git a/third_party/ibis/ibis_oracle/api.py b/third_party/ibis/ibis_oracle/api.py
index 5fc97ab..2f5230b 100644
--- a/third_party/ibis/ibis_oracle/api.py
+++ b/third_party/ibis/ibis_oracle/api.py
@@ -12,9 +12,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 from typing import Literal
-from third_party.ibis.ibis_oracle import Backend as OracleBackend
+
 import oracledb  # NOQA fail early if the driver is missing
 
+from data_validation.util import dvt_config_string_to_dict
+from third_party.ibis.ibis_oracle import Backend as OracleBackend
+
 
 def oracle_connect(
     host: str = "localhost",
@@ -23,9 +26,11 @@ def oracle_connect(
     port: int = 1521,
     database: str = None,
     protocol: str = "TCP",
-    url: str = None,
+    thick_mode: bool = False,
     driver: Literal["oracledb"] = "oracledb",
+    connect_args: str = None,
 ):
+    connect_args_dict = dvt_config_string_to_dict(connect_args) if connect_args else {}
     backend = OracleBackend()
     backend.do_connect(
         host=host,
@@ -34,7 +39,8 @@ def oracle_connect(
         password=password,
         database=database,
         protocol=protocol,
-        url=url,
+        thick_mode=thick_mode,
         driver=driver,
+        connect_args=connect_args_dict,
     )
     return backend
diff --git a/third_party/ibis/ibis_snowflake/api.py b/third_party/ibis/ibis_snowflake/api.py
index 1ddefec..5522808 100644
--- a/third_party/ibis/ibis_snowflake/api.py
+++ b/third_party/ibis/ibis_snowflake/api.py
@@ -12,13 +12,11 @@
 # License for the specific language governing permissions and limitations under
 # the License.
 import logging
-from typing import Any, Mapping
 
 import ibis
 
-# Fail fast with imports
-import snowflake.connector
-import snowflake.sqlalchemy
+import snowflake.connector  # NOQA fail early if the driver is missing
+import snowflake.sqlalchemy  # NOQA fail early if the driver is missing
 
 logging.getLogger("snowflake.connector").setLevel(logging.WARNING)
 


Test patch:
diff --git a/tests/unit/test_cli_tools.py b/tests/unit/test_cli_tools.py
index 6ef6c58..ea7072a 100644
--- a/tests/unit/test_cli_tools.py
+++ b/tests/unit/test_cli_tools.py
@@ -154,7 +154,8 @@ CLI_ADD_ORACLE_WALLET_CONNECTION_ARGS = [
     "--connection-name",
     "ora_wal_test",
     "Oracle",
-    "--url=oracle+cx_oracle://@dvt_user_db",
+    """--connect-args='{"dsn": "@dvt_user_db", "config_dir": "/opt/oracle/client_files", "disable_oob": true}'""",
+    "--thick-mode",
 ]
 
 TEST_VALIDATION_CONFIG = {
@@ -329,7 +330,9 @@ def test_create_connections_oracle(mock_write_file):
     parser = cli_tools.configure_arg_parser()
     args = parser.parse_args(CLI_ADD_ORACLE_WALLET_CONNECTION_ARGS)
     conn = cli_tools.get_connection_config_from_args(args)
-    assert "url" in conn
+    assert "connect_args" in conn
+    assert "config_dir" in conn["connect_args"]
+    assert "thick_mode" in conn
     cli_tools.store_connection(args.connection_name, conn)