Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions airflow/config_templates/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,17 @@
type: string
example: ~
default: ""
- name: backends_config
description: |
Advanced secrets backend configuration, allow to user configure more than one secret backend,
order of secrets backend and turn off built-in backends.
Expected JSON list of objects. See
https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/secrets-backend/index.html#advanced-configuration
for more details.
version_added: 2.4.0
example: ~
type: string
default: ""
- name: cli
description: ~
options:
Expand Down
7 changes: 7 additions & 0 deletions airflow/config_templates/default_airflow.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,13 @@ backend =
# ``{{"connections_prefix": "/airflow/connections", "profile_name": "default"}}``
backend_kwargs =

# Advanced secrets backend configuration, allow to user configure more than one secret backend,
Comment thread
dstandish marked this conversation as resolved.
# order of secrets backend and turn off built-in backends.
# Expected JSON list of objects. See
# https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/secrets-backend/index.html#advanced-configuration
# for more details.
backends_config =

@potiuk potiuk Jul 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it woudl be nice to add the default configuration as comment. It would give the user an opportunity to figure out how to do it without reaching to documentation.

There are two kinds of users, those who love reading the docs, and those who prefer to explore. Comment (or even default value written here) would actually be nice one for the latter group.

@Taragolis Taragolis Jul 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... default value for this PR implementation - "not defined", null or None and fallback to current implementation.

However if we decide to keep only one way to configure backends in this case we might set as default:

backends_config = [{"backend":"airflow.secrets.environment_variables.EnvironmentVariablesBackend"}, {"backend":"airflow.secrets.metastore.MetastoreBackend"}]

@potiuk potiuk Jul 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that we cannot do that actually - because lack of the backend_configs determines if we should use backend and backend_kwargs - by adding default we would break compatibiliyt if someone generates the default config and moves their "backend/backend_kwarg" - it's mabe not very proabable scenario but might happen

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing - I think we shoudl raise a deprecation warning when backend/backend_kwargs is used and maybe even provide a detailed information how backends_config should look like in this case? We should have enough info to work-out what should be the replacement?

Also another warning should be raised when 'backends_configis defined and either of thebackend_kwargs, backend` are set. This might happen that someone will set the three of them, and backends_config should get precedence, but it should also raise warnig to the user to remove the two old settings - this might be a mistake on the side of configuring user, and in the worst case it should lead to cleaer and less ambiguous config eventually.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HAving al those warnings will give a transition path for the users so that in 3 we could remove the old configs (and then default value for backend_config would make perfect sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might happen that someone will set the three of them, and backends_config should get precedence

I can't find the good way how to do it right now with AirflowConfigParser.deprecated_options, because if we put this configs as deprecated and try to check is user actually put something in this config the result will - deprecation warning every time even if user do not use that.

I think I do not try only one option - get this configs without fallback, in this case probably we only have to catch exception and no deprecation warnings raised in case if user do not set anything. It just my assuming.

Another interesting moment it would a first time when one config option replace two options but right now all deprecation expected one-to-one

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


[cli]
# In what way should the cli access the API. The LocalClient will use the
# database directly, while the json_client will use the api running on the
Expand Down
166 changes: 125 additions & 41 deletions airflow/configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,21 @@
import sys
import warnings
from base64 import b64encode
from collections import OrderedDict
from collections import OrderedDict, UserList

# Ignored Mypy on configparser because it thinks the configparser module has no _UNSET attribute
from configparser import _UNSET, ConfigParser, NoOptionError, NoSectionError # type: ignore
from contextlib import suppress
from dataclasses import dataclass, field
from json.decoder import JSONDecodeError
from re import Pattern
from typing import IO, Any, Dict, Iterable, List, Optional, Set, Tuple, Union
from typing import IO, Any, Dict, Iterable, List, Optional, Sequence, Set, Tuple, Union
from urllib.parse import urlparse

from typing_extensions import overload

from airflow.exceptions import AirflowConfigException
from airflow.secrets import DEFAULT_SECRETS_SEARCH_PATH, BaseSecretsBackend
from airflow.secrets import BaseSecretsBackend
from airflow.utils import yaml
from airflow.utils.module_loading import import_string
from airflow.utils.weight_rule import WeightRule
Expand All @@ -62,6 +63,16 @@
ENV_VAR_PREFIX = 'AIRFLOW__'


DEFAULT_SECRETS_SEARCH_PATH: List[Dict[str, Any]] = [
{
"backend": "airflow.secrets.environment_variables.EnvironmentVariablesBackend",
},
{
"backend": "airflow.secrets.metastore.MetastoreBackend",
},
]


def _parse_sqlite_version(s: str) -> Tuple[int, ...]:
match = _SQLITE3_VERSION_PATTERN.match(s)
if match is None:
Expand Down Expand Up @@ -113,18 +124,20 @@ def run_command(command: str) -> str:

def _get_config_value_from_secret_backend(config_key: str) -> Optional[str]:
"""Get Config option values from Secret Backend"""
try:
secrets_client = get_custom_secret_backend()
if not secrets_client:
return None
return secrets_client.get_config(config_key)
except Exception as e:
raise AirflowConfigException(
'Cannot retrieve config from alternative secrets backend. '
'Make sure it is configured properly and that the Backend '
'is accessible.\n'
f'{e}'
)
config_value = None
for secrets_client in ensure_secrets_loaded():
try:
config_value = secrets_client.get_config(config_key)
if config_value:
break
except Exception as e:
raise AirflowConfigException(
f'Cannot retrieve config from secrets backend `{secrets_client.__class__.__name__}`. '
f'Make sure it is configured properly and that the Backend is accessible.\n'
f'{e}'
)

return config_value


def _default_config_file_path(file_name: str) -> str:
Expand Down Expand Up @@ -1487,47 +1500,118 @@ def set(*args, **kwargs) -> None:
conf.set(*args, **kwargs)


def ensure_secrets_loaded() -> List[BaseSecretsBackend]:
"""
Ensure that all secrets backends are loaded.
If the secrets_backend_list contains only 2 default backends, reload it.
"""
# Check if the secrets_backend_list contains only 2 default backends
if len(secrets_backend_list) == 2:
return initialize_secrets_backends()
return secrets_backend_list
class DefaultSecretsBackend(UserList):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this class? Is not just normal "list" enough?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not enough in this PR implementation (just what we have right now). It is fine for current implementation which assume that if length of backends list is 2 than backends not initialised

if len(secrets_backend_list) == 2:

Might be better (just an idea) create separate class for initialise secrets backends which care about initialise backends itself and some additional methods inside this class, like this

def _get_config_value_from_secret_backend(config_key: str) -> Optional[str]:
"""Get Config option values from Secret Backend"""
try:
secrets_client = get_custom_secret_backend()
if not secrets_client:
return None
return secrets_client.get_config(config_key)
except Exception as e:
raise AirflowConfigException(
'Cannot retrieve config from alternative secrets backend. '
'Make sure it is configured properly and that the Backend '
'is accessible.\n'
f'{e}'
)

"""List Container which use for store default secrets backends."""


@dataclass(frozen=True)
class SecretsBackendConfig:
Comment thread
dstandish marked this conversation as resolved.
"""Secrets Backend Config dataclass helper."""

backend: str
backend_kwargs: Dict[str, Any] = field(default_factory=dict)

@classmethod
def from_dict(cls, d: Dict[str, Any]) -> 'SecretsBackendConfig':
"""
Read Secret Backend Config from dictionary

Ignores all unexpected keywords
"""
return cls(d['backend'], d.get('backend_kwargs', {}))

@classmethod
def from_config(cls) -> Optional['SecretsBackendConfig']:
"""Try to get ``SecretsBackendConfig`` from airflow config [secrets] section"""
secrets_backend = conf.get(section='secrets', key='backend', fallback=None)

def get_custom_secret_backend() -> Optional[BaseSecretsBackend]:
"""Get Secret Backend if defined in airflow.cfg"""
secrets_backend_cls = conf.getimport(section='secrets', key='backend')
if not secrets_backend:
return None

if secrets_backend_cls:
try:
backends: Any = conf.get(section='secrets', key='backend_kwargs', fallback='{}')
alternative_secrets_config_dict = json.loads(backends)
secrets_backend_kwargs: Any = conf.get(section='secrets', key='backend_kwargs', fallback='{}')
secrets_backend_kwargs = json.loads(secrets_backend_kwargs)
except JSONDecodeError:
alternative_secrets_config_dict = {}
secrets_backend_kwargs = {}

return cls(secrets_backend, secrets_backend_kwargs)

def initialize(self) -> BaseSecretsBackend:
"""Initialize Secrets Backend."""
return import_string(self.backend)(**self.backend_kwargs)


class UniqueSecretsBackendsConfigs(UserList):
Comment thread
dstandish marked this conversation as resolved.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any valid reason why a user would want to specify duplicates of (class, kwargs) so it feels like we should treat that case as an error and throw an exception during validate time.

And since this is used in exactly one location, a custom class here feels like huge overkill -- essentially all we need is if len(set(backends)) != len(backends): raise AirflowConfigException.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I've tried to solve couple issues

  1. Set only unique values - I think no reason to check backend if it already added
  2. Inform user if this backend already exists.
  3. If we decide to deprecate old values (initially I think keep both), than it would be easy to add in the end default backend, and if user already add it - nothing bad happen

Actually this class didn't solve second issue -there are no warnings in service logs. I think something suppress it.

"""List Container which use for store unique secrets backends configs."""

return secrets_backend_cls(**alternative_secrets_config_dict)
return None
def append(self, config) -> None:
"""Append item to a list if it not exists yet"""
if config in self.data:
log.warning("%r already exists.", config)
return
self.data.append(config)

def extend(self, configs) -> None:
"""Extends item if it not exists yet"""
for config in configs:
self.append(config)


def ensure_secrets_loaded() -> Sequence[BaseSecretsBackend]:
"""Ensure that all secrets backends are loaded."""
if isinstance(secrets_backend_list, DefaultSecretsBackend):
return initialize_secrets_backends()
return secrets_backend_list


def initialize_secrets_backends() -> List[BaseSecretsBackend]:
def initialize_secrets_backends() -> Sequence[BaseSecretsBackend]:
"""
* read secrets backend configurations
* keep only unique secret backend configurations
* import secrets backend classes
* instantiate them and return them in a list
"""
backend_list = []
backend_configs = UniqueSecretsBackendsConfigs()
default_configs = [SecretsBackendConfig.from_dict(config) for config in DEFAULT_SECRETS_SEARCH_PATH]

secrets_backend_config = conf.getjson(section='secrets', key='backends_config', fallback=None)
if secrets_backend_config:
# If [secrets] 'backend_config' exists and defined than use this configuration
# instead of [secrets] 'backend' and [secrets] 'backend_kwargs'.
if not isinstance(secrets_backend_config, list):
raise AirflowConfigException(
f"[secrets] 'backends_config' expected list of backends configurations but got"
f" {secrets_backend_config!r}."
)

for config in secrets_backend_config:
try:
backend_configs.append(SecretsBackendConfig.from_dict(config))
except Exception as e:
raise AirflowConfigException(
f"Cannot read config: {config!r} from [secrets] 'backends_config'.\n{e}"
) from e

custom_secret_backend = get_custom_secret_backend()
else:
custom_secrets_backend_config = SecretsBackendConfig.from_config()
if not custom_secrets_backend_config:
# Returns default secrets backend list for further checks in `ensure_secrets_loaded()`.
return DefaultSecretsBackend(config.initialize() for config in default_configs)

if custom_secret_backend is not None:
backend_list.append(custom_secret_backend)
backend_configs.append(custom_secrets_backend_config)
backend_configs.extend(default_configs)

for class_name in DEFAULT_SECRETS_SEARCH_PATH:
secrets_backend_cls = import_string(class_name)
backend_list.append(secrets_backend_cls())
# Initialize secrets backends
backend_list = []
for config in backend_configs:
try:
backend_list.append(config.initialize())
except Exception as e:
raise AirflowConfigException(
f"Cannot initialize secrets backend {config.backend!r} "
f"with keyword arguments {config.backend_kwargs}.\n"
f"{e}"
) from e

return backend_list

Expand Down
7 changes: 1 addition & 6 deletions airflow/secrets/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@
* Metastore database
* AWS SSM Parameter store
"""
__all__ = ['BaseSecretsBackend', 'DEFAULT_SECRETS_SEARCH_PATH']
__all__ = ['BaseSecretsBackend']

from airflow.secrets.base_secrets import BaseSecretsBackend

DEFAULT_SECRETS_SEARCH_PATH = [
"airflow.secrets.environment_variables.EnvironmentVariablesBackend",
"airflow.secrets.metastore.MetastoreBackend",
]
50 changes: 46 additions & 4 deletions docs/apache-airflow/security/secrets/secrets-backend/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ Search path
When looking up a connection/variable, by default Airflow will search environment variables first and metastore
database second.

If you enable an alternative secrets backend, it will be searched first, followed by environment variables,
then metastore. This search ordering is not configurable.
If you enable an alternative secrets backend, it will be searched first, followed by environment
variables, then metastore. This search ordering is not configurable in Airflow <2.4.0.

.. warning::

Expand All @@ -59,6 +59,7 @@ The ``[secrets]`` section has the following options:
[secrets]
backend =
backend_kwargs =
backends_config =

Set ``backend`` to the fully qualified class name of the backend you want to enable.

Expand All @@ -73,6 +74,47 @@ the example below.
$ airflow config get-value secrets backend
airflow.providers.google.cloud.secrets.secret_manager.CloudSecretManagerBackend

Advanced Configuration
""""""""""""""""""""""

.. versionadded:: 2.4.0

You can provide ``backends_config`` with json-list which contains json-objects of required backends for:

* Configure more than one alternative secrets backend.
* Change search ordering of secrets backends.
* Turn off built-in backends (environment variables or the metastore database).

Object should contains mandatory **backend** attribute with fully qualified class name of the backend you want to enable
and optionally contains **backend_kwargs** with json-object which will be passed as kwargs to the ``__init__``
method of your secrets backend.

Here's a basic example of how to configure to use Environment Variables first,
next :ref:`SSM Parameter Store <ssm_parameter_store_secrets>`, and then metastore.:

.. code-block:: json

[
{
"backend": "airflow.secrets.environment_variables.EnvironmentVariablesBackend"
},
{
"backend": "airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
"backend_kwargs": {
"connections_prefix": "/airflow/connections",
"profile_name": "default"
}
},
{
"backend": "airflow.secrets.metastore.MetastoreBackend"
}
]
Comment on lines +97 to +111

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this as a simpler-for-users syntax:

Suggested change
[
{
"backend": "airflow.secrets.environment_variables.EnvironmentVariablesBackend"
},
{
"backend": "airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
"backend_kwargs": {
"connections_prefix": "/airflow/connections",
"profile_name": "default"
}
},
{
"backend": "airflow.secrets.metastore.MetastoreBackend"
}
]
[
"airflow.secrets.environment_variables.EnvironmentVariablesBackend"
[
"airflow.providers.amazon.aws.secrets.systems_manager.SystemsManagerParameterStoreBackend",
{
"connections_prefix": "/airflow/connections",
"profile_name": "default"
}
],
"airflow.secrets.metastore.MetastoreBackend"
]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is very much a suggestion/talking point, not a "please do it this way")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One benefit with dictionary (json-object) we could add additional configuration and do not worry about actual position in list, however side effect a bit longer json-string in configuration which could make users unhappy.


.. warning::

If you specify ``backends_config`` options than Airflow will ignore values
in ``backend`` and ``backend_kwargs`` options.
Comment on lines +115 to +116

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silently ignoring config is a recipie for confusion -- we should make this an error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I configure AIRFLOW__CORE__SQL_ALCHEMY_CONN and AIRFLOW__DATABASE__SQL_ALCHEMY_CONN in airflow 2.3.x then AIRFLOW__CORE__SQL_ALCHEMY_CONN would ignored, am I right?

Anyway if we decide deprecate old values in some new version then better remove information about old configuration with warning if user use old configurations.


Supported core backends
^^^^^^^^^^^^^^^^^^^^^^^

Expand Down Expand Up @@ -101,8 +143,8 @@ Roll your own secrets backend
A secrets backend is a subclass of :py:class:`airflow.secrets.BaseSecretsBackend` and must implement either
:py:meth:`~airflow.secrets.BaseSecretsBackend.get_connection` or :py:meth:`~airflow.secrets.BaseSecretsBackend.get_conn_value`.

After writing your backend class, provide the fully qualified class name in the ``backend`` key in the ``[secrets]``
section of ``airflow.cfg``.
After writing your backend class, provide the fully qualified class name in the ``backend`` key or provide

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than supporting two ways of configuration, I think it would be better to just support one way and deprecate the old way. if you look at AirflowConfigParser, you'll find things concerning "deprecated_values" and "upgraded_values"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that better support only one way of configuration secrets backends.
However I think old way in any cases should be supported until it entirely remove from Airflow (3.0.0 ?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right and what i am telling you is that you need to initiate deprecation properly, using the framework we have for this kind of thing. for example see here: https://github.com/apache/airflow/blob/main/airflow/configuration.py#L171

the option for base_log_folder was moved from core to logging -- we don't keep the deprecated values around in the old location. we only keep the new location. and we handle "upgrading" deprecated approaches in AirflowConfigParser. see also methods _upgrade_auth_backends and _upgrade_postgres_metastore_conn. so it seems to me the right approach is to formally remove it from the config immediately as a deprecation and add the logic to "upgrade" or "convert" the deprecated legacy values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach in general fine only one thing that could be a problem to do it in the same way for secrets backend

conf = initialize_config()
secrets_backend_list = initialize_secrets_backends()
conf.validate()

Secrets Backend initialised before config validated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably it not a big deal actually. Need to check it a bit later.
Seems like when secret backends initialised first time it can't get any values, so it initialised with default backend

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not proposing rewrite entire configurations of Airflow and deprecation model or solve all issues of configurations related only to SB. I'm essentially saying, when you want to change configuration style, you change it; you don't add a another style.

I understand that with SB there are complexities with being also able to take "secret" config items from it, and I understand that unfortunately it's a bit chicken egg and confusing and it's not my favorite --- but it's already like that. So maybe this means you can't copy the existing "upgrade" examples exactly -- maybe you have to do something a little different, a little more difficult. But the fundamental principle here is not to "multiply" configuration approaches but to just change the configuration approach, and to maintain backcompat with deprecation warning for a time. I am pretty confident you can do that.

But keep in mind, I'm not the boss, right? This is open source. And if you disagree with my thinking, and if no one else is chimes in here, one thing you can always do is start a discussion on the dev list, starting a thread like [DISCUSS] To add or update secrets backend configuration approach and you can state your case and see if you find support.

@Taragolis Taragolis Jun 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing personal but let me explain how it looks like for my side. I invest some my time to check how it possible to do it right now with same approach and found that withouts hacks in runtime it is hardly possible but you've told me:

So maybe this means you can't copy the existing "upgrade" examples exactly -- maybe you have to do something a little different, a little more difficult

Which more like for me I don't know how to do it, but think better and do it better - that is last thing that I or someone else expects see on review (both on open source or commercial development) . If you have suggestion how to do it better right now without rewrite a lot of stuff - feel free to suggest.


Right now Secrets Backends ignore "better approach", implicit upgrades values and do not track it. I think everyone sleep well

try:
backends: Any = conf.get(section='secrets', key='backend_kwargs', fallback='{}')
alternative_secrets_config_dict = json.loads(backends)
except JSONDecodeError:
alternative_secrets_config_dict = {}


Just for reduce your or someone else time for investigate how to do it "in right way right now".

Specify directly AirflowConfigParser.deprecated_options

You can't just placed in AirflowConfigParser.deprecated_options something like ('secrets', 'backends_config'): ('secrets', 'backend', '2.4.0') because we need to check that [secrets] backend defined and upgrades required.

    def _upgrade_secrets_backend(self):
        old_backend = self.get(section='secrets', key='backend', fallback='')
        if old_backend:
            ...

Code above always will return warning even if user do not define anything in [secrets] backend
DeprecationWarning: The backend option in [secrets] has been renamed to backends_config - the old setting has been used, but please update your config.

Possible solution? Define in deprecation config ('secrets', 'backends_config'): ('secrets', 'backend', '2.4.0') right after upgrade. Which might be unclear for developers, because they should keep it in their mind that something outside could change AirflowConfigParser.deprecated_options

Deprecate [secrets] backend_kwargs

It is not possible right now by define something in AirflowConfigParser.

Possible solution? Show warning that this config doesn't use anymore and anywhere on upgrade. However right now we also could warn users with custom message on init secrets backend

Warning show that only one option deprecated

We need to inform user by this message

DeprecationWarning: The backend and backend_kwargs options in [secrets] has been moved to backends_config - the old setting has been used, but please update your config.

But now we can get only predefined

            warnings.warn(
                f'The {deprecated_name} option in [{deprecated_section}] has been moved to the {key} option '
                f'in [{section}] - the old setting has been used, but please update your config.',
                DeprecationWarning,
                stacklevel=3,
            )

Possible solution? Rewrite AirflowConfigParser.deprecated_options and methods which check and show this warning to make it more generic.


I'm absolutely agree with your idea that we need to have only one approach with deprecated values.
But right now Secrets Backend doesn't use this approach in anyway and without changes in AirflowConfigParser it is hardly possible.

And this PR only about extends functional of Secrets Backend not changes in AirflowConfigParser. If we want to change AirflowConfigParser better to open separate PR in this case if we need to revert back changes we would revert "Add advanced secrets backend configurations" or "Make AirflowConfigParser more generic with deprecation warning" but not both.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which more like for me I don't know how to do it, but think better and do it better - that is last thing that I or someone else expects see on review (both on open source or commercial development) . If you have suggestion how to do it better right now without rewrite a lot of stuff - feel free to suggest.

I'm just saying on the face of it, it seems it should be possible to update the config approach for secrets backend without keeping the old one live. And no, I did not investigate exactly how you should do this.

I'm absolutely agree with your idea that we need to have only one approach with deprecated values.
But right now Secrets Backend doesn't use this approach in anyway and without changes in AirflowConfigParser it is hardly possible.

What i'm saying is that we should not maintain two config styles concurrently, that the old one should be deprecated, and the nuts and bolts of exactly how I'm less concerned with.

I may be able to explore solutions next week. In the meantime who knows maybe someone else will support your approach.

Thanks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is precedent for handling deprecations specially, checkout

def _upgrade_auth_backends(self):
"""
Ensure a custom auth_backends setting contains session,
which is needed by the UI for ajax queries.
"""
old_value = self.get("api", "auth_backends", fallback="")
if old_value in ('airflow.api.auth.backend.default', ''):
# handled by deprecated_values
pass
elif old_value.find('airflow.api.auth.backend.session') == -1:
new_value = old_value + ",airflow.api.auth.backend.session"
self._update_env_var(section="api", name="auth_backends", new_value=new_value)
self.upgraded_values[("api", "auth_backends")] = old_value
# if the old value is set via env var, we need to wipe it
# otherwise, it'll "win" over our adjusted value
old_env_var = self._env_var_name("api", "auth_backend")
os.environ.pop(old_env_var, None)
warnings.warn(
'The auth_backends setting in [api] has had airflow.api.auth.backend.session added '
'in the running config, which is needed by the UI. Please update your config before '
'Apache Airflow 3.0.',
FutureWarning,
)

So deprecate_config doesn't have to handle this if it's a more complex situation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for suggestion!
I don't think I got how it actually work but I need check also this way.

configurations in the ``backends_config`` in the ``[secrets]`` section of ``airflow.cfg``.

Additional arguments to your SecretsBackend can be configured in ``airflow.cfg`` by supplying a JSON string to ``backend_kwargs``, which will be passed to the ``__init__`` of your SecretsBackend.
See :ref:`Configuration <secrets_backend_configuration>` for more details, and :ref:`SSM Parameter Store <ssm_parameter_store_secrets>` for an example.
Expand Down
Loading