From 6d081fd7f23a757730efc060e139c61392b0884f Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Wed, 7 Jun 2023 01:15:29 -0700 Subject: [PATCH 1/7] Control permissibility of driver config in extra from airflow.cfg --- airflow/configuration.py | 2 +- airflow/providers/odbc/hooks/odbc.py | 29 +++++++++---------- .../connections/odbc.rst | 8 +++++ tests/core/test_configuration.py | 19 +++++++++++- tests/providers/odbc/hooks/test_odbc.py | 5 ++-- 5 files changed, 44 insertions(+), 19 deletions(-) diff --git a/airflow/configuration.py b/airflow/configuration.py index 694032cd9ed34..66e117cae4f11 100644 --- a/airflow/configuration.py +++ b/airflow/configuration.py @@ -486,7 +486,7 @@ def _create_future_warning(name: str, section: str, current_value: Any, new_valu ) def _env_var_name(self, section: str, key: str) -> str: - return f"{ENV_VAR_PREFIX}{section.upper()}__{key.upper()}" + return f"{ENV_VAR_PREFIX}{section.replace('.', '_').upper()}__{key.upper()}" def _get_env_var_option(self, section: str, key: str): # must have format AIRFLOW__{SECTION}__{KEY} (note double underscore) diff --git a/airflow/providers/odbc/hooks/odbc.py b/airflow/providers/odbc/hooks/odbc.py index 1b125ae2f215f..d6260a6418c58 100644 --- a/airflow/providers/odbc/hooks/odbc.py +++ b/airflow/providers/odbc/hooks/odbc.py @@ -36,15 +36,13 @@ class OdbcHook(DbApiHook): :param database: database to use -- overrides connection ``schema`` :param driver: name of driver or path to driver. You can also set the driver via: * setting ``driver`` parameter in ``hook_params`` dictionary when instantiating hook by SQL operators. - * setting `driver`` extra in the connection and setting ``allow_driver_extra`` to True. + * setting ``driver`` extra in the connection and setting ``allow_driver_in_extra`` to True in + section ``providers.odbc`` section of airflow config. * setting ``OdbcHook.default_driver`` in ``local_settings.py`` file. :param dsn: name of DSN to use. overrides DSN supplied in connection ``extra`` :param connect_kwargs: keyword arguments passed to ``pyodbc.connect`` :param sqlalchemy_scheme: Scheme sqlalchemy connection. Default is ``mssql+pyodbc`` Only used for ``get_sqlalchemy_engine`` and ``get_sqlalchemy_connection`` methods. - :param allow_driver_extra: If True, allows to use driver extra in connection string (default False). - You should make sure that you trust the users who can edit connections in the UI to not use it - maliciously. :param kwargs: passed to DbApiHook """ @@ -65,7 +63,6 @@ def __init__( dsn: str | None = None, connect_kwargs: dict | None = None, sqlalchemy_scheme: str | None = None, - allow_driver_extra: bool = False, **kwargs, ) -> None: super().__init__(*args, **kwargs) @@ -76,7 +73,6 @@ def __init__( self._sqlalchemy_scheme = sqlalchemy_scheme self._connection = None self._connect_kwargs = connect_kwargs - self._allow_driver_extra = allow_driver_extra @property def connection(self): @@ -112,15 +108,18 @@ def connection_extra_lower(self) -> dict: def driver(self) -> str | None: """Driver from init param if given; else try to find one in connection extra.""" extra_driver = self.connection_extra_lower.get("driver") - if extra_driver: - if self._allow_driver_extra: - self._driver = extra_driver - else: - self.log.warning( - "Please provide driver via 'driver' parameter of the Hook constructor" - " or via 'hook_params' dictionary 'driver' key when instantiating hook by the" - " SQL operators. The 'driver' extra will not be used." - ) + from airflow.configuration import conf + + if extra_driver and conf.getboolean("providers.odbc", "allow_driver_in_extra", fallback=False): + self._driver = extra_driver + else: + self.log.warning( + "You have supplied 'driver' via connection extra but it will not be used. In order to " + "use 'driver' from extra you must set airflow config setting `allow_driver_in_extra = True` " + "in section `providers.odbc`. Alternatively you may specify driver via 'driver' parameter of " + "the hook constructor or via 'hook_params' dictionary with key 'driver' if using SQL " + "operators." + ) if not self._driver: self._driver = self.default_driver return self._driver.strip().lstrip("{").rstrip("}").strip() if self._driver else None diff --git a/docs/apache-airflow-providers-odbc/connections/odbc.rst b/docs/apache-airflow-providers-odbc/connections/odbc.rst index 176977d8fec04..f74780cbe52c2 100644 --- a/docs/apache-airflow-providers-odbc/connections/odbc.rst +++ b/docs/apache-airflow-providers-odbc/connections/odbc.rst @@ -67,6 +67,14 @@ Extra (optional) * This is only used when ``get_uri`` is invoked in :py:meth:`~airflow.providers.common.sql.hooks.sql.DbApiHook.get_sqlalchemy_engine`. By default, the hook uses scheme ``mssql+pyodbc``. You may pass a string value here to override. + - ``driver`` + * The name of the driver to use on your system. Note that this is only considered if ``allow_driver_in_extra`` + is set to True in airflow config section ``providers.odbc`` (by default it is not considered). + + .. note:: + If setting ``allow_driver_extra``to True, this allows users to set the driver via the Airflow Connection's + ``extra`` field. By default this is not allowed. If enabling this functionality, you should make sure + that you trust the users who can edit connections in the UI to not use it maliciously. .. note:: You are responsible for installing an ODBC driver on your system. diff --git a/tests/core/test_configuration.py b/tests/core/test_configuration.py index 961bc3c7cc1c2..2afc78bcaf000 100644 --- a/tests/core/test_configuration.py +++ b/tests/core/test_configuration.py @@ -226,6 +226,24 @@ def test_command_precedence(self): assert "key4" not in cfg_dict["test"] assert "printf key4_result" == cfg_dict["test"]["key4_cmd"] + def test_can_read_dot_section(self): + test_config = """[test.abc] +key1 = true +""" + test_conf = AirflowConfigParser() + test_conf.read_string(test_config) + section = "test.abc" + key = "key1" + assert test_conf.getboolean(section, key) is True + + with mock.patch.dict( + "os.environ", + { + "AIRFLOW__TEST_ABC__KEY1": "false", # note that the '.' is converted to '_' + }, + ): + assert test_conf.getboolean(section, key) is False + @mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac") @conf_vars( { @@ -596,7 +614,6 @@ def test_command_from_env(self): @pytest.mark.parametrize("display_sensitive, result", [(True, "OK"), (False, "< hidden >")]) def test_as_dict_display_sensitivewith_command_from_env(self, display_sensitive, result): - test_cmdenv_conf = AirflowConfigParser() test_cmdenv_conf.sensitive_config_values.add(("testcmdenv", "itsacommand")) with mock.patch.dict("os.environ"): diff --git a/tests/providers/odbc/hooks/test_odbc.py b/tests/providers/odbc/hooks/test_odbc.py index 3d21806cf98a7..a7ea20a77d955 100644 --- a/tests/providers/odbc/hooks/test_odbc.py +++ b/tests/providers/odbc/hooks/test_odbc.py @@ -184,9 +184,10 @@ def test_driver(self): def test_driver_extra_raises_warning_by_default(self, caplog): with caplog.at_level(logging.WARNING, logger="airflow.providers.odbc.hooks.test_odbc"): driver = self.get_hook(conn_params=dict(extra='{"driver": "Blah driver"}')).driver - assert "Please provide driver via 'driver' parameter of the Hook" in caplog.text + assert "You have supplied 'driver' via connection extra but it will not be used" in caplog.text assert driver is None + @mock.patch.dict("os.environ", {"AIRFLOW__PROVIDERS_ODBC__ALLOW_DRIVER_IN_EXTRA": "TRUE"}) def test_driver_extra_works_when_allow_driver_extra(self): hook = self.get_hook( conn_params=dict(extra='{"driver": "Blah driver"}'), hook_params=dict(allow_driver_extra=True) @@ -211,7 +212,7 @@ def test_driver_extra_raises_warning_and_returns_default_driver_by_default(self, with patch.object(OdbcHook, "default_driver", "Blah driver"): with caplog.at_level(logging.WARNING, logger="airflow.providers.odbc.hooks.test_odbc"): driver = self.get_hook(conn_params=dict(extra='{"driver": "Blah driver2"}')).driver - assert "Please provide driver via 'driver' parameter of the Hook" in caplog.text + assert "have supplied 'driver' via connection extra but it will not be used" in caplog.text assert driver == "Blah driver" def test_database(self): From 444077d891b128d254a834a99485db70f27360d4 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Wed, 7 Jun 2023 10:38:41 -0700 Subject: [PATCH 2/7] fix docs --- docs/apache-airflow-providers-odbc/connections/odbc.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/apache-airflow-providers-odbc/connections/odbc.rst b/docs/apache-airflow-providers-odbc/connections/odbc.rst index f74780cbe52c2..04dbe00f1d058 100644 --- a/docs/apache-airflow-providers-odbc/connections/odbc.rst +++ b/docs/apache-airflow-providers-odbc/connections/odbc.rst @@ -73,8 +73,8 @@ Extra (optional) .. note:: If setting ``allow_driver_extra``to True, this allows users to set the driver via the Airflow Connection's - ``extra`` field. By default this is not allowed. If enabling this functionality, you should make sure - that you trust the users who can edit connections in the UI to not use it maliciously. + ``extra`` field. By default this is not allowed. If enabling this functionality, you should make sure + that you trust the users who can edit connections in the UI to not use it maliciously. .. note:: You are responsible for installing an ODBC driver on your system. From f4961280baac3433a8137bd7d06a39e9512f80e7 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Wed, 7 Jun 2023 11:12:06 -0700 Subject: [PATCH 3/7] note on env vars --- docs/apache-airflow-providers-odbc/connections/odbc.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/apache-airflow-providers-odbc/connections/odbc.rst b/docs/apache-airflow-providers-odbc/connections/odbc.rst index 04dbe00f1d058..11d382eeb661b 100644 --- a/docs/apache-airflow-providers-odbc/connections/odbc.rst +++ b/docs/apache-airflow-providers-odbc/connections/odbc.rst @@ -69,7 +69,8 @@ Extra (optional) scheme ``mssql+pyodbc``. You may pass a string value here to override. - ``driver`` * The name of the driver to use on your system. Note that this is only considered if ``allow_driver_in_extra`` - is set to True in airflow config section ``providers.odbc`` (by default it is not considered). + is set to True in airflow config section ``providers.odbc`` (by default it is not considered). Note: if setting + this config from env vars, use ``AIRFLOW__PROVIDERS_ODBC__ALLOW_DRIVER_IN_EXTRA=true``. .. note:: If setting ``allow_driver_extra``to True, this allows users to set the driver via the Airflow Connection's From 89b1ffbdbcded4c2170f5cf8f8b57e6ed579c8c6 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Wed, 7 Jun 2023 11:21:26 -0700 Subject: [PATCH 4/7] add doc in core config docs --- airflow/providers/odbc/hooks/odbc.py | 2 +- docs/apache-airflow/howto/set-config.rst | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/airflow/providers/odbc/hooks/odbc.py b/airflow/providers/odbc/hooks/odbc.py index d6260a6418c58..5bb1b4074a6b2 100644 --- a/airflow/providers/odbc/hooks/odbc.py +++ b/airflow/providers/odbc/hooks/odbc.py @@ -37,7 +37,7 @@ class OdbcHook(DbApiHook): :param driver: name of driver or path to driver. You can also set the driver via: * setting ``driver`` parameter in ``hook_params`` dictionary when instantiating hook by SQL operators. * setting ``driver`` extra in the connection and setting ``allow_driver_in_extra`` to True in - section ``providers.odbc`` section of airflow config. + section ``providers.odbc`` section of airflow config. * setting ``OdbcHook.default_driver`` in ``local_settings.py`` file. :param dsn: name of DSN to use. overrides DSN supplied in connection ``extra`` :param connect_kwargs: keyword arguments passed to ``pyodbc.connect`` diff --git a/docs/apache-airflow/howto/set-config.rst b/docs/apache-airflow/howto/set-config.rst index 02fa9bac2dc51..928b2d7103e62 100644 --- a/docs/apache-airflow/howto/set-config.rst +++ b/docs/apache-airflow/howto/set-config.rst @@ -38,6 +38,19 @@ or by creating a corresponding environment variable: export AIRFLOW__DATABASE__SQL_ALCHEMY_CONN=my_conn_string +Note that when the section name has a dot in it, you must replace it with an underscore when setting the env var. +For example consider section ``providers.odbc``: + +.. code-block:: ini + + [providers.odbc] + allow_driver_in_extra = true + +.. code-block:: bash + + export AIRFLOW__PROVIDERS_ODBC__ALLOW_DRIVER_IN_EXTRA=true + + You can also derive the connection string at run time by appending ``_cmd`` to the key like this: From b24dfbcfc9b6f7e15ffcef094f05f22d3ecb9f83 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Wed, 7 Jun 2023 12:27:25 -0700 Subject: [PATCH 5/7] fix docs build --- airflow/providers/odbc/hooks/odbc.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/airflow/providers/odbc/hooks/odbc.py b/airflow/providers/odbc/hooks/odbc.py index 5bb1b4074a6b2..5f65517c8732e 100644 --- a/airflow/providers/odbc/hooks/odbc.py +++ b/airflow/providers/odbc/hooks/odbc.py @@ -30,15 +30,17 @@ class OdbcHook(DbApiHook): """ Interact with odbc data sources using pyodbc. + To configure driver, in addition to supplying as constructor arg, the following are also supported: + * set ``driver`` parameter in ``hook_params`` dictionary when instantiating hook by SQL operators. + * set ``driver`` extra in the connection and set ``allow_driver_in_extra`` to True in + section ``providers.odbc`` section of airflow config. + * patch ``OdbcHook.default_driver`` in ``local_settings.py`` file. + See :doc:`/connections/odbc` for full documentation. :param args: passed to DbApiHook :param database: database to use -- overrides connection ``schema`` - :param driver: name of driver or path to driver. You can also set the driver via: - * setting ``driver`` parameter in ``hook_params`` dictionary when instantiating hook by SQL operators. - * setting ``driver`` extra in the connection and setting ``allow_driver_in_extra`` to True in - section ``providers.odbc`` section of airflow config. - * setting ``OdbcHook.default_driver`` in ``local_settings.py`` file. + :param driver: name of driver or path to driver. see above for more info :param dsn: name of DSN to use. overrides DSN supplied in connection ``extra`` :param connect_kwargs: keyword arguments passed to ``pyodbc.connect`` :param sqlalchemy_scheme: Scheme sqlalchemy connection. Default is ``mssql+pyodbc`` Only used for From 8f8fd6fd26a230c663ace4314a6220b065094193 Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Wed, 7 Jun 2023 13:27:45 -0700 Subject: [PATCH 6/7] agnostify w.r.t. provider --- docs/apache-airflow/howto/set-config.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/apache-airflow/howto/set-config.rst b/docs/apache-airflow/howto/set-config.rst index 928b2d7103e62..8220ca2b0c1b3 100644 --- a/docs/apache-airflow/howto/set-config.rst +++ b/docs/apache-airflow/howto/set-config.rst @@ -39,16 +39,16 @@ or by creating a corresponding environment variable: export AIRFLOW__DATABASE__SQL_ALCHEMY_CONN=my_conn_string Note that when the section name has a dot in it, you must replace it with an underscore when setting the env var. -For example consider section ``providers.odbc``: +For example consider pretend section ``providers.some_provider``: .. code-block:: ini - [providers.odbc] - allow_driver_in_extra = true + [providers.some_provider>] + this_param = true .. code-block:: bash - export AIRFLOW__PROVIDERS_ODBC__ALLOW_DRIVER_IN_EXTRA=true + export AIRFLOW__PROVIDERS_SOME_PROVIDER__THIS_PARAM=true You can also derive the connection string at run time by appending ``_cmd`` to From 6fcf5eba6503846eaba787ca2848283dddaf168b Mon Sep 17 00:00:00 2001 From: Daniel Standish <15932138+dstandish@users.noreply.github.com> Date: Wed, 7 Jun 2023 14:04:42 -0700 Subject: [PATCH 7/7] Update docs/apache-airflow/howto/set-config.rst --- docs/apache-airflow/howto/set-config.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/apache-airflow/howto/set-config.rst b/docs/apache-airflow/howto/set-config.rst index 8220ca2b0c1b3..b443cff9c81ae 100644 --- a/docs/apache-airflow/howto/set-config.rst +++ b/docs/apache-airflow/howto/set-config.rst @@ -39,7 +39,7 @@ or by creating a corresponding environment variable: export AIRFLOW__DATABASE__SQL_ALCHEMY_CONN=my_conn_string Note that when the section name has a dot in it, you must replace it with an underscore when setting the env var. -For example consider pretend section ``providers.some_provider``: +For example consider the pretend section ``providers.some_provider``: .. code-block:: ini