Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: Begin accepting auth header and have it override callback URL values #2333

Closed
wants to merge 4 commits into from

Conversation

thomshutt
Copy link
Contributor

@thomshutt thomshutt commented Mar 22, 2022

What does this pull request do? Explain your changes. (required)

  • Begin accepting a new Livepeer-Transcode-Configuration header in the HTTP Push segment endpoint.
  • If present, this header should have the same format as the response from the "HTTP Auth Callback URL".
  • If present and the Manifest ID from the path is not referring to an existing Manifest ID, then a call is still made to the callback URL as before (and fails if this call returns an error) but the fields it returns are overridden by the ones in the header
  • If present and the Manifest ID in the URL already exists, this field is ignored

Specific updates (required)

  • Create a new method for parsing the header and associated unit tests
  • Add logic to override response from callback URL in the case outlined above. Unit test for this override behavior.

How did you test each of these updates (required)
✅ Wrote new unit tests
❌ TODO: Manual testing

Does this pull request close any open issues?

Checklist:

@thomshutt
Copy link
Contributor Author

@yondonfu @iameli Here's my first pass at this, would appreciate you guys taking a quick look (possibly at just the description in this PR) and checking it lines up with the behavior you were expecting

@yondonfu
Copy link
Member

yondonfu commented Mar 23, 2022

The description of the flow sounds right to me, but just had a few comments on these bits:

If present and the Manifest ID from the path is not referring to an existing Manifest ID, then a call is still made to the callback URL as before (and fails if this call returns an error) but the fields it returns are overridden by the ones in the header

  • Just confirming that if there is no callback URL that auth will be skipped and Manifest ID will still be created which I believe is also the current behavior?
  • I think we had originally discussed returning an error if the profiles configuration in the header do not match the ones returned by the callback URL, but I don't feel strongly about returning an error vs. overriding the profiles with whatever is returned by the callback URL. Seems like either option works as long as there is a way to avoid accepting the profiles in the header if there is a mismatch with what is returned by the callback URL

If present and the Manifest ID in the URL already exists, this field is ignored

This sounds right as we're explicitly not supporting changing the profile configuration mid-stream right now so if the Manifest ID already exists then the profiles originally configured for the stream should continue to be used.

@thomshutt
Copy link
Contributor Author

thomshutt commented Mar 23, 2022

Just confirming that if there is no callback URL that auth will be skipped and Manifest ID will still be created which I believe is also the current behavior?

That's correct, for a new Manifest ID in the URL:

  • If there's no callback URL and no header then it behaves as before (allow the request, create new Manifest ID)
  • If there's no callback URL but there is a header then it acts the same as if it had got a successful callback response in the old flow
  • If there's a callback URL and no header then it behaves identically to before

I think we had originally discussed returning an error if the profiles configuration in the header do not match the ones returned by the callback URL, but I don't feel strongly about returning an error vs. overriding the profiles with whatever is returned by the callback URL. Seems like either option works as long as there is a way to avoid accepting the profiles in the header if there is a mismatch with what is returned by the callback URL

I think that rejecting probably does make more sense in terms of simplicity / being able to reason about what's going on for the first iteration of this. I'll update that now.

@thomshutt thomshutt marked this pull request as ready for review March 24, 2022 11:27
@thomshutt thomshutt requested review from oscar-davids and removed request for oscar-davids March 31, 2022 09:47
@thomshutt
Copy link
Contributor Author

Closing this in favour of a simpler change (no logging refactoring etc.) since a lot has changed on master in the meantime #2357

@thomshutt thomshutt closed this Apr 8, 2022
@thomshutt thomshutt deleted the auth-header branch April 8, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants