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

Allow get_aws_credentials to assume_role_with_web_identity #137

Merged
merged 4 commits into from
Apr 19, 2024

Conversation

Qmando
Copy link
Member

@Qmando Qmando commented Jan 30, 2024

This change will allow the caller of get_aws_credentials to use assume_role_with_web_identity with a simple argument. This is ultimately to allow paasta spark-run to be used from within a Pod Identity context. The previous workaround was to create a AWS config file with a profile that did the same thing.

Pairs with Yelp/paasta#3787

Manual test, after setting ENV variables with a real token:
https://fluffy.yelpcorp.com/i/9nt6FK4T2dlSScKdVF53371flGRPS05F.html

@Qmando Qmando changed the title Allow get_aws_credentials to use default boto3 session Allow get_aws_credentials to assume_role_with_web_identity Jan 30, 2024
Comment on lines 139 to 141
if not token_path or not role_arn:
log.warning('No web identity token file found.')
return None, None, None
Copy link
Member

Choose a reason for hiding this comment

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

This case seems like it should raise instead of returning the (None, None, None) tuple and logging a warning. If you want it to use web_identity and it doesn't find the token or the role ARN, it will likely make it more difficult to diagnose why permission errors are happening (e.g. whomever is calling this may end up just defaulting to whatever standard resolution like the instance profile

@88manpreet
Copy link
Contributor

We are soon going to migration our spark jobs onto driver on k8s. Which means:

  • Tron jobs: Spark driver would run on k8s pod (not batch box)
  • Adhoc jobs: Spark driver for now keeps running on given host. But in future it would be launched on k8s pod

Would the functional behavior of using the above flags (assume-aws-role or aws-profile or use-web-identity) remain the same?
cc'ing: @CaptainSame who can help you provide context and testing for driver on k8s.

@Qmando
Copy link
Member Author

Qmando commented Feb 27, 2024

We are soon going to migration our spark jobs onto driver on k8s. Which means:

  • Tron jobs: Spark driver would run on k8s pod (not batch box)
  • Adhoc jobs: Spark driver for now keeps running on given host. But in future it would be launched on k8s pod

Would the functional behavior of using the above flags (assume-aws-role or aws-profile or use-web-identity) remain the same?

Seems like there's potentially two things that need to happen:

  • include paasta_tools::spark_role_assumer in kube node puppet module
  • Spark driver pod needs to mount /nail/etc/spark_role_assumer/
  • Maybe a file permission change so the pod can access the file

@CaptainSame
Copy link
Contributor

Is this meant to solve the adhoc paasta spark-run use-case where AWS_WEB_IDENTITY_TOKEN_FILE and AWS_ROLE_ARN env variables are present? Does the already implemented --assume-aws-role method have some limitations? Can you please explain the use-cases

@CaptainSame
Copy link
Contributor

In our latest release with Spark drivers running on k8s, we have removed the usage of paasta spark-run for Spark batches running on tron in favor of a cleaner spark submit (The role assumption shenanigans are being done using a KUBECONFIG file). So I am not sure where this use-case fits

@nurdann
Copy link
Member

nurdann commented Apr 17, 2024

Is this meant to solve the adhoc paasta spark-run use-case where AWS_WEB_IDENTITY_TOKEN_FILE and AWS_ROLE_ARN env variables are present? Does the already implemented --assume-aws-role method have some limitations? Can you please explain the use-cases

This is specifically to solve the issue of spark-run not picking up pod identity when running Jenkins pipeline. See for more details https://jira.yelpcorp.com/browse/SEC-18599

Copy link
Contributor

@CaptainSame CaptainSame left a comment

Choose a reason for hiding this comment

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

Sure. This looks good to me

@nurdann nurdann merged commit 4f859db into master Apr 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants