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

Dev image does not send proper url for webhook callbacks #1739

Open
naanlizard opened this issue Nov 14, 2024 · 14 comments
Open

Dev image does not send proper url for webhook callbacks #1739

naanlizard opened this issue Nov 14, 2024 · 14 comments
Assignees
Labels
bug Confirmed as bug patched Patch applied

Comments

@naanlizard
Copy link
Contributor

naanlizard commented Nov 14, 2024

Describe the bug
On the latest dev image (airensoft/ovenmediaengine@sha256:773acc79230548f8d942b2321f94d10e0846135128a5b0dc35eab60ff0fa528e), admission webhooks do not work because the URL sent does not include the streamkey

To Reproduce
Steps to reproduce the behavior:
Server.xml

(Note, in Server.xml, the Decodes section is either commented out (for 0.17.1) or uncommented (for dev)

Dockerfile

Maybe relevant, if you put the streamkey in the URL (e.g. in OBS, your rtmp url is set to rtmp://ingest.hostname.tld:1935/live/streamkey) both versions work.

Expected behavior
URL sent should include the streamkey, like on 0.17.1.

Logs
Relevant log lines from our backend API, Parameters being the values from the request OME sends

0.17.1
Parameters: {"client"=>{"address"=>"x", "port"=>58323, "real_ip"=>"x"}, "request"=>{"direction"=>"incoming", "protocol"=>"RTMP", "status"=>"opening", "time"=>"2024-11-14T15:23:54.039+00:00", "url"=>"rtmp://ingest.hostname.tld:1935/live/streamkey"}, "stream"=>{}}

dev
Parameters: {"client"=>{"address"=>"x", "port"=>58619, "real_ip"=>"x"}, "request"=>{"direction"=>"incoming", "protocol"=>"RTMP", "status"=>"opening", "time"=>"2024-11-14T15:28:11.658+00:00", "url"=>"rtmp://ingest.hostname.tld:1935/live"}, "stream"=>{}}

Additional info:

Interestingly, it seems like the closing webhook does send the streamkey in the URL

Request params: {"direction"=>"incoming", "new_url"=>"rtmp://ingest.hostname.tld:1935/live/streamkey", "protocol"=>"RTMP", "status"=>"closing", "time"=>"2024-11-14T15:27:15.762+00:00", "url"=>"rtmp://apollo-rtmp://ingest.hostname.tld:1935/live/streamkey"}

Double checked the above, it seems like it does NOT send the streamkey in the closing call, or at least not always. Strange

@dimiden dimiden self-assigned this Nov 16, 2024
@dimiden dimiden added the in progress Being actively worked on but may take some time to complete label Nov 16, 2024
@dimiden
Copy link
Member

dimiden commented Nov 16, 2024

@naanlizard
Thank you for the report!
Upon reviewing the issue you mentioned, I found that a recent improvement in the URL parsing process caused the problem.
I have just fixed this issue and committed it as 9f92959.

@dimiden dimiden added bug Confirmed as bug patched Patch applied and removed in progress Being actively worked on but may take some time to complete labels Nov 16, 2024
@naanlizard
Copy link
Contributor Author

naanlizard commented Nov 16, 2024

The proper URL is now sent, but there's a new problem, which I'm not sure how to describe

When I start, the stream immediately stops, even though the backend returns an authorized

Backend logs (slightly modified for clarity with some lines removed for irrelevant things our backend is doing - fundamentally, it's doing the right thing I think is the point, this is the same code as on our production server)

I, [2024-11-16T17:17:51.137604 #190]  INFO -- : [319dc37e-f979-4892-a80b-2d1ab2ead660] Started POST "/streams/publish" for x at 2024-11-16 17:17:51 +0000
I, [2024-11-16T17:17:51.139766 #190]  INFO -- : [319dc37e-f979-4892-a80b-2d1ab2ead660] Processing by StreamsController#publish as JSON
I, [2024-11-16T17:17:51.139873 #190]  INFO -- : [319dc37e-f979-4892-a80b-2d1ab2ead660]   Parameters: {"client"=>{"address"=>"x", "port"=>56813, "real_ip"=>"x"}, "request"=>{"direction"=>"incoming", "protocol"=>"RTMP", "status"=>"opening", "time"=>"2024-11-16T17:17:51.135+00:00", "url"=>"rtmp://subdomain-ingest.hostname.tld:1935/live/streamkey"}, "stream"=>{}}
I, [2024-11-16T17:17:51.140378 #190]  INFO -- : [319dc37e-f979-4892-a80b-2d1ab2ead660] Starting publish action
I, [2024-11-16T17:17:51.140454 #190]  INFO -- : [319dc37e-f979-4892-a80b-2d1ab2ead660] Request params: {"direction"=>"incoming", "protocol"=>"RTMP", "status"=>"opening", "time"=>"2024-11-16T17:17:51.135+00:00", "url"=>"rtmp://subdomain-ingest.hostname.tld:1935/live/streamkey"}
I, [2024-11-16T17:17:51.140556 #190]  INFO -- : [319dc37e-f979-4892-a80b-2d1ab2ead660] Extracted token: streamkey
D, [2024-11-16T17:17:51.142152 #190] DEBUG -- : [319dc37e-f979-4892-a80b-2d1ab2ead660]   Stream Load (0.7ms)  SELECT `streams`.* FROM `streams` WHERE `streams`.`id` = 1 LIMIT 1
I, [2024-11-16T17:17:51.143132 #190]  INFO -- : [319dc37e-f979-4892-a80b-2d1ab2ead660] Fetched stream: #<Stream id: 1, title: "Stream", description: nil, slug: "username", token: "streamkey", live_since: nil, adult: true, in_multi: false, parent_stream_id: nil, user_id: 1, created_at: "2024-11-16 02:00:57.721218000 +0000", updated_at: "2024-11-16 17:16:35.570224000 +0000", rendered_description: "<p></p>", last_seen: "2024-11-16 17:16:35.568285000 +0000", offline_image: nil, banner: nil, banner_link: nil, viewers_count: 0, preview: nil>
D, [2024-11-16T17:17:51.144171 #190] DEBUG -- : [319dc37e-f979-4892-a80b-2d1ab2ead660]   User Load (0.3ms)  SELECT `users`.* FROM `users` WHERE `users`.`id` = 1 LIMIT 1
I, [2024-11-16T17:17:51.144478 #190]  INFO -- : [319dc37e-f979-4892-a80b-2d1ab2ead660] Authorization succeeded. Setting stream to live.

I, [2024-11-16T17:17:51.178706 #91]  INFO -- : [23f53614-0876-4402-a453-871142cd4b3e] Started POST "/streams/publish" for x at 2024-11-16 17:17:51 +0000
I, [2024-11-16T17:17:51.180101 #91]  INFO -- : [23f53614-0876-4402-a453-871142cd4b3e] Processing by StreamsController#publish as JSON
I, [2024-11-16T17:17:51.180216 #91]  INFO -- : [23f53614-0876-4402-a453-871142cd4b3e]   Parameters: {"client"=>{"address"=>"x", "port"=>56813, "real_ip"=>"x"}, "request"=>{"direction"=>"incoming", "new_url"=>"://", "protocol"=>"RTMP", "status"=>"closing", "time"=>"2024-11-16T17:17:51.177+00:00", "url"=>"rtmp://subdomain-ingest.hostname.tld:1935/live/streamkey"}, "stream"=>{}}
I, [2024-11-16T17:17:51.180688 #91]  INFO -- : [23f53614-0876-4402-a453-871142cd4b3e] Starting publish action
I, [2024-11-16T17:17:51.180754 #91]  INFO -- : [23f53614-0876-4402-a453-871142cd4b3e] Request params: {"direction"=>"incoming", "new_url"=>"://", "protocol"=>"RTMP", "status"=>"closing", "time"=>"2024-11-16T17:17:51.177+00:00", "url"=>"rtmp://subdomain-ingest.hostname.tld:1935/live/streamkey"}
I, [2024-11-16T17:17:51.180807 #91]  INFO -- : [23f53614-0876-4402-a453-871142cd4b3e] Extracted token: streamkey
D, [2024-11-16T17:17:51.181696 #91] DEBUG -- : [23f53614-0876-4402-a453-871142cd4b3e]   Stream Load (0.5ms)  SELECT `streams`.* FROM `streams` WHERE `streams`.`id` = 1 LIMIT 1
I, [2024-11-16T17:17:51.182434 #91]  INFO -- : [23f53614-0876-4402-a453-871142cd4b3e] Fetched stream: #<Stream id: 1, title: "Stream", description: nil, slug: "garrett", token: "streamkey", live_since: "2024-11-16 17:17:51.145101000 +0000", adult: true, in_multi: false, parent_stream_id: nil, user_id: 1, created_at: "2024-11-16 02:00:57.721218000 +0000", updated_at: "2024-11-16 17:17:51.146750000 +0000", rendered_description: "<p></p>", last_seen: "2024-11-16 17:16:35.568285000 +0000", offline_image: nil, banner: nil, banner_link: nil, viewers_count: 0, preview: nil>
D, [2024-11-16T17:17:51.183619 #91] DEBUG -- : [23f53614-0876-4402-a453-871142cd4b3e]   User Load (0.4ms)  SELECT `users`.* FROM `users` WHERE `users`.`id` = 1 LIMIT 1
I, [2024-11-16T17:17:51.183960 #91]  INFO -- : [23f53614-0876-4402-a453-871142cd4b3e] Stream is closing. Stream ID: 1

omebug111624.log-clean.txt

Dockerfile is the same as above but with

FROM airensoft/ovenmediaengine@sha256:0510bb6178a96083139a66b601c8aceeba5bb787f2e5694c8abfbcbb0010edef

@dimiden dimiden added in progress Being actively worked on but may take some time to complete and removed patched Patch applied labels Nov 18, 2024
@dimiden
Copy link
Member

dimiden commented Nov 18, 2024

@naanlizard
Oh no... The logs show there’s an issue. I thought the URL parser was modified to be compatible with the previous code, but it seems there are unexpected side effects.
I will carefully check if there are any other issues besides the one you mentioned and get back to you!

@dimiden
Copy link
Member

dimiden commented Nov 18, 2024

@naanlizard
The issue occurred because a different value was returned for invalid URL, such as empty URL, compared to the previous implementation.
This issue has been fixed in 4c4ca98, and it has been confirmed that it works correctly even if empty URL is responsed from admission webhook server.

Thank you for the testing!

@dimiden dimiden added patched Patch applied and removed in progress Being actively worked on but may take some time to complete labels Nov 18, 2024
@naanlizard
Copy link
Contributor Author

naanlizard commented Nov 18, 2024 via email

@dimiden
Copy link
Member

dimiden commented Nov 18, 2024

According to my analysis, it seems that you are assigning "" to new_url, which essentially behaves as if an invalid URL is being provided.
If there is no URL to redirect to, it would be better to either omit new_url entirely or set it to null.

@naanlizard
Copy link
Contributor Author

naanlizard commented Nov 18, 2024 via email

@dimiden
Copy link
Member

dimiden commented Nov 18, 2024

Yes. If new_url is not empty, the issue might be with the regular expression used for URL parsing. It would be helpful if you could provide the URL being passed as new_url by the server.
(Since the URL pattern is important, you may replace sensitive information with arbitrary characters.)

For reference, I tested the parser with various URL combinations and encountered no issues, so I suspect there’s a high probability that a empty string was responsed.

@naanlizard
Copy link
Contributor Author

naanlizard commented Nov 18, 2024 via email

@hernanrz
Copy link

Looks like the latest dev docker image fixed this, we were responding to OME admission webhook with the following response, which wasn't working on image hash sha256:0510bb6178a96083139a66b601c8aceeba5bb787f2e5694c8abfbcbb0010edef

{"allowed":true,"new_url":"live/1"}

After further testing on that image I found that if you set the url to an absolute URL, it worked again

imagen

After pulling the latest dev docker image sha256:103e6e0efafe507c6e6f3065d457a1fd2db11ce87a2a74539db8da963716a70a it works again with the relative URL format

@naanlizard
Copy link
Contributor Author

See hernan's comment above - what is the intended use case for new_url?

For our purpose, we have users connect to rtmp://hostname.tld:1935/live and put userid?streamkey in their streamkey slot in OBS. So the URL OME sees is in sum, rtmp://hostname.tld:1935/live/userid?streamkey

Should we be returning the /live/userid value, so that the playback url is correct (e.g. https://hostname.tld/live/1/llhls.m3u8) or is that unnecessary?

If so, should new_url be the full URL (e.g. rtmp://hostname.tld:1935/live/userid) or just the relative path (e.g. /live/userid)

I'll make a pull request for documentation improvements once I understand

@choigilhoon
Copy link
Member

@naanlizard

First, the new_url should be set in the form of a full URL (scheme://host[:port]/app/stream/file?query=value&query2=value2). (Please refer to this)
Then, depending on the form of the playback URL, the value of new_url should change.

Below are two use cases for setting new_url:
[Use case 1]
If you want to hide the streamkey and expose only the userid in the playback URL, such as https://hostname.tld/live/{userid}/llhls.m3u8, set new_url as follows:
"rtmp://hostname.tld:1935/live/{userid}"

[Use case 2]
If you want to hide the userid and expose only the streamkey in the playback URL, such as https://hostname.tld/live/{streamkey}/llhls.m3u8, set new_url as follows:
"rtmp://hostname.tld:1935/live/{streamkey}"

And, if new_url is not a full URL, this value will be ignored, and the value set in OBS (rtmp://hostname.tld:1935/live/userid?streamkey) will be used.

This explains why, even though you entered a relative path in new_url, it seems to work properly because the stream name is automatically set to {userid}, as in [Use case 1].

@naanlizard
Copy link
Contributor Author

To be clear @choigilhoon -

In a vanilla OME install/config, sending an OBS url and streamkey like

rtmp://hostname.tld:1935/appname

and

abc?xyz123 (or any random strings separated by a ?)

Will lead to OME serving that stream as

https://hostname.tld/appname/abc/llhls.m3u8

Correct?

@choigilhoon
Copy link
Member

@naanlizard

That's correct.

In the situation you mentioned, you should respond to the OME admission webhook with one of the cases below.

  • Case 1
{
  "allowed": true
}
  • Case 2
{
  "allowed": true,
  "new_url": "rtmp://hostname.tld:1935/appname/abc"
}

Please note that the query-string part of the OBS streamkey (?xyz123) is ignored when generating the playback URL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed as bug patched Patch applied
Projects
None yet
Development

No branches or pull requests

4 participants