Skip to content

Support runtime CORS configuration #340

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

Closed
captaincoordinates opened this issue Jan 27, 2022 · 4 comments
Closed

Support runtime CORS configuration #340

captaincoordinates opened this issue Jan 27, 2022 · 4 comments

Comments

@captaincoordinates
Copy link
Contributor

The documentation describes how to configure CORS for stac-fastapi and this currently requires code modifications. As a result anyone wanting to run the API with CORS needs to fork the repo to add a few lines of code. This is not ideal as CORS is a common requirement.

stac-fastapi should support runtime configuration of CORS with the following approach:

  • CORS configuration file added to a container image or copied / mounted into a running container
  • environment variable points to file location
  • API recognizes and loads CORS configuration during init sequence

This approach has been implemented in a fork and a PR will be submitted shortly.

@vincentsarago
Copy link
Member

As a result anyone wanting to run the API with CORS needs to fork the repo

stac-fastapi is a proper python module, users should not have to fork but just do pip install. Then it's pretty easy to setup your own application: https://github.com/developmentseed/eoAPI/blob/master/src/eoapi/stac/eoapi/stac/app.py

I worry that if we add a special case for CORS, then another user might come with other middleware needs (e.g cache control).

previous conversation: #127

🤦 well in ☝️ @geospatial-jeff makes a good point that cors should be maybe set by default and to be permissive (https://github.com/radiantearth/stac-api-spec/blob/master/implementation.md#cors)

Still IMO, we shouldn't add more complexity on the application part, but work on the documentation explaining how create/customize stac-fastapi application

@captaincoordinates
Copy link
Contributor Author

@vincentsarago I appreciate your input, however I would counter that creating your own application is similarly overkill when all you want is a CORS configuration. Whether it's a forked repo or a new application, either way you're maintaining a repo purely for CORS. pgstac and sqlalchemy each contains >3,000 lines of python, which suggests that this is not a trivial exercise.

I'm not clear on why it's bad to set a precedent here - if it lowers the barriers to using the software without intrusive changes in the code I don't see a problem.

@geospatial-jeff
Copy link
Collaborator

geospatial-jeff commented Jan 28, 2022

I'm not clear on why it's bad to set a precedent here

It's not bad. Precedent is set by the STAC API spec. It says to enable CORS, so we should probably do this to avoid the issue brought up in your original comment (you shouldn't have to fork/write your own app to enable CORS) and to align with STAC best practices.

CORS configuration file added to a container image or copied / mounted into a running container

Using a config file to set CORS is a code leak. Configuration should be done through environment variables to follow microservice best practices.

@geospatial-jeff
Copy link
Collaborator

Closed with #436

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

No branches or pull requests

3 participants