Skip to content

Commit 93e067e

Browse files
author
PI-Victor
committed
refactor webhooks to account for multiple sources
*fix oc describe to show multiple webhooks when defined and adapt to the new allowenv variable for generic webhooks *refactor and abstract logic for validating secrets and defined webhooks *fix a bug where the webhook panics whenever the buildconfig source isn't a git repository *adapt current tests to account for multiple webhooks. Signed-off-by: PI-Victor <[email protected]>
1 parent d6879a6 commit 93e067e

File tree

12 files changed

+544
-106
lines changed

12 files changed

+544
-106
lines changed

‎pkg/build/webhook/controller.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,11 @@ func (c *controller) ServeHTTP(w http.ResponseWriter, req *http.Request) {
9191
}
9292
}
9393

94-
// parseURL retrieves the namespace from the query parameters and returns a context wrapping the namespace,
95-
// the parameters for the webhook call, and an error.
96-
// according to the docs (http://godoc.org/code.google.com/p/go.net/context) ctx is not supposed to be wrapped in another object
94+
// parseURL retrieves the namespace from the query parameters and returns a
95+
// context wrapping the namespace, the parameters for the webhook call, and an
96+
// error. according to the docs
97+
// (http://godoc.org/code.google.com/p/go.net/context) ctx is not supposed to
98+
// be wrapped in another object
9799
func parseURL(req *http.Request) (uv urlVars, err error) {
98100
url := req.URL.Path
99101

‎pkg/build/webhook/generic/fixtures/push-github-envs.json renamed to ‎pkg/build/webhook/generic/fixtures/push-generic-envs.json

File renamed without changes.

‎pkg/build/webhook/generic/fixtures/push-github.json renamed to ‎pkg/build/webhook/generic/fixtures/push-generic.json

File renamed without changes.

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

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package generic
22

33
import (
4-
"crypto/hmac"
54
"encoding/json"
65
"fmt"
76
"io/ioutil"
@@ -26,25 +25,25 @@ func New() *WebHookPlugin {
2625

2726
// Extract services generic webhooks.
2827
func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string, req *http.Request) (revision *api.SourceRevision, envvars []kapi.EnvVar, proceed bool, err error) {
29-
trigger, ok := webhook.FindTriggerPolicy(api.GenericWebHookBuildTriggerType, buildCfg)
30-
if !ok {
31-
err = webhook.ErrHookNotEnabled
32-
return
28+
triggers, err := webhook.FindTriggerPolicy(api.GenericWebHookBuildTriggerType, buildCfg)
29+
if err != nil {
30+
return revision, envvars, false, err
3331
}
3432
glog.V(4).Infof("Checking if the provided secret for BuildConfig %s/%s matches", buildCfg.Namespace, buildCfg.Name)
35-
if !hmac.Equal([]byte(trigger.GenericWebHook.Secret), []byte(secret)) {
36-
err = webhook.ErrSecretMismatch
37-
return
33+
34+
trigger, err := webhook.ValidateWebHookSecret(triggers, secret)
35+
if err != nil {
36+
return revision, envvars, false, err
3837
}
38+
3939
glog.V(4).Infof("Verifying build request for BuildConfig %s/%s", buildCfg.Namespace, buildCfg.Name)
4040
if err = verifyRequest(req); err != nil {
41-
return
41+
return revision, envvars, false, err
4242
}
4343

44-
git := buildCfg.Spec.Source.Git
45-
if git == nil {
46-
glog.V(4).Infof("No source defined for BuildConfig %s/%s, but triggering anyway", buildCfg.Namespace, buildCfg.Name)
47-
return nil, envvars, true, nil
44+
if buildCfg.Spec.Source.Git == nil {
45+
glog.V(4).Infof("No git source defined for BuildConfig %s/%s, but triggering anyway", buildCfg.Namespace, buildCfg.Name)
46+
return revision, envvars, true, err
4847
}
4948

5049
contentType := req.Header.Get("Content-Type")
@@ -60,15 +59,17 @@ func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string,
6059
if err != nil {
6160
return nil, envvars, false, err
6261
}
62+
6363
if len(body) == 0 {
6464
return nil, envvars, true, nil
6565
}
66+
6667
var data api.GenericWebHookEvent
6768
if err = json.Unmarshal(body, &data); err != nil {
6869
glog.V(4).Infof("Error unmarshaling json %v, but continuing", err)
6970
return nil, envvars, true, nil
7071
}
71-
if len(data.Env) > 0 && trigger.GenericWebHook.AllowEnv {
72+
if len(data.Env) > 0 && trigger.AllowEnv {
7273
envvars = data.Env
7374
}
7475
if data.Git == nil {
@@ -78,17 +79,17 @@ func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string,
7879

7980
if data.Git.Refs != nil {
8081
for _, ref := range data.Git.Refs {
81-
if webhook.GitRefMatches(ref.Ref, git.Ref) {
82+
if webhook.GitRefMatches(ref.Ref, webhook.DefaultConfigRef, &buildCfg.Spec.Source) {
8283
revision = &api.SourceRevision{
8384
Git: &ref.GitSourceRevision,
8485
}
8586
return revision, envvars, true, nil
8687
}
8788
}
88-
glog.V(2).Infof("Skipping build for BuildConfig %s/%s. None of the supplied refs matched %q", buildCfg.Namespace, buildCfg, git.Ref)
89+
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)
8990
return nil, envvars, false, nil
9091
}
91-
if !webhook.GitRefMatches(data.Git.Ref, git.Ref) {
92+
if !webhook.GitRefMatches(data.Git.Ref, webhook.DefaultConfigRef, &buildCfg.Spec.Source) {
9293
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)
9394
return nil, envvars, false, nil
9495
}

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

Lines changed: 158 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func TestVerifyRequestForMethod(t *testing.T) {
7070
revision, _, proceed, err := plugin.Extract(buildConfig, "secret100", "", req)
7171

7272
if err == nil || !strings.Contains(err.Error(), "Unsupported HTTP method") {
73-
t.Errorf("Excepcted unsupported HTTP method, got %v!", err)
73+
t.Errorf("Expected unsupported HTTP method, got %v!", err)
7474
}
7575
if proceed {
7676
t.Error("Expected 'proceed' return value to be 'false'")
@@ -80,6 +80,157 @@ func TestVerifyRequestForMethod(t *testing.T) {
8080
}
8181
}
8282

83+
func TestWrongSecretMultipleGenericWebHooks(t *testing.T) {
84+
req := GivenRequest("GET")
85+
buildConfig := &api.BuildConfig{
86+
Spec: api.BuildConfigSpec{
87+
Triggers: []api.BuildTriggerPolicy{
88+
{
89+
Type: api.GenericWebHookBuildTriggerType,
90+
GenericWebHook: &api.WebHookTrigger{
91+
Secret: "secret101",
92+
},
93+
},
94+
{
95+
Type: api.GenericWebHookBuildTriggerType,
96+
GenericWebHook: &api.WebHookTrigger{
97+
Secret: "secret100",
98+
},
99+
},
100+
{
101+
Type: api.GenericWebHookBuildTriggerType,
102+
GenericWebHook: &api.WebHookTrigger{
103+
Secret: "secret102",
104+
},
105+
},
106+
},
107+
},
108+
}
109+
110+
plugin := New()
111+
revision, _, proceed, err := plugin.Extract(buildConfig, "wrongsecret", "", req)
112+
113+
if err != webhook.ErrSecretMismatch {
114+
t.Errorf("Expected %s, got %s", webhook.ErrSecretMismatch, err)
115+
}
116+
117+
if proceed {
118+
t.Error("Expected 'proceed' to return 'false'")
119+
}
120+
121+
if revision != nil {
122+
t.Errorf("Expected the 'revision' to be nil, go %v instead", revision)
123+
}
124+
}
125+
126+
func TestMatchSecretMultipleGenericWebHooks(t *testing.T) {
127+
req := GivenRequestWithPayload(t, "push-generic.json")
128+
buildConfig := &api.BuildConfig{
129+
Spec: api.BuildConfigSpec{
130+
Triggers: []api.BuildTriggerPolicy{
131+
{
132+
Type: api.GenericWebHookBuildTriggerType,
133+
GenericWebHook: &api.WebHookTrigger{
134+
Secret: "secret101",
135+
},
136+
},
137+
{
138+
Type: api.GenericWebHookBuildTriggerType,
139+
GenericWebHook: &api.WebHookTrigger{
140+
Secret: "secret100",
141+
},
142+
},
143+
{
144+
Type: api.GenericWebHookBuildTriggerType,
145+
GenericWebHook: &api.WebHookTrigger{
146+
Secret: "secret102",
147+
},
148+
},
149+
},
150+
BuildSpec: api.BuildSpec{
151+
Source: api.BuildSource{
152+
Git: &api.GitBuildSource{
153+
Ref: "master",
154+
},
155+
},
156+
Strategy: mockBuildStrategy,
157+
},
158+
},
159+
}
160+
161+
plugin := New()
162+
revision, _, proceed, err := plugin.Extract(buildConfig, "secret102", "", req)
163+
164+
if err != nil {
165+
t.Errorf("Expected no error, got %s", err)
166+
}
167+
168+
if !proceed {
169+
t.Error("Expected 'proceed' to return 'true', got 'false' instead")
170+
}
171+
172+
if revision == nil {
173+
t.Errorf("Expected the 'revision' to not be nil")
174+
}
175+
}
176+
177+
func TestEnvVarsMultipleGenericWebHooks(t *testing.T) {
178+
req := GivenRequestWithPayload(t, "push-generic-envs.json")
179+
buildConfig := &api.BuildConfig{
180+
Spec: api.BuildConfigSpec{
181+
Triggers: []api.BuildTriggerPolicy{
182+
{
183+
Type: api.GenericWebHookBuildTriggerType,
184+
GenericWebHook: &api.WebHookTrigger{
185+
Secret: "secret101",
186+
AllowEnv: true,
187+
},
188+
},
189+
{
190+
Type: api.GenericWebHookBuildTriggerType,
191+
GenericWebHook: &api.WebHookTrigger{
192+
Secret: "secret100",
193+
},
194+
},
195+
{
196+
Type: api.GenericWebHookBuildTriggerType,
197+
GenericWebHook: &api.WebHookTrigger{
198+
Secret: "secret102",
199+
},
200+
},
201+
},
202+
BuildSpec: api.BuildSpec{
203+
Source: api.BuildSource{
204+
Git: &api.GitBuildSource{
205+
Ref: "master",
206+
},
207+
},
208+
Strategy: mockBuildStrategy,
209+
},
210+
},
211+
}
212+
213+
plugin := New()
214+
revision, envvars, proceed, err := plugin.Extract(buildConfig, "secret101", "", req)
215+
216+
if err != nil {
217+
t.Errorf("Expected to be able to trigger a build without a payload error: %v", err)
218+
}
219+
220+
if !proceed {
221+
t.Error("Expected 'proceed' to return 'true'")
222+
}
223+
224+
if revision == nil {
225+
t.Errorf("Expected the 'revision' to not be nil")
226+
}
227+
228+
if len(envvars) == 0 {
229+
t.Error("Expected env vars to be set")
230+
}
231+
232+
}
233+
83234
func TestWrongSecret(t *testing.T) {
84235
req := GivenRequest("POST")
85236
buildConfig := &api.BuildConfig{
@@ -98,7 +249,7 @@ func TestWrongSecret(t *testing.T) {
98249
revision, _, proceed, err := plugin.Extract(buildConfig, "wrongsecret", "", req)
99250

100251
if err != webhook.ErrSecretMismatch {
101-
t.Errorf("Excepcted %v, got %v!", webhook.ErrSecretMismatch, err)
252+
t.Errorf("Expected %v, got %v!", webhook.ErrSecretMismatch, err)
102253
}
103254
if proceed {
104255
t.Error("Expected 'proceed' return value to be 'false'")
@@ -152,7 +303,7 @@ func TestExtractWithEmptyPayload(t *testing.T) {
152303
}
153304

154305
func TestExtractWithUnmatchedRefGitPayload(t *testing.T) {
155-
req := GivenRequestWithPayload(t, "push-github.json")
306+
req := GivenRequestWithPayload(t, "push-generic.json")
156307
buildConfig := &api.BuildConfig{
157308
Spec: api.BuildConfigSpec{
158309
Triggers: []api.BuildTriggerPolicy{
@@ -188,7 +339,7 @@ func TestExtractWithUnmatchedRefGitPayload(t *testing.T) {
188339
}
189340

190341
func TestExtractWithGitPayload(t *testing.T) {
191-
req := GivenRequestWithPayload(t, "push-github.json")
342+
req := GivenRequestWithPayload(t, "push-generic.json")
192343
buildConfig := &api.BuildConfig{
193344
Spec: api.BuildConfigSpec{
194345
Triggers: []api.BuildTriggerPolicy{
@@ -224,7 +375,7 @@ func TestExtractWithGitPayload(t *testing.T) {
224375
}
225376

226377
func TestExtractWithGitPayloadAndUTF8Charset(t *testing.T) {
227-
req := GivenRequestWithPayloadAndContentType(t, "push-github.json", "application/json; charset=utf-8")
378+
req := GivenRequestWithPayloadAndContentType(t, "push-generic.json", "application/json; charset=utf-8")
228379
buildConfig := &api.BuildConfig{
229380
Spec: api.BuildConfigSpec{
230381
Triggers: []api.BuildTriggerPolicy{
@@ -332,7 +483,7 @@ func TestExtractWithUnmatchedGitRefsPayload(t *testing.T) {
332483
}
333484

334485
func TestExtractWithKeyValuePairs(t *testing.T) {
335-
req := GivenRequestWithPayload(t, "push-github-envs.json")
486+
req := GivenRequestWithPayload(t, "push-generic-envs.json")
336487
buildConfig := &api.BuildConfig{
337488
Spec: api.BuildConfigSpec{
338489
Triggers: []api.BuildTriggerPolicy{
@@ -373,7 +524,7 @@ func TestExtractWithKeyValuePairs(t *testing.T) {
373524
}
374525

375526
func TestExtractWithKeyValuePairsDisabled(t *testing.T) {
376-
req := GivenRequestWithPayload(t, "push-github-envs.json")
527+
req := GivenRequestWithPayload(t, "push-generic-envs.json")
377528
buildConfig := &api.BuildConfig{
378529
Spec: api.BuildConfigSpec{
379530
Triggers: []api.BuildTriggerPolicy{

0 commit comments

Comments
 (0)