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

S3 Mode url reconstruction defaults to wrong server type #2947

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

mannreis
Copy link
Contributor

@mannreis mannreis commented Jul 9, 2024

We faced an issue when using ncdump to read Zarr from a S3 instance that is not hosted by AWS or Google.

ncdump -h https://bucket.s3.region.example.com/cmip6/CMIP6/CMIP/NASA-GISS/GISS-E2-1-G/historical/r1i1p1f1/day/tasmin/gn/v20181015/#mode=zarr,s3

Noting the "dummy" host on the url of the command above, unfortunately it works. This is because on this NC_s3urlrebuild call, uri->host is overwritten to the default: storage.googleapis.com.

In this PR:

  • NCS3SVC is re-declared, so that NCS3UNK != NCS3GS - fixing the issue.
  • Optional: Added support to use the host of a url to infer bucket and region inference, similar to AWS.
  • Optional: Unit test NC_s3urlrebuild

(edit: links)

@CLAassistant
Copy link

CLAassistant commented Jul 9, 2024

CLA assistant check
All committers have signed the CLA.

@mannreis mannreis changed the title S3 Mode url recontruction defaults to wrong server type S3 Mode url reconstruction defaults to wrong server type Jul 9, 2024
@dopplershift dopplershift added this to the 4.9.3 milestone Jul 9, 2024
@WardF WardF self-assigned this Jul 9, 2024
Copy link
Member

@WardF WardF left a comment

Choose a reason for hiding this comment

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

Approved pending tests passing.

@WardF
Copy link
Member

WardF commented Jul 9, 2024

The failures appear unrelated to the PR, and are part of an issue we're fixing on the Unidata side. This looks good and will be merged shortly.

@WardF
Copy link
Member

WardF commented Jul 10, 2024

The certificate issue on our end is resolved so I'm re-running the jobs; hopefully this should all pass, and we'll get this merged! Thanks!

@DennisHeimbigner
Copy link
Collaborator

Thanks fo this. We did not support this case because we could find a test site.

@WardF
Copy link
Member

WardF commented Jul 11, 2024

The current failure is the result of a combination of two different PRs which both passed checks, but combined, are resulting in a strange error. Once we get that fixed we can re-run the checks and merge this. @DennisHeimbigner this is related to the anonymous dimension issue I reached out about.

@WardF WardF merged commit 62ab618 into Unidata:main Jul 15, 2024
107 checks passed
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

Successfully merging this pull request may close these issues.

5 participants