Skip to content

Conversation

@cccs-cat001
Copy link
Contributor

Adds back azure postgres connection info that got broke in the following commit: af5eb55

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@mik-laj
Copy link
Member

mik-laj commented Mar 31, 2021

What version of Airflow are you using? This code should not run on Airflow 1.10.14 and later.

if ! airflow db check --help >/dev/null 2>&1; then

Maybe we should add support for the airflow checkdb command to support older Airflow versions as well?
http://airflow.apache.org/docs/apache-airflow/1.10.13/cli-ref.html#checkdb
This command was added in Airflow 1.10.8, so it's a bit of a mature version.

I would like to eliminate the source of the problem, so that it would not be necessary to make further improvements to this regular expression.

@cccs-cat001
Copy link
Contributor Author

cccs-cat001 commented Mar 31, 2021

What version of Airflow are you using? This code should not run on Airflow 1.10.14 and later.

if ! airflow db check --help >/dev/null 2>&1; then

Maybe we should add support for the airflow checkdb command to support older Airflow versions as well?
http://airflow.apache.org/docs/apache-airflow/1.10.13/cli-ref.html#checkdb
This command was added in Airflow 1.10.8, so it's a bit of a mature version.

I would like to eliminate the source of the problem, so that it would not be necessary to make further improvements to this regular expression.

I'm using airflow 2.0.1. without this change, the entrypoint fails saying BACKEND is not defined.

@cccs-cat001
Copy link
Contributor Author

in the airflow:2.0.1-python3.8 image I'm using, this line you're quoting doesn't exist, it always runs the wait_for_connection function

@mik-laj
Copy link
Member

mik-laj commented Mar 31, 2021

@cccs-cat001 it is a new feature added by me. Can you copy this file to your image to check if this patch meets your requirements?

@cccs-cat001
Copy link
Contributor Author

Yeah I just ran with that version of the file. The airflow db check doesn't run, it goes straight to the wait_for_connection.

@potiuk
Copy link
Member

potiuk commented Apr 5, 2021

I looked at it again, and this PR proposal is actually wrong. The URI you are trying to use is not correct according to URI specfication.

We actually even have a detailed description about this in http://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html?highlight=uri#handling-of-special-characters-in-connection-params and we even described how to generate the connection URI properly: http://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html?highlight=uri#generating-connection-uri

You are not supposed to have @ character in the user part of the URI. This is strictly forbidden in the https://tools.ietf.org/html/rfc3986 and if we allow that, it leads to a number of ambiguities.

We MUST strictly follow the specification rather than bypass it.

Read the "Reserved Characters" part of the specification. Those are the types of characters we have:

 unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
 gen-delims  = ":" / "/" / "?" / "#" / "[" / "]" / "@"
 sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
                 / "*" / "+" / "," / ";" / "="

Follow it further and you will see that while the user information can contain sub-delims, it
MUST NOT contain gen-delims characters. If they are needed, they must be percent-encoded:

userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )

This means that in your case, if you want to use user name containing the @ character (which is part of gen-delims) you must percent-encode it. Your user info part should look like this:

YourUserName%40YourDatabaseName:

I am closing that one as invalid - but it would be great @cccs-cat001 if you verify that the percent-encoding helps to address your problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants