storage: introduce requesterPays #2392

stephenplusplus · 2017-06-15T03:00:30Z

Fixes #2371

To Dos

Bucket Operations

`storage.createBucket({ requesterPays: true })`

`bucket.create({ requesterPays: true })`

`bucket.disableRequesterPays()`

`bucket.enableRequesterPays()`

`bucket.combine(sourceFiles, destinationFile, { userProject: 'project-id' })`

`bucket.createChannel(id, config, { userProject: 'project-id' })`

`bucket.delete({ userProject: 'project-id' })`

`bucket.deleteFiles({ userProject: 'project-id' })`

`bucket.exists({ userProject: 'project-id' })` // calls `bucket.getMetadata`

`bucket.get({ userProject: 'project-id' })` // calls `bucket.getMetadata`

`bucket.getFiles({ userProject: 'project-id' })`

`bucket.getFilesStream({ userProject: 'project-id' })`

`bucket.getMetadata({ userProject: 'project-id' })`

`bucket.makePrivate({ userProject: 'project-id' })` // calls `bucket.getFiles` and `bucket.setMetadata`

`bucket.setMetadata({ userProject: 'project-id' })`

`bucket.upload('file.txt', { userProject: 'project-id' })` // calls `file.createWriteStream`

File Operations

`file.copy('newfile.txt', { userProject: 'project-id' })`

`file.createReadStream({ userProject: 'project-id' })`

`file.createResumableUpload({ userProject: 'project-id' })`

`file.createWriteStream({ userProject: 'project-id' })`

`file.delete({ userProject: 'project-id' })`

`file.download({ userProject: 'project-id' })` // calls `file.createReadStream`

`file.exists({ userProject: 'project-id' })` // calls `file.getMetadata`

`file.get({ userProject: 'project-id' })` // calls `file.getMetadata`

`file.getMetadata({ userProject: 'project-id' })`

`file.makePrivate({ userProject: 'project-id' })` // calls `file.setMetadata`

`file.move({ userProject: 'project-id' })` // calls `file.copy`

`file.save(data, { userProject: 'project-id' })` // calls `file.createWriteStream`

`file.setMetadata(metadata, { userProject: 'project-id' })`

`file.setStorageClass(storageClass, { userProject: 'project-id' })` // calls `file.copy`

coveralls · 2017-06-16T19:29:51Z

Coverage decreased (-0.001%) to 99.987% when pulling c66bb60 on stephenplusplus:spp--2371 into 02afcef on GoogleCloudPlatform:master.

stephenplusplus · 2017-06-16T19:35:49Z

Storage coverage is 100% - the drop is just from #2342.

stephenplusplus · 2017-06-23T12:50:04Z

I've run into a problem when writing the system tests. A series of requests are outlined here:

USER A: nth-circlet-705
USER B: stephen-sawchuk@gcloud-node-whitelist-ci-tests.iam.gserviceaccount.com

// USER A
// enables requesterPays on a bucket
{
  "method": "PATCH",
  "uri": "https://www.googleapis.com/storage/v1/b/gcloud-test-155fe610-5786-11e7-afc5-0991cf1123b2",
  "json": {
    "billing": {
      "requesterPays": true
    }
  }
}

// USER A
// adds IAM permissions to USER B
{
  "method": "PUT",
  "uri": "https://www.googleapis.com/storage/v1/b/gcloud-test-155fe610-5786-11e7-afc5-0991cf1123b2/iam",
  "json": {
    "resourceId": "projects/_/buckets/gcloud-test-155fe610-5786-11e7-afc5-0991cf1123b2",
    "kind": "storage#policy",
    "bindings": [
      {
        "role": "roles/storage.legacyBucketOwner",
        "members": [
          "projectEditor:nth-circlet-705",
          "projectOwner:nth-circlet-705"
        ]
      },
      {
        "role": "roles/storage.legacyBucketReader",
        "members": [
          "projectViewer:nth-circlet-705"
        ]
      },
      {
        "role": "roles/storage.legacyBucketWriter",
        "members": [
          "serviceAccount:stephen-sawchuk@gcloud-node-whitelist-ci-tests.iam.gserviceaccount.com"
        ]
      }
    ],
    "etag": "CAI="
  }
}

// USER B
// deletes bucket
{
  "method": "DELETE",
  "uri": "https://www.googleapis.com/storage/v1/b/gcloud-test-155fe610-5786-11e7-afc5-0991cf1123b2",
  "qs": {
    "userProject": "gcloud-node-whitelist-ci-tests"
  }
}

// which returns an error
Error: Caller does not have storage.buckets.create access to gcloud-node-whitelist-ci-tests.

"gcloud-node-whitelist-ci-tests" is User B's project ID, but not the name of the bucket we're trying to delete.

I've tried granting storage.objectViewer and storage.objectAdmin as well.

frankyn · 2017-06-23T16:54:16Z

@stephenplusplus I'll try to reproduce this error using the JSON API directly. I was able to get this workflow working using the Ruby client. I'll keep you posted!

stephenplusplus · 2017-06-23T21:20:57Z

All system tests are added, however I'm having trouble testing the download behavior of a file.

The system tests are configured to use two accounts:

User A: The account of the user who ran npm run system-test
User B: The new project Song created

There is a problem when User B tries to download a file from User A. When User B provides the userProject query parameter, an error is returned: "Bad Request". When User B omits the query parameter, a different error is expected: 'Bucket is requester pays bucket but no user project provided.', however the error message is still "Bad Request".

@frankyn any ideas? Thanks!

coveralls · 2017-06-23T21:30:05Z

Coverage increased (+0.0001%) to 99.988% when pulling 56032a0 on stephenplusplus:spp--2371 into a11c47a on GoogleCloudPlatform:master.

coveralls · 2017-06-26T20:30:22Z

Coverage increased (+0.0002%) to 99.988% when pulling a25a18c on stephenplusplus:spp--2371 into ca0d96c on GoogleCloudPlatform:master.

swcloud · 2017-06-27T05:06:43Z

There are many code refactoring in this PR, which I'm not familiar with. @callmehiphop Can you help review this?

stephenplusplus · 2017-06-27T18:12:52Z

The issue with downloading a file has been caught, and a solution is in the works.

The premise of the problem: In file.createReadStream(), we don't buffer data, we just hand it off to the user directly. When we get a response event from the server, and see a status code that isn't a success, we return the code & default status message for it to the user.

What we need to do: buffer the stream for the response body, right before we return that error. I've done that, and it works! I just need some time to figure out if it's going to affect other parts of the library. But, at this time, we can safely mark the surface problems as resolved.

coveralls · 2017-06-27T21:11:50Z

Coverage increased (+0.0002%) to 99.988% when pulling 175e6f9 on stephenplusplus:spp--2371 into ca0d96c on GoogleCloudPlatform:master.

stephenplusplus · 2017-06-27T21:13:45Z

This is ready for a review / merge / party to celebrate @frankyn.

packages/storage/src/file.js

-        bucketName: self.bucket.name,
-        fileName: encodeURIComponent(self.name)
-      }),
+      uri: '',


frankyn

Looking great, I left a question and comment. Not a strict code review and only verifying documentation for the feature.

packages/storage/src/file.js

-        bucketName: self.bucket.name,
-        fileName: encodeURIComponent(self.name)
-      }),
+      uri: '',


packages/storage/src/bucket.js

+ *   </p>
+ * </div>
+ *
+ * Enable `requesterPays` functionality for this bucket.


packages/storage/test/bucket.js

+  describe('exists', function() {
+    it('should call get', function(done) {
+      bucket.get = function() {
+        assert.strictEqual();


coveralls · 2017-06-29T22:58:31Z

Coverage increased (+0.0002%) to 99.988% when pulling 121ce05 on stephenplusplus:spp--2371 into ca0d96c on GoogleCloudPlatform:master.

frankyn

LGTM, not sure if it's still required.

stephenplusplus added the api: storage Issues related to the Cloud Storage API. label Jun 15, 2017

stephenplusplus requested review from tswast and callmehiphop June 15, 2017 03:00

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 15, 2017

tswast requested a review from frankyn June 15, 2017 16:03

stephenplusplus force-pushed the spp--2371 branch from eefabb9 to 2463780 Compare June 16, 2017 16:52

googleapis deleted a comment from coveralls Jun 16, 2017

stephenplusplus force-pushed the spp--2371 branch from c66bb60 to 56032a0 Compare June 23, 2017 21:16

stephenplusplus added 9 commits June 26, 2017 14:10

storage: introduce requesterPays

736b035

test index.js

d3d6a58

test bucket.js

2277afa

lint fix

7dea494

test file.js

7c57293

update gcs-resumable-upload

7a1585b

add system tests

d2afdc2

debug

4ab2587

bring in label methods

376e285

stephenplusplus force-pushed the spp--2371 branch from 56032a0 to 376e285 Compare June 26, 2017 18:52

stephenplusplus added 3 commits June 26, 2017 14:54

export non-whitelist CI account service account JSON

4756aee

configure for appveyor

9242e38

old console.log

a25a18c

use raw response payload as error message

175e6f9

swcloud requested a review from evaogbe June 27, 2017 22:55

frankyn reviewed Jun 28, 2017

View reviewed changes

frankyn suggested changes Jun 28, 2017

View reviewed changes

evaogbe reviewed Jun 29, 2017

View reviewed changes

packages/storage/test/bucket.js Outdated

describe('exists', function() {

it('should call get', function(done) {

bucket.get = function() {

assert.strictEqual();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

elaborate on the requester pays enabling documentation

121ce05

stephenplusplus merged commit 90b41d2 into googleapis:master Jun 29, 2017

frankyn reviewed Jun 29, 2017

View reviewed changes

This was referenced Jul 5, 2017

Update dependency @google-cloud/datastore to version 1.0.7 Platane/velib-paris#24

Merged

Update dependency @google-cloud/storage to version 1.2.0 Platane/velib-paris#25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: introduce requesterPays #2392

storage: introduce requesterPays #2392

stephenplusplus commented Jun 15, 2017 •

edited

Loading

coveralls commented Jun 16, 2017

stephenplusplus commented Jun 16, 2017

stephenplusplus commented Jun 23, 2017

frankyn commented Jun 23, 2017

stephenplusplus commented Jun 23, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 26, 2017

swcloud commented Jun 27, 2017

stephenplusplus commented Jun 27, 2017

coveralls commented Jun 27, 2017

stephenplusplus commented Jun 27, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

frankyn left a comment •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jun 29, 2017

frankyn left a comment

storage: introduce requesterPays #2392

storage: introduce requesterPays #2392

Conversation

stephenplusplus commented Jun 15, 2017 • edited Loading

To Dos

Bucket Operations

storage.createBucket({ requesterPays: true })

bucket.create({ requesterPays: true })

bucket.disableRequesterPays()

bucket.enableRequesterPays()

bucket.combine(sourceFiles, destinationFile, { userProject: 'project-id' })

bucket.createChannel(id, config, { userProject: 'project-id' })

bucket.delete({ userProject: 'project-id' })

bucket.deleteFiles({ userProject: 'project-id' })

bucket.exists({ userProject: 'project-id' }) // calls bucket.getMetadata

bucket.get({ userProject: 'project-id' }) // calls bucket.getMetadata

bucket.getFiles({ userProject: 'project-id' })

bucket.getFilesStream({ userProject: 'project-id' })

bucket.getMetadata({ userProject: 'project-id' })

bucket.makePrivate({ userProject: 'project-id' }) // calls bucket.getFiles and bucket.setMetadata

bucket.setMetadata({ userProject: 'project-id' })

bucket.upload('file.txt', { userProject: 'project-id' }) // calls file.createWriteStream

File Operations

file.copy('newfile.txt', { userProject: 'project-id' })

file.createReadStream({ userProject: 'project-id' })

file.createResumableUpload({ userProject: 'project-id' })

file.createWriteStream({ userProject: 'project-id' })

file.delete({ userProject: 'project-id' })

file.download({ userProject: 'project-id' }) // calls file.createReadStream

file.exists({ userProject: 'project-id' }) // calls file.getMetadata

file.get({ userProject: 'project-id' }) // calls file.getMetadata

file.getMetadata({ userProject: 'project-id' })

file.makePrivate({ userProject: 'project-id' }) // calls file.setMetadata

file.move({ userProject: 'project-id' }) // calls file.copy

file.save(data, { userProject: 'project-id' }) // calls file.createWriteStream

file.setMetadata(metadata, { userProject: 'project-id' })

file.setStorageClass(storageClass, { userProject: 'project-id' }) // calls file.copy

coveralls commented Jun 16, 2017

stephenplusplus commented Jun 16, 2017

stephenplusplus commented Jun 23, 2017

frankyn commented Jun 23, 2017

stephenplusplus commented Jun 23, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 26, 2017

swcloud commented Jun 27, 2017

stephenplusplus commented Jun 27, 2017

coveralls commented Jun 27, 2017

stephenplusplus commented Jun 27, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

frankyn left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jun 29, 2017

frankyn left a comment

Choose a reason for hiding this comment

stephenplusplus commented Jun 15, 2017 •

edited

Loading

`storage.createBucket({ requesterPays: true })`

`bucket.create({ requesterPays: true })`

`bucket.disableRequesterPays()`

`bucket.enableRequesterPays()`

`bucket.combine(sourceFiles, destinationFile, { userProject: 'project-id' })`

`bucket.createChannel(id, config, { userProject: 'project-id' })`

`bucket.delete({ userProject: 'project-id' })`

`bucket.deleteFiles({ userProject: 'project-id' })`

`bucket.exists({ userProject: 'project-id' })` // calls `bucket.getMetadata`

`bucket.get({ userProject: 'project-id' })` // calls `bucket.getMetadata`

`bucket.getFiles({ userProject: 'project-id' })`

`bucket.getFilesStream({ userProject: 'project-id' })`

`bucket.getMetadata({ userProject: 'project-id' })`

`bucket.makePrivate({ userProject: 'project-id' })` // calls `bucket.getFiles` and `bucket.setMetadata`

`bucket.setMetadata({ userProject: 'project-id' })`

`bucket.upload('file.txt', { userProject: 'project-id' })` // calls `file.createWriteStream`

`file.copy('newfile.txt', { userProject: 'project-id' })`

`file.createReadStream({ userProject: 'project-id' })`

`file.createResumableUpload({ userProject: 'project-id' })`

`file.createWriteStream({ userProject: 'project-id' })`

`file.delete({ userProject: 'project-id' })`

`file.download({ userProject: 'project-id' })` // calls `file.createReadStream`

`file.exists({ userProject: 'project-id' })` // calls `file.getMetadata`

`file.get({ userProject: 'project-id' })` // calls `file.getMetadata`

`file.getMetadata({ userProject: 'project-id' })`

`file.makePrivate({ userProject: 'project-id' })` // calls `file.setMetadata`

`file.move({ userProject: 'project-id' })` // calls `file.copy`

`file.save(data, { userProject: 'project-id' })` // calls `file.createWriteStream`

`file.setMetadata(metadata, { userProject: 'project-id' })`

`file.setStorageClass(storageClass, { userProject: 'project-id' })` // calls `file.copy`

frankyn left a comment •

edited

Loading