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.

Sign up for GitHub

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

ai/live: Send event when auth fails #3331

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ai/live: Send event when auth fails #3331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a75f520
ai/live: Send event when auth fails
j0sh Dec 19, 2024
ba1a388
Merge branch 'master' into ja/auth-error-event
j0sh Dec 20, 2024
a669dae
Merge branch 'master' into ja/auth-error-event
j0sh Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion media/rtmp2segment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
Muxer: ffmpeg.ComponentOptions{Name: "segment"},
}})
if err != nil {
clog.Errorf(ctx, "Failed to run segmentation. in=%s err=%s", in, err)
clog.Errorf(ctx, "Failed to run segmentation. in=%s try=%d err=%s", in, retryCount, err)

Check warning on line 62 in media/rtmp2segment.go

View check run for this annotation

Codecov / codecov/patch

media/rtmp2segment.go#L62 

Added line #L62 was not covered by tests
}
retryCount++
time.Sleep(5 * time.Second)
Expand Down
2 changes: 1 addition & 1 deletion server/ai_live_video.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@
go func() {
defer StreamStatusStore.Clear(streamId)
for {
clog.Infof(ctx, "Reading from event subscription for URL: %s", url.String())
clog.V(8).Infof(ctx, "Reading from event subscription for URL: %s", url.String())

Check warning on line 268 in server/ai_live_video.go

View check run for this annotation

Codecov / codecov/patch

server/ai_live_video.go#L268 

Added line #L268 was not covered by tests
segment, err := subscriber.Read()
if err != nil {
clog.Infof(ctx, "Error reading events subscription: %s", err)
Expand Down
7 changes: 6 additions & 1 deletion server/ai_mediaserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,8 @@
// if auth webhook returns pipeline config these will be replaced
pipeline := qp.Get("pipeline")
rawParams := qp.Get("params")
var streamID, pipelineID string
streamID := streamName
var pipelineID string

Check warning on line 434 in server/ai_mediaserver.go

View check run for this annotation

Codecov / codecov/patch

server/ai_mediaserver.go#L433-L434 

Added lines #L433 - L434 were not covered by tests
var pipelineParams map[string]interface{}
if rawParams != "" {
if err := json.Unmarshal([]byte(rawParams), &pipelineParams); err != nil {
Expand All @@ -454,6 +455,10 @@
clog.Errorf(ctx, "failed to kick input connection: %s", kickErr.Error())
}
clog.Errorf(ctx, "Live AI auth failed: %s", err.Error())
monitor.SendQueueEventAsync("ai_stream_events", map[string]string{
"type": "error",
"message": "Live AI auth failed: " + err.Error(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ecmulli @gioelecerati wanted to check this would not break anything downstream since we only have a limited set of fields here. Most of the rest are unknown at this point - they come as part of the auth webhook, but the auth webhook has failed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping on this @ecmulli @gioelecerati

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@j0sh this shouldn't break anything, since the messages are sent to Kafka without any deserialization - worst case scenario they won't be collected anywhere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@j0sh, @gioelecerati is correct here. the missing fields should be ignored.

is streamId, pipelineID, etc automatically included in this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is streamId, pipelineID, etc automatically included in this?

They are not included, because they don't exist at this point. We typically receive those via the auth webhook but this event is reporting that the auth webhook has failed.

If we wanted some form of identifier we could include the stream key as stream_key or something, although that is not part of other events IIRC. The key itself might also be a little strange from brute force connection attempts which has some risk to downstream consumers through eg, injection.

})

Check warning on line 461 in server/ai_mediaserver.go

View check run for this annotation

Codecov / codecov/patch

server/ai_mediaserver.go#L458-L461 

Added lines #L458 - L461 were not covered by tests
http.Error(w, "Forbidden", http.StatusForbidden)
return
}
Expand Down
Loading