Address Oracle connection for encrypted, ssl instance#27374
Address Oracle connection for encrypted, ssl instance#27374
Conversation
github-actions
bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the Oracle ingestion connector to better support/enforce python-oracledb 2.x usage and adds integration coverage around Oracle connection modes, including thick-mode initialization fallback behavior.
Changes:
- Add Oracle Testcontainers support and an Oracle container fixture for integration tests.
- Add new Oracle connection integration tests covering Service Name, TNS, URL building, and thick-mode fallback.
- Update Oracle connector dependency to
oracledb>=2.1,<3and improve thick-client initialization handling/logging.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ingestion/tests/integration/containers.py | Adds OracleContainerConfigs and get_oracle_container helper using OracleDbContainer. |
| ingestion/tests/integration/connections/conftest.py | Adds package-scoped oracle_container fixture and grants needed Oracle privileges for test connection steps. |
| ingestion/tests/integration/connections/test_oracle_connection.py | New integration test suite for Oracle connection types and thick/thin mode behavior expectations. |
| ingestion/src/metadata/ingestion/source/database/oracle/connection.py | Improves thick-client init error handling (ProgrammingError vs DatabaseError) and warning message for NNE scenarios. |
| ingestion/setup.py | Updates oracle extra dependency set to use python-oracledb 2.x and removes direct cx_Oracle pin from the extra. |
| port: int = 1521 | ||
| dbname: str = "test" | ||
| container_name: str = "test-oracle" | ||
| exposed_port: Optional[int] = None |
There was a problem hiding this comment.
OracleContainerConfigs is later used as a mutable carrier for extra attributes (e.g., docker_container in the connections conftest), but that attribute is not declared on the dataclass. Either add a typed field for it (e.g., docker_container: Optional[OracleDbContainer] = None) or avoid attaching dynamic attributes so editors/type-checkers and readers can understand what the object contains.
| exposed_port: Optional[int] = None | |
| exposed_port: Optional[int] = None | |
| docker_container: Optional[OracleDbContainer] = None |
| oracle_config = OracleContainerConfigs(container_name=str(uuid.uuid4())) | ||
| with get_oracle_container(oracle_config) as container: | ||
| oracle_config.with_exposed_port(container) | ||
| oracle_config.docker_container = container |
There was a problem hiding this comment.
The fixture sets oracle_config.docker_container = container, but docker_container is not part of OracleContainerConfigs and it is not used anywhere else in this module. Please either remove this assignment or add docker_container as an explicit, typed field on the dataclass to avoid undocumented dynamic attributes.
| oracle_config.docker_container = container |
| connection = oracledb.connect( | ||
| user="sys", | ||
| password=config.oracle_password, | ||
| dsn=dsn, | ||
| mode=oracledb.AUTH_MODE_SYSDBA, | ||
| ) | ||
| cursor = connection.cursor() | ||
| grants = [ | ||
| f"GRANT SELECT ANY DICTIONARY TO {config.username}", | ||
| f"GRANT SELECT ON gv_$sql TO {config.username}", | ||
| f"GRANT SELECT ON v_$sql TO {config.username}", | ||
| f"GRANT CREATE TABLE TO {config.username}", | ||
| ] | ||
| for grant in grants: | ||
| cursor.execute(grant) | ||
| connection.commit() | ||
| cursor.close() | ||
| connection.close() |
There was a problem hiding this comment.
_grant_oracle_privileges opens a DB connection and cursor but doesn’t ensure they’re closed if any cursor.execute(...) fails (which can happen if the container isn’t ready yet or grants change). Use context managers / try-finally (or with oracledb.connect(...) as conn and with conn.cursor() as cursor) so resources are always released and failures don’t leak connections in the test process.
| connection = oracledb.connect( | |
| user="sys", | |
| password=config.oracle_password, | |
| dsn=dsn, | |
| mode=oracledb.AUTH_MODE_SYSDBA, | |
| ) | |
| cursor = connection.cursor() | |
| grants = [ | |
| f"GRANT SELECT ANY DICTIONARY TO {config.username}", | |
| f"GRANT SELECT ON gv_$sql TO {config.username}", | |
| f"GRANT SELECT ON v_$sql TO {config.username}", | |
| f"GRANT CREATE TABLE TO {config.username}", | |
| ] | |
| for grant in grants: | |
| cursor.execute(grant) | |
| connection.commit() | |
| cursor.close() | |
| connection.close() | |
| grants = [ | |
| f"GRANT SELECT ANY DICTIONARY TO {config.username}", | |
| f"GRANT SELECT ON gv_$sql TO {config.username}", | |
| f"GRANT SELECT ON v_$sql TO {config.username}", | |
| f"GRANT CREATE TABLE TO {config.username}", | |
| ] | |
| with oracledb.connect( | |
| user="sys", | |
| password=config.oracle_password, | |
| dsn=dsn, | |
| mode=oracledb.AUTH_MODE_SYSDBA, | |
| ) as connection: | |
| with connection.cursor() as cursor: | |
| for grant in grants: | |
| cursor.execute(grant) | |
| connection.commit() |
| "nifi": {}, # uses requests | ||
| "openlineage": {*COMMONS["kafka"]}, | ||
| "oracle": {"cx_Oracle>=8.3.0,<9", "oracledb~=1.2", DATA_DIFF["oracle"]}, | ||
| "oracle": {"oracledb>=2.1,<3", DATA_DIFF["oracle"]}, |
There was a problem hiding this comment.
The PR description still contains placeholders (e.g. “Fixes ”, “I worked on ... because ...”) and does not describe what was changed, why, or how it was tested. Please update the PR description to include the linked issue (if any), a brief rationale, and the validation performed—this is important given the dependency bump and new integration coverage.
🔴 Playwright Results — 1 failure(s), 18 flaky✅ 2984 passed · ❌ 1 failed · 🟡 18 flaky · ⏭️ 83 skipped
Genuine Failures (failed on all attempts)❌
|
| logger.info( | ||
| "Thin mode connection failed. Attempting thick mode with" | ||
| " Oracle Instant Client for NNE support." | ||
| ) | ||
| if self._init_thick_mode(): | ||
| engine = self._create_engine() |
There was a problem hiding this comment.
In _get_client_with_fallback, the log says it's attempting thick mode for NNE, but _init_thick_mode() immediately returns False when instantClientDirectory is empty. This produces misleading logs and means the advertised “thin-first, thick-fallback” path can never actually try thick mode unless a directory is configured. Consider either (a) only logging/attempting thick mode when a lib dir is configured, or (b) attempting init_oracle_client() without lib_dir when no directory is provided (so system-installed Instant Client can be used).
| @pytest.mark.order(4) | ||
| def test_nne_requires_thick_mode(): | ||
| """Verify that NNE (Native Network Encryption) requires thick mode. | ||
|
|
||
| Oracle NNE is only available in thick mode (with Oracle Instant Client). | ||
| Thin mode cannot negotiate NNE — when the server has | ||
| SQLNET.ENCRYPTION_SERVER=required, thin mode gets DPY-4011. | ||
|
|
||
| This was verified against Oracle Enterprise 19c: | ||
| - Thick mode (Instant Client 19c): AES256 encryption negotiated successfully | ||
| - Thin mode (any oracledb version): connection rejected with DPY-4011 | ||
|
|
||
| The gvenzl/oracle-free container used in CI does not include NNE (it's | ||
| Enterprise-only), so we validate that oracledb is on 2.x (for other | ||
| improvements) and that thick mode is the documented path for NNE. | ||
| """ | ||
| assert oracledb.is_thin_mode is not None # module loaded | ||
| major_version = int(oracledb.__version__.split(".")[0]) | ||
| assert major_version >= 2, f"oracledb {oracledb.__version__} should be >= 2.0" | ||
|
|
There was a problem hiding this comment.
test_nne_requires_thick_mode name/docstring claims to verify that NNE requires thick mode, but the assertions only check that the installed oracledb major version is >= 2. This makes the test misleading and could pass even if thick-mode/NNE handling regresses. Either rename the test to reflect what it actually validates, or add an assertion that exercises/validates the connector’s thick-mode path (e.g., via mocking or verifying the fallback behavior triggered by a thin-mode failure).
Code Review ✅ Approved 3 resolved / 3 findingsOracle connection logic updated for encrypted SSL instances while resolving resource cleanup gaps, redundant probe connections, and incorrect engine instantiation. No issues remain. ✅ 3 resolved✅ Quality: _grant_oracle_privileges lacks safe resource cleanup
✅ Performance: Unnecessary probe connection when instantClientDirectory is set
✅ Bug: _get_client returns known-broken engine when both modes fail
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| else: | ||
| kwargs["instantClientDirectory"] = "" |
There was a problem hiding this comment.
These integration tests force instantClientDirectory to "", which bypasses the schema default of /instantclient. Given the connection logic now branches heavily on this field, consider adding coverage for the default/omitted case (e.g., ensure a default /instantclient that isn’t present does not break thin-mode connectivity, or validate the intended fallback behavior).
| else: | |
| kwargs["instantClientDirectory"] = "" |
|
|
||
| def _grant_oracle_privileges(config: OracleContainerConfigs) -> None: | ||
| """Grant DBA-level privileges needed by test_connection steps.""" | ||
| dsn = oracledb.makedsn("localhost", config.exposed_port, service_name=config.dbname) |
There was a problem hiding this comment.
config.exposed_port is typed as Optional[int] and is used directly in makedsn(...). Since a missing/failed port mapping would lead to a confusing runtime error here, consider asserting it is set (e.g., after with_exposed_port) before building the DSN to fail with a clearer message.
| dsn = oracledb.makedsn("localhost", config.exposed_port, service_name=config.dbname) | |
| if config.exposed_port is None: | |
| raise ValueError( | |
| "Oracle container exposed port is not set. " | |
| "Ensure with_exposed_port(container) succeeds before granting privileges." | |
| ) | |
| dsn = oracledb.makedsn( | |
| "localhost", config.exposed_port, service_name=config.dbname | |
| ) |
| if self.service_connection.instantClientDirectory: | ||
| return self._get_client_thick_mode() | ||
| return self._get_client_thin_mode() | ||
|
|
There was a problem hiding this comment.
instantClientDirectory has a schema default of /instantclient (see openmetadata-spec/.../oracleConnection.json), so this check will route most configs into thick mode by default. With the new fail-fast behavior in _get_client_thick_mode, environments that don’t have Instant Client mounted at /instantclient will now error instead of falling back to thin mode (previous behavior). Consider implementing the advertised thin-first + thick-fallback strategy (try thin, then init thick/retry on failure) or only enforcing fail-fast when the directory was explicitly set by the user (non-default), to avoid a breaking change for existing Oracle connections.
| oracledb.init_oracle_client(lib_dir=lib_dir) | ||
| os.environ[LD_LIB_ENV] = lib_dir |
There was a problem hiding this comment.
LD_LIBRARY_PATH is being set after calling oracledb.init_oracle_client(). Since the schema/doc for instantClientDirectory says it is used to set LD_LIBRARY_PATH, and env vars typically need to be set before native library loading, it’d be safer to set/prepend the env var before initialization (or remove the env mutation if lib_dir is sufficient) to avoid cases where dependent libraries can’t be resolved at init time.
| oracledb.init_oracle_client(lib_dir=lib_dir) | |
| os.environ[LD_LIB_ENV] = lib_dir | |
| existing_ld_library_path = os.environ.get(LD_LIB_ENV) | |
| if existing_ld_library_path: | |
| ld_library_path_entries = existing_ld_library_path.split(os.pathsep) | |
| if lib_dir not in ld_library_path_entries: | |
| os.environ[LD_LIB_ENV] = ( | |
| f"{lib_dir}{os.pathsep}{existing_ld_library_path}" | |
| ) | |
| else: | |
| os.environ[LD_LIB_ENV] = lib_dir | |
| oracledb.init_oracle_client(lib_dir=lib_dir) |
|
Summary
oracledbfrom 1.x to 2.x (oracledb>=2.1,<3) — removes deadcx_OracledependencySQLNET.ENCRYPTION_SERVER=requiredContext
A customer on Oracle 19c Enterprise with
SQLNET.ENCRYPTION_SERVER=requiredcould not connect. Investigation revealed:DPY-4011errororacledbwas pinned to 1.x which lacks improvements in 2.xVerified end-to-end against Oracle Enterprise 19c with encryption required: thick mode successfully negotiates AES256 encryption.
Test plan
SQLNET.ENCRYPTION_SERVER=required🤖 Generated with Claude Code