From eebd49db423a7c107ea52a0b43517685be9130b8 Mon Sep 17 00:00:00 2001 From: Marcel Ludwig Date: Thu, 25 Mar 2021 11:57:24 +0100 Subject: [PATCH 1/5] Use io noopcloser instead since close call is not required --- eval/http.go | 2 +- handler/endpoint.go | 3 ++- handler/transport/roundtrip.go | 5 +---- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/eval/http.go b/eval/http.go index 35da7d4f6..4ae26f04f 100644 --- a/eval/http.go +++ b/eval/http.go @@ -61,7 +61,7 @@ func SetGetBody(req *http.Request, bodyLimit int64) error { bodyBytes := buf.Bytes() req.GetBody = func() (io.ReadCloser, error) { - return NewReadCloser(bytes.NewBuffer(bodyBytes), req.Body), nil + return io.NopCloser(bytes.NewBuffer(bodyBytes)), nil } } diff --git a/handler/endpoint.go b/handler/endpoint.go index 05a0dee88..fc087294c 100644 --- a/handler/endpoint.go +++ b/handler/endpoint.go @@ -2,6 +2,7 @@ package handler import ( "context" + "io" "net" "net/http" "strconv" @@ -203,7 +204,7 @@ func (e *Endpoint) newResponse(req *http.Request, evalCtx *eval.Context) (*http. } r := strings.NewReader(seetie.ValueToString(val)) - clientres.Body = eval.NewReadCloser(r, nil) + clientres.Body = io.NopCloser(r) } return clientres, nil diff --git a/handler/transport/roundtrip.go b/handler/transport/roundtrip.go index 1f0cd6f92..e1ccb558f 100644 --- a/handler/transport/roundtrip.go +++ b/handler/transport/roundtrip.go @@ -6,8 +6,6 @@ import ( "io" "net" "net/http" - - "github.com/avenga/couper/eval" ) var ( @@ -55,8 +53,7 @@ func (r *Recorder) Read(p []byte) (n int, err error) { func (r *Recorder) Response(req *http.Request) (*http.Response, error) { return &http.Response{ - Body: eval.NewReadCloser(r.body, nil), - Header: r.Header(), + Body: io.NopCloser(r.body), Proto: req.Proto, ProtoMajor: req.ProtoMajor, ProtoMinor: req.ProtoMinor, From 1f1e96230d731ef6b5cf99284f06ff748ba1bc5b Mon Sep 17 00:00:00 2001 From: Marcel Ludwig Date: Thu, 25 Mar 2021 12:02:04 +0100 Subject: [PATCH 2/5] Fix possible data race while accessing headers in server clone first, setGetBody afterwards --- handler/producer/proxy.go | 3 ++- handler/transport/roundtrip.go | 1 + server/http.go | 6 ++++-- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/handler/producer/proxy.go b/handler/producer/proxy.go index 0ed5a68ad..27d340707 100644 --- a/handler/producer/proxy.go +++ b/handler/producer/proxy.go @@ -37,7 +37,8 @@ func (pr Proxies) Produce(ctx context.Context, clientReq *http.Request, results currentName = proxy.Name outCtx := withRoundTripName(ctx, proxy.Name) outCtx = context.WithValue(outCtx, request.RoundTripProxy, true) - outReq := clientReq.WithContext(outCtx) + // since proxy and backend may work on the "same" outReq this must be cloned. + outReq := clientReq.Clone(outCtx) wg.Add(1) go roundtrip(proxy.RoundTrip, outReq, results, wg) diff --git a/handler/transport/roundtrip.go b/handler/transport/roundtrip.go index e1ccb558f..004d000f3 100644 --- a/handler/transport/roundtrip.go +++ b/handler/transport/roundtrip.go @@ -54,6 +54,7 @@ func (r *Recorder) Read(p []byte) (n int, err error) { func (r *Recorder) Response(req *http.Request) (*http.Response, error) { return &http.Response{ Body: io.NopCloser(r.body), + Header: r.Header().Clone(), Proto: req.Proto, ProtoMajor: req.ProtoMajor, ProtoMinor: req.ProtoMinor, diff --git a/server/http.go b/server/http.go index 2c125379d..4d6fa23f7 100644 --- a/server/http.go +++ b/server/http.go @@ -199,13 +199,15 @@ func (s *HTTPServer) ServeHTTP(rw http.ResponseWriter, req *http.Request) { ) rw = w - if err := s.setGetBody(h, req); err != nil { + clientReq := req.Clone(ctx) + + if err := s.setGetBody(h, clientReq); err != nil { mux.opts.ServerOptions.ServerErrTpl.ServeError(err).ServeHTTP(rw, req) return } ctx = s.evalCtx.WithClientRequest(req) - clientReq := req.Clone(ctx) + *clientReq = *clientReq.WithContext(ctx) s.accessLog.ServeHTTP(rw, clientReq, h, startTime) From 51184d0e18544bc9795cd79d2679af13e57b5073 Mon Sep 17 00:00:00 2001 From: Marcel Ludwig Date: Tue, 30 Mar 2021 14:51:17 +0200 Subject: [PATCH 3/5] Fixup eval test due to added req clone for proxy outreqs the test-headers must be reflected --- handler/endpoint_test.go | 32 ++++++++++++++++++------------- handler/transport/backend_test.go | 6 +++--- internal/test/helper_proxy.go | 2 +- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/handler/endpoint_test.go b/handler/endpoint_test.go index 375dd96f4..e212d1a42 100644 --- a/handler/endpoint_test.go +++ b/handler/endpoint_test.go @@ -133,16 +133,22 @@ func TestEndpoint_RoundTripContext_Variables_json_body(t *testing.T) { defaultMethods := []string{ http.MethodGet, - //http.MethodHead, - //http.MethodPost, - //http.MethodPut, - //http.MethodPatch, - //http.MethodDelete, - //http.MethodConnect, - //http.MethodOptions, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + http.MethodConnect, + http.MethodOptions, } origin := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + // reflect req headers + for k, v := range r.Header { + if !strings.HasPrefix(strings.ToLower(k), "x-") { + continue + } + rw.Header()[k] = v + } rw.WriteHeader(http.StatusNoContent) })) defer origin.Close() @@ -164,7 +170,7 @@ func TestEndpoint_RoundTripContext_Variables_json_body(t *testing.T) { origin = "` + origin.URL + `" set_request_headers = { x-test = req.json_body.foo - }`, []string{http.MethodTrace}, test.Header{"Content-Type": "application/json"}, `{"foo": "bar"}`, want{req: test.Header{"x-test": ""}}}, + }`, []string{http.MethodTrace, http.MethodHead}, test.Header{"Content-Type": "application/json"}, `{"foo": "bar"}`, want{req: test.Header{"x-test": ""}}}, {"method /wo body", ` origin = "` + origin.URL + `" set_request_headers = { @@ -182,7 +188,7 @@ func TestEndpoint_RoundTripContext_Variables_json_body(t *testing.T) { helper := test.New(subT) backend := transport.NewBackend( - helper.NewProxyContext(tt.inlineCtx), + helper.NewInlineContext(tt.inlineCtx), &transport.Config{NoProxyFromEnv: true}, nil, logger) ep := handler.NewEndpoint(&handler.EndpointOptions{ @@ -208,11 +214,11 @@ func TestEndpoint_RoundTripContext_Variables_json_body(t *testing.T) { rw := server.NewRWWrapper(rec, false, "") // crucial for working ep due to res.Write() ep.ServeHTTP(rw, req) rec.Flush() - //res := rec.Result() + res := rec.Result() for k, v := range tt.want.req { - if req.Header.Get(k) != v { - subT.Errorf("want: %q for key %q, got: %q", v, k, req.Header.Get(k)) + if res.Header.Get(k) != v { + subT.Errorf("want: %q for key %q, got: %q", v, k, res.Header[k]) } } }) @@ -272,7 +278,7 @@ func TestEndpoint_RoundTripContext_Null_Eval(t *testing.T) { ep := handler.NewEndpoint(&handler.EndpointOptions{ Error: errors.DefaultJSON, - Context: helper.NewProxyContext(tc.remain), + Context: helper.NewInlineContext(tc.remain), ReqBodyLimit: 1024, }, logger, producer.Proxies{ &producer.Proxy{Name: "default", RoundTrip: backend}, diff --git a/handler/transport/backend_test.go b/handler/transport/backend_test.go index 607d8801d..38ba9dd63 100644 --- a/handler/transport/backend_test.go +++ b/handler/transport/backend_test.go @@ -232,7 +232,7 @@ func TestBackend_RoundTrip_Validation(t *testing.T) { if err != nil { subT.Fatal(err) } - content := helper.NewProxyContext(` + content := helper.NewInlineContext(` origin = "` + origin.URL + `" `) @@ -311,7 +311,7 @@ func TestBackend_director(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - hclContext := helper.NewProxyContext(tt.inlineCtx) + hclContext := helper.NewInlineContext(tt.inlineCtx) backend := transport.NewBackend(hclContext, &transport.Config{ Timeout: time.Second, @@ -400,7 +400,7 @@ func TestProxy_BufferingOptions(t *testing.T) { backend := transport.NewBackend(configload.MergeBodies([]hcl.Body{ test.NewRemainContext("origin", "http://"+origin.Listener.Addr().String()), - helper.NewProxyContext(tc.remain), + helper.NewInlineContext(tc.remain), }), &transport.Config{}, &transport.BackendOptions{ OpenAPI: newOptions(), }, nullLog) diff --git a/internal/test/helper_proxy.go b/internal/test/helper_proxy.go index ff7c965ec..1a34fe3b1 100644 --- a/internal/test/helper_proxy.go +++ b/internal/test/helper_proxy.go @@ -36,7 +36,7 @@ func (h *Helper) NewProxy(conf *transport.Config, backendContext, proxyContext h return proxy } -func (h *Helper) NewProxyContext(inlineHCL string) hcl.Body { +func (h *Helper) NewInlineContext(inlineHCL string) hcl.Body { type hclBody struct { Inline hcl.Body `hcl:",remain"` } From e1367ff244e330ddebe6a364f7e986570d39a7b7 Mon Sep 17 00:00:00 2001 From: Marcel Ludwig Date: Tue, 30 Mar 2021 17:42:42 +0200 Subject: [PATCH 4/5] Extend proxy / req test Accessing client req headers multiple times --- server/http_endpoints_test.go | 8 +++--- server/testdata/endpoints/01_couper.hcl | 34 ++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/server/http_endpoints_test.go b/server/http_endpoints_test.go index 4158d3134..5c46c1157 100644 --- a/server/http_endpoints_test.go +++ b/server/http_endpoints_test.go @@ -33,8 +33,8 @@ func TestEndpoints_ProxyReqRes(t *testing.T) { helper.Must(err) entries := logHook.Entries - if l := len(entries); l != 3 { - t.Fatalf("Expected 3 log entries, given %d", l) + if l := len(entries); l != 5 { + t.Fatalf("Expected 5 log entries, given %d", l) } if res.StatusCode != http.StatusMethodNotAllowed { @@ -45,8 +45,8 @@ func TestEndpoints_ProxyReqRes(t *testing.T) { helper.Must(err) res.Body.Close() - if string(resBytes) != "808" { - t.Errorf("Expected body 808, given %s", resBytes) + if string(resBytes) != "1616" { + t.Errorf("Expected body 1616, given %s", resBytes) } } diff --git a/server/testdata/endpoints/01_couper.hcl b/server/testdata/endpoints/01_couper.hcl index d90ca223a..e97959329 100644 --- a/server/testdata/endpoints/01_couper.hcl +++ b/server/testdata/endpoints/01_couper.hcl @@ -10,15 +10,37 @@ server "api" { proxy { url = "${env.COUPER_TEST_BACKEND_ADDR}/proxy" backend = "proxy" + set_request_headers = { + x-inline = "test" + } } - request "request" { + + proxy "p2" { + url = "${env.COUPER_TEST_BACKEND_ADDR}/proxy" + backend = "proxy" + set_request_headers = { + x-inline = "test" + } + } + + request "r1" { + url = "${env.COUPER_TEST_BACKEND_ADDR}/request" + backend = "request" + } + + request "r2" { url = "${env.COUPER_TEST_BACKEND_ADDR}/request" backend = "request" } + + set_request_headers = { + x-ep-inline = "test" + } + response { status = beresp.status + 1 - # 404 + 404 - body = beresps.request.status + beresps.default.status + # 404 + 404 + 404 + 404 + body = beresps.r1.status + beresps.default.status + beresps.r2.status + beresps.p2.status } } } @@ -29,9 +51,15 @@ definitions { backend "proxy" { path = "/override/me" origin = env.COUPER_TEST_BACKEND_ADDR + set_request_headers = { + x-data = "proxy-test" + } } backend "request" { path = "/override/me" origin = env.COUPER_TEST_BACKEND_ADDR + set_request_headers = { + x-data = "request-test" + } } } From e1babaea82f0fefc327965389b31250987fef7e4 Mon Sep 17 00:00:00 2001 From: Marcel Ludwig Date: Tue, 30 Mar 2021 17:44:08 +0200 Subject: [PATCH 5/5] Fix loading optional labels of same type Replaced chicken-and-egg retry method with bodyToContent --- config/configload/load.go | 72 +++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/config/configload/load.go b/config/configload/load.go index 0be8a808e..7afdce21c 100644 --- a/config/configload/load.go +++ b/config/configload/load.go @@ -5,7 +5,6 @@ import ( "io/ioutil" "path/filepath" "regexp" - "strings" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/gohcl" @@ -221,17 +220,11 @@ func mergeBackendBodies(definedBackends Backends, inline config.Inline) (hcl.Bod // getBackendReference tries to fetch a backend from `definitions` // block by a reference name, e.g. `backend = "name"`. func getBackendReference(definedBackends Backends, body hcl.Body) (hcl.Body, error) { - content, _, diags := body.PartialContent(&hcl.BodySchema{ - Attributes: []hcl.AttributeSchema{ - {Name: backend}, - }}) - if diags.HasErrors() { - return nil, diags - } + content := bodyToContent(body) // read out possible attribute reference var name string - if attr, ok := content.Attributes["backend"]; ok { + if attr, ok := content.Attributes[backend]; ok { val, valDiags := attr.Expr.Value(envContext) if valDiags.HasErrors() { return nil, valDiags @@ -263,22 +256,7 @@ func getBackendReference(definedBackends Backends, body hcl.Body) (hcl.Body, err func refineEndpoints(definedBackends Backends, endpoints config.Endpoints) error { for _, endpoint := range endpoints { - // try to obtain proxy and request block with a chicken-and-egg situation: - // hcl labels are required if set, to make them optional we must know the content - // which could not unwrapped without label errors. We will handle this by block type - // and may have to throw an error which hints the user to configure the file properly. - endpointContent := &hcl.BodyContent{Attributes: make(hcl.Attributes)} - for _, t := range []string{proxy, request} { - c, err := contentByType(t, endpoint.Remain) - if err != nil { - return err - } - endpointContent.MissingItemRange = c.MissingItemRange - endpointContent.Blocks = append(endpointContent.Blocks, c.Blocks...) - for n, attr := range c.Attributes { // possible same key and content override, it's ok. - endpointContent.Attributes[n] = attr - } - } + endpointContent := bodyToContent(endpoint.Remain) proxies := endpointContent.Blocks.OfType(proxy) requests := endpointContent.Blocks.OfType(request) @@ -422,6 +400,41 @@ func uniqueLabelName(unique map[string]struct{}, name string, hr *hcl.Range) err return nil } +func bodyToContent(body hcl.Body) *hcl.BodyContent { + content := &hcl.BodyContent{ + MissingItemRange: body.MissingItemRange(), + } + b, ok := body.(*hclsyntax.Body) + if !ok { + return content + } + + if len(b.Attributes) > 0 { + content.Attributes = make(hcl.Attributes) + } + for name, attr := range b.Attributes { + content.Attributes[name] = &hcl.Attribute{ + Name: attr.Name, + Expr: attr.Expr, + Range: attr.Range(), + NameRange: attr.NameRange, + } + } + + for _, block := range b.Blocks { + content.Blocks = append(content.Blocks, &hcl.Block{ + Body: block.Body, + DefRange: block.DefRange(), + LabelRanges: block.LabelRanges, + Labels: block.Labels, + Type: block.Type, + TypeRange: block.TypeRange, + }) + } + + return content +} + func contentByType(blockType string, body hcl.Body) (*hcl.BodyContent, error) { headerSchema := &hcl.BodySchema{ Blocks: []hcl.BlockHeaderSchema{ @@ -501,18 +514,11 @@ func newOAuthBackend(definedBackends Backends, parent hcl.Body) (hcl.Body, error return nil, err } - b, err := newBackend(definedBackends, &config.OAuth2{Remain: hclbody.New(&hcl.BodyContent{ + return newBackend(definedBackends, &config.OAuth2{Remain: hclbody.New(&hcl.BodyContent{ Blocks: []*hcl.Block{ {Type: backend, Body: oauthBackend}, }, })}) - if err != nil { - diags := err.(hcl.Diagnostics) - if strings.HasPrefix(diags[0].Summary, "The host of 'url'") { - diags[0].Summary = strings.Replace(diags[0].Summary, "The host of 'url'", "The host of 'token_endpoint'", 1) - } - } - return b, err } func renameAttribute(content *hcl.BodyContent, old, new string) {