Skip to content

Commit 833eb03

Browse files
author
OpenShift Bot
authored
Merge pull request #11077 from jim-minter/bz1373330
Merged by openshift-bot
2 parents a76ec57 + aea22a7 commit 833eb03

File tree

5 files changed

+207
-72
lines changed

5 files changed

+207
-72
lines changed

‎pkg/build/registry/buildconfig/webhook.go

+10-10
Original file line numberDiff line numberDiff line change
@@ -51,17 +51,17 @@ func (w *WebHook) ServeHTTP(writer http.ResponseWriter, req *http.Request, ctx k
5151
}
5252

5353
revision, envvars, proceed, err := plugin.Extract(config, secret, "", req)
54-
switch err {
55-
case webhook.ErrSecretMismatch, webhook.ErrHookNotEnabled:
56-
return errors.NewUnauthorized(fmt.Sprintf("the webhook %q for %q did not accept your secret", hookType, name))
57-
case nil:
58-
default:
59-
return errors.NewInternalError(fmt.Errorf("hook failed: %v", err))
60-
}
61-
6254
if !proceed {
63-
return nil
55+
switch err {
56+
case webhook.ErrSecretMismatch, webhook.ErrHookNotEnabled:
57+
return errors.NewUnauthorized(fmt.Sprintf("the webhook %q for %q did not accept your secret", hookType, name))
58+
case nil:
59+
return nil
60+
default:
61+
return errors.NewInternalError(fmt.Errorf("hook failed: %v", err))
62+
}
6463
}
64+
warning := err
6565

6666
buildTriggerCauses := generateBuildTriggerInfo(revision, hookType, secret)
6767
request := &buildapi.BuildRequest{
@@ -73,7 +73,7 @@ func (w *WebHook) ServeHTTP(writer http.ResponseWriter, req *http.Request, ctx k
7373
if _, err := w.instantiator.Instantiate(config.Namespace, request); err != nil {
7474
return errors.NewInternalError(fmt.Errorf("could not generate a build: %v", err))
7575
}
76-
return nil
76+
return warning
7777
}
7878

7979
func generateBuildTriggerInfo(revision *buildapi.SourceRevision, hookType, secret string) (buildTriggerCauses []buildapi.BuildTriggerCause) {

‎pkg/build/registry/buildconfig/webhook_test.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -36,25 +36,27 @@ type plugin struct {
3636
Secret, Path string
3737
Err error
3838
Env []kapi.EnvVar
39+
Proceed bool
3940
}
4041

4142
func (p *plugin) Extract(buildCfg *api.BuildConfig, secret, path string, req *http.Request) (*api.SourceRevision, []kapi.EnvVar, bool, error) {
4243
p.Secret, p.Path = secret, path
43-
return nil, p.Env, true, p.Err
44+
return nil, p.Env, p.Proceed, p.Err
4445
}
4546

4647
func newStorage() (*rest.WebHook, *buildConfigInstantiator, *test.BuildConfigRegistry) {
4748
mockRegistry := &test.BuildConfigRegistry{}
4849
bci := &buildConfigInstantiator{}
4950
hook := NewWebHookREST(mockRegistry, bci, map[string]webhook.Plugin{
50-
"ok": &plugin{},
51+
"ok": &plugin{Proceed: true},
5152
"okenv": &plugin{
5253
Env: []kapi.EnvVar{
5354
{
5455
Name: "foo",
5556
Value: "bar",
5657
},
5758
},
59+
Proceed: true,
5860
},
5961
"errsecret": &plugin{Err: webhook.ErrSecretMismatch},
6062
"errhook": &plugin{Err: webhook.ErrHookNotEnabled},
@@ -239,7 +241,7 @@ func (p *pathPlugin) Extract(buildCfg *api.BuildConfig, secret, path string, req
239241
type errPlugin struct{}
240242

241243
func (*errPlugin) Extract(buildCfg *api.BuildConfig, secret, path string, req *http.Request) (*api.SourceRevision, []kapi.EnvVar, bool, error) {
242-
return nil, []kapi.EnvVar{}, true, errors.New("Plugin error!")
244+
return nil, []kapi.EnvVar{}, false, errors.New("Plugin error!")
243245
}
244246

245247
var testBuildConfig = &api.BuildConfig{

‎pkg/build/webhook/generic/generic.go

+54-48
Original file line numberDiff line numberDiff line change
@@ -42,68 +42,74 @@ func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string,
4242
return revision, envvars, false, err
4343
}
4444

45-
if buildCfg.Spec.Source.Git == nil {
46-
glog.V(4).Infof("No git source defined for BuildConfig %s/%s, but triggering anyway", buildCfg.Namespace, buildCfg.Name)
47-
return revision, envvars, true, err
48-
}
49-
5045
contentType := req.Header.Get("Content-Type")
5146
if len(contentType) != 0 {
5247
contentType, _, err = mime.ParseMediaType(contentType)
5348
if err != nil {
54-
return nil, envvars, false, fmt.Errorf("non-parseable Content-Type %s (%s)", contentType, err)
49+
return revision, envvars, false, fmt.Errorf("error parsing Content-Type: %s", err)
5550
}
5651
}
5752

58-
if req.Body != nil && (contentType == "application/json" || contentType == "application/yaml") {
59-
body, err := ioutil.ReadAll(req.Body)
60-
if err != nil {
61-
return nil, envvars, false, err
62-
}
53+
if req.Body == nil {
54+
return revision, envvars, true, nil
55+
}
6356

64-
if len(body) == 0 {
65-
return nil, envvars, true, nil
66-
}
57+
if contentType != "application/json" && contentType != "application/yaml" {
58+
warning := webhook.NewWarning("invalid Content-Type on payload, ignoring payload and continuing with build")
59+
return revision, envvars, true, warning
60+
}
6761

68-
var data api.GenericWebHookEvent
69-
if contentType == "application/yaml" {
70-
body, err = yaml.ToJSON(body)
71-
if err != nil {
72-
glog.V(4).Infof("Error converting payload to json %v, but continuing with build", err)
73-
return nil, envvars, true, nil
74-
}
75-
}
76-
if err = json.Unmarshal(body, &data); err != nil {
77-
glog.V(4).Infof("Error unmarshalling payload %v, but continuing with build", err)
78-
return nil, envvars, true, nil
79-
}
80-
if len(data.Env) > 0 && trigger.AllowEnv {
81-
envvars = data.Env
82-
}
83-
if data.Git == nil {
84-
glog.V(4).Infof("No git information for the generic webhook found in %s/%s", buildCfg.Namespace, buildCfg.Name)
85-
return nil, envvars, true, nil
62+
body, err := ioutil.ReadAll(req.Body)
63+
if err != nil {
64+
return revision, envvars, false, err
65+
}
66+
67+
if len(body) == 0 {
68+
return revision, envvars, true, nil
69+
}
70+
71+
var data api.GenericWebHookEvent
72+
if contentType == "application/yaml" {
73+
body, err = yaml.ToJSON(body)
74+
if err != nil {
75+
warning := webhook.NewWarning(fmt.Sprintf("error converting payload to json: %v, ignoring payload and continuing with build", err))
76+
return revision, envvars, true, warning
8677
}
78+
}
79+
if err = json.Unmarshal(body, &data); err != nil {
80+
warning := webhook.NewWarning(fmt.Sprintf("error unmarshalling payload: %v, ignoring payload and continuing with build", err))
81+
return revision, envvars, true, warning
82+
}
83+
if len(data.Env) > 0 && trigger.AllowEnv {
84+
envvars = data.Env
85+
}
86+
if buildCfg.Spec.Source.Git == nil {
87+
// everything below here is specific to git-based builds
88+
return revision, envvars, true, nil
89+
}
90+
if data.Git == nil {
91+
warning := webhook.NewWarning("no git information found in payload, ignoring and continuing with build")
92+
return revision, envvars, true, warning
93+
}
8794

88-
if data.Git.Refs != nil {
89-
for _, ref := range data.Git.Refs {
90-
if webhook.GitRefMatches(ref.Ref, webhook.DefaultConfigRef, &buildCfg.Spec.Source) {
91-
revision = &api.SourceRevision{
92-
Git: &ref.GitSourceRevision,
93-
}
94-
return revision, envvars, true, nil
95+
if data.Git.Refs != nil {
96+
for _, ref := range data.Git.Refs {
97+
if webhook.GitRefMatches(ref.Ref, webhook.DefaultConfigRef, &buildCfg.Spec.Source) {
98+
revision = &api.SourceRevision{
99+
Git: &ref.GitSourceRevision,
95100
}
101+
return revision, envvars, true, nil
96102
}
97-
glog.V(2).Infof("Skipping build for BuildConfig %s/%s. None of the supplied refs matched %q", buildCfg.Namespace, buildCfg, buildCfg.Spec.Source.Git.Ref)
98-
return nil, envvars, false, nil
99-
}
100-
if !webhook.GitRefMatches(data.Git.Ref, webhook.DefaultConfigRef, &buildCfg.Spec.Source) {
101-
glog.V(2).Infof("Skipping build for BuildConfig %s/%s. Branch reference from %q does not match configuration", buildCfg.Namespace, buildCfg.Name, data.Git.Ref)
102-
return nil, envvars, false, nil
103-
}
104-
revision = &api.SourceRevision{
105-
Git: &data.Git.GitSourceRevision,
106103
}
104+
warning := webhook.NewWarning(fmt.Sprintf("skipping build. None of the supplied refs matched %q", buildCfg.Spec.Source.Git.Ref))
105+
return revision, envvars, false, warning
106+
}
107+
if !webhook.GitRefMatches(data.Git.Ref, webhook.DefaultConfigRef, &buildCfg.Spec.Source) {
108+
warning := webhook.NewWarning(fmt.Sprintf("skipping build. Branch reference from %q does not match configuration", data.Git.Ref))
109+
return revision, envvars, false, warning
110+
}
111+
revision = &api.SourceRevision{
112+
Git: &data.Git.GitSourceRevision,
107113
}
108114
return revision, envvars, true, nil
109115
}

‎pkg/build/webhook/generic/generic_test.go

+127-11
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import (
1111
"github.com/openshift/origin/pkg/build/api"
1212
"github.com/openshift/origin/pkg/build/webhook"
1313
kapi "k8s.io/kubernetes/pkg/api"
14+
"k8s.io/kubernetes/pkg/api/errors"
15+
"k8s.io/kubernetes/pkg/api/unversioned"
1416
)
1517

1618
var mockBuildStrategy = api.BuildStrategy{
@@ -52,6 +54,24 @@ func GivenRequestWithRefsPayload(t *testing.T) *http.Request {
5254
return req
5355
}
5456

57+
func matchWarning(t *testing.T, err error, message string) {
58+
status, ok := err.(*errors.StatusError)
59+
if !ok {
60+
t.Errorf("Expected %v to be a StatusError object", err)
61+
return
62+
}
63+
64+
if status.ErrStatus.Status != unversioned.StatusSuccess {
65+
t.Errorf("Unexpected response status %v, expected %v", status.ErrStatus.Status, unversioned.StatusSuccess)
66+
}
67+
if status.ErrStatus.Code != http.StatusOK {
68+
t.Errorf("Unexpected response code %v, expected %v", status.ErrStatus.Code, http.StatusOK)
69+
}
70+
if status.ErrStatus.Message != message {
71+
t.Errorf("Unexpected response message %v, expected %v", status.ErrStatus.Message, message)
72+
}
73+
}
74+
5575
func TestVerifyRequestForMethod(t *testing.T) {
5676
req := GivenRequest("GET")
5777
buildConfig := &api.BuildConfig{
@@ -327,9 +347,7 @@ func TestExtractWithUnmatchedRefGitPayload(t *testing.T) {
327347
plugin := New()
328348
build, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
329349

330-
if err != nil {
331-
t.Errorf("Unexpected error when triggering build: %v", err)
332-
}
350+
matchWarning(t, err, `skipping build. Branch reference from "refs/heads/master" does not match configuration`)
333351
if proceed {
334352
t.Error("Expected 'proceed' return value to be 'false' for unmatched refs")
335353
}
@@ -471,9 +489,7 @@ func TestExtractWithUnmatchedGitRefsPayload(t *testing.T) {
471489
plugin := New()
472490
revision, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
473491

474-
if err != nil {
475-
t.Errorf("Expected to be able to trigger a build without a payload error: %v", err)
476-
}
492+
matchWarning(t, err, `skipping build. None of the supplied refs matched "other"`)
477493
if proceed {
478494
t.Error("Expected 'proceed' return value to be 'false'")
479495
}
@@ -627,9 +643,7 @@ func TestGitlabPush(t *testing.T) {
627643
plugin := New()
628644
_, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
629645

630-
if err != nil {
631-
t.Errorf("Expected to be able to trigger a build without a payload error: %v", err)
632-
}
646+
matchWarning(t, err, "no git information found in payload, ignoring and continuing with build")
633647
if !proceed {
634648
t.Error("Expected 'proceed' return value to be 'true'")
635649
}
@@ -699,13 +713,115 @@ func TestExtractWithUnmarshalError(t *testing.T) {
699713
}
700714
plugin := New()
701715
revision, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
702-
if err != nil {
703-
t.Errorf("Expected to be able to trigger a build without a payload error: %v", err)
716+
matchWarning(t, err, `error unmarshalling payload: invalid character '\x00' looking for beginning of value, ignoring payload and continuing with build`)
717+
if !proceed {
718+
t.Error("Expected 'proceed' return value to be 'true'")
719+
}
720+
if revision != nil {
721+
t.Error("Expected the 'revision' return value to be nil")
722+
}
723+
}
724+
725+
func TestExtractWithUnmarshalErrorYAML(t *testing.T) {
726+
req, _ := http.NewRequest("POST", "http://someurl.com", errJSON{})
727+
req.Header.Add("Content-Type", "application/yaml")
728+
buildConfig := &api.BuildConfig{
729+
Spec: api.BuildConfigSpec{
730+
Triggers: []api.BuildTriggerPolicy{
731+
{
732+
Type: api.GenericWebHookBuildTriggerType,
733+
GenericWebHook: &api.WebHookTrigger{
734+
Secret: "secret100",
735+
},
736+
},
737+
},
738+
CommonSpec: api.CommonSpec{
739+
Source: api.BuildSource{
740+
Git: &api.GitBuildSource{
741+
Ref: "other",
742+
},
743+
},
744+
Strategy: mockBuildStrategy,
745+
},
746+
},
747+
}
748+
plugin := New()
749+
revision, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
750+
matchWarning(t, err, "error converting payload to json: yaml: control characters are not allowed, ignoring payload and continuing with build")
751+
if !proceed {
752+
t.Error("Expected 'proceed' return value to be 'true'")
753+
}
754+
if revision != nil {
755+
t.Error("Expected the 'revision' return value to be nil")
756+
}
757+
}
758+
759+
func TestExtractWithBadContentType(t *testing.T) {
760+
req, _ := http.NewRequest("POST", "http://someurl.com", errJSON{})
761+
req.Header.Add("Content-Type", "bad")
762+
buildConfig := &api.BuildConfig{
763+
Spec: api.BuildConfigSpec{
764+
Triggers: []api.BuildTriggerPolicy{
765+
{
766+
Type: api.GenericWebHookBuildTriggerType,
767+
GenericWebHook: &api.WebHookTrigger{
768+
Secret: "secret100",
769+
},
770+
},
771+
},
772+
CommonSpec: api.CommonSpec{
773+
Source: api.BuildSource{
774+
Git: &api.GitBuildSource{
775+
Ref: "other",
776+
},
777+
},
778+
Strategy: mockBuildStrategy,
779+
},
780+
},
704781
}
782+
plugin := New()
783+
revision, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
784+
matchWarning(t, err, "invalid Content-Type on payload, ignoring payload and continuing with build")
705785
if !proceed {
706786
t.Error("Expected 'proceed' return value to be 'true'")
707787
}
708788
if revision != nil {
709789
t.Error("Expected the 'revision' return value to be nil")
710790
}
711791
}
792+
793+
func TestExtractWithUnparseableContentType(t *testing.T) {
794+
req, _ := http.NewRequest("POST", "http://someurl.com", errJSON{})
795+
req.Header.Add("Content-Type", "bad//bad")
796+
buildConfig := &api.BuildConfig{
797+
Spec: api.BuildConfigSpec{
798+
Triggers: []api.BuildTriggerPolicy{
799+
{
800+
Type: api.GenericWebHookBuildTriggerType,
801+
GenericWebHook: &api.WebHookTrigger{
802+
Secret: "secret100",
803+
},
804+
},
805+
},
806+
CommonSpec: api.CommonSpec{
807+
Source: api.BuildSource{
808+
Git: &api.GitBuildSource{
809+
Ref: "other",
810+
},
811+
},
812+
Strategy: mockBuildStrategy,
813+
},
814+
},
815+
}
816+
plugin := New()
817+
revision, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
818+
if err == nil || err.Error() != "error parsing Content-Type: mime: expected token after slash" {
819+
t.Errorf("Unexpected error %v", err)
820+
}
821+
if proceed {
822+
t.Error("Expected 'proceed' return value to be 'false'")
823+
}
824+
if revision != nil {
825+
t.Error("Expected the 'revision' return value to be nil")
826+
}
827+
}

0 commit comments

Comments
 (0)