[ARVADOS] updated: 1.3.0-2793-gb2af45c27

Git user git at public.arvados.org
Wed Aug 5 14:29:05 UTC 2020


Summary of changes:
 lib/config/config.default.yml    |   3 +
 lib/config/export.go             |  47 ++++++------
 lib/config/generated_config.go   |   3 +
 sdk/go/arvados/config.go         |   1 +
 services/keep-web/main.go        |   3 +-
 services/keep-web/s3.go          | 162 +++++++++++++++++++++++++++------------
 services/keep-web/s3_test.go     | 124 ++++++++++++++++++++++++------
 services/keep-web/server.go      |   6 +-
 services/keep-web/server_test.go |   2 +-
 9 files changed, 252 insertions(+), 99 deletions(-)

       via  b2af45c27eb49ccddfbdbfe824a56721acddad27 (commit)
       via  0ad35b5c3002910ea60d2ac7685af5936079a46f (commit)
       via  2cbd0a76bf874daf7608da688a34eae271e7e017 (commit)
       via  a527f80f44889537819cbcfc514d813592c08975 (commit)
       via  89bd8ab4b7863d958fd844eafadc51ce27b0b86f (commit)
      from  d6246cb3a2c49d1e19edc9579c96d80fd6535ccc (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit b2af45c27eb49ccddfbdbfe824a56721acddad27
Author: Tom Clegg <tom at tomclegg.ca>
Date:   Wed Aug 5 10:26:44 2020 -0400

    16535: Require Content-Type application/x-directory for dir markers.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>

diff --git a/services/keep-web/s3.go b/services/keep-web/s3.go
index c6781e96f..d7d08668e 100644
--- a/services/keep-web/s3.go
+++ b/services/keep-web/s3.go
@@ -127,7 +127,10 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool {
 				http.Error(w, fmt.Sprintf("error reading request body: %s", err), http.StatusInternalServerError)
 				return true
 			} else if n > 0 {
-				http.Error(w, "cannot write a non-empty file with a trailing '/' char", http.StatusBadRequest)
+				http.Error(w, "cannot create object with trailing '/' char unless content is empty", http.StatusBadRequest)
+				return true
+			} else if strings.SplitN(r.Header.Get("Content-Type"), ";", 2)[0] != "application/x-directory" {
+				http.Error(w, "cannot create object with trailing '/' char unless Content-Type is 'application/x-directory'", http.StatusBadRequest)
 				return true
 			}
 			// Given PUT "foo/bar/", we'll use "foo/bar/."

commit 0ad35b5c3002910ea60d2ac7685af5936079a46f
Author: Tom Clegg <tom at tomclegg.ca>
Date:   Tue Aug 4 15:48:59 2020 -0400

    16535: Accept application/x-directory sentinels for dirs.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>

diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index 907acdc87..e0c1d7576 100644
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -486,6 +486,9 @@ Clusters:
       # Use of this feature is not recommended, if it can be avoided.
       ForwardSlashNameSubstitution: ""
 
+      # Include "folder objects" in S3 ListObjects responses.
+      S3FolderObjects: true
+
       # Managed collection properties. At creation time, if the client didn't
       # provide the listed keys, they will be automatically populated following
       # one of the following behaviors:
diff --git a/lib/config/export.go b/lib/config/export.go
index d6b02b750..f15a29961 100644
--- a/lib/config/export.go
+++ b/lib/config/export.go
@@ -59,10 +59,10 @@ func ExportJSON(w io.Writer, cluster *arvados.Cluster) error {
 // exists.
 var whitelist = map[string]bool{
 	// | sort -t'"' -k2,2
-	"ClusterID":                                    true,
 	"API":                                          true,
 	"API.AsyncPermissionsUpdateInterval":           false,
 	"API.DisabledAPIs":                             false,
+	"API.KeepServiceRequestTimeout":                false,
 	"API.MaxConcurrentRequests":                    false,
 	"API.MaxIndexDatabaseRead":                     false,
 	"API.MaxItemsPerResponse":                      true,
@@ -71,24 +71,29 @@ var whitelist = map[string]bool{
 	"API.MaxRequestSize":                           true,
 	"API.RailsSessionSecretToken":                  false,
 	"API.RequestTimeout":                           true,
-	"API.WebsocketClientEventQueue":                false,
 	"API.SendTimeout":                              true,
+	"API.WebsocketClientEventQueue":                false,
 	"API.WebsocketServerEventQueue":                false,
-	"API.KeepServiceRequestTimeout":                false,
 	"AuditLogs":                                    false,
 	"AuditLogs.MaxAge":                             false,
 	"AuditLogs.MaxDeleteBatch":                     false,
 	"AuditLogs.UnloggedAttributes":                 false,
+	"ClusterID":                                    true,
 	"Collections":                                  true,
+	"Collections.BalanceCollectionBatch":           false,
+	"Collections.BalanceCollectionBuffers":         false,
+	"Collections.BalancePeriod":                    false,
+	"Collections.BalanceTimeout":                   false,
+	"Collections.BlobDeleteConcurrency":            false,
+	"Collections.BlobMissingReport":                false,
+	"Collections.BlobReplicateConcurrency":         false,
 	"Collections.BlobSigning":                      true,
 	"Collections.BlobSigningKey":                   false,
 	"Collections.BlobSigningTTL":                   true,
 	"Collections.BlobTrash":                        false,
-	"Collections.BlobTrashLifetime":                false,
-	"Collections.BlobTrashConcurrency":             false,
 	"Collections.BlobTrashCheckInterval":           false,
-	"Collections.BlobDeleteConcurrency":            false,
-	"Collections.BlobReplicateConcurrency":         false,
+	"Collections.BlobTrashConcurrency":             false,
+	"Collections.BlobTrashLifetime":                false,
 	"Collections.CollectionVersioning":             false,
 	"Collections.DefaultReplication":               true,
 	"Collections.DefaultTrashLifetime":             true,
@@ -97,18 +102,14 @@ var whitelist = map[string]bool{
 	"Collections.ManagedProperties.*":              true,
 	"Collections.ManagedProperties.*.*":            true,
 	"Collections.PreserveVersionIfIdle":            true,
+	"Collections.S3FolderObjects":                  true,
 	"Collections.TrashSweepInterval":               false,
 	"Collections.TrustAllContent":                  false,
 	"Collections.WebDAVCache":                      false,
-	"Collections.BalanceCollectionBatch":           false,
-	"Collections.BalancePeriod":                    false,
-	"Collections.BalanceTimeout":                   false,
-	"Collections.BlobMissingReport":                false,
-	"Collections.BalanceCollectionBuffers":         false,
 	"Containers":                                   true,
 	"Containers.CloudVMs":                          false,
-	"Containers.CrunchRunCommand":                  false,
 	"Containers.CrunchRunArgumentsList":            false,
+	"Containers.CrunchRunCommand":                  false,
 	"Containers.DefaultKeepCacheRAM":               true,
 	"Containers.DispatchPrivateKey":                false,
 	"Containers.JobsAPI":                           true,
@@ -155,28 +156,28 @@ var whitelist = map[string]bool{
 	"Login.OpenIDConnect":                          true,
 	"Login.OpenIDConnect.ClientID":                 false,
 	"Login.OpenIDConnect.ClientSecret":             false,
-	"Login.OpenIDConnect.Enable":                   true,
-	"Login.OpenIDConnect.Issuer":                   false,
 	"Login.OpenIDConnect.EmailClaim":               false,
 	"Login.OpenIDConnect.EmailVerifiedClaim":       false,
+	"Login.OpenIDConnect.Enable":                   true,
+	"Login.OpenIDConnect.Issuer":                   false,
 	"Login.OpenIDConnect.UsernameClaim":            false,
 	"Login.PAM":                                    true,
 	"Login.PAM.DefaultEmailDomain":                 false,
 	"Login.PAM.Enable":                             true,
 	"Login.PAM.Service":                            false,
+	"Login.RemoteTokenRefresh":                     true,
 	"Login.SSO":                                    true,
 	"Login.SSO.Enable":                             true,
 	"Login.SSO.ProviderAppID":                      false,
 	"Login.SSO.ProviderAppSecret":                  false,
-	"Login.RemoteTokenRefresh":                     true,
 	"Mail":                                         true,
+	"Mail.EmailFrom":                               false,
+	"Mail.IssueReporterEmailFrom":                  false,
+	"Mail.IssueReporterEmailTo":                    false,
 	"Mail.MailchimpAPIKey":                         false,
 	"Mail.MailchimpListID":                         false,
 	"Mail.SendUserSetupNotificationEmail":          false,
-	"Mail.IssueReporterEmailFrom":                  false,
-	"Mail.IssueReporterEmailTo":                    false,
 	"Mail.SupportEmailAddress":                     true,
-	"Mail.EmailFrom":                               false,
 	"ManagementToken":                              false,
 	"PostgreSQL":                                   false,
 	"RemoteClusters":                               true,
@@ -194,8 +195,8 @@ var whitelist = map[string]bool{
 	"SystemRootToken":                              false,
 	"TLS":                                          false,
 	"Users":                                        true,
-	"Users.AnonymousUserToken":                     true,
 	"Users.AdminNotifierEmailFrom":                 false,
+	"Users.AnonymousUserToken":                     true,
 	"Users.AutoAdminFirstUser":                     false,
 	"Users.AutoAdminUserWithEmail":                 false,
 	"Users.AutoSetupNewUsers":                      false,
@@ -232,6 +233,7 @@ var whitelist = map[string]bool{
 	"Workbench.EnableGettingStartedPopup":          true,
 	"Workbench.EnablePublicProjectsPage":           true,
 	"Workbench.FileViewersConfigURL":               true,
+	"Workbench.InactivePageHTML":                   true,
 	"Workbench.LogViewerMaxBytes":                  true,
 	"Workbench.MultiSiteSearch":                    true,
 	"Workbench.ProfilingEnabled":                   true,
@@ -243,6 +245,8 @@ var whitelist = map[string]bool{
 	"Workbench.ShowUserAgreementInline":            true,
 	"Workbench.ShowUserNotifications":              true,
 	"Workbench.SiteName":                           true,
+	"Workbench.SSHHelpHostSuffix":                  true,
+	"Workbench.SSHHelpPageHTML":                    true,
 	"Workbench.Theme":                              true,
 	"Workbench.UserProfileFormFields":              true,
 	"Workbench.UserProfileFormFields.*":            true,
@@ -251,9 +255,6 @@ var whitelist = map[string]bool{
 	"Workbench.UserProfileFormMessage":             true,
 	"Workbench.VocabularyURL":                      true,
 	"Workbench.WelcomePageHTML":                    true,
-	"Workbench.InactivePageHTML":                   true,
-	"Workbench.SSHHelpPageHTML":                    true,
-	"Workbench.SSHHelpHostSuffix":                  true,
 }
 
 func redactUnsafe(m map[string]interface{}, mPrefix, lookupPrefix string) error {
diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go
index 96da19dfc..e02ed6a41 100644
--- a/lib/config/generated_config.go
+++ b/lib/config/generated_config.go
@@ -492,6 +492,9 @@ Clusters:
       # Use of this feature is not recommended, if it can be avoided.
       ForwardSlashNameSubstitution: ""
 
+      # Include "folder objects" in S3 ListObjects responses.
+      S3FolderObjects: true
+
       # Managed collection properties. At creation time, if the client didn't
       # provide the listed keys, they will be automatically populated following
       # one of the following behaviors:
diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go
index a54712f33..da03fba7d 100644
--- a/sdk/go/arvados/config.go
+++ b/sdk/go/arvados/config.go
@@ -121,6 +121,7 @@ type Cluster struct {
 		TrashSweepInterval           Duration
 		TrustAllContent              bool
 		ForwardSlashNameSubstitution string
+		S3FolderObjects              bool
 
 		BlobMissingReport        string
 		BalancePeriod            Duration
diff --git a/services/keep-web/main.go b/services/keep-web/main.go
index e4028842f..647eab165 100644
--- a/services/keep-web/main.go
+++ b/services/keep-web/main.go
@@ -14,6 +14,7 @@ import (
 	"git.arvados.org/arvados.git/sdk/go/arvados"
 	"github.com/coreos/go-systemd/daemon"
 	"github.com/ghodss/yaml"
+	"github.com/sirupsen/logrus"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -111,7 +112,7 @@ func main() {
 
 	os.Setenv("ARVADOS_API_HOST", cfg.cluster.Services.Controller.ExternalURL.Host)
 	srv := &server{Config: cfg}
-	if err := srv.Start(); err != nil {
+	if err := srv.Start(logrus.StandardLogger()); err != nil {
 		log.Fatal(err)
 	}
 	if _, err := daemon.SdNotify(false, "READY=1"); err != nil {
diff --git a/services/keep-web/s3.go b/services/keep-web/s3.go
index aa0cbd3c8..c6781e96f 100644
--- a/services/keep-web/s3.go
+++ b/services/keep-web/s3.go
@@ -94,6 +94,11 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool {
 			}
 			return true
 		}
+		if err == nil && fi.IsDir() && objectNameGiven && strings.HasSuffix(fspath, "/") && h.Config.cluster.Collections.S3FolderObjects {
+			w.Header().Set("Content-Type", "application/x-directory")
+			w.WriteHeader(http.StatusOK)
+			return true
+		}
 		if os.IsNotExist(err) ||
 			(err != nil && err.Error() == "not a directory") ||
 			(fi != nil && fi.IsDir()) {
@@ -106,55 +111,82 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool {
 		http.FileServer(fs).ServeHTTP(w, &r)
 		return true
 	case r.Method == "PUT":
-		if strings.HasSuffix(r.URL.Path, "/") {
-			http.Error(w, "invalid object name (trailing '/' char)", http.StatusBadRequest)
+		if !objectNameGiven {
+			http.Error(w, "missing object name in PUT request", http.StatusBadRequest)
 			return true
 		}
 		fspath := "by_id" + r.URL.Path
-		_, err = fs.Stat(fspath)
+		var objectIsDir bool
+		if strings.HasSuffix(fspath, "/") {
+			if !h.Config.cluster.Collections.S3FolderObjects {
+				http.Error(w, "invalid object name: trailing slash", http.StatusBadRequest)
+				return true
+			}
+			n, err := r.Body.Read(make([]byte, 1))
+			if err != nil && err != io.EOF {
+				http.Error(w, fmt.Sprintf("error reading request body: %s", err), http.StatusInternalServerError)
+				return true
+			} else if n > 0 {
+				http.Error(w, "cannot write a non-empty file with a trailing '/' char", http.StatusBadRequest)
+				return true
+			}
+			// Given PUT "foo/bar/", we'll use "foo/bar/."
+			// in the "ensure parents exist" block below,
+			// and then we'll be done.
+			fspath += "."
+			objectIsDir = true
+		}
+		fi, err := fs.Stat(fspath)
 		if err != nil && err.Error() == "not a directory" {
 			// requested foo/bar, but foo is a file
 			http.Error(w, "object name conflicts with existing object", http.StatusBadRequest)
 			return true
 		}
-		f, err := fs.OpenFile(fspath, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0644)
-		if os.IsNotExist(err) {
-			// create missing intermediate directories, then try again
-			for i, c := range fspath {
-				if i > 0 && c == '/' {
-					dir := fspath[:i]
-					if strings.HasSuffix(dir, "/") {
-						err = errors.New("invalid object name (consecutive '/' chars)")
-						http.Error(w, err.Error(), http.StatusBadRequest)
-						return true
-					}
-					err := fs.Mkdir(dir, 0755)
-					if err != nil && err != os.ErrExist {
-						err = fmt.Errorf("mkdir %q failed: %w", dir, err)
-						http.Error(w, err.Error(), http.StatusInternalServerError)
-						return true
-					}
-				}
-			}
-			f, err = fs.OpenFile(fspath, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0644)
-		}
-		if err != nil {
-			err = fmt.Errorf("open %q failed: %w", r.URL.Path, err)
-			http.Error(w, err.Error(), http.StatusBadRequest)
+		if strings.HasSuffix(r.URL.Path, "/") && err == nil && !fi.IsDir() {
+			// requested foo/bar/, but foo/bar is a file
+			http.Error(w, "object name conflicts with existing object", http.StatusBadRequest)
 			return true
 		}
-		defer f.Close()
-		_, err = io.Copy(f, r.Body)
-		if err != nil {
-			err = fmt.Errorf("write to %q failed: %w", r.URL.Path, err)
-			http.Error(w, err.Error(), http.StatusBadGateway)
-			return true
+		// create missing parent/intermediate directories, if any
+		for i, c := range fspath {
+			if i > 0 && c == '/' {
+				dir := fspath[:i]
+				if strings.HasSuffix(dir, "/") {
+					err = errors.New("invalid object name (consecutive '/' chars)")
+					http.Error(w, err.Error(), http.StatusBadRequest)
+					return true
+				}
+				err := fs.Mkdir(dir, 0755)
+				if err != nil && err != os.ErrExist {
+					err = fmt.Errorf("mkdir %q failed: %w", dir, err)
+					http.Error(w, err.Error(), http.StatusInternalServerError)
+					return true
+				}
+			}
 		}
-		err = f.Close()
-		if err != nil {
-			err = fmt.Errorf("write to %q failed: close: %w", r.URL.Path, err)
-			http.Error(w, err.Error(), http.StatusBadGateway)
-			return true
+		if !objectIsDir {
+			f, err := fs.OpenFile(fspath, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0644)
+			if os.IsNotExist(err) {
+				f, err = fs.OpenFile(fspath, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0644)
+			}
+			if err != nil {
+				err = fmt.Errorf("open %q failed: %w", r.URL.Path, err)
+				http.Error(w, err.Error(), http.StatusBadRequest)
+				return true
+			}
+			defer f.Close()
+			_, err = io.Copy(f, r.Body)
+			if err != nil {
+				err = fmt.Errorf("write to %q failed: %w", r.URL.Path, err)
+				http.Error(w, err.Error(), http.StatusBadGateway)
+				return true
+			}
+			err = f.Close()
+			if err != nil {
+				err = fmt.Errorf("write to %q failed: close: %w", r.URL.Path, err)
+				http.Error(w, err.Error(), http.StatusBadGateway)
+				return true
+			}
 		}
 		err = fs.Sync()
 		if err != nil {
@@ -170,9 +202,30 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool {
 	}
 }
 
-func walkFS(fs arvados.CustomFileSystem, path string, ignoreNotFound bool, fn func(path string, fi os.FileInfo) error) error {
+// Call fn on the given path (directory) and its contents, in
+// lexicographic order.
+//
+// If isRoot==true and path is not a directory, return nil.
+//
+// If fn returns filepath.SkipDir when called on a directory, don't
+// descend into that directory.
+func walkFS(fs arvados.CustomFileSystem, path string, isRoot bool, fn func(path string, fi os.FileInfo) error) error {
+	if isRoot {
+		fi, err := fs.Stat(path)
+		if os.IsNotExist(err) || (err == nil && !fi.IsDir()) {
+			return nil
+		} else if err != nil {
+			return err
+		}
+		err = fn(path, fi)
+		if err == filepath.SkipDir {
+			return nil
+		} else if err != nil {
+			return err
+		}
+	}
 	f, err := fs.Open(path)
-	if os.IsNotExist(err) && ignoreNotFound {
+	if os.IsNotExist(err) && isRoot {
 		return nil
 	} else if err != nil {
 		return fmt.Errorf("open %q: %w", path, err)
@@ -247,7 +300,15 @@ func (h *handler) s3list(w http.ResponseWriter, r *http.Request, fs arvados.Cust
 	}
 	commonPrefixes := map[string]bool{}
 	err := walkFS(fs, strings.TrimSuffix(bucketdir+"/"+walkpath, "/"), true, func(path string, fi os.FileInfo) error {
+		if path == bucketdir {
+			return nil
+		}
 		path = path[len(bucketdir)+1:]
+		filesize := fi.Size()
+		if fi.IsDir() {
+			path += "/"
+			filesize = 0
+		}
 		if len(path) <= len(params.prefix) {
 			if path > params.prefix[:len(path)] {
 				// with prefix "foobar", walking "fooz" means we're done
@@ -257,7 +318,7 @@ func (h *handler) s3list(w http.ResponseWriter, r *http.Request, fs arvados.Cust
 				// with prefix "foobar", walking "foobag" is pointless
 				return filepath.SkipDir
 			}
-			if fi.IsDir() && !strings.HasPrefix(params.prefix+"/", path+"/") {
+			if fi.IsDir() && !strings.HasPrefix(params.prefix+"/", path) {
 				// with prefix "foo/bar", walking "fo"
 				// is pointless (but walking "foo" or
 				// "foo/bar" is necessary)
@@ -279,7 +340,12 @@ func (h *handler) s3list(w http.ResponseWriter, r *http.Request, fs arvados.Cust
 		if path < params.marker || path < params.prefix {
 			return nil
 		}
-		if fi.IsDir() {
+		if fi.IsDir() && !h.Config.cluster.Collections.S3FolderObjects {
+			// Note we don't add anything to
+			// commonPrefixes here even if delimiter is
+			// "/". We descend into the directory, and
+			// return a commonPrefix only if we end up
+			// finding a regular file inside it.
 			return nil
 		}
 		if params.delimiter != "" {
@@ -288,12 +354,7 @@ func (h *handler) s3list(w http.ResponseWriter, r *http.Request, fs arvados.Cust
 				// with prefix "foobar" and delimiter
 				// "z", when we hit "foobar/baz", we
 				// add "/baz" to commonPrefixes and
-				// stop descending (note that even if
-				// delimiter is "/" we don't add
-				// anything to commonPrefixes when
-				// seeing a dir: we wait until we see
-				// a file, so we don't incorrectly
-				// return results for empty dirs)
+				// stop descending.
 				commonPrefixes[path[:len(params.prefix)+idx+1]] = true
 				return filepath.SkipDir
 			}
@@ -308,7 +369,7 @@ func (h *handler) s3list(w http.ResponseWriter, r *http.Request, fs arvados.Cust
 		resp.Contents = append(resp.Contents, s3.Key{
 			Key:          path,
 			LastModified: fi.ModTime().UTC().Format("2006-01-02T15:04:05.999") + "Z",
-			Size:         fi.Size(),
+			Size:         filesize,
 		})
 		return nil
 	})
diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go
index ff1b661f7..c76768a14 100644
--- a/services/keep-web/s3_test.go
+++ b/services/keep-web/s3_test.go
@@ -171,18 +171,26 @@ func (s *IntegrationSuite) TestS3ProjectPutObjectSuccess(c *check.C) {
 }
 func (s *IntegrationSuite) testS3PutObjectSuccess(c *check.C, bucket *s3.Bucket, prefix string) {
 	for _, trial := range []struct {
-		path string
-		size int
+		path        string
+		size        int
+		contentType string
 	}{
 		{
-			path: "newfile",
-			size: 128000000,
+			path:        "newfile",
+			size:        128000000,
+			contentType: "application/octet-stream",
+		}, {
+			path:        "newdir/newfile",
+			size:        1 << 26,
+			contentType: "application/octet-stream",
 		}, {
-			path: "newdir/newfile",
-			size: 1 << 26,
+			path:        "newdir1/newdir2/newfile",
+			size:        0,
+			contentType: "application/octet-stream",
 		}, {
-			path: "newdir1/newdir2/newfile",
-			size: 0,
+			path:        "newdir1/newdir2/newdir3/",
+			size:        0,
+			contentType: "application/x-directory",
 		},
 	} {
 		c.Logf("=== %v", trial)
@@ -195,11 +203,14 @@ func (s *IntegrationSuite) testS3PutObjectSuccess(c *check.C, bucket *s3.Bucket,
 		buf := make([]byte, trial.size)
 		rand.Read(buf)
 
-		err = bucket.PutReader(objname, bytes.NewReader(buf), int64(len(buf)), "application/octet-stream", s3.Private, s3.Options{})
+		err = bucket.PutReader(objname, bytes.NewReader(buf), int64(len(buf)), trial.contentType, s3.Private, s3.Options{})
 		c.Check(err, check.IsNil)
 
 		rdr, err := bucket.GetReader(objname)
-		if !c.Check(err, check.IsNil) {
+		if strings.HasSuffix(trial.path, "/") && !s.testServer.Config.cluster.Collections.S3FolderObjects {
+			c.Check(err, check.NotNil)
+			continue
+		} else if !c.Check(err, check.IsNil) {
 			continue
 		}
 		buf2, err := ioutil.ReadAll(rdr)
@@ -220,6 +231,7 @@ func (s *IntegrationSuite) TestS3ProjectPutObjectFailure(c *check.C) {
 	s.testS3PutObjectFailure(c, stage.projbucket, stage.coll.Name+"/")
 }
 func (s *IntegrationSuite) testS3PutObjectFailure(c *check.C, bucket *s3.Bucket, prefix string) {
+	s.testServer.Config.cluster.Collections.S3FolderObjects = false
 	var wg sync.WaitGroup
 	for _, trial := range []struct {
 		path string
@@ -308,13 +320,23 @@ func (s *IntegrationSuite) TestS3CollectionList(c *check.C) {
 	stage := s.s3setup(c)
 	defer stage.teardown(c)
 
-	filesPerDir := 1001
-	stage.writeBigDirs(c, 2, filesPerDir)
-	s.testS3List(c, stage.collbucket, "", 4000, 2+filesPerDir*2)
-	s.testS3List(c, stage.collbucket, "", 131, 2+filesPerDir*2)
-	s.testS3List(c, stage.collbucket, "dir0/", 71, filesPerDir)
+	var markers int
+	for markers, s.testServer.Config.cluster.Collections.S3FolderObjects = range []bool{false, true} {
+		dirs := 2
+		filesPerDir := 1001
+		stage.writeBigDirs(c, dirs, filesPerDir)
+		// Total # objects is:
+		//                 2 file entries from s3setup (emptyfile and sailboat.txt)
+		//                +1 fake "directory" marker from s3setup (emptydir) (if enabled)
+		//             +dirs fake "directory" marker from writeBigDirs (dir0/, dir1/) (if enabled)
+		// +filesPerDir*dirs file entries from writeBigDirs (dir0/file0.txt, etc.)
+		s.testS3List(c, stage.collbucket, "", 4000, markers+2+(filesPerDir+markers)*dirs)
+		s.testS3List(c, stage.collbucket, "", 131, markers+2+(filesPerDir+markers)*dirs)
+		s.testS3List(c, stage.collbucket, "dir0/", 71, filesPerDir+markers)
+	}
 }
 func (s *IntegrationSuite) testS3List(c *check.C, bucket *s3.Bucket, prefix string, pageSize, expectFiles int) {
+	c.Logf("testS3List: prefix=%q pageSize=%d S3FolderObjects=%v", prefix, pageSize, s.testServer.Config.cluster.Collections.S3FolderObjects)
 	expectPageSize := pageSize
 	if expectPageSize > 1000 {
 		expectPageSize = 1000
@@ -350,6 +372,12 @@ func (s *IntegrationSuite) testS3List(c *check.C, bucket *s3.Bucket, prefix stri
 }
 
 func (s *IntegrationSuite) TestS3CollectionListRollup(c *check.C) {
+	for _, s.testServer.Config.cluster.Collections.S3FolderObjects = range []bool{false, true} {
+		s.testS3CollectionListRollup(c)
+	}
+}
+
+func (s *IntegrationSuite) testS3CollectionListRollup(c *check.C) {
 	stage := s.s3setup(c)
 	defer stage.teardown(c)
 
@@ -372,17 +400,40 @@ func (s *IntegrationSuite) TestS3CollectionListRollup(c *check.C) {
 			break
 		}
 	}
-	c.Check(allfiles, check.HasLen, dirs*filesPerDir+3)
+	markers := 0
+	if s.testServer.Config.cluster.Collections.S3FolderObjects {
+		markers = 1
+	}
+	c.Check(allfiles, check.HasLen, dirs*(filesPerDir+markers)+3+markers)
+
+	gotDirMarker := map[string]bool{}
+	for _, name := range allfiles {
+		isDirMarker := strings.HasSuffix(name, "/")
+		if markers == 0 {
+			c.Check(isDirMarker, check.Equals, false, check.Commentf("name %q", name))
+		} else if isDirMarker {
+			gotDirMarker[name] = true
+		} else if i := strings.LastIndex(name, "/"); i >= 0 {
+			c.Check(gotDirMarker[name[:i+1]], check.Equals, true, check.Commentf("name %q", name))
+			gotDirMarker[name[:i+1]] = true // skip redundant complaints about this dir marker
+		}
+	}
 
 	for _, trial := range []struct {
 		prefix    string
 		delimiter string
 		marker    string
 	}{
+		{"", "", ""},
 		{"di", "/", ""},
 		{"di", "r", ""},
 		{"di", "n", ""},
 		{"dir0", "/", ""},
+		{"dir0/", "/", ""},
+		{"dir0/f", "/", ""},
+		{"dir0", "", ""},
+		{"dir0/", "", ""},
+		{"dir0/f", "", ""},
 		{"dir0", "/", "dir0/file14.txt"},       // no commonprefixes
 		{"", "", "dir0/file14.txt"},            // middle page, skip walking dir1
 		{"", "", "dir1/file14.txt"},            // middle page, skip walking dir0
@@ -393,7 +444,7 @@ func (s *IntegrationSuite) TestS3CollectionListRollup(c *check.C) {
 		{"dir2", "/", ""},                      // prefix "dir2" does not exist
 		{"", "/", ""},
 	} {
-		c.Logf("\n\n=== trial %+v", trial)
+		c.Logf("\n\n=== trial %+v markers=%d", trial, markers)
 
 		maxKeys := 20
 		resp, err := stage.collbucket.List(trial.prefix, trial.delimiter, trial.marker, maxKeys)
@@ -445,10 +496,11 @@ func (s *IntegrationSuite) TestS3CollectionListRollup(c *check.C) {
 		for _, prefix := range resp.CommonPrefixes {
 			gotPrefixes = append(gotPrefixes, prefix)
 		}
-		c.Check(gotKeys, check.DeepEquals, expectKeys)
-		c.Check(gotPrefixes, check.DeepEquals, expectPrefixes)
-		c.Check(resp.NextMarker, check.Equals, expectNextMarker)
-		c.Check(resp.IsTruncated, check.Equals, expectTruncated)
+		commentf := check.Commentf("trial %+v markers=%d", trial, markers)
+		c.Check(gotKeys, check.DeepEquals, expectKeys, commentf)
+		c.Check(gotPrefixes, check.DeepEquals, expectPrefixes, commentf)
+		c.Check(resp.NextMarker, check.Equals, expectNextMarker, commentf)
+		c.Check(resp.IsTruncated, check.Equals, expectTruncated, commentf)
 		c.Logf("=== trial %+v keys %q prefixes %q nextMarker %q", trial, gotKeys, gotPrefixes, resp.NextMarker)
 	}
 }
diff --git a/services/keep-web/server.go b/services/keep-web/server.go
index 46dc3d301..8f623c627 100644
--- a/services/keep-web/server.go
+++ b/services/keep-web/server.go
@@ -20,12 +20,12 @@ type server struct {
 	Config *Config
 }
 
-func (srv *server) Start() error {
+func (srv *server) Start(logger *logrus.Logger) error {
 	h := &handler{Config: srv.Config}
 	reg := prometheus.NewRegistry()
 	h.Config.Cache.registry = reg
-	ctx := ctxlog.Context(context.Background(), logrus.StandardLogger())
-	mh := httpserver.Instrument(reg, nil, httpserver.HandlerWithContext(ctx, httpserver.AddRequestIDs(httpserver.LogRequests(h))))
+	ctx := ctxlog.Context(context.Background(), logger)
+	mh := httpserver.Instrument(reg, logger, httpserver.HandlerWithContext(ctx, httpserver.AddRequestIDs(httpserver.LogRequests(h))))
 	h.MetricsAPI = mh.ServeAPI(h.Config.cluster.ManagementToken, http.NotFoundHandler())
 	srv.Handler = mh
 	var listen arvados.URL
diff --git a/services/keep-web/server_test.go b/services/keep-web/server_test.go
index bca7ff49f..c37852a12 100644
--- a/services/keep-web/server_test.go
+++ b/services/keep-web/server_test.go
@@ -442,7 +442,7 @@ func (s *IntegrationSuite) SetUpTest(c *check.C) {
 	cfg.cluster.ManagementToken = arvadostest.ManagementToken
 	cfg.cluster.Users.AnonymousUserToken = arvadostest.AnonymousToken
 	s.testServer = &server{Config: cfg}
-	err = s.testServer.Start()
+	err = s.testServer.Start(ctxlog.TestLogger(c))
 	c.Assert(err, check.Equals, nil)
 }
 

commit 2cbd0a76bf874daf7608da688a34eae271e7e017
Author: Tom Clegg <tom at tomclegg.ca>
Date:   Fri Jul 31 16:36:22 2020 -0400

    16535: Test XML header and content-type.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>

diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go
index 77097b152..ff1b661f7 100644
--- a/services/keep-web/s3_test.go
+++ b/services/keep-web/s3_test.go
@@ -297,9 +297,10 @@ func (s *IntegrationSuite) TestS3GetBucketVersioning(c *check.C) {
 		req.URL.RawQuery = "versioning"
 		resp, err := http.DefaultClient.Do(req)
 		c.Assert(err, check.IsNil)
+		c.Check(resp.Header.Get("Content-Type"), check.Equals, "application/xml")
 		buf, err := ioutil.ReadAll(resp.Body)
 		c.Assert(err, check.IsNil)
-		c.Check(strings.TrimSpace(string(buf)), check.Equals, `<VersioningConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"/>`)
+		c.Check(string(buf), check.Equals, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<VersioningConfiguration xmlns=\"http://s3.amazonaws.com/doc/2006-03-01/\"/>\n")
 	}
 }
 

commit a527f80f44889537819cbcfc514d813592c08975
Author: Tom Clegg <tom at tomclegg.ca>
Date:   Fri Jul 31 16:32:42 2020 -0400

    16535: Test Size field in list response.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>

diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go
index d25e9e47f..77097b152 100644
--- a/services/keep-web/s3_test.go
+++ b/services/keep-web/s3_test.go
@@ -332,6 +332,9 @@ func (s *IntegrationSuite) testS3List(c *check.C, bucket *s3.Bucket, prefix stri
 		}
 		for _, key := range resp.Contents {
 			gotKeys[key.Key] = key
+			if strings.Contains(key.Key, "sailboat.txt") {
+				c.Check(key.Size, check.Equals, int64(4))
+			}
 		}
 		if !resp.IsTruncated {
 			c.Check(resp.NextMarker, check.Equals, "")

commit 89bd8ab4b7863d958fd844eafadc51ce27b0b86f
Author: Tom Clegg <tom at tomclegg.ca>
Date:   Fri Jul 31 16:23:25 2020 -0400

    16535: Test HeadObject and HeadBucket.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>

diff --git a/services/keep-web/s3_test.go b/services/keep-web/s3_test.go
index ce2731ce4..d25e9e47f 100644
--- a/services/keep-web/s3_test.go
+++ b/services/keep-web/s3_test.go
@@ -104,6 +104,18 @@ func (stage s3stage) teardown(c *check.C) {
 	}
 }
 
+func (s *IntegrationSuite) TestS3HeadBucket(c *check.C) {
+	stage := s.s3setup(c)
+	defer stage.teardown(c)
+
+	for _, bucket := range []*s3.Bucket{stage.collbucket, stage.projbucket} {
+		c.Logf("bucket %s", bucket.Name)
+		exists, err := bucket.Exists("")
+		c.Check(err, check.IsNil)
+		c.Check(exists, check.Equals, true)
+	}
+}
+
 func (s *IntegrationSuite) TestS3CollectionGetObject(c *check.C) {
 	stage := s.s3setup(c)
 	defer stage.teardown(c)
@@ -123,9 +135,16 @@ func (s *IntegrationSuite) testS3GetObject(c *check.C, bucket *s3.Bucket, prefix
 	err = rdr.Close()
 	c.Check(err, check.IsNil)
 
+	// GetObject
 	rdr, err = bucket.GetReader(prefix + "missingfile")
 	c.Check(err, check.ErrorMatches, `404 Not Found`)
 
+	// HeadObject
+	exists, err := bucket.Exists(prefix + "missingfile")
+	c.Check(err, check.IsNil)
+	c.Check(exists, check.Equals, false)
+
+	// GetObject
 	rdr, err = bucket.GetReader(prefix + "sailboat.txt")
 	c.Assert(err, check.IsNil)
 	buf, err = ioutil.ReadAll(rdr)
@@ -133,6 +152,11 @@ func (s *IntegrationSuite) testS3GetObject(c *check.C, bucket *s3.Bucket, prefix
 	c.Check(buf, check.DeepEquals, []byte("⛵\n"))
 	err = rdr.Close()
 	c.Check(err, check.IsNil)
+
+	// HeadObject
+	exists, err = bucket.Exists(prefix + "sailboat.txt")
+	c.Check(err, check.IsNil)
+	c.Check(exists, check.Equals, true)
 }
 
 func (s *IntegrationSuite) TestS3CollectionPutObjectSuccess(c *check.C) {

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list