HTTP PUT output support #737

termoose · 2020-03-24T12:13:25Z

Experimental feature support for HTTP ingest.

Tested with ingesting encrypted DASH segments against Akamai MSL4 EP's.
Tested with flaky ingests, disconnecting and reconnecting.

Relates and hopefully closes issue #149.

… failed requests

prevent propagation as zero-length PUT request

… file

…o libcurl

to mitigate calls to File::GetFileSize

googlebot · 2020-03-24T12:13:53Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-03-24T12:13:53Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

amotl · 2020-03-24T13:41:10Z

Dear Ole Andre,

thanks again for taking care about this. Will you also add 8ddbbf0 into the mix?

With kind regards,
Andreas.

amotl · 2020-03-24T13:41:20Z

@googlebot I signed it!

kqyang · 2020-10-13T17:20:30Z

I don't know what to do about the @googlebot I consent. part above. Is this an issue related to my CLA signing?

Emm... I am not sure if it is because of yours CLA signing, but the bot was complaining "one or more commits were authored or co-authored by someone other than the pull request submitter."

I'll see if I can find out.

Thanks for your effort on this PR!

amotl · 2020-10-21T12:01:36Z

Hi again,

seems like most of the conflicts were that you're now using a BufferWriter

AppVeyor shows different kinds of errors [1]. Is this related somehow?

Is [there still] an issue related to my CLA signing?

@kqyang promised to check. Maybe this is related to these two commits by Rintaro Kuroiwa (@rkuroiwa) somehow? At least, both commits are not directly associated with his pen.

With kind regards,
Andreas.

[1] https://ci.appveyor.com/project/shaka/shaka-packager/builds/35724371/job/d3va1bvda78ago4o

kqyang · 2020-10-21T17:24:33Z

@kqyang promised to check. Maybe this is related to these two commits by Rintaro Kuroiwa (@rkuroiwa) somehow? At least, both commits are not directly associated with his pen.

We are still checking. Apologize for the delay.

AppVeyor shows different kinds of errors [1]. Is this related somehow?

It looks like a problem in the test.

TEST_F(HttpFileTest, PutChunkedTranser) {
  std::unique_ptr<File, FileCloser> writer(
      File::Open("put+http://127.0.0.1:8080/test_out", "w"));
  ASSERT_TRUE(writer);
  ASSERT_EQ(kWriteBufferSize, writer->Write(kWriteBuffer, kWriteBufferSize));
  writer.release()->Close();
}

[ RUN      ] HttpFileTest.PutChunkedTranser
[1013/131933:ERROR:local_file.cc(85)] Failed to create directory put+http: ErrorCode 123
c:\projects\shaka-packager\src\packager\file\http_file_unittest.cc(20): error: Value of: writer
  Actual: false
Expected: true
[  FAILED  ] HttpFileTest.PutChunkedTranser (27 ms)

Should the URL of the file open just be "http://127.0.0.1:8080/test_out"?

Also, a separate question, do we support HTTP methods other than "put"?

kqyang · 2020-10-21T18:18:16Z

@amotl Here is the problem:

amotl | andreas.motl@elmyra.de | need_author_consent

We need your consent for this PR. See the instruction at #737 (comment), i.e.

Please have them confirm that by leaving a comment that contains only @googlebot I consent

amotl · 2020-10-21T18:29:15Z

Oh, it was me! I believed I had solved this properly already. Thanks!

amotl · 2020-10-21T18:29:23Z

@googlebot I consent

googlebot · 2020-10-21T18:29:42Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2020-10-21T18:29:42Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

termoose · 2020-10-27T08:48:07Z

It looks like a problem in the test.

TEST_F(HttpFileTest, PutChunkedTranser) {
  std::unique_ptr<File, FileCloser> writer(
      File::Open("put+http://127.0.0.1:8080/test_out", "w"));
  ASSERT_TRUE(writer);
  ASSERT_EQ(kWriteBufferSize, writer->Write(kWriteBuffer, kWriteBufferSize));
  writer.release()->Close();
}

[ RUN      ] HttpFileTest.PutChunkedTranser
[1013/131933:ERROR:local_file.cc(85)] Failed to create directory put+http: ErrorCode 123
c:\projects\shaka-packager\src\packager\file\http_file_unittest.cc(20): error: Value of: writer
  Actual: false
Expected: true
[  FAILED  ] HttpFileTest.PutChunkedTranser (27 ms)

Should the URL of the file open just be "http://127.0.0.1:8080/test_out"?

That's a strange test, but I'm guessing it should be just http since that's the only prefix we're checking for with the kHttpFilePrefix and kHttpsFilePrefix.
It does run for me though, which doesn't make sense at all?

[----------] 1 test from HttpFileTest
[ RUN      ] HttpFileTest.PutChunkedTranser
[       OK ] HttpFileTest.PutChunkedTranser (20 ms)
[----------] 1 test from HttpFileTest (20 ms total)

It gives me a 404 if I change the test to http

[----------] 1 test from HttpFileTest
[ RUN      ] HttpFileTest.PutChunkedTranser
[1027/094205:ERROR:http_file.cc(218)] PUT request for http://127.0.0.1:8080/test_out failed. Reason: HTTP response code said error. Response code: 404.
[       OK ] HttpFileTest.PutChunkedTranser (47 ms)
[----------] 1 test from HttpFileTest (47 ms total)

It makes sense that the test is not failing anyway since it's only asserting writer and checking that kWriteBufferSize is equal to bytes written, but Writer writes to a cache_ buffer before doing the http request, so that will probably never fail. I made the change to http anyway.

Also, a separate question, do we support HTTP methods other than "put"?

It's only doing PUT for now but I see there's code for other methods added in the large commit above, does not seem to be in use.

amotl · 2020-10-27T09:33:44Z

Hi there,

It gives me a 404 if I change the test to http.

It will not be so easy to just remove the prefix. However, I can't remember the intrinsic details that took me to that decision to add the prefix like that - apart from only supporting PUT anyway. I believe it has something to do with that I had to use something like the put+ prefix to detect that the output should go to a non-file handler.

Will you be able to make it work after sublimating yourself into the code again, @termoose?

With kind regards,
Andreas.

P.S.: Great so see @joeyparrish is already building upon our work here, see termoose#1.

amotl · 2020-10-27T10:07:20Z

The innocent and rather superfluous code like 4491dee ff. has been there for a reason to be able to tell different kinds of files apart.

Otherwise, the machinery doesn't know to handle that as HTTP and will try to access the local file system

[1013/131933:ERROR:local_file.cc(85)] Failed to create directory put+http: ErrorCode 123

I can't describe more precisely what I have been going through back then but I can remember it was a bit of a brainfuck because the machinery doesn't offer a way to signal this through all the loosely coupled subsystems between ingress and egress.

I would have to review the whole bunch of changes again and compare the current state of the onion against all the commits I made in the past to get back on track with this.

amotl · 2020-10-27T10:59:23Z

Is it actually a good sign that AppVeyor build succeeded right now? Maybe all my ramblings above are just moot. If so, sorry for the noise!

So, will that be ready to merge?

termoose · 2020-10-27T13:09:29Z

Ah I didn't see your GetEffectiveFileAddress function! Could it be that there are other mechanism at work in the "loosely coupled subsystem" now that makes it handle the http protocol? Whenever I tested with ingesting to Akamai MSL4 I didn't specify the output gateway to have put+http:// BTW. I will test the branch once again against Akamai's gateways for DASH and HLS and see how it behaves. This merge is getting complicated :)

But yes the test passes, so that's a bit strange, but I don't see how it could fail actually. The only way I see this test failing is if the writer is nullptr, and for some strange reason it was when the test was running with put+http. I guessing that this is because put+http is not in any of the elements in kFileTypeInfo[] and the construction fails? Anyway enough rabling from me! I'll run some tests tomorrow and see how it behaves. I'd also like to add support for multiple output gateways at some point so we can push to a backup, but that's for another time!

amotl · 2020-10-27T13:40:20Z

Dear Ole Andre,

Could it be that there are other mechanism at work in the "loosely coupled subsystem" now that makes it handle the http protocol?

That might well be the case. Magic happens sometimes. ;] Maybe also I was just too stupid back then. Thanks for the fix through a1e5a40. If we need to support other HTTP methods than PUT, we will be able to revisit this later.

I will test the branch once again against Akamai's gateways for DASH and HLS and see how it behaves.

Thank you so much! I believe if that will succeed, we will be ready to merge and address other things related to the tests later.

With kind regards,
Andreas.

packager/file/http_file.h

TheModMaker · 2020-11-05T23:42:26Z

packager/file/http_file.h

+  bool Open() override;
+
+ private:
+  enum HttpMethod {


It's better to use a C++11 enum class instead. Also, the names should be kGet, kPost, etc.

TheModMaker · 2020-11-05T23:48:30Z

packager/file/http_file.h

+  std::string cert_private_key_file_;
+  std::string cert_private_key_pass_;
+
+  const uint32_t timeout_in_seconds_;


This isn't really used. It can be added later once support is added. Also, a timeout for a streaming upload isn't that useful since we're streaming in real-time, so it can take a while.

TheModMaker · 2020-11-05T23:54:55Z

packager/file/http_file.cc

+  headers = curl_slist_append(headers, "Content-Type: application/octet-stream");
+  headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+
+  // Don't stop on 200 OK responses.


Don't wait for a 100-continue

TheModMaker · 2020-11-05T23:56:49Z

packager/file/http_file.h

+
+ private:
+  enum HttpMethod {
+    GET,


Since this is an upload-only type, using GET won't make sense. I suggest removing it.

TheModMaker · 2020-11-06T00:00:27Z

packager/file/http_file.cc

+
+// Perform HTTP request
+Status HttpFile::Request(HttpMethod http_method,
+                         const std::string& url,


The URL is in a field, so no need to pass as an argument.

amotl · 2020-11-06T08:30:42Z

Dear Jacob,

@TheModMaker requested changes 8 hours ago.

Thanks a bunch for your thorough review. Sorry that most of these shortcomings are coming from the fact that I am not a native C++ speaker. I have been happy that @kqyang and @rkuroiwa guided me the other day when doing the initial work on this.

I also want to apologize that I am currently busy with other obligations, so I am humbly asking if you will be able to address these adjustments suggested by Jacob, @termoose?

With kind regards,
Andreas.

…tializer

termoose · 2020-11-10T13:08:24Z

Thanks for the very thorough review @TheModMaker! I just started at the top and can continue when time allows, think we should be able to get through these. Most of these changes are not done by me so it's a bit challenging to amend them.

amotl · 2020-11-26T11:04:49Z

Hi again,

I just wanted to ping you about this again. Because @termoose stated:

Most of these changes are not done by me so it's a bit challenging to amend them.

I believe most of them are coming from my pen while working on this the other day. However, as I currently don't see any chance to continue working on those, I want to humbly ask if one of the core developers might be able to take over addressing the issues reported by @TheModMaker?

With kind regards,
Andreas.

CaitlinOCallaghan · 2021-02-02T03:49:02Z

Hi everyone, @amotl @termoose @TheModMaker @joeyparrish! I was wondering what the status is on this PR? My work uses the HTTP upload feature, and I would love to see it get pulled into master soon. Please let me know if I can help in any way.

Cheers,
Caitlin

TheModMaker · 2021-02-02T18:54:02Z

We've decided to merge this as-is and I'll make the requested changes. Thanks for the contribution.

amotl · 2021-02-28T15:59:29Z

Hi Jacob,

thank you very much for integrating this feature and for taking over development.

With kind regards,
Andreas.

amotl and others added 26 commits October 15, 2018 14:59

Add stub for HttpFile support

f32c934

Add HttpFile implementing the HTTP PATCH flavor

344c36f

Add HttpFile implementing the HTTP PUT chunked flavor

5f980ff

Fix the formatting and added unit test file

a9605f4

Gets to the point where curl thinks all message is sent

5bab0bd

HTTP upload: Improve VLOG messages

96b248c

HTTP upload: Improve error path by also signalling task completion on…

2ca0a5b

… failed requests

HTTP upload: Ignore read access for HttpFile to

fac6688

prevent propagation as zero-length PUT request

HTTP upload: Use effective filename/url for propagating into playlist…

4491dee

… file

HTTP upload: Remove custom libcurl debug function

e76c871

HTTP upload: Drop support for HTTP PATCH

6975cb3

HTTP upload: Improve logging

88609dc

HTTP upload: Propagate log level indicated by "--libcurl_verbosity" t…

c3403f9

…o libcurl

HTTP upload: Update documentation

7f83ebb

HTTP upload: Compute TS segment size at runtime

5e17e0a

to mitigate calls to File::GetFileSize

HTTP upload: Update documentation

32b6e32

Support for custom User-Agent string as command line argument

439c30e

Make HttpFile work with https://, no client cert support yet

ecc3474

Properly prepending the appropriate scheme in HttpFile

5d01d5e

Fixed http/http mistake

01e3d27

HTTPS certs, similar to playready_key_encryption_flags.cc

2778272

Renamed command line arguments to avoid collisions

9ec1bd2

Updated CONTRIBUTORS and AUTHORS in accordance with Google CLA

e99fa95

Sort contributors and authors alphabetically

01386a3

Updated documentation explaining new command line arguments

15bb8ae

Use a point list in documentation

d48e1ac

joeyparrish mentioned this pull request Oct 21, 2020

Add HTTP auth and DELETE support, plus GCS documentation termoose/shaka-packager#1

Closed

change http file unit test to use http protocol

a1e5a40

TheModMaker suggested changes Nov 6, 2020

View reviewed changes

replaced ScopedCurl by unique_ptr, moved and stripped down LibCurlIni…

1cbc6ff

…tializer

include <memory> so std::unique_ptr can be found

d516f95

TheModMaker merged commit aa17521 into shaka-project:master Feb 2, 2021

amotl mentioned this pull request Feb 28, 2021

HTTP PUT output #149

Closed

Canta mentioned this pull request Apr 28, 2021

HTTP upload - Akamai #939

Closed

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP PUT output support #737

HTTP PUT output support #737

termoose commented Mar 24, 2020

googlebot commented Mar 24, 2020

googlebot commented Mar 24, 2020

amotl commented Mar 24, 2020

amotl commented Mar 24, 2020

kqyang commented Oct 13, 2020

amotl commented Oct 21, 2020

kqyang commented Oct 21, 2020

kqyang commented Oct 21, 2020

amotl commented Oct 21, 2020

amotl commented Oct 21, 2020

googlebot commented Oct 21, 2020

google-cla bot commented Oct 21, 2020

termoose commented Oct 27, 2020

amotl commented Oct 27, 2020 •

edited

Loading

amotl commented Oct 27, 2020 •

edited

Loading

amotl commented Oct 27, 2020 •

edited

Loading

termoose commented Oct 27, 2020

amotl commented Oct 27, 2020

TheModMaker Nov 5, 2020

TheModMaker Nov 5, 2020

TheModMaker Nov 5, 2020

TheModMaker Nov 5, 2020

TheModMaker Nov 6, 2020

amotl commented Nov 6, 2020 •

edited

Loading

termoose commented Nov 10, 2020

amotl commented Nov 26, 2020

CaitlinOCallaghan commented Feb 2, 2021

TheModMaker commented Feb 2, 2021

amotl commented Feb 28, 2021

HTTP PUT output support #737

HTTP PUT output support #737

Conversation

termoose commented Mar 24, 2020

googlebot commented Mar 24, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Mar 24, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

amotl commented Mar 24, 2020

amotl commented Mar 24, 2020

kqyang commented Oct 13, 2020

amotl commented Oct 21, 2020

kqyang commented Oct 21, 2020

kqyang commented Oct 21, 2020

amotl commented Oct 21, 2020

amotl commented Oct 21, 2020

googlebot commented Oct 21, 2020

google-cla bot commented Oct 21, 2020

termoose commented Oct 27, 2020

amotl commented Oct 27, 2020 • edited Loading

amotl commented Oct 27, 2020 • edited Loading

amotl commented Oct 27, 2020 • edited Loading

termoose commented Oct 27, 2020

amotl commented Oct 27, 2020

TheModMaker Nov 5, 2020

Choose a reason for hiding this comment

TheModMaker Nov 5, 2020

Choose a reason for hiding this comment

TheModMaker Nov 5, 2020

Choose a reason for hiding this comment

TheModMaker Nov 5, 2020

Choose a reason for hiding this comment

TheModMaker Nov 6, 2020

Choose a reason for hiding this comment

amotl commented Nov 6, 2020 • edited Loading

termoose commented Nov 10, 2020

amotl commented Nov 26, 2020

CaitlinOCallaghan commented Feb 2, 2021

TheModMaker commented Feb 2, 2021

amotl commented Feb 28, 2021

amotl commented Oct 27, 2020 •

edited

Loading

amotl commented Oct 27, 2020 •

edited

Loading

amotl commented Oct 27, 2020 •

edited

Loading

amotl commented Nov 6, 2020 •

edited

Loading