Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

httpx breaking API change for redirect parameter causes crash #20088

Closed
2 tasks done
philipherrmann opened this issue Dec 6, 2021 · 11 comments
Closed
2 tasks done

httpx breaking API change for redirect parameter causes crash #20088

philipherrmann opened this issue Dec 6, 2021 · 11 comments
Assignees
Labels
area:providers kind:bug This is a clearly a bug

Comments

@philipherrmann
Copy link
Contributor

Apache Airflow Provider(s)

google

Versions of Apache Airflow Providers

apache-airflow-providers-google==6.1.0

Apache Airflow version

2.2.2 (latest released)

Operating System

Debian GNU/Linux 10 (buster)

Deployment

Official Apache Airflow Helm Chart

Deployment details

No response

What happened

When using the CloudSQLExecuteQueryOperator the Cloud SQL Proxy Download fails due to a breaking API change in the httpx dependency, logging the following traceback:

Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.9/site-packages/airflow/providers/google/cloud/operators/cloud_sql.py", line 1056, in _execute_query
    cloud_sql_proxy_runner.start_proxy()
  File "/home/airflow/.local/lib/python3.9/site-packages/airflow/providers/google/cloud/hooks/cloud_sql.py", line 558, in start_proxy
    self._download_sql_proxy_if_needed()
  File "/home/airflow/.local/lib/python3.9/site-packages/airflow/providers/google/cloud/hooks/cloud_sql.py", line 502, in _download_sql_proxy_if_needed
    response = httpx.get(download_url, allow_redirects=True)
TypeError: get() got an unexpected keyword argument 'allow_redirects'

The keyword allow_redirects has been renamed to follow_redirects in this PR encode/httpx#1808. One should probably fix either of those options and pin a suitable version of httpx.

What you expected to happen

No response

How to reproduce

No response

Anything else

task = CloudSQLExecuteQueryOperator(
    task_id="test",
    gcp_cloudsql_conn_id="postgres-google-cloud-sql-connection",
    trigger_rule=TriggerRule.ALL_SUCCESS,
    sql="""
        SELECT %s
    """,
    parameters=["test"],
)

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@philipherrmann philipherrmann added area:providers kind:bug This is a clearly a bug labels Dec 6, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Dec 6, 2021

Thanks for opening your first issue here! Be sure to follow the issue template!

@mik-laj
Copy link
Member

mik-laj commented Dec 7, 2021

@philipherrmann Assigned

@uranusjr
Copy link
Member

uranusjr commented Dec 7, 2021

Note that when you fix this, you’ll need to also change httpx in setup.cfg to httpx>=0.20. (Or if that’s not viable due to maybe providers need the old argument name, pin setup.cfg to httpx<0.20 for now and don’t change the argument.)

@potiuk
Copy link
Member

potiuk commented Dec 7, 2021

Really "nice" from httpx POV to make this breaking change here :(. httpx >=0.20 in this case is a good idea.
I will also manually update constraints to 0.19 for the past releases of airflow that had 0.20.

@potiuk
Copy link
Member

potiuk commented Dec 7, 2021

This is what one of the nice properties of the constraint mechanism of ours, that we can fix such problems.

potiuk added a commit to potiuk/airflow that referenced this issue Dec 7, 2021
Following apache#20088 we
are downgrading httpx to be 0.19.0 as 0.20.0 introduced a
breaking change.
potiuk added a commit that referenced this issue Dec 7, 2021
Following #20088 we
are downgrading httpx to be 0.19.0 as 0.20.0 introduced a
breaking change.
@potiuk
Copy link
Member

potiuk commented Dec 7, 2021

Ok. It was only for 2.2.2, The 2.2.1 version already had httpx == 0.19.0. I fixed the constraints now, so anyone installing airfllow in the "constraint" way will not have the problem. I am also building and pushing the images for 2.2.2 to include that.

@philipherrmann
Copy link
Contributor Author

So the quick fix is in place for the current version (2.2.2) of airflow. Would you still like me to create a PR for use in future versions of airflow, which

  1. changes the parameter name to follow_redirects in httpx calls and
  2. maybe even changes the dependency constraint to httpx~=0.21.1?

No worries if you'd prefer to do that yourself, I have a bunch things to do :) Otherwise, I would give it a try next weekend probably.

@uranusjr
Copy link
Member

uranusjr commented Dec 8, 2021

If you change the parameter name, you must change the constraint. Otherwise sure, give it a go.

potiuk added a commit to PolideaInternal/airflow that referenced this issue Dec 11, 2021
This one limits httpx to <0.20.0 in order to handle breaaking
change for CloudSQL provider.

apache#20088

We do not add it in main /2.3 because we will fix the problem
in main and release new provider then.
potiuk added a commit to potiuk/airflow that referenced this issue Dec 11, 2021
This one limits httpx to <0.20.0 in order to handle breaaking
change for CloudSQL provider.

apache#20088

We do not add it in main /2.3 because we will fix the problem
in main and release new provider then.
potiuk added a commit that referenced this issue Dec 11, 2021
This one limits httpx to <0.20.0 in order to handle breaaking
change for CloudSQL provider.

#20088

We do not add it in main /2.3 because we will fix the problem
in main and release new provider then.
@philipherrmann
Copy link
Contributor Author

If you change the parameter name, you must change the constraint. Otherwise sure, give it a go.
Surely, someone must change the constraints. I meant to offer that, but as they are in this extra branches I am slightly confused. Impossible to get those two contributions into the same PR, isn't it?

@potiuk
Copy link
Member

potiuk commented Dec 14, 2021

If you change the parameter name, you must change the constraint. Otherwise sure, give it a go.

It's OK what you did - changing setup.* is the right way to go. Constraints will update automatically

@eladkal
Copy link
Contributor

eladkal commented Jan 3, 2022

fixed by #20239

@eladkal eladkal closed this as completed Jan 3, 2022
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Mar 10, 2022
This one limits httpx to <0.20.0 in order to handle breaaking
change for CloudSQL provider.

apache/airflow#20088

We do not add it in main /2.3 because we will fix the problem
in main and release new provider then.

GitOrigin-RevId: 1d9f2aa42e982bfbe8d1bc5227c984f4aa5d6277
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this issue Jun 4, 2022
This one limits httpx to <0.20.0 in order to handle breaaking
change for CloudSQL provider.

apache/airflow#20088

We do not add it in main /2.3 because we will fix the problem
in main and release new provider then.

GitOrigin-RevId: 1d9f2aa42e982bfbe8d1bc5227c984f4aa5d6277
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers kind:bug This is a clearly a bug
Projects
None yet
Development

No branches or pull requests

5 participants