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 2021/07/02 08:56:19 UTC
[GitHub] [airflow] baolsen opened a new pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
baolsen opened a new pull request #16771:
URL: https://github.com/apache/airflow/pull/16771
Update AWS Base hook to use refreshable credentials when doing assume_role or similar (#16770)
TODO: Unit tests.
I've already tested in my proper Airflow environment and it works as expected.
I'm submitting this to CI so long so I can find issues more easily.
closes: #16770
--
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] mik-laj commented on a change in pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#discussion_r663051792
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -97,57 +99,61 @@ def _create_basic_session(self, session_kwargs: Dict[str, Any]) -> boto3.session
**session_kwargs,
)
- def _impersonate_to_role(
- self, role_arn: str, session: boto3.session.Session, session_kwargs: Dict[str, Any]
- ) -> boto3.session.Session:
- assume_role_kwargs = self.extra_config.get("assume_role_kwargs", {})
- assume_role_method = self.extra_config.get('assume_role_method')
+ def _create_session_with_assume_role(self, session_kwargs: Dict[str, Any]) -> boto3.session.Session:
+ assume_role_method = self.extra_config.get('assume_role_method', 'assume_role')
self.log.info("assume_role_method=%s", assume_role_method)
- if not assume_role_method or assume_role_method == 'assume_role':
- sts_client = session.client("sts", config=self.config)
- sts_response = self._assume_role(
- sts_client=sts_client, role_arn=role_arn, assume_role_kwargs=assume_role_kwargs
- )
- elif assume_role_method == 'assume_role_with_saml':
- sts_client = session.client("sts", config=self.config)
- sts_response = self._assume_role_with_saml(
- sts_client=sts_client, role_arn=role_arn, assume_role_kwargs=assume_role_kwargs
- )
- elif assume_role_method == 'assume_role_with_web_identity':
- botocore_session = self._assume_role_with_web_identity(
- role_arn=role_arn,
- assume_role_kwargs=assume_role_kwargs,
- base_session=session._session,
- )
- return boto3.session.Session(
- region_name=session.region_name,
- botocore_session=botocore_session,
- **session_kwargs,
- )
- else:
+ supported_methods = ['assume_role', 'assume_role_with_saml', 'assume_role_with_web_identity']
+ if assume_role_method not in supported_methods:
raise NotImplementedError(
f'assume_role_method={assume_role_method} in Connection {self.conn.conn_id} Extra.'
- 'Currently "assume_role" or "assume_role_with_saml" are supported.'
+ f'Currently {supported_methods} are supported.'
'(Exclude this setting will default to "assume_role").'
)
- # Use credentials retrieved from STS
- credentials = sts_response["Credentials"]
- aws_access_key_id = credentials["AccessKeyId"]
- aws_secret_access_key = credentials["SecretAccessKey"]
- aws_session_token = credentials["SessionToken"]
- self.log.info(
- "Creating session with aws_access_key_id=%s region_name=%s",
- aws_access_key_id,
- session.region_name,
- )
-
- return boto3.session.Session(
- aws_access_key_id=aws_access_key_id,
- aws_secret_access_key=aws_secret_access_key,
- region_name=session.region_name,
- aws_session_token=aws_session_token,
- **session_kwargs,
- )
+ if assume_role_method == 'assume_role_with_web_identity':
+ # Deferred credentials have no initial credentials
+ credential_fetcher = self._get_web_identity_credential_fetcher()
+ credentials = DeferredRefreshableCredentials(
+ method='assume-role-with-web-identity',
+ refresh_using=credential_fetcher.fetch_credentials,
+ time_fetcher=lambda: datetime.datetime.now(tz=tzlocal()),
+ )
+ else:
+ # Refreshable credentials do have initial credentials
+ credentials = RefreshableCredentials.create_from_metadata(
+ metadata=self._refresh_credentials(),
+ refresh_using=self._refresh_credentials,
+ method="sts-assume-role",
+ )
+ session = botocore.session.get_session()
+ session._credentials = credentials # pylint: disable=protected-access
+ region_name = self.basic_session.region_name
+ session.set_config_variable("region", region_name)
+ return boto3.session.Session(botocore_session=session, **session_kwargs)
+
+ def _refresh_credentials(self) -> boto3.session.Session:
Review comment:
I have a feeling this function returns a dictionary not `boto3.session.Session`. Am I right?
--
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] mik-laj commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874559256
> Second nitpick, I believe the three commits need to be squashed, or perhaps I misunderstood when I read that somewhere. Kamil or Jarek can confirm or refute that.
This is not needed and I do not recommend doing this as several commits make reviews easier. Commiters do this automatically when merging PRs.
--
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] baolsen commented on a change in pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#discussion_r664251182
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -289,8 +297,8 @@ def _fetch_saml_assertion_using_http_spegno_auth(self, saml_config: Dict[str, An
raise ValueError('Invalid SAML Assertion')
return saml_assertion
- def _assume_role_with_web_identity(self, role_arn, assume_role_kwargs, base_session):
- base_session = base_session or botocore.session.get_session()
+ def _get_web_identity_credential_fetcher(self):
Review comment:
Done, thanks for the feedback.
--
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] mik-laj merged pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #16771:
URL: https://github.com/apache/airflow/pull/16771
--
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] baolsen commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-873867207
Added unit test.
--
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 #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-873408162
Yep. nice one, as you mentioned @baolsen -> unit test are needed.
--
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 a change in pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#discussion_r664196614
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -289,8 +297,8 @@ def _fetch_saml_assertion_using_http_spegno_auth(self, saml_config: Dict[str, An
raise ValueError('Invalid SAML Assertion')
return saml_assertion
- def _assume_role_with_web_identity(self, role_arn, assume_role_kwargs, base_session):
- base_session = base_session or botocore.session.get_session()
+ def _get_web_identity_credential_fetcher(self):
Review comment:
I know this didn't have one previously, but could you drop a return type on the signature? I think it should be `botocore.credentials`?
--
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] baolsen commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874510901
CI sorted, ready to merge I think... Have pinged reviewer.
--
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 a change in pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#discussion_r664196614
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -289,8 +297,8 @@ def _fetch_saml_assertion_using_http_spegno_auth(self, saml_config: Dict[str, An
raise ValueError('Invalid SAML Assertion')
return saml_assertion
- def _assume_role_with_web_identity(self, role_arn, assume_role_kwargs, base_session):
- base_session = base_session or botocore.session.get_session()
+ def _get_web_identity_credential_fetcher(self):
Review comment:
I know this didn't have one previously, but could you drop a return type on the signature? I think it should be `botocore.credentials`?
--
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] baolsen commented on a change in pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#discussion_r663659851
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -97,57 +99,61 @@ def _create_basic_session(self, session_kwargs: Dict[str, Any]) -> boto3.session
**session_kwargs,
)
- def _impersonate_to_role(
- self, role_arn: str, session: boto3.session.Session, session_kwargs: Dict[str, Any]
- ) -> boto3.session.Session:
- assume_role_kwargs = self.extra_config.get("assume_role_kwargs", {})
- assume_role_method = self.extra_config.get('assume_role_method')
+ def _create_session_with_assume_role(self, session_kwargs: Dict[str, Any]) -> boto3.session.Session:
+ assume_role_method = self.extra_config.get('assume_role_method', 'assume_role')
self.log.info("assume_role_method=%s", assume_role_method)
- if not assume_role_method or assume_role_method == 'assume_role':
- sts_client = session.client("sts", config=self.config)
- sts_response = self._assume_role(
- sts_client=sts_client, role_arn=role_arn, assume_role_kwargs=assume_role_kwargs
- )
- elif assume_role_method == 'assume_role_with_saml':
- sts_client = session.client("sts", config=self.config)
- sts_response = self._assume_role_with_saml(
- sts_client=sts_client, role_arn=role_arn, assume_role_kwargs=assume_role_kwargs
- )
- elif assume_role_method == 'assume_role_with_web_identity':
- botocore_session = self._assume_role_with_web_identity(
- role_arn=role_arn,
- assume_role_kwargs=assume_role_kwargs,
- base_session=session._session,
- )
- return boto3.session.Session(
- region_name=session.region_name,
- botocore_session=botocore_session,
- **session_kwargs,
- )
- else:
+ supported_methods = ['assume_role', 'assume_role_with_saml', 'assume_role_with_web_identity']
+ if assume_role_method not in supported_methods:
raise NotImplementedError(
f'assume_role_method={assume_role_method} in Connection {self.conn.conn_id} Extra.'
- 'Currently "assume_role" or "assume_role_with_saml" are supported.'
+ f'Currently {supported_methods} are supported.'
'(Exclude this setting will default to "assume_role").'
)
- # Use credentials retrieved from STS
- credentials = sts_response["Credentials"]
- aws_access_key_id = credentials["AccessKeyId"]
- aws_secret_access_key = credentials["SecretAccessKey"]
- aws_session_token = credentials["SessionToken"]
- self.log.info(
- "Creating session with aws_access_key_id=%s region_name=%s",
- aws_access_key_id,
- session.region_name,
- )
-
- return boto3.session.Session(
- aws_access_key_id=aws_access_key_id,
- aws_secret_access_key=aws_secret_access_key,
- region_name=session.region_name,
- aws_session_token=aws_session_token,
- **session_kwargs,
- )
+ if assume_role_method == 'assume_role_with_web_identity':
+ # Deferred credentials have no initial credentials
+ credential_fetcher = self._get_web_identity_credential_fetcher()
+ credentials = DeferredRefreshableCredentials(
+ method='assume-role-with-web-identity',
+ refresh_using=credential_fetcher.fetch_credentials,
+ time_fetcher=lambda: datetime.datetime.now(tz=tzlocal()),
+ )
+ else:
+ # Refreshable credentials do have initial credentials
+ credentials = RefreshableCredentials.create_from_metadata(
+ metadata=self._refresh_credentials(),
+ refresh_using=self._refresh_credentials,
+ method="sts-assume-role",
+ )
+ session = botocore.session.get_session()
+ session._credentials = credentials # pylint: disable=protected-access
+ region_name = self.basic_session.region_name
+ session.set_config_variable("region", region_name)
+ return boto3.session.Session(botocore_session=session, **session_kwargs)
+
+ def _refresh_credentials(self) -> boto3.session.Session:
Review comment:
Fixed up, thank you for the catch
--
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] baolsen commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-873828496
Fixed existing tests.
New unit tests still TODO
--
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 #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-875114496
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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] baolsen commented on a change in pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#discussion_r664251182
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -289,8 +297,8 @@ def _fetch_saml_assertion_using_http_spegno_auth(self, saml_config: Dict[str, An
raise ValueError('Invalid SAML Assertion')
return saml_assertion
- def _assume_role_with_web_identity(self, role_arn, assume_role_kwargs, base_session):
- base_session = base_session or botocore.session.get_session()
+ def _get_web_identity_credential_fetcher(self):
Review comment:
Done, thanks for the feedback.
--
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 #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874935312
> > Second nitpick, I believe the three commits need to be squashed, or perhaps I misunderstood when I read that somewhere. Kamil or Jarek can confirm or refute that.
>
> This is not needed and I do not recommend doing this as several commits make reviews easier. Commiters do this automatically when merging PRs.
Oh wow, then I am quite sorry. I was positive someone had told me to make sure I do that in my previous PR. Apologies for the frustration and the misinformation.
> Thanks for the suggestion. I'm doing development on Windows which can make testing interesting.
> The closest I can get to a Linux environment is WSL + Ubuntu.
Ah, that does complicate things. Maybe a virtualbox running linux, but either way... not as convenient and useful as intended :P
--
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] baolsen commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874479932
> @baolsen For what it's worth, I recently learned you can run the same CI tetss locally using the script at `./scripts/ci/testing/ci_run_airflow_testing.sh`. That may help in the future.
Thanks for the suggestion. I'm doing development on Windows which can make testing interesting.
The closest I can get to a Linux environment is WSL + Ubuntu.
I'll try getting CI actions working on my github account next time.
--
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] baolsen commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-873957330
--
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 edited a comment on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
ferruzzi edited a comment on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874409805
> @ferruzzi Can you take a look at it? I know you've been authoring with AWS/EKS lately, so your review would be valuable here.
@mik-laj Yup, sorry about that, it was a holiday weekend here and I was away. Taking a look.
> I'm submitting this to CI so long so I can find issues more easily.
@baolsen For what it's worth, I recently learned you can run the same CI tetss locally using the script at `./scripts/ci/testing/ci_run_airflow_testing.sh`. That may help in the future.
--
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 #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874409805
> @ferruzzi Can you take a look at it? I know you've been authoring with AWS/EKS lately, so your review would be valuable here.
Yup, sorry about that, it was a holiday weekend here and I was away. Taking 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] ferruzzi edited a comment on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
ferruzzi edited a comment on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874409805
@mik-laj
> @ferruzzi Can you take a look at it? I know you've been authoring with AWS/EKS lately, so your review would be valuable here.
Yup, sorry about that, it was a holiday weekend here and I was away. Taking a look.
@baolsen
> I'm submitting this to CI so long so I can find issues more easily.
For what it's worth, i recently learned you can run the same CI tetss locally using the script at `./scripts/ci/testing/ci_run_airflow_testing.sh`. That may help in the future.
--
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] baolsen commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-873957330
CI failing due to unknown / unrelated issues, possibly flaky. Will attempt a rerun sometime
--
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 edited a comment on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
ferruzzi edited a comment on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874409805
--
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] baolsen commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
baolsen commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874478743
> With the one nitpick request above (which I won't make a required change), and the caveat that I still have a bit to learn about the AWS auth/credential system, I think it looks good. Seems like a good addition and I don't see anything jumping out at me as an obvious issue.
Thanks. My only "concern" is that I can't verify that the assume_role_with_web_identity control path works on my environment. I guess that is what the unit tests are for, but it would be handy if someone who uses that control path could pull this and verify it works for them. Logically I have stepped through the changes and it does look correct.
> [EDIT] Second nitpick, I believe the three commits need to be squashed, or perhaps I misunderstood when I read that somewhere. Kamil or Jarek can confirm or refute that.
I'm also not sure... I figured that separate commits may be easier to review what changed, provided it's not too spammy.
Happy to rebase and squash if the others ask, otherwise I guess it can be done on merge :)
--
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] mik-laj commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874559256
> Second nitpick, I believe the three commits need to be squashed, or perhaps I misunderstood when I read that somewhere. Kamil or Jarek can confirm or refute that.
This is not needed and I do not recommend doing this as several commits make reviews easier. Commiters do this automatically when merging PRs.
--
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] mik-laj commented on pull request #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-873040647
@ferruzzi Can you take a look at it? I know you've been authoring with AWS/EKS lately, so your review would be valuable here.
--
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 #16771: Update AWS Base hook to use refreshable credentials (#16770)
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #16771:
URL: https://github.com/apache/airflow/pull/16771#issuecomment-874409805
> @ferruzzi Can you take a look at it? I know you've been authoring with AWS/EKS lately, so your review would be valuable here.
Yup, sorry about that, it was a holiday weekend here and I was away. Taking 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