Skip to content

Conversation

@sunank200
Copy link
Collaborator

@sunank200 sunank200 commented May 26, 2023

This is a bug in the current implementation as per the PR.

When user passes the following as part of redshift connection, it would break:

{
  "iam": true,
  "cluster_identifier": "redshift-cluster-1",
  "port": 5439,
  "region": "us-east-1",
  "db_user": "awsuser",
  "database": "dev",
  "profile": "default"
}

The following image shows that the host is not set in the connection but cluster_identifier is set.
Screenshot 2023-05-26 at 3 56 58 PM

Even though the cluster_identifier is set here this would break with the following error on the line as host is not set because conn.host is evaluated first and when we split it to NoneType object it would throw attribute error.

conn.extra_dejson.get("cluster_identifier", conn.host.split(".")[0])

Screenshot 2023-05-26 at 3 58 00 PM

Following is the test we did.

>>> conn.extra_dejson.get("cluster_identifier")
PyDev console: starting.
'<REDSHIFT_CLUSTER_IDENTIFIER>'
>>> conn.extra_dejson.get("cluster_identifier", None)
'<REDSHIFT_CLUSTER_IDENTIFIER>'
>>> conn.host

>>> conn.extra_dejson.get("cluster_identifier", conn.host.split(".")[0])
Traceback (most recent call last):
  File "/Applications/PyCharm.app/Contents/plugins/python/helpers/pydev/_pydevd_bundle/pydevd_exec2.py", line 3, in Exec
    exec(exp, global_vars, local_vars)
  File "<input>", line 1, in <module>
AttributeError: 'NoneType' object has no attribute 'split'

This is a bug in the current implementation for all users when host is not set in the connection (not only for default connections). This would require a null check for conn.host.

closes: #31551
Added a null check for conn.host. conn.host.split(".")[0] is compiled regardless of the extra values.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels May 26, 2023
@pankajkoti pankajkoti requested a review from hussein-awala May 26, 2023 11:24
Copy link
Contributor

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

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

can you add tests for this change please

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Could you add a test for this fix to test the different cases (loading from cluster_identifier, loading from host and raising an exception if both are not preset)?

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Tests please!

@sunank200
Copy link
Collaborator Author

sunank200 commented May 26, 2023

Tests please!

@phanikumv @ashb @hussein-awala added the test cases here: 4e643c3

@sunank200 sunank200 requested a review from dstandish May 29, 2023 09:12
@sunank200 sunank200 requested a review from uranusjr May 29, 2023 09:29
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

I just pushed a commit to simplify the tests, you can change it if you think that there is something missing

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

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redshift connection breaking change with IAM authentication

10 participants