Skip to content

Commit

Permalink
fix panic in github interceptor
Browse files Browse the repository at this point in the history
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.

wip:
 - tests for HandEventWithSingleInterceptor
 - tests for HandleEventWithMultipleInterceptors

fixes #355

Signed-off-by: Dibyo Mukherjee <[email protected]>
  • Loading branch information
dibyom committed Jan 17, 2020
1 parent bc6916b commit a3c6b94
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 25 deletions.
11 changes: 4 additions & 7 deletions pkg/interceptors/cel/cel.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package cel

import (
"bytes"
"encoding/json"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -58,12 +59,8 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err
return nil, fmt.Errorf("error creating cel environment: %w", err)
}

body, err := request.GetBody()
if err != nil {
return nil, fmt.Errorf("error getting request body: %w", err)
}
defer body.Close()
payload, err := ioutil.ReadAll(body)
defer request.Body.Close()
payload, err := ioutil.ReadAll(request.Body)
if err != nil {
return nil, fmt.Errorf("error reading request body: %w", err)
}
Expand All @@ -84,7 +81,7 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err

return &http.Response{
Header: request.Header,
Body: request.Body,
Body: ioutil.NopCloser(bytes.NewBuffer(payload)),
}, nil
}

Expand Down
19 changes: 8 additions & 11 deletions pkg/interceptors/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package github

import (
"bytes"
"errors"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -46,22 +47,18 @@ func NewInterceptor(gh *triggersv1.GitHubInterceptor, k kubernetes.Interface, ns
}

func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, error) {
defer request.Body.Close()
payload, err := ioutil.ReadAll(request.Body)
if err != nil {
return nil, fmt.Errorf("failed to read request body: %w", err)
}

// Validate secrets first before anything else, if set
if w.GitHub.SecretRef != nil {
header := request.Header.Get("X-Hub-Signature")
if header == "" {
return nil, errors.New("no X-Hub-Signature header set")
}

body, err := request.GetBody()
if err != nil {
return nil, err
}
defer body.Close()
payload, err := ioutil.ReadAll(body)
if err != nil {
return nil, err
}
secretToken, err := interceptors.GetSecretToken(w.KubeClientSet, w.GitHub.SecretRef, w.EventListenerNamespace)
if err != nil {
return nil, err
Expand All @@ -88,6 +85,6 @@ func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, err

return &http.Response{
Header: request.Header,
Body: request.Body,
Body: ioutil.NopCloser(bytes.NewBuffer(payload)),
}, nil
}
2 changes: 1 addition & 1 deletion pkg/resources/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID,
if name == "" {
name = data.GetGenerateName()
}
logger.Infof("Generating resource: kind: %+v, name: %s", apiResource, name)
logger.Infof("Generating resource: kind: %s, name: %s", apiResource, name)

gvr := schema.GroupVersionResource{
Group: apiResource.Group,
Expand Down
11 changes: 7 additions & 4 deletions pkg/sink/sink.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package sink

import (
"bytes"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -72,7 +73,6 @@ func (r Sink) HandleEvent(response http.ResponseWriter, request *http.Request) {
response.WriteHeader(http.StatusInternalServerError)
return
}

event, err := ioutil.ReadAll(request.Body)
if err != nil {
r.Logger.Errorf("Error reading event body: %s", err)
Expand Down Expand Up @@ -158,7 +158,11 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R
return event, in.Header, nil
}

request := in
// The request body to the first interceptor in the chain should be the received event body.
request := &http.Request{
Header: in.Header,
Body: ioutil.NopCloser(bytes.NewBuffer(event)),
}
var resp *http.Response
for _, i := range t.Interceptors {
var interceptor interceptors.Interceptor
Expand All @@ -174,7 +178,6 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R
default:
return nil, nil, fmt.Errorf("unknown interceptor type: %v", i)
}

var err error
resp, err = interceptor.ExecuteTrigger(request)
if err != nil {
Expand All @@ -185,7 +188,7 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R
// request chaining.
request = &http.Request{
Header: resp.Header,
Body: resp.Body,
Body: ioutil.NopCloser(resp.Body),
}
}
payload, err := ioutil.ReadAll(resp.Body)
Expand Down
4 changes: 2 additions & 2 deletions pkg/sink/sink_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -295,11 +295,11 @@ func TestExecuteInterceptor(t *testing.T) {
Interceptors: []*triggersv1.EventInterceptor{a, a},
}

req, err := http.NewRequest(http.MethodPost, "/", bytes.NewBufferString(`{}`))
req, err := http.NewRequest(http.MethodPost, "/", nil)
if err != nil {
t.Fatalf("http.NewRequest: %v", err)
}
resp, header, err := r.executeInterceptors(trigger, req, nil, "", logger)
resp, header, err := r.executeInterceptors(trigger, req, []byte(`{}`), "", logger)
if err != nil {
t.Fatalf("executeInterceptors: %v", err)
}
Expand Down

0 comments on commit a3c6b94

Please sign in to comment.