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

Expose AWS IAM missing param in Hashicorp secret #38536

Merged
merged 13 commits into from
Apr 25, 2024

Conversation

pankajastro
Copy link
Member

@pankajastro pankajastro commented Mar 27, 2024

Expose arn_role param in VaultBackend and add boto3 as an additional dependency. This will help authenticate with temporary credentials.

Example config

AIRFLOW__SECRETS__BACKEND_KWARGS='{"kv_engine_version": 1, "mount_point": "kv", "url": "http://35.161.42.91:8200/", "auth_type": "aws_iam", "arn_role": "arn:aws:iam::123456789000:role/hashicorp-aws-iam-role"}'

Expose aws_conn_id param in VaultBackend and add apache-airflow-providers-amazon as dependency

This PR expose below missing param in hashicorp secret for aws auth

- session_token
- header_value
- use_token
- region

https://github.com/hvac/hvac/blob/48027db42c037fe8f6b1c6fd8f6dbf80c1ea8595/hvac/api/auth_methods/aws.py#L734-L743


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@pankajastro pankajastro marked this pull request as draft March 28, 2024 19:37
@pankajastro pankajastro marked this pull request as ready for review April 8, 2024 13:00
Copy link
Contributor

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

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

Wondering if we need docs for this change?

@pankajastro pankajastro force-pushed the fix_iam_auth branch 2 times, most recently from 28466a7 to 900e319 Compare April 16, 2024 11:52
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

I thought there should be some breaking test for this change 🤔 Should we add some tests for hasicorp?

@pankajastro pankajastro merged commit d08f893 into apache:main Apr 25, 2024
41 checks passed
@pankajastro pankajastro deleted the fix_iam_auth branch April 25, 2024 05:47

if self.role_arn:
sts_client = boto3.client("sts")
credentials = sts_client.assume_role(RoleArn=self.role_arn, RoleSessionName="airflow")
Copy link
Contributor

Choose a reason for hiding this comment

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

@pankajastro - we should consider exposing assume_role_kwargs as a backend_kwarg.

This is currently available as an AWS Connection Extra parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you please share an example of AIRFLOW__SECRETS__BACKEND_KWARGS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the Request Syntax section for example kwargs.

I imagine it would be a dictionary that you would pass to AIRFLOW__SECRETS__BACKEND_KWARGS, like one does today in the AWS connection.

Copy link
Member Author

Choose a reason for hiding this comment

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

wdyt #39279

AIRFLOW__SECRETS__BACKEND_KWARGS='{"kv_engine_version": 1, "mount_point": "kv", "variables_path": "airflow", "url": "http://127.0.0.0:8200/", "auth_type": "aws_iam", "assume_role_kwargs": {"RoleArn": "arn:aws:iam::1234567890000:role/hashicorp-aws-iam", "RoleSessionName": "airflow", "DurationSeconds": 900}}'

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that looks good (assuming that it works). You wouldn't need a role_arn arg in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's included in the assume_role_kwargs parameter. I've shared an example for reference ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I am saying you no longer need a dedicated role_arn backend kwarg. It would be a part of assume_role_kwargs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants