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

Support optional AWS requester-pays S3 transfer costs configuration #159

Merged
merged 18 commits into from
Mar 15, 2023

Conversation

anayeaye
Copy link
Collaborator

@anayeaye anayeaye commented Mar 9, 2023

What

This PR adds optional raster-api configuration to pay all S3 transfer costs. The setting is applied globally to all requests by setting AWS_REQUEST_PAYER=requester in the raster-api lambda environment.

Why

We want to use data hosted by an external in-region provider that requires that the requester pays S3 transfer costs.

Assumptions

VEDA data read role is already restricted to only in-region buckets so requester pays costs will be negligible.

How tested

Manually deployed the CloudFormation update to the dev stack and confirmed that several raster-api read requests from different buckets were successful.

Further work

  • Costs should be monitored to see if further restrictions are needed to limit costs (i.e. are we incurring excessive costs requesting data from buckets that do not require the requester pay S3 transfer costs).
  • Tests are now run against a newer version of pgstac than we are actually running, if we do not upgrade in Investigate upgrading pgSTAC version #154, we should update our documentation to clarify why the versions differ

Note To use this optional parameter this change for an operation veda-backend stack:

  1. Add VEDA_RASTER_AWS_REQUEST_PAYER=requester to the secret containing the environment variables for a given stage
  2. If using VEDA_RASTER_DATA_ACCESS_ROLE_ARN, confirm that the role has a policy specifically allowing GET requests to the in-region requester-pays bucket.

anayeaye added 2 commits March 9, 2023 12:55
…da environment to agree to pay all S3 transfer costs
@anayeaye
Copy link
Collaborator Author

anayeaye commented Mar 9, 2023

Pre-commit is running locally for me but I am on python 3.9 (not 3.8 like the github runner). I'm sorting through the debug messages in the failed lint action.

@vincentsarago
Copy link
Contributor

🤷 that predeploy step fails because of plpygis module which is shipped with the .pyc files 🤦
Screenshot 2023-03-10 at 12 20 08 PM

I've started a PR over bosth/plpygis#8 but it's unlikely that it will get merged/deploy soon

multiple solutions:

@anayeaye
Copy link
Collaborator Author

Thank you for getting these action unblocked @vincentsarago!

Since the first bullet worked for the pre-deploy, do you think it'll be safe to just use the same for our deploy on merge and then plan on watching for the merge on you plpygis pr and/or removal of plpygis from pgstac?

multiple solutions:

@vincentsarago
Copy link
Contributor

🥳 bosth/plpygis#7 (comment) plpygis should be fixed soon, let's wait for it

@vincentsarago
Copy link
Contributor

🤔 tests are now failing because of a pydantic issue in the raster api 🤷

veda.raster  |     result = context.run(func, *args)
veda.raster  |   File "/opt/bitnami/python/lib/python3.9/site-packages/titiler/pgstac/factory.py", line 359, in register_search
veda.raster  |     model.Link(
veda.raster  |   File "pydantic/main.py", line 341, in pydantic.main.BaseModel.__init__
veda.raster  | pydantic.error_wrappers.ValidationError: 1 validation error for Link

@vincentsarago
Copy link
Contributor

@anayeaye I have no idea about what's going on and I can't reproduce locally :-(
my hope is that by upgrading titiler-pgstac in #148 this could fix the issue

@@ -9,6 +9,7 @@
"titiler.pgstac==0.1.0.a9",
"titiler.application>=0.5,<0.6",
"starlette-cramjam>=0.1.0,<0.2",
"fastapi==0.93.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Comparing logs between 🟢 and 🔴 I saw that the only differences were the FastAPI and Starlette versions. This PR fix FastAPI to the latest working version.

I'll check in titiler-pgstac if something is broken with FastAPI>=0.94

@vincentsarago vincentsarago mentioned this pull request Mar 14, 2023
@anayeaye anayeaye merged commit 26ead17 into develop Mar 15, 2023
@anayeaye anayeaye deleted the feature/raster-aws-request-payer branch March 15, 2023 16:59
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