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

AWS_HTTPS support + fix missing scheme #476

Merged
merged 8 commits into from
Feb 4, 2022

Conversation

emmanuelmathot
Copy link
Contributor

@emmanuelmathot emmanuelmathot commented Jan 24, 2022

Proposed Changes:

This PR add support for setting the S3 endpoint url scheme via the AWS_HTTPS environment variables in aws_get_object function using boto3. AWS_HTTPS is aligned on on gdal vsis3 implmenation: https://github.com/OSGeo/gdal/blob/12aaf4be2c20a399efe005482e811a579a12a9fb/port/cpl_aws.cpp#L1207

PR Checklist:

  • This PR has no breaking changes.
  • Tests pass (run tox)
  • I have added my changes to the CHANGELOG

rio_tiler/utils.py Outdated Show resolved Hide resolved
@vincentsarago
Copy link
Member

@emmanuelmathot I'm not quite sure we should use AWS_S3_ENDPOINT variable here. It might be better to use a rio-tiler specific variable to not mix GDAL/Boto3 config.

I'm not sure of this but in theory a STAC url could need an S3 endpoint while the asset could not 🤷

@emmanuelmathot
Copy link
Contributor Author

I agree with a more dedicated solution for S3 config. I came up with this pragmatic one to make it work seamlessly with GDAL config.
Concerning multi S3 endpoints, this is indeed a legitimate use case but unfortunately not trivial to tackle. For the current being, we assume that a deployment is "linked" to a S3 endpoint.
This will remain the case until the S3 URLs do not include the endpoint and the bucket consistently.
Some links for the discussion:
https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-bucket-intro.html
https://aws.amazon.com/fr/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/
https://aws.amazon.com/fr/s3/features/multi-region-access-points/

@vincentsarago
Copy link
Member

ok lets go ahead with what you propose. Later we could add another variable if a user need to have different endpoints for the item and assets

@vincentsarago vincentsarago merged commit 90e5065 into cogeotiff:master Feb 4, 2022
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.

2 participants