[MM-26385] Include cache and rate limiter to GH client

config/config-mattermod.default.json

@@ -2,7 +2,7 @@
     "ListenAddress": "0.0.0.0:8086",
     "MattermodURL": "",
     "GithubAccessToken": "",
-    "GitHubTokenReserve": 50,
+    "GitHubTokenReserve": 10,
     "Org": "",
     "Username": "",
     "CircleCIToken": "",

go.mod

@@ -5,17 +5,21 @@ go 1.14
 require (
 	github.com/aws/aws-sdk-go v1.31.5
 	github.com/cpanato/golang-jenkins v0.0.0-20181010175751-6a66fc16d07d
+	github.com/die-net/lrucache v0.0.0-20181227122439-19a39ef22a11
 	github.com/go-gorp/gorp v2.2.0+incompatible
 	github.com/go-sql-driver/mysql v1.5.0
 	github.com/golang/mock v1.4.3
 	github.com/google/go-github/v32 v32.0.0
 	github.com/gorilla/mux v1.7.4
 	github.com/heroku/docker-registry-client v0.0.0-20190909225348-afc9e1acc3d5
+	github.com/jarcoal/httpmock v1.0.5
+	github.com/m4ns0ur/httpcache v0.0.0-20200426190423-1040e2e8823f
 	github.com/mattermost/go-circleci v0.4.0
 	github.com/mattermost/mattermost-server/v5 v5.23.0
 	github.com/pkg/errors v0.9.1
 	github.com/poy/onpar v1.0.0 // indirect
 	github.com/robfig/cron/v3 v3.0.1
 	github.com/stretchr/testify v1.5.1
 	golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d
+	golang.org/x/time v0.0.0-20190308202827-9d24e82272b4
 )

go.sum

@@ -80,6 +80,7 @@ github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs
 github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ=
 github.com/dgryski/dgoogauth v0.0.0-20190221195224-5a805980a5f3/go.mod h1:hEfFauPHz7+NnjR/yHJGhrKo1Za+zStgwUETx3yzqgY=
 github.com/dgryski/go-sip13 v0.0.0-20181026042036-e10d5fee7954/go.mod h1:vAd38F8PWV+bWy6jNmig1y/TA+kYO4g3RSRF0IAv0no=
+github.com/die-net/lrucache v0.0.0-20181227122439-19a39ef22a11 h1:tFq0KToN9jzAQI8o1eIgIv+wDK6p2v+OD3yuQrAbwmA=
 github.com/die-net/lrucache v0.0.0-20181227122439-19a39ef22a11/go.mod h1:ew0MSjCVDdtGMjF3kzLK9hwdgF5mOE8SbYVF3Rc7mkU=
 github.com/disintegration/imaging v1.6.0/go.mod h1:xuIt+sRxDFrHS0drzXUlCJthkJ8k7lkkUojDSR247MQ=
 github.com/disintegration/imaging v1.6.2/go.mod h1:44/5580QXChDfwIclfc/PCwrr44amcmDAg8hxG0Ewe4=
@@ -214,6 +215,7 @@ github.com/gorilla/websocket v1.4.1/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/ad
 github.com/gostaticanalysis/analysisutil v0.0.0-20190318220348-4088753ea4d3/go.mod h1:eEOZF4jCKGi+aprrirO9e7WKB3beBRtWgqGunKl6pKE=
 github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
 github.com/gregjones/httpcache v0.0.0-20190212212710-3befbb6ad0cc/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
+github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79 h1:+ngKgrYPPJrOjhax5N+uePQ0Fh1Z7PheYoUI/0nzkPA=
 github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
 github.com/grpc-ecosystem/go-grpc-middleware v1.0.0/go.mod h1:FiyG127CGDf3tlThmgyCl78X/SZQqEOJBCDaAfeWzPs=
 github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk=
@@ -253,6 +255,8 @@ github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpO
 github.com/icrowley/fake v0.0.0-20180203215853-4178557ae428/go.mod h1:uhpZMVGznybq1itEKXj6RYw9I71qK4kH+OGMjRC4KEo=
 github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
 github.com/jamiealquiza/envy v1.1.0/go.mod h1:MP36BriGCLwEHhi1OU8E9569JNZrjWfCvzG7RsPnHus=
+github.com/jarcoal/httpmock v1.0.5 h1:cHtVEcTxRSX4J0je7mWPfc9BpDpqzXSJ5HbymZmyHck=
+github.com/jarcoal/httpmock v1.0.5/go.mod h1:ATjnClrvW/3tijVmpL/va5Z3aAyGvqU3gCT8nX0Txik=
 github.com/jaytaylor/html2text v0.0.0-20190408195923-01ec452cbe43/go.mod h1:CVKlgaMiht+LXvHG173ujK6JUhZXKb2u/BQtjPDIvyk=
 github.com/jellevandenhooff/dkim v0.0.0-20150330215556-f50fe3d243e1/go.mod h1:E0B/fFc00Y+Rasa88328GlI/XbtyysCtTHZS8h7IrBU=
 github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af h1:pmfjZENx5imkbgOkpRUYLnmbU7UEFbjtDA2hxJ1ichM=
@@ -292,6 +296,8 @@ github.com/lib/pq v1.3.0 h1:/qkRGz8zljWiDcFvgpwUpwIAPu3r07TDvs3Rws+o/pU=
 github.com/lib/pq v1.3.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
 github.com/logrusorgru/aurora v0.0.0-20181002194514-a7b3b318ed4e/go.mod h1:7rIyQOR62GCctdiQpZ/zOJlFyk6y+94wXzv6RNZgaR4=
 github.com/lunixbochs/vtclean v1.0.0/go.mod h1:pHhQNgMf3btfWnGBVipUOjRYhoOsdGqdm/+2c2E2WMI=
+github.com/m4ns0ur/httpcache v0.0.0-20200426190423-1040e2e8823f h1:MBcrTbmCf7CZa9yAwcB7ArveQb9TPVy4zFnQGz/LiUU=
+github.com/m4ns0ur/httpcache v0.0.0-20200426190423-1040e2e8823f/go.mod h1:UawoqorwkpZ58qWiL+nVJM0Po7FrzAdCxYVh9GgTTaA=
 github.com/magiconair/properties v1.7.6/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
 github.com/magiconair/properties v1.8.0/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
 github.com/magiconair/properties v1.8.1/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
@@ -667,6 +673,7 @@ golang.org/x/text v0.3.2 h1:tW2bmiBqwgJj/UpqtC8EpXEZVYOwU0yG4iWbprSVAcs=
 golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
 golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
+golang.org/x/time v0.0.0-20190308202827-9d24e82272b4 h1:SvFZT6jyqRaOeXpc5h/JSfZenJ2O330aBsf7JfSUXmQ=
 golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/tools v0.0.0-20170915040203-e531a2a1c15f/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=

server/github_client.go

@@ -5,9 +5,14 @@ package server
 
 import (
 	"context"
+	"errors"
+	"time"
 
+	"github.com/die-net/lrucache"
 	"github.com/google/go-github/v32/github"
+	"github.com/m4ns0ur/httpcache"
 	"golang.org/x/oauth2"
+	"golang.org/x/time/rate"
 )
 
 type ChecksService interface {
@@ -71,10 +76,21 @@ type GithubClient struct {
 	Repositories  RepositoriesService
 }
 
-func NewGithubClient(accessToken string) *GithubClient {
+// NewGithubClientWithLimiter returns a new Github client with the provided limit and burst tokens
+// that will be used by the rate limit transport.
+func NewGithubClientWithLimiter(accessToken string, limit rate.Limit, burstTokens int) *GithubClient {
+	const (
+		lruCacheMaxSizeInBytes  = 1000 * 1000 * 1000 // 1GB
+		lruCacheMaxAgeInSeconds = 2629800            // 1 month
+	)
+
 	ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: accessToken})
 	tc := oauth2.NewClient(context.Background(), ts)
-	client := github.NewClient(tc)
+	limiterTransport := NewRateLimitTransport(limit, burstTokens, tc.Transport)
+	httpCache := lrucache.New(lruCacheMaxSizeInBytes, lruCacheMaxAgeInSeconds)
+	httpCacheTransport := httpcache.NewTransport(httpCache)
+	httpCacheTransport.Transport = limiterTransport
+	client := github.NewClient(httpCacheTransport.Client())
 
 	return &GithubClient{
 		client:        client,
@@ -87,6 +103,16 @@ func NewGithubClient(accessToken string) *GithubClient {
 	}
 }
 
+// NewGithubClient returns a new Github client that will use a fixed 10 req/sec / 10 burst
+// tokens rate limiter configuration
+func NewGithubClient(accessToken string, limitTokens int) (*GithubClient, error) {
+	if limitTokens <= 0 {
+		return nil, errors.New("rate limit tokens for github client must be greater than 0")
+	}
+	limit := rate.Every(time.Second / time.Duration(limitTokens))
+	return NewGithubClientWithLimiter(accessToken, limit, limitTokens), nil
+}
+
 func (c *GithubClient) RateLimits(ctx context.Context) (*github.RateLimits, *github.Response, error) {
 	return c.client.RateLimits(ctx)
 }

server/github_test.go

@@ -1,18 +1,35 @@
 package server_test
 
 import (
+	"context"
+	"encoding/json"
 	"net/http"
 	"strconv"
 	"testing"
+	"time"
 
 	"github.com/golang/mock/gomock"
 	"github.com/google/go-github/v32/github"
+	"github.com/jarcoal/httpmock"
+	"golang.org/x/time/rate"
 
 	"github.com/mattermost/mattermost-mattermod/server"
 	"github.com/mattermost/mattermost-mattermod/server/mocks"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+var testGetRefReturn = []byte(`{
+  "ref": "refs/heads/featureA",
+  "node_id": "MDM6UmVmcmVmcy9oZWFkcy9mZWF0dXJlQQ==",
+  "url": "https://api.github.com/repos/octocat/Hello-World/git/refs/heads/featureA",
+  "object": {
+    "type": "commit",
+    "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd",
+    "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd"
+  }
+}`)
+
 func TestIsOrgMember(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
@@ -98,3 +115,223 @@ func TestCannotGetAllOrgMembersDueToRateLimit(t *testing.T) {
 
 	assert.Equal(t, originalUserSize, len(s.OrgMembers))
 }
+
+func TestCacheTransport(t *testing.T) {
+	t.Run("Should return cached response", func(t *testing.T) {
+		httpmock.Activate()
+		defer httpmock.DeactivateAndReset()
+
+		httpmock.RegisterResponder("GET", "https://api.github.com/repos/ownerTest/repoTest/git/ref/refTest",
+			func(req *http.Request) (*http.Response, error) {
+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal(testGetRefReturn, &body)
+				require.NoError(t, err)
+				resp, err := httpmock.NewJsonResponse(http.StatusOK, body)
+				// Needed by httpcache cache the response
+				resp.Header.Set("Date", time.Now().Format(time.RFC1123))
+				resp.Header.Set("Cache-Control", "max-age=60")
+				return resp, err
+			},
+		)
+
+		// First request should return a non-cached request
+		ghClient, _ := server.NewGithubClient("testtoken", 10)
+		_, resp, err := ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+
+		// This part should answer the cached response because max-age hasn't expired
+		_, resp, err = ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "1", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+	})
+
+	t.Run("Shouldn't return cached response if max-age expires", func(t *testing.T) {
+		httpmock.Activate()
+		defer httpmock.DeactivateAndReset()
+
+		httpmock.RegisterResponder("GET", "https://api.github.com/repos/ownerTest/repoTest/git/ref/refTest",
+			func(req *http.Request) (*http.Response, error) {
+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal(testGetRefReturn, &body)
+				require.NoError(t, err)
+				resp, err := httpmock.NewJsonResponse(http.StatusOK, body)
+				// Needed by httpcache cache the response
+				resp.Header.Set("Date", time.Now().Format(time.RFC1123))
+				resp.Header.Set("Cache-Control", "max-age=0")
+				return resp, err
+			},
+		)
+
+		// First request should return a non-cached request
+		ghClient, _ := server.NewGithubClient("testtoken", 10)
+		_, resp, err := ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+
+		// Here we should return a non-cached request because the max-age value has expired
+		_, resp, err = ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+	})
+
+	t.Run("Should returned cached response if Expires is defined", func(t *testing.T) {
+		httpmock.Activate()
+		defer httpmock.DeactivateAndReset()
+
+		httpmock.RegisterResponder("GET", "https://api.github.com/repos/ownerTest/repoTest/git/ref/refTest",
+			func(req *http.Request) (*http.Response, error) {
+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal(testGetRefReturn, &body)
+				require.NoError(t, err)
+				resp, err := httpmock.NewJsonResponse(http.StatusOK, body)
+				expireTime := time.Now().Local().Add(time.Minute * time.Duration(1))
+				resp.Header.Set("Date", time.Now().Format(time.RFC1123))
+				resp.Header.Set("Expires", expireTime.Format(time.RFC1123))
+				return resp, err
+			},
+		)
+
+		// First request should return a non-cached request
+		ghClient, _ := server.NewGithubClient("testtoken", 10)
+		_, resp, err := ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+
+		// Here we should return a cached request
+		_, resp, err = ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "1", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+	})
+
+	t.Run("Shouldn't return cached response if Expires header expired", func(t *testing.T) {
+		httpmock.Activate()
+		defer httpmock.DeactivateAndReset()
+
+		httpmock.RegisterResponder("GET", "https://api.github.com/repos/ownerTest/repoTest/git/ref/refTest",
+			func(req *http.Request) (*http.Response, error) {
+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal(testGetRefReturn, &body)
+				require.NoError(t, err)
+				resp, err := httpmock.NewJsonResponse(http.StatusOK, body)
+				expireTime := time.Now().Local().Add(-time.Minute * time.Duration(1))
+				resp.Header.Set("Date", time.Now().Format(time.RFC1123))
+				resp.Header.Set("Expires", expireTime.Format(time.RFC1123))
+				return resp, err
+			},
+		)
+
+		// First request should return a non-cached request
+		ghClient, _ := server.NewGithubClient("testtoken", 10)
+		_, resp, err := ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+
+		// Here we should return a non-cached request because the max-age value has expired
+		_, resp, err = ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+	})
+}
+
+func TestRateLimitTransport(t *testing.T) {
+	t.Run("Should be able to perform a request without being hit by rate limiter", func(t *testing.T) {
+		httpmock.Activate()
+		defer httpmock.DeactivateAndReset()
+		ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+		defer cancel()
+
+		httpmock.RegisterResponder("GET", "https://api.github.com/repos/ownerTest/repoTest/git/ref/refTest",
+			func(req *http.Request) (*http.Response, error) {
+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal(testGetRefReturn, &body)
+				require.NoError(t, err)
+				return httpmock.NewJsonResponse(http.StatusOK, body)
+			},
+		)
+
+		ghClient, _ := server.NewGithubClient("testtoken", 1)
+		_, resp, err := ghClient.Git.GetRef(ctx, "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		require.Equal(t, "", resp.Header.Get("X-From-Cache"))
+		require.Equal(t, http.StatusOK, resp.StatusCode)
+	})
+
+	t.Run("Should return error when the rate limit is exceeded", func(t *testing.T) {
+		httpmock.Activate()
+		defer httpmock.DeactivateAndReset()
+
+		httpmock.RegisterResponder("GET", "https://api.github.com/repos/ownerTest/repoTest/git/ref/refTest",
+			func(req *http.Request) (*http.Response, error) {
+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal(testGetRefReturn, &body)
+				require.NoError(t, err)
+				return httpmock.NewJsonResponse(http.StatusOK, body)
+			},
+		)
+
+		ghClient := server.NewGithubClientWithLimiter("testtoken", 0, 0)
+		_, _, err := ghClient.Git.GetRef(context.TODO(), "ownerTest", "repoTest", "refTest")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "exceeds limiter's burst 0")
+	})
+
+	t.Run("Should return context error when the rate limit wait is larger than the context timeout", func(t *testing.T) {
+		httpmock.Activate()
+		defer httpmock.DeactivateAndReset()
+
+		httpmock.RegisterResponder("GET", "https://api.github.com/repos/ownerTest/repoTest/git/ref/refTest",
+			func(req *http.Request) (*http.Response, error) {
+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal(testGetRefReturn, &body)
+				require.NoError(t, err)
+				return httpmock.NewJsonResponse(http.StatusOK, body)
+			},
+		)
+
+		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Microsecond)
+		defer cancel()
+		limit := rate.Every(time.Minute * 1)
+		ghClient := server.NewGithubClientWithLimiter("testtoken", limit, 10)
+		_, _, err := ghClient.Git.GetRef(ctx, "ownerTest", "repoTest", "refTest")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "would exceed context deadline")
+	})
+
+	t.Run("Should delay the request execution until the rate limiter have room", func(t *testing.T) {
+		httpmock.Activate()
+		defer httpmock.DeactivateAndReset()
+
+		httpmock.RegisterResponder("GET", "https://api.github.com/repos/ownerTest/repoTest/git/ref/refTest",
+			func(req *http.Request) (*http.Response, error) {
+				body := &github.Reference{Object: &github.GitObject{}}
+				err := json.Unmarshal(testGetRefReturn, &body)
+				require.NoError(t, err)
+				return httpmock.NewJsonResponse(http.StatusOK, body)
+			},
+		)
+
+		ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+		defer cancel()
+		limit := rate.Every(time.Millisecond * 100)
+		ghClient := server.NewGithubClientWithLimiter("testtoken", limit, 1)
+		_, _, err := ghClient.Git.GetRef(ctx, "ownerTest", "repoTest", "refTest")
+		require.NoError(t, err)
+		start := time.Now()
+		_, _, err = ghClient.Git.GetRef(ctx, "ownerTest", "repoTest", "refTest")
+		elapsed := time.Since(start)
+		require.NoError(t, err)
+		// With a rate limiting of 10 requests per second, or 1 per 100ms)
+		// rate limit is going to make the second request wait because is
+		// too fast
+		require.True(t, elapsed*time.Millisecond > 90*time.Millisecond)
+	})
+}