feat!: typed, opt-in configuration for icechunk virtual chunk access - #146
feat!: typed, opt-in configuration for icechunk virtual chunk access#146maxrjones wants to merge 3 commits into
Conversation
hrodmn
left a comment
There was a problem hiding this comment.
Thanks for this change @maxrjones! I think the settings -> icechunk credentials approach you built works well. I admit that I get confused by the credentials API for virtual icechunk stores, but your changes and accompanying documentation are really helpful.
FWIW I don't think we should trigger v1.0 with this change (!feat), it would be fine to merge as a feat: ... since we are pre 1.0 still and we are not yet depending on the service in VEDA.
| # icechunk's builders fall back to FromEnv (the service's own ambient | ||
| # credentials) when called with no arguments; requiring an explicit | ||
| # mode keeps that a deliberate grant instead of a default |
There was a problem hiding this comment.
Do you think the earthmover team would be receptive to moving some of the pydantic models into the icechunk library? It's useful here but does not feel specific to titiler-multidim.
| ] | ||
|
|
||
|
|
||
| def build_virtual_chunk_access( |
There was a problem hiding this comment.
is there any reason not to add this as a method on one of the pydantic classes? I not sure where the boundary should be between application code and the pydantic settings, but it could be nice to keep all of the icechunk credentials logic close together instead of spread between reader.py and settings.py.
Sounds good. We could also reduce the impact of this change by incorporating earthaccess-auth at the same time, authorizing access to all the buckets registered in CMR metadata via temporary S3 credentials rather than role-based access. Do you think that's worth it either here or as a separate PR? |
I think that sounds like a separate PR. Would that mean we don't need to add the DAAC buckets to the JSON config? |
Summary
This PR types the configuration for Icechunk virtual chunk access authorization. I believe this is worth it because:
file://(also see https://www.earthmover.io/blog/secure-virtual-chunks).This is a breaking change, since it requires granting access via the chunk configuration. I think this is the right call, but it does mean that the configuration would need to be updated when new data sources are added, rather than just adding access to the deployment credentials. However, NASA data is stored in a reasonably number of buckets that we could enumerate, so I don't think this is a huge downside.
It also relies on keeping the typed configuration up-to-date with Icechunk's, since they don't expose types for this. I added a test to catch drift and pinned the major version of Icehunk to protect against drift.
Lastly, I add documentation for how to authorize access to chunks.
Testing
PR checks
run-cdk-checkslabel to this PR.deploy-devlabel. It smoke-tests tiles from the native MUR, virtual MUR, and virtual NLDAS Icechunk stores after deployment.