Merge branch '17151-system-token-validation'
authorTom Clegg <tom@curii.com>
Mon, 28 Dec 2020 21:52:16 +0000 (16:52 -0500)
committerTom Clegg <tom@curii.com>
Mon, 28 Dec 2020 21:52:16 +0000 (16:52 -0500)
closes #17151

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

45 files changed:
build/run-build-packages.sh
build/run-library.sh
doc/admin/upgrading.html.textile.liquid
doc/api/methods/collections.html.textile.liquid
doc/install/packages.html.textile.liquid
docker/jobs/apt.arvados.org-dev.list
docker/jobs/apt.arvados.org-stable.list
docker/jobs/apt.arvados.org-testing.list
docker/migrate-docker19/Dockerfile
lib/cloud/ec2/ec2.go
lib/controller/federation_test.go
lib/controller/integration_test.go
lib/costanalyzer/costanalyzer.go
lib/costanalyzer/costanalyzer_test.go
lib/crunchrun/crunchrun.go
lib/crunchrun/crunchrun_test.go
lib/crunchrun/logging.go
lib/crunchrun/logging_test.go
lib/dispatchcloud/test/ssh_service.go
lib/service/cmd.go
lib/service/tls.go
sdk/go/arvadosclient/arvadosclient.go
sdk/go/manifest/manifest.go
sdk/python/arvados/keep.py
sdk/python/tests/run_test_server.py
services/api/app/controllers/arvados/v1/collections_controller.rb
services/api/app/controllers/arvados/v1/schema_controller.rb
services/api/app/models/api_client_authorization.rb
services/api/app/models/collection.rb
services/api/db/migrate/20201202174753_fix_collection_versions_timestamps.rb [new file with mode: 0644]
services/api/db/structure.sql
services/api/lib/fix_collection_versions_timestamps.rb [new file with mode: 0644]
services/api/test/fixtures/collections.yml
services/api/test/functional/arvados/v1/collections_controller_test.rb
services/api/test/integration/collections_api_test.rb
services/api/test/unit/collection_test.rb
services/api/test/unit/log_test.rb
services/fuse/README.rst
services/keep-web/handler.go
services/keep-web/handler_test.go
services/keep-web/s3.go
services/keep-web/s3_test.go
tools/arvbox/bin/arvbox
tools/compute-images/scripts/base.sh
tools/salt-install/provision.sh

index 42c5f3d0947fa196a9ee9949fb0853a2e5065182..9030072d59970186506c38e63ee5887e52e9519f 100755 (executable)
@@ -259,10 +259,8 @@ debug_echo -e "\nPython packages\n"
       git checkout $DASHQ_UNLESS_DEBUG "$COMMIT_HASH"
       echo "$COMMIT_HASH" >git-commit.version
 
-      cd "$SRC_BUILD_DIR"
-      PKG_VERSION=$(version_from_git)
       cd $WORKSPACE/packages/$TARGET
-      fpm_build "$WORKSPACE" $SRC_BUILD_DIR/=/usr/local/arvados/src arvados-src 'dir' "$PKG_VERSION" "--exclude=usr/local/arvados/src/.git" "--url=https://arvados.org" "--license=GNU Affero General Public License, version 3.0" "--description=The Arvados source code" "--architecture=all"
+      fpm_build "$WORKSPACE" $SRC_BUILD_DIR/=/usr/local/arvados/src arvados-src 'dir' "$arvados_src_version" "--exclude=usr/local/arvados/src/.git" "--url=https://arvados.org" "--license=GNU Affero General Public License, version 3.0" "--description=The Arvados source code" "--architecture=all"
 
       rm -rf "$SRC_BUILD_DIR"
     fi
index 6f95a8f4bfd8cb9736a5b9fba6c8076005ce2de3..9efc8028b51f395d4e344bcd34dfb6489cb1374c 100755 (executable)
@@ -347,11 +347,11 @@ test_package_presence() {
         repo_subdir=${pkgname:0:1}
       fi
 
-      repo_pkg_list=$(curl -s -o - http://apt.arvados.org/pool/${D}-dev/main/${repo_subdir}/${pkgname}/)
+      repo_pkg_list=$(curl -s -o - http://apt.arvados.org/${D}/pool/main/${repo_subdir}/${pkgname}/)
       echo "${repo_pkg_list}" |grep -q ${full_pkgname}
       if [ $? -eq 0 ] ; then
         echo "Package $full_pkgname exists upstream, not rebuilding, downloading instead!"
-        curl -s -o "$WORKSPACE/packages/$TARGET/${full_pkgname}" http://apt.arvados.org/pool/${D}-dev/main/${repo_subdir}/${pkgname}/${full_pkgname}
+        curl -s -o "$WORKSPACE/packages/$TARGET/${full_pkgname}" http://apt.arvados.org/${D}/pool/main/${repo_subdir}/${pkgname}/${full_pkgname}
         return 1
       elif test -f "$WORKSPACE/packages/$TARGET/processed/${full_pkgname}" ; then
         echo "Package $full_pkgname exists, not rebuilding!"
index 86ac509626db5806239a4c904257d5f26aa749fa..8317a744a705e614051f16f6f3c908ff85329e31 100644 (file)
@@ -35,10 +35,14 @@ TODO: extract this information based on git commit messages and generate changel
 <div class="releasenotes">
 </notextile>
 
-h2(#main). development main (as of 2020-10-28)
+h2(#main). development main (as of 2020-12-10)
 
 "Upgrading from 2.1.0":#v2_1_0
 
+h3. Changes on the collection's @preserve_version@ attribute semantics
+
+The @preserve_version@ attribute on collections was originally designed to allow clients to persist a preexisting collection version. This forced clients to make 2 requests if the intention is to "make this set of changes in a new version that will be kept", so we have changed the semantics to do just that: When passing @preserve_version=true@ along with other collection updates, the current version is persisted and also the newly created one will be persisted on the next update.
+
 h3. System token requirements
 
 System services now log a warning at startup if any of the system tokens (@ManagementToken@, @SystemRootToken@, and @Collections.BlobSigningKey@) are less than 32 characters, or contain characters other than a-z, A-Z, and 0-9. After upgrading, run @arvados-server config-check@ and update your configuration file if needed to resolve any warnings.
index 4372cc2f5e7dd17bd2c662bf7224cf6fdd5d4c6f..fd4a36f291ae90641a2e48606af66b35311c0780 100644 (file)
@@ -38,7 +38,7 @@ table(table table-bordered table-condensed).
 |is_trashed|boolean|True if @trash_at@ is in the past, false if not.||
 |current_version_uuid|string|UUID of the collection's current version. On new collections, it'll be equal to the @uuid@ attribute.||
 |version|number|Version number, starting at 1 on new collections. This attribute is read-only.||
-|preserve_version|boolean|When set to true on a current version, it will be saved on the next versionable update.||
+|preserve_version|boolean|When set to true on a current version, it will be persisted. When passing @true@ as part of a bigger update call, both current and newly created versions are persisted.||
 |file_count|number|The total number of files in the collection. This attribute is read-only.||
 |file_size_total|number|The sum of the file sizes in the collection. This attribute is read-only.||
 
index cb7102bb3770ebaa74fba78317446c78c7c215a1..81866507bedb8ee870c112d2e3e262df709437e9 100644 (file)
@@ -41,9 +41,9 @@ As root, add the Arvados package repository to your sources.  This command depen
 
 table(table table-bordered table-condensed).
 |_. OS version|_. Command|
-|Debian 10 ("buster")|<notextile><code><span class="userinput">echo "deb http://apt.arvados.org/ buster main" &#x7c; tee /etc/apt/sources.list.d/arvados.list</span></code></notextile>|
-|Ubuntu 18.04 ("bionic")[1]|<notextile><code><span class="userinput">echo "deb http://apt.arvados.org/ bionic main" &#x7c; tee /etc/apt/sources.list.d/arvados.list</span></code></notextile>|
-|Ubuntu 16.04 ("xenial")[1]|<notextile><code><span class="userinput">echo "deb http://apt.arvados.org/ xenial main" &#x7c; tee /etc/apt/sources.list.d/arvados.list</span></code></notextile>|
+|Debian 10 ("buster")|<notextile><code><span class="userinput">echo "deb http://apt.arvados.org/buster buster main" &#x7c; tee /etc/apt/sources.list.d/arvados.list</span></code></notextile>|
+|Ubuntu 18.04 ("bionic")[1]|<notextile><code><span class="userinput">echo "deb http://apt.arvados.org/bionic bionic main" &#x7c; tee /etc/apt/sources.list.d/arvados.list</span></code></notextile>|
+|Ubuntu 16.04 ("xenial")[1]|<notextile><code><span class="userinput">echo "deb http://apt.arvados.org/xenial xenial main" &#x7c; tee /etc/apt/sources.list.d/arvados.list</span></code></notextile>|
 
 
 {% include 'notebox_begin' %}
index 4de87397bca754a57e384c3155d88b82a30983fc..210f5d55119da35ff6e2060fa6dfddeb8099a54d 100644 (file)
@@ -1,2 +1,2 @@
 # apt.arvados.org
-deb http://apt.arvados.org/ buster-dev main
+deb http://apt.arvados.org/buster buster-dev main
index 7882afd01c96235b1fde32767d56a68aeada8d03..153e7298057eff34ccd2e4e8f39e2bcda978adf2 100644 (file)
@@ -1,2 +1,2 @@
 # apt.arvados.org
-deb http://apt.arvados.org/ buster main
+deb http://apt.arvados.org/buster buster main
index 3bb599087eaf513bb5c3f6dc2e32d54108d3db53..d5f458168585ada0d74aa36afa79d5a2bdbf9f31 100644 (file)
@@ -1,2 +1,2 @@
 # apt.arvados.org
-deb http://apt.arvados.org/ buster-testing main
+deb http://apt.arvados.org/buster buster-testing main
index a515ec45cbe7c4b538f44ef7144f730bd86e23df..23bb63547b404301367a98531dd7a19b1d0c6d25 100644 (file)
@@ -14,7 +14,7 @@ RUN apt-key adv --keyserver pool.sks-keyservers.net --recv 1078ECD7 && \
 VOLUME /var/lib/docker
 
 RUN mkdir -p /etc/apt/sources.list.d && \
-    echo deb http://apt.arvados.org/ jessie main > /etc/apt/sources.list.d/apt.arvados.org.list && \
+    echo deb http://apt.arvados.org/jessie jessie main > /etc/apt/sources.list.d/apt.arvados.org.list && \
     apt-get clean && \
     apt-get update && \
     apt-get install -yq --no-install-recommends -o Acquire::Retries=6 \
index 29062c491e3467dc31e5782f754d42023217fb2d..b20dbfcc986f764e78c51dead8a4ec11d427516a 100644 (file)
@@ -103,10 +103,10 @@ func awsKeyFingerprint(pk ssh.PublicKey) (md5fp string, sha1fp string, err error
        sha1pkix := sha1.Sum([]byte(pkix))
        md5fp = ""
        sha1fp = ""
-       for i := 0; i < len(md5pkix); i += 1 {
+       for i := 0; i < len(md5pkix); i++ {
                md5fp += fmt.Sprintf(":%02x", md5pkix[i])
        }
-       for i := 0; i < len(sha1pkix); i += 1 {
+       for i := 0; i < len(sha1pkix); i++ {
                sha1fp += fmt.Sprintf(":%02x", sha1pkix[i])
        }
        return md5fp[1:], sha1fp[1:], nil
@@ -251,7 +251,7 @@ func (instanceSet *ec2InstanceSet) Instances(tags cloud.InstanceTags) (instances
        }
 }
 
-func (az *ec2InstanceSet) Stop() {
+func (instanceSet *ec2InstanceSet) Stop() {
 }
 
 type ec2Instance struct {
index 6a9ad8c15f3db2132bf5c122d8ae639764dbfff7..031166b29151d3023ae386b34ffdb29afc521728 100644 (file)
@@ -820,7 +820,7 @@ func (s *FederationSuite) TestListMultiRemoteContainersPaged(c *check.C) {
                        w.WriteHeader(200)
                        w.Write([]byte(`{"kind": "arvados#containerList", "items": [{"uuid": "zhome-xvhdp-cr6queuedcontnr", "command": ["efg"]}]}`))
                }
-               callCount += 1
+               callCount++
        })).Close()
        req := httptest.NewRequest("GET", fmt.Sprintf("/arvados/v1/containers?count=none&filters=%s",
                url.QueryEscape(fmt.Sprintf(`[["uuid", "in", ["%v", "zhome-xvhdp-cr5queuedcontnr", "zhome-xvhdp-cr6queuedcontnr"]]]`,
@@ -856,7 +856,7 @@ func (s *FederationSuite) TestListMultiRemoteContainersMissing(c *check.C) {
                        w.WriteHeader(200)
                        w.Write([]byte(`{"kind": "arvados#containerList", "items": []}`))
                }
-               callCount += 1
+               callCount++
        })).Close()
        req := httptest.NewRequest("GET", fmt.Sprintf("/arvados/v1/containers?count=none&filters=%s",
                url.QueryEscape(fmt.Sprintf(`[["uuid", "in", ["%v", "zhome-xvhdp-cr5queuedcontnr", "zhome-xvhdp-cr6queuedcontnr"]]]`,
index 6ac8c2e338205ad0fbcf0ae30e019b49b2705116..0388f21bee916ff4a0046971edfa9908553d00b1 100644 (file)
@@ -350,11 +350,10 @@ func (s *IntegrationSuite) TestS3WithFederatedToken(c *check.C) {
                c.Check(err, check.IsNil)
                c.Check(string(buf), check.Matches, `.* `+fmt.Sprintf("%d", len(testText))+` +s3://`+coll.UUID+`/test.txt\n`)
 
-               buf, err = exec.Command("s3cmd", append(s3args, "get", "s3://"+coll.UUID+"/test.txt", c.MkDir()+"/tmpfile")...).CombinedOutput()
+               buf, _ = exec.Command("s3cmd", append(s3args, "get", "s3://"+coll.UUID+"/test.txt", c.MkDir()+"/tmpfile")...).CombinedOutput()
                // Command fails because we don't return Etag header.
-               // c.Check(err, check.IsNil)
                flen := strconv.Itoa(len(testText))
-               c.Check(string(buf), check.Matches, `(?ms).*`+flen+` of `+flen+`.*`)
+               c.Check(string(buf), check.Matches, `(?ms).*`+flen+` (bytes in|of `+flen+`).*`)
        }
 }
 
index 4284542b869a07d713d698fbb7eaa1a243e77ec5..e8dd186050fb200ca575a480ff6d15db1d88adf0 100644 (file)
@@ -44,7 +44,9 @@ func (i *arrayFlags) String() string {
 }
 
 func (i *arrayFlags) Set(value string) error {
-       *i = append(*i, value)
+       for _, s := range strings.Split(value, ",") {
+               *i = append(*i, s)
+       }
        return nil
 }
 
@@ -54,20 +56,26 @@ func parseFlags(prog string, args []string, loader *config.Loader, logger *logru
        flags.Usage = func() {
                fmt.Fprintf(flags.Output(), `
 Usage:
-  %s [options ...]
+  %s [options ...] <uuid> ...
 
        This program analyzes the cost of Arvados container requests. For each uuid
        supplied, it creates a CSV report that lists all the containers used to
        fulfill the container request, together with the machine type and cost of
-       each container.
+       each container. At least one uuid must be specified.
 
        When supplied with the uuid of a container request, it will calculate the
-       cost of that container request and all its children. When suplied with a
-       project uuid or when supplied with multiple container request uuids, it will
-       create a CSV report for each supplied uuid, as well as a CSV file with
-       aggregate cost accounting for all supplied uuids. The aggregate cost report
-       takes container reuse into account: if a container was reused between several
-       container requests, its cost will only be counted once.
+       cost of that container request and all its children.
+
+       When supplied with the uuid of a collection, it will see if there is a
+       container_request uuid in the properties of the collection, and if so, it
+       will calculate the cost of that container request and all its children.
+
+       When supplied with a project uuid or when supplied with multiple container
+       request or collection uuids, it will create a CSV report for each supplied
+       uuid, as well as a CSV file with aggregate cost accounting for all supplied
+       uuids. The aggregate cost report takes container reuse into account: if a
+       container was reused between several container requests, its cost will only
+       be counted once.
 
        To get the node costs, the progam queries the Arvados API for current cost
        data for each node type used. This means that the reported cost always
@@ -86,13 +94,18 @@ Usage:
        In order to get the data for the uuids supplied, the ARVADOS_API_HOST and
        ARVADOS_API_TOKEN environment variables must be set.
 
+       This program prints the total dollar amount from the aggregate cost
+       accounting across all provided uuids on stdout.
+
+       When the '-output' option is specified, a set of CSV files with cost details
+       will be written to the provided directory.
+
 Options:
 `, prog)
                flags.PrintDefaults()
        }
        loglevel := flags.String("log-level", "info", "logging `level` (debug, info, ...)")
-       flags.StringVar(&resultsDir, "output", "", "output `directory` for the CSV reports (required)")
-       flags.Var(&uuids, "uuid", "Toplevel `project or container request` uuid. May be specified more than once. (required)")
+       flags.StringVar(&resultsDir, "output", "", "output `directory` for the CSV reports")
        flags.BoolVar(&cache, "cache", true, "create and use a local disk cache of Arvados objects")
        err = flags.Parse(args)
        if err == flag.ErrHelp {
@@ -103,17 +116,11 @@ Options:
                exitCode = 2
                return
        }
+       uuids = flags.Args()
 
        if len(uuids) < 1 {
                flags.Usage()
-               err = fmt.Errorf("Error: no uuid(s) provided")
-               exitCode = 2
-               return
-       }
-
-       if resultsDir == "" {
-               flags.Usage()
-               err = fmt.Errorf("Error: output directory must be specified")
+               err = fmt.Errorf("error: no uuid(s) provided")
                exitCode = 2
                return
        }
@@ -180,8 +187,8 @@ func addContainerLine(logger *logrus.Logger, node nodeInfo, cr arvados.Container
 
 func loadCachedObject(logger *logrus.Logger, file string, uuid string, object interface{}) (reload bool) {
        reload = true
-       if strings.Contains(uuid, "-j7d0g-") {
-               // We do not cache projects, they have no final state
+       if strings.Contains(uuid, "-j7d0g-") || strings.Contains(uuid, "-4zz18-") {
+               // We do not cache projects or collections, they have no final state
                return
        }
        // See if we have a cached copy of this object
@@ -251,6 +258,8 @@ func loadObject(logger *logrus.Logger, ac *arvados.Client, path string, uuid str
                err = ac.RequestAndDecode(&object, "GET", "arvados/v1/container_requests/"+uuid, nil, nil)
        } else if strings.Contains(uuid, "-dz642-") {
                err = ac.RequestAndDecode(&object, "GET", "arvados/v1/containers/"+uuid, nil, nil)
+       } else if strings.Contains(uuid, "-4zz18-") {
+               err = ac.RequestAndDecode(&object, "GET", "arvados/v1/collections/"+uuid, nil, nil)
        } else {
                err = fmt.Errorf("unsupported object type with UUID %q:\n  %s", uuid, err)
                return
@@ -276,7 +285,7 @@ func loadObject(logger *logrus.Logger, ac *arvados.Client, path string, uuid str
 
 func getNode(arv *arvadosclient.ArvadosClient, ac *arvados.Client, kc *keepclient.KeepClient, cr arvados.ContainerRequest) (node nodeInfo, err error) {
        if cr.LogUUID == "" {
-               err = errors.New("No log collection")
+               err = errors.New("no log collection")
                return
        }
 
@@ -309,7 +318,6 @@ func getNode(arv *arvadosclient.ArvadosClient, ac *arvados.Client, kc *keepclien
 }
 
 func handleProject(logger *logrus.Logger, uuid string, arv *arvadosclient.ArvadosClient, ac *arvados.Client, kc *keepclient.KeepClient, resultsDir string, cache bool) (cost map[string]float64, err error) {
-
        cost = make(map[string]float64)
 
        var project arvados.Group
@@ -366,14 +374,32 @@ func generateCrCsv(logger *logrus.Logger, uuid string, arv *arvadosclient.Arvado
        var tmpTotalCost float64
        var totalCost float64
 
+       var crUUID = uuid
+       if strings.Contains(uuid, "-4zz18-") {
+               // This is a collection, find the associated container request (if any)
+               var c arvados.Collection
+               err = loadObject(logger, ac, uuid, uuid, cache, &c)
+               if err != nil {
+                       return nil, fmt.Errorf("error loading collection object %s: %s", uuid, err)
+               }
+               value, ok := c.Properties["container_request"]
+               if !ok {
+                       return nil, fmt.Errorf("error: collection %s does not have a 'container_request' property", uuid)
+               }
+               crUUID, ok = value.(string)
+               if !ok {
+                       return nil, fmt.Errorf("error: collection %s does not have a 'container_request' property of the string type", uuid)
+               }
+       }
+
        // This is a container request, find the container
        var cr arvados.ContainerRequest
-       err = loadObject(logger, ac, uuid, uuid, cache, &cr)
+       err = loadObject(logger, ac, crUUID, crUUID, cache, &cr)
        if err != nil {
                return nil, fmt.Errorf("error loading cr object %s: %s", uuid, err)
        }
        var container arvados.Container
-       err = loadObject(logger, ac, uuid, cr.ContainerUUID, cache, &container)
+       err = loadObject(logger, ac, crUUID, cr.ContainerUUID, cache, &container)
        if err != nil {
                return nil, fmt.Errorf("error loading container object %s: %s", cr.ContainerUUID, err)
        }
@@ -402,7 +428,7 @@ func generateCrCsv(logger *logrus.Logger, uuid string, arv *arvadosclient.Arvado
        if err != nil {
                return nil, fmt.Errorf("error querying container_requests: %s", err.Error())
        }
-       logger.Infof("Collecting child containers for container request %s", uuid)
+       logger.Infof("Collecting child containers for container request %s", crUUID)
        for _, cr2 := range childCrs.Items {
                logger.Info(".")
                node, err := getNode(arv, ac, kc, cr2)
@@ -411,7 +437,7 @@ func generateCrCsv(logger *logrus.Logger, uuid string, arv *arvadosclient.Arvado
                }
                logger.Debug("\nChild container: " + cr2.ContainerUUID + "\n")
                var c2 arvados.Container
-               err = loadObject(logger, ac, uuid, cr2.ContainerUUID, cache, &c2)
+               err = loadObject(logger, ac, cr.UUID, cr2.ContainerUUID, cache, &c2)
                if err != nil {
                        return nil, fmt.Errorf("error loading object %s: %s", cr2.ContainerUUID, err)
                }
@@ -424,13 +450,15 @@ func generateCrCsv(logger *logrus.Logger, uuid string, arv *arvadosclient.Arvado
 
        csv += "TOTAL,,,,,,,,," + strconv.FormatFloat(totalCost, 'f', 8, 64) + "\n"
 
-       // Write the resulting CSV file
-       fName := resultsDir + "/" + uuid + ".csv"
-       err = ioutil.WriteFile(fName, []byte(csv), 0644)
-       if err != nil {
-               return nil, fmt.Errorf("error writing file with path %s: %s", fName, err.Error())
+       if resultsDir != "" {
+               // Write the resulting CSV file
+               fName := resultsDir + "/" + crUUID + ".csv"
+               err = ioutil.WriteFile(fName, []byte(csv), 0644)
+               if err != nil {
+                       return nil, fmt.Errorf("error writing file with path %s: %s", fName, err.Error())
+               }
+               logger.Infof("\nUUID report in %s\n\n", fName)
        }
-       logger.Infof("\nUUID report in %s\n\n", fName)
 
        return
 }
@@ -440,10 +468,12 @@ func costanalyzer(prog string, args []string, loader *config.Loader, logger *log
        if exitcode != 0 {
                return
        }
-       err = ensureDirectory(logger, resultsDir)
-       if err != nil {
-               exitcode = 3
-               return
+       if resultsDir != "" {
+               err = ensureDirectory(logger, resultsDir)
+               if err != nil {
+                       exitcode = 3
+                       return
+               }
        }
 
        // Arvados Client setup
@@ -474,12 +504,12 @@ func costanalyzer(prog string, args []string, loader *config.Loader, logger *log
                        for k, v := range cost {
                                cost[k] = v
                        }
-               } else if strings.Contains(uuid, "-xvhdp-") {
+               } else if strings.Contains(uuid, "-xvhdp-") || strings.Contains(uuid, "-4zz18-") {
                        // This is a container request
                        var crCsv map[string]float64
                        crCsv, err = generateCrCsv(logger, uuid, arv, ac, kc, resultsDir, cache)
                        if err != nil {
-                               err = fmt.Errorf("Error generating container_request CSV for uuid %s: %s", uuid, err.Error())
+                               err = fmt.Errorf("error generating CSV for uuid %s: %s", uuid, err.Error())
                                exitcode = 2
                                return
                        }
@@ -491,7 +521,11 @@ func costanalyzer(prog string, args []string, loader *config.Loader, logger *log
                        // It is identified by the user uuid. As such, cost analysis for the
                        // "Home" project is not supported by this program. Skip this uuid, but
                        // keep going.
-                       logger.Errorf("Cost analysis is not supported for the 'Home' project: %s", uuid)
+                       logger.Errorf("cost analysis is not supported for the 'Home' project: %s", uuid)
+               } else {
+                       logger.Errorf("this argument does not look like a uuid: %s\n", uuid)
+                       exitcode = 3
+                       return
                }
        }
 
@@ -515,14 +549,20 @@ func costanalyzer(prog string, args []string, loader *config.Loader, logger *log
 
        csv += "TOTAL," + strconv.FormatFloat(total, 'f', 8, 64) + "\n"
 
-       // Write the resulting CSV file
-       aFile := resultsDir + "/" + time.Now().Format("2006-01-02-15-04-05") + "-aggregate-costaccounting.csv"
-       err = ioutil.WriteFile(aFile, []byte(csv), 0644)
-       if err != nil {
-               err = fmt.Errorf("Error writing file with path %s: %s", aFile, err.Error())
-               exitcode = 1
-               return
+       if resultsDir != "" {
+               // Write the resulting CSV file
+               aFile := resultsDir + "/" + time.Now().Format("2006-01-02-15-04-05") + "-aggregate-costaccounting.csv"
+               err = ioutil.WriteFile(aFile, []byte(csv), 0644)
+               if err != nil {
+                       err = fmt.Errorf("error writing file with path %s: %s", aFile, err.Error())
+                       exitcode = 1
+                       return
+               }
+               logger.Infof("Aggregate cost accounting for all supplied uuids in %s\n", aFile)
        }
-       logger.Infof("Aggregate cost accounting for all supplied uuids in %s\n", aFile)
+
+       // Output the total dollar amount on stdout
+       fmt.Fprintf(stdout, "%s\n", strconv.FormatFloat(total, 'f', 8, 64))
+
        return
 }
index 4fab93bf4fadb13919b0346109bc175c96084c93..b1ddf97a36af2c3a7a18de7f4ef7e5a865b60199 100644 (file)
@@ -160,12 +160,57 @@ func (*Suite) TestUsage(c *check.C) {
 
 func (*Suite) TestContainerRequestUUID(c *check.C) {
        var stdout, stderr bytes.Buffer
+       resultsDir := c.MkDir()
        // Run costanalyzer with 1 container request uuid
-       exitcode := Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.CompletedContainerRequestUUID, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       exitcode := Command.RunCommand("costanalyzer.test", []string{"-output", resultsDir, arvadostest.CompletedContainerRequestUUID}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
 
-       uuidReport, err := ioutil.ReadFile("results/" + arvadostest.CompletedContainerRequestUUID + ".csv")
+       uuidReport, err := ioutil.ReadFile(resultsDir + "/" + arvadostest.CompletedContainerRequestUUID + ".csv")
+       c.Assert(err, check.IsNil)
+       c.Check(string(uuidReport), check.Matches, "(?ms).*TOTAL,,,,,,,,,7.01302889")
+       re := regexp.MustCompile(`(?ms).*supplied uuids in (.*?)\n`)
+       matches := re.FindStringSubmatch(stderr.String()) // matches[1] contains a string like 'results/2020-11-02-18-57-45-aggregate-costaccounting.csv'
+
+       aggregateCostReport, err := ioutil.ReadFile(matches[1])
+       c.Assert(err, check.IsNil)
+
+       c.Check(string(aggregateCostReport), check.Matches, "(?ms).*TOTAL,7.01302889")
+}
+
+func (*Suite) TestCollectionUUID(c *check.C) {
+       var stdout, stderr bytes.Buffer
+
+       resultsDir := c.MkDir()
+       // Run costanalyzer with 1 collection uuid, without 'container_request' property
+       exitcode := Command.RunCommand("costanalyzer.test", []string{"-output", resultsDir, arvadostest.FooCollection}, &bytes.Buffer{}, &stdout, &stderr)
+       c.Check(exitcode, check.Equals, 2)
+       c.Assert(stderr.String(), check.Matches, "(?ms).*does not have a 'container_request' property.*")
+
+       // Update the collection, attach a 'container_request' property
+       ac := arvados.NewClientFromEnv()
+       var coll arvados.Collection
+
+       // Update collection record
+       err := ac.RequestAndDecode(&coll, "PUT", "arvados/v1/collections/"+arvadostest.FooCollection, nil, map[string]interface{}{
+               "collection": map[string]interface{}{
+                       "properties": map[string]interface{}{
+                               "container_request": arvadostest.CompletedContainerRequestUUID,
+                       },
+               },
+       })
+       c.Assert(err, check.IsNil)
+
+       stdout.Truncate(0)
+       stderr.Truncate(0)
+
+       // Run costanalyzer with 1 collection uuid
+       resultsDir = c.MkDir()
+       exitcode = Command.RunCommand("costanalyzer.test", []string{"-output", resultsDir, arvadostest.FooCollection}, &bytes.Buffer{}, &stdout, &stderr)
+       c.Check(exitcode, check.Equals, 0)
+       c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
+
+       uuidReport, err := ioutil.ReadFile(resultsDir + "/" + arvadostest.CompletedContainerRequestUUID + ".csv")
        c.Assert(err, check.IsNil)
        c.Check(string(uuidReport), check.Matches, "(?ms).*TOTAL,,,,,,,,,7.01302889")
        re := regexp.MustCompile(`(?ms).*supplied uuids in (.*?)\n`)
@@ -179,16 +224,17 @@ func (*Suite) TestContainerRequestUUID(c *check.C) {
 
 func (*Suite) TestDoubleContainerRequestUUID(c *check.C) {
        var stdout, stderr bytes.Buffer
+       resultsDir := c.MkDir()
        // Run costanalyzer with 2 container request uuids
-       exitcode := Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.CompletedContainerRequestUUID, "-uuid", arvadostest.CompletedContainerRequestUUID2, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       exitcode := Command.RunCommand("costanalyzer.test", []string{"-output", resultsDir, arvadostest.CompletedContainerRequestUUID, arvadostest.CompletedContainerRequestUUID2}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
 
-       uuidReport, err := ioutil.ReadFile("results/" + arvadostest.CompletedContainerRequestUUID + ".csv")
+       uuidReport, err := ioutil.ReadFile(resultsDir + "/" + arvadostest.CompletedContainerRequestUUID + ".csv")
        c.Assert(err, check.IsNil)
        c.Check(string(uuidReport), check.Matches, "(?ms).*TOTAL,,,,,,,,,7.01302889")
 
-       uuidReport2, err := ioutil.ReadFile("results/" + arvadostest.CompletedContainerRequestUUID2 + ".csv")
+       uuidReport2, err := ioutil.ReadFile(resultsDir + "/" + arvadostest.CompletedContainerRequestUUID2 + ".csv")
        c.Assert(err, check.IsNil)
        c.Check(string(uuidReport2), check.Matches, "(?ms).*TOTAL,,,,,,,,,42.27031111")
 
@@ -220,15 +266,16 @@ func (*Suite) TestDoubleContainerRequestUUID(c *check.C) {
        c.Assert(err, check.IsNil)
 
        // Run costanalyzer with the project uuid
-       exitcode = Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.AProjectUUID, "-cache=false", "-log-level", "debug", "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       resultsDir = c.MkDir()
+       exitcode = Command.RunCommand("costanalyzer.test", []string{"-cache=false", "-log-level", "debug", "-output", resultsDir, arvadostest.AProjectUUID}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
 
-       uuidReport, err = ioutil.ReadFile("results/" + arvadostest.CompletedContainerRequestUUID + ".csv")
+       uuidReport, err = ioutil.ReadFile(resultsDir + "/" + arvadostest.CompletedContainerRequestUUID + ".csv")
        c.Assert(err, check.IsNil)
        c.Check(string(uuidReport), check.Matches, "(?ms).*TOTAL,,,,,,,,,7.01302889")
 
-       uuidReport2, err = ioutil.ReadFile("results/" + arvadostest.CompletedContainerRequestUUID2 + ".csv")
+       uuidReport2, err = ioutil.ReadFile(resultsDir + "/" + arvadostest.CompletedContainerRequestUUID2 + ".csv")
        c.Assert(err, check.IsNil)
        c.Check(string(uuidReport2), check.Matches, "(?ms).*TOTAL,,,,,,,,,42.27031111")
 
@@ -243,16 +290,28 @@ func (*Suite) TestDoubleContainerRequestUUID(c *check.C) {
 
 func (*Suite) TestMultipleContainerRequestUUIDWithReuse(c *check.C) {
        var stdout, stderr bytes.Buffer
+       // Run costanalyzer with 2 container request uuids, without output directory specified
+       exitcode := Command.RunCommand("costanalyzer.test", []string{arvadostest.CompletedDiagnosticsContainerRequest1UUID, arvadostest.CompletedDiagnosticsContainerRequest2UUID}, &bytes.Buffer{}, &stdout, &stderr)
+       c.Check(exitcode, check.Equals, 0)
+       c.Assert(stderr.String(), check.Not(check.Matches), "(?ms).*supplied uuids in .*")
+
+       // Check that the total amount was printed to stdout
+       c.Check(stdout.String(), check.Matches, "0.01492030\n")
+
+       stdout.Truncate(0)
+       stderr.Truncate(0)
+
        // Run costanalyzer with 2 container request uuids
-       exitcode := Command.RunCommand("costanalyzer.test", []string{"-uuid", arvadostest.CompletedDiagnosticsContainerRequest1UUID, "-uuid", arvadostest.CompletedDiagnosticsContainerRequest2UUID, "-output", "results"}, &bytes.Buffer{}, &stdout, &stderr)
+       resultsDir := c.MkDir()
+       exitcode = Command.RunCommand("costanalyzer.test", []string{"-output", resultsDir, arvadostest.CompletedDiagnosticsContainerRequest1UUID, arvadostest.CompletedDiagnosticsContainerRequest2UUID}, &bytes.Buffer{}, &stdout, &stderr)
        c.Check(exitcode, check.Equals, 0)
        c.Assert(stderr.String(), check.Matches, "(?ms).*supplied uuids in .*")
 
-       uuidReport, err := ioutil.ReadFile("results/" + arvadostest.CompletedDiagnosticsContainerRequest1UUID + ".csv")
+       uuidReport, err := ioutil.ReadFile(resultsDir + "/" + arvadostest.CompletedDiagnosticsContainerRequest1UUID + ".csv")
        c.Assert(err, check.IsNil)
        c.Check(string(uuidReport), check.Matches, "(?ms).*TOTAL,,,,,,,,,0.00916192")
 
-       uuidReport2, err := ioutil.ReadFile("results/" + arvadostest.CompletedDiagnosticsContainerRequest2UUID + ".csv")
+       uuidReport2, err := ioutil.ReadFile(resultsDir + "/" + arvadostest.CompletedDiagnosticsContainerRequest2UUID + ".csv")
        c.Assert(err, check.IsNil)
        c.Check(string(uuidReport2), check.Matches, "(?ms).*TOTAL,,,,,,,,,0.00588088")
 
index 3a4f3a102b86d8adb1be71d41d85e8b1723f053e..341938354cd963997730eddecd35946f1cb44af9 100644 (file)
@@ -539,7 +539,7 @@ func (runner *ContainerRunner) SetupMounts() (err error) {
                                src = fmt.Sprintf("%s/tmp%d", runner.ArvMountPoint, tmpcount)
                                arvMountCmd = append(arvMountCmd, "--mount-tmp")
                                arvMountCmd = append(arvMountCmd, fmt.Sprintf("tmp%d", tmpcount))
-                               tmpcount += 1
+                               tmpcount++
                        }
                        if mnt.Writable {
                                if bind == runner.Container.OutputPath {
@@ -944,15 +944,15 @@ func (runner *ContainerRunner) AttachStreams() (err error) {
 
        // If stdin mount is provided, attach it to the docker container
        var stdinRdr arvados.File
-       var stdinJson []byte
+       var stdinJSON []byte
        if stdinMnt, ok := runner.Container.Mounts["stdin"]; ok {
                if stdinMnt.Kind == "collection" {
                        var stdinColl arvados.Collection
-                       collId := stdinMnt.UUID
-                       if collId == "" {
-                               collId = stdinMnt.PortableDataHash
+                       collID := stdinMnt.UUID
+                       if collID == "" {
+                               collID = stdinMnt.PortableDataHash
                        }
-                       err = runner.ContainerArvClient.Get("collections", collId, nil, &stdinColl)
+                       err = runner.ContainerArvClient.Get("collections", collID, nil, &stdinColl)
                        if err != nil {
                                return fmt.Errorf("While getting stdin collection: %v", err)
                        }
@@ -966,14 +966,14 @@ func (runner *ContainerRunner) AttachStreams() (err error) {
                                return fmt.Errorf("While getting stdin collection path %v: %v", stdinMnt.Path, err)
                        }
                } else if stdinMnt.Kind == "json" {
-                       stdinJson, err = json.Marshal(stdinMnt.Content)
+                       stdinJSON, err = json.Marshal(stdinMnt.Content)
                        if err != nil {
                                return fmt.Errorf("While encoding stdin json data: %v", err)
                        }
                }
        }
 
-       stdinUsed := stdinRdr != nil || len(stdinJson) != 0
+       stdinUsed := stdinRdr != nil || len(stdinJSON) != 0
        response, err := runner.Docker.ContainerAttach(context.TODO(), runner.ContainerID,
                dockertypes.ContainerAttachOptions{Stream: true, Stdin: stdinUsed, Stdout: true, Stderr: true})
        if err != nil {
@@ -1016,9 +1016,9 @@ func (runner *ContainerRunner) AttachStreams() (err error) {
                        stdinRdr.Close()
                        response.CloseWrite()
                }()
-       } else if len(stdinJson) != 0 {
+       } else if len(stdinJSON) != 0 {
                go func() {
-                       _, err := io.Copy(response.Conn, bytes.NewReader(stdinJson))
+                       _, err := io.Copy(response.Conn, bytes.NewReader(stdinJSON))
                        if err != nil {
                                runner.CrunchLog.Printf("While writing stdin json to docker container: %v", err)
                                runner.stop(nil)
@@ -1814,18 +1814,18 @@ func (command) RunCommand(prog string, args []string, stdin io.Reader, stdout, s
                }
        }
 
-       containerId := flags.Arg(0)
+       containerID := flags.Arg(0)
 
        switch {
        case *detach && !ignoreDetachFlag:
-               return Detach(containerId, prog, args, os.Stdout, os.Stderr)
+               return Detach(containerID, prog, args, os.Stdout, os.Stderr)
        case *kill >= 0:
-               return KillProcess(containerId, syscall.Signal(*kill), os.Stdout, os.Stderr)
+               return KillProcess(containerID, syscall.Signal(*kill), os.Stdout, os.Stderr)
        case *list:
                return ListProcesses(os.Stdout, os.Stderr)
        }
 
-       if containerId == "" {
+       if containerID == "" {
                log.Printf("usage: %s [options] UUID", prog)
                return 1
        }
@@ -1839,14 +1839,14 @@ func (command) RunCommand(prog string, args []string, stdin io.Reader, stdout, s
 
        api, err := arvadosclient.MakeArvadosClient()
        if err != nil {
-               log.Printf("%s: %v", containerId, err)
+               log.Printf("%s: %v", containerID, err)
                return 1
        }
        api.Retries = 8
 
        kc, kcerr := keepclient.MakeKeepClient(api)
        if kcerr != nil {
-               log.Printf("%s: %v", containerId, kcerr)
+               log.Printf("%s: %v", containerID, kcerr)
                return 1
        }
        kc.BlockCache = &keepclient.BlockCache{MaxBlocks: 2}
@@ -1856,21 +1856,21 @@ func (command) RunCommand(prog string, args []string, stdin io.Reader, stdout, s
        // minimum version we want to support.
        docker, dockererr := dockerclient.NewClient(dockerclient.DefaultDockerHost, "1.21", nil, nil)
 
-       cr, err := NewContainerRunner(arvados.NewClientFromEnv(), api, kc, docker, containerId)
+       cr, err := NewContainerRunner(arvados.NewClientFromEnv(), api, kc, docker, containerID)
        if err != nil {
                log.Print(err)
                return 1
        }
        if dockererr != nil {
-               cr.CrunchLog.Printf("%s: %v", containerId, dockererr)
+               cr.CrunchLog.Printf("%s: %v", containerID, dockererr)
                cr.checkBrokenNode(dockererr)
                cr.CrunchLog.Close()
                return 1
        }
 
-       parentTemp, tmperr := cr.MkTempDir("", "crunch-run."+containerId+".")
+       parentTemp, tmperr := cr.MkTempDir("", "crunch-run."+containerID+".")
        if tmperr != nil {
-               log.Printf("%s: %v", containerId, tmperr)
+               log.Printf("%s: %v", containerID, tmperr)
                return 1
        }
 
@@ -1904,7 +1904,7 @@ func (command) RunCommand(prog string, args []string, stdin io.Reader, stdout, s
        }
 
        if runerr != nil {
-               log.Printf("%s: %v", containerId, runerr)
+               log.Printf("%s: %v", containerID, runerr)
                return 1
        }
        return 0
index 02ad1d0e22ef6c607be698ebb3e08caba53f9aee..eb83bbd4106cf51f89dc04e1d9d3a4dc5e5798fc 100644 (file)
@@ -74,7 +74,7 @@ type KeepTestClient struct {
 
 var hwManifest = ". 82ab40c24fc8df01798e57ba66795bb1+841216+Aa124ac75e5168396c73c0a18eda641a4f41791c0@569fa8c3 0:841216:9c31ee32b3d15268a0754e8edc74d4f815ee014b693bc5109058e431dd5caea7.tar\n"
 var hwPDH = "a45557269dcb65a6b78f9ac061c0850b+120"
-var hwImageId = "9c31ee32b3d15268a0754e8edc74d4f815ee014b693bc5109058e431dd5caea7"
+var hwImageID = "9c31ee32b3d15268a0754e8edc74d4f815ee014b693bc5109058e431dd5caea7"
 
 var otherManifest = ". 68a84f561b1d1708c6baff5e019a9ab3+46+Ae5d0af96944a3690becb1decdf60cc1c937f556d@5693216f 0:46:md5sum.txt\n"
 var otherPDH = "a3e8f74c6f101eae01fa08bfb4e49b3a+54"
@@ -207,7 +207,7 @@ func (t *TestDockerClient) ImageLoad(ctx context.Context, input io.Reader, quiet
        if err != nil {
                return dockertypes.ImageLoadResponse{}, err
        }
-       t.imageLoaded = hwImageId
+       t.imageLoaded = hwImageID
        return dockertypes.ImageLoadResponse{Body: ioutil.NopCloser(input)}, nil
 }
 
@@ -425,7 +425,7 @@ func (fw FileWrapper) Sync() error {
 }
 
 func (client *KeepTestClient) ManifestFileReader(m manifest.Manifest, filename string) (arvados.File, error) {
-       if filename == hwImageId+".tar" {
+       if filename == hwImageID+".tar" {
                rdr := ioutil.NopCloser(&bytes.Buffer{})
                client.Called = true
                return FileWrapper{rdr, 1321984}, nil
@@ -447,10 +447,10 @@ func (s *TestSuite) TestLoadImage(c *C) {
        cr.ContainerArvClient = &ArvTestClient{}
        cr.ContainerKeepClient = kc
 
-       _, err = cr.Docker.ImageRemove(nil, hwImageId, dockertypes.ImageRemoveOptions{})
+       _, err = cr.Docker.ImageRemove(nil, hwImageID, dockertypes.ImageRemoveOptions{})
        c.Check(err, IsNil)
 
-       _, _, err = cr.Docker.ImageInspectWithRaw(nil, hwImageId)
+       _, _, err = cr.Docker.ImageInspectWithRaw(nil, hwImageID)
        c.Check(err, NotNil)
 
        cr.Container.ContainerImage = hwPDH
@@ -463,13 +463,13 @@ func (s *TestSuite) TestLoadImage(c *C) {
 
        c.Check(err, IsNil)
        defer func() {
-               cr.Docker.ImageRemove(nil, hwImageId, dockertypes.ImageRemoveOptions{})
+               cr.Docker.ImageRemove(nil, hwImageID, dockertypes.ImageRemoveOptions{})
        }()
 
        c.Check(kc.Called, Equals, true)
-       c.Check(cr.ContainerConfig.Image, Equals, hwImageId)
+       c.Check(cr.ContainerConfig.Image, Equals, hwImageID)
 
-       _, _, err = cr.Docker.ImageInspectWithRaw(nil, hwImageId)
+       _, _, err = cr.Docker.ImageInspectWithRaw(nil, hwImageID)
        c.Check(err, IsNil)
 
        // (2) Test using image that's already loaded
@@ -479,7 +479,7 @@ func (s *TestSuite) TestLoadImage(c *C) {
        err = cr.LoadImage()
        c.Check(err, IsNil)
        c.Check(kc.Called, Equals, false)
-       c.Check(cr.ContainerConfig.Image, Equals, hwImageId)
+       c.Check(cr.ContainerConfig.Image, Equals, hwImageID)
 
 }
 
@@ -771,7 +771,7 @@ func (s *TestSuite) fullRunHelper(c *C, record string, extraMounts []string, exi
 
        s.docker.exitCode = exitCode
        s.docker.fn = fn
-       s.docker.ImageRemove(nil, hwImageId, dockertypes.ImageRemoveOptions{})
+       s.docker.ImageRemove(nil, hwImageID, dockertypes.ImageRemoveOptions{})
 
        api = &ArvTestClient{Container: rec}
        s.docker.api = api
@@ -1131,7 +1131,7 @@ func (s *TestSuite) testStopContainer(c *C, setup func(cr *ContainerRunner)) {
                t.logWriter.Write(dockerLog(1, "foo\n"))
                t.logWriter.Close()
        }
-       s.docker.ImageRemove(nil, hwImageId, dockertypes.ImageRemoveOptions{})
+       s.docker.ImageRemove(nil, hwImageID, dockertypes.ImageRemoveOptions{})
 
        api := &ArvTestClient{Container: rec}
        kc := &KeepTestClient{}
@@ -1618,7 +1618,7 @@ func (s *TestSuite) stdoutErrorRunHelper(c *C, record string, fn func(t *TestDoc
        c.Check(err, IsNil)
 
        s.docker.fn = fn
-       s.docker.ImageRemove(nil, hwImageId, dockertypes.ImageRemoveOptions{})
+       s.docker.ImageRemove(nil, hwImageID, dockertypes.ImageRemoveOptions{})
 
        api = &ArvTestClient{Container: rec}
        kc := &KeepTestClient{}
index febfb1404d32b4f59776a97c45692d7b5c5ab0a9..050894383d757a1e90e79999ef9fa8f2f08b9f01 100644 (file)
@@ -335,7 +335,7 @@ func (arvlog *ArvLogWriter) rateLimit(line []byte, now time.Time) (bool, []byte)
 
                arvlog.bytesLogged += lineSize
                arvlog.logThrottleBytesSoFar += lineSize
-               arvlog.logThrottleLinesSoFar += 1
+               arvlog.logThrottleLinesSoFar++
 
                if arvlog.bytesLogged > crunchLimitLogBytesPerJob {
                        message = fmt.Sprintf("%s Exceeded log limit %d bytes (crunch_limit_log_bytes_per_job). Log will be truncated.",
index fab333b433c04d52663f203d888f745fd02346c3..e3fa3af0bb275279c0d3e5c234da1618b63b40ee 100644 (file)
@@ -23,9 +23,9 @@ type TestTimestamper struct {
        count int
 }
 
-func (this *TestTimestamper) Timestamp(t time.Time) string {
-       this.count += 1
-       t, err := time.ParseInLocation(time.RFC3339Nano, fmt.Sprintf("2015-12-29T15:51:45.%09dZ", this.count), t.Location())
+func (stamper *TestTimestamper) Timestamp(t time.Time) string {
+       stamper.count++
+       t, err := time.ParseInLocation(time.RFC3339Nano, fmt.Sprintf("2015-12-29T15:51:45.%09dZ", stamper.count), t.Location())
        if err != nil {
                panic(err)
        }
index f1fde4f422ce55198742871883f8a0bbd7c682d3..31919b566df81769f791782b4cb3709b468122e8 100644 (file)
@@ -18,6 +18,8 @@ import (
        check "gopkg.in/check.v1"
 )
 
+// LoadTestKey returns a public/private ssh keypair, read from the files
+// identified by the path of the private key.
 func LoadTestKey(c *check.C, fnm string) (ssh.PublicKey, ssh.Signer) {
        rawpubkey, err := ioutil.ReadFile(fnm + ".pub")
        c.Assert(err, check.IsNil)
index a81cf505294e0ce1bbc442da3b844d4cb5c5b073..9ca24312582060d42f6ed878f600c780ae9d5872 100644 (file)
@@ -165,8 +165,6 @@ func (c *command) RunCommand(prog string, args []string, stdin io.Reader, stdout
        return 0
 }
 
-const rfc3339NanoFixed = "2006-01-02T15:04:05.000000000Z07:00"
-
 func getListenAddr(svcs arvados.Services, prog arvados.ServiceName, log logrus.FieldLogger) (arvados.URL, error) {
        svc, ok := svcs.Map()[prog]
        if !ok {
index db3c567eec5ed3da1e95efbe962e162dad573067..c6307b76ab02b79342cfa3395899c5f27ffd5f57 100644 (file)
@@ -23,7 +23,7 @@ func tlsConfigWithCertUpdater(cluster *arvados.Cluster, logger logrus.FieldLogge
 
        key, cert := cluster.TLS.Key, cluster.TLS.Certificate
        if !strings.HasPrefix(key, "file://") || !strings.HasPrefix(cert, "file://") {
-               return nil, errors.New("cannot use TLS certificate: TLS.Key and TLS.Certificate must be specified as file://...")
+               return nil, errors.New("cannot use TLS certificate: TLS.Key and TLS.Certificate must be specified with a 'file://' prefix")
        }
        key, cert = key[7:], cert[7:]
 
index 54602fb54e4b9bee7a167b80127f1eba18064056..d90c618f7a1effa8c3da8031cb98909a1b37df3f 100644 (file)
@@ -210,7 +210,7 @@ func (c *ArvadosClient) CallRaw(method string, resourceType string, uuid string,
                Scheme: scheme,
                Host:   c.ApiServer}
 
-       if resourceType != API_DISCOVERY_RESOURCE {
+       if resourceType != ApiDiscoveryResource {
                u.Path = "/arvados/v1"
        }
 
@@ -400,7 +400,7 @@ func (c *ArvadosClient) List(resource string, parameters Dict, output interface{
        return c.Call("GET", resource, "", "", parameters, output)
 }
 
-const API_DISCOVERY_RESOURCE = "discovery/v1/apis/arvados/v1/rest"
+const ApiDiscoveryResource = "discovery/v1/apis/arvados/v1/rest"
 
 // Discovery returns the value of the given parameter in the discovery
 // document. Returns a non-nil error if the discovery document cannot
@@ -409,7 +409,7 @@ const API_DISCOVERY_RESOURCE = "discovery/v1/apis/arvados/v1/rest"
 func (c *ArvadosClient) Discovery(parameter string) (value interface{}, err error) {
        if len(c.DiscoveryDoc) == 0 {
                c.DiscoveryDoc = make(Dict)
-               err = c.Call("GET", API_DISCOVERY_RESOURCE, "", "", nil, &c.DiscoveryDoc)
+               err = c.Call("GET", ApiDiscoveryResource, "", "", nil, &c.DiscoveryDoc)
                if err != nil {
                        return nil, err
                }
index 7a705eb8c2f6ac9c6a7fe71687f209e6f67fdc1a..954fb710c0596a4580f76bf2a78945e39203f2f6 100644 (file)
@@ -529,6 +529,8 @@ func (m *Manifest) FileSegmentIterByName(filepath string) <-chan *FileSegment {
        return ch
 }
 
+// BlockIterWithDuplicates iterates over the block locators of a manifest.
+//
 // Blocks may appear multiple times within the same manifest if they
 // are used by multiple files. In that case this Iterator will output
 // the same block multiple times.
index bc43b849c3a01dd661c4ba080d83f65f597adde6..bd0e5dc1e686befa94e815396bb5d5a208ca9c1d 100644 (file)
@@ -648,7 +648,7 @@ class KeepClient(object):
 
 
     class KeepWriterThread(threading.Thread):
-        TaskFailed = RuntimeError()
+        class TaskFailed(RuntimeError): pass
 
         def __init__(self, queue, data, data_hash, timeout=None):
             super(KeepClient.KeepWriterThread, self).__init__()
@@ -667,7 +667,7 @@ class KeepClient(object):
                 try:
                     locator, copies = self.do_task(service, service_root)
                 except Exception as e:
-                    if e is not self.TaskFailed:
+                    if not isinstance(e, self.TaskFailed):
                         _logger.exception("Exception in KeepWriterThread")
                     self.queue.write_fail(service)
                 else:
@@ -687,7 +687,7 @@ class KeepClient(object):
                                   self.data_hash,
                                   result['status_code'],
                                   result['body'])
-                raise self.TaskFailed
+                raise self.TaskFailed()
 
             _logger.debug("KeepWriterThread %s succeeded %s+%i %s",
                           str(threading.current_thread()),
index 4562a0654639415ad117b594d1f11ddb7ac36ff9..6a0f7d9f49d0820476449998030b951688aab7a6 100644 (file)
@@ -73,6 +73,7 @@ if not os.path.exists(TEST_TMPDIR):
 my_api_host = None
 _cached_config = {}
 _cached_db_config = {}
+_already_used_port = {}
 
 def find_server_pid(PID_PATH, wait=10):
     now = time.time()
@@ -181,11 +182,15 @@ def find_available_port():
     would take care of the races, and this wouldn't be needed at all.
     """
 
-    sock = socket.socket()
-    sock.bind(('0.0.0.0', 0))
-    port = sock.getsockname()[1]
-    sock.close()
-    return port
+    global _already_used_port
+    while True:
+        sock = socket.socket()
+        sock.bind(('0.0.0.0', 0))
+        port = sock.getsockname()[1]
+        sock.close()
+        if port not in _already_used_port:
+            _already_used_port[port] = True
+            return port
 
 def _wait_until_port_listens(port, timeout=10, warn=True):
     """Wait for a process to start listening on the given port.
index 2e7e2f82b07c024168bd34080f000a91eaea55ac..440ac640169404bc7a0fac4738f55febbd78c0cc 100644 (file)
@@ -43,6 +43,14 @@ class Arvados::V1::CollectionsController < ApplicationController
     super
   end
 
+  def update
+    # preserve_version should be disabled unless explicitly asked otherwise.
+    if !resource_attrs[:preserve_version]
+      resource_attrs[:preserve_version] = false
+    end
+    super
+  end
+
   def find_objects_for_index
     opts = {
       include_trash: params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name),
index b9aba2726f555883d304fac490f050b6177275b4..9e19397994f2c13abd924ba11a40c88fdccb71ec 100644 (file)
@@ -36,7 +36,7 @@ class Arvados::V1::SchemaController < ApplicationController
         # format is YYYYMMDD, must be fixed width (needs to be lexically
         # sortable), updated manually, may be used by clients to
         # determine availability of API server features.
-        revision: "20200331",
+        revision: "20201210",
         source_version: AppVersion.hash,
         sourceVersion: AppVersion.hash, # source_version should be deprecated in the future
         packageVersion: AppVersion.package_version,
index 6b308a231cb7ede8cf50b949da75a861a46219d3..9290e01a1a7a5b4284580615585d963a5201c386 100644 (file)
@@ -394,7 +394,6 @@ class ApiClientAuthorization < ArvadosModel
   end
 
   def log_update
-
     super unless (saved_changes.keys - UNLOGGED_CHANGES).empty?
   end
 end
index 8b549a71ab4fba348ab9279f456595912fb693db..4e7b64cf5374fb38003d70790aebd8caee0931fb 100644 (file)
@@ -62,6 +62,8 @@ class Collection < ArvadosModel
     t.add :file_size_total
   end
 
+  UNLOGGED_CHANGES = ['preserve_version', 'updated_at']
+
   after_initialize do
     @signatures_checked = false
     @computed_pdh_for_manifest_text = false
@@ -269,11 +271,12 @@ class Collection < ArvadosModel
         snapshot = self.dup
         snapshot.uuid = nil # Reset UUID so it's created as a new record
         snapshot.created_at = self.created_at
+        snapshot.modified_at = self.modified_at_was
       end
 
       # Restore requested changes on the current version
       changes.keys.each do |attr|
-        if attr == 'preserve_version' && changes[attr].last == false
+        if attr == 'preserve_version' && changes[attr].last == false && !should_preserve_version
           next # Ignore false assignment, once true it'll be true until next version
         end
         self.attributes = {attr => changes[attr].last}
@@ -285,7 +288,6 @@ class Collection < ArvadosModel
 
       if should_preserve_version
         self.version += 1
-        self.preserve_version = false
       end
 
       yield
@@ -294,14 +296,22 @@ class Collection < ArvadosModel
       if snapshot
         snapshot.attributes = self.syncable_updates
         leave_modified_by_user_alone do
-          act_as_system_user do
-            snapshot.save
+          leave_modified_at_alone do
+            act_as_system_user do
+              snapshot.save
+            end
           end
         end
       end
     end
   end
 
+  def maybe_update_modified_by_fields
+    if !(self.changes.keys - ['updated_at', 'preserve_version']).empty?
+      super
+    end
+  end
+
   def syncable_updates
     updates = {}
     if self.changes.any?
@@ -356,6 +366,7 @@ class Collection < ArvadosModel
 
     idle_threshold = Rails.configuration.Collections.PreserveVersionIfIdle
     if !self.preserve_version_was &&
+      !self.preserve_version &&
       (idle_threshold < 0 ||
         (idle_threshold > 0 && self.modified_at_was > db_current_time-idle_threshold.seconds))
       return false
@@ -739,4 +750,8 @@ class Collection < ArvadosModel
     self.current_version_uuid ||= self.uuid
     true
   end
+
+  def log_update
+    super unless (saved_changes.keys - UNLOGGED_CHANGES).empty?
+  end
 end
diff --git a/services/api/db/migrate/20201202174753_fix_collection_versions_timestamps.rb b/services/api/db/migrate/20201202174753_fix_collection_versions_timestamps.rb
new file mode 100644 (file)
index 0000000..4c56d3d
--- /dev/null
@@ -0,0 +1,17 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'fix_collection_versions_timestamps'
+
+class FixCollectionVersionsTimestamps < ActiveRecord::Migration[5.2]
+  def up
+    # Defined in a function for easy testing.
+    fix_collection_versions_timestamps
+  end
+
+  def down
+    # This migration is not reversible.  However, the results are
+    # backwards compatible.
+  end
+end
index 58c064ac3341ce953d41f83cab0425a0370e5f67..12a28c6c723609bd14b2c20129b8c749695bdc28 100644 (file)
@@ -3186,6 +3186,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200602141328'),
 ('20200914203202'),
 ('20201103170213'),
-('20201105190435');
+('20201105190435'),
+('20201202174753');
 
 
diff --git a/services/api/lib/fix_collection_versions_timestamps.rb b/services/api/lib/fix_collection_versions_timestamps.rb
new file mode 100644 (file)
index 0000000..61da988
--- /dev/null
@@ -0,0 +1,43 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'set'
+
+include CurrentApiClient
+include ArvadosModelUpdates
+
+def fix_collection_versions_timestamps
+  act_as_system_user do
+    uuids = [].to_set
+    # Get UUIDs from collections with more than 1 version
+    Collection.where(version: 2).find_each(batch_size: 100) do |c|
+      uuids.add(c.current_version_uuid)
+    end
+    uuids.each do |uuid|
+      first_pair = true
+      # All versions of a particular collection get fixed together.
+      ActiveRecord::Base.transaction do
+        Collection.where(current_version_uuid: uuid).order(version: :desc).each_cons(2) do |c1, c2|
+          # Skip if the 2 newest versions' modified_at values are separate enough;
+          # this means that this collection doesn't require fixing, allowing for
+          # migration re-runs in case of transient problems.
+          break if first_pair && (c2.modified_at.to_f - c1.modified_at.to_f) > 1
+          first_pair = false
+          # Fix modified_at timestamps by assigning to N-1's value to N.
+          # Special case: the first version's modified_at will be == to created_at
+          leave_modified_by_user_alone do
+            leave_modified_at_alone do
+              c1.modified_at = c2.modified_at
+              c1.save!(validate: false)
+              if c2.version == 1
+                c2.modified_at = c2.created_at
+                c2.save!(validate: false)
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end
\ No newline at end of file
index 767f035b88cb824c47de95ef9571c5531c228c23..1f2eab73afedd748070086e8083f8bccc2256af8 100644 (file)
@@ -21,10 +21,10 @@ collection_owned_by_active:
   portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
   created_at: 2014-02-03T17:22:54Z
-  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_client_uuid: zzzzz-ozdt8-teyxzyd8qllg11h
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
-  modified_at: 2014-02-03T17:22:54Z
-  updated_at: 2014-02-03T17:22:54Z
+  modified_at: 2014-02-03T18:22:54Z
+  updated_at: 2014-02-03T18:22:54Z
   manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
   name: owned_by_active
   version: 2
@@ -43,7 +43,7 @@ collection_owned_by_active_with_file_stats:
   file_count: 1
   file_size_total: 3
   name: owned_by_active_with_file_stats
-  version: 2
+  version: 1
 
 collection_owned_by_active_past_version_1:
   uuid: zzzzz-4zz18-znfnqtbbv4spast
@@ -53,8 +53,8 @@ collection_owned_by_active_past_version_1:
   created_at: 2014-02-03T17:22:54Z
   modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
-  modified_at: 2014-02-03T15:22:54Z
-  updated_at: 2014-02-03T15:22:54Z
+  modified_at: 2014-02-03T18:22:54Z
+  updated_at: 2014-02-03T18:22:54Z
   manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
   name: owned_by_active_version_1
   version: 1
@@ -106,8 +106,8 @@ w_a_z_file:
   created_at: 2015-02-09T10:53:38Z
   modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
-  modified_at: 2015-02-09T10:53:38Z
-  updated_at: 2015-02-09T10:53:38Z
+  modified_at: 2015-02-09T10:55:38Z
+  updated_at: 2015-02-09T10:55:38Z
   manifest_text: ". 4c6c2c0ac8aa0696edd7316a3be5ca3c+5 0:5:w\\040\\141\\040z\n"
   name: "\"w a z\" file"
   version: 2
@@ -120,8 +120,8 @@ w_a_z_file_version_1:
   created_at: 2015-02-09T10:53:38Z
   modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
-  modified_at: 2015-02-09T10:53:38Z
-  updated_at: 2015-02-09T10:53:38Z
+  modified_at: 2015-02-09T10:55:38Z
+  updated_at: 2015-02-09T10:55:38Z
   manifest_text: ". 4d20280d5e516a0109768d49ab0f3318+3 0:3:waz\n"
   name: "waz file"
   version: 1
index c025394bc1f33616684be72861862126642d95c4..1ca2dd1dc109857e6987fdaa32caad2b04a52f8b 100644 (file)
@@ -1145,7 +1145,7 @@ EOS
   end
 
   [:admin, :active].each do |user|
-    test "get trashed collection via filters and #{user} user" do
+    test "get trashed collection via filters and #{user} user without including its past versions" do
       uuid = 'zzzzz-4zz18-mto52zx1s7sn3ih' # expired_collection
       authorize_with user
       get :index, params: {
index 86195fba750877af031abc92d451570a567ac096..73cbad64303391e82ef593d7a9cffc080ae6084f 100644 (file)
@@ -495,4 +495,82 @@ class CollectionsApiTest < ActionDispatch::IntegrationTest
     assert_equal Hash, json_response['properties'].class, 'Collection properties attribute should be of type hash'
     assert_equal 'value_1', json_response['properties']['property_1']
   end
+
+  test "update collection with versioning enabled and using preserve_version" do
+    Rails.configuration.Collections.CollectionVersioning = true
+    Rails.configuration.Collections.PreserveVersionIfIdle = -1 # Disable auto versioning
+
+    signed_manifest = Collection.sign_manifest(". bad42fa702ae3ea7d888fef11b46f450+44 0:44:my_test_file.txt\n", api_token(:active))
+    post "/arvados/v1/collections",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection',
+          manifest_text: signed_manifest,
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_not_nil json_response['uuid']
+    assert_equal 1, json_response['version']
+    assert_equal false, json_response['preserve_version']
+
+    # Versionable update including preserve_version=true should create a new
+    # version that will also be persisted.
+    put "/arvados/v1/collections/#{json_response['uuid']}",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection v2',
+          preserve_version: true,
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_equal 2, json_response['version']
+    assert_equal true, json_response['preserve_version']
+
+    # 2nd versionable update including preserve_version=true should create a new
+    # version that will also be persisted.
+    put "/arvados/v1/collections/#{json_response['uuid']}",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection v3',
+          preserve_version: true,
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_equal 3, json_response['version']
+    assert_equal true, json_response['preserve_version']
+
+    # 3rd versionable update without including preserve_version should create a new
+    # version that will have its preserve_version attr reset to false.
+    put "/arvados/v1/collections/#{json_response['uuid']}",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection v4',
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_equal 4, json_response['version']
+    assert_equal false, json_response['preserve_version']
+
+    # 4th versionable update without including preserve_version=true should NOT
+    # create a new version.
+    put "/arvados/v1/collections/#{json_response['uuid']}",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection v5?',
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_equal 4, json_response['version']
+    assert_equal false, json_response['preserve_version']
+  end
 end
index 48cae5afee92622fcda41c2b48e89761f54ac80d..916ca095872db7a3a80d59799654dc32504e1f2b 100644 (file)
@@ -4,6 +4,7 @@
 
 require 'test_helper'
 require 'sweep_trashed_objects'
+require 'fix_collection_versions_timestamps'
 
 class CollectionTest < ActiveSupport::TestCase
   include DbCurrentTime
@@ -187,9 +188,9 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
-  test "preserve_version=false assignment is ignored while being true and not producing a new version" do
+  test "preserve_version updates" do
     Rails.configuration.Collections.CollectionVersioning = true
-    Rails.configuration.Collections.PreserveVersionIfIdle = 3600
+    Rails.configuration.Collections.PreserveVersionIfIdle = -1 # disabled
     act_as_user users(:active) do
       # Set up initial collection
       c = create_collection 'foo', Encoding::US_ASCII
@@ -198,28 +199,61 @@ class CollectionTest < ActiveSupport::TestCase
       assert_equal false, c.preserve_version
       # This update shouldn't produce a new version, as the idle time is not up
       c.update_attributes!({
-        'name' => 'bar',
-        'preserve_version' => true
+        'name' => 'bar'
       })
       c.reload
       assert_equal 1, c.version
       assert_equal 'bar', c.name
+      assert_equal false, c.preserve_version
+      # This update should produce a new version, even if the idle time is not up
+      # and also keep the preserve_version=true flag to persist it.
+      c.update_attributes!({
+        'name' => 'baz',
+        'preserve_version' => true
+      })
+      c.reload
+      assert_equal 2, c.version
+      assert_equal 'baz', c.name
       assert_equal true, c.preserve_version
       # Make sure preserve_version is not disabled after being enabled, unless
       # a new version is created.
+      # This is a non-versionable update
       c.update_attributes!({
         'preserve_version' => false,
         'replication_desired' => 2
       })
       c.reload
-      assert_equal 1, c.version
+      assert_equal 2, c.version
       assert_equal 2, c.replication_desired
       assert_equal true, c.preserve_version
-      c.update_attributes!({'name' => 'foobar'})
+      # This is a versionable update
+      c.update_attributes!({
+        'preserve_version' => false,
+        'name' => 'foobar'
+      })
       c.reload
-      assert_equal 2, c.version
+      assert_equal 3, c.version
       assert_equal false, c.preserve_version
       assert_equal 'foobar', c.name
+      # Flipping only 'preserve_version' to true doesn't create a new version
+      c.update_attributes!({'preserve_version' => true})
+      c.reload
+      assert_equal 3, c.version
+      assert_equal true, c.preserve_version
+    end
+  end
+
+  test "preserve_version updates don't change modified_at timestamp" do
+    act_as_user users(:active) do
+      c = create_collection 'foo', Encoding::US_ASCII
+      assert c.valid?
+      assert_equal false, c.preserve_version
+      modified_at = c.modified_at.to_f
+      c.update_attributes!({'preserve_version' => true})
+      c.reload
+      assert_equal true, c.preserve_version
+      assert_equal modified_at, c.modified_at.to_f,
+        'preserve_version updates should not trigger modified_at changes'
     end
   end
 
@@ -334,6 +368,7 @@ class CollectionTest < ActiveSupport::TestCase
       # Set up initial collection
       c = create_collection 'foo', Encoding::US_ASCII
       assert c.valid?
+      original_version_modified_at = c.modified_at.to_f
       # Make changes so that a new version is created
       c.update_attributes!({'name' => 'bar'})
       c.reload
@@ -344,9 +379,7 @@ class CollectionTest < ActiveSupport::TestCase
 
       version_creation_datetime = c_old.modified_at.to_f
       assert_equal c.created_at.to_f, c_old.created_at.to_f
-      # Current version is updated just a few milliseconds before the version is
-      # saved on the database.
-      assert_operator c.modified_at.to_f, :<, version_creation_datetime
+      assert_equal original_version_modified_at, version_creation_datetime
 
       # Make update on current version so old version get the attribute synced;
       # its modified_at should not change.
@@ -361,6 +394,29 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  # Bug #17152 - This test relies on fixtures simulating the problem.
+  test "migration fixing collection versions' modified_at timestamps" do
+    versioned_collection_fixtures = [
+      collections(:w_a_z_file).uuid,
+      collections(:collection_owned_by_active).uuid
+    ]
+    versioned_collection_fixtures.each do |uuid|
+      cols = Collection.where(current_version_uuid: uuid).order(version: :desc)
+      assert_equal cols.size, 2
+      # cols[0] -> head version // cols[1] -> old version
+      assert_operator (cols[0].modified_at.to_f - cols[1].modified_at.to_f), :==, 0
+      assert cols[1].modified_at != cols[1].created_at
+    end
+    fix_collection_versions_timestamps
+    versioned_collection_fixtures.each do |uuid|
+      cols = Collection.where(current_version_uuid: uuid).order(version: :desc)
+      assert_equal cols.size, 2
+      # cols[0] -> head version // cols[1] -> old version
+      assert_operator (cols[0].modified_at.to_f - cols[1].modified_at.to_f), :>, 1
+      assert_operator cols[1].modified_at, :==, cols[1].created_at
+    end
+  end
+
   test "past versions should not be directly updatable" do
     Rails.configuration.Collections.CollectionVersioning = true
     Rails.configuration.Collections.PreserveVersionIfIdle = 0
index 76d78f9eaa6cf14ebd83b8b7b138e3f94c69dff5..66c8c8d923d06f9f745f0c393f29ff99ce605f84 100644 (file)
@@ -228,6 +228,20 @@ class LogTest < ActiveSupport::TestCase
     assert_logged(auth, :update)
   end
 
+  test "don't log changes only to Collection.preserve_version" do
+    set_user_from_auth :admin_trustedclient
+    col = collections(:collection_owned_by_active)
+    start_log_count = get_logs_about(col).size
+    assert_equal false, col.preserve_version
+    col.preserve_version = true
+    col.save!
+    assert_equal(start_log_count, get_logs_about(col).size,
+                 "log count changed after updating Collection.preserve_version")
+    col.name = 'updated by admin'
+    col.save!
+    assert_logged(col, :update)
+  end
+
   test "token isn't included in ApiClientAuthorization logs" do
     set_user_from_auth :admin_trustedclient
     auth = ApiClientAuthorization.new
index d91ae05a1158fc3f327162d4507914fb8cebed20..25f1cda3b25d17d812c33471967573dab68e4d56 100644 (file)
@@ -38,7 +38,7 @@ Installing on Debian systems
 
 1. Add this Arvados repository to your sources list::
 
-     deb http://apt.arvados.org/ jessie main
+     deb http://apt.arvados.org/buster buster main
 
 2. Update your package list.
 
index ab1bc080bd5928d5d03fe3d6764beb6bae167dce..2d6fb78f8098a7752a2e9075f8ea84ca537c445f 100644 (file)
@@ -296,27 +296,32 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
        }
 
        formToken := r.FormValue("api_token")
-       if formToken != "" && r.Header.Get("Origin") != "" && attachment && r.URL.Query().Get("api_token") == "" {
-               // The client provided an explicit token in the POST
-               // body. The Origin header indicates this *might* be
-               // an AJAX request, in which case redirect-with-cookie
-               // won't work: we should just serve the content in the
-               // POST response. This is safe because:
+       origin := r.Header.Get("Origin")
+       cors := origin != "" && !strings.HasSuffix(origin, "://"+r.Host)
+       safeAjax := cors && (r.Method == http.MethodGet || r.Method == http.MethodHead)
+       safeAttachment := attachment && r.URL.Query().Get("api_token") == ""
+       if formToken == "" {
+               // No token to use or redact.
+       } else if safeAjax || safeAttachment {
+               // If this is a cross-origin request, the URL won't
+               // appear in the browser's address bar, so
+               // substituting a clipboard-safe URL is pointless.
+               // Redirect-with-cookie wouldn't work anyway, because
+               // it's not safe to allow third-party use of our
+               // cookie.
                //
-               // * We're supplying an attachment, not inline
-               //   content, so we don't need to convert the POST to
-               //   a GET and avoid the "really resubmit form?"
-               //   problem.
-               //
-               // * The token isn't embedded in the URL, so we don't
-               //   need to worry about bookmarks and copy/paste.
+               // If we're supplying an attachment, we don't need to
+               // convert POST to GET to avoid the "really resubmit
+               // form?" problem, so provided the token isn't
+               // embedded in the URL, there's no reason to do
+               // redirect-with-cookie in this case either.
                reqTokens = append(reqTokens, formToken)
-       } else if formToken != "" && browserMethod[r.Method] {
-               // The client provided an explicit token in the query
-               // string, or a form in POST body. We must put the
-               // token in an HttpOnly cookie, and redirect to the
-               // same URL with the query param redacted and method =
-               // GET.
+       } else if browserMethod[r.Method] {
+               // If this is a page view, and the client provided a
+               // token via query string or POST body, we must put
+               // the token in an HttpOnly cookie, and redirect to an
+               // equivalent URL with the query param redacted and
+               // method = GET.
                h.seeOtherWithCookie(w, r, "", credentialsOK)
                return
        }
@@ -768,6 +773,7 @@ func (h *handler) seeOtherWithCookie(w http.ResponseWriter, r *http.Request, loc
                        Value:    auth.EncodeTokenCookie([]byte(formToken)),
                        Path:     "/",
                        HttpOnly: true,
+                       SameSite: http.SameSiteLaxMode,
                })
        }
 
index 8e2e05c76184823d0a550bce92b93473879ec254..5291efeb822a4a2fe22af022cf15208d0ee1ba7f 100644 (file)
@@ -583,6 +583,25 @@ func (s *IntegrationSuite) TestXHRNoRedirect(c *check.C) {
        c.Check(resp.Code, check.Equals, http.StatusOK)
        c.Check(resp.Body.String(), check.Equals, "foo")
        c.Check(resp.Header().Get("Access-Control-Allow-Origin"), check.Equals, "*")
+
+       // GET + Origin header is representative of both AJAX GET
+       // requests and inline images via <IMG crossorigin="anonymous"
+       // src="...">.
+       u.RawQuery = "api_token=" + url.QueryEscape(arvadostest.ActiveTokenV2)
+       req = &http.Request{
+               Method:     "GET",
+               Host:       u.Host,
+               URL:        u,
+               RequestURI: u.RequestURI(),
+               Header: http.Header{
+                       "Origin": {"https://origin.example"},
+               },
+       }
+       resp = httptest.NewRecorder()
+       s.testServer.Handler.ServeHTTP(resp, req)
+       c.Check(resp.Code, check.Equals, http.StatusOK)
+       c.Check(resp.Body.String(), check.Equals, "foo")
+       c.Check(resp.Header().Get("Access-Control-Allow-Origin"), check.Equals, "*")
 }
 
 func (s *IntegrationSuite) testVhostRedirectTokenToCookie(c *check.C, method, hostPath, queryString, contentType, reqBody string, expectStatus int, expectRespBody string) *httptest.ResponseRecorder {
index 97201d2922116bf7db8cd8368557b4247093a074..1c84976d2b1bf26622ba67619438f4536e6551f0 100644 (file)
@@ -75,6 +75,8 @@ func s3querystring(u *url.URL) string {
        return strings.Join(keys, "&")
 }
 
+var reMultipleSlashChars = regexp.MustCompile(`//+`)
+
 func s3stringToSign(alg, scope, signedHeaders string, r *http.Request) (string, error) {
        timefmt, timestr := "20060102T150405Z", r.Header.Get("X-Amz-Date")
        if timestr == "" {
@@ -97,7 +99,10 @@ func s3stringToSign(alg, scope, signedHeaders string, r *http.Request) (string,
                }
        }
 
-       canonicalRequest := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n%s", r.Method, r.URL.EscapedPath(), s3querystring(r.URL), canonicalHeaders, signedHeaders, r.Header.Get("X-Amz-Content-Sha256"))
+       normalizedURL := *r.URL
+       normalizedURL.RawPath = ""
+       normalizedURL.Path = reMultipleSlashChars.ReplaceAllString(normalizedURL.Path, "/")
+       canonicalRequest := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n%s", r.Method, normalizedURL.EscapedPath(), s3querystring(r.URL), canonicalHeaders, signedHeaders, r.Header.Get("X-Amz-Content-Sha256"))
        ctxlog.FromContext(r.Context()).Debugf("s3stringToSign: canonicalRequest %s", canonicalRequest)
        return fmt.Sprintf("%s\n%s\n%s\n%s", alg, r.Header.Get("X-Amz-Date"), scope, hashdigest(sha256.New(), canonicalRequest)), nil
 }
@@ -259,7 +264,7 @@ func (h *handler) serveS3(w http.ResponseWriter, r *http.Request) bool {
                bucketName = strings.SplitN(strings.TrimPrefix(r.URL.Path, "/"), "/", 2)[0]
                objectNameGiven = strings.Count(strings.TrimSuffix(r.URL.Path, "/"), "/") > 1
        }
-       fspath += r.URL.Path
+       fspath += reMultipleSlashChars.ReplaceAllString(r.URL.Path, "/")
 
        switch {
        case r.Method == http.MethodGet && !objectNameGiven:
index a6aab357e301e4b4703f2c72ef4a1a490ab65766..52ef79509759ecbafe57b75f085d5d3f57c7940e 100644 (file)
@@ -7,6 +7,7 @@ package main
 import (
        "bytes"
        "crypto/rand"
+       "crypto/sha256"
        "fmt"
        "io/ioutil"
        "net/http"
@@ -202,6 +203,11 @@ func (s *IntegrationSuite) testS3GetObject(c *check.C, bucket *s3.Bucket, prefix
        c.Check(err, check.IsNil)
        c.Check(resp.StatusCode, check.Equals, http.StatusOK)
        c.Check(resp.ContentLength, check.Equals, int64(4))
+
+       // HeadObject with superfluous leading slashes
+       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) {
@@ -228,6 +234,18 @@ func (s *IntegrationSuite) testS3PutObjectSuccess(c *check.C, bucket *s3.Bucket,
                        path:        "newdir/newfile",
                        size:        1 << 26,
                        contentType: "application/octet-stream",
+               }, {
+                       path:        "/aaa",
+                       size:        2,
+                       contentType: "application/octet-stream",
+               }, {
+                       path:        "//bbb",
+                       size:        2,
+                       contentType: "application/octet-stream",
+               }, {
+                       path:        "ccc//",
+                       size:        0,
+                       contentType: "application/x-directory",
                }, {
                        path:        "newdir1/newdir2/newfile",
                        size:        0,
@@ -243,9 +261,14 @@ func (s *IntegrationSuite) testS3PutObjectSuccess(c *check.C, bucket *s3.Bucket,
                objname := prefix + trial.path
 
                _, err := bucket.GetReader(objname)
+               if !c.Check(err, check.NotNil) {
+                       continue
+               }
                c.Check(err.(*s3.Error).StatusCode, check.Equals, 404)
                c.Check(err.(*s3.Error).Code, check.Equals, `NoSuchKey`)
-               c.Assert(err, check.ErrorMatches, `The specified key does not exist.`)
+               if !c.Check(err, check.ErrorMatches, `The specified key does not exist.`) {
+                       continue
+               }
 
                buf := make([]byte, trial.size)
                rand.Read(buf)
@@ -363,14 +386,6 @@ func (s *IntegrationSuite) TestS3ProjectPutObjectFailure(c *check.C) {
 func (s *IntegrationSuite) testS3PutObjectFailure(c *check.C, bucket *s3.Bucket, prefix string) {
        s.testServer.Config.cluster.Collections.S3FolderObjects = false
 
-       // Can't use V4 signature for these tests, because
-       // double-slash is incorrectly cleaned by the aws.V4Signature,
-       // resulting in a "bad signature" error. (Cleaning the path is
-       // appropriate for other services, but not in S3 where object
-       // names "foo//bar" and "foo/bar" are semantically different.)
-       bucket.S3.Auth = *(aws.NewAuth(arvadostest.ActiveToken, "none", "", time.Now().Add(time.Hour)))
-       bucket.S3.Signature = aws.V2Signature
-
        var wg sync.WaitGroup
        for _, trial := range []struct {
                path string
@@ -393,8 +408,6 @@ func (s *IntegrationSuite) testS3PutObjectFailure(c *check.C, bucket *s3.Bucket,
                        path: "/",
                }, {
                        path: "//",
-               }, {
-                       path: "foo//bar",
                }, {
                        path: "",
                },
@@ -441,6 +454,17 @@ func (stage *s3stage) writeBigDirs(c *check.C, dirs int, filesPerDir int) {
        c.Assert(fs.Sync(), check.IsNil)
 }
 
+func (s *IntegrationSuite) sign(c *check.C, req *http.Request, key, secret string) {
+       scope := "20200202/region/service/aws4_request"
+       signedHeaders := "date"
+       req.Header.Set("Date", time.Now().UTC().Format(time.RFC1123))
+       stringToSign, err := s3stringToSign(s3SignAlgorithm, scope, signedHeaders, req)
+       c.Assert(err, check.IsNil)
+       sig, err := s3signature(secret, scope, signedHeaders, stringToSign)
+       c.Assert(err, check.IsNil)
+       req.Header.Set("Authorization", s3SignAlgorithm+" Credential="+key+"/"+scope+", SignedHeaders="+signedHeaders+", Signature="+sig)
+}
+
 func (s *IntegrationSuite) TestS3VirtualHostStyleRequests(c *check.C) {
        stage := s.s3setup(c)
        defer stage.teardown(c)
@@ -487,12 +511,29 @@ func (s *IntegrationSuite) TestS3VirtualHostStyleRequests(c *check.C) {
                        responseCode:   http.StatusOK,
                        responseRegexp: []string{`boop`},
                },
+               {
+                       url:          "https://" + stage.projbucket.Name + ".example.com/" + stage.coll.Name + "//boop",
+                       method:       "GET",
+                       responseCode: http.StatusNotFound,
+               },
+               {
+                       url:          "https://" + stage.projbucket.Name + ".example.com/" + stage.coll.Name + "//boop",
+                       method:       "PUT",
+                       body:         "boop",
+                       responseCode: http.StatusOK,
+               },
+               {
+                       url:            "https://" + stage.projbucket.Name + ".example.com/" + stage.coll.Name + "//boop",
+                       method:         "GET",
+                       responseCode:   http.StatusOK,
+                       responseRegexp: []string{`boop`},
+               },
        } {
                url, err := url.Parse(trial.url)
                c.Assert(err, check.IsNil)
                req, err := http.NewRequest(trial.method, url.String(), bytes.NewReader([]byte(trial.body)))
                c.Assert(err, check.IsNil)
-               req.Header.Set("Authorization", "AWS "+arvadostest.ActiveTokenV2+":none")
+               s.sign(c, req, arvadostest.ActiveTokenUUID, arvadostest.ActiveToken)
                rr := httptest.NewRecorder()
                s.testServer.Server.Handler.ServeHTTP(rr, req)
                resp := rr.Result()
@@ -505,6 +546,38 @@ func (s *IntegrationSuite) TestS3VirtualHostStyleRequests(c *check.C) {
        }
 }
 
+func (s *IntegrationSuite) TestS3NormalizeURIForSignature(c *check.C) {
+       stage := s.s3setup(c)
+       defer stage.teardown(c)
+       for _, trial := range []struct {
+               rawPath        string
+               normalizedPath string
+       }{
+               {"/foo", "/foo"},             // boring case
+               {"/foo%5fbar", "/foo_bar"},   // _ must not be escaped
+               {"/foo%2fbar", "/foo/bar"},   // / must not be escaped
+               {"/(foo)", "/%28foo%29"},     // () must be escaped
+               {"/foo%5bbar", "/foo%5Bbar"}, // %XX must be uppercase
+       } {
+               date := time.Now().UTC().Format("20060102T150405Z")
+               scope := "20200202/fakeregion/S3/aws4_request"
+               canonicalRequest := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n%s", "GET", trial.normalizedPath, "", "host:host.example.com\n", "host", "")
+               c.Logf("canonicalRequest %q", canonicalRequest)
+               expect := fmt.Sprintf("%s\n%s\n%s\n%s", s3SignAlgorithm, date, scope, hashdigest(sha256.New(), canonicalRequest))
+               c.Logf("expected stringToSign %q", expect)
+
+               req, err := http.NewRequest("GET", "https://host.example.com"+trial.rawPath, nil)
+               req.Header.Set("X-Amz-Date", date)
+               req.Host = "host.example.com"
+
+               obtained, err := s3stringToSign(s3SignAlgorithm, scope, "host", req)
+               if !c.Check(err, check.IsNil) {
+                       continue
+               }
+               c.Check(obtained, check.Equals, expect)
+       }
+}
+
 func (s *IntegrationSuite) TestS3GetBucketVersioning(c *check.C) {
        stage := s.s3setup(c)
        defer stage.teardown(c)
index a180b43630471f0c92ded944e6e1b8290ece4245..96f3666cda9ee498970ebf0c5ce29badd0fc18d8 100755 (executable)
@@ -205,6 +205,7 @@ run() {
               --publish=8900:8900
               --publish=9000:9000
               --publish=9002:9002
+              --publish=9004:9004
               --publish=25101:25101
               --publish=8001:8001
               --publish=8002:8002
index 73c7b9dac05d73144a668d1d5b7c8e538ffcc1af..78cbccdb98fa6856b464c0411e086d11f94d2c54 100644 (file)
@@ -29,7 +29,7 @@ LSB_RELEASE_CODENAME=${TMP_LSB//[$'\t\r\n ']}
 
 # Add the arvados apt repository
 echo "# apt.arvados.org" |$SUDO tee --append /etc/apt/sources.list.d/apt.arvados.org.list
-echo "deb http://apt.arvados.org/ $LSB_RELEASE_CODENAME${REPOSUFFIX} main" |$SUDO tee --append /etc/apt/sources.list.d/apt.arvados.org.list
+echo "deb http://apt.arvados.org/$LSB_RELEASE_CODENAME $LSB_RELEASE_CODENAME${REPOSUFFIX} main" |$SUDO tee --append /etc/apt/sources.list.d/apt.arvados.org.list
 
 # Add the arvados signing key
 cat /tmp/1078ECD7.asc | $SUDO apt-key add -
index a4d55c2216dff7064be0f028473395b0ef97a7f5..31266c1b8f11ab5c02ccca6989970b3b3efa6975 100755 (executable)
@@ -49,7 +49,7 @@ VERSION="latest"
 # Usually there's no need to modify things below this line
 
 # Formulas versions
-ARVADOS_TAG="v1.1.3"
+ARVADOS_TAG="v1.1.4"
 POSTGRES_TAG="v0.41.3"
 NGINX_TAG="v2.4.0"
 DOCKER_TAG="v1.0.0"