You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/07 09:54:29 UTC
[GitHub] [airflow] IAL32 opened a new pull request, #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
IAL32 opened a new pull request, #28187:
URL: https://github.com/apache/airflow/pull/28187
closes: https://github.com/apache/airflow/issues/28000
Using same technique as already implemented into PostgreSQL Hook - manual obtain credentials by call [GetClusterCredentials](https://docs.aws.amazon.com/redshift/latest/APIReference/API_GetClusterCredentials.html) thought Redshift API.
https://github.com/apache/airflow/blob/56b5f3f4eed6a48180e9d15ba9bb9664656077b1/airflow/providers/postgres/hooks/postgres.py#L221-L235
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] shubham22 commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by "shubham22 (via GitHub)" <gi...@apache.org>.
shubham22 commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1420180247
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] cjames23 commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by "cjames23 (via GitHub)" <gi...@apache.org>.
cjames23 commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1528897547
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1341474782
Could you also update [documentation](https://github.com/apache/airflow/blob/main/docs/apache-airflow-providers-amazon/connections/redshift.rst), you could have a look in [BREEZE.rst](https://github.com/apache/airflow/blob/main/BREEZE.rst#building-the-documentation) how to build and check documentation locally
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1528813989
I rebased this one, it looks cool, but maybe someone from teh AWS team can comment on it ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1376337474
I'm also definitely not a Redshift expert, but nothing there jumps out as a problem to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28187:
URL: https://github.com/apache/airflow/pull/28187#discussion_r1042589864
##########
airflow/providers/amazon/aws/hooks/redshift_sql.py:
##########
@@ -22,6 +22,8 @@
from sqlalchemy.engine.url import URL
from airflow.compat.functools import cached_property
+from airflow.models.connection import Connection
Review Comment:
Me use Connection only for annotation so we could specify that it uses only for it
https://github.com/apache/airflow/blob/5802469fbc452a5727c938f033f2753571989d92/airflow/providers/amazon/aws/hooks/base_aws.py#L56-L59
In this module there is small chance that we got circular import error but still better to use it inside `if TYPE_CHECKING` block
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by GitBox <gi...@apache.org>.
vincbeck commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1375848749
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1344853490
@shubham22 a chance that you know who might help with review extending integration Redshift Connection and `redshift-connector`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] IAL32 commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by GitBox <gi...@apache.org>.
IAL32 commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1343343884
I would also like some feedback from someone with more experience with RedShift. I don't use it myself very often either
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #28187:
URL: https://github.com/apache/airflow/pull/28187
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #28187:
URL: https://github.com/apache/airflow/pull/28187#discussion_r1086234925
##########
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):
Review Comment:
@IAL32 thoughts on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1531408544
@cjames23 did your test had any findings?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1375264996
@vincbeck @ferruzzi @o-nikolas @shubham22 can you please take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28187:
URL: https://github.com/apache/airflow/pull/28187#discussion_r1043159197
##########
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:
Review Comment:
`aws_conn_id` might be None, in this case default `boto3` strategy will use (usual EC2 Instance Profile or ECS Execution Role)
##########
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):
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] cjames23 commented on a diff in pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by "cjames23 (via GitHub)" <gi...@apache.org>.
cjames23 commented on code in PR #28187:
URL: https://github.com/apache/airflow/pull/28187#discussion_r1179532001
##########
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):
Review Comment:
I am not sure if this would break any connections. @o-nikolas I will test this week and come back with findings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #28187: Add IAM authentication to Amazon Redshift Connection by AWS Connection
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28187:
URL: https://github.com/apache/airflow/pull/28187#issuecomment-1483617562
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org