Webhook from github to jenkins fails with 500 Status (#342)

* Webhook from github to jenkins fails with 500 Status

The code was using an empty namespace, fixed that

Added test check for namespace

More tests for github webhook requests

Fixes openshiftio/openshift.io#4600

internal/proxy/github.go

@@ -57,6 +57,8 @@ func (p *Proxy) handleGitHubRequest(w http.ResponseWriter, r *http.Request, requ
 		return
 	}
 
+	ns = namespace.Name
+
 	nsLogger := requestLogEntry.WithField("ns", ns)
 
 	pci := CacheItem{
@@ -101,6 +103,7 @@ func (p *Proxy) handleGitHubRequest(w http.ResponseWriter, r *http.Request, requ
 	r.Body = ioutil.NopCloser(bytes.NewReader(body))
 	//If Jenkins is up, we can simply proxy through
 	nsLogger.Infof("Passing through %s", r.URL.String())
+	w.WriteHeader(http.StatusOK)
 	return
 }
 

internal/proxy/github_test.go

@@ -6,18 +6,71 @@ import (
 	"net/http/httptest"
 	"testing"
 
+	"github.com/fabric8-services/fabric8-jenkins-proxy/internal/idler"
+	"github.com/fabric8-services/fabric8-jenkins-proxy/internal/wit"
+
 	uuid "github.com/satori/go.uuid"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestWebhookRequestHasGHHeader(t *testing.T) {
-	p := NewMock("")
+	p := NewMock(idler.Idled, wit.DefaultMockOwner)
 	req := httptest.NewRequest("GET", "http://proxy", nil)
 	req.Header.Add("User-Agent", "GitHub-Hookshot"+uuid.NewV4().String())
 	assert.True(t, p.isGitHubRequest(req), "p.isGitHubRequest(req) should evaluate to true")
 }
-func TestGHWebHookRequest(t *testing.T) {
-	p := NewMock("")
+func TestGHWebHookRequestJenkinsIdled(t *testing.T) {
+	testGHWebHook(t, idler.Idled)
+}
+
+func TestGHWebHookRequestJenkinsRunning(t *testing.T) {
+	testGHWebHook(t, idler.Running)
+}
+
+func testGHWebHook(t *testing.T, jenkinsState idler.PodState) {
+	p := NewMock(jenkinsState, wit.DefaultMockOwner)
+
+	w, req := getGHWebHookRecorderAndRequest()
+	ns, okToForward := p.handleGitHubRequest(w, req, proxyLogger)
+
+	assert.Equal(t, ns, "namespace-jenkins")
+
+	_, ok := p.TenantCache.Get("https://github.com/test-username/test-repo.git")
+	assert.True(t, ok, "An entry should have been created in tenant cache with repo url as key")
+
+	if jenkinsState == idler.Running {
+		assert.True(t, okToForward, "It should be ok to forward, because state of jenkins is running")
+		// writes status http.StatusOK when request is ok to forward
+		assert.Equal(t, http.StatusOK, w.Code)
+	} else if jenkinsState == idler.Idled {
+		assert.False(t, okToForward, "It should not be ok to forward, because state of jenkins is idled")
+		// writes status http.StatusAccepted on accepting and storing the request
+		assert.Equal(t, http.StatusAccepted, w.Code)
+	}
+}
+
+func TestGHWebHookUnableToGetUser(t *testing.T) {
+	p := NewMock(idler.Idled, "")
+
+	w, req := getGHWebHookRecorderAndRequest()
+	ns, okToForward := p.handleGitHubRequest(w, req, proxyLogger)
+
+	// Since we failed to get user because wit.OwnedBy being empty,
+	// we should have go an empty namespace and it should not be
+	// ok to forward
+	assert.Equal(t, ns, "")
+
+	_, ok := p.TenantCache.Get("https://github.com/test-username/test-repo.git")
+	assert.False(t, ok, `An entry should not have been created in tenant cache with repo url as key,
+		since we failed to get the namespace associated with this repo url`)
+
+	assert.False(t, okToForward, "It should not be ok to forward, because state of jenkins is idled")
+
+	assert.Equal(t, http.StatusInternalServerError, w.Code)
+
+}
+
+func getGHWebHookRecorderAndRequest() (*httptest.ResponseRecorder, *http.Request) {
 	gh := []byte(`{
 		"repository": {
 			"name": "test-repo",
@@ -26,12 +79,10 @@ func TestGHWebHookRequest(t *testing.T) {
 			"clone_url": "https://github.com/test-username/test-repo.git"
 		}
 	}`)
+
 	w := httptest.NewRecorder()
 	req := httptest.NewRequest("GET", "http://proxy", bytes.NewBuffer(gh))
 	req.Header.Add("User-Agent", "GitHub-Hookshot"+uuid.NewV4().String())
-	p.handleGitHubRequest(w, req, proxyLogger)
-	_, ok := p.TenantCache.Get("https://github.com/test-username/test-repo.git")
-	assert.True(t, ok, "An entry should have been created in tenant cache with repo url as key")
-	// writes status http.StatusAccepted on accepting and storing the request
-	assert.Equal(t, w.Code, http.StatusAccepted)
+
+	return w, req
 }

internal/proxy/jenkins.go

@@ -2,6 +2,7 @@ package proxy
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -41,18 +42,22 @@ func GetJenkins(clusters map[string]string,
 	logger *log.Entry) (j *Jenkins, osioToken string, err error) {
 
 	if pci != nil {
-		return &Jenkins{
-			info:   *pci,
-			idler:  idler,
-			tenant: tenantClient,
-			logger: logger.WithFields(log.Fields{"ns": pci.NS, "cluster": pci.ClusterURL}),
-		}, "", nil
+		if pci.NS != "" && pci.ClusterURL != "" {
+			return &Jenkins{
+				info:   *pci,
+				idler:  idler,
+				tenant: tenantClient,
+				logger: logger.WithFields(log.Fields{"ns": pci.NS, "cluster": pci.ClusterURL}),
+			}, "", nil
+		}
+
+		return &Jenkins{}, "", errors.New("empty namespace and cluster URL in passed proxy cache item")
 	}
 
 	tokenJSON := &auth.TokenJSON{}
 	err = json.Unmarshal([]byte(tokenData), tokenJSON)
 	if err != nil {
-		return j, "", err
+		return &Jenkins{}, "", err
 	}
 
 	authClient, err := auth.DefaultClient()

internal/proxy/mock_proxy.go

@@ -14,7 +14,7 @@ import (
 )
 
 // NewMock returns an instance of proxy object that uses mocked dependency services
-func NewMock(jenkinsState idler.PodState) *Proxy {
+func NewMock(jenkinsState idler.PodState, ownedBy string) *Proxy {
 
 	if jenkinsState == "" {
 		jenkinsState = idler.Idled
@@ -24,7 +24,9 @@ func NewMock(jenkinsState idler.PodState) *Proxy {
 	return &Proxy{
 		tenant: &tenant.Mock{},
 		idler:  idler.NewMock("", jenkinsState, false),
-		wit:    &wit.Mock{},
+		wit: &wit.Mock{
+			OwnedBy: ownedBy,
+		},
 		clusters: map[string]string{
 			"Valid_OpenShift_API_URL": "test_route",
 		},

internal/proxy/ui_requests_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/fabric8-services/fabric8-jenkins-proxy/internal/configuration"
 	"github.com/fabric8-services/fabric8-jenkins-proxy/internal/idler"
 	"github.com/fabric8-services/fabric8-jenkins-proxy/internal/proxy/reverseproxy"
+	"github.com/fabric8-services/fabric8-jenkins-proxy/internal/wit"
 	log "github.com/sirupsen/logrus"
 
 	uuid "github.com/satori/go.uuid"
@@ -22,7 +23,7 @@ const (
 )
 
 func TestBadToken(t *testing.T) {
-	p := NewMock("")
+	p := NewMock("", wit.DefaultMockOwner)
 	req := httptest.NewRequest("GET", "http://proxy?token_json=BADTOKEN", nil)
 	w := httptest.NewRecorder()
 
@@ -32,7 +33,7 @@ func TestBadToken(t *testing.T) {
 
 func TestNoTokenRedirectstoAuthService(t *testing.T) {
 
-	p := NewMock("")
+	p := NewMock("", wit.DefaultMockOwner)
 
 	req := httptest.NewRequest("GET", "http://proxy", nil)
 	w := httptest.NewRecorder()
@@ -44,7 +45,7 @@ func TestNoTokenRedirectstoAuthService(t *testing.T) {
 
 func TestCorrectTokenWithJenkinsIdled(t *testing.T) {
 
-	p := NewMock(idler.Idled)
+	p := NewMock(idler.Idled, wit.DefaultMockOwner)
 
 	req := httptest.NewRequest("GET", "http://proxy?token_json="+testTokenJSON, nil)
 	req.AddCookie(&http.Cookie{
@@ -87,7 +88,7 @@ func TestWithTokenJenkinsRunningButLoginFailed(t *testing.T) {
 		Get("").
 		Reply(401)
 
-	p := NewMock(idler.Running)
+	p := NewMock(idler.Running, wit.DefaultMockOwner)
 
 	req := httptest.NewRequest("GET", "http://proxy?token_json="+testTokenJSON, nil)
 
@@ -106,7 +107,7 @@ func TestWithTokenJenkinsRunningAndLoginSuccessful(t *testing.T) {
 			"Set-Cookie": "JSESSIONID." + uuid.NewV4().String() + "=" + uuid.NewV4().String(),
 		})
 
-	p := NewMock(idler.Running)
+	p := NewMock(idler.Running, wit.DefaultMockOwner)
 
 	req := httptest.NewRequest("GET", "http://proxy?token_json="+testTokenJSON, nil)
 	req.AddCookie(&http.Cookie{
@@ -150,7 +151,7 @@ func TestWithTokenJenkinsRunningAndLoginSuccessful(t *testing.T) {
 }
 
 func TestExpireCookieIfNotInCache(t *testing.T) {
-	p := NewMock("")
+	p := NewMock("", wit.DefaultMockOwner)
 
 	req := httptest.NewRequest("GET", "http://proxy", nil)
 	req.AddCookie(&http.Cookie{
@@ -183,7 +184,7 @@ func TestValidateSession(t *testing.T) {
 
 	config := configuration.NewMock()
 
-	p := NewMock(idler.Running)
+	p := NewMock(idler.Running, wit.DefaultMockOwner)
 	cookieVal := uuid.NewV4().String()
 	info := CacheItem{
 		ClusterURL: "Valid_OpenShift_API_URL",

internal/wit/mock_wit.go

@@ -1,12 +1,16 @@
 package wit
 
+// DefaultMockOwner is a mock owner value to be used by tests
+const DefaultMockOwner = "mockTenantID"
+
 // Mock implementation of WIT Service
 type Mock struct {
+	OwnedBy string
 }
 
 // SearchCodebase is a mock method
 func (w *Mock) SearchCodebase(repo string) (*Info, error) {
 	return &Info{
-		OwnedBy: "mockTenantID",
+		OwnedBy: w.OwnedBy,
 	}, nil
 }