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

Support IAM authentication for Redshift serverless #35897

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

hussein-awala
Copy link
Member

This PR adds some extra parameters to Redshift connection to support IAM authentication for AWS Redshift Serverless. When is_serverless extra is set to True, it uses redshift-serverless client instead of redshift, and provides the same param to redshift_connector.connect to initiate serverless connection.

closes: #35805

Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just small nit

Comment on lines 84 to 86
conn.login, conn.password, conn.port = self.get_iam_token(conn)
conn.login, conn.password, conn.port = self.get_iam_token(
conn, is_serverless=conn.extra_dejson.get("is_serverless", False)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we provide entire Connection object into the get_iam_token method, I guess we do not need to use is_serverless argument and could resolve it into the get_iam_token

Comment on lines +113 to +115
serverless_token_duration_seconds = conn.extra_dejson.get(
"serverless_token_duration_seconds", 3600
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This parts also makes me sad, there is no info into the documentation about this parameter, so end users might know about this settings only if they open the source code 😿

Copy link
Contributor

Choose a reason for hiding this comment

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

But it a general problem, how to not forget to document some parameters from the abstract Connection Extra

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add it to the connection example, but I agree with the need for a general solution. I have some ideas but don't know if they are functional; I will try some of them and then open a discussion.

@hussein-awala hussein-awala merged commit f6962a9 into apache:main Nov 28, 2023
47 checks passed
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.

RedshiftSQLHook does not work with iam=True
4 participants