Re-enable support for multiple streams. #618

j0sh · 2018-12-11T09:25:08Z

What does this pull request do? Explain your changes. (required)
This enables broadcasters to receive multiple incoming RTMP streams.

Multistream Changes

RNG

Seed RNG when LivepeerNode is initialized, rather than each time RandomIdGenerator is called.
Makes tests more predictable (eg, we can reset the RNG to for fixed randomness such as testing ID collisions) and mitigates certain types of testing failures around concurrency.

Common Struct for Per-Connection State

We add the rtmpConnection struct in lieu of holding similar information in separate maps.

 type rtmpConnection struct {
        nonce  uint64
        stream stream.RTMPVideoStream
        pl     core.PlaylistManager
        sess   *BroadcastSession
 }
 
 type LivepeerServer struct {
  ...
        rtmpConnections map[core.ManifestID]*rtmpConnection
        connectionLock  *sync.Mutex
  ...
 }

RTMP-HLS ManifestID Sharing

Refresher: a StreamID is composed of

<ManifestID><Rendition>

Previously, RTMP StreamIDs were independent from HLS StreamIDs. This offered protection from hijacking since RTMP URIs were uncorrelated to HLS playback URLs.

To support multi-stream, we incorporate the HLS playlist into the rtmpConnection struct alongside other elements of the RTMP connection.

However, to look up the playlist at playback time, the rtmpConnection needs to be accessible using the HLS StreamID alone, where a playlist is accessed via the HTTP path

/stream/<HLS_StreamID>.m3u8

We solve this by sharing the ManifestID between RTMP and HLS.

However, by sharing the ManifestID for HLS and RTMP, we open ourselves up to stream hijacking due to the easily guessable RTMP StreamID structure, which uses "RTMP" as the rendition:

<RTMP_ManifestID>RTMP

Stream Keys

To mitigate RTMP hijacking based on a known ManifestID, we replace the fixed "RTMP" rendition with a random stream key. This preserves the StreamID construct while restoring most of the security properties of the earlier setup.

<ManifestID><RTMP_StreamKey>

Authorizing Incoming RTMP

Note that stream key only protects playback (eg, via the segmenter); it doesn't prevent unauthorized streams from being ingested.

The current thinking for accomplishing this is to have a callback to some user-specified API. The API can then validate the RTMP path. If validation passes, the path can be used as the Manifest ID.

For example, streaming into the RTMP URL

rtmp://localhost/abc/def

Would generate a ManifestID of abc and a stream key of def for a StreamID of abcdef .

Later on we can discuss and experiment with variations on this, such as making the stream key optional during authorization, and generating it if omitted from the path. And so forth...

Alternative Introduce a layer of indirection rather than sharing ManifestIDs between RTMP and HLS. Maintain separate HLS ManifestID map for playlists, and RTMP connecton struct holds the HLS ManifestID. Requires more bookeeping for this separate map, but allows us to keep the ManifestIDs for RTMP and HLS strictly separate.

How did you test each of these updates (required)

Unit test coverage improved around the area of changes.
Manual testing. Start up a broadcast node and stream into it from several ffmpeg instances. Ensure segmentation succeeds, RTMP playback succeeds, and that the transcoding flow still works for all streams.
Test various CLI endpoints, including /status and /debug to ensure the expected (same) data is returned. The output isn't pretty but eyeballing it is enough for now.
The livepeer_cli doesn't quite work because it isn't tuned for offchain mode yet and I don't have a working onchain mode here yet. But should be OK since the webserver changes are minimal.

Does this pull request close any open issues?

Checklist:

~~README and other documentation updated~~
Node runs in OSX and devenv

j0sh · 2018-12-14T22:30:29Z

Ready for review.

yondonfu · 2018-12-17T15:40:21Z

server/mediaserver.go


 	ExposeCurrentManifest bool

-	rtmpStreams                map[core.StreamID]stream.RTMPVideoStream
-	broadcastRtmpToManifestMap map[string]string
+	rtmpConnections map[core.ManifestID]*rtmpConnection


Could we use a sync.Map here instead of a normal Map + Mutex?

Hadn't known about sync.Map -- thanks for the tip!

Looking into it, a sync.Map would probably work here, since we don't really need to assure mutual exclusion outside reading/writing the map itself. However, the documentation seems to discourage its use:

Most code should use a plain Go map instead, with separate locking or coordination, for better type safety and to make it easier to maintain other invariants along with the map content. ... The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys

In particular, we don't fit the first case of an "always-growing cache". We sort of fit the second, but we'd still have concurrent access to the same set of keys (eg, playback).

Do you still feel that the sync.Map would be appropriate? I'm fairly indifferent; it would certainly help reduce manual locking (and the potential for mistakes associated with that), but I'm unsure if the caveats should be a concern. In particular, the lack of type safety makes me twitch a bit.

When you mention playback requiring concurrent access to the same set of keys are you referring to the access in getHLSMasterPlaylistHandler()?

If we have concurrent access to the same set of keys as a result of the above, then I think it makes sense not to stick with a plain Go map + mutex. I originally suggested using sync.Map because I thought rtmpConnections fell under category 2 usage from the documentation. But if not, in the interest of keeping type safety and only using sync.Map when usage falls under one of those 2 categories I think sticking with a plain Go map + mutex makes sense.

Yeah, there actually are a number of places that could read the same keys, although getHLSMasterPlaylistHandler and getHLSMediaPlaylistHandler are the major ones. Based on this conversation, I went ahead and changed those places to use a read lock (which is more correct in our case), here's the result (as diff'd from the streamid refactor branch):

diff --git a/server/mediaserver.go b/server/mediaserver.go index 04a67632..155dd9ba 100644 --- a/server/mediaserver.go +++ b/server/mediaserver.go @@ -75,7 +75,7 @@ type LivepeerServer struct { ExposeCurrentManifest bool rtmpConnections map[core.ManifestID]*rtmpConnection - connectionLock *sync.Mutex + connectionLock *sync.RWMutex } func NewLivepeerServer(rtmpAddr string, httpAddr string, lpNode *core.LivepeerNode) *LivepeerServer { @@ -91,7 +91,7 @@ func NewLivepeerServer(rtmpAddr string, httpAddr string, lpNode *core.LivepeerNo opts.HttpMux = http.NewServeMux() } server := lpmscore.New(&opts) - return &LivepeerServer{RTMPSegmenter: server, LPMS: server, LivepeerNode: lpNode, HttpMux: opts.HttpMux, connectionLock: &sync.Mutex{}, rtmpConnections: make(map[core.ManifestID]*rtmpConnection)} + return &LivepeerServer{RTMPSegmenter: server, LPMS: server, LivepeerNode: lpNode, HttpMux: opts.HttpMux, connectionLock: &sync.RWMutex{}, rtmpConnections: make(map[core.ManifestID]*rtmpConnection)} } //StartServer starts the LPMS server @@ -517,7 +517,7 @@ func getHLSMasterPlaylistHandler(s *LivepeerServer) func(url *url.URL) (*m3u8.Ma manifestID = sid.ManifestID } - s.connectionLock.Lock() + s.connectionLock.RLock() defer s.connectionLock.Unlock() cxn, ok := s.rtmpConnections[manifestID] if !ok || cxn.pl == nil { @@ -536,7 +536,7 @@ func getHLSMediaPlaylistHandler(s *LivepeerServer) func(url *url.URL) (*m3u8.Med return func(url *url.URL) (*m3u8.MediaPlaylist, error) { strmID := parseStreamID(url.Path) mid := strmID.ManifestID - s.connectionLock.Lock() + s.connectionLock.RLock() defer s.connectionLock.Unlock() cxn, ok := s.rtmpConnections[mid] if !ok || cxn.pl == nil { @@ -589,7 +589,7 @@ func getHLSSegmentHandler(s *LivepeerServer) func(url *url.URL) ([]byte, error) func getRTMPStreamHandler(s *LivepeerServer) func(url *url.URL) (stream.RTMPVideoStream, error) { return func(url *url.URL) (stream.RTMPVideoStream, error) { mid := parseManifestID(url.Path) - s.connectionLock.Lock() + s.connectionLock.RLock() cxn, ok := s.rtmpConnections[mid] s.connectionLock.Unlock() if !ok { @@ -627,7 +627,7 @@ func (s *LivepeerServer) GetNodeStatus() *net.NodeStatus { // not threadsafe; need to deep copy the playlist m := make(map[string]*m3u8.MasterPlaylist, 0) - s.connectionLock.Lock() + s.connectionLock.RLock() defer s.connectionLock.Unlock() for _, cxn := range s.rtmpConnections { if cxn.sess == nil || cxn.pl == nil { @@ -641,7 +641,7 @@ func (s *LivepeerServer) GetNodeStatus() *net.NodeStatus { // Debug helpers func (s *LivepeerServer) LatestPlaylist() core.PlaylistManager { - s.connectionLock.Lock() + s.connectionLock.RLock() defer s.connectionLock.Unlock() cxn, ok := s.rtmpConnections[LastManifestID] if !ok || cxn.pl == nil {

yondonfu · 2018-12-17T15:43:27Z

server/mediaserver.go


 const HLSWaitInterval = time.Second
 const HLSBufferCap = uint(43200) //12 hrs assuming 1s segment
 const HLSBufferWindow = uint(5)
+const StreamKeyBytes = 6


Out of curiosity is there a particular reason for 6 bytes?

No reason. Should it be 4? 5? 8? 32?

The stream key only makes it harder for an attacker to read the incoming RTMP. A typical node really should have monitoring for repeated attempts in case someone tries to brute force things, eg fail2ban. However, at the moment, we don't really have the logging granularity to facilitate this monitoring, nor do we expose enough information to the goclient to determine data like incoming RTMP IP addresses. We should though.

Got it - seems fine as long as we have a constant for it!

Wouldn't using 8 bytes result in less lines of code because we can just use rand.Uint64()?

We'd still want to use our RNG helpers to generate the stream key. It seems that something else also mutates the RNG state during testing, so resetting the seed alone is not the best way to ensure deterministic results during testing.

Also see 735482a

Wouldn't the built-in helpers from the rand package (i.e. rand.Uint64()) be relying on the same random seed (since they rely on the same default source) as RandomIdGenerator (https://github.com/livepeer/go-livepeer/blob/master/core/streamdata.go#L49) since this function is using rand.Uint32() under the hood?

Correct, and we can't rely on resetting the seed to give us consistent behavior during unit tests. Hence we want to generate the stream key via the monkey-patchable RandomIdGenerator that's introduced above, to get deterministic values for testing.

Ah I see - I initially missed your point about monkey patching RandomIdGenerator. 👍

yondonfu · 2018-12-17T16:06:05Z

server/mediaserver.go

+		s.connectionLock.Lock()
+		defer s.connectionLock.Unlock()
+		cxn, ok := s.rtmpConnections[mid]
+		if !ok || cxn.pl == nil {


Are there cases where we could have a rtmpConnection without a core.PlaylistManager? Or is this just for extra safety?

This check is just for safety -- at the moment, there shouldn't be any cases where the PlaylistManager is empty.

ericxtang · 2018-12-18T20:49:38Z

server/mediaserver_test.go

+	storage := drivers.NodeStorage.NewSession(string(mid))
+	pl := core.NewBasicPlaylistManager(mid, storage)
+	if _, err := s.startBroadcast(pl); err != ErrDiscovery {
+		glog.Error("Expected error with discovery")


Why are we doing glog.Error instead of t.Errof in TestStartBroadcast?

Typo! Good catch, fixed.

Note that we use glog.Error in a few other places such as SegmentRTMPToHLS which is right above this function. https://github.com/livepeer/go-livepeer/blob/master/server/mediaserver_test.go#L37

Needed to clean up playlists after a RTMP disconnect.

ericxtang

LGTM. 🚢 after @yondonfu is good with the PR too.

yondonfu · 2018-12-18T21:08:30Z

server/mediaserver.go

+			glog.Error("Error constructing manifest ID", err)
+			return ""
+		}
+		s.connectionLock.Lock()


Doesn't look like we're writing here - should this be s.connectionLock.RLock()?

yondonfu · 2018-12-18T21:12:13Z

server/mediaserver.go

-				s.VideoNonceLock.Unlock()
+				s.connectionLock.Lock()
+				cxn, active := s.rtmpConnections[mid]
+				s.connectionLock.Unlock()


Since we potentially modify the pointer cxn.sess on L490, should this unlock statement be after the active check (maybe using defer)?

Yeah, that's probably safer. Fixed.

Right now, we don't really have a strong requirement for mutual exclusion of the cxn object (eg, cxn is currently unprotected elsewhere in gotRTMPStreamHandler) but using defer makes things more consistent with the rest of the code and gives us that extra bit of safety should it every come up later.

Avoids the need to maintain separate maps tracking various things.

This allows for multi stream support.

This avoids collisions when creating concurrent StreamIDs.

yondonfu

LGTM

eladmallel · 2018-12-19T16:51:09Z

server/mediaserver.go


 	ExposeCurrentManifest bool

-	rtmpStreams                map[core.StreamID]stream.RTMPVideoStream
-	broadcastRtmpToManifestMap map[string]string
+	rtmpConnections map[core.ManifestID]*rtmpConnection


@j0sh I'm not sure I understand why rtmpConnections is part of the LivepeerServer struct which is shared by both B and O roles. What's the best way for me to better understand why Os need to hold variables only useful for B?

thanks!

O doesn't use most of the fields in LivepeerServer -- in fact, the RTMP listener isn't even started for O.

What you're seeing is a reflection of how things have evolved up to this point, same as how we have a single LivepeerNode with fields that are only used by one or the other. We can clean this up at some point.

j0sh added the WIP label Dec 11, 2018

j0sh force-pushed the ja/multistream branch from 686810d to 4be8bcc Compare December 14, 2018 22:29

j0sh requested review from darkdarkdragon, ericxtang, yondonfu, angyangie and eladmallel December 14, 2018 22:29

j0sh removed the WIP label Dec 14, 2018

j0sh changed the title ~~WIP: Re-enable support for multiple streams.~~ Re-enable support for multiple streams. Dec 14, 2018

j0sh force-pushed the ja/multistream branch from 4be8bcc to 04e763b Compare December 14, 2018 23:06

yondonfu reviewed Dec 17, 2018

View reviewed changes

j0sh force-pushed the ja/multistream branch 2 times, most recently from f33a53e to 3078504 Compare December 18, 2018 18:01

ericxtang reviewed Dec 18, 2018

View reviewed changes

j0sh added 3 commits December 18, 2018 12:53

mediaserver: Add tests for StartBroadcast.

43da7ce

mediaserver: Remove unused bits.

2b42db2

mediaserver: Share ManifestID between RTMP and HLS.

1b90287

Needed to clean up playlists after a RTMP disconnect.

j0sh force-pushed the ja/multistream branch from 3078504 to ebf0755 Compare December 18, 2018 20:55

ericxtang approved these changes Dec 18, 2018

View reviewed changes

yondonfu reviewed Dec 18, 2018

View reviewed changes

j0sh added 5 commits December 18, 2018 13:43

mediaserver: Store per-connection state in a common struct.

193f054

Avoids the need to maintain separate maps tracking various things.

mediaserver: Store playlists in common struct.

b064655

This allows for multi stream support.

livepeer: Seed RNG at node startup.

09d18e4

This avoids collisions when creating concurrent StreamIDs.

mediaserver: Add tests.

0ab42ce

mediaserver: Add stream keys for RTMP.

2411d81

j0sh force-pushed the ja/multistream branch from ebf0755 to 2411d81 Compare December 18, 2018 21:53

yondonfu approved these changes Dec 18, 2018

View reviewed changes

Merge branch 'master' into ja/multistream

92d4764

eladmallel reviewed Dec 19, 2018

View reviewed changes

ericxtang merged commit 663db3e into master Dec 19, 2018

angyangie mentioned this pull request Jan 2, 2019

Rendition handling updates #643

Merged

3 tasks

yondonfu mentioned this pull request Jan 3, 2019

StreamID Refactoring #625

Merged

3 tasks

j0sh mentioned this pull request Jan 12, 2019

Multiple Streams per Broadcaster #481

Closed

6 tasks

j0sh deleted the ja/multistream branch December 19, 2019 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable support for multiple streams. #618

Re-enable support for multiple streams. #618

j0sh commented Dec 11, 2018 •

edited

Loading

j0sh commented Dec 14, 2018

yondonfu Dec 17, 2018

j0sh Dec 17, 2018

yondonfu Dec 18, 2018

j0sh Dec 18, 2018

yondonfu Dec 17, 2018

j0sh Dec 17, 2018

yondonfu Dec 18, 2018

eladmallel Dec 19, 2018

j0sh Dec 19, 2018 •

edited

Loading

yondonfu Dec 19, 2018

j0sh Dec 19, 2018

yondonfu Dec 19, 2018

yondonfu Dec 17, 2018

j0sh Dec 17, 2018

ericxtang Dec 18, 2018

j0sh Dec 18, 2018 •

edited

Loading

ericxtang left a comment

yondonfu Dec 18, 2018

j0sh Dec 18, 2018

yondonfu Dec 18, 2018

j0sh Dec 18, 2018

yondonfu left a comment

eladmallel Dec 19, 2018

j0sh Dec 19, 2018

Re-enable support for multiple streams. #618

Re-enable support for multiple streams. #618

Conversation

j0sh commented Dec 11, 2018 • edited Loading

Multistream Changes

RNG

Common Struct for Per-Connection State

RTMP-HLS ManifestID Sharing

Stream Keys

Authorizing Incoming RTMP

j0sh commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0sh Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0sh Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

ericxtang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yondonfu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0sh commented Dec 11, 2018 •

edited

Loading

j0sh Dec 19, 2018 •

edited

Loading

j0sh Dec 18, 2018 •

edited

Loading