enable webhooks with multiple sources #8580

PI-Victor · 2016-04-20T21:48:56Z

fixes: #8551
fixes: #8619

*fix oc describe to show multiple webhooks when defined
*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

Dunno yet what tests fail, so it's still work in progress. Also need to write new tests.

oc describe output now:

single source for each type

[vagrant@origindevel ~]$ oc describe bc ruby-sample-build
Name:           ruby-sample-build
Created:        34 hours ago
Labels:         app=ruby-sample-build
                name=ruby-sample-build
                template=application-template-dockerbuild
Annotations:    openshift.io/generated-by=OpenShiftNewApp
Latest Version: 13

Strategy:               Docker
URL:                    git://github.com/openshift/ruby-hello-world.git
From Image:             ImageStreamTag ruby-22-centos7:latest
Output to:              ImageStreamTag origin-ruby-sample:latest
Post Commit Hook:       ["<image-entrypoint>", "bundle", "exec", "rake", "test"]
Triggered by:           ImageChange, Config
Webhook GitHub: 
        https://10.0.2.15:8443/oapi/v1/namespaces/demo/buildconfigs/ruby-sample-build/webhooks/secret101/github
Webhook Generic: 
        https://10.0.2.15:8443/oapi/v1/namespaces/demo/buildconfigs/ruby-sample-build/webhooks/secret101/generic

Build                   Status          Duration        Creation Time
ruby-sample-build-13    complete        1m34s           2016-04-20 20:49:42 +0000 UTC
ruby-sample-build-12    complete        1m20s           2016-04-19 13:28:37 +0000 UTC
ruby-sample-build-11    failed          2s              2016-04-19 13:26:02 +0000 UTC
ruby-sample-build-10    complete        1m17s           2016-04-19 13:11:13 +0000 UTC
ruby-sample-build-9     failed          2s              2016-04-19 12:17:51 +0000 UTC
ruby-sample-build-8     complete        1m23s           2016-04-19 12:04:35 +0000 UTC
ruby-sample-build-7     complete        1m21s           2016-04-19 12:01:36 +0000 UTC
ruby-sample-build-6     complete        1m17s           2016-04-19 11:56:26 +0000 UTC
ruby-sample-build-5     complete        1m20s           2016-04-19 11:40:38 +0000 UTC
ruby-sample-build-4     failed          1s              2016-04-19 11:39:30 +0000 UTC

multiple sources for one type

[vagrant@origindevel ~]$ oc project demo
Now using project "demo" on server "https://10.0.2.15:8443".
[vagrant@origindevel ~]$ oc describe bc centos
Name:           centos
Created:        34 hours ago
Labels:         build=centos
Annotations:    openshift.io/generated-by=OpenShiftNewBuild
Latest Version: 6

Strategy:       Docker
Dockerfile:
  FROM centos:7
  RUN yum install -y httpd
From Image:     ImageStreamTag centos:7
Output to:      ImageStreamTag centos:latest
Triggered by:   Config, ImageChange
Webhook GitHub: 
        https://10.0.2.15:8443/oapi/v1/namespaces/demo/buildconfigs/centos/webhooks/t8qqTg2eQg1SdWVS0eG3/github
        https://10.0.2.15:8443/oapi/v1/namespaces/demo/buildconfigs/centos/webhooks/t8qqTg2eQg1SdWVS0eG2/github
        https://10.0.2.15:8443/oapi/v1/namespaces/demo/buildconfigs/centos/webhooks/t8qqTg2eQg1SdWVS0eG1/github
Webhook Generic: 
        https://10.0.2.15:8443/oapi/v1/namespaces/demo/buildconfigs/centos/webhooks/pu7tBf5A-c_74KVKfP3g/generic
        https://10.0.2.15:8443/oapi/v1/namespaces/demo/buildconfigs/centos/webhooks/test1/generic
        https://10.0.2.15:8443/oapi/v1/namespaces/demo/buildconfigs/centos/webhooks/test2/generic

Build           Status          Duration        Creation Time
centos-6        complete        3m27s           2016-04-20 20:53:18 +0000 UTC
centos-5        complete        43s             2016-04-20 16:58:22 +0000 UTC
centos-4        complete        50s             2016-04-20 16:08:47 +0000 UTC
centos-3        complete        43s             2016-04-20 16:08:13 +0000 UTC
centos-2        complete        17s             2016-04-19 11:24:17 +0000 UTC
centos-1        complete        28s             2016-04-19 11:17:10 +0000 UTC

Signed-off-by: PI-Victor [email protected]

bparees · 2016-04-20T22:01:57Z

lgtm but needs test cases

PI-Victor · 2016-04-21T12:49:07Z

[test]

PI-Victor · 2016-04-26T13:44:31Z

@mfojtik @bparees PTAL. still pending test cases green, but last time they flaked.
i've adapted test cases to account for multiple webhooks.
one thing to mention is generic webhooks don't have tests like github webhooks do here: https://github.com/PI-Victor/origin/blob/pi-victor/fix_multiple_webhook_triggers/test/integration/webhookgithub_test.go
should they be added?
this will also fix #8619

PI-Victor · 2016-04-26T14:24:45Z

just noticed, tests are still failing. will fix.

bparees · 2016-04-26T22:22:39Z

pkg/build/webhook/generic/generic.go

+		// REVIEW: a real world scenario is that we don't use the same request to
+		// verify a secret. if that would be the case, then the req.body is already
+		// consumed for that request after the first try and we return proceed=true
+		// here even when the git ref doesn't match (which shouldn't happen).


sorry i'm not following the scenario you're trying to describe

i was ambiguous in this note. i thought we might reuse a request somewhere else, but i don't see a real case scenario. you have a webhook request, you trigger a build or fail if it doesn't match the ref/secret. it's that simple. i was over thinking it

PI-Victor · 2016-04-27T11:43:32Z

i fixed the tests, finally. wondering if this works [testonlyintegration]

PI-Victor · 2016-04-28T08:44:35Z

[test]

PI-Victor · 2016-04-28T11:36:09Z

well, tests fail because of #7429

PI-Victor · 2016-04-28T17:07:26Z

[test]

PI-Victor · 2016-04-28T18:36:08Z

@bparees finally it's green, can you take a look whenever you have time

bparees · 2016-04-28T21:26:07Z

pkg/build/webhook/generic/generic.go

 			}
-			glog.V(2).Infof("Skipping build for BuildConfig %s/%s. None of the supplied refs matched %q", buildCfg.Namespace, buildCfg, git.Ref)
+			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)
 			return nil, false, nil


this return needs to be outside the for loop. you're going to check the first ref and if it doesn't match, return false.

which also means we need better tests.

also since you removed the check for nil, you're always going to return false without checking the data.Git.Ref field, so you need to put that nil check back in.

which again means we need better tests.

gotcha, thanks!

PI-Victor · 2016-04-28T22:46:12Z

@bparees btw since now there could be possible multiple webhooks do we need to ping the UI team and tell them do add them to the webconsole? right now i think only one type of source shows up there

bparees · 2016-04-28T22:59:58Z

@bparees btw since now there could be possible multiple webhooks do we need to ping the UI team and tell them do add them to the webconsole? right now i think only one type of source shows up there

I think @jhadvig already knows.

jhadvig · 2016-05-02T07:47:36Z

@PI-Victor @bparees thanks for heads up. Yeap this was already taken care in as part of jwforres#55
In the BC browse page you will seemultiple webhook triggers, but we still need to solve adding/removing webhooks in the BC editor, since now it would be handy to add/remove those you want.
Will open issue for that so we can track it. Thanks @PI-Victor 👍

PI-Victor · 2016-05-02T09:05:53Z

this was just pending some tests but now i need to refactor this, since, after rebase, this #8477 broke everything.

PI-Victor · 2016-05-02T22:16:08Z

@bparees PTAL. i've eliminated URL tag from the oc describe bc because there are no other tags on the right side of the description.

Formatting not that great on github, but they're actually aligned, if you feel strong about keeping the URL tag, i'll add it back.

Name:       django-psql-example
Created:    3 days ago
Labels:     app=django-psql-example
        template=django-psql-example
Description:    Defines how to build the application
Annotations:    openshift.io/generated-by=OpenShiftNewApp
Latest Version: 2

Strategy:       Source
URL:            https://github.com/pi-victor/django-ex.git
Ref:            pi-victor/upgrade_django_vers
From Image:     ImageStreamTag openshift/python:3.4
Output to:      ImageStreamTag django-psql-example:latest
Post Commit Hook:   ["/bin/sh", "-ic", "./manage.py test"]

Build Run Policy:   Serial
Triggered by:       ImageChange, Config
Webhook GitHub:     https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm82/github
            https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm83/github
            https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm81/github
Webhook Generic:    https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUV55/generic
            AllowEnv:   false
            https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUV55/generic
            AllowEnv:   false
            https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUV55/generic
            AllowEnv:   false

Build           Status      Duration    Creation Time
django-psql-example-2   complete    1m3s        2016-05-02 15:06:32 +0200 CEST
django-psql-example-1   complete    2m27s       2016-04-29 16:28:41 +0200 CEST

bparees · 2016-05-02T22:57:26Z

it looks strange having the first url on the same line as the label, and then the rest indented. I think we should just start a new line after the label, eg:

Webhook GitHub:     
            https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm82/github
            https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm83/github
            https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm81/github

as for URL label removal, we have it for AllowEnv in your example, so i think we should keep it, though it's not totally necessary..but it looks weird not to have it when we have AllowEnv.

PI-Victor · 2016-05-03T10:22:21Z

fair point

Build Run Policy:   Serial
Triggered by:       ImageChange, Config
Webhook GitHub:
    URL:    https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm82/github
    URL:    https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm83/github
    URL:    https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUVm81/github
Webhook Generic:
    URL:        https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUV55/generic
    AllowEnv:   false
    URL:        https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUV55/generic
    AllowEnv:   false
    URL:        https://localhost:8443/oapi/v1/namespaces/demo/buildconfigs/django-psql-example/webhooks/pTXDwQY8R0AKJ2vqkuQRJMUdsJAqCjWdeYtUV55/generic
    AllowEnv:   false

PI-Victor · 2016-05-03T11:25:25Z

@bparees done. also i figured out in the end there were test cases for different refs so i didn't add anything, i just adapted everything to account for multiple webhooks, i'm not sure what else to do at this point and i hate to say it but maybe it's the way that the whole plugin system for these webhooks is written, that is a problem rather than the tests, what do you think?

bparees · 2016-05-03T13:16:38Z

pkg/build/webhook/generic/generic_test.go

+		req := GivenRequest("GET")
+		revision, _, proceed, err := plugin.Extract(buildConfig, secret, "", req)
+		if err == nil || !strings.Contains(err.Error(), "Unsupported HTTP method") {
+			t.Errorf("Excepcted unsupported HTTP method, got %v!", err)


ah i fixed this but lost it during rebase :D

bparees · 2016-05-03T13:22:55Z

@openshift/api-review needs review of new describe output for multiple webhooks.

@PI-Victor one typo, otherwise lgtm. Regarding what you did w/ the tests: it's ok, but next time around i'd just create a single new test or two that confirms:

if you provide a bad secret against multiple webhooks, none of them match it.
if you provide a valid secret against multiple webhooks (a) it does match and (b) it matches the right one.

Adding the loop to all the other TCs is kinda overkill.

deads2k · 2016-05-03T13:30:04Z

@openshift/api-review needs review of new describe output for multiple webhooks.

oc describe output isn't guaranteed to be stable, so it doesn't matter.

bparees · 2016-05-03T13:49:37Z

@deads2k yeah the concern is not compatibility, but describe output seems to fall under the api review team's area of interest/approval.

PI-Victor · 2016-05-03T14:16:48Z

@bparees thanks for the tip. i suck at writing tests, that's for sure. i can actually do just what you suggested instead of introducing bogus stuff in testing, plus it's not hard to do and also would not take long. i'd like to do it properly

bparees · 2016-05-03T14:24:48Z

@PI-Victor knock yourself out, it'll make my rebase for refactoring the webhooks a little easier too.

smarterclayton · 2016-05-03T17:05:16Z

pkg/build/webhook/webhook.go

-	if configRef == "" {
-		configRef = "master"
+func GitRefMatches(eventRef string, buildSource *api.BuildSource) bool {
+	configRef := "master"


hoist this out to a constant and require it to be passed in - this method shouldn't infer "master"

PI-Victor · 2016-05-04T15:47:16Z

@bparees ping! i've reviewed the tests cases and also added tests for the webhook.go "controller" hopefully i reduced your work with that. *fingers crossed

bparees · 2016-05-04T22:04:01Z

pkg/build/webhook/webhook_test.go

+
+	if triggers == nil {
+		t.Error("Expected a slice of matched 'triggers', got nil")
+	}


should check the # of matches.

bparees · 2016-05-04T22:05:53Z

@PI-Victor couple TC nits and then lgtm.

PI-Victor · 2016-05-05T15:39:25Z

@bparees btw, what you suggested above was fixed. can we merge?

bparees · 2016-05-05T20:41:30Z

pkg/build/webhook/webhook_test.go

+		t.Error("Expected a slice of matched 'triggers', got nil")
+	}
+
+	if len(triggers) < 3 {


why not check the exact size?

bparees · 2016-05-05T20:41:55Z

@PI-Victor i'm merging this so we can move on, but i'm not sure why you didn't check for an exact size in your tests.

PI-Victor · 2016-05-05T20:57:36Z

i did actually :/, but it went in with some other changes in the git stash. should i fix it now, or are you going to in you PR?

bparees · 2016-05-05T21:01:04Z

@PI-Victor go ahead and fix it.

bparees · 2016-05-05T21:01:28Z

(merge tag removed)

*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]>

openshift-bot · 2016-05-05T21:05:30Z

Evaluated for origin test up to 93e067e

openshift-bot · 2016-05-05T22:01:39Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3631/)

bparees · 2016-05-05T22:21:12Z

thanks @PI-Victor! [merge]

openshift-bot · 2016-05-05T22:25:36Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5829/) (Image: devenv-rhel7_4120)

openshift-bot · 2016-05-05T22:25:37Z

Evaluated for origin merge up to 93e067e

PI-Victor mentioned this pull request Apr 25, 2016

GitHub webhook panics when source is different than git #8619

Closed

bparees reviewed Apr 26, 2016
View reviewed changes

PI-Victor changed the title ~~[WIP] enable webhooks with multiple sources~~ enable webhooks with multiple sources Apr 28, 2016

bparees reviewed Apr 28, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2016

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2016

PI-Victor mentioned this pull request May 2, 2016

Allow parameters on generic webhook build trigger call #8477

Merged

bparees reviewed May 3, 2016
View reviewed changes

bparees added the needs-api-review label May 3, 2016

smarterclayton reviewed May 3, 2016
View reviewed changes

smarterclayton added api-approved and removed needs-api-review labels May 3, 2016

bparees reviewed May 4, 2016
View reviewed changes

bparees reviewed May 5, 2016
View reviewed changes

openshift-bot merged commit 9bb252d into openshift:master May 6, 2016

PI-Victor deleted the pi-victor/fix_multiple_webhook_triggers branch May 6, 2016 16:07

PI-Victor restored the pi-victor/fix_multiple_webhook_triggers branch May 9, 2016 15:12

enable webhooks with multiple sources #8580

enable webhooks with multiple sources #8580

Conversation

PI-Victor commented Apr 20, 2016 • edited Loading

bparees commented Apr 20, 2016

PI-Victor commented Apr 21, 2016

PI-Victor commented Apr 26, 2016

PI-Victor commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PI-Victor commented Apr 27, 2016

PI-Victor commented Apr 28, 2016

PI-Victor commented Apr 28, 2016

PI-Victor commented Apr 28, 2016

PI-Victor commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PI-Victor commented Apr 28, 2016

bparees commented Apr 28, 2016

jhadvig commented May 2, 2016

PI-Victor commented May 2, 2016

PI-Victor commented May 2, 2016

bparees commented May 2, 2016

PI-Victor commented May 3, 2016

PI-Victor commented May 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented May 3, 2016

deads2k commented May 3, 2016

bparees commented May 3, 2016

PI-Victor commented May 3, 2016

bparees commented May 3, 2016

Choose a reason for hiding this comment

PI-Victor commented May 4, 2016

Choose a reason for hiding this comment

bparees commented May 4, 2016

PI-Victor commented May 5, 2016

Choose a reason for hiding this comment

bparees commented May 5, 2016 • edited Loading

PI-Victor commented May 5, 2016

bparees commented May 5, 2016

bparees commented May 5, 2016

openshift-bot commented May 5, 2016

openshift-bot commented May 5, 2016

bparees commented May 5, 2016

openshift-bot commented May 5, 2016 • edited Loading

openshift-bot commented May 5, 2016

PI-Victor commented Apr 20, 2016 •

edited

Loading

bparees commented May 5, 2016 •

edited

Loading

openshift-bot commented May 5, 2016 •

edited

Loading