Cleanup a number of issues.

re: Issue Unidata#2748 This PR fixes a number of issues and bugs. ## s3cleanup fixes * Delete extraneous s3cleanup.sh related files. * Remove duplicate s3cleanup.uids entries. ## Support the Google S3 API * Add code to recognize "storage.gooleapis.com" * Add extra code to track the kind of server being accessed: unknown, Amazon, Google. * Add a new mode flag "gs3" (analog to "s3") to support this api. * Modify the S3 URL code to support this case. * Modify the listobjects result parsing because Google returns some non-standard XML elements. * Change signature and calls for NC_s3urlrebuild. ## Handle corrupt Zarr files where shape is empty for a variable. Modify behavior when a variable's "shape" dictionary entry. Previously it returned an error, but now it suppresses such a variable. This change makes it possible to read non-corrupt data from the file. Also added a test case. ## Misc. Other Changes * Fix the nclog level handling to suppress output by default. * Fix de-duplicates code in ncuri.c * Restore testing of iridl.ldeo.columbia.edu. * Fix bug in define_vars() which did not always do a proper reclaim between variables.
seanm · Oct 8, 2023 · 1552d89 · 1552d89
1 parent 8b5f1e2
commit 1552d89
Show file tree

Hide file tree

Showing 24 changed files with 280 additions and 133 deletions.
diff --git a/Makefile.am b/Makefile.am
@@ -211,7 +211,11 @@ install-data-hook:
 all-local: liblib/libnetcdf.la
 	echo ${PACKAGE_VERSION} > VERSION
 if ENABLE_S3_TESTALL
+	rm -f ${abs_top_builddir}/tmp_@[email protected]
 	echo "@TESTUID@" >> ${abs_top_builddir}/s3cleanup_@[email protected]
+	cat ${abs_top_builddir}/s3cleanup_@[email protected] | sort | uniq > ${abs_top_builddir}/tmp_@[email protected]
+	rm -f ${abs_top_builddir}/s3cleanup_@[email protected]
+	mv ${abs_top_builddir}/tmp_@[email protected] ${abs_top_builddir}/s3cleanup_@[email protected]
 endif
 
 if ENABLE_S3_TESTALL

diff --git a/include/ncrc.h b/include/ncrc.h
@@ -53,6 +53,9 @@ typedef struct NCRCinfo {
 	NClist* s3profiles; /* NClist<struct AWSprofile*> */
 } NCRCinfo;
 
+/* Opaque structures */
+struct NCS3INFO;
+
 #if defined(__cplusplus)
 extern "C" {
 #endif
@@ -94,7 +97,7 @@ EXTERNL int NC_getactives3profile(NCURI* uri, const char** profilep);
 EXTERNL int NC_s3profilelookup(const char* profile, const char* key, const char** valuep);
 EXTERNL int NC_authgets3profile(const char* profile, struct AWSprofile** profilep);
 EXTERNL int NC_iss3(NCURI* uri);
-EXTERNL int NC_s3urlrebuild(NCURI* url, char** inoutbucketp, char** inoutregionp, NCURI** newurlp);
+EXTERNL int NC_s3urlrebuild(NCURI* url, struct NCS3INFO* s3, NCURI** newurlp);
 
 #if defined(__cplusplus)
 }

diff --git a/include/ncs3sdk.h b/include/ncs3sdk.h
@@ -6,13 +6,19 @@
 #ifndef NCS3SDK_H
 #define NCS3SDK_H 1
 
+/* Track the server type, if known */
+typedef enum NCS3SVC {NCS3UNK=0, /* unknown */
+                 NCS3=1,    /* s3.amazon.aws */
+		 NCS3GS=0   /* storage.googleapis.com */
+} NCS3SVC;
 
 typedef struct NCS3INFO {
     char* host; /* non-null if other*/
     char* region; /* region */
     char* bucket; /* bucket name */
     char* rootkey;
     char* profile;
+    NCS3SVC svc;
 } NCS3INFO;
 
 #ifdef __cplusplus

diff --git a/libdispatch/dinfermodel.c b/libdispatch/dinfermodel.c
@@ -136,6 +136,7 @@ static const struct MACRODEF {
 {"xarray","mode",{"zarr", NULL}},
 {"noxarray","mode",{"nczarr", "noxarray", NULL}},
 {"zarr","mode",{"nczarr","zarr", NULL}},
+{"gs3","mode",{"gs3","nczarr",NULL}}, /* Google S3 API */
 {NULL,NULL,{NULL}}
 };
 
@@ -196,6 +197,7 @@ static struct NCPROTOCOLLIST {
     {"dods","http","mode=dap2"},
     {"dap4","http","mode=dap4"},
     {"s3","s3","mode=s3"},
+    {"gs3","gs3","mode=gs3"},
     {NULL,NULL,NULL} /* Terminate search */
 };
 
@@ -914,7 +916,7 @@ NC_infermodel(const char* path, int* omodep, int iscreate, int useparallel, void
 	/* If s3, then rebuild the url */
 	if(NC_iss3(uri)) {
 	    NCURI* newuri = NULL;
-	    if((stat = NC_s3urlrebuild(uri,NULL,NULL,&newuri))) goto done;
+	    if((stat = NC_s3urlrebuild(uri,NULL,&newuri))) goto done;
 	    ncurifree(uri);
 	    uri = newuri;
 	} else if(strcmp(uri->protocol,"file")==0) {

diff --git a/libdispatch/drc.c b/libdispatch/drc.c
@@ -437,7 +437,9 @@ rccompile(const char* filepath)
     NCURI* uri = NULL;
     char* nextline = NULL;
     NCglobalstate* globalstate = NC_getglobalstate();
-    char* bucket = NULL;
+    NCS3INFO s3;
+
+    memset(&s3,0,sizeof(s3));
 
     if((ret=NC_readfile(filepath,tmp))) {
         nclog(NCLOGWARN, "Could not open configuration file: %s",filepath);
@@ -484,9 +486,8 @@ rccompile(const char* filepath)
 	    if(NC_iss3(uri)) {
 	         NCURI* newuri = NULL;
 	        /* Rebuild the url to S3 "path" format */
-	        nullfree(bucket);
-		bucket = NULL;
-	        if((ret = NC_s3urlrebuild(uri,&bucket,NULL,&newuri))) goto done;
+		NC_s3clear(&s3);
+	        if((ret = NC_s3urlrebuild(uri,&s3,&newuri))) goto done;
 		ncurifree(uri);
 		uri = newuri;
 		newuri = NULL;
@@ -546,6 +547,7 @@ rccompile(const char* filepath)
     rcorder(rc);
 
 done:
+    NC_s3clear(&s3);
     if(contents) free(contents);
     ncurifree(uri);
     ncbytesfree(tmp);

diff --git a/libdispatch/ds3util.c b/libdispatch/ds3util.c
@@ -30,6 +30,7 @@
 #undef AWSDEBUG
 
 #define AWSHOST ".amazonaws.com"
+#define GOOGLEHOST "storage.googleapis.com"
 
 enum URLFORMAT {UF_NONE=0, UF_VIRTUAL=1, UF_PATH=2, UF_S3=3, UF_OTHER=4};
 
@@ -44,15 +45,12 @@ Rebuild an S3 url into a canonical path-style url.
 If region is not in the host, then use specified region
 if provided, otherwise us-east-1.
 @param url (in) the current url
-@param region	(in) region to use if needed; NULL => us-east-1
-		(out) region from url or the input region
-@param bucketp	(in) bucket to use if needed
-		(out) bucket from url
+@param s3	(in/out) NCS3INFO structure
 @param pathurlp (out) the resulting pathified url string
 */
 
 int
-NC_s3urlrebuild(NCURI* url, char** inoutbucketp, char** inoutregionp, NCURI** newurlp)
+NC_s3urlrebuild(NCURI* url, NCS3INFO* s3, NCURI** newurlp)
 {
     int i,stat = NC_NOERR;
     NClist* hostsegments = NULL;
@@ -63,6 +61,7 @@ NC_s3urlrebuild(NCURI* url, char** inoutbucketp, char** inoutregionp, NCURI** ne
     char* host = NULL;
     char* path = NULL;
     char* region = NULL;
+    NCS3SVC svc = NCS3UNK;
 
     if(url == NULL)
         {stat = NC_EURL; goto done;}
@@ -83,14 +82,27 @@ NC_s3urlrebuild(NCURI* url, char** inoutbucketp, char** inoutregionp, NCURI** ne
        Path: https://s3.<region>.amazonaws.com/<bucket-name>/<path>				(3)
          or: https://s3.amazonaws.com/<bucket-name>/<path> -- region defaults to us-east-1      (4)
        S3: s3://<bucket-name>/<path>								(5)
-      Other: https://<host>/<bucket-name>/<path>						(6)
+       Google: https://storage.googleapis.com/<bucket-name>/<path>				(6)
+           or: gs3://<bucket-name>/<path>							(7)
+       Other: https://<host>/<bucket-name>/<path>						(8)
     */
     if(url->host == NULL || strlen(url->host) == 0)
         {stat = NC_EURL; goto done;}
+
+    /* Reduce the host to standard form such as s3.amazonaws.com by pulling out the
+       region and bucket from the host */
     if(strcmp(url->protocol,"s3")==0 && nclistlength(hostsegments)==1) { /* Format (5) */
 	bucket = nclistremove(hostsegments,0);
 	/* region unknown at this point */
+	/* Host will be set to canonical form later */
+	svc = NCS3;
+    } else if(strcmp(url->protocol,"gs3")==0 && nclistlength(hostsegments)==1) { /* Format (7) */
+	bucket = nclistremove(hostsegments,0);
+	/* region unknown at this point */
+	/* Host will be set to canonical form later */
+	svc = NCS3GS;
     } else if(endswith(url->host,AWSHOST)) { /* Virtual or path */
+	svc = NCS3;
 	/* If we find a bucket as part of the host, then remove it */
 	switch (nclistlength(hostsegments)) {
 	default: stat = NC_EURL; goto done;
@@ -99,32 +111,39 @@ NC_s3urlrebuild(NCURI* url, char** inoutbucketp, char** inoutregionp, NCURI** ne
     	    /* bucket unknown at this point */
 	    break;
 	case 4: /* Format (2) or (3) */
-            if(strcasecmp(nclistget(hostsegments,1),"s3")==0) { /* Format (2) */
+            if(strcasecmp(nclistget(hostsegments,0),"s3")!=0) { /* Presume format (2) */
 	        /* region unknown at this point */
-	        bucket = nclistremove(hostsegments,0); /* Note removeal */
+	        bucket = nclistremove(hostsegments,0); /* Make canonical */
             } else if(strcasecmp(nclistget(hostsegments,0),"s3")==0) { /* Format (3) */
-	        region = strdup(nclistget(hostsegments,1));
+	        region = nclistremove(hostsegments,1); /* Make canonical */
 	        /* bucket unknown at this point */
 	    } else /* ! Format (2) and ! Format (3) => error */
 	        {stat = NC_EURL; goto done;}
 	    break;
 	case 5: /* Format (1) */
             if(strcasecmp(nclistget(hostsegments,1),"s3")!=0)
 	        {stat = NC_EURL; goto done;}
-	    region = strdup(nclistget(hostsegments,2));
-    	    bucket = strdup(nclistremove(hostsegments,0));
+	    /* Make canonical */
+	    region = nclistremove(hostsegments,2);
+    	    bucket = nclistremove(hostsegments,0);
 	    break;
 	}
-    } else { /* Presume Format (6) */
+    } else if(strcasecmp(url->host,GOOGLEHOST)==0) { /* Google (6) */
+        if((host = strdup(url->host))==NULL)
+	    {stat = NC_ENOMEM; goto done;}
+        /* region is unknown */
+	/* bucket is unknown at this point */
+	svc = NCS3GS;
+    } else { /* Presume Format (8) */
         if((host = strdup(url->host))==NULL)
 	    {stat = NC_ENOMEM; goto done;}
         /* region is unknown */
 	/* bucket is unknown */
     }
 
-    /* region = (1) from url, (2) inoutregion, (3) default */
-    if(region == NULL)
-	region = (inoutregionp?nulldup(*inoutregionp):NULL);
+    /* region = (1) from url, (2) s3->region, (3) default */
+    if(region == NULL && s3 != NULL)
+	region = nulldup(s3->region);
     if(region == NULL) {
         const char* region0 = NULL;
 	/* Get default region */
@@ -133,23 +152,30 @@ NC_s3urlrebuild(NCURI* url, char** inoutbucketp, char** inoutregionp, NCURI** ne
     }
     if(region == NULL) {stat = NC_ES3; goto done;}
 
-    /* bucket = (1) from url, (2) inoutbucket */
+    /* bucket = (1) from url, (2) s3->bucket */
     if(bucket == NULL && nclistlength(pathsegments) > 0) {
 	bucket = nclistremove(pathsegments,0); /* Get from the URL path; will reinsert below */
     }
-    if(bucket == NULL)
-	bucket = (inoutbucketp?nulldup(*inoutbucketp):NULL);
+    if(bucket == NULL && s3 != NULL)
+	bucket = nulldup(s3->bucket);
     if(bucket == NULL) {stat = NC_ES3; goto done;}
 
-    if(host == NULL) { /* Construct the revised host */
+    if(svc == NCS3) {
+        /* Construct the revised host */
+	ncbytesclear(buf);
         ncbytescat(buf,"s3.");
         ncbytescat(buf,region);
         ncbytescat(buf,AWSHOST);
+	nullfree(host);
         host = ncbytesextract(buf);
+    } else if(svc == NCS3GS) {
+	nullfree(host);
+	host = strdup(GOOGLEHOST);
     }
 
-    /* Construct the revised path */
     ncbytesclear(buf);
+
+    /* Construct the revised path */
     if(bucket != NULL) {
         ncbytescat(buf,"/");
         ncbytescat(buf,bucket);
@@ -159,10 +185,13 @@ NC_s3urlrebuild(NCURI* url, char** inoutbucketp, char** inoutregionp, NCURI** ne
 	ncbytescat(buf,nclistget(pathsegments,i));
     }
     path = ncbytesextract(buf);
+
     /* complete the new url */
     if((newurl=ncuriclone(url))==NULL) {stat = NC_ENOMEM; goto done;}
     ncurisetprotocol(newurl,"https");
+    assert(host != NULL);
     ncurisethost(newurl,host);
+    assert(path != NULL);
     ncurisetpath(newurl,path);
     /* Rebuild the url->url */
     ncurirebuild(newurl);
@@ -171,9 +200,11 @@ NC_s3urlrebuild(NCURI* url, char** inoutbucketp, char** inoutregionp, NCURI** ne
     fprintf(stderr,">>> NC_s3urlrebuild: final=%s bucket=%s region=%s\n",uri->uri,bucket,region);
 #endif
     if(newurlp) {*newurlp = newurl; newurl = NULL;}
-    if(inoutbucketp) {*inoutbucketp = bucket; bucket = NULL;}
-    if(inoutregionp) {*inoutregionp = region; region = NULL;}
-
+    if(s3 != NULL) {
+        s3->bucket = bucket; bucket = NULL;
+        s3->region = region; region = NULL;
+        s3->svc = svc;
+    }
 done:
     nullfree(region);
     nullfree(bucket)
@@ -218,7 +249,7 @@ NC_s3urlprocess(NCURI* url, NCS3INFO* s3, NCURI** newurlp)
     s3->profile = strdup(profile0);
 
     /* Rebuild the URL to path format and get a usable region and optional bucket*/
-    if((stat = NC_s3urlrebuild(url,&s3->bucket,&s3->region,&url2))) goto done;
+    if((stat = NC_s3urlrebuild(url,s3,&url2))) goto done;
     s3->host = strdup(url2->host);
     /* construct the rootkey minus the leading bucket */
     pathsegments = nclistnew();
@@ -268,7 +299,7 @@ NC_s3clear(NCS3INFO* s3)
 }
 
 /*
-Check if a url has indicators that signal an S3 url.
+Check if a url has indicators that signal an S3 or Google S3 url.
 */
 
 int
@@ -277,13 +308,17 @@ NC_iss3(NCURI* uri)
     int iss3 = 0;
 
     if(uri == NULL) goto done; /* not a uri */
-    /* is the protocol "s3"? */
+    /* is the protocol "s3" or "gs3" ? */
     if(strcasecmp(uri->protocol,"s3")==0) {iss3 = 1; goto done;}
-    /* Is "s3" in the mode list? */
-    if(NC_testmode(uri,"s3")) {iss3 = 1; goto done;}    
+    if(strcasecmp(uri->protocol,"gs3")==0) {iss3 = 1; goto done;}
+    /* Is "s3" or "gs3" in the mode list? */
+    if(NC_testmode(uri,"s3")) {iss3 = 1; goto done;}
+    if(NC_testmode(uri,"gs3")) {iss3 = 1; goto done;}    
     /* Last chance; see if host looks s3'y */
-    if(endswith(uri->host,AWSHOST)) {iss3 = 1; goto done;}
-
+    if(uri->host != NULL) {
+        if(endswith(uri->host,AWSHOST)) {iss3 = 1; goto done;}
+        if(strcasecmp(uri->host,GOOGLEHOST)==0) {iss3 = 1; goto done;}
+    }    
 done:
     return iss3;
 }

diff --git a/libdispatch/nch5s3comms.c b/libdispatch/nch5s3comms.c
@@ -96,6 +96,7 @@
 
 /*****************/
 
+#include "ncs3sdk.h"
 #include "nch5s3comms.h" /* S3 Communications */
 
 /****************/
@@ -1063,7 +1064,7 @@ NCH5_s3comms_s3r_execute(s3r_t *handle, const char* url,
  *----------------------------------------------------------------------------
  */
 s3r_t *
-NCH5_s3comms_s3r_open(const char* root, const char *region, const char *access_id, const char* access_key)
+NCH5_s3comms_s3r_open(const char* root, NCS3SVC svc, const char *region, const char *access_id, const char* access_key)
 {
     int ret_value = SUCCEED;
     size_t         tmplen    = 0;
@@ -1092,10 +1093,15 @@ NCH5_s3comms_s3r_open(const char* root, const char *region, const char *access_i
      * RECORD THE ROOT PATH
      *************************************/
 
-    /* Verify that the region is a substring of root */
-    if(region != NULL && region[0] != '\0') {
-	if(strstr(root,region) == NULL)
-	    HGOTO_ERROR(H5E_ARGS, NC_EINVAL, NULL, "region not present in root path.");
+    switch (svc) {
+    case NCS3:
+        /* Verify that the region is a substring of root */
+        if(region != NULL && region[0] != '\0') {
+	    if(strstr(root,region) == NULL)
+	        HGOTO_ERROR(H5E_ARGS, NC_EINVAL, NULL, "region not present in root path.");
+        }
+	break;
+    default: break;
     }
     handle->rootpath = nulldup(root);
 

diff --git a/libdispatch/nch5s3comms.h b/libdispatch/nch5s3comms.h
@@ -502,7 +502,7 @@ EXTERNL hrb_t *NCH5_s3comms_hrb_init_request(const char *resource, const char *h
  * DECLARATION OF S3REQUEST ROUTINES *
  *************************************/
 
-EXTERNL s3r_t *NCH5_s3comms_s3r_open(const char* root, const char* region, const char* id, const char* access_key);
+EXTERNL s3r_t *NCH5_s3comms_s3r_open(const char* root, NCS3SVC svc, const char* region, const char* id, const char* access_key);
 
 EXTERNL int NCH5_s3comms_s3r_close(s3r_t *handle);
 

diff --git a/libdispatch/nclog.c b/libdispatch/nclog.c
@@ -136,7 +136,7 @@ ncvlog(int level, const char* fmt, va_list ap)
     const char* prefix;
 
     if(!nclogginginitialized) ncloginit();
-    if(nclog_global.loglevel > level || nclog_global.nclogstream == NULL)
+    if(nclog_global.loglevel < level)
 	return;
     prefix = nctagname(level);
     fprintf(nclog_global.nclogstream,"%s: ",prefix);