From bf3d2f436f66344e5a9a94761656361d49e25a86 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Tue, 4 Feb 2020 14:38:55 -0500 Subject: [PATCH 01/22] Added new FlagList type and used it in jaeger-query Signed-off-by: Joe Elliott --- cmd/query/app/flags.go | 17 +++++++++++------ cmd/query/app/flags_test.go | 3 +++ pkg/config/flag_list.go | 26 ++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) create mode 100644 pkg/config/flag_list.go diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index d6cee8cfa80..2de520b6a7f 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -20,15 +20,17 @@ import ( "github.com/spf13/viper" + "github.com/jaegertracing/jaeger/pkg/config" "github.com/jaegertracing/jaeger/ports" ) const ( - queryPort = "query.port" - queryBasePath = "query.base-path" - queryStaticFiles = "query.static-files" - queryUIConfig = "query.ui-config" - queryTokenPropagation = "query.bearer-token-propagation" + queryPort = "query.port" + queryBasePath = "query.base-path" + queryStaticFiles = "query.static-files" + queryUIConfig = "query.ui-config" + queryTokenPropagation = "query.bearer-token-propagation" + queryAdditionalHeaders = "query.additional-headers" ) // QueryOptions holds configuration for query service @@ -43,16 +45,18 @@ type QueryOptions struct { UIConfig string // BearerTokenPropagation activate/deactivate bearer token propagation to storage BearerTokenPropagation bool + // AdditionalHeaders + AdditionalHeaders []string } // AddFlags adds flags for QueryOptions func AddFlags(flagSet *flag.FlagSet) { + flagSet.Var(&config.FlagList{}, queryAdditionalHeaders, "Additional HTTP response headers that will be returned with all responses") flagSet.Int(queryPort, ports.QueryHTTP, "The port for the query service") flagSet.String(queryBasePath, "/", "The base path for all HTTP routes, e.g. /jaeger; useful when running behind a reverse proxy") flagSet.String(queryStaticFiles, "", "The directory path override for the static assets for the UI") flagSet.String(queryUIConfig, "", "The path to the UI configuration file in JSON format") flagSet.Bool(queryTokenPropagation, false, "Allow propagation of bearer token to be used by storage plugins") - } // InitFromViper initializes QueryOptions with properties from viper @@ -62,5 +66,6 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper) *QueryOptions { qOpts.StaticAssets = v.GetString(queryStaticFiles) qOpts.UIConfig = v.GetString(queryUIConfig) qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation) + qOpts.AdditionalHeaders = v.GetStringSlice(queryAdditionalHeaders) return qOpts } diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index b12b08fef39..f3ba3660bfc 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -30,10 +30,13 @@ func TestQueryBuilderFlags(t *testing.T) { "--query.ui-config=some.json", "--query.base-path=/jaeger", "--query.port=80", + "--query.additional-headers=access-control-allow-origin=blerg", + "--query.additional-headers=whatever=thing", }) qOpts := new(QueryOptions).InitFromViper(v) assert.Equal(t, "/dev/null", qOpts.StaticAssets) assert.Equal(t, "some.json", qOpts.UIConfig) assert.Equal(t, "/jaeger", qOpts.BasePath) assert.Equal(t, 80, qOpts.Port) + assert.Equal(t, []string{"access-control-allow-origin=blerg", "whatever=thing"}, qOpts.AdditionalHeaders) } diff --git a/pkg/config/flag_list.go b/pkg/config/flag_list.go new file mode 100644 index 00000000000..cc677b86cc1 --- /dev/null +++ b/pkg/config/flag_list.go @@ -0,0 +1,26 @@ +package config + +import "strings" + +// FlagList implements the pflag.Value interface and allows for parsing multiple +// config values with the same name +// It purposefully mimics pFlag.stringSliceValue (https://github.com/spf13/pflag/blob/master/string_slice.go) +// in order to be treated like a string slice by both viper and pflag cleanly +type FlagList []string + +// String implements pflag.Value +func (l *FlagList) String() string { + return strings.Join(*l, ",") +} + +// Set implements pflag.Value +func (l *FlagList) Set(value string) error { + *l = append(*l, value) + return nil +} + +// Type implements pflag.Value +func (l *FlagList) Type() string { + // this type string needs to match pflag.stringSliceValue's Type + return "stringSlice" +} From a79dd093442bebe0f7253f8d3dea5c5ee7569e35 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Tue, 4 Feb 2020 15:04:11 -0500 Subject: [PATCH 02/22] Added tests to confirm FlagList will be treated as stringSlice by viper Signed-off-by: Joe Elliott --- pkg/config/flag_list.go | 2 +- pkg/config/flag_list_test.go | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 pkg/config/flag_list_test.go diff --git a/pkg/config/flag_list.go b/pkg/config/flag_list.go index cc677b86cc1..aea187f5e1d 100644 --- a/pkg/config/flag_list.go +++ b/pkg/config/flag_list.go @@ -10,7 +10,7 @@ type FlagList []string // String implements pflag.Value func (l *FlagList) String() string { - return strings.Join(*l, ",") + return "[" + strings.Join(*l, ",") + "]" } // Set implements pflag.Value diff --git a/pkg/config/flag_list_test.go b/pkg/config/flag_list_test.go new file mode 100644 index 00000000000..47f7e863367 --- /dev/null +++ b/pkg/config/flag_list_test.go @@ -0,0 +1,43 @@ +package config + +import ( + "flag" + "testing" + + "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" +) + +func TestFlagList(t *testing.T) { + f := &FlagList{} + + assert.Equal(t, f.String(), "[]") + assert.Equal(t, f.Type(), "stringSlice") + + f.Set("test") + assert.Equal(t, f.String(), "[test]") + + f.Set("test2") + assert.Equal(t, f.String(), "[test,test2]") +} + +func TestFlagListTreatedAsStringSlice(t *testing.T) { + f := &FlagList{} + + // create and add flags/values to a go flag set + flagset := flag.NewFlagSet("test", flag.ContinueOnError) + flagset.Var(f, "test", "test") + + err := flagset.Set("test", "asdf") + assert.NoError(t, err) + err = flagset.Set("test", "blerg") + assert.NoError(t, err) + + // add go flag set to pflag + pflagset := pflag.FlagSet{} + pflagset.AddGoFlagSet(flagset) + actual, err := pflagset.GetStringSlice("test") + assert.NoError(t, err) + + assert.Equal(t, []string{"asdf", "blerg"}, actual) +} From d5de7ed189dc3604df5780a6844efe119a0bc513 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Tue, 4 Feb 2020 15:33:23 -0500 Subject: [PATCH 03/22] Added additionalheaders logic. Added tests Signed-off-by: Joe Elliott --- cmd/query/app/handler_options.go | 7 ++++ cmd/query/app/http_handler.go | 20 +++++++---- cmd/query/app/http_handler_test.go | 53 +++++++++++++++++++++++------- 3 files changed, 61 insertions(+), 19 deletions(-) diff --git a/cmd/query/app/handler_options.go b/cmd/query/app/handler_options.go index 6f164156910..c1c9785a21b 100644 --- a/cmd/query/app/handler_options.go +++ b/cmd/query/app/handler_options.go @@ -65,3 +65,10 @@ func (handlerOptions) Tracer(tracer opentracing.Tracer) HandlerOption { apiHandler.tracer = tracer } } + +// AdditionalHeaders creates a HandlerOption that adds abitrary Response Headers +func (handlerOptions) AdditionalHeaders(additionalHeaders map[string]string) HandlerOption { + return func(apiHandler *APIHandler) { + apiHandler.additionalHeaders = additionalHeaders + } +} diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index b80c9203d70..77103b43df4 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -74,12 +74,13 @@ func NewRouter() *mux.Router { // APIHandler implements the query service public API by registering routes at httpPrefix type APIHandler struct { - queryService *querysvc.QueryService - queryParser queryParser - basePath string - apiPrefix string - logger *zap.Logger - tracer opentracing.Tracer + queryService *querysvc.QueryService + queryParser queryParser + basePath string + apiPrefix string + logger *zap.Logger + tracer opentracing.Tracer + additionalHeaders map[string]string } // NewAPIHandler returns an APIHandler @@ -450,6 +451,11 @@ func (aH *APIHandler) writeJSON(w http.ResponseWriter, r *http.Request, response } } resp, _ := marshall(response) - w.Header().Set("Content-Type", "application/json") + + header := w.Header() + header.Set("Content-Type", "application/json") + for h, v := range aH.additionalHeaders { + header.Set(h, v) + } w.Write(resp) } diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go index a38c8324eb8..af296e03a4c 100644 --- a/cmd/query/app/http_handler_test.go +++ b/cmd/query/app/http_handler_test.go @@ -562,6 +562,27 @@ func TestGetOperationsLegacyStorageFailure(t *testing.T) { assert.Error(t, err) } +func TestAdditionalHeaders(t *testing.T) { + additionalHeaders := map[string]string{ + "Access-Control-Allow-Origin": "https://mozilla.org", + "Access-Control-Expose-Headers": "X-My-Custom-Header,X-Another-Custom-Header", + } + server, readMock, _ := initializeTestServer(HandlerOptions.AdditionalHeaders(additionalHeaders)) + defer server.Close() + readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). + Return(mockTrace, nil).Once() + + req, err := http.NewRequest("GET", server.URL+`/api/traces/123456`, nil) + assert.NoError(t, err) + resp, err := execJSONHTTPResponse(req) + assert.NoError(t, err) + resp.Body.Close() + + for k, v := range additionalHeaders { + assert.Equal(t, v, resp.Header.Get(k)) + } +} + // getJSON fetches a JSON document from a server via HTTP GET func getJSON(url string, out interface{}) error { req, err := http.NewRequest("GET", url, nil) @@ -587,23 +608,12 @@ func postJSON(url string, req interface{}, out interface{}) error { // execJSON executes an http request against a server and parses response as JSON func execJSON(req *http.Request, out interface{}) error { - req.Header.Add("Accept", "application/json") - - resp, err := httpClient.Do(req) + resp, err := execJSONHTTPResponse(req) if err != nil { return err } - defer resp.Body.Close() - if resp.StatusCode > 399 { - body, err := ioutil.ReadAll(resp.Body) - if err != nil { - return err - } - return fmt.Errorf("%d error from server: %s", resp.StatusCode, body) - } - if out == nil { io.Copy(ioutil.Discard, resp.Body) return nil @@ -613,6 +623,25 @@ func execJSON(req *http.Request, out interface{}) error { return decoder.Decode(out) } +func execJSONHTTPResponse(req *http.Request) (*http.Response, error) { + req.Header.Add("Accept", "application/json") + + resp, err := httpClient.Do(req) + if err != nil { + return nil, err + } + + if resp.StatusCode > 399 { + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + return nil, err + } + return nil, fmt.Errorf("%d error from server: %s", resp.StatusCode, body) + } + + return resp, nil +} + // Generates a JSON response that the server should produce given a certain error code and error. func parsedError(code int, err string) string { return fmt.Sprintf(`%d error from server: {"data":null,"total":0,"limit":0,"offset":0,"errors":[{"code":%d,"msg":"%s"}]}`+"\n", code, code, err) From f17ecd241fbe487c1bacb167e017308437330ccc Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 09:19:41 -0500 Subject: [PATCH 04/22] Changed http handler to take a header struct Signed-off-by: Joe Elliott --- cmd/query/app/handler_options.go | 3 ++- cmd/query/app/http_handler.go | 6 +++--- cmd/query/app/http_handler_test.go | 11 ++++++----- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cmd/query/app/handler_options.go b/cmd/query/app/handler_options.go index c1c9785a21b..fc074acc020 100644 --- a/cmd/query/app/handler_options.go +++ b/cmd/query/app/handler_options.go @@ -16,6 +16,7 @@ package app import ( + "net/http" "time" "github.com/opentracing/opentracing-go" @@ -67,7 +68,7 @@ func (handlerOptions) Tracer(tracer opentracing.Tracer) HandlerOption { } // AdditionalHeaders creates a HandlerOption that adds abitrary Response Headers -func (handlerOptions) AdditionalHeaders(additionalHeaders map[string]string) HandlerOption { +func (handlerOptions) AdditionalHeaders(additionalHeaders http.Header) HandlerOption { return func(apiHandler *APIHandler) { apiHandler.additionalHeaders = additionalHeaders } diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index 77103b43df4..8ab2ec0c3aa 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -80,7 +80,7 @@ type APIHandler struct { apiPrefix string logger *zap.Logger tracer opentracing.Tracer - additionalHeaders map[string]string + additionalHeaders http.Header } // NewAPIHandler returns an APIHandler @@ -454,8 +454,8 @@ func (aH *APIHandler) writeJSON(w http.ResponseWriter, r *http.Request, response header := w.Header() header.Set("Content-Type", "application/json") - for h, v := range aH.additionalHeaders { - header.Set(h, v) + for key, values := range aH.additionalHeaders { + header[key] = values } w.Write(resp) } diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go index af296e03a4c..633ece2a32f 100644 --- a/cmd/query/app/http_handler_test.go +++ b/cmd/query/app/http_handler_test.go @@ -563,10 +563,11 @@ func TestGetOperationsLegacyStorageFailure(t *testing.T) { } func TestAdditionalHeaders(t *testing.T) { - additionalHeaders := map[string]string{ - "Access-Control-Allow-Origin": "https://mozilla.org", - "Access-Control-Expose-Headers": "X-My-Custom-Header,X-Another-Custom-Header", - } + additionalHeaders := http.Header{} + additionalHeaders.Add("Access-Control-Allow-Origin", "https://mozilla.org") + additionalHeaders.Add("Access-Control-Expose-Headers", "X-My-Custom-Header") + additionalHeaders.Add("Access-Control-Expose-Headers", "X-Another-Custom-Header") + additionalHeaders.Add("Access-Control-Request-Headers", "field1, field2") server, readMock, _ := initializeTestServer(HandlerOptions.AdditionalHeaders(additionalHeaders)) defer server.Close() readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). @@ -579,7 +580,7 @@ func TestAdditionalHeaders(t *testing.T) { resp.Body.Close() for k, v := range additionalHeaders { - assert.Equal(t, v, resp.Header.Get(k)) + assert.Equal(t, v, resp.Header[k]) } } From dfd486175964ddc5e6da496b37af1db14862ec1d Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 09:28:09 -0500 Subject: [PATCH 05/22] Updated help text on param Signed-off-by: Joe Elliott --- cmd/query/app/flags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index 2de520b6a7f..90131cc12e4 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -51,7 +51,7 @@ type QueryOptions struct { // AddFlags adds flags for QueryOptions func AddFlags(flagSet *flag.FlagSet) { - flagSet.Var(&config.FlagList{}, queryAdditionalHeaders, "Additional HTTP response headers that will be returned with all responses") + flagSet.Var(&config.FlagList{}, queryAdditionalHeaders, `Additional HTTP response headers. Can be specified multiple times. Format: "Key: Value"`) flagSet.Int(queryPort, ports.QueryHTTP, "The port for the query service") flagSet.String(queryBasePath, "/", "The base path for all HTTP routes, e.g. /jaeger; useful when running behind a reverse proxy") flagSet.String(queryStaticFiles, "", "The directory path override for the static assets for the UI") From 26cdad5b54e02491e45dc3402050a91e261f25cd Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 09:29:33 -0500 Subject: [PATCH 06/22] FlagList => StringSlice Signed-off-by: Joe Elliott --- cmd/query/app/flags.go | 2 +- pkg/config/{flag_list.go => string_slice.go} | 10 +++++----- pkg/config/{flag_list_test.go => string_slice_test.go} | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) rename pkg/config/{flag_list.go => string_slice.go} (69%) rename pkg/config/{flag_list_test.go => string_slice_test.go} (85%) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index 90131cc12e4..274df3f83f2 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -51,7 +51,7 @@ type QueryOptions struct { // AddFlags adds flags for QueryOptions func AddFlags(flagSet *flag.FlagSet) { - flagSet.Var(&config.FlagList{}, queryAdditionalHeaders, `Additional HTTP response headers. Can be specified multiple times. Format: "Key: Value"`) + flagSet.Var(&config.StringSlice{}, queryAdditionalHeaders, `Additional HTTP response headers. Can be specified multiple times. Format: "Key: Value"`) flagSet.Int(queryPort, ports.QueryHTTP, "The port for the query service") flagSet.String(queryBasePath, "/", "The base path for all HTTP routes, e.g. /jaeger; useful when running behind a reverse proxy") flagSet.String(queryStaticFiles, "", "The directory path override for the static assets for the UI") diff --git a/pkg/config/flag_list.go b/pkg/config/string_slice.go similarity index 69% rename from pkg/config/flag_list.go rename to pkg/config/string_slice.go index aea187f5e1d..e39a38484fd 100644 --- a/pkg/config/flag_list.go +++ b/pkg/config/string_slice.go @@ -2,25 +2,25 @@ package config import "strings" -// FlagList implements the pflag.Value interface and allows for parsing multiple +// StringSlice implements the pflag.Value interface and allows for parsing multiple // config values with the same name // It purposefully mimics pFlag.stringSliceValue (https://github.com/spf13/pflag/blob/master/string_slice.go) // in order to be treated like a string slice by both viper and pflag cleanly -type FlagList []string +type StringSlice []string // String implements pflag.Value -func (l *FlagList) String() string { +func (l *StringSlice) String() string { return "[" + strings.Join(*l, ",") + "]" } // Set implements pflag.Value -func (l *FlagList) Set(value string) error { +func (l *StringSlice) Set(value string) error { *l = append(*l, value) return nil } // Type implements pflag.Value -func (l *FlagList) Type() string { +func (l *StringSlice) Type() string { // this type string needs to match pflag.stringSliceValue's Type return "stringSlice" } diff --git a/pkg/config/flag_list_test.go b/pkg/config/string_slice_test.go similarity index 85% rename from pkg/config/flag_list_test.go rename to pkg/config/string_slice_test.go index 47f7e863367..67564a39ffe 100644 --- a/pkg/config/flag_list_test.go +++ b/pkg/config/string_slice_test.go @@ -8,8 +8,8 @@ import ( "github.com/stretchr/testify/assert" ) -func TestFlagList(t *testing.T) { - f := &FlagList{} +func TestStringSlice(t *testing.T) { + f := &StringSlice{} assert.Equal(t, f.String(), "[]") assert.Equal(t, f.Type(), "stringSlice") @@ -21,8 +21,8 @@ func TestFlagList(t *testing.T) { assert.Equal(t, f.String(), "[test,test2]") } -func TestFlagListTreatedAsStringSlice(t *testing.T) { - f := &FlagList{} +func TestStringSliceTreatedAsStringSlice(t *testing.T) { + f := &StringSlice{} // create and add flags/values to a go flag set flagset := flag.NewFlagSet("test", flag.ContinueOnError) From c34f27c823634a69885900bdb1912bb8a70604c8 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 09:34:08 -0500 Subject: [PATCH 07/22] Additional tests to further specify string slice behavior Signed-off-by: Joe Elliott --- cmd/query/app/flags.go | 11 +++++++++-- cmd/query/app/server.go | 1 + pkg/config/string_slice_test.go | 7 ++++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index 274df3f83f2..91fc4110081 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -17,6 +17,7 @@ package app import ( "flag" + "net/http" "github.com/spf13/viper" @@ -46,7 +47,7 @@ type QueryOptions struct { // BearerTokenPropagation activate/deactivate bearer token propagation to storage BearerTokenPropagation bool // AdditionalHeaders - AdditionalHeaders []string + AdditionalHeaders http.Header } // AddFlags adds flags for QueryOptions @@ -66,6 +67,12 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper) *QueryOptions { qOpts.StaticAssets = v.GetString(queryStaticFiles) qOpts.UIConfig = v.GetString(queryUIConfig) qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation) - qOpts.AdditionalHeaders = v.GetStringSlice(queryAdditionalHeaders) + qOpts.AdditionalHeaders = stringSliceAsHeader(v.GetStringSlice(queryAdditionalHeaders)) return qOpts } + +// stringSliceAsHeader parses a string slice and returns a header. each line +// +func stringSliceAsHeader(s []string) http.Header { + +} diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 4eda7d95597..a35266278ff 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -70,6 +70,7 @@ func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, apiHandlerOptions := []HandlerOption{ HandlerOptions.Logger(logger), HandlerOptions.Tracer(tracer), + HandlerOptions.AdditionalHeaders(queryOpts.AdditionalHeaders) } apiHandler := NewAPIHandler( querySvc, diff --git a/pkg/config/string_slice_test.go b/pkg/config/string_slice_test.go index 67564a39ffe..8b609f057e7 100644 --- a/pkg/config/string_slice_test.go +++ b/pkg/config/string_slice_test.go @@ -19,6 +19,9 @@ func TestStringSlice(t *testing.T) { f.Set("test2") assert.Equal(t, f.String(), "[test,test2]") + + f.Set("test3,test4") + assert.Equal(t, f.String(), "[test,test2,test3,test4]") } func TestStringSliceTreatedAsStringSlice(t *testing.T) { @@ -32,6 +35,8 @@ func TestStringSliceTreatedAsStringSlice(t *testing.T) { assert.NoError(t, err) err = flagset.Set("test", "blerg") assert.NoError(t, err) + err = flagset.Set("test", "other,thing") + assert.NoError(t, err) // add go flag set to pflag pflagset := pflag.FlagSet{} @@ -39,5 +44,5 @@ func TestStringSliceTreatedAsStringSlice(t *testing.T) { actual, err := pflagset.GetStringSlice("test") assert.NoError(t, err) - assert.Equal(t, []string{"asdf", "blerg"}, actual) + assert.Equal(t, []string{"asdf", "blerg", "other", "thing"}, actual) } From 185ac596aa610d9fa497b8fc9e25dd6753657878 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 11:10:59 -0500 Subject: [PATCH 08/22] Added header parsing method and tests Signed-off-by: Joe Elliott --- cmd/query/app/flags.go | 11 ++--------- cmd/query/app/server.go | 27 ++++++++++++++++++++++++++- cmd/query/app/server_test.go | 16 ++++++++++++++++ 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index 91fc4110081..274df3f83f2 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -17,7 +17,6 @@ package app import ( "flag" - "net/http" "github.com/spf13/viper" @@ -47,7 +46,7 @@ type QueryOptions struct { // BearerTokenPropagation activate/deactivate bearer token propagation to storage BearerTokenPropagation bool // AdditionalHeaders - AdditionalHeaders http.Header + AdditionalHeaders []string } // AddFlags adds flags for QueryOptions @@ -67,12 +66,6 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper) *QueryOptions { qOpts.StaticAssets = v.GetString(queryStaticFiles) qOpts.UIConfig = v.GetString(queryUIConfig) qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation) - qOpts.AdditionalHeaders = stringSliceAsHeader(v.GetStringSlice(queryAdditionalHeaders)) + qOpts.AdditionalHeaders = v.GetStringSlice(queryAdditionalHeaders) return qOpts } - -// stringSliceAsHeader parses a string slice and returns a header. each line -// -func stringSliceAsHeader(s []string) http.Header { - -} diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index a35266278ff..4711445ae9d 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -15,9 +15,12 @@ package app import ( + "bufio" "fmt" + "io" "net" "net/http" + "net/textproto" "strings" "github.com/gorilla/handlers" @@ -67,10 +70,11 @@ func createGRPCServer(querySvc *querysvc.QueryService, logger *zap.Logger, trace } func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) *http.Server { + headers := stringSliceAsHeader(queryOpts.AdditionalHeaders, logger) apiHandlerOptions := []HandlerOption{ HandlerOptions.Logger(logger), HandlerOptions.Tracer(tracer), - HandlerOptions.AdditionalHeaders(queryOpts.AdditionalHeaders) + HandlerOptions.AdditionalHeaders(headers), } apiHandler := NewAPIHandler( querySvc, @@ -162,3 +166,24 @@ func (s *Server) Close() { s.httpServer.Close() s.conn.Close() } + +// stringSliceAsHeader parses a slice of strings and returns a http.Header. +// Each string in the slice is expected to be in the format "key: value" +func stringSliceAsHeader(slice []string, logger *zap.Logger) http.Header { + if len(slice) == 0 { + return nil + } + + allHeaders := strings.Join(slice, "\r\n") + + reader := bufio.NewReader(strings.NewReader(allHeaders)) + tp := textproto.NewReader(reader) + + header, err := tp.ReadMIMEHeader() + if err != nil && err != io.EOF { + logger.Error("Failed to parse headers", zap.Strings("headers", slice)) + return nil + } + + return http.Header(header) +} diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 2afa1460d52..09d0e1d3911 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -122,3 +122,19 @@ func TestServerHandlesPortZero(t *testing.T) { port := onlyEntry.ContextMap()["port"].(int64) assert.Greater(t, port, int64(0)) } + +func TestStringSliceAsHeader(t *testing.T) { + headers := []string{"Access-Control-Allow-Origin: https://mozilla.org", + "Access-Control-Expose-Headers: X-My-Custom-Header", + "Access-Control-Expose-Headers: X-Another-Custom-Header", + } + + parsedHeaders := stringSliceAsHeader(headers, zap.NewNop()) + + assert.Equal(t, []string{"https://mozilla.org"}, parsedHeaders["Access-Control-Allow-Origin"]) + assert.Equal(t, []string{"X-My-Custom-Header", "X-Another-Custom-Header"}, parsedHeaders["Access-Control-Expose-Headers"]) + + malformedHeaders := append(headers, "this is not a valid header") + parsedHeaders = stringSliceAsHeader(malformedHeaders, zap.NewNop()) + assert.Nil(t, parsedHeaders) +} From 7ebaba8dd0bc8456ac7be0ee31cebac88acf6698 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 11:59:05 -0500 Subject: [PATCH 09/22] Added support for commas in params Signed-off-by: Joe Elliott --- pkg/config/string_slice.go | 20 +++++++++++++++++++- pkg/config/string_slice_test.go | 26 ++++++++++++++++++++------ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/pkg/config/string_slice.go b/pkg/config/string_slice.go index e39a38484fd..f96860be09c 100644 --- a/pkg/config/string_slice.go +++ b/pkg/config/string_slice.go @@ -1,3 +1,17 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package config import "strings" @@ -10,7 +24,11 @@ type StringSlice []string // String implements pflag.Value func (l *StringSlice) String() string { - return "[" + strings.Join(*l, ",") + "]" + if len(*l) == 0 { + return "[]" + } + + return `["` + strings.Join(*l, `","`) + `"]` } // Set implements pflag.Value diff --git a/pkg/config/string_slice_test.go b/pkg/config/string_slice_test.go index 8b609f057e7..55c08d2d925 100644 --- a/pkg/config/string_slice_test.go +++ b/pkg/config/string_slice_test.go @@ -1,3 +1,17 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package config import ( @@ -11,17 +25,17 @@ import ( func TestStringSlice(t *testing.T) { f := &StringSlice{} - assert.Equal(t, f.String(), "[]") - assert.Equal(t, f.Type(), "stringSlice") + assert.Equal(t, "[]", f.String()) + assert.Equal(t, "stringSlice", f.Type()) f.Set("test") - assert.Equal(t, f.String(), "[test]") + assert.Equal(t, `["test"]`, f.String()) f.Set("test2") - assert.Equal(t, f.String(), "[test,test2]") + assert.Equal(t, `["test","test2"]`, f.String()) f.Set("test3,test4") - assert.Equal(t, f.String(), "[test,test2,test3,test4]") + assert.Equal(t, `["test","test2","test3,test4"]`, f.String()) } func TestStringSliceTreatedAsStringSlice(t *testing.T) { @@ -44,5 +58,5 @@ func TestStringSliceTreatedAsStringSlice(t *testing.T) { actual, err := pflagset.GetStringSlice("test") assert.NoError(t, err) - assert.Equal(t, []string{"asdf", "blerg", "other", "thing"}, actual) + assert.Equal(t, []string{"asdf", "blerg", "other,thing"}, actual) } From 4f02500fdb1070e805422b23822dcf8a3880a481 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 13:19:50 -0500 Subject: [PATCH 10/22] Updated flags to use expected format Signed-off-by: Joe Elliott --- cmd/query/app/flags_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index f3ba3660bfc..b0edbf5eb3a 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -30,13 +30,13 @@ func TestQueryBuilderFlags(t *testing.T) { "--query.ui-config=some.json", "--query.base-path=/jaeger", "--query.port=80", - "--query.additional-headers=access-control-allow-origin=blerg", - "--query.additional-headers=whatever=thing", + "--query.additional-headers=access-control-allow-origin:blerg", + "--query.additional-headers=whatever:thing", }) qOpts := new(QueryOptions).InitFromViper(v) assert.Equal(t, "/dev/null", qOpts.StaticAssets) assert.Equal(t, "some.json", qOpts.UIConfig) assert.Equal(t, "/jaeger", qOpts.BasePath) assert.Equal(t, 80, qOpts.Port) - assert.Equal(t, []string{"access-control-allow-origin=blerg", "whatever=thing"}, qOpts.AdditionalHeaders) + assert.Equal(t, []string{"access-control-allow-origin:blerg", "whatever:thing"}, qOpts.AdditionalHeaders) } From c0e28d39b4f42f15c3d318139062d4202c36ee3d Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 13:42:13 -0500 Subject: [PATCH 11/22] Moved stringSliceAsHeader() to flags Signed-off-by: Joe Elliott --- cmd/all-in-one/main.go | 2 +- cmd/query/app/flags.go | 33 ++++++++++++++++++++++++++++++--- cmd/query/app/flags_test.go | 22 ++++++++++++++++++++-- cmd/query/app/server.go | 27 +-------------------------- cmd/query/app/server_test.go | 16 ---------------- cmd/query/main.go | 2 +- 6 files changed, 53 insertions(+), 49 deletions(-) diff --git a/cmd/all-in-one/main.go b/cmd/all-in-one/main.go index eba1b7afe84..e159f159af4 100644 --- a/cmd/all-in-one/main.go +++ b/cmd/all-in-one/main.go @@ -129,7 +129,7 @@ by default uses only in-memory database.`, tchanBuilder := agentTchanRep.NewBuilder().InitFromViper(v, logger) grpcBuilder := agentGrpcRep.NewConnBuilder().InitFromViper(v) cOpts := new(collector.CollectorOptions).InitFromViper(v) - qOpts := new(queryApp.QueryOptions).InitFromViper(v) + qOpts := new(queryApp.QueryOptions).InitFromViper(v, logger) collectorSrv := startCollector(cOpts, spanWriter, logger, metricsFactory, strategyStore, svc.HC()) startAgent(aOpts, repOpts, tchanBuilder, grpcBuilder, cOpts, logger, metricsFactory) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index 274df3f83f2..65907e8f4cb 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -16,9 +16,15 @@ package app import ( + "bufio" "flag" + "io" + "net/http" + "net/textproto" + "strings" "github.com/spf13/viper" + "go.uber.org/zap" "github.com/jaegertracing/jaeger/pkg/config" "github.com/jaegertracing/jaeger/ports" @@ -46,7 +52,7 @@ type QueryOptions struct { // BearerTokenPropagation activate/deactivate bearer token propagation to storage BearerTokenPropagation bool // AdditionalHeaders - AdditionalHeaders []string + AdditionalHeaders http.Header } // AddFlags adds flags for QueryOptions @@ -60,12 +66,33 @@ func AddFlags(flagSet *flag.FlagSet) { } // InitFromViper initializes QueryOptions with properties from viper -func (qOpts *QueryOptions) InitFromViper(v *viper.Viper) *QueryOptions { +func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *QueryOptions { qOpts.Port = v.GetInt(queryPort) qOpts.BasePath = v.GetString(queryBasePath) qOpts.StaticAssets = v.GetString(queryStaticFiles) qOpts.UIConfig = v.GetString(queryUIConfig) qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation) - qOpts.AdditionalHeaders = v.GetStringSlice(queryAdditionalHeaders) + qOpts.AdditionalHeaders = stringSliceAsHeader(v.GetStringSlice(queryAdditionalHeaders), logger) return qOpts } + +// stringSliceAsHeader parses a slice of strings and returns a http.Header. +// Each string in the slice is expected to be in the format "key: value" +func stringSliceAsHeader(slice []string, logger *zap.Logger) http.Header { + if len(slice) == 0 { + return nil + } + + allHeaders := strings.Join(slice, "\r\n") + + reader := bufio.NewReader(strings.NewReader(allHeaders)) + tp := textproto.NewReader(reader) + + header, err := tp.ReadMIMEHeader() + if err != nil && err != io.EOF { + logger.Error("Failed to parse headers", zap.Strings("headers", slice)) + return nil + } + + return http.Header(header) +} diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index b0edbf5eb3a..2cc35941c61 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -16,9 +16,11 @@ package app import ( + "net/http" "testing" "github.com/stretchr/testify/assert" + "go.uber.org/zap" "github.com/jaegertracing/jaeger/pkg/config" ) @@ -33,10 +35,26 @@ func TestQueryBuilderFlags(t *testing.T) { "--query.additional-headers=access-control-allow-origin:blerg", "--query.additional-headers=whatever:thing", }) - qOpts := new(QueryOptions).InitFromViper(v) + qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop()) assert.Equal(t, "/dev/null", qOpts.StaticAssets) assert.Equal(t, "some.json", qOpts.UIConfig) assert.Equal(t, "/jaeger", qOpts.BasePath) assert.Equal(t, 80, qOpts.Port) - assert.Equal(t, []string{"access-control-allow-origin:blerg", "whatever:thing"}, qOpts.AdditionalHeaders) + assert.Equal(t, http.Header{"Access-Control-Allow-Origin": []string{"blerg"}, "Whatever": []string{"thing"}}, qOpts.AdditionalHeaders) +} + +func TestStringSliceAsHeader(t *testing.T) { + headers := []string{"Access-Control-Allow-Origin: https://mozilla.org", + "Access-Control-Expose-Headers: X-My-Custom-Header", + "Access-Control-Expose-Headers: X-Another-Custom-Header", + } + + parsedHeaders := stringSliceAsHeader(headers, zap.NewNop()) + + assert.Equal(t, []string{"https://mozilla.org"}, parsedHeaders["Access-Control-Allow-Origin"]) + assert.Equal(t, []string{"X-My-Custom-Header", "X-Another-Custom-Header"}, parsedHeaders["Access-Control-Expose-Headers"]) + + malformedHeaders := append(headers, "this is not a valid header") + parsedHeaders = stringSliceAsHeader(malformedHeaders, zap.NewNop()) + assert.Nil(t, parsedHeaders) } diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 4711445ae9d..21b530a4502 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -15,12 +15,9 @@ package app import ( - "bufio" "fmt" - "io" "net" "net/http" - "net/textproto" "strings" "github.com/gorilla/handlers" @@ -70,11 +67,10 @@ func createGRPCServer(querySvc *querysvc.QueryService, logger *zap.Logger, trace } func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) *http.Server { - headers := stringSliceAsHeader(queryOpts.AdditionalHeaders, logger) apiHandlerOptions := []HandlerOption{ HandlerOptions.Logger(logger), HandlerOptions.Tracer(tracer), - HandlerOptions.AdditionalHeaders(headers), + HandlerOptions.AdditionalHeaders(queryOpts.AdditionalHeaders), } apiHandler := NewAPIHandler( querySvc, @@ -166,24 +162,3 @@ func (s *Server) Close() { s.httpServer.Close() s.conn.Close() } - -// stringSliceAsHeader parses a slice of strings and returns a http.Header. -// Each string in the slice is expected to be in the format "key: value" -func stringSliceAsHeader(slice []string, logger *zap.Logger) http.Header { - if len(slice) == 0 { - return nil - } - - allHeaders := strings.Join(slice, "\r\n") - - reader := bufio.NewReader(strings.NewReader(allHeaders)) - tp := textproto.NewReader(reader) - - header, err := tp.ReadMIMEHeader() - if err != nil && err != io.EOF { - logger.Error("Failed to parse headers", zap.Strings("headers", slice)) - return nil - } - - return http.Header(header) -} diff --git a/cmd/query/app/server_test.go b/cmd/query/app/server_test.go index 09d0e1d3911..2afa1460d52 100644 --- a/cmd/query/app/server_test.go +++ b/cmd/query/app/server_test.go @@ -122,19 +122,3 @@ func TestServerHandlesPortZero(t *testing.T) { port := onlyEntry.ContextMap()["port"].(int64) assert.Greater(t, port, int64(0)) } - -func TestStringSliceAsHeader(t *testing.T) { - headers := []string{"Access-Control-Allow-Origin: https://mozilla.org", - "Access-Control-Expose-Headers: X-My-Custom-Header", - "Access-Control-Expose-Headers: X-Another-Custom-Header", - } - - parsedHeaders := stringSliceAsHeader(headers, zap.NewNop()) - - assert.Equal(t, []string{"https://mozilla.org"}, parsedHeaders["Access-Control-Allow-Origin"]) - assert.Equal(t, []string{"X-My-Custom-Header", "X-Another-Custom-Header"}, parsedHeaders["Access-Control-Expose-Headers"]) - - malformedHeaders := append(headers, "this is not a valid header") - parsedHeaders = stringSliceAsHeader(malformedHeaders, zap.NewNop()) - assert.Nil(t, parsedHeaders) -} diff --git a/cmd/query/main.go b/cmd/query/main.go index 41162d8b646..d14d4f2626b 100644 --- a/cmd/query/main.go +++ b/cmd/query/main.go @@ -85,7 +85,7 @@ func main() { } defer closer.Close() opentracing.SetGlobalTracer(tracer) - queryOpts := new(app.QueryOptions).InitFromViper(v) + queryOpts := new(app.QueryOptions).InitFromViper(v, logger) // TODO: Need to figure out set enable/disable propagation on storage plugins. v.Set(spanstore.StoragePropagationKey, queryOpts.BearerTokenPropagation) storageFactory.InitFromViper(v) From 2d9f75bd48839da8acb8b836d9ba3ca624781e42 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Wed, 5 Feb 2020 14:26:20 -0500 Subject: [PATCH 12/22] Added test for empty string slice Signed-off-by: Joe Elliott --- cmd/query/app/flags_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index 2cc35941c61..c417ec28ab1 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -57,4 +57,7 @@ func TestStringSliceAsHeader(t *testing.T) { malformedHeaders := append(headers, "this is not a valid header") parsedHeaders = stringSliceAsHeader(malformedHeaders, zap.NewNop()) assert.Nil(t, parsedHeaders) + + parsedHeaders = stringSliceAsHeader([]string{}, zap.NewNop()) + assert.Nil(t, parsedHeaders) } From 4e19e401ea0560beaca54441c79b235b0dce2a1d Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Fri, 7 Feb 2020 16:18:34 -0500 Subject: [PATCH 13/22] Moved additional headers to server handler Signed-off-by: Joe Elliott --- cmd/query/app/handler_options.go | 8 -------- cmd/query/app/http_handler.go | 16 ++++++---------- cmd/query/app/http_handler_test.go | 22 ---------------------- cmd/query/app/server.go | 2 +- 4 files changed, 7 insertions(+), 41 deletions(-) diff --git a/cmd/query/app/handler_options.go b/cmd/query/app/handler_options.go index fc074acc020..6f164156910 100644 --- a/cmd/query/app/handler_options.go +++ b/cmd/query/app/handler_options.go @@ -16,7 +16,6 @@ package app import ( - "net/http" "time" "github.com/opentracing/opentracing-go" @@ -66,10 +65,3 @@ func (handlerOptions) Tracer(tracer opentracing.Tracer) HandlerOption { apiHandler.tracer = tracer } } - -// AdditionalHeaders creates a HandlerOption that adds abitrary Response Headers -func (handlerOptions) AdditionalHeaders(additionalHeaders http.Header) HandlerOption { - return func(apiHandler *APIHandler) { - apiHandler.additionalHeaders = additionalHeaders - } -} diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index 8ab2ec0c3aa..ee3508ead73 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -74,13 +74,12 @@ func NewRouter() *mux.Router { // APIHandler implements the query service public API by registering routes at httpPrefix type APIHandler struct { - queryService *querysvc.QueryService - queryParser queryParser - basePath string - apiPrefix string - logger *zap.Logger - tracer opentracing.Tracer - additionalHeaders http.Header + queryService *querysvc.QueryService + queryParser queryParser + basePath string + apiPrefix string + logger *zap.Logger + tracer opentracing.Tracer } // NewAPIHandler returns an APIHandler @@ -454,8 +453,5 @@ func (aH *APIHandler) writeJSON(w http.ResponseWriter, r *http.Request, response header := w.Header() header.Set("Content-Type", "application/json") - for key, values := range aH.additionalHeaders { - header[key] = values - } w.Write(resp) } diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go index 633ece2a32f..4375a051691 100644 --- a/cmd/query/app/http_handler_test.go +++ b/cmd/query/app/http_handler_test.go @@ -562,28 +562,6 @@ func TestGetOperationsLegacyStorageFailure(t *testing.T) { assert.Error(t, err) } -func TestAdditionalHeaders(t *testing.T) { - additionalHeaders := http.Header{} - additionalHeaders.Add("Access-Control-Allow-Origin", "https://mozilla.org") - additionalHeaders.Add("Access-Control-Expose-Headers", "X-My-Custom-Header") - additionalHeaders.Add("Access-Control-Expose-Headers", "X-Another-Custom-Header") - additionalHeaders.Add("Access-Control-Request-Headers", "field1, field2") - server, readMock, _ := initializeTestServer(HandlerOptions.AdditionalHeaders(additionalHeaders)) - defer server.Close() - readMock.On("GetTrace", mock.AnythingOfType("*context.valueCtx"), mock.AnythingOfType("model.TraceID")). - Return(mockTrace, nil).Once() - - req, err := http.NewRequest("GET", server.URL+`/api/traces/123456`, nil) - assert.NoError(t, err) - resp, err := execJSONHTTPResponse(req) - assert.NoError(t, err) - resp.Body.Close() - - for k, v := range additionalHeaders { - assert.Equal(t, v, resp.Header[k]) - } -} - // getJSON fetches a JSON document from a server via HTTP GET func getJSON(url string, out interface{}) error { req, err := http.NewRequest("GET", url, nil) diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index 21b530a4502..f7ce108b2d3 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -70,7 +70,6 @@ func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, apiHandlerOptions := []HandlerOption{ HandlerOptions.Logger(logger), HandlerOptions.Tracer(tracer), - HandlerOptions.AdditionalHeaders(queryOpts.AdditionalHeaders), } apiHandler := NewAPIHandler( querySvc, @@ -87,6 +86,7 @@ func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, handler = bearerTokenPropagationHandler(logger, r) } handler = handlers.CompressHandler(handler) + handler = additionalHeadersHandler(handler, queryOpts.AdditionalHeaders) recoveryHandler := recoveryhandler.NewRecoveryHandler(logger, true) return &http.Server{ Handler: recoveryHandler(handler), From 92224514866749088ef5aa438e013f87fe665258 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Fri, 7 Feb 2020 16:53:55 -0500 Subject: [PATCH 14/22] Added handler and tests Signed-off-by: Joe Elliott --- cmd/query/app/additional_headers_handler.go | 30 +++++++++++++ cmd/query/app/additional_headers_test.go | 50 +++++++++++++++++++++ cmd/query/app/server.go | 4 +- 3 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 cmd/query/app/additional_headers_handler.go create mode 100644 cmd/query/app/additional_headers_test.go diff --git a/cmd/query/app/additional_headers_handler.go b/cmd/query/app/additional_headers_handler.go new file mode 100644 index 00000000000..de423921696 --- /dev/null +++ b/cmd/query/app/additional_headers_handler.go @@ -0,0 +1,30 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package app + +import ( + "net/http" +) + +func additionalHeadersHandler(h http.Handler, additionalHeaders http.Header) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + header := w.Header() + for key, values := range additionalHeaders { + header[key] = values + } + + h.ServeHTTP(w, r) + }) +} diff --git a/cmd/query/app/additional_headers_test.go b/cmd/query/app/additional_headers_test.go new file mode 100644 index 00000000000..e7dc7328457 --- /dev/null +++ b/cmd/query/app/additional_headers_test.go @@ -0,0 +1,50 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package app + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAdditionalHeadersHandler(t *testing.T) { + + additionalHeaders := http.Header{} + additionalHeaders.Add("Access-Control-Allow-Origin", "https://mozilla.org") + additionalHeaders.Add("Access-Control-Expose-Headers", "X-My-Custom-Header") + additionalHeaders.Add("Access-Control-Expose-Headers", "X-Another-Custom-Header") + additionalHeaders.Add("Access-Control-Request-Headers", "field1, field2") + + emptyHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte{}) + }) + + handler := additionalHeadersHandler(emptyHandler, additionalHeaders) + server := httptest.NewServer(handler) + defer server.Close() + + req, err := http.NewRequest("GET", server.URL, nil) + assert.NoError(t, err) + + resp, err := httpClient.Do(req) + assert.NoError(t, err) + + for k, v := range additionalHeaders { + assert.Equal(t, v, resp.Header[k]) + } +} diff --git a/cmd/query/app/server.go b/cmd/query/app/server.go index f7ce108b2d3..a8c40300145 100644 --- a/cmd/query/app/server.go +++ b/cmd/query/app/server.go @@ -82,11 +82,11 @@ func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, apiHandler.RegisterRoutes(r) RegisterStaticHandler(r, logger, queryOpts) var handler http.Handler = r + handler = additionalHeadersHandler(handler, queryOpts.AdditionalHeaders) if queryOpts.BearerTokenPropagation { - handler = bearerTokenPropagationHandler(logger, r) + handler = bearerTokenPropagationHandler(logger, handler) } handler = handlers.CompressHandler(handler) - handler = additionalHeadersHandler(handler, queryOpts.AdditionalHeaders) recoveryHandler := recoveryhandler.NewRecoveryHandler(logger, true) return &http.Server{ Handler: recoveryHandler(handler), From ac5f961d94854e15be8861f75fecfc7812c2dbc6 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Fri, 7 Feb 2020 21:51:55 -0500 Subject: [PATCH 15/22] removed unnecessary whitespace Signed-off-by: Joe Elliott --- cmd/query/app/additional_headers_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/query/app/additional_headers_test.go b/cmd/query/app/additional_headers_test.go index e7dc7328457..ede05c7a3a4 100644 --- a/cmd/query/app/additional_headers_test.go +++ b/cmd/query/app/additional_headers_test.go @@ -23,7 +23,6 @@ import ( ) func TestAdditionalHeadersHandler(t *testing.T) { - additionalHeaders := http.Header{} additionalHeaders.Add("Access-Control-Allow-Origin", "https://mozilla.org") additionalHeaders.Add("Access-Control-Expose-Headers", "X-My-Custom-Header") From f7fefe643328a203d32e3bcd27899d7ca768cefe Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Fri, 7 Feb 2020 21:54:48 -0500 Subject: [PATCH 16/22] checked out http_handler_test to master Signed-off-by: Joe Elliott --- cmd/query/app/http_handler_test.go | 32 +++++++++++------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/cmd/query/app/http_handler_test.go b/cmd/query/app/http_handler_test.go index 4375a051691..a38c8324eb8 100644 --- a/cmd/query/app/http_handler_test.go +++ b/cmd/query/app/http_handler_test.go @@ -587,38 +587,30 @@ func postJSON(url string, req interface{}, out interface{}) error { // execJSON executes an http request against a server and parses response as JSON func execJSON(req *http.Request, out interface{}) error { - resp, err := execJSONHTTPResponse(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if out == nil { - io.Copy(ioutil.Discard, resp.Body) - return nil - } - - decoder := json.NewDecoder(resp.Body) - return decoder.Decode(out) -} - -func execJSONHTTPResponse(req *http.Request) (*http.Response, error) { req.Header.Add("Accept", "application/json") resp, err := httpClient.Do(req) if err != nil { - return nil, err + return err } + defer resp.Body.Close() + if resp.StatusCode > 399 { body, err := ioutil.ReadAll(resp.Body) if err != nil { - return nil, err + return err } - return nil, fmt.Errorf("%d error from server: %s", resp.StatusCode, body) + return fmt.Errorf("%d error from server: %s", resp.StatusCode, body) + } + + if out == nil { + io.Copy(ioutil.Discard, resp.Body) + return nil } - return resp, nil + decoder := json.NewDecoder(resp.Body) + return decoder.Decode(out) } // Generates a JSON response that the server should produce given a certain error code and error. From b78b27a12045f9e3b1d0607678108af7617ce6e2 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Fri, 7 Feb 2020 21:58:14 -0500 Subject: [PATCH 17/22] checked out http_handler to master Signed-off-by: Joe Elliott --- cmd/query/app/http_handler.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/query/app/http_handler.go b/cmd/query/app/http_handler.go index ee3508ead73..b80c9203d70 100644 --- a/cmd/query/app/http_handler.go +++ b/cmd/query/app/http_handler.go @@ -450,8 +450,6 @@ func (aH *APIHandler) writeJSON(w http.ResponseWriter, r *http.Request, response } } resp, _ := marshall(response) - - header := w.Header() - header.Set("Content-Type", "application/json") + w.Header().Set("Content-Type", "application/json") w.Write(resp) } From 6b6278cccc8c605559587ed087b578a908ed9f93 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Sat, 8 Feb 2020 07:13:50 -0500 Subject: [PATCH 18/22] Update cmd/query/app/additional_headers_test.go Co-Authored-By: Yuri Shkuro Signed-off-by: Joe Elliott --- cmd/query/app/additional_headers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/query/app/additional_headers_test.go b/cmd/query/app/additional_headers_test.go index ede05c7a3a4..51813b4a3bc 100644 --- a/cmd/query/app/additional_headers_test.go +++ b/cmd/query/app/additional_headers_test.go @@ -40,7 +40,7 @@ func TestAdditionalHeadersHandler(t *testing.T) { req, err := http.NewRequest("GET", server.URL, nil) assert.NoError(t, err) - resp, err := httpClient.Do(req) + resp, err := server.Client().Do(req) assert.NoError(t, err) for k, v := range additionalHeaders { From 89c8e7943e3f9f8e701b4966f18ee8756cbf5360 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Sat, 8 Feb 2020 07:13:57 -0500 Subject: [PATCH 19/22] Update cmd/query/app/flags_test.go Co-Authored-By: Yuri Shkuro Signed-off-by: Joe Elliott --- cmd/query/app/flags_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index c417ec28ab1..cfcfba4534f 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -44,7 +44,8 @@ func TestQueryBuilderFlags(t *testing.T) { } func TestStringSliceAsHeader(t *testing.T) { - headers := []string{"Access-Control-Allow-Origin: https://mozilla.org", + headers := []string{ + "Access-Control-Allow-Origin: https://mozilla.org", "Access-Control-Expose-Headers: X-My-Custom-Header", "Access-Control-Expose-Headers: X-Another-Custom-Header", } From ee58d2d6f85daa8f42502cedfded0420f3823f8e Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Sat, 8 Feb 2020 07:14:39 -0500 Subject: [PATCH 20/22] Update cmd/query/app/flags_test.go Co-Authored-By: Yuri Shkuro Signed-off-by: Joe Elliott --- cmd/query/app/flags_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index cfcfba4534f..15faad4577d 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -40,7 +40,10 @@ func TestQueryBuilderFlags(t *testing.T) { assert.Equal(t, "some.json", qOpts.UIConfig) assert.Equal(t, "/jaeger", qOpts.BasePath) assert.Equal(t, 80, qOpts.Port) - assert.Equal(t, http.Header{"Access-Control-Allow-Origin": []string{"blerg"}, "Whatever": []string{"thing"}}, qOpts.AdditionalHeaders) + assert.Equal(t, http.Header{ + "Access-Control-Allow-Origin": []string{"blerg"}, + "Whatever": []string{"thing"} + }, qOpts.AdditionalHeaders) } func TestStringSliceAsHeader(t *testing.T) { From 5e269c1d3a539ec2c3a29f19c5f388206e618a34 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Sat, 8 Feb 2020 07:20:03 -0500 Subject: [PATCH 21/22] Made stringSliceAsHeader return err Signed-off-by: Joe Elliott --- cmd/query/app/flags.go | 19 +++++++++++++------ cmd/query/app/flags_test.go | 21 ++++++++++++++------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/cmd/query/app/flags.go b/cmd/query/app/flags.go index 65907e8f4cb..64d4a2ae9c8 100644 --- a/cmd/query/app/flags.go +++ b/cmd/query/app/flags.go @@ -18,6 +18,7 @@ package app import ( "bufio" "flag" + "fmt" "io" "net/http" "net/textproto" @@ -72,15 +73,22 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) *Qu qOpts.StaticAssets = v.GetString(queryStaticFiles) qOpts.UIConfig = v.GetString(queryUIConfig) qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation) - qOpts.AdditionalHeaders = stringSliceAsHeader(v.GetStringSlice(queryAdditionalHeaders), logger) + + stringSlice := v.GetStringSlice(queryAdditionalHeaders) + headers, err := stringSliceAsHeader(stringSlice) + if err != nil { + logger.Error("Failed to parse headers", zap.Strings("slice", stringSlice), zap.Error(err)) + } else { + qOpts.AdditionalHeaders = headers + } return qOpts } // stringSliceAsHeader parses a slice of strings and returns a http.Header. // Each string in the slice is expected to be in the format "key: value" -func stringSliceAsHeader(slice []string, logger *zap.Logger) http.Header { +func stringSliceAsHeader(slice []string) (http.Header, error) { if len(slice) == 0 { - return nil + return nil, nil } allHeaders := strings.Join(slice, "\r\n") @@ -90,9 +98,8 @@ func stringSliceAsHeader(slice []string, logger *zap.Logger) http.Header { header, err := tp.ReadMIMEHeader() if err != nil && err != io.EOF { - logger.Error("Failed to parse headers", zap.Strings("headers", slice)) - return nil + return nil, fmt.Errorf("failed to parse headers") } - return http.Header(header) + return http.Header(header), nil } diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index 15faad4577d..00e365ab84b 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -41,27 +41,34 @@ func TestQueryBuilderFlags(t *testing.T) { assert.Equal(t, "/jaeger", qOpts.BasePath) assert.Equal(t, 80, qOpts.Port) assert.Equal(t, http.Header{ - "Access-Control-Allow-Origin": []string{"blerg"}, - "Whatever": []string{"thing"} - }, qOpts.AdditionalHeaders) + "Access-Control-Allow-Origin": []string{"blerg"}, + "Whatever": []string{"thing"}, + }, qOpts.AdditionalHeaders) } func TestStringSliceAsHeader(t *testing.T) { headers := []string{ - "Access-Control-Allow-Origin: https://mozilla.org", + "Access-Control-Allow-Origin: https://mozilla.org", "Access-Control-Expose-Headers: X-My-Custom-Header", "Access-Control-Expose-Headers: X-Another-Custom-Header", } - parsedHeaders := stringSliceAsHeader(headers, zap.NewNop()) + parsedHeaders, err := stringSliceAsHeader(headers) assert.Equal(t, []string{"https://mozilla.org"}, parsedHeaders["Access-Control-Allow-Origin"]) assert.Equal(t, []string{"X-My-Custom-Header", "X-Another-Custom-Header"}, parsedHeaders["Access-Control-Expose-Headers"]) + assert.NoError(t, err) malformedHeaders := append(headers, "this is not a valid header") - parsedHeaders = stringSliceAsHeader(malformedHeaders, zap.NewNop()) + parsedHeaders, err = stringSliceAsHeader(malformedHeaders) assert.Nil(t, parsedHeaders) + assert.Error(t, err) - parsedHeaders = stringSliceAsHeader([]string{}, zap.NewNop()) + parsedHeaders, err = stringSliceAsHeader([]string{}) assert.Nil(t, parsedHeaders) + assert.NoError(t, err) + + parsedHeaders, err = stringSliceAsHeader(nil) + assert.Nil(t, parsedHeaders) + assert.NoError(t, err) } From bc95bb292828a0328af1120c3e048c5ae164b1b2 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Sat, 8 Feb 2020 11:59:36 -0500 Subject: [PATCH 22/22] Added test for bad header params Signed-off-by: Joe Elliott --- cmd/query/app/flags_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cmd/query/app/flags_test.go b/cmd/query/app/flags_test.go index 00e365ab84b..4308385e12f 100644 --- a/cmd/query/app/flags_test.go +++ b/cmd/query/app/flags_test.go @@ -46,6 +46,15 @@ func TestQueryBuilderFlags(t *testing.T) { }, qOpts.AdditionalHeaders) } +func TestQueryBuilderBadHeadersFlags(t *testing.T) { + v, command := config.Viperize(AddFlags) + command.ParseFlags([]string{ + "--query.additional-headers=malformedheader", + }) + qOpts := new(QueryOptions).InitFromViper(v, zap.NewNop()) + assert.Nil(t, qOpts.AdditionalHeaders) +} + func TestStringSliceAsHeader(t *testing.T) { headers := []string{ "Access-Control-Allow-Origin: https://mozilla.org",