Skip to content

feat!: typed, opt-in configuration for icechunk virtual chunk access - #146

Open
maxrjones wants to merge 3 commits into
developmentseed:mainfrom
maxrjones:feat/icechunk-virtual-chunk-access
Open

feat!: typed, opt-in configuration for icechunk virtual chunk access#146
maxrjones wants to merge 3 commits into
developmentseed:mainfrom
maxrjones:feat/icechunk-virtual-chunk-access

Conversation

@maxrjones

Copy link
Copy Markdown
Member

Summary

This PR types the configuration for Icechunk virtual chunk access authorization. I believe this is worth it because:

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

  • I ran the tests locally, and tested with TEMPO data.

PR checks

  • Standard CI runs automatically on each push.
  • To run the CDK synth check, add the run-cdk-checks label to this PR.
  • If you push more commits after that run completes, remove and re-add the label to run it again.
  • To trigger a dev deployment, add the deploy-dev label. It smoke-tests tiles from the native MUR, virtual MUR, and virtual NLDAS Icechunk stores after deployment.

@hrodmn hrodmn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +29 to +31
# 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a nice guardrail!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@maxrjones

maxrjones commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

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.

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?

@hrodmn

hrodmn commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants