SameerMesiah97 commented on code in PR #70160: URL: https://github.com/apache/airflow/pull/70160#discussion_r3647672235
########## providers/oracle/docs/connections/oci.rst: ########## @@ -0,0 +1,125 @@ + .. Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + .. http://www.apache.org/licenses/LICENSE-2.0 + + .. Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +.. _howto/connection:oci: + +Oracle Cloud Infrastructure Connection +====================================== + +The Oracle Cloud Infrastructure connection configures authentication for OCI SDK clients. +It is separate from the :ref:`Oracle Database connection <howto/connection:oracle>`, which +uses the ``oracle`` connection type and the ``oracledb`` driver. + +OCI support is optional because the OCI Python SDK has transitive dependencies that Apache Airflow +cannot distribute as required dependencies. Install the provider with the ``oci`` extra before using +this connection or its service hooks: + +.. code-block:: bash + + pip install 'apache-airflow-providers-oracle[oci]' + +The default connection ID is ``oci_default``. + +Service hooks reuse this connection for credentials and region while selecting their own OCI SDK +client class. Each SDK client derives its endpoint from the configured region unless the Dag author +passes a ``service_endpoint`` argument to the hook. Review Comment: I would change the first sentence to this as it is a bit more clear: `Service hooks reuse the connection's credentials and region while instantiating the appropriate OCI SDK client.` ########## providers/oracle/docs/generative_ai.rst: ########## @@ -0,0 +1,143 @@ + .. Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + .. http://www.apache.org/licenses/LICENSE-2.0 + + .. Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +OCI Generative AI Hosted Applications +===================================== + +:class:`~airflow.providers.oracle.hooks.generative_ai.OciGenerativeAIHook` uses the official +`OCI Python SDK <https://docs.oracle.com/en-us/iaas/tools/python/latest/>`__ to manage +`Hosted Applications and deployments +<https://docs.oracle.com/en-us/iaas/Content/generative-ai/agents.htm#deployments>`__. +Install ``apache-airflow-providers-oracle[oci]`` and configure an +:ref:`OCI connection <howto/connection:oci>` before using the hook. +The hook exposes the native :class:`oci.generative_ai.GenerativeAiClient` through ``conn`` and +``get_conn()``. Operators can therefore call OCI SDK methods directly without an Airflow wrapper +for every API operation. + +Oracle exposes separate application APIs for `two inbound authentication variants +<https://docs.oracle.com/en-us/iaas/Content/generative-ai/create-application.htm#authentication>`__: + +* Identity domain bearer tokens use the OCI SDK ``HostedApplication`` resource and client methods + ending in ``hosted_application`` or ``hosted_applications``. +* OCI IAM request signing uses ``HostedApplicationIam`` and client methods ending in + ``hosted_application_iam`` or ``hosted_applications_iam``. + +This distinction configures how clients invoke the deployed application. It does not change how +the Airflow hook authenticates to the OCI management API; both variants use the configured +:ref:`OCI connection <howto/connection:oci>`. + +Management endpoints +-------------------- + +The OCI SDK derives the management endpoint as +``https://generativeai.<region>.oci.oraclecloud.com`` and adds the ``20231130`` API base path. +The hook exposes these operations without changing OCI retry, pagination, or concurrency-control +arguments. + +========================================================== =================================================== +OCI SDK client method REST operation +========================================================== =================================================== +``create_hosted_application`` ``POST /20231130/hostedApplications`` +``get_hosted_application`` ``GET /20231130/hostedApplications/{id}`` +``list_hosted_applications`` ``GET /20231130/hostedApplications`` +``update_hosted_application`` ``PUT /20231130/hostedApplications/{id}`` +``delete_hosted_application`` ``DELETE /20231130/hostedApplications/{id}`` +``create_hosted_application_iam`` ``POST /20231130/hostedApplicationsIam`` +``get_hosted_application_iam`` ``GET /20231130/hostedApplicationsIam/{id}`` +``list_hosted_applications_iam`` ``GET /20231130/hostedApplicationsIam`` +``update_hosted_application_iam`` ``PUT /20231130/hostedApplicationsIam/{id}`` +``delete_hosted_application_iam`` ``DELETE /20231130/hostedApplicationsIam/{id}`` +``create_hosted_deployment`` ``POST /20231130/hostedDeployments`` +``get_hosted_deployment`` ``GET /20231130/hostedDeployments/{id}`` +``list_hosted_deployments`` ``GET /20231130/hostedDeployments`` +``update_hosted_deployment`` ``PUT /20231130/hostedDeployments/{id}`` +``delete_hosted_deployment`` ``DELETE /20231130/hostedDeployments/{id}`` +``get_work_request`` ``GET /20231130/workRequests/{id}`` +``list_work_request_errors`` ``GET /20231130/workRequests/{id}/errors`` +``list_work_request_logs`` ``GET /20231130/workRequests/{id}/logs`` +``list_work_requests`` ``GET /20231130/workRequests`` +========================================================== =================================================== + +All client methods return the native :class:`oci.response.Response`. This preserves response data and +headers such as ``etag``, ``opc-request-id``, and ``opc-work-request-id``. Create, update, and +delete operations can be asynchronous; use ``opc-work-request-id`` with +``hook.conn.get_work_request`` to observe their status. + +Use ``hook.get_compartment_id()`` to resolve an explicit compartment or the connection default +before calling list methods. To filter deployments for a Hosted Application, pass its OCID as the +OCI SDK ``application_id`` keyword argument. + +Creating an identity domain Hosted Application +----------------------------------------------- + +Identity domain applications require an ``InboundAuthConfig`` containing the identity domain URL +and OAuth settings: + +.. code-block:: python + + from oci.generative_ai.models import ( + CreateHostedApplicationDetails, + IdcsAuthConfig, + InboundAuthConfig, + ) + + from airflow.providers.oracle.hooks.generative_ai import OciGenerativeAIHook + + hook = OciGenerativeAIHook(oci_conn_id="oci_default") + response = hook.conn.create_hosted_application( + CreateHostedApplicationDetails( + display_name="airflow-agent-oauth", + compartment_id="ocid1.compartment.oc1..example", + inbound_auth_config=InboundAuthConfig( + inbound_auth_config_type="IDCS_AUTH_CONFIG", + idcs_config=IdcsAuthConfig( + domain_url="https://idcs-example.identity.oraclecloud.com";, + scope="agent.invoke", + audience="https://agent.example.com";, + ), + ), + ) + ) + work_request_id = response.headers.get("opc-work-request-id") Review Comment: I wonder if these examples could be simplified. Since the purpose of the hook is simply to expose the native OCI SDK client, I think the docs only need to communicate the intended usage rather than provide a fully runnable OCI SDK example. I would suggest a pattern like the below for these code blocks: ``` hook = OciGenerativeAIHook(oci_conn_id="oci_default") response = hook.conn.create_hosted_application(...) work_request_id = response.headers["opc-work-request-id"] ``` ########## providers/oracle/docs/connections/oci.rst: ########## @@ -0,0 +1,125 @@ + .. Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + .. http://www.apache.org/licenses/LICENSE-2.0 + + .. Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +.. _howto/connection:oci: + +Oracle Cloud Infrastructure Connection +====================================== + +The Oracle Cloud Infrastructure connection configures authentication for OCI SDK clients. +It is separate from the :ref:`Oracle Database connection <howto/connection:oracle>`, which +uses the ``oracle`` connection type and the ``oracledb`` driver. + +OCI support is optional because the OCI Python SDK has transitive dependencies that Apache Airflow +cannot distribute as required dependencies. Install the provider with the ``oci`` extra before using +this connection or its service hooks: + +.. code-block:: bash + + pip install 'apache-airflow-providers-oracle[oci]' + +The default connection ID is ``oci_default``. + +Service hooks reuse this connection for credentials and region while selecting their own OCI SDK +client class. Each SDK client derives its endpoint from the configured region unless the Dag author +passes a ``service_endpoint`` argument to the hook. + +Authentication types +-------------------- + +API key + This is the default. Configure ``User OCID`` in Login, the optional private key passphrase in + Password, and ``tenancy``, ``fingerprint``, ``region``, and ``key_content`` in Extra. A Dag + author may pass ``key_file`` to the hook instead of storing ``key_content`` in the connection. Review Comment: I think we could specify what fields are required and optional here. The phrasing could be better as well. ########## providers/oracle/tests/unit/oracle/hooks/test_base_oci.py: ########## @@ -0,0 +1,403 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import subprocess +import sys +from unittest import mock + +import pytest + +from airflow.models import Connection +from airflow.providers.common.compat.sdk import AirflowOptionalProviderFeatureException +from airflow.providers.oracle.get_provider_info import get_provider_info +from airflow.providers.oracle.hooks.base_oci import ( + OCI_AUTH_TYPE_CONFIG_FILE, + OCI_AUTH_TYPE_INSTANCE_PRINCIPAL, + OCI_AUTH_TYPE_RESOURCE_PRINCIPAL, + OciBaseHook, + _get_oci_sdk, +) + +GenerativeAiClient = pytest.importorskip("oci.generative_ai").GenerativeAiClient + + +class TestOciBaseHook: + def setup_method(self): + self.hook = OciBaseHook() + + def set_connection(self, connection: Connection) -> None: + self.hook.get_connection = mock.create_autospec(self.hook.get_connection, return_value=connection) + + @pytest.mark.parametrize( + ("hook_kwargs", "connection_extra", "key_field", "key_value"), + [ + ({"key_file": "/keys/oci.pem"}, {}, "key_file", "/keys/oci.pem"), + ({}, {"key_content": "private-key-content"}, "key_content", "private-key-content"), + ], + ) + def test_get_oci_config_with_api_key(self, hook_kwargs, connection_extra, key_field, key_value): + self.hook = OciBaseHook(**hook_kwargs) + self.set_connection( + Connection( + login="ocid1.user.test", + password="passphrase", + extra={ + "tenancy": "ocid1.tenancy.test", + "fingerprint": "fingerprint", + "region": "us-chicago-1", + **connection_extra, + }, + ) + ) + + config, signer = self.hook.get_oci_config() + + assert config == { + "tenancy": "ocid1.tenancy.test", + "user": "ocid1.user.test", + "fingerprint": "fingerprint", + "region": "us-chicago-1", + "pass_phrase": "passphrase", + key_field: key_value, + } + assert signer is None + + def test_connection_extra_cannot_control_hook_configuration(self): + self.set_connection( + Connection( + login="ocid1.user.test", + extra={ + "auth_type": OCI_AUTH_TYPE_INSTANCE_PRINCIPAL, + "key_file": "/etc/hosts", + "config_file": "/etc/hosts", + "profile": "UNTRUSTED", + "service_endpoint": "https://untrusted.example.test";, + "tenancy": "ocid1.tenancy.test", + "fingerprint": "fingerprint", + "region": "us-chicago-1", + "key_content": "private-key-content", + }, + ) + ) + + config, signer = self.hook.get_oci_config() + + assert config == { + "tenancy": "ocid1.tenancy.test", + "user": "ocid1.user.test", + "fingerprint": "fingerprint", + "region": "us-chicago-1", + "pass_phrase": None, + "key_content": "private-key-content", + } + assert signer is None + + @pytest.mark.parametrize( + ("hook_kwargs", "extra", "error_message"), + [ + ( + {}, + {}, + "OCI API key authentication requires either 'key_file' or 'key_content'", + ), + ( + {"key_file": "/keys/oci.pem"}, + {"key_content": "private-key-content"}, + "OCI API key authentication cannot use both 'key_file' and 'key_content'", + ), + ], + ids=["missing-key", "conflicting-keys"], + ) + def test_get_oci_config_rejects_invalid_api_key_material(self, hook_kwargs, extra, error_message): + self.hook = OciBaseHook(**hook_kwargs) + self.set_connection(Connection(extra=extra)) + + with pytest.raises(ValueError, match=error_message): + self.hook.get_oci_config() + + @mock.patch("oci.config.from_file", autospec=True) + def test_get_oci_config_from_file_with_region_override(self, mock_from_file): + self.hook = OciBaseHook( + auth_type=OCI_AUTH_TYPE_CONFIG_FILE, + config_file="/config/oci", + profile="AIRFLOW", + ) + mock_from_file.return_value = {"region": "us-ashburn-1"} + self.set_connection(Connection(extra={"region": "eu-frankfurt-1"})) + + config, signer = self.hook.get_oci_config() + + mock_from_file.assert_called_once_with( + file_location="/config/oci", + profile_name="AIRFLOW", + ) + assert config == {"region": "eu-frankfurt-1"} + assert signer is None + + @pytest.mark.parametrize( + ("config_file", "profile"), + [ + (None, None), + ("", ""), + ], + ) + @mock.patch("oci.config.from_file", autospec=True) + def test_get_oci_config_from_default_file(self, mock_from_file, config_file, profile): + self.hook = OciBaseHook( + auth_type=OCI_AUTH_TYPE_CONFIG_FILE, + config_file=config_file, + profile=profile, + ) + mock_from_file.return_value = {"region": "us-ashburn-1"} + self.set_connection(Connection()) + + config, signer = self.hook.get_oci_config() + + mock_from_file.assert_called_once_with( + file_location="~/.oci/config", + profile_name="DEFAULT", + ) + assert config == {"region": "us-ashburn-1"} + assert signer is None + + @mock.patch( + "oci.auth.signers.InstancePrincipalsSecurityTokenSigner", + autospec=True, + ) + def test_get_oci_config_with_instance_principal_and_connection_region(self, mock_signer_class): + self.hook = OciBaseHook(auth_type=OCI_AUTH_TYPE_INSTANCE_PRINCIPAL) + signer = mock_signer_class.return_value + signer.region = "us-ashburn-1" + self.set_connection(Connection(extra={"region": "eu-frankfurt-1"})) + + config, actual_signer = self.hook.get_oci_config() + + assert config == {"region": "eu-frankfurt-1"} + assert actual_signer is signer + + @mock.patch( + "oci.auth.signers.get_resource_principals_signer", + autospec=True, + ) + def test_get_oci_config_with_resource_principal_region(self, mock_get_signer): + self.hook = OciBaseHook(auth_type=OCI_AUTH_TYPE_RESOURCE_PRINCIPAL) + signer = mock_get_signer.return_value + signer.region = "us-phoenix-1" + self.set_connection(Connection()) + + config, actual_signer = self.hook.get_oci_config() + + assert config == {"region": "us-phoenix-1"} + assert actual_signer is signer + + @mock.patch( + "oci.auth.signers.get_resource_principals_signer", + autospec=True, + ) + def test_get_oci_config_with_resource_principal_without_region(self, mock_get_signer): + self.hook = OciBaseHook(auth_type=OCI_AUTH_TYPE_RESOURCE_PRINCIPAL) + signer = mock_get_signer.return_value + del signer.region + self.set_connection(Connection()) + + config, actual_signer = self.hook.get_oci_config() + + assert config == {} + assert actual_signer is signer + + def test_get_oci_config_rejects_unknown_auth_type(self): + self.hook = OciBaseHook(auth_type="unknown") + self.set_connection(Connection()) + + with pytest.raises(ValueError, match="Unsupported OCI authentication type: 'unknown'"): + self.hook.get_oci_config() + + def test_get_client_with_signer_and_explicit_endpoint(self): + signer = mock.sentinel.signer + client = mock.sentinel.client + client_class = mock.create_autospec(GenerativeAiClient, return_value=client) + self.hook.get_oci_config = mock.create_autospec( + self.hook.get_oci_config, return_value=({"region": "us-chicago-1"}, signer) + ) + self.hook.service_endpoint = "https://generativeai.example.test/"; + + result = self.hook.get_client(client_class, timeout=30) + + assert result is client + client_class.assert_called_once_with( + config={"region": "us-chicago-1"}, + signer=signer, + service_endpoint="https://generativeai.example.test";, + timeout=30, + ) + + def test_get_client_without_signer_or_endpoint(self): + client = mock.sentinel.client + client_class = mock.create_autospec(GenerativeAiClient, return_value=client) + self.hook.get_oci_config = mock.create_autospec( + self.hook.get_oci_config, return_value=({"region": "us-chicago-1"}, None) + ) + self.set_connection(Connection()) + + result = self.hook.get_client(client_class) + + assert result is client + client_class.assert_called_once_with(config={"region": "us-chicago-1"}) + + def test_get_conn_requires_service_client_class(self): + with pytest.raises(ValueError, match="client_class must be specified by an OCI service hook"): + self.hook.get_conn() + + def test_get_client_class_returns_configured_class(self): + self.hook.client_class = GenerativeAiClient + + assert self.hook._get_client_class() is GenerativeAiClient + + @pytest.mark.parametrize("signer", [None, mock.sentinel.signer]) + @mock.patch("oci.identity.IdentityClient", autospec=True) + def test_connection_success(self, mock_identity_client, signer): + config = {"region": "us-chicago-1"} + self.hook.get_oci_config = mock.create_autospec( + self.hook.get_oci_config, return_value=(config, signer) + ) + + result = self.hook.test_connection() + + assert result == (True, "Connection successfully tested") + expected_kwargs = {"config": config} + if signer is not None: + expected_kwargs["signer"] = signer + mock_identity_client.assert_called_once_with(**expected_kwargs) + mock_identity_client.return_value.list_regions.assert_called_once_with() + + @mock.patch("oci.identity.IdentityClient", autospec=True) + def test_connection_failure(self, mock_identity_client): + self.hook.get_oci_config = mock.create_autospec( + self.hook.get_oci_config, return_value=({"region": "us-chicago-1"}, None) + ) + mock_identity_client.return_value.list_regions.side_effect = ValueError("invalid credentials") + + result = self.hook.test_connection() + + assert result == (False, "ValueError error occurred while testing connection: invalid credentials") + + @pytest.mark.parametrize( + ("hook_endpoint", "expected"), + [ + ("https://hook.test/";, "https://hook.test";), + (None, None), + ], + ) + def test_service_endpoint_is_controlled_by_hook_argument(self, hook_endpoint, expected): + self.hook = OciBaseHook(service_endpoint=hook_endpoint) + self.set_connection( + Connection( + host="https://connection-host.test";, + extra={"service_endpoint": "https://connection-extra.test"}, + ) + ) + + assert self.hook._get_service_endpoint() == expected + + def test_get_compartment_id_prefers_explicit_value(self): + self.set_connection(Connection(extra={"compartment_id": "connection-compartment"})) + + assert self.hook.get_compartment_id("explicit-compartment") == "explicit-compartment" + + def test_get_compartment_id_from_connection(self): + self.set_connection(Connection(extra={"compartment_id": "connection-compartment"})) + + assert self.hook.get_compartment_id() == "connection-compartment" + + def test_get_compartment_id_requires_value(self): + self.set_connection(Connection()) + + with pytest.raises(ValueError, match="An OCI compartment OCID must be provided"): + self.hook.get_compartment_id() + Review Comment: Minor nit: I think a few of these tests (for example `get_compartment_id` and possibly the `get_client `cases) could be parameterized to reduce a bit of duplication. Not a blocker though. ########## providers/oracle/tests/unit/oracle/hooks/test_base_oci.py: ########## @@ -0,0 +1,403 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import subprocess +import sys +from unittest import mock + +import pytest + +from airflow.models import Connection +from airflow.providers.common.compat.sdk import AirflowOptionalProviderFeatureException +from airflow.providers.oracle.get_provider_info import get_provider_info +from airflow.providers.oracle.hooks.base_oci import ( + OCI_AUTH_TYPE_CONFIG_FILE, + OCI_AUTH_TYPE_INSTANCE_PRINCIPAL, + OCI_AUTH_TYPE_RESOURCE_PRINCIPAL, + OciBaseHook, + _get_oci_sdk, +) + +GenerativeAiClient = pytest.importorskip("oci.generative_ai").GenerativeAiClient + + +class TestOciBaseHook: + def setup_method(self): + self.hook = OciBaseHook() + + def set_connection(self, connection: Connection) -> None: + self.hook.get_connection = mock.create_autospec(self.hook.get_connection, return_value=connection) + + @pytest.mark.parametrize( + ("hook_kwargs", "connection_extra", "key_field", "key_value"), + [ + ({"key_file": "/keys/oci.pem"}, {}, "key_file", "/keys/oci.pem"), + ({}, {"key_content": "private-key-content"}, "key_content", "private-key-content"), + ], + ) + def test_get_oci_config_with_api_key(self, hook_kwargs, connection_extra, key_field, key_value): + self.hook = OciBaseHook(**hook_kwargs) + self.set_connection( + Connection( + login="ocid1.user.test", + password="passphrase", + extra={ + "tenancy": "ocid1.tenancy.test", + "fingerprint": "fingerprint", + "region": "us-chicago-1", + **connection_extra, + }, + ) + ) + + config, signer = self.hook.get_oci_config() + + assert config == { + "tenancy": "ocid1.tenancy.test", + "user": "ocid1.user.test", + "fingerprint": "fingerprint", + "region": "us-chicago-1", + "pass_phrase": "passphrase", + key_field: key_value, + } + assert signer is None + + def test_connection_extra_cannot_control_hook_configuration(self): + self.set_connection( + Connection( + login="ocid1.user.test", + extra={ + "auth_type": OCI_AUTH_TYPE_INSTANCE_PRINCIPAL, + "key_file": "/etc/hosts", + "config_file": "/etc/hosts", + "profile": "UNTRUSTED", + "service_endpoint": "https://untrusted.example.test";, + "tenancy": "ocid1.tenancy.test", + "fingerprint": "fingerprint", + "region": "us-chicago-1", + "key_content": "private-key-content", + }, + ) + ) + + config, signer = self.hook.get_oci_config() + + assert config == { + "tenancy": "ocid1.tenancy.test", + "user": "ocid1.user.test", + "fingerprint": "fingerprint", + "region": "us-chicago-1", + "pass_phrase": None, + "key_content": "private-key-content", + } + assert signer is None + + @pytest.mark.parametrize( + ("hook_kwargs", "extra", "error_message"), + [ + ( + {}, + {}, + "OCI API key authentication requires either 'key_file' or 'key_content'", + ), + ( + {"key_file": "/keys/oci.pem"}, + {"key_content": "private-key-content"}, + "OCI API key authentication cannot use both 'key_file' and 'key_content'", + ), + ], + ids=["missing-key", "conflicting-keys"], + ) + def test_get_oci_config_rejects_invalid_api_key_material(self, hook_kwargs, extra, error_message): + self.hook = OciBaseHook(**hook_kwargs) + self.set_connection(Connection(extra=extra)) + + with pytest.raises(ValueError, match=error_message): + self.hook.get_oci_config() + + @mock.patch("oci.config.from_file", autospec=True) + def test_get_oci_config_from_file_with_region_override(self, mock_from_file): + self.hook = OciBaseHook( + auth_type=OCI_AUTH_TYPE_CONFIG_FILE, + config_file="/config/oci", + profile="AIRFLOW", + ) + mock_from_file.return_value = {"region": "us-ashburn-1"} + self.set_connection(Connection(extra={"region": "eu-frankfurt-1"})) + + config, signer = self.hook.get_oci_config() + + mock_from_file.assert_called_once_with( + file_location="/config/oci", + profile_name="AIRFLOW", + ) + assert config == {"region": "eu-frankfurt-1"} + assert signer is None + + @pytest.mark.parametrize( + ("config_file", "profile"), + [ + (None, None), + ("", ""), + ], + ) + @mock.patch("oci.config.from_file", autospec=True) + def test_get_oci_config_from_default_file(self, mock_from_file, config_file, profile): + self.hook = OciBaseHook( + auth_type=OCI_AUTH_TYPE_CONFIG_FILE, + config_file=config_file, + profile=profile, + ) + mock_from_file.return_value = {"region": "us-ashburn-1"} + self.set_connection(Connection()) + + config, signer = self.hook.get_oci_config() + + mock_from_file.assert_called_once_with( + file_location="~/.oci/config", + profile_name="DEFAULT", + ) + assert config == {"region": "us-ashburn-1"} + assert signer is None + + @mock.patch( + "oci.auth.signers.InstancePrincipalsSecurityTokenSigner", + autospec=True, + ) + def test_get_oci_config_with_instance_principal_and_connection_region(self, mock_signer_class): + self.hook = OciBaseHook(auth_type=OCI_AUTH_TYPE_INSTANCE_PRINCIPAL) + signer = mock_signer_class.return_value + signer.region = "us-ashburn-1" + self.set_connection(Connection(extra={"region": "eu-frankfurt-1"})) + + config, actual_signer = self.hook.get_oci_config() + + assert config == {"region": "eu-frankfurt-1"} + assert actual_signer is signer + + @mock.patch( + "oci.auth.signers.get_resource_principals_signer", + autospec=True, + ) + def test_get_oci_config_with_resource_principal_region(self, mock_get_signer): + self.hook = OciBaseHook(auth_type=OCI_AUTH_TYPE_RESOURCE_PRINCIPAL) + signer = mock_get_signer.return_value + signer.region = "us-phoenix-1" + self.set_connection(Connection()) + + config, actual_signer = self.hook.get_oci_config() + + assert config == {"region": "us-phoenix-1"} + assert actual_signer is signer + + @mock.patch( + "oci.auth.signers.get_resource_principals_signer", + autospec=True, + ) + def test_get_oci_config_with_resource_principal_without_region(self, mock_get_signer): + self.hook = OciBaseHook(auth_type=OCI_AUTH_TYPE_RESOURCE_PRINCIPAL) + signer = mock_get_signer.return_value + del signer.region + self.set_connection(Connection()) + + config, actual_signer = self.hook.get_oci_config() + + assert config == {} + assert actual_signer is signer + + def test_get_oci_config_rejects_unknown_auth_type(self): + self.hook = OciBaseHook(auth_type="unknown") + self.set_connection(Connection()) + + with pytest.raises(ValueError, match="Unsupported OCI authentication type: 'unknown'"): + self.hook.get_oci_config() + + def test_get_client_with_signer_and_explicit_endpoint(self): + signer = mock.sentinel.signer + client = mock.sentinel.client + client_class = mock.create_autospec(GenerativeAiClient, return_value=client) + self.hook.get_oci_config = mock.create_autospec( + self.hook.get_oci_config, return_value=({"region": "us-chicago-1"}, signer) + ) + self.hook.service_endpoint = "https://generativeai.example.test/"; + + result = self.hook.get_client(client_class, timeout=30) + + assert result is client + client_class.assert_called_once_with( + config={"region": "us-chicago-1"}, + signer=signer, + service_endpoint="https://generativeai.example.test";, + timeout=30, + ) + + def test_get_client_without_signer_or_endpoint(self): + client = mock.sentinel.client + client_class = mock.create_autospec(GenerativeAiClient, return_value=client) + self.hook.get_oci_config = mock.create_autospec( + self.hook.get_oci_config, return_value=({"region": "us-chicago-1"}, None) + ) + self.set_connection(Connection()) + + result = self.hook.get_client(client_class) + + assert result is client + client_class.assert_called_once_with(config={"region": "us-chicago-1"}) + + def test_get_conn_requires_service_client_class(self): + with pytest.raises(ValueError, match="client_class must be specified by an OCI service hook"): + self.hook.get_conn() + + def test_get_client_class_returns_configured_class(self): + self.hook.client_class = GenerativeAiClient + + assert self.hook._get_client_class() is GenerativeAiClient + + @pytest.mark.parametrize("signer", [None, mock.sentinel.signer]) + @mock.patch("oci.identity.IdentityClient", autospec=True) + def test_connection_success(self, mock_identity_client, signer): + config = {"region": "us-chicago-1"} + self.hook.get_oci_config = mock.create_autospec( + self.hook.get_oci_config, return_value=(config, signer) + ) + + result = self.hook.test_connection() + + assert result == (True, "Connection successfully tested") + expected_kwargs = {"config": config} + if signer is not None: + expected_kwargs["signer"] = signer + mock_identity_client.assert_called_once_with(**expected_kwargs) + mock_identity_client.return_value.list_regions.assert_called_once_with() + + @mock.patch("oci.identity.IdentityClient", autospec=True) + def test_connection_failure(self, mock_identity_client): + self.hook.get_oci_config = mock.create_autospec( + self.hook.get_oci_config, return_value=({"region": "us-chicago-1"}, None) + ) + mock_identity_client.return_value.list_regions.side_effect = ValueError("invalid credentials") + + result = self.hook.test_connection() + + assert result == (False, "ValueError error occurred while testing connection: invalid credentials") + + @pytest.mark.parametrize( + ("hook_endpoint", "expected"), + [ + ("https://hook.test/";, "https://hook.test";), + (None, None), + ], + ) + def test_service_endpoint_is_controlled_by_hook_argument(self, hook_endpoint, expected): + self.hook = OciBaseHook(service_endpoint=hook_endpoint) + self.set_connection( + Connection( + host="https://connection-host.test";, + extra={"service_endpoint": "https://connection-extra.test"}, + ) + ) + + assert self.hook._get_service_endpoint() == expected + + def test_get_compartment_id_prefers_explicit_value(self): + self.set_connection(Connection(extra={"compartment_id": "connection-compartment"})) + + assert self.hook.get_compartment_id("explicit-compartment") == "explicit-compartment" + + def test_get_compartment_id_from_connection(self): + self.set_connection(Connection(extra={"compartment_id": "connection-compartment"})) + + assert self.hook.get_compartment_id() == "connection-compartment" + + def test_get_compartment_id_requires_value(self): + self.set_connection(Connection()) + + with pytest.raises(ValueError, match="An OCI compartment OCID must be provided"): + self.hook.get_compartment_id() + + def test_connection_form_widgets(self): + pytest.importorskip("flask_appbuilder") + pytest.importorskip("flask_babel") + password_field = pytest.importorskip("wtforms").PasswordField + + widgets = self.hook.get_connection_form_widgets() + + assert set(widgets) == { + "tenancy", + "fingerprint", + "key_content", + "region", + "compartment_id", + } + assert widgets["key_content"].field_class is password_field + + def test_ui_field_behaviour(self): + assert self.hook.get_ui_field_behaviour() == { + "hidden_fields": ["host", "schema", "port"], + "relabeling": { + "login": "User OCID", + "password": "Private Key Passphrase", + }, + "placeholders": { + "login": "ocid1.user...", + "password": "Optional API key passphrase", + "tenancy": "ocid1.tenancy...", + "fingerprint": "aa:bb:cc:...", + "region": "us-chicago-1", + "compartment_id": "ocid1.compartment...", + }, + } + + def test_declarative_placeholders_match_legacy_hook(self): + oci_connection = next( + connection + for connection in get_provider_info()["connection-types"] + if connection["connection-type"] == "oci" + ) + + assert ( + oci_connection["ui-field-behaviour"]["placeholders"] + == self.hook.get_ui_field_behaviour()["placeholders"] + ) + assert oci_connection["conn-fields"]["key_content"]["schema"]["format"] == "password" + + +def test_get_oci_sdk_requires_optional_extra(): + with mock.patch.dict(sys.modules, {"oci": None}): + with pytest.raises( + AirflowOptionalProviderFeatureException, + match=r"pip install 'apache-airflow-providers-oracle\[oci\]'", + ): + _get_oci_sdk() + + +def test_hook_modules_import_without_optional_oci_sdk(): + subprocess.run( + [ + sys.executable, + "-c", + """ +import sys + +sys.modules["oci"] = None +import airflow.providers.oracle.hooks.base_oci +import airflow.providers.oracle.hooks.generative_ai +""", + ], + check=True, + ) Review Comment: I would import the below: `from textwrap import dedent` and then do this: ``` subprocess.run( [ sys.executable, "-c", dedent( """ import sys sys.modules["oci"] = None import airflow.providers.oracle.hooks.base_oci import airflow.providers.oracle.hooks.generative_ai """ ), ], check=True, ) ``` ########## providers/oracle/src/airflow/providers/oracle/hooks/base_oci.py: ########## @@ -0,0 +1,233 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from collections.abc import Callable +from functools import cached_property +from typing import TYPE_CHECKING, Any, Generic, TypeVar + +from airflow.providers.common.compat.sdk import AirflowOptionalProviderFeatureException, BaseHook + +if TYPE_CHECKING: + from airflow.sdk import Connection + + OciSigner = Any + +OciClient = TypeVar("OciClient") + +OCI_AUTH_TYPE_API_KEY = "api_key" +OCI_AUTH_TYPE_CONFIG_FILE = "config_file" +OCI_AUTH_TYPE_INSTANCE_PRINCIPAL = "instance_principal" +OCI_AUTH_TYPE_RESOURCE_PRINCIPAL = "resource_principal" +OCI_AUTH_TYPES = ( + OCI_AUTH_TYPE_API_KEY, + OCI_AUTH_TYPE_CONFIG_FILE, + OCI_AUTH_TYPE_INSTANCE_PRINCIPAL, + OCI_AUTH_TYPE_RESOURCE_PRINCIPAL, +) + + +def _get_oci_sdk() -> Any: + try: + import oci + except ImportError as e: + raise AirflowOptionalProviderFeatureException( + "OCI features require the optional OCI Python SDK. " + "Install it with: pip install 'apache-airflow-providers-oracle[oci]'" + ) from e + return oci + + +class OciBaseHook(BaseHook, Generic[OciClient]): + """ + Base hook for Oracle Cloud Infrastructure services. + + The hook supports API key, OCI configuration file, instance principal, and resource principal + authentication. API key credentials are read from the connection fields, while principal + authentication is delegated to the OCI SDK. + + :param oci_conn_id: The :ref:`OCI connection id <howto/connection:oci>`. + :param auth_type: OCI authentication type selected by the Dag author. + :param key_file: API signing private key path selected by the Dag author. + :param config_file: OCI SDK configuration file selected by the Dag author. + :param profile: Profile to load from the OCI SDK configuration file. + :param service_endpoint: Optional service endpoint selected by the Dag author. Review Comment: I would add the default values for each parameter to this docstring. ########## providers/oracle/src/airflow/providers/oracle/hooks/base_oci.py: ########## @@ -0,0 +1,233 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from collections.abc import Callable +from functools import cached_property +from typing import TYPE_CHECKING, Any, Generic, TypeVar + +from airflow.providers.common.compat.sdk import AirflowOptionalProviderFeatureException, BaseHook + +if TYPE_CHECKING: + from airflow.sdk import Connection + + OciSigner = Any + +OciClient = TypeVar("OciClient") + +OCI_AUTH_TYPE_API_KEY = "api_key" +OCI_AUTH_TYPE_CONFIG_FILE = "config_file" +OCI_AUTH_TYPE_INSTANCE_PRINCIPAL = "instance_principal" +OCI_AUTH_TYPE_RESOURCE_PRINCIPAL = "resource_principal" Review Comment: This looks like a textbook scenario for an enum class. Please see the below: ``` class OciAuthType(str, Enum): API_KEY = "api_key" CONFIG_FILE = "config_file" INSTANCE_PRINCIPAL = "instance_principal" RESOURCE_PRINCIPAL = "resource_principal" ``` ########## providers/oracle/docs/generative_ai.rst: ########## @@ -0,0 +1,143 @@ + .. Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + .. http://www.apache.org/licenses/LICENSE-2.0 + + .. Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +OCI Generative AI Hosted Applications +===================================== + +:class:`~airflow.providers.oracle.hooks.generative_ai.OciGenerativeAIHook` uses the official +`OCI Python SDK <https://docs.oracle.com/en-us/iaas/tools/python/latest/>`__ to manage +`Hosted Applications and deployments +<https://docs.oracle.com/en-us/iaas/Content/generative-ai/agents.htm#deployments>`__. +Install ``apache-airflow-providers-oracle[oci]`` and configure an +:ref:`OCI connection <howto/connection:oci>` before using the hook. +The hook exposes the native :class:`oci.generative_ai.GenerativeAiClient` through ``conn`` and +``get_conn()``. Operators can therefore call OCI SDK methods directly without an Airflow wrapper +for every API operation. + +Oracle exposes separate application APIs for `two inbound authentication variants +<https://docs.oracle.com/en-us/iaas/Content/generative-ai/create-application.htm#authentication>`__: + +* Identity domain bearer tokens use the OCI SDK ``HostedApplication`` resource and client methods + ending in ``hosted_application`` or ``hosted_applications``. +* OCI IAM request signing uses ``HostedApplicationIam`` and client methods ending in + ``hosted_application_iam`` or ``hosted_applications_iam``. + +This distinction configures how clients invoke the deployed application. It does not change how +the Airflow hook authenticates to the OCI management API; both variants use the configured +:ref:`OCI connection <howto/connection:oci>`. + +Management endpoints +-------------------- + +The OCI SDK derives the management endpoint as +``https://generativeai.<region>.oci.oraclecloud.com`` and adds the ``20231130`` API base path. +The hook exposes these operations without changing OCI retry, pagination, or concurrency-control +arguments. + +========================================================== =================================================== +OCI SDK client method REST operation +========================================================== =================================================== +``create_hosted_application`` ``POST /20231130/hostedApplications`` Review Comment: Why is the version number for the API endpoint i.e. 20231130 hardcoded? Won't this change? Also, I think this table might not be needed here. I go into more detail in a comment below but basically it appears that you are duplicating information that should belong in the OCI SDK documentation. ########## providers/oracle/docs/generative_ai.rst: ########## @@ -0,0 +1,143 @@ + .. Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + .. http://www.apache.org/licenses/LICENSE-2.0 + + .. Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + +OCI Generative AI Hosted Applications +===================================== + +:class:`~airflow.providers.oracle.hooks.generative_ai.OciGenerativeAIHook` uses the official +`OCI Python SDK <https://docs.oracle.com/en-us/iaas/tools/python/latest/>`__ to manage +`Hosted Applications and deployments +<https://docs.oracle.com/en-us/iaas/Content/generative-ai/agents.htm#deployments>`__. +Install ``apache-airflow-providers-oracle[oci]`` and configure an +:ref:`OCI connection <howto/connection:oci>` before using the hook. +The hook exposes the native :class:`oci.generative_ai.GenerativeAiClient` through ``conn`` and +``get_conn()``. Operators can therefore call OCI SDK methods directly without an Airflow wrapper +for every API operation. + +Oracle exposes separate application APIs for `two inbound authentication variants +<https://docs.oracle.com/en-us/iaas/Content/generative-ai/create-application.htm#authentication>`__: + +* Identity domain bearer tokens use the OCI SDK ``HostedApplication`` resource and client methods + ending in ``hosted_application`` or ``hosted_applications``. +* OCI IAM request signing uses ``HostedApplicationIam`` and client methods ending in + ``hosted_application_iam`` or ``hosted_applications_iam``. + +This distinction configures how clients invoke the deployed application. It does not change how +the Airflow hook authenticates to the OCI management API; both variants use the configured +:ref:`OCI connection <howto/connection:oci>`. + +Management endpoints +-------------------- + +The OCI SDK derives the management endpoint as +``https://generativeai.<region>.oci.oraclecloud.com`` and adds the ``20231130`` API base path. +The hook exposes these operations without changing OCI retry, pagination, or concurrency-control +arguments. + +========================================================== =================================================== +OCI SDK client method REST operation +========================================================== =================================================== +``create_hosted_application`` ``POST /20231130/hostedApplications`` +``get_hosted_application`` ``GET /20231130/hostedApplications/{id}`` +``list_hosted_applications`` ``GET /20231130/hostedApplications`` +``update_hosted_application`` ``PUT /20231130/hostedApplications/{id}`` +``delete_hosted_application`` ``DELETE /20231130/hostedApplications/{id}`` +``create_hosted_application_iam`` ``POST /20231130/hostedApplicationsIam`` +``get_hosted_application_iam`` ``GET /20231130/hostedApplicationsIam/{id}`` +``list_hosted_applications_iam`` ``GET /20231130/hostedApplicationsIam`` +``update_hosted_application_iam`` ``PUT /20231130/hostedApplicationsIam/{id}`` +``delete_hosted_application_iam`` ``DELETE /20231130/hostedApplicationsIam/{id}`` +``create_hosted_deployment`` ``POST /20231130/hostedDeployments`` +``get_hosted_deployment`` ``GET /20231130/hostedDeployments/{id}`` +``list_hosted_deployments`` ``GET /20231130/hostedDeployments`` +``update_hosted_deployment`` ``PUT /20231130/hostedDeployments/{id}`` +``delete_hosted_deployment`` ``DELETE /20231130/hostedDeployments/{id}`` +``get_work_request`` ``GET /20231130/workRequests/{id}`` +``list_work_request_errors`` ``GET /20231130/workRequests/{id}/errors`` +``list_work_request_logs`` ``GET /20231130/workRequests/{id}/logs`` +``list_work_requests`` ``GET /20231130/workRequests`` +========================================================== =================================================== + +All client methods return the native :class:`oci.response.Response`. This preserves response data and +headers such as ``etag``, ``opc-request-id``, and ``opc-work-request-id``. Create, update, and +delete operations can be asynchronous; use ``opc-work-request-id`` with +``hook.conn.get_work_request`` to observe their status. + +Use ``hook.get_compartment_id()`` to resolve an explicit compartment or the connection default +before calling list methods. To filter deployments for a Hosted Application, pass its OCID as the +OCI SDK ``application_id`` keyword argument. + +Creating an identity domain Hosted Application +----------------------------------------------- + +Identity domain applications require an ``InboundAuthConfig`` containing the identity domain URL +and OAuth settings: + +.. code-block:: python + + from oci.generative_ai.models import ( + CreateHostedApplicationDetails, + IdcsAuthConfig, + InboundAuthConfig, + ) + + from airflow.providers.oracle.hooks.generative_ai import OciGenerativeAIHook + + hook = OciGenerativeAIHook(oci_conn_id="oci_default") + response = hook.conn.create_hosted_application( + CreateHostedApplicationDetails( + display_name="airflow-agent-oauth", + compartment_id="ocid1.compartment.oc1..example", + inbound_auth_config=InboundAuthConfig( + inbound_auth_config_type="IDCS_AUTH_CONFIG", + idcs_config=IdcsAuthConfig( + domain_url="https://idcs-example.identity.oraclecloud.com";, + scope="agent.invoke", + audience="https://agent.example.com";, + ), + ), + ) + ) + work_request_id = response.headers.get("opc-work-request-id") + +Creating an OCI IAM Hosted Application +-------------------------------------- + +OCI IAM applications do not require an OAuth or identity domain configuration: + +.. code-block:: python + + from oci.generative_ai.models import CreateHostedApplicationIamDetails + + from airflow.providers.oracle.hooks.generative_ai import OciGenerativeAIHook + + hook = OciGenerativeAIHook(oci_conn_id="oci_default") + response = hook.conn.create_hosted_application_iam( + CreateHostedApplicationIamDetails( + display_name="airflow-agent", + compartment_id="ocid1.compartment.oc1..example", + description="Hosted application managed by Airflow", + ) + ) + work_request_id = response.headers.get("opc-work-request-id") + +Agent invocation +---------------- + +This hook covers the Generative AI management API only. Invoking an active Hosted Application uses +the Generative AI inference endpoint and a custom application path; it is intentionally outside this +management hook's contract. Review Comment: This is more of a general comment about this piece of documentation: it seems like you are going quite deep into the OCI SDK rather than zooming out and focusing on the Airflow interface. My concern is that this effectively becomes a second API reference that needs to stay in sync with Oracle's SDK and REST documentation. If new methods are added or endpoints change, we'll need to update these docs even though the Airflow hook itself hasn't changed. Maybe it would be bettter to briefly describe the connection/authentication model, explain that `conn`/`get_conn()` expose the native `GenerativeAiClient`, provide a couple of usage examples, and then link to the official OCI SDK documentation for the complete API surface. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
