[SRE-160] CDN Redirect - add missing playbackID prefix

balancer/balancer.go

@@ -308,12 +308,13 @@ func (b *BalancerImpl) GetBestNode(ctx context.Context, redirectPrefixes []strin
 	for _, prefix := range redirectPrefixes {
 		waitGroup.Add(1)
 		go func(prefix string) {
+			defer waitGroup.Done()
 			addr, e := b.queryMistForClosestNode(ctx, playbackID, lat, lon, prefix)
 			mu.Lock()
 			defer mu.Unlock()
 			if e != nil {
 				err = e
-				glog.V(8).Infof("error finding origin server playbackID=%s prefix=%s error=%s", playbackID, prefix, e)
+				glog.V(9).Infof("error finding origin server playbackID=%s prefix=%s error=%s", playbackID, prefix, e)
 				// If we didn't find a stream but we did find a server, keep that so we can use it to handle a 404
 				if addr != "" {
 					fallbackAddr = addr
@@ -322,7 +323,6 @@ func (b *BalancerImpl) GetBestNode(ctx context.Context, redirectPrefixes []strin
 				nodeAddr = addr
 				fullPlaybackID = prefix + "+" + playbackID
 			}
-			waitGroup.Done()
 		}(prefix)
 	}
 	waitGroup.Wait()

handlers/geolocation/gelocation_test.go

@@ -2,6 +2,7 @@ package geolocation
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math/rand"
 	"net/http"
@@ -21,8 +22,10 @@ import (
 )
 
 const (
-	closestNodeAddr = "someurl.com"
-	playbackID      = "abc_XYZ-123"
+	closestNodeAddr         = "someurl.com"
+	playbackID              = "we91yp6cmq41niox"
+	CdnRedirectedPlaybackID = "cdn40y22lq7z1m8o"
+	UnknownPlaybackID       = "unknown2aybmvI02"
 )
 
 var fakeSerfMember = cluster.Member{
@@ -130,6 +133,16 @@ func mockHandlers(t *testing.T) *GeolocationHandlersCollection {
 		AnyTimes().
 		Return(closestNodeAddr, fmt.Sprintf("%s+%s", prefixes[0], playbackID), nil)
 
+	mb.EXPECT().
+		GetBestNode(context.Background(), prefixes[:], CdnRedirectedPlaybackID, "", "", "").
+		AnyTimes().
+		Return(closestNodeAddr, fmt.Sprintf("%s+%s", prefixes[0], CdnRedirectedPlaybackID), nil)
+
+	mb.EXPECT().
+		GetBestNode(context.Background(), prefixes[:], UnknownPlaybackID, "", "", "").
+		AnyTimes().
+		Return("", "", errors.New(""))
+
 	mc.EXPECT().
 		Member(map[string]string{}, "alive", closestNodeAddr).
 		AnyTimes().
@@ -333,10 +346,9 @@ func TestNodeHostPortRedirect(t *testing.T) {
 
 func TestCdnRedirect(t *testing.T) {
 	n := mockHandlers(t)
-	CdnRedirectedPlaybackId := "def_ZXY-999"
-	n.Config.NodeHost = "someurl.com"
+	n.Config.NodeHost = closestNodeAddr
 	n.Config.CdnRedirectPrefix, _ = url.Parse("https://external-cdn.com/mist")
-	n.Config.CdnRedirectPlaybackIDs = []string{CdnRedirectedPlaybackId}
+	n.Config.CdnRedirectPlaybackIDs = []string{CdnRedirectedPlaybackID}
 
 	// to be redirected to the closest node
 	requireReq(t, fmt.Sprintf("/hls/%s/index.m3u8", playbackID)).
@@ -345,38 +357,73 @@ func TestCdnRedirect(t *testing.T) {
 		hasHeader("Location", fmt.Sprintf("http://%s/hls/%s/index.m3u8", closestNodeAddr, playbackID))
 
 	// playbackID is configured to be redirected to CDN but the path is /json_video... so redirect it to the closest node
-	requireReq(t, fmt.Sprintf("/json_video+%s.js", CdnRedirectedPlaybackId)).
+	requireReq(t, fmt.Sprintf("/json_video+%s.js", CdnRedirectedPlaybackID)).
 		result(n).
 		hasStatus(http.StatusTemporaryRedirect).
-		hasHeader("Location", fmt.Sprintf("http://%s/json_video+%s.js", closestNodeAddr, CdnRedirectedPlaybackId))
+		hasHeader("Location", fmt.Sprintf("http://%s/json_video+%s.js", closestNodeAddr, CdnRedirectedPlaybackID))
+
+	// don't redirect if `CdnRedirectPrefix` is not configured
+	n.Config.CdnRedirectPrefix = nil
+	requireReq(t, fmt.Sprintf("/hls/%s/index.m3u8", CdnRedirectedPlaybackID)).
+		result(n).
+		hasStatus(http.StatusTemporaryRedirect).
+		hasHeader("Location", fmt.Sprintf("http://%s/hls/%s/index.m3u8", closestNodeAddr, CdnRedirectedPlaybackID))
+}
+
+func TestCdnRedirectWebRTC(t *testing.T) {
+	n := mockHandlers(t)
+	n.Config.NodeHost = closestNodeAddr
+	n.Config.CdnRedirectPrefix, _ = url.Parse("https://external-cdn.com/mist")
+	n.Config.CdnRedirectPlaybackIDs = []string{CdnRedirectedPlaybackID}
 
 	// playbackID is configured to be redirected to CDN but it's /webrtc
-	require.Equal(t, testutil.CollectAndCount(metrics.Metrics.CDNRedirectWebRTC406), 0)
-	requireReq(t, fmt.Sprintf("/webrtc/%s", CdnRedirectedPlaybackId)).
+	requireReq(t, fmt.Sprintf("/webrtc/%s", CdnRedirectedPlaybackID)).
 		result(n).
 		hasStatus(http.StatusNotAcceptable)
-	require.Equal(t, testutil.CollectAndCount(metrics.Metrics.CDNRedirectWebRTC406), 1)
+
+	require.Equal(t, testutil.ToFloat64(metrics.Metrics.CDNRedirectWebRTC406), float64(1))
 	require.Equal(t, testutil.ToFloat64(metrics.Metrics.CDNRedirectWebRTC406.WithLabelValues("unknown")), float64(0))
-	require.Equal(t, testutil.ToFloat64(metrics.Metrics.CDNRedirectWebRTC406.WithLabelValues(CdnRedirectedPlaybackId)), float64(1))
+	require.Equal(t, testutil.ToFloat64(metrics.Metrics.CDNRedirectWebRTC406.WithLabelValues(CdnRedirectedPlaybackID)), float64(1))
 
-	// this playbackID is configured to be redirected to CDN
-	require.Equal(t, testutil.CollectAndCount(metrics.Metrics.CDNRedirectCount), 0)
+}
+
+func TestCdnRedirectHLS(t *testing.T) {
+	n := mockHandlers(t)
+	n.Config.NodeHost = closestNodeAddr
+	n.Config.CdnRedirectPrefix, _ = url.Parse("https://external-cdn.com/mist")
+	n.Config.CdnRedirectPlaybackIDs = []string{CdnRedirectedPlaybackID}
 
-	requireReq(t, fmt.Sprintf("/hls/%s/index.m3u8", CdnRedirectedPlaybackId)).
+	// this playbackID is configured to be redirected to CDN
+	requireReq(t, fmt.Sprintf("/hls/%s/index.m3u8", CdnRedirectedPlaybackID)).
 		result(n).
 		hasStatus(http.StatusTemporaryRedirect).
-		hasHeader("Location", fmt.Sprintf("http://external-cdn.com/mist/hls/%s/index.m3u8", CdnRedirectedPlaybackId))
+		hasHeader("Location", fmt.Sprintf("http://external-cdn.com/mist/hls/video+%s/index.m3u8", CdnRedirectedPlaybackID))
 
-	require.Equal(t, testutil.CollectAndCount(metrics.Metrics.CDNRedirectCount), 1)
+	require.Equal(t, testutil.ToFloat64(metrics.Metrics.CDNRedirectCount), float64(1))
 	require.Equal(t, testutil.ToFloat64(metrics.Metrics.CDNRedirectCount.WithLabelValues("unknown")), float64(0))
-	require.Equal(t, testutil.ToFloat64(metrics.Metrics.CDNRedirectCount.WithLabelValues(CdnRedirectedPlaybackId)), float64(1))
+	require.Equal(t, testutil.ToFloat64(metrics.Metrics.CDNRedirectCount.WithLabelValues(CdnRedirectedPlaybackID)), float64(1))
 
-	// don't redirect if `CdnRedirectPrefix` is not configured
-	n.Config.CdnRedirectPrefix = nil
-	requireReq(t, fmt.Sprintf("/hls/%s/index.m3u8", CdnRedirectedPlaybackId)).
+}
+
+func TestCdnRedirectHLSUnknownPlaybackId(t *testing.T) {
+	n := mockHandlers(t)
+	n.Config.NodeHost = closestNodeAddr
+	n.Config.CdnRedirectPrefix, _ = url.Parse("https://external-cdn.com/mist")
+	n.Config.CdnRedirectPlaybackIDs = []string{CdnRedirectedPlaybackID, UnknownPlaybackID}
+
+	// Mist doesn't know this playbackID at all
+	requireReq(t, fmt.Sprintf("/hls/%s/index.m3u8", UnknownPlaybackID)).
 		result(n).
-		hasStatus(http.StatusTemporaryRedirect).
-		hasHeader("Location", fmt.Sprintf("http://%s/hls/%s/index.m3u8", closestNodeAddr, CdnRedirectedPlaybackId))
+		hasStatus(http.StatusBadGateway)
+
+	defer func() {
+		if err := recover(); err == nil {
+			panic(err)
+		}
+	}()
+
+	// raises a panic() as there are no metrics collected
+	testutil.ToFloat64(metrics.Metrics.CDNRedirectCount)
 
 }
 

handlers/geolocation/geolocation.go

@@ -31,6 +31,9 @@ func (c *GeolocationHandlersCollection) RedirectHandler() httprouter.Handle {
 	return func(w http.ResponseWriter, r *http.Request, params httprouter.Params) {
 		host := r.Host
 		pathType, prefix, playbackID, pathTmpl := parsePlaybackID(r.URL.Path)
+		redirectPrefixes := c.Config.RedirectPrefixes
+		lat := r.Header.Get("X-Latitude")
+		lon := r.Header.Get("X-Longitude")
 
 		if c.Config.CdnRedirectPrefix != nil && (pathType == "hls" || pathType == "webrtc") {
 			if contains(c.Config.CdnRedirectPlaybackIDs, playbackID) {
@@ -43,10 +46,18 @@ func (c *GeolocationHandlersCollection) RedirectHandler() httprouter.Handle {
 					glog.V(6).Infof("%s not supported for CDN-redirected %s", r.URL.Path, playbackID)
 					return
 				}
+
+				_, fullPlaybackID, err := c.Balancer.GetBestNode(context.Background(), redirectPrefixes, playbackID, lat, lon, prefix)
+				if err != nil {
+					glog.Errorf("failed to find either origin or fallback server for playbackID=%s err=%s", playbackID, err)
+					w.WriteHeader(http.StatusBadGateway)
+					return
+				}
+
 				newURL, _ := url.Parse(r.URL.String())
 				newURL.Scheme = protocol(r)
 				newURL.Host = c.Config.CdnRedirectPrefix.Host
-				newURL.Path, _ = url.JoinPath(c.Config.CdnRedirectPrefix.Path, newURL.Path)
+				newURL.Path, _ = url.JoinPath(c.Config.CdnRedirectPrefix.Path, fmt.Sprintf(pathTmpl, fullPlaybackID))
 				http.Redirect(w, r, newURL.String(), http.StatusTemporaryRedirect)
 				metrics.Metrics.CDNRedirectCount.WithLabelValues(playbackID).Inc()
 				glog.V(6).Infof("CDN redirect host=%s from=%s to=%s", host, r.URL, newURL)
@@ -76,10 +87,6 @@ func (c *GeolocationHandlersCollection) RedirectHandler() httprouter.Handle {
 			return
 		}
 
-		lat := r.Header.Get("X-Latitude")
-		lon := r.Header.Get("X-Longitude")
-
-		redirectPrefixes := c.Config.RedirectPrefixes
 		bestNode, fullPlaybackID, err := c.Balancer.GetBestNode(context.Background(), redirectPrefixes, playbackID, lat, lon, prefix)
 		if err != nil {
 			glog.Errorf("failed to find either origin or fallback server for playbackID=%s err=%s", playbackID, err)