Add IAM authentication to Amazon Redshift Connection by AWS Connection #28187

IAL32 · 2022-12-07T09:54:27Z

Using same technique as already implemented into PostgreSQL Hook - manual obtain credentials by call GetClusterCredentials thought Redshift API.

airflow/airflow/providers/postgres/hooks/postgres.py

Lines 221 to 235 in 56b5f3f

    
           if conn.extra_dejson.get("redshift", False): 
        
               port = conn.port or 5439 
        
               # Pull the custer-identifier from the beginning of the Redshift URL 
        
               # ex. my-cluster.ccdre4hpd39h.us-east-1.redshift.amazonaws.com returns my-cluster 
        
               cluster_identifier = conn.extra_dejson.get("cluster-identifier", conn.host.split(".")[0]) 
        
               redshift_client = AwsBaseHook(aws_conn_id=aws_conn_id, client_type="redshift").conn 
        
               # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/redshift.html#Redshift.Client.get_cluster_credentials 
        
               cluster_creds = redshift_client.get_cluster_credentials( 
        
                   DbUser=login, 
        
                   DbName=self.database or conn.schema, 
        
                   ClusterIdentifier=cluster_identifier, 
        
                   AutoCreate=False, 
        
               ) 
        
               token = cluster_creds["DbPassword"] 
        
               login = cluster_creds["DbUser"]

airflow/providers/amazon/aws/hooks/redshift_sql.py

Taragolis · 2022-12-07T19:20:32Z

Could you also update documentation, you could have a look in BREEZE.rst how to build and check documentation locally

Taragolis

Add some findings. Personally I'm do not work with Redshift and redshift-connector for almost 1.5 year.
So might someone who work on daily basis could review more detailed

Taragolis · 2022-12-08T10:03:02Z

airflow/providers/amazon/aws/hooks/redshift_sql.py

@@ -44,6 +58,10 @@ class RedshiftSQLHook(DbApiHook):
    hook_name = "Amazon Redshift"
    supports_autocommit = True

+    def __init__(self, *args, aws_conn_id: str = "aws_default", **kwargs) -> None:


aws_conn_id might be None, in this case default boto3 strategy will use (usual EC2 Instance Profile or ECS Execution Role)

Taragolis · 2022-12-08T10:08:45Z

airflow/providers/amazon/aws/hooks/redshift_sql.py

@@ -62,6 +80,9 @@ def _get_conn_params(self) -> dict[str, str | int]:

        conn_params: dict[str, str | int] = {}

+        if conn.extra_dejson.get("iam", False):


I'm worry that redshift-connector also might use same connection parameter, see list available parameters: https://github.com/aws/amazon-redshift-python-driver#connection-parameters

As result it might broke someone connections

@IAL32 thoughts on this?

I am not sure if this would break any connections. @o-nikolas I will test this week and come back with findings.

IAL32 · 2022-12-08T20:58:34Z

I would also like some feedback from someone with more experience with RedShift. I don't use it myself very often either

Taragolis · 2022-12-09T22:36:15Z

@shubham22 a chance that you know who might help with review extending integration Redshift Connection and redshift-connector?

eladkal · 2023-01-09T08:32:52Z

@vincbeck @ferruzzi @o-nikolas @shubham22 can you please take a look?

vincbeck · 2023-01-09T15:56:01Z

I am not a Redshift expert myself so I dont really want to approve blindly. It is probably worth trying to find someone who's familiar with Redshift. Maybe @shubham22 can help us?

ferruzzi · 2023-01-09T21:23:42Z

I'm also definitely not a Redshift expert, but nothing there jumps out as a problem to me.

shubham22 · 2023-02-07T04:21:44Z

Apologies for the delayed response -- I subscribed to all Airflow GitHub discussions in the hope that I will be able to follow them, but ended up drowning my direct mentions.

I will see if I can find a Redshift expert to chime on this one by early next week.

github-actions · 2023-03-25T00:11:14Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

potiuk · 2023-04-29T15:35:15Z

I rebased this one, it looks cool, but maybe someone from teh AWS team can comment on it ?

cjames23 · 2023-04-29T23:42:30Z

@o-nikolas @shubham22 @eladkal so far testing against a redshift cluster with other Redshift connections not using IAM I am not seeing anything break connections wise. It is just setting the iam to false when it is not passed in as extra args which would then indicate the connection expects credentials in the connection object in airflow rather than getting them through IAM. It might be worth having a unit test that involves both a non IAM and IAM based connection created and checking to ensure the arg is not overwritten to test that case. Let me know if you want me to test further and see if I can find any edge cases where it does break.

@potiuk I was doing some testing for the AWS folks as someone with a bit deeper knowledge of Redshift. I work at Amazon and have a working relationship with those tagged above.

potiuk

LGTM - but would love someone with AWS hat on to confirm it looks cool :)

eladkal · 2023-05-02T12:40:47Z

LGTM

vincbeck

It looks good to me even though this is not an area I am an expert in. If testing has been done and it works, the code looks good to me

Since there was a recent change introduced on OSS - apache/airflow#28187 we are calling the `get_iam_token` method even for the default redshift connection which doesn't have a host by default, therefore failing in CI. In this PR we are overriding the default connection - `redshift_conn` with `iam: false` so that the `get_iam_token` is not called. Once this issue if fixed in the OSS Airflow we can remove this change.

IAL32 requested a review from eladkal as a code owner December 7, 2022 09:54

boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Dec 7, 2022

Taragolis reviewed Dec 7, 2022

View reviewed changes

airflow/providers/amazon/aws/hooks/redshift_sql.py Outdated Show resolved Hide resolved

Taragolis reviewed Dec 8, 2022

View reviewed changes

o-nikolas self-requested a review as a code owner January 25, 2023 06:02

github-actions bot added stale Stale PRs per the .github/workflows/stale.yml policy file needs-triage label for new issues that we didn't triage yet and removed stale Stale PRs per the .github/workflows/stale.yml policy file labels Mar 25, 2023

eladkal removed the needs-triage label for new issues that we didn't triage yet label Mar 27, 2023

eladkal mentioned this pull request Mar 27, 2023

adjust stale bots #30321

Merged

IAL32 added 3 commits April 29, 2023 17:34

Add IAM authentication to Amazon Redshift Connection by AWS Connection

5e84020

Fix type checking

b80adce

Fixed documentation

a238355

potiuk force-pushed the ac/28000-iam-redshift branch from 0568cc0 to a238355 Compare April 29, 2023 15:34

potiuk approved these changes May 2, 2023

View reviewed changes

eladkal approved these changes May 2, 2023

View reviewed changes

vincbeck approved these changes May 2, 2023

View reviewed changes

potiuk merged commit 2f247a2 into apache:main May 2, 2023

IAL32 deleted the ac/28000-iam-redshift branch May 2, 2023 13:59

eladkal mentioned this pull request May 16, 2023

Status of testing Providers that were prepared on May 19, 2023 #31322

Closed

80 tasks

utkarsharma2 mentioned this pull request May 25, 2023

Override redshift default conn astronomer/astro-sdk#1949

Merged

This was referenced May 25, 2023

Redshift connection breaking change with IAM authentication #31551

Closed

Add null check for host in Amazon Redshift connection #31567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IAM authentication to Amazon Redshift Connection by AWS Connection #28187

Add IAM authentication to Amazon Redshift Connection by AWS Connection #28187

IAL32 commented Dec 7, 2022

Taragolis commented Dec 7, 2022

Taragolis left a comment

Taragolis Dec 8, 2022

Taragolis Dec 8, 2022

o-nikolas Jan 25, 2023

cjames23 Apr 27, 2023

IAL32 commented Dec 8, 2022

Taragolis commented Dec 9, 2022

eladkal commented Jan 9, 2023

vincbeck commented Jan 9, 2023

ferruzzi commented Jan 9, 2023

shubham22 commented Feb 7, 2023

github-actions bot commented Mar 25, 2023

potiuk commented Apr 29, 2023

cjames23 commented Apr 29, 2023

potiuk left a comment

eladkal commented May 2, 2023 •

edited

Loading

vincbeck left a comment

	if conn.extra_dejson.get("redshift", False):
	port = conn.port or 5439
	# Pull the custer-identifier from the beginning of the Redshift URL
	# ex. my-cluster.ccdre4hpd39h.us-east-1.redshift.amazonaws.com returns my-cluster
	cluster_identifier = conn.extra_dejson.get("cluster-identifier", conn.host.split(".")[0])
	redshift_client = AwsBaseHook(aws_conn_id=aws_conn_id, client_type="redshift").conn
	# https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/redshift.html#Redshift.Client.get_cluster_credentials
	cluster_creds = redshift_client.get_cluster_credentials(
	DbUser=login,
	DbName=self.database or conn.schema,
	ClusterIdentifier=cluster_identifier,
	AutoCreate=False,
	)
	token = cluster_creds["DbPassword"]
	login = cluster_creds["DbUser"]

		@@ -62,6 +80,9 @@ def _get_conn_params(self) -> dict[str, str \| int]:

		conn_params: dict[str, str \| int] = {}

		if conn.extra_dejson.get("iam", False):

Add IAM authentication to Amazon Redshift Connection by AWS Connection #28187

Add IAM authentication to Amazon Redshift Connection by AWS Connection #28187

Conversation

IAL32 commented Dec 7, 2022

Taragolis commented Dec 7, 2022

Taragolis left a comment

Choose a reason for hiding this comment

Taragolis Dec 8, 2022

Choose a reason for hiding this comment

Taragolis Dec 8, 2022

Choose a reason for hiding this comment

o-nikolas Jan 25, 2023

Choose a reason for hiding this comment

cjames23 Apr 27, 2023

Choose a reason for hiding this comment

IAL32 commented Dec 8, 2022

Taragolis commented Dec 9, 2022

eladkal commented Jan 9, 2023

vincbeck commented Jan 9, 2023

ferruzzi commented Jan 9, 2023

shubham22 commented Feb 7, 2023

github-actions bot commented Mar 25, 2023

potiuk commented Apr 29, 2023

cjames23 commented Apr 29, 2023

potiuk left a comment

Choose a reason for hiding this comment

eladkal commented May 2, 2023 • edited Loading

vincbeck left a comment

Choose a reason for hiding this comment

eladkal commented May 2, 2023 •

edited

Loading