Support for GDAL related assets #218
Conversation
| @@ -412,6 +412,18 @@ def sign_asset_href(self, asset_href): | |||
| signed_href = pc.sign(asset_href) | |||
| return signed_href | |||
|
|
|||
| if asset_href.startswith("s3://"): | |||
There was a problem hiding this comment.
@ahuarte47 can you put the code for with the new changes into its own function the sign_asset_href method is for signing planetary computer SAS based items hrefs with a SAS token.
There was a problem hiding this comment.
Is not the generation of signed urls the purpose of sign_asset_href? IMHO this function can manage all cases without modifying the current behavior or the existence of new future sign methods. Do you agree? Just chaning the description of the method?
There was a problem hiding this comment.
Lets make another function for the new changes.
There was a problem hiding this comment.
@Samweli can't really understand. The plugin and the sign method are not named as microsoft or planetarycomputer dependent => because the plugin is a Common, have to be and remain general.
sign_asset_href is a enough general name that can refer to the signing action that is common to any cloud resource not only that from microsoft services.
In resume, what is the rationale to move a sign feature for only GC or AWS to a different method respect signing of microsoft cloud?
There was a problem hiding this comment.
@ahuarte47 @luipir I prefer to have another function for the new changes, at some point I ll also rename the current sign_asset_href to a more specific PC related name.
There was a problem hiding this comment.
@ahuarte47 I tried your approach with a couple of STAC catalogs that use AWS S3 service for item assets hrefs but couldn't make them work. What catalogs are you using to test this functionality?
| @@ -412,6 +412,18 @@ def sign_asset_href(self, asset_href): | |||
| signed_href = pc.sign(asset_href) | |||
| return signed_href | |||
|
|
|||
| if asset_href.startswith("s3://"): | |||
| signed_href = gdal.GetSignedURL(f"/vsis3/{asset_href[5:]}") | |||
There was a problem hiding this comment.
Couldn't find documentation for the GetSignedURL https://gdal.org/api/python/osgeo.gdal.html?highlight=getsigned#osgeo.gdal.GetSignedURL, can you explain what it tries to achieve?
There was a problem hiding this comment.
gdal.GetSignedURL tranforms a GDAL virtual file system path to one https signed url. Depending on the virtual file system type, GDAL reads its specific authentication settings from environment variables. This is an example:
https://github.com/OSGeo/gdal/blob/master/autotest/gcore/vsioss.py#L185
There was a problem hiding this comment.
"A signed URL is a URL that provides limited permission and time to make a request. Signed URLs contain authentication information in their query string, allowing users without credentials to perform specific actions on a resource" from https://cloud.google.com/storage/docs/access-control/signed-urls
There was a problem hiding this comment.
For example, GDAL reads several environment variables (AWS_NO_SIGN_REQUEST, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN...) to create the signed url. It is very useful custom way to auhenticate, apart from using the UI of authentication that your plugin provides.
There was a problem hiding this comment.
@Samweli you can use the Collection Digital Earth Africa - s2l2a in the predefined Catalog. Before, You have to define this environment variable AWS_NO_SIGN_REQUEST=YES. Using gdal 3.5.1 it works.
There was a problem hiding this comment.
So these new changes will not work without user having to define the environment variable AWS_NO_SIGN_REQUEST?
There was a problem hiding this comment.
Yes. For other use cases, credentials will be configured defining AWS_PROFILE or AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY. It depends on how the STAC Catalog is deployed in the cloud-provider.
|
Hi @Samweli I have uploaded more changes. I have noticed that original |
|
Hi @Samweli do you need more info about last changes? Can I help with anything to get this PR merged? Thanks!! |
|
Any feedback @Samweli how to proceed ? |
@luipir @ahuarte47 I have added new comments on the review suggestions, In general I like these new changes to be implemented in a separate function, as I have plans to update the |
|
@Samweli reasonable your proposed changes if you are planning to move the code to be more agnostic an not only PC oriented :) |
|
please @Samweli give us a feedback regarding latest modification done by @ahuarte47, thank you |
|
Just gave a try with private S3 endpoint: it is working great. The only cumbersome thing is to define the |
|
Just saw that GDAL |
|
Any updates on when this PR will be merged? |
This PR implements #217
It provides support for loading assets from GDAL Virtual File Systems keys.