WIP Fix bug 1373330 Invalid formatted generic webhook can trigger new… #11077

jim-minter · 2016-09-23T16:13:31Z

… build without warning

jim-minter · 2016-09-23T16:47:16Z

bparees · 2016-09-23T19:08:59Z

pkg/build/webhook/generic/generic.go

@@ -43,67 +43,74 @@ func (p *WebHookPlugin) Extract(buildCfg *api.BuildConfig, secret, path string,
 	}

 	if buildCfg.Spec.Source.Git == nil {
-		glog.V(4).Infof("No git source defined for BuildConfig %s/%s, but triggering anyway", buildCfg.Namespace, buildCfg.Name)
-		return revision, envvars, true, err
+		warning := webhook.NewWarning("BuildConfig has no git source defined, ignoring and continuing with build")


i don't think we need this warning anymore. once upon a time git source was pretty much it, but now you could have pipeline builds, or builds that take input from images and builds that embed docker files. if we're going to keep it, it needs to check for all the possible sources of "source" input, basically to ensure it's not a pure binary-only buildconfig.

bparees · 2016-09-23T19:10:13Z

pkg/build/webhook/generic/generic.go

 	}

 	contentType := req.Header.Get("Content-Type")
 	if len(contentType) != 0 {
 		contentType, _, err = mime.ParseMediaType(contentType)
 		if err != nil {
-			return nil, envvars, false, fmt.Errorf("non-parseable Content-Type %s (%s)", contentType, err)
+			return revision, envvars, false, fmt.Errorf("Error parsing Content-Type: %s", err)


i don't know the reason behind it, but we never uppercase the start of our error messages. possibly because they end up embedded in other output and look strange.

bparees · 2016-09-23T19:13:44Z

pkg/build/webhook/generic/generic.go

+		envvars = data.Env
+	}
+	if data.Git == nil {
+		warning := webhook.NewWarning("No git information found in payload, ignoring and continuing with build")


i don't think this necessitates a warning. if we warn here, users who just want to supply env variables are going to get spurious warnings.

I think if

we can parse the payload and

the result is either git data or env variable data (or both)

we should not warn.

bparees · 2016-09-23T19:15:55Z

pkg/build/webhook/generic/generic_test.go

+func matchWarning(t *testing.T, err error, message string) {
+	if status, ok := err.(*errors.StatusError); !ok {
+		t.Errorf("Expected %v to be a StatusError object", err)
+	} else {


no need for the else. just return inside the if-block above.

bparees · 2016-09-23T19:25:34Z

@openshift/api-review any other thoughts on how we can handle returning a warning on the webhook invocation? basically we need to tell them "you gave us bad payload data, but we're running a build anyway(because that's what we've always done, so compatibility...), using the buildconfig defaults".

so here we're returning a StatusError w/ a status code of 200 and a warning message which feels icky to me, but i don't have a better idea.

jim-minter · 2016-09-26T10:53:45Z

@bparees all changes requested above made, ptal

bparees · 2016-09-26T12:41:47Z

pkg/build/webhook/generic/generic.go

 		}
+		warning := webhook.NewWarning(fmt.Sprintf("skipping build. None of the supplied refs matched %q", buildCfg.Spec.Source.Git.Ref))


why is this a warning and not an error, since it's not actually going to result in a build?

It is maintaining previous API. This is identical behaviour as before, but I converted a glog.V(2).Infof (only accessible by admin, and tricky to find) to a returned "warning" (clearer to end user) as it felt within the scope and aims of this patch.

The existing api returns a 200 in this scenario?

That is my understanding.

And I've tried it again - yes, it currently returns a 200 in this case.

hm. that was probably the wrong choice when we implemented the api, but ok I guess we're stuck with it. Still feels awkward to call it a warning since in all the other cases a warning means "something went wrong but we're proceeding with the build" and here the warning means "something went wrong and we're not doing the build even though we gave you back a 200"

I wonder if we could get away w/ changing this to a non-200 return code even though it's technically breaking the existing api. @smarterclayton ?

i'm going to interpret @smarterclayton's response on the main thread as a "no, you can't change it to a non-200". oh well.

bparees · 2016-09-26T12:41:53Z

pkg/build/webhook/generic/generic.go

+		return revision, envvars, false, warning
+	}
+	if !webhook.GitRefMatches(data.Git.Ref, webhook.DefaultConfigRef, &buildCfg.Spec.Source) {
+		warning := webhook.NewWarning(fmt.Sprintf("skipping build. Branch reference from %q does not match configuration", data.Git.Ref))


same question here

smarterclayton · 2016-09-26T15:18:37Z

Is this bad data we allowed to be sent in 1.0? Or bad data we started allowing someone to send us in 1.3?

bparees · 2016-09-26T15:26:46Z

Is this bad data we allowed to be sent in 1.0? Or bad data we started allowing someone to send us in 1.3?

we've always allowed this data to be sent, though it looks like in the original implementation we immediately failed if the data wasn't readable and at some point we changed to proceeding w/ the build but ignoring the bad data:

6594ffd#diff-d0545fc405d0111150cfcc584145ed18

That change was in march 2015.

smarterclayton · 2016-09-26T20:53:09Z

Did we actually honor any data in here in 1.0? Or was that only later
where we added info to generic web hook?

On Mon, Sep 26, 2016 at 11:26 AM, Ben Parees [email protected]
wrote:

Is this bad data we allowed to be sent in 1.0? Or bad data we started
allowing someone to send us in 1.3?

we've always allowed this data to be sent, though it looks like in the
original implementation we immediately failed if the data wasn't readable
and at some point we changed to proceeding w/ the build but ignoring the
bad data:

6594ffd#diff-d0545fc405d0111150cfcc584145ed18
6594ffd#diff-d0545fc405d0111150cfcc584145ed18

That change was in march 2015.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11077 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7RSsMh5HPAzvlAkZAv6Phh63MHSks5qt-Q4gaJpZM4KFJXe
.

bparees · 2016-09-26T21:19:18Z

Did we actually honor any data in here in 1.0? Or was that only later
where we added info to generic web hook?

yes we honored it. we used the git ref.

jim-minter · 2016-09-28T09:11:56Z

@smarterclayton @bparees the conversation left me hanging - please advise if I need to make changes to the PR in order to get it merged.

bparees · 2016-09-28T13:38:53Z

@smarterclayton @bparees the conversation left me hanging - please advise if I need to make changes to the PR in order to get it merged.

at this point i'm fine with it as is. still need @openshift/api-review to sign off though.

bparees · 2016-10-19T00:54:03Z

@openshift/api-review bump

smarterclayton · 2016-10-19T23:01:46Z

Preserve existing error behavior and document why it is that way.

bparees · 2016-10-20T02:56:44Z

this PR is preserving the existing behavior, just adding a warning to the response body (while still returning a 200 status)

@jim-minter you can doc the behavior here:
https://docs.openshift.org/latest/dev_guide/builds.html#webhook-triggers

smarterclayton · 2016-10-20T03:42:34Z

That is correct.

On Wed, Oct 19, 2016 at 10:59 PM, Ben Parees [email protected]
wrote:

@bparees commented on this pull request.

In pkg/build/webhook/generic/generic.go
#11077:
    }
warning := webhook.NewWarning(fmt.Sprintf("skipping build. None of the supplied refs matched %q", buildCfg.Spec.Source.Git.Ref))
i'm going to interpret @smarterclayton https://github.com/smarterclayton's
response on the main thread as a "no, you can't change it to a non-200". oh
well.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11077, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p9lyQQ6SPBxyllLWw_fXjWTaaOFJks5q1tktgaJpZM4KFJXe
.

bparees · 2016-10-20T16:19:02Z

@openshift/api-review so is this api approved? basically the change is a warning message that comes back w/ your 200 status code in certain cases.

smarterclayton · 2016-10-20T22:19:37Z

It's possible you're going to break people because they aren't expecting a body. What are the odds of that?

jim-minter · 2016-10-21T07:12:52Z

@smarterclayton Inevitably it is feasible, but I think it's a pretty poor show if any modern HTTP client posts a GET or a POST and doesn't know what to do with a response. It's hardly out of HTTP spec for us to do this. The warnings closely match the k8s StatusError responses so are in idiom for a client which is already reading these for error cases. In addition I think they add helpful information (you sent content with an invalid content-type; your build didn't trigger because of XYZ) which is better sent back than left lurking in a server-side log.

smarterclayton · 2016-10-22T00:14:28Z

Once we've released an API we have to act like every change could break a client. For this one the odds seem low, so I'll let it pass here. But we've broken people before with much less why it's important to be cautious.

bparees · 2016-10-22T02:56:54Z

@smarterclayton can you mark it approved, then?

bparees · 2016-10-24T21:37:46Z

[merge]

openshift-bot · 2016-10-24T23:37:33Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10622/) (Image: devenv-rhel7_5240)

bparees · 2016-10-25T01:26:30Z

--- FAIL: TestWebhookGitHubPing (5.16s)
    webhookgithub_test.go:300: Wrong response code, expecting 200, got 500: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: hook failed: \u003cnil\u003e","reason":"InternalError","details":{"causes":[{"message":"hook failed: \u003cnil\u003e"}]},"code":500}

…-build without warning

jim-minter · 2016-10-25T14:35:39Z

flake #11517

bparees · 2016-10-25T15:21:19Z

[test]

openshift-bot · 2016-10-25T15:29:37Z

Evaluated for origin test up to aea22a7

openshift-bot · 2016-10-25T17:00:34Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10622/) (Base Commit: 1f62556)

bparees · 2016-10-25T17:33:51Z

[merge]

openshift-bot · 2016-10-25T17:37:21Z

Evaluated for origin merge up to aea22a7

jim-minter · 2016-10-31T18:54:04Z

@bparees just to check with this - is the expectation a docs PR against https://docs.openshift.org/latest/dev_guide/builds.html#webhook-triggers and doc text in https://bugzilla.redhat.com/show_bug.cgi?id=1373330 ?

bparees · 2016-10-31T19:41:15Z

@jim-minter yup

…ors instead of 500 errors where possible

bparees requested changes Sep 23, 2016

View reviewed changes

bparees added the needs-api-review label Sep 23, 2016

jim-minter force-pushed the bz1373330 branch from c7e392e to a17f812 Compare September 26, 2016 10:52

bparees requested changes Sep 26, 2016

View reviewed changes

jim-minter self-assigned this Sep 28, 2016

bparees assigned bparees and unassigned jim-minter Oct 21, 2016

smarterclayton added api-approved and removed needs-api-review labels Oct 22, 2016

WIP Fix bug 1373330 Invalid formatted generic webhook can trigger new…

aea22a7

…-build without warning

jim-minter force-pushed the bz1373330 branch from a17f812 to aea22a7 Compare October 25, 2016 12:05

openshift-bot merged commit 833eb03 into openshift:master Oct 25, 2016

jim-minter mentioned this pull request Nov 2, 2016

Triggering build with invalid branch will cause the invoke to fail and return a code 500 #11736

Closed

jim-minter pushed a commit to jim-minter/origin that referenced this pull request Nov 7, 2016

resolve logic errors introduced in openshift#11077 and return 40x err…

5968f11

…ors instead of 500 errors where possible

jim-minter pushed a commit to jim-minter/origin that referenced this pull request Nov 8, 2016

resolve logic errors introduced in openshift#11077 and return 40x err…

6243982

…ors instead of 500 errors where possible

		}
		warning := webhook.NewWarning(fmt.Sprintf("skipping build. None of the supplied refs matched %q", buildCfg.Spec.Source.Git.Ref))

WIP Fix bug 1373330 Invalid formatted generic webhook can trigger new… #11077

WIP Fix bug 1373330 Invalid formatted generic webhook can trigger new… #11077

Conversation

jim-minter commented Sep 23, 2016

jim-minter commented Sep 23, 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

bparees commented Sep 23, 2016

jim-minter commented Sep 26, 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

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

smarterclayton commented Sep 26, 2016

bparees commented Sep 26, 2016

smarterclayton commented Sep 26, 2016

bparees commented Sep 26, 2016

jim-minter commented Sep 28, 2016

bparees commented Sep 28, 2016

bparees commented Oct 19, 2016

smarterclayton commented Oct 19, 2016

bparees commented Oct 20, 2016

smarterclayton commented Oct 20, 2016

@bparees commented on this pull request.

bparees commented Oct 20, 2016

smarterclayton commented Oct 20, 2016

jim-minter commented Oct 21, 2016

smarterclayton commented Oct 22, 2016

bparees commented Oct 22, 2016

bparees commented Oct 24, 2016

openshift-bot commented Oct 24, 2016 • edited Loading

bparees commented Oct 25, 2016

jim-minter commented Oct 25, 2016

bparees commented Oct 25, 2016

openshift-bot commented Oct 25, 2016

openshift-bot commented Oct 25, 2016

bparees commented Oct 25, 2016

openshift-bot commented Oct 25, 2016

jim-minter commented Oct 31, 2016

bparees commented Oct 31, 2016

openshift-bot commented Oct 24, 2016 •

edited

Loading