fix panic in github interceptor #357
Conversation
tekton-robot
added
the
do-not-merge/work-in-progress
tekton-robot
added
the
size/M
dibyom
force-pushed
the
panic
branch
2 times, most recently
from
a3c6b94
to
b2535aa
Compare
tekton-robot
added
size/L
tekton-robot
removed
the
do-not-merge/work-in-progress
|
tektoncd/plumbing#186 is hitting here too :( |
|
/retest |
| @@ -231,6 +231,189 @@ func TestHandleEvent(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestHandleEventWithInterceptors(t *testing.T) { | |||
There was a problem hiding this comment.
This is repeating TestHandleEvent with only a few differences. We could use a testing table so that we could test [0-2] interceptors in a bit less verbose way. This seems fine though.
There was a problem hiding this comment.
Yeah long term, I want to extract the common bits out into a couple of helper methods that create the required resources, and the clients (like we do in the reconciler tests in pipeline). For this PR though, I wanted to keep the non-fix changes to a minimum
pkg/interceptors/cel/cel.go
Outdated
| } | ||
| defer body.Close() | ||
| payload, err := ioutil.ReadAll(body) | ||
| defer request.Body.Close() |
There was a problem hiding this comment.
This still runs the risk of panicking if request or Body is nil. We probably want to have checks here to handle gracefully.
Example - https://play.golang.org/p/HstyZm09vuE
There was a problem hiding this comment.
Good point; let me add some checks
There was a problem hiding this comment.
testing this is going to require us to refactor our tests a bit...gonna make this a new commit in this PR!
fixed! PTAL
We were passing the original request where the body had been drained to the first interceptor meaning that the body to the first interceptor in the chain was always empty. Further, we were using the `GetBody` which can sometimes be empty leading to a panic. fixes tektoncd#355 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
| payload, err := ioutil.ReadAll(body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error reading request body: %w", err) | ||
| var payload = []byte(`{}`) |
There was a problem hiding this comment.
I don't feel too strongly about this, but should we be consistent with the webhook handler and default to []byte{}?
There was a problem hiding this comment.
Sure, we'd have to do it the other way around though since makeCelEnv errors out if you pass it []byte{}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester, wlynch The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
Changes
We were passing the original request where the body had been drained
to the first interceptor meaning that the body to the first interceptor
in the chain was always empty. Further, we were using the
GetBodywhichcan sometimes be empty leading to a panic.
fixes #355
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes