-
Notifications
You must be signed in to change notification settings - Fork 17
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 the AWS SDK to get auth from the environment (fixes #28) #88
base: dev
Are you sure you want to change the base?
Conversation
|
||
const s3 = new S3Client({ | ||
credentials: { | ||
if (options.STORAGE_S3_ACCESS_KEY && options.STORAGE_S3_SECRET_KEY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This being separate is required, because having secretAccessKey
and accessKeyId
keys set to undefined in a credential object will cause the AWS SDK to throw and not actually grab from the env
This would also be very useful on EC2 for automatically using instance profiles. Is there anything preventing this from being merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR! As we don't have experience with AWS, we appreciate you helping to include this platform.
The changes proposed improve AWS usage, but it seems, a little bit at the cost of the manual configuration.
Not everyone using S3 is on AWS, so it would be nice to combine the two approaches equally.
A compromise would be to keep the current manual documentation and validation, but to add a new Auto-Config parameter, making it easier for people using AWS. This extra parameter would add your logic, instead of replacing the current manual way.
#### `STORAGE_S3_ACCESS_KEY` | ||
|
||
Example: `access_key` | ||
|
||
The access key for S3 storage. | ||
|
||
#### `STORAGE_S3_SECRET_KEY` | ||
|
||
Example: `secret_key` | ||
|
||
The secret key for S3 storage. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep the manual approach in the docs, as not everyone using S3 is running with AWS.
STORAGE_S3_BUCKET: z.string().min(1), | ||
STORAGE_S3_ENDPOINT: z.string().min(1), | ||
STORAGE_S3_ENDPOINT: z.string().optional(), | ||
STORAGE_S3_REGION: z.string().min(1).default('us-east-1'), | ||
STORAGE_S3_PORT: z.coerce.number().positive(), | ||
STORAGE_S3_USE_SSL: z.string().transform((v) => v === 'true'), | ||
STORAGE_S3_ACCESS_KEY: z.string().min(1), | ||
STORAGE_S3_SECRET_KEY: z.string().min(1), | ||
STORAGE_S3_PORT: z.coerce.number().positive().optional(), | ||
STORAGE_S3_USE_SSL: z | ||
.string() | ||
.transform((v) => v === 'true') | ||
.optional(), | ||
STORAGE_S3_ACCESS_KEY: z.string().optional(), | ||
STORAGE_S3_SECRET_KEY: z.string().optional(), | ||
AWS_REGION: z.string().optional(), | ||
AWS_DEFAULT_REGION: z.string().optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion to please both sides would be to use Zods discriminated union around a new parameter (how about STORAGE_S3_AWS_AUTO_CREDENTIALS
?).
When this parameter is set to 'true', then all the other parameters could be optional, otherwise it would function normal for the manual approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're using the AWS SDK, the services:
postgres:
image: postgres:15
ports:
- 5432:5432
environment:
POSTGRES_USER: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_DB: postgres
mysql:
image: mysql:8
ports:
- 3306:3306
environment:
MYSQL_ROOT_PASSWORD: root
MYSQL_DATABASE: mysql
minio:
image: quay.io/minio/minio:latest
entrypoint: sh
command: -c 'mkdir -p /data/test && /usr/bin/minio server /data'
ports:
- 9000:9000
environment:
MINIO_ROOT_USER: access_key
MINIO_ROOT_PASSWORD: secret_key
cache-server:
build:
dockerfile: Dockerfile
context: .
ports:
- '3000:3000'
environment:
API_BASE_URL: http://localhost:3000
STORAGE_DRIVER: s3
STORAGE_S3_BUCKET: test
###### vvvvv
AWS_ACCESS_KEY_ID: access_key
AWS_SECRET_ACCESS_KEY: secret_key
AWS_ENDPOINT_URL: http://minio:9000 They're functionally identical to passing the vars in when invoking the SDK, like you are currently, so I wouldn't say it's an AWS-centric change - merely unlocking the rest of the functionality that AWS SDK exposes rather than just a limited sliver. The SDK already has many different auth mechanisms whether you're using AWS or not, and promoting using them rather than adding a limited custom config setup on top reduces maintenance overhead for you and improves adoption by allowing for more advanced use-cases that would be non-feasible for you to support (Such as OIDC authentication with minio). Would some better documentation, eg examples and links to the SDK environment variables list help? (this is actually the CLI variable list, but they're very intentionally identical to the SDK variable list) |
I would probably completely replace our custom env vars in favor of aws defaults and only validate vars which are always required. The basic env vars should also be mentioned in the docs so people don't have to look it up elsewhere. |
You're right. I didn't know that the AWS SDK handles so many credentials environments cases. |
The AWS SDK can automatically gather auth from various sources in the environment, but this couldn't happen because the zod schema validation would throw if you didn't provide
STORAGE_S3_ACCESS_KEY
andSTORAGE_S3_SECRET_KEY
.This PR makes several of the env vars optional, and deprecates them in favour of the env vars AWS SDK will infer by default.
I've tested these changes in my EKS cluster with IRSA set up for the pod to assume a role for S3 access, and it seems to work as expected.
Fixes #28, and should still be backwards compatible.