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

Use aws_credentials for S3 file system if Access Key ID is missing #2347

Merged
merged 10 commits into from
Nov 16, 2023

Conversation

mbklein
Copy link
Contributor

@mbklein mbklein commented Nov 14, 2023

Resolves #2341. Tested for the following cases:

  • EC2 with instance role
  • ECS with task role
  • AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables
  • Access Key ID and Secret Access Key provided via the FileSystem form

I ran into an issue trying to test with the aws_credentials_file provider, which uses the canonical AWS profile configuration files (~/.aws/config and ~/.aws/credentials). The error was coming from the eini parser, and I couldn't come up with a version of my config file that would satisfy it. Every other ini-file-compatible AWS client seems to work with my configuration just fine, so I'm not sure what can be done besides try to fix it upstream.

I also made the Access Key ID and Secret Access Key fields in the FileSystem form optional, but if either one is present, the other has to be as well in order for the changeset to validate.

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2023

CLA assistant check
All committers have signed the CLA.

lib/livebook/utils.ex Outdated Show resolved Hide resolved
lib/livebook/utils.ex Outdated Show resolved Hide resolved
@mbklein
Copy link
Contributor Author

mbklein commented Nov 15, 2023

@josevalim I've refactored things a little bit and pushed to an alternate branch for comparison. I think it's a more elegant solution and solves the two issues above.

The new option:

  • Moves ensure_credentials/1 from S3.Client to S3 and makes it public
  • Adds validate_credentials/1 so that the S3 changeset will only validate if both Access Key ID and Secret Key are supplied by the user or credentials exist in the environment at the time the FileSystem is added
  • Removes Livebook.Utils.validate_mutual_inclusion/2 because it's no longer needed

If you agree that it's the better approach I'll merge it into this PR branch.

@mbklein
Copy link
Contributor Author

mbklein commented Nov 15, 2023

PR branch is now up to date with all comments thus far

@@ -14,6 +14,12 @@ defmodule Livebook.FileSystem.S3 do
hub_id: String.t()
}

@type credentials_t :: %{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@type credentials_t :: %{
@type credentials :: %{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally called the type credentials, but I wasn't clear on whether a module could contain a type and a function with the same name.

@josevalim
Copy link
Contributor

This looks super clean! I think we are almost there. :)

Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

LGTM!

@jonatanklosko
Copy link
Member

@josevalim we should update FFS to accept the token and pass it from kino, right?

Copy link

github-actions bot commented Nov 16, 2023

Uffizzi Preview deployment-40863 was deleted.

@josevalim
Copy link
Contributor

@jonatanklosko yes, I will ask @philss to work on it :)

@josevalim josevalim merged commit fbcf3e5 into livebook-dev:main Nov 16, 2023
7 checks passed
@josevalim
Copy link
Contributor

💚 💙 💜 💛 ❤️

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.

Support aws_credentials
4 participants