From 9cb824e7ee19c62db90340341bc68ee96bd7575e Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Fri, 31 May 2024 11:04:14 -0700 Subject: [PATCH 01/15] Remove grou_federation for serverless --- dbt/adapters/redshift/connections.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 752c81e32..7f11b15e9 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -234,10 +234,13 @@ def _iam_user_kwargs(self) -> Dict[str, Any]: def _iam_role_kwargs(self) -> Dict[str, Optional[Any]]: logger.debug("Connecting to redshift with 'iam_role' credentials method") kwargs = self._iam_kwargs - kwargs.update( - group_federation=True, - db_user=None, - ) + + # It's a role, we're ignoring the user + kwargs.update(db_user=None) + + # Serverless shouldn't get group_federation, Provisoned clusters should + if cluster_id := self.credentials.cluster_id: + kwargs.update(group_federation=True) if iam_profile := self.credentials.iam_profile: kwargs.update(profile=iam_profile) From 334b118ccb9b62bee09bf058ae76205fcc74f348 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Fri, 31 May 2024 11:36:30 -0700 Subject: [PATCH 02/15] Add changie log --- .changes/unreleased/Fixes-20240531-113620.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/unreleased/Fixes-20240531-113620.yaml diff --git a/.changes/unreleased/Fixes-20240531-113620.yaml b/.changes/unreleased/Fixes-20240531-113620.yaml new file mode 100644 index 000000000..e8d6aa7a0 --- /dev/null +++ b/.changes/unreleased/Fixes-20240531-113620.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Remove group_federation when using serverless for iam authentication +time: 2024-05-31T11:36:20.397521-07:00 +custom: + Author: fleid + Issue: "835" From 145b3cc285c1f2b9d2b58dc45b9e30c430acbd87 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Fri, 31 May 2024 14:20:00 -0700 Subject: [PATCH 03/15] Fix test on cluster_id --- dbt/adapters/redshift/connections.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 7f11b15e9..52b0397e3 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -239,7 +239,7 @@ def _iam_role_kwargs(self) -> Dict[str, Optional[Any]]: kwargs.update(db_user=None) # Serverless shouldn't get group_federation, Provisoned clusters should - if cluster_id := self.credentials.cluster_id: + if self.credentials.cluster_id: kwargs.update(group_federation=True) if iam_profile := self.credentials.iam_profile: From 00c53d6e5d1d4bb5433ea0fae4cebcb1fd7319a6 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Fri, 31 May 2024 14:21:25 -0700 Subject: [PATCH 04/15] Add explicit role_arn support --- dbt/adapters/redshift/connections.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 52b0397e3..4d9765948 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -124,6 +124,7 @@ class RedshiftCredentials(Credentials): autocommit: Optional[bool] = True access_key_id: Optional[str] = None secret_access_key: Optional[str] = None + role_arn: Optional[str] = None _ALIASES = {"dbname": "database", "pass": "password"} @@ -153,6 +154,7 @@ def _connection_keys(self): "retries", "autocommit", "access_key_id", + "role_arn", ) @property @@ -242,8 +244,19 @@ def _iam_role_kwargs(self) -> Dict[str, Optional[Any]]: if self.credentials.cluster_id: kwargs.update(group_federation=True) - if iam_profile := self.credentials.iam_profile: - kwargs.update(profile=iam_profile) + # Either we get the 3 required fields together, or we load an AWS iam profile + if self.credentials.access_key_id and self.credentials.secret_access_key and self.credentials.role_arn: + kwargs.update( + access_key_id=self.credentials.access_key_id, + secret_access_key=self.credentials.secret_access_key, + role_arn=self.credentials.role_arn + ) + elif self.credentials.access_key_id or self.credentials.secret_access_key or self.credentials.role_arn: + raise FailedToConnectError( + "'access_key_id', 'secret_access_key' and 'role_arn' are all needed if providing explicit credentials" + ) + else: + kwargs.update(profile=self.credentials.iam_profile) return kwargs From a3f5f3c223e6760bf6c4f572f90277d2fde7c58e Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 11:50:01 -0700 Subject: [PATCH 05/15] Revert "Add explicit role_arn support" This reverts commit 00c53d6e5d1d4bb5433ea0fae4cebcb1fd7319a6. --- dbt/adapters/redshift/connections.py | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 4d9765948..52b0397e3 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -124,7 +124,6 @@ class RedshiftCredentials(Credentials): autocommit: Optional[bool] = True access_key_id: Optional[str] = None secret_access_key: Optional[str] = None - role_arn: Optional[str] = None _ALIASES = {"dbname": "database", "pass": "password"} @@ -154,7 +153,6 @@ def _connection_keys(self): "retries", "autocommit", "access_key_id", - "role_arn", ) @property @@ -244,19 +242,8 @@ def _iam_role_kwargs(self) -> Dict[str, Optional[Any]]: if self.credentials.cluster_id: kwargs.update(group_federation=True) - # Either we get the 3 required fields together, or we load an AWS iam profile - if self.credentials.access_key_id and self.credentials.secret_access_key and self.credentials.role_arn: - kwargs.update( - access_key_id=self.credentials.access_key_id, - secret_access_key=self.credentials.secret_access_key, - role_arn=self.credentials.role_arn - ) - elif self.credentials.access_key_id or self.credentials.secret_access_key or self.credentials.role_arn: - raise FailedToConnectError( - "'access_key_id', 'secret_access_key' and 'role_arn' are all needed if providing explicit credentials" - ) - else: - kwargs.update(profile=self.credentials.iam_profile) + if iam_profile := self.credentials.iam_profile: + kwargs.update(profile=iam_profile) return kwargs From cd86483fa046fb4553a6f1842e6e5677734277de Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 12:43:33 -0700 Subject: [PATCH 06/15] Add tests --- tests/unit/test_auth_method.py | 88 ++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/tests/unit/test_auth_method.py b/tests/unit/test_auth_method.py index bd9912d0c..6d5a60d01 100644 --- a/tests/unit/test_auth_method.py +++ b/tests/unit/test_auth_method.py @@ -456,3 +456,91 @@ def test_profile(self): group_federation=True, **DEFAULT_SSL_CONFIG, ) + + +class TestIAMRoleMethodServerless(AuthMethod): + + @mock.patch("redshift_connector.connect", MagicMock()) + def test_profile_default_region(self): + self.config.credentials = self.config.credentials.replace( + method="iam_role", + cluster_id="my_redshift", + host="doesnotexist.1233.us-east-2.redshift-serverless.amazonaws.com", + ) + connection = self.adapter.acquire_connection("dummy") + connection.handle + redshift_connector.connect.assert_called_once_with( + iam=True, + host="doesnotexist.1233.us-east-2.redshift-serverless.amazonaws.com", + database="redshift", + cluster_identifier=None, + region=None, + auto_create=False, + db_groups=[], + db_user="", + password="", + user="", + profile="test", + timeout=None, + port=5439, + group_federation=False, + **DEFAULT_SSL_CONFIG, + ) + + @mock.patch("redshift_connector.connect", MagicMock()) + def test_profile_explicit_region(self): + # Successful test + self.config.credentials = self.config.credentials.replace( + method="iam", + iam_profile="test", + host="doesnotexist.1233.redshift-serverless.amazonaws.com", + region="us-east-2", + ) + connection = self.adapter.acquire_connection("dummy") + connection.handle + redshift_connector.connect.assert_called_once_with( + iam=True, + host="doesnotexist.1233.redshift-serverless.amazonaws.com", + database="redshift", + cluster_identifier=None, + region="us-east-2", + auto_create=False, + db_groups=[], + db_user="root", + password="", + user="", + profile="test", + timeout=None, + port=5439, + group_federation=False, + **DEFAULT_SSL_CONFIG, + ) + + @mock.patch("redshift_connector.connect", MagicMock()) + def test_profile_invalid_serverless(self): + self.config.credentials = self.config.credentials.replace( + method="iam", + iam_profile="test", + host="doesnotexist.1233.us-east-2.redshift-srvrlss.amazonaws.com", + ) + with self.assertRaises(FailedToConnectError) as context: + connection = self.adapter.acquire_connection("dummy") + connection.handle + redshift_connector.connect.assert_called_once_with( + iam=True, + host="doesnotexist.1233.us-east-2.redshift-srvrlss.amazonaws.com", + database="redshift", + cluster_identifier=None, + region=None, + auto_create=False, + db_groups=[], + db_user="root", + password="", + user="", + profile="test", + port=5439, + timeout=None, + group_federation=False, + **DEFAULT_SSL_CONFIG, + ) + self.assertTrue("'host' must be provided" in context.exception.msg) From 073b7b174ae100f2f73350961870798ac7e424fe Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 12:48:47 -0700 Subject: [PATCH 07/15] Linting --- dbt/adapters/redshift/connections.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 52b0397e3..2c47042da 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -237,7 +237,7 @@ def _iam_role_kwargs(self) -> Dict[str, Optional[Any]]: # It's a role, we're ignoring the user kwargs.update(db_user=None) - + # Serverless shouldn't get group_federation, Provisoned clusters should if self.credentials.cluster_id: kwargs.update(group_federation=True) From b696109f7cebec11affb509c6828af89886ae063 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 12:49:33 -0700 Subject: [PATCH 08/15] Linting --- tests/unit/test_auth_method.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_auth_method.py b/tests/unit/test_auth_method.py index 6d5a60d01..63e1103ed 100644 --- a/tests/unit/test_auth_method.py +++ b/tests/unit/test_auth_method.py @@ -459,7 +459,7 @@ def test_profile(self): class TestIAMRoleMethodServerless(AuthMethod): - + @mock.patch("redshift_connector.connect", MagicMock()) def test_profile_default_region(self): self.config.credentials = self.config.credentials.replace( From 9fe57e3b2910d8c2338851dd74c966fa2c5cf7c0 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 12:54:37 -0700 Subject: [PATCH 09/15] Tests should ignore group_federation, and not expect false --- tests/unit/test_auth_method.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_auth_method.py b/tests/unit/test_auth_method.py index 63e1103ed..97d59d60a 100644 --- a/tests/unit/test_auth_method.py +++ b/tests/unit/test_auth_method.py @@ -459,6 +459,7 @@ def test_profile(self): class TestIAMRoleMethodServerless(AuthMethod): + # Should behave like IAM Role provisioned, with the exception of not having group_federation set @mock.patch("redshift_connector.connect", MagicMock()) def test_profile_default_region(self): @@ -483,7 +484,7 @@ def test_profile_default_region(self): profile="test", timeout=None, port=5439, - group_federation=False, + # group_federation=False, **DEFAULT_SSL_CONFIG, ) @@ -512,7 +513,7 @@ def test_profile_explicit_region(self): profile="test", timeout=None, port=5439, - group_federation=False, + # group_federation=False, **DEFAULT_SSL_CONFIG, ) @@ -540,7 +541,7 @@ def test_profile_invalid_serverless(self): profile="test", port=5439, timeout=None, - group_federation=False, + # group_federation=False, **DEFAULT_SSL_CONFIG, ) self.assertTrue("'host' must be provided" in context.exception.msg) From 92744afec206baec730af808870db3fe9f26f681 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 13:15:54 -0700 Subject: [PATCH 10/15] Pick the correct method for the test --- tests/unit/test_auth_method.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_auth_method.py b/tests/unit/test_auth_method.py index 97d59d60a..19852b4a5 100644 --- a/tests/unit/test_auth_method.py +++ b/tests/unit/test_auth_method.py @@ -492,7 +492,7 @@ def test_profile_default_region(self): def test_profile_explicit_region(self): # Successful test self.config.credentials = self.config.credentials.replace( - method="iam", + method="iam_role", iam_profile="test", host="doesnotexist.1233.redshift-serverless.amazonaws.com", region="us-east-2", @@ -520,7 +520,7 @@ def test_profile_explicit_region(self): @mock.patch("redshift_connector.connect", MagicMock()) def test_profile_invalid_serverless(self): self.config.credentials = self.config.credentials.replace( - method="iam", + method="iam_role", iam_profile="test", host="doesnotexist.1233.us-east-2.redshift-srvrlss.amazonaws.com", ) From 0da52a8f3d4cdd2b101fbe6096f32460a0af6981 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 13:20:15 -0700 Subject: [PATCH 11/15] db_user should line up with IAMRole on Provisionned --- tests/unit/test_auth_method.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_auth_method.py b/tests/unit/test_auth_method.py index 19852b4a5..91178090a 100644 --- a/tests/unit/test_auth_method.py +++ b/tests/unit/test_auth_method.py @@ -478,7 +478,7 @@ def test_profile_default_region(self): region=None, auto_create=False, db_groups=[], - db_user="", + db_user=None, password="", user="", profile="test", @@ -507,7 +507,7 @@ def test_profile_explicit_region(self): region="us-east-2", auto_create=False, db_groups=[], - db_user="root", + db_user=None, password="", user="", profile="test", @@ -535,7 +535,7 @@ def test_profile_invalid_serverless(self): region=None, auto_create=False, db_groups=[], - db_user="root", + db_user=None, password="", user="", profile="test", From 552137d9ca99c03c4f4fffc7baedad53f8d72083 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 15:20:03 -0700 Subject: [PATCH 12/15] Align tests to spec --- tests/unit/test_auth_method.py | 46 +++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/tests/unit/test_auth_method.py b/tests/unit/test_auth_method.py index 91178090a..55b1aad74 100644 --- a/tests/unit/test_auth_method.py +++ b/tests/unit/test_auth_method.py @@ -465,7 +465,7 @@ class TestIAMRoleMethodServerless(AuthMethod): def test_profile_default_region(self): self.config.credentials = self.config.credentials.replace( method="iam_role", - cluster_id="my_redshift", + iam_profile="iam_profile_test", host="doesnotexist.1233.us-east-2.redshift-serverless.amazonaws.com", ) connection = self.adapter.acquire_connection("dummy") @@ -481,10 +481,38 @@ def test_profile_default_region(self): db_user=None, password="", user="", - profile="test", + profile="iam_profile_test", + timeout=None, + port=5439, + group_federation=False, + **DEFAULT_SSL_CONFIG, + ) + + @mock.patch("redshift_connector.connect", MagicMock()) + def test_profile_ignore_cluster(self): + self.config.credentials = self.config.credentials.replace( + method="iam_role", + iam_profile="iam_profile_test", + host="doesnotexist.1233.us-east-2.redshift-serverless.amazonaws.com", + cluster_id="my_redshift", + ) + connection = self.adapter.acquire_connection("dummy") + connection.handle + redshift_connector.connect.assert_called_once_with( + iam=True, + host="doesnotexist.1233.us-east-2.redshift-serverless.amazonaws.com", + database="redshift", + cluster_identifier=None, + region=None, + auto_create=False, + db_groups=[], + db_user=None, + password="", + user="", + profile="iam_profile_test", timeout=None, port=5439, - # group_federation=False, + group_federation=False, **DEFAULT_SSL_CONFIG, ) @@ -493,7 +521,7 @@ def test_profile_explicit_region(self): # Successful test self.config.credentials = self.config.credentials.replace( method="iam_role", - iam_profile="test", + iam_profile="iam_profile_test", host="doesnotexist.1233.redshift-serverless.amazonaws.com", region="us-east-2", ) @@ -510,10 +538,10 @@ def test_profile_explicit_region(self): db_user=None, password="", user="", - profile="test", + profile="iam_profile_test", timeout=None, port=5439, - # group_federation=False, + group_federation=False, **DEFAULT_SSL_CONFIG, ) @@ -521,7 +549,7 @@ def test_profile_explicit_region(self): def test_profile_invalid_serverless(self): self.config.credentials = self.config.credentials.replace( method="iam_role", - iam_profile="test", + iam_profile="iam_profile_test", host="doesnotexist.1233.us-east-2.redshift-srvrlss.amazonaws.com", ) with self.assertRaises(FailedToConnectError) as context: @@ -538,10 +566,10 @@ def test_profile_invalid_serverless(self): db_user=None, password="", user="", - profile="test", + profile="iam_profile_test", port=5439, timeout=None, - # group_federation=False, + group_federation=False, **DEFAULT_SSL_CONFIG, ) self.assertTrue("'host' must be provided" in context.exception.msg) From 5c4c636e2229c619af6e3a5f316edd56c73e0584 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 15:31:24 -0700 Subject: [PATCH 13/15] Explicitely set group federation for serverless --- dbt/adapters/redshift/connections.py | 4 +++- tests/unit/test_auth_method.py | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 2c47042da..566056d99 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -239,7 +239,9 @@ def _iam_role_kwargs(self) -> Dict[str, Optional[Any]]: kwargs.update(db_user=None) # Serverless shouldn't get group_federation, Provisoned clusters should - if self.credentials.cluster_id: + if "serverless" in self.credentials.host: + kwargs.update(group_federation=False) + else: kwargs.update(group_federation=True) if iam_profile := self.credentials.iam_profile: diff --git a/tests/unit/test_auth_method.py b/tests/unit/test_auth_method.py index 55b1aad74..38d78eb84 100644 --- a/tests/unit/test_auth_method.py +++ b/tests/unit/test_auth_method.py @@ -514,7 +514,6 @@ def test_profile_ignore_cluster(self): port=5439, group_federation=False, **DEFAULT_SSL_CONFIG, - ) @mock.patch("redshift_connector.connect", MagicMock()) def test_profile_explicit_region(self): From 6c07d464efd0e202d7ddd5de6d3a371e81628d0f Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Tue, 11 Jun 2024 15:44:16 -0700 Subject: [PATCH 14/15] Solved the cluster id issue - should be null on serverless --- dbt/adapters/redshift/connections.py | 6 +++--- tests/unit/test_auth_method.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/dbt/adapters/redshift/connections.py b/dbt/adapters/redshift/connections.py index 566056d99..916d2e00e 100644 --- a/dbt/adapters/redshift/connections.py +++ b/dbt/adapters/redshift/connections.py @@ -258,10 +258,10 @@ def _iam_kwargs(self) -> Dict[str, Any]: password="", ) - if cluster_id := self.credentials.cluster_id: - kwargs.update(cluster_identifier=cluster_id) - elif "serverless" in self.credentials.host: + if "serverless" in self.credentials.host: kwargs.update(cluster_identifier=None) + elif cluster_id := self.credentials.cluster_id: + kwargs.update(cluster_identifier=cluster_id) else: raise FailedToConnectError( "Failed to use IAM method:" diff --git a/tests/unit/test_auth_method.py b/tests/unit/test_auth_method.py index 38d78eb84..55b1aad74 100644 --- a/tests/unit/test_auth_method.py +++ b/tests/unit/test_auth_method.py @@ -514,6 +514,7 @@ def test_profile_ignore_cluster(self): port=5439, group_federation=False, **DEFAULT_SSL_CONFIG, + ) @mock.patch("redshift_connector.connect", MagicMock()) def test_profile_explicit_region(self): From 46dc6496d6fc7fd50c71a38fca64a41e5023eef1 Mon Sep 17 00:00:00 2001 From: Florian Eiden Date: Wed, 12 Jun 2024 14:12:57 -0700 Subject: [PATCH 15/15] Update changie desc --- .changes/unreleased/Fixes-20240531-113620.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/unreleased/Fixes-20240531-113620.yaml b/.changes/unreleased/Fixes-20240531-113620.yaml index e8d6aa7a0..a020ca045 100644 --- a/.changes/unreleased/Fixes-20240531-113620.yaml +++ b/.changes/unreleased/Fixes-20240531-113620.yaml @@ -1,5 +1,5 @@ kind: Fixes -body: Remove group_federation when using serverless for iam authentication +body: Support IAM Role authentication for Redshift Serverless time: 2024-05-31T11:36:20.397521-07:00 custom: Author: fleid