pgagnon commented on code in PR #29623:
URL: https://github.com/apache/airflow/pull/29623#discussion_r1112360505
##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -312,19 +312,35 @@ def _get_web_identity_credential_fetcher(
base_session = self.basic_session._session or
botocore.session.get_session()
client_creator = base_session.create_client
federation =
self.extra_config.get("assume_role_with_web_identity_federation")
- if federation == "google":
- web_identity_token_loader =
self._get_google_identity_token_loader()
- else:
- raise AirflowException(
- f'Unsupported federation: {federation}. Currently "google"
only are supported.'
- )
+
+ web_identity_token_loader = (
+ {
+ "file": self._get_file_token_loader,
+ "google": self._get_google_identity_token_loader,
+ }.get(federation)()
+ if type(federation) == str
+ else None
+ )
Review Comment:
@Taragolis I think two areas are being raised here. On the implementation, I
am well aware that it can be improved, and I fully expected to go through a few
rounds to get this in shape. If you have specific review comments, I would
happily work with you on this. 🙂
Regarding the alternatives that you have outlined, I have considered them
previously, and I do not think they address the objective. Specifically:
1. The objective is more about using multiple `role_arn` than multiple
tokens. It is widespread in practice for users not to have the ability to
override config files (for instance, in corporate environments or on PaaS), so
defining profiles in the AWS config or shared credentials file is not always
practical.
2. This shares the same concern as point 1 regarding the inability to
overwrite config files. Also, I think this option would be dismissed as too
complex and burdensome by 99% of Airflow users.
3. This does not permit configuration through the Airflow connections
subsystem without relying on external configs, which are (1) not necessarily
mutable and (2) more-or-less static. This would be possible if `boto3` allowed
passing `web_identity_token_file,` `role_arn`, or `role_session_name` as
parameters to clients or sessions, but sadly it does not at this point.
This being said, I share your concern about the lack of system tests for
connections, and this is something worthwhile to look at but probably out of
the scope of this discussion.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
