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

Host path handling for S3 #2151

Closed
magnusuMET opened this issue Nov 23, 2021 · 9 comments · Fixed by #2152
Closed

Host path handling for S3 #2151

magnusuMET opened this issue Nov 23, 2021 · 9 comments · Fixed by #2152
Assignees
Labels
area/nczarr nczarr related topics.
Milestone

Comments

@magnusuMET
Copy link
Contributor

During some testing to get minio (S3-compatible storage) to work we encountered some issues with the host path handling in this library.

ncbytescat(buf,AWSHOST);
Should probably use host and not the default AWSHOST.

ncbytescat(buf,"s3.");
It should be possible to opt out of in instances where one does not use AWS

ncbytescat(buf,AWSHOST);
A dot is missing between the region and host if one uses a URL like
https://example.com/BUCKET/dataset#mode=zarr,s3&aws.region=minio
The last one can be worked around by specifying the region as minio. (trailing dot), although the assumption seems to be that host will always start with a dot (e.g. AWSHOST).

@WardF WardF self-assigned this Nov 23, 2021
@WardF WardF added this to the 4.8.2 milestone Nov 23, 2021
@WardF WardF added the area/nczarr nczarr related topics. label Nov 23, 2021
@WardF
Copy link
Member

WardF commented Nov 23, 2021

Tagging @DennisHeimbigner but we can address these in the next release. Thanks!

@DennisHeimbigner
Copy link
Collaborator

We were using a local appliance supplied to us by NCAR for testing.
I switched to using S3 when a bucket became available. Looks like
I forgot to check for backward compatibility when I switched.
I will re-instate additional testing on the NCAR appliance and fix any bugs.
Hopefully that will correct your problems.

@DennisHeimbigner
Copy link
Collaborator

Ok, make the following change to ds3util.c and see if it fixes your problem.

  1. edit libdispatch/ds3util.c
  2. at line 122 change

ncbytescat(buf,"s3.");
ncbytescat(buf,region);
ncbytescat(buf,AWSHOST);
host = ncbytesextract(buf);

to

if(host == NULL) { /* Construct the revised host */
ncbytescat(buf,"s3.");
ncbytescat(buf,region);
ncbytescat(buf,AWSHOST);
host = ncbytesextract(buf);
}

@magnusuMET
Copy link
Contributor Author

Still need to prepend the region in the non-aws case, the following resolves to the correct host:

/* Construct the revised host */
if (host == NULL) {
    ncbytescat(buf, "s3.");
    ncbytescat(buf, region);
    ncbytescat(buf, AWSHOST);
} else {
    ncbytescat(buf, region);
    ncbytescat(buf, ".");
    ncbytescat(buf, host);
}
host = ncbytesextract(buf);

@magnusuMET
Copy link
Contributor Author

I am posting this here as it is relevant to s3 handling but could post this as a separate issue if you'd like:
There are disagreements on the signature of NC_s3sdkfinalize;

liblib/nc_initialize.c
51:extern int NC_s3sdkfinalize(void);

include/ncs3sdk.h
14:EXTERNL void NC_s3sdkfinalize(void);

libdispatch/ncs3sdk.cpp
80:EXTERNL void
81:NC_s3sdkfinalize(void)

@DennisHeimbigner
Copy link
Collaborator

There is a problem with your prepending the region to non-aws host.
This is appliance dependent. using the host without prepending the region
works fine for the NCAR appliance. I think is better for the caller to add the region
to the host than assuming the appliance will add it.

@magnusuMET
Copy link
Contributor Author

That makes a lot of sense, the code snippet posted above by @DennisHeimbigner works as intended when I change the host to the actual host.

@DennisHeimbigner
Copy link
Collaborator

Anyway, I am in the process of putting up a PR for these issues.

DennisHeimbigner added a commit to DennisHeimbigner/netcdf-c that referenced this issue Nov 26, 2021
re: Unidata#2151

The of a non-aws appliance broke during the switch to testing
against Amazon S3.
So make necessary changes to get non-aws appliances work correctly.
@DennisHeimbigner
Copy link
Collaborator

Fixed by PR #2152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/nczarr nczarr related topics.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants