Description of problem: Using generic webhook json file contains invalid content can trigger new build without any warnings Version-Release number of selected component (if applicable): openshift v3.2.1.4-1-g1864c8f kubernetes v1.2.0-36-g4a3f9c5 etcd 2.2.5 How reproducible: Always Steps to Reproduce: 1.Create a project, create application $ oc new-app https://raw.githubusercontent.com/openshift/origin/master/examples/sample-app/application-template-stibuild.json 2.Create a generic webhook json file $ vim push-generic.json push-xxxx-xxx 3.Trigger build with generic webhook $ curl -A "Openshift/generic" -H "Content-Type:application/json" -d @push-generic.json https://xxx/oapi/v1/namespaces/test/buildconfigs/ruby-sample-build/webhooks/secret101/generic -k Actual results: can trriger new build Expected results: Should output some warnings Additional info:
We know that payload is optional, the issue is that as a user, if I supply a payload, I'd expect it being respected. And I would be frustrated if server accepts payload without warning, but in fact disregards that payload. btw it is easy for users to supply a filename as a payload instead of the actual content of that file. Typo or ignorance, I observe it often done. For a reasonable UX I think we need to somehow let user know that there is something wrong.
For backwards compatibility reasons I don't think we can change this behavior significantly(for better or worse, the API we put out is one that accepts arbitrary payloads and ignores content it doesn't understand, not one that returns errors/warnings on bad content). Since we currently return nothing but a 200 on a successful posting, I guess we could return a warning page/message along with the 200 if the payload was invalid.
Would be an improvement.
https://github.com/openshift/origin/pull/11077
Commit pushed to master at https://github.com/openshift/origin https://github.com/openshift/origin/commit/aea22a7bbc0bcea587eb76f274ca0d7ce6840f72 WIP Fix bug 1373330 Invalid formatted generic webhook can trigger new-build without warning
Trigger build with invalid branch will cause the invoke failed, and return a code 500 , internal error, should we return code 400? means a bad request ? this is the response : {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Internal error occurred: hook failed: skipping build. Branch reference from \"refs/heads/nobranch\" does not match configuration","reason":"InternalError","details":{"causes":[{"message":"hook failed: skipping build. Branch reference from \"refs/heads/nobranch\" does not match configuration"}]},"code":500}
Wang, it's a fair point. Ben, how do you want to handle this? i.e. where do you want us to stand on the API backwards compatibility vs otherwise correctness continuum? Reviewing the return paths, you could argue that there are points where we should be returning 400 bad request, 401 unauthorized, 404 not found and 500 internal error. Almost certainly the same would apply to the GitHub trigger...
In this case where *I* stand unfortunately doesn't matter :) We'll have to take it to clayton. I suggest we open an issue on github (your changes didn't introduce this behavior, right? so i think this bug can still be verified and we can start a new discussion on the new github issue about other changes). But based on how the other return code discussion went during this PR's development, I assume the answer will be "we're stuck with it."
https://github.com/openshift/origin/issues/11736
Code which introduces the warning when the payload is ignored has merged, so moving to modified.
verified with : openshift v3.4.0.21+ca4702d kubernetes v1.4.0+776c994 etcd 3.1.0-rc.0
Actual result: output warning # curl -A "Openshift/generic" -H "Content-Type:application/json" -d @push-generic.json https://104.197.150.236:8443/oapi/v1/namespaces/dyan/buildconfigs/ruby-sample-build/webhooks/secret101/generic -k {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Success","message":"error unmarshalling payload: invalid character 'p' looking for beginning of value, ignoring payload and continuing with build","code":200}
(In reply to Dongbo Yan from comment #13) > Actual result: output warning > # curl -A "Openshift/generic" -H "Content-Type:application/json" -d > @push-generic.json > https://104.197.150.236:8443/oapi/v1/namespaces/dyan/buildconfigs/ruby- > sample-build/webhooks/secret101/generic -k > > {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Success", > "message":"error unmarshalling payload: invalid character 'p' looking for > beginning of value, ignoring payload and continuing with build","code":200} Hi, Ben, with incorrect format payload(not a valid json file), should fail the trigger?
No, because we couldn't change the existing behavior which returned a 200 and ran the build. What you see is the expected result: the build is run, you get a 200, but you get some warning text letting you know the payload was ignored.
(In reply to Wang Haoran from comment #6) > Trigger build with invalid branch will cause the invoke failed, and return a > code 500 , internal error, should we return code 400? means a bad request ? > this is the response : > {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure", > "message":"Internal error occurred: hook failed: skipping build. Branch > reference from \"refs/heads/nobranch\" does not match > configuration","reason":"InternalError","details":{"causes":[{"message": > "hook failed: skipping build. Branch reference from \"refs/heads/nobranch\" > does not match configuration"}]},"code":500} Wang, I'm sorry, I didn't read your comment closely enough. It is in fact wrong that 500 is returned in this case, it should be 200 as it was before. https://github.com/openshift/origin/pull/11810 is open to resolve this, as well as convert as many other 500s to 40*s as is feasible. Once the PR is merged, it will be necessary to review.
Merged, please review.
This has been merged into ose and is in OSE v3.4.0.25 or newer.
Hi, Jim, I found that for the generic/webhook trigger url, when we failed to trigger a build with incorrect payload we will get an status json response like : {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"unexpected end of JSON input","reason":"BadRequest","code":400} but when we succeed to trigger a build with correct payload, will not receive a status json response, question is whether we need return this when the trigger succeed, with a status 200 and "trigger succeed " message ?
Hi Wang, no, the intention was not to add status messages to correct invocations: only as warnings to invocations which were bad but would result in a trigger nonetheless for historical reasons. The reason was, as far as possible, to minimise changes while improving experience where possible. So I believe what you found is correct.
verified with : openshift v3.4.0.26+f7e109e kubernetes v1.4.0+776c994 etcd 3.1.0-rc.0
Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory, and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHBA-2017:0066