Improve connection fetching from AWS Secrets Manager

[wolfier]

Description

Users need to make the values are URL safe in the when storing them in AWS Secrets Managers to experience the expected behaviour when fetching a connection and/or using a hook.

Use case/motivation

When a user adds the connection from the Airflow UI, the attributes of the connection, the individual fields, are stored in the metadata database as is. They are not made URL safe. This is why a user would expect saving the values as is would suffice when storing values in Secrets Manager. The difference here is how Airflow fetches the value from the MetaStoreBackend versus the SecretsManagerBackend.

In the case of MetaStoreBackend, when Airflow pulls the row in the connection table from the metadata database, the data is already a Connection object as defined by the ORM.

1. Query database for connection data (source)
2. Connection data is returned as an ORM object

However, with SecretsManagerBackend, Airflow first converts it to an URI string then create a connection object from the string which expects the string to be url safe.

1. Grab the plaintext value and convert it to a dictionary (source)
2. Format a URI template with the dictionary (source)
3. Create a connection object from the URI string (source)

Related issues

No response

Are you willing to submit a PR?

• Yes I am willing to submit a PR!

Code of Conduct

• I agree to follow this project's Code of Conduct

[potiuk]

I am not sure what you are proposing here :)

Do you propose to document that they need to be url-encoded, or do you propose to change the behaviour when values are read from dict ? @dstandish - I think that might be of interest of yours. But I think @wolfier you should clarify what your proposal is.

[wolfier]

The goal of this feature request is to improve the process such that users don't need to worry about making values URL safe in the Secrets Manager UI.

I think we should align what the (Airflow) backends are expecting when storing connection objects as attributes of a connection. This does not change how a connection needs to be stored when the value itself is an URI.

When attributes of a Connection object is saved, MetaStoreBackend accepts non url safe value but SecretsManagerBackend does not.

My proposal for the process is:

1. grab the plaintext value from Secrets Managers and convert into a dictionary (SecretsManagerBackend already does this)
2. Use the dictionary to initialize a Connection object

Something like this should suffice (not tested).

import ast
from airflow.models.connection import Connection

def get_connection(self, conn_id) -> Optional['Connection']:
    if self.connections_prefix is None:
        return None

    if self.full_url_mode:
        return self._get_secret(self.connections_prefix, conn_id)
    else:
        secret_string = self._get_secret(self.connections_prefix, conn_id)
        connection = None

        try:
            conn_args = ast.literal_eval(secret_string)  # json.loads gives error
            connection = Connection(**conn_args)
        except ValueError:  # 'malformed node or string: ' error, for empty conns
            self.log.debug("secret is malformed")
        except TypeError:
            self.log.debug("connection arguments are not as expected")

    return connection

[potiuk]

@dstandish - I think you've been dealing with various connection dictionary vs. string approaches recently - wdyt ?

[dstandish]

yeah the problem is this line:

airflow/airflow/providers/amazon/aws/secrets/secrets_manager.py

Line 179 in 7ab45d4

conn_string = "{conn_type}://{user}:{password}@{host}:{port}/{schema}".format(**conn_d)

they are building the URI in a very naive way. the way to reliably build a uri is with Connection.get_uri(). So that's what i'd do here. But it's a little backwards cus we're generating a Conn, to generate a URI, to ultimately generate a connection again. So best is probably if this backend is updated so that get_connection is implemented and not get_conn_value (or previously get_conn_uri). Then in full_url_mode we can retrieve the uri and build the connection, but if not using full_url_mode there's really no reason to bother generating the URI at all -- it's only asking for trouble.