You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/16 01:55:12 UTC

[GitHub] [airflow] dwreeves opened a new issue, #25104: `SecretsManagerBackend()` does not need to require URL-encoded values for `full_url_mode=False` connections.

dwreeves opened a new issue, #25104:
URL: https://github.com/apache/airflow/issues/25104

### Apache Airflow Provider(s)

amazon

### Versions of Apache Airflow Providers

I am using `apache-airflow-providers-amazon==2.4.0`, but this issue is still relevant as of the most recent version, `apache-airflow-providers-amazon==4.0.0`.

### Apache Airflow version

2.2.2

### Operating System

OS X

### Deployment

MWAA

### Deployment details

These are my relevant configuration details:

```
[secrets]
backend = airflow.providers.amazon.aws.secrets.secrets_manager.SecretsManagerBackend
backend_kwargs = {"connections_prefix": "", "full_url_mode": false}
```

### What happened

When `full_url_mode=False`, values still need to be URL-encoded.

This creates a break in behavior between how the Airflow UI works and how `SecretsManagerBackend()` works. When I was porting over my config from the Airflow UI to the AWS Secrets Manager, I could not literally copy+paste my config as a JSON, otherwise this resulted in an error. It was also unclear and surprising that it works this aay.

### What you think should happen instead

This behavior makes sense for `full_url_mode=True` in order to ensure that the URL serialization can be parsed properly.

However, this behavior is not actually required for when `full_url_mode=False` because the quotes in the JSON delimit the values, which is what one would reasonably expect to be happening behind the scenes.

This is why I consider this a bug, or at least bug-like:

* it's unexpected behavior (because it's not a URL, it's a JSON)
* It deviates from how a core implementation works (i.e. the Airflow UI)
* It is not required to behave like this.

### How to reproduce

The way I struggled with this was in creating a Slack hook. The Slack hook takes an input something like this:

```json
{
"conn_type": "http",
"host": "https://hooks.slack.com/services",
"password": "/path/to/hook"
}
```

This works fine using the Airflow UI without URL-encoding, however it is necessary to URL encode everything before passing it through the `SecretsManagerBackend()`.

![image](https://user-images.githubusercontent.com/31971762/179329871-ef7ae12d-0583-4ac6-b0d2-752132e961cd.png)

### Anything else

The `SecretsManagerBackend` uses the `BaseSecretsBackend` implementation of the `get_connection()` method. This method takes a `conn_id: str` and returns an `Optional[Connection]`.

The base implementation gets some `value` from the method `self.get_conn_value(conn_id=conn_id)`, then passes it to `self.deserialize_connection(conn_id=conn_id, value=value)`. This method accepts either a URL or a JSON (a string that starts with `{`).

However, the `get_conn_value()` method of the `SecretsManagerBackend` _never_ returns a JSON. Even if the value it retrieves from the cloud is a JSON, it will stick it into a URL at first.

At least as of Airflow 2.3.0, this is unnecessary as `get_conn_value()` is allowed to return a JSON, and the base implementation of get_conn_value`.

The Apache Airflow Amazon provider package supports Airflow 2.2. Supporting this version of Airflow is also possible, but is slightly more involved: the `SecretsManagerBackend` would need to override the base implementation for `get_connection` such that the JSON kwargs are passed into `Connection.__init__`. (Without overridding this method, only the `uri=?` kwarg is used to construct the URL).

### Are you willing to submit PR?

- [X] Yes I am willing to submit a PR!

### Code of Conduct

- [X] I agree to follow this project's [Code of Conduct](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.md)

--
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: commits-unsubscribe@airflow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] eladkal closed issue #25104: `SecretsManagerBackend()` does not need to require URL-encoded values for `full_url_mode=False` connections.

Posted by GitBox <gi...@apache.org>.

eladkal closed issue #25104: `SecretsManagerBackend()` does not need to require URL-encoded values for `full_url_mode=False` connections.
URL: https://github.com/apache/airflow/issues/25104


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] dwreeves commented on issue #25104: `SecretsManagerBackend()` does not need to require URL-encoded values for `full_url_mode=False` connections.

Posted by GitBox <gi...@apache.org>.

dwreeves commented on issue #25104:
URL: https://github.com/apache/airflow/issues/25104#issuecomment-1200460353

   Sorry for radio silence. I _should_ have a first pass at a PR done later today, so long as I don't uncover any bugs as I flesh out the test cases.
   
   It was tricky to maintain backwards compatibility, make the migration as smooth as possible for users, and make sure the changes can off-ramp into a more sane API down the line. But I think I got something. :)


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] eladkal commented on issue #25104: `SecretsManagerBackend()` does not need to require URL-encoded values for `full_url_mode=False` connections.

Posted by GitBox <gi...@apache.org>.

eladkal commented on issue #25104:
URL: https://github.com/apache/airflow/issues/25104#issuecomment-1214045177

   Fixed in https://github.com/apache/airflow/pull/25432


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #25104: `SecretsManagerBackend()` does not need to require URL-encoded values for `full_url_mode=False` connections.

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #25104:
URL: https://github.com/apache/airflow/issues/25104#issuecomment-1186104491

   Agree it would be nice to improve it - feel free to create PR on it when we can iterate on possble solutions. 
   
   I think it is important to keep backwards compatibility and do not go 2.3+. There must be a very good reason to break compatibility before the time, unless you want to wait till 11th October:
   
   https://github.com/apache/airflow#release-process-for-providers
   
   > For example this means that by default we upgrade the minimum version of Airflow supported by providers to 2.3.0 in the first Provider's release after 11th of October 2022 (11th of October 2021 is the date when the first PATCHLEVEL of 2.2 (2.2.0) has been released.
   
   Also I think there is another aspect here. We should make it in the way that:
   
   a) there is a parameter needed to determine if the parameters are url-encoded or not
   b) default should be true (but it should raise a deprecation warning).
   
   Otherwise we are risking that if someone configured it with URL-encoding (as cumbersome as it might see - you finally did it too) and if we just change the behaviour, we should not break experience of those people - otherwise upgrading AWS provider might mysteriously make their DAGs break.


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org