resolve logic errors introduced in openshift#11077 and return 40x errors instead of 500 errors where possible

Author: Jim Minter

pkg/build/registry/buildconfig/webhook.go

@@ -55,11 +55,13 @@ func (w *WebHook) ServeHTTP(writer http.ResponseWriter, req *http.Request, ctx k
 		switch err {
 		case webhook.ErrSecretMismatch, webhook.ErrHookNotEnabled:
 			return errors.NewUnauthorized(fmt.Sprintf("the webhook %q for %q did not accept your secret", hookType, name))
-		case nil:
-			return nil
-		default:
+		case webhook.MethodNotSupported:
+			return errors.NewMethodNotSupported(buildapi.Resource("buildconfighook"), req.Method)
+		}
+		if _, ok := err.(*errors.StatusError); !ok {
 			return errors.NewInternalError(fmt.Errorf("hook failed: %v", err))
 		}
+		return err
 	}
 	warning := err
 

pkg/build/webhook/generic/generic.go

@@ -10,6 +10,7 @@ import (
 	"github.com/golang/glog"
 
 	kapi "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/util/yaml"
 
 	"github.com/openshift/origin/pkg/build/api"
@@ -46,7 +47,7 @@ func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string,
 	if len(contentType) != 0 {
 		contentType, _, err = mime.ParseMediaType(contentType)
 		if err != nil {
-			return revision, envvars, false, fmt.Errorf("error parsing Content-Type: %s", err)
+			return revision, envvars, false, errors.NewBadRequest(fmt.Sprintf("error parsing Content-Type: %s", err))
 		}
 	}
 
@@ -61,7 +62,7 @@ func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string,
 
 	body, err := ioutil.ReadAll(req.Body)
 	if err != nil {
-		return revision, envvars, false, err
+		return revision, envvars, false, errors.NewBadRequest(err.Error())
 	}
 
 	if len(body) == 0 {
@@ -115,8 +116,8 @@ func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string,
 }
 
 func verifyRequest(req *http.Request) error {
-	if method := req.Method; method != "POST" {
-		return fmt.Errorf("Unsupported HTTP method %s", method)
+	if req.Method != "POST" {
+		return webhook.MethodNotSupported
 	}
 	return nil
 }

pkg/build/webhook/generic/generic_test.go

@@ -89,7 +89,7 @@ func TestVerifyRequestForMethod(t *testing.T) {
 	plugin := New()
 	revision, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
 
-	if err == nil || !strings.Contains(err.Error(), "Unsupported HTTP method") {
+	if err == nil || !strings.Contains(err.Error(), "unsupported HTTP method") {
 		t.Errorf("Expected unsupported HTTP method, got %v!", err)
 	}
 	if proceed {

pkg/build/webhook/github/github.go

@@ -2,23 +2,19 @@ package github
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"mime"
 	"net/http"
 
 	kapi "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/errors"
 
 	"github.com/golang/glog"
 	"github.com/openshift/origin/pkg/build/api"
 	"github.com/openshift/origin/pkg/build/webhook"
 )
 
-var (
-	ErrNoGitHubEvent = errors.New("missing X-GitHub-Event, X-Gogs-Event or X-Gitlab-Event")
-)
-
 // WebHook used for processing github webhook requests.
 type WebHook struct{}
 
@@ -58,18 +54,18 @@ func (p *WebHook) Extract(buildCfg *api.BuildConfig, secret, path string, req *h
 	}
 	method := getEvent(req.Header)
 	if method != "ping" && method != "push" && method != "Push Hook" {
-		return revision, envvars, proceed, fmt.Errorf("Unknown X-GitHub-Event, X-Gogs-Event or X-Gitlab-Event %s", method)
+		return revision, envvars, proceed, errors.NewBadRequest(fmt.Sprintf("Unknown X-GitHub-Event, X-Gogs-Event or X-Gitlab-Event %s", method))
 	}
 	if method == "ping" {
 		return revision, envvars, proceed, err
 	}
 	body, err := ioutil.ReadAll(req.Body)
 	if err != nil {
-		return revision, envvars, proceed, err
+		return revision, envvars, proceed, errors.NewBadRequest(err.Error())
 	}
 	var event pushEvent
 	if err = json.Unmarshal(body, &event); err != nil {
-		return revision, envvars, proceed, err
+		return revision, envvars, proceed, errors.NewBadRequest(err.Error())
 	}
 	if !webhook.GitRefMatches(event.Ref, webhook.DefaultConfigRef, &buildCfg.Spec.Source) {
 		glog.V(2).Infof("Skipping build for BuildConfig %s/%s.  Branch reference from '%s' does not match configuration", buildCfg.Namespace, buildCfg, event)
@@ -89,18 +85,18 @@ func (p *WebHook) Extract(buildCfg *api.BuildConfig, secret, path string, req *h
 
 func verifyRequest(req *http.Request) error {
 	if method := req.Method; method != "POST" {
-		return fmt.Errorf("unsupported HTTP method %s", method)
+		return webhook.MethodNotSupported
 	}
 	contentType := req.Header.Get("Content-Type")
 	mediaType, _, err := mime.ParseMediaType(contentType)
 	if err != nil {
-		return fmt.Errorf("non-parseable Content-Type %s (%s)", contentType, err)
+		return errors.NewBadRequest(fmt.Sprintf("non-parseable Content-Type %s (%s)", contentType, err))
 	}
 	if mediaType != "application/json" {
-		return fmt.Errorf("unsupported Content-Type %s", contentType)
+		return errors.NewBadRequest(fmt.Sprintf("unsupported Content-Type %s", contentType))
 	}
 	if len(getEvent(req.Header)) == 0 {
-		return ErrNoGitHubEvent
+		return errors.NewBadRequest("missing X-GitHub-Event, X-Gogs-Event or X-Gitlab-Event")
 	}
 	return nil
 }

pkg/build/webhook/webhook.go

@@ -19,8 +19,9 @@ const (
 )
 
 var (
-	ErrSecretMismatch = errors.New("the provided secret does not match")
-	ErrHookNotEnabled = errors.New("the specified hook is not enabled")
+	ErrSecretMismatch  = errors.New("the provided secret does not match")
+	ErrHookNotEnabled  = errors.New("the specified hook is not enabled")
+	MethodNotSupported = errors.New("unsupported HTTP method")
 )
 
 // Plugin for Webhook verification is dependent on the sending side, it can be