Return 406 if Accept-Encoding doesn't match store Content-Encoding #264

jonasfj · 2018-04-16T11:23:18Z

We only do this for new the blob artifacts, as they are not in wide-spread use yet.
Hence, as we adopt them people will have time to switch over.

jonasfj · 2018-04-16T11:24:11Z

I think my editor also removed some trailing whitespaces... But getting rid of those is just a bonus I guess.

jhford

Please use existing code where it makes sense instead of reimplementing functionality. The existing code does much more exhaustive checks.

jhford · 2018-04-16T12:35:26Z

src/artifacts.js

+        'available in one of these encodings.',
+        '',
+        'To download this artifact the request must support `Content-Encoding: {{contentEncoding}}`,',
+        'the request can indicating by setting `Accept-Encoding` accordingly.',


'the request can indicating by setting Accept-Encoding accordingly.',

Let's go all the way and suggest "Try using Accept-Encoding: {{contentEncoding}}" :)

jhford · 2018-04-16T12:38:11Z

src/data.js

+     *   contentEncoding:   Content encoding, such as 'gzip',
+     *                      NOTE: This property is not present if contentEncoding is 'identity'.
+     *   contentSha256:     Hex encoded sha256 hash of the un-encoded artifact
+     *   contentLength:     Size of the un-encoded artifact


Number of bytes is more specific, and so more accurate than "size", let's leave it as number of bytes as that's exactly what it is.

jhford · 2018-04-16T12:38:35Z

src/data.js

-     *              to determine if the operation would result in an idempotency
-     *              error
+     *   contentEncoding:   Content encoding, such as 'gzip',
+     *                      NOTE: This property is not present if contentEncoding is 'identity'.


I thought if not present, we would store 'identity' there, am I wrong?

If I look at the code yes... maybe we have default in the schema and I missed that.. but then we have unnecessary special cases in the code.

jhford · 2018-04-16T16:29:00Z

src/data.js

+     *                      NOTE: This property is not present if contentEncoding is 'identity'.
+     *   transferSha256:    Hax encooded sha256 hash of the content-encoding encoded artifact,
+     *                      NOTE: This property is not present if contentEncoding is 'identity'.
+     *   provider:          Always 's3'


Knowing this is very likely to change at some point in this year, let's say "Currently always 's3' but might change later"

jhford · 2018-04-16T16:29:15Z

src/data.js

+     *   transferSha256:    Hax encooded sha256 hash of the content-encoding encoded artifact,
+     *                      NOTE: This property is not present if contentEncoding is 'identity'.
+     *   provider:          Always 's3'
+     *   region:            Always 'us-west-2'


Knowing this is very likely to change at some point in this year, let's say "Currently always 'us-west-2' but might change later"

jhford · 2018-04-16T16:33:52Z

test/artifact_test.js

+        }],
+      });
+
+      debug('### Put first part of artifact');


do you mean the only part :)

jhford · 2018-04-16T16:34:23Z

test/artifact_test.js

+      });
+
+      debug('### Put first part of artifact');
+      const {method, url, headers} = requests[0];


Please match the way that all the other blob-type unit tests run uploads instead of reimplementing it.

let uploadOutcome = await client.runUpload(response.requests, uploadInfo);

I actually want to test the REST APIs work as documented in api.declare?

I'm not interested in testing that some client library works. Our public interface is the REST API, if the REST API breaks I want to know about it.

To be honest, I considered going all the way down to require('http') and use that instead of superagent, because I can about the HTTP level correctness.

note: I don't mind having the other tests, it's great to have more test coverage.

jhford · 2018-04-16T16:37:19Z

test/artifact_test.js

+        contentSha256: crypto.createHash('sha256').update(data).digest('hex'),
+        transferLength: gzipped.length,
+        transferSha256: crypto.createHash('sha256').update(gzipped).digest('hex'),
+        parts: [{


This is testing single part uploads according to the test name and all the artifact names used, but you're doing a multipart upload with a single part. Please delete the parts list from the request to do a true single part upload.

hmm, interesting... :)

I didn't notice this. let's talk about it tomorrow.

jhford · 2018-04-16T16:40:44Z

test/artifact_test.js

+        taskId, 0, 'public/singlepart.dat',
+      );
+      debug('### Fetching artifact from: %s', artifactUrl);
+      const res2 = await request.get(artifactUrl).responseType('blob');


There's already code for doing this:

await verifyDownload(artifact.headers.location, bigfilehash, bigfilesize);

Please use this code as it's what we do for all the other tests and handles many more tests than are duplicated here.

tried adding it that won't work because of content-encoding.

jhford · 2018-04-16T16:41:49Z

test/artifact_test.js

+
+      debug('### Downloading artifact with incorrect "Accept-Encoding"');
+      let e;
+      await request.get(artifactUrl).set('Accept-Encoding', 'identity').catch(err => e = err);


Let's also test for no Accept-Encoding value set, since that's by far the more likely scenario in reality.

identity is the same thing as setting no accept-encoding.

but it should also be one extra line to account for us doing something incorrectly and handling the two different options differently.

@jhford

…ug 1416270, credits @jhford for suggesting this

jonasfj · 2018-04-20T13:56:15Z

@jhford, I'm not sure how well, we want to test this... We basically have to do everything manually with http if you want perfect tests.

IMO this exercises all aspect of the code added. If we want better tests we'll need to write a lot of low-level, and that won't be nice to maintain.

jhford · 2018-04-23T08:20:33Z

The code that does the verify stuff is written using the standard library's HTTP library already, so I think we should just use that. Adding support for content-encoding to those functions should be pretty trivial.

jhford · 2018-04-23T08:22:48Z

in the case of verifyDownload, it's already actually directly using the system's http and https libraries :)

djmitche · 2019-01-04T20:10:07Z

highly bitrotted..

jonasfj requested a review from jhford April 16, 2018 11:23

jhford suggested changes Apr 16, 2018

View reviewed changes

jonasfj added 4 commits April 20, 2018 15:30

Return 406 if Accept-Encoding does not support the stored artifact, b…

e1d0824

…ug 1416270, credits @jhford for suggesting this

Improved a few doc coments

6c20f94

Add test cases for Accept-Encoding checking

52a41cc

Fixed some of the comments

d8905a8

jonasfj force-pushed the limit-blob-accept-gzip branch from 54561b7 to d8905a8 Compare April 20, 2018 13:54

jonasfj requested a review from jhford April 20, 2018 13:56

djmitche closed this Jan 4, 2019

Return 406 if Accept-Encoding doesn't match store Content-Encoding #264

Return 406 if Accept-Encoding doesn't match store Content-Encoding #264

Uh oh!

Conversation

jonasfj commented Apr 16, 2018

Uh oh!

jonasfj commented Apr 16, 2018

Uh oh!

jhford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhford Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasfj commented Apr 20, 2018

Uh oh!

jhford commented Apr 23, 2018

Uh oh!

jhford commented Apr 23, 2018

Uh oh!

djmitche commented Jan 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jhford Apr 16, 2018 •

edited

Loading