16535: Accept application/x-directory sentinels for dirs.
authorTom Clegg <tom@tomclegg.ca>
Tue, 4 Aug 2020 19:48:59 +0000 (15:48 -0400)
committerTom Clegg <tom@tomclegg.ca>
Tue, 4 Aug 2020 19:48:59 +0000 (15:48 -0400)
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>

lib/config/config.default.yml
lib/config/export.go
lib/config/generated_config.go
sdk/go/arvados/config.go
services/keep-web/main.go
services/keep-web/s3.go
services/keep-web/s3_test.go
services/keep-web/server.go
services/keep-web/server_test.go

index 907acdc87847f9c052aee71c5e1d1fbe8c4f78aa..e0c1d75765b5195a402d4b638ba089bd1dc13753 100644 (file)
@@ -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:
index d6b02b750de122582e35a5aa34b508861106ac40..f15a2996197804c24face6be71fc4f97afb558f4 100644 (file)
@@ -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 {
index 96da19dfcdc14c6e20f0d1ea348c2423f909b1ba..e02ed6a418a5aa2f1623f15e7d03664110b82c26 100644 (file)
@@ -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:
index a54712f330ea2b1ff2a6b8107daec5639c082c32..da03fba7d9547589a80814264da93ae2be668911 100644 (file)
@@ -121,6 +121,7 @@ type Cluster struct {
                TrashSweepInterval           Duration
                TrustAllContent              bool
                ForwardSlashNameSubstitution string
+               S3FolderObjects              bool
 
                BlobMissingReport        string
                BalancePeriod            Duration
index e4028842f0c6b9390715a93c836846f2d9ba753b..647eab1653294311644bdce91faa367bd0ec1832 100644 (file)
@@ -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 {
index aa0cbd3c8a38aca2ced6fbfec00fbe56db081f2b..c6781e96f7c104731c4f8bd3c5f027ea6fb3be29 100644 (file)
@@ -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
        })
index ff1b661f7be043e21c38da15b3f6147b0377695c..c76768a14a486461c734b48f728ec6d8ec774f3a 100644 (file)
@@ -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)
        }
 }
index 46dc3d30179343e29edea208588103fe947c3c08..8f623c627d067f843f2746a2b8b64248006b1a18 100644 (file)
@@ -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
index bca7ff49fa0820c8affd818dd6c8b26ceca81755..c37852a128bbaa9571ebf1527f3f9f6b6cee41ae 100644 (file)
@@ -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)
 }