From e8b86f61a21c5209473913531c2b3b72b42e3bc0 Mon Sep 17 00:00:00 2001 From: Amy Tobey Date: Tue, 31 Jan 2023 14:28:59 -0500 Subject: [PATCH] #142 follow spec for OTLP protocol (#148) Per #142, recent PRs had otel-cli deviate from the OTel standards for selecting OTLP exporter protocol. This PR revises the protocol handling for `--endpoint` and adds `--protocol` to more closely or exactly follow the spec. Matching envvars `OTEL_EXPORTER_OTLP_PROTOCOL` and `OTEL_EXPORTER_OTLP_TRACES_PROTOCOL` have been added as well. From the README.md: otel-cli deviates from the OTel specification for endpoint URIs. Mainly, otel-cli supports bare host:port for grpc endpoints and continues to default to gRPC. The optional http/json is not supported by opentelemetry-go so otel-cli does not support it. To use gRPC with an http endpoint, set the protocol with --protocol or the envvar. * bare host:port endpoints are assumed to be gRPC and are not supported for HTTP * http:// and https:// are assumed to be HTTP unless --protocol is set to grpc. * loopback addresses without an https:// prefix are assumed to be unencrypted * go mod tidy seems to be cruft leftover from the renovate bot merge * add --protocol flag & envvar + failing tests Adds the flag & envvars along with a bunch of tests that mostly fail at this point. Once these all pass it should be good to ship. * set correct test server protocol on new tests * minor improvements to failure testing Don't try to parse JSON output when command is expected to fail. Initialize empty maps in results so they don't come back as nil in failure tests. * tune up tests, get them to pass Lots of little adjustments all over. gRPC with http endpoints now works when protocol is grpc. Protocol flag & envvars seem to be fully implemented. Removed mentions of http/json since opentelemetry-go doesn't support it. Updated README to reflect new functionality. * fix up README text * remove OtelError from tests It doesn't work anyways, will revisit when/if it's needed. --- README.md | 11 +-- data_for_test.go | 144 +++++++++++++++++++++++++++++++++++++-- go.sum | 14 ---- main_test.go | 9 ++- otelcli/config.go | 9 +++ otelcli/plumbing.go | 14 +++- otelcli/root.go | 2 + otlpserver/httpserver.go | 9 +-- 8 files changed, 180 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index caaee1e..9dbe9f0 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,7 @@ then config file, then environment variables. | CLI argument | environment variable | config file key | example value | | -------------------- | ----------------------------- | ------------------------ | -------------- | | --endpoint | OTEL_EXPORTER_OTLP_ENDPOINT | endpoint | localhost:4317 | +| --protocol | OTEL_EXPORTER_OTLP_PROTOCOL | protocol | http/protobuf | | --insecure | OTEL_EXPORTER_OTLP_INSECURE | insecure | false | | --timeout | OTEL_EXPORTER_OTLP_TIMEOUT | timeout | 1s | | --otlp-headers | OTEL_EXPORTER_OTLP_HEADERS | otlp_headers | k=v,a=b | @@ -126,11 +127,13 @@ then config file, then environment variables. ### Endpoint URIs -otel-cli deviates from the OTel specification for endpoint URIs. +otel-cli deviates from the OTel specification for endpoint URIs. Mainly, otel-cli supports +bare host:port for grpc endpoints and continues to default to gRPC. The optional http/json +is not supported by opentelemetry-go so otel-cli does not support it. To use gRPC with an +http endpoint, set the protocol with --protocol or the envvar. - * bare `host:port` endpoints are assumed to be gRPC - * `grpc://` URIs are supported - * `http://` and `https://` are assumed to be HTTP _only_ (the spec reuses them for gRPC) + * bare `host:port` endpoints are assumed to be gRPC and are not supported for HTTP + * `http://` and `https://` are assumed to be HTTP unless --protocol is set to `grpc`. * loopback addresses without an https:// prefix are assumed to be unencrypted ## Easy local dev diff --git a/data_for_test.go b/data_for_test.go index c31af44..f4864bd 100644 --- a/data_for_test.go +++ b/data_for_test.go @@ -78,7 +78,6 @@ var suites = []FixtureSuite{ Diagnostics: otelcli.Diagnostics{ IsRecording: false, NumArgs: 1, - OtelError: "", }, }, }, @@ -107,7 +106,6 @@ var suites = []FixtureSuite{ NumArgs: 3, DetectedLocalhost: true, ParsedTimeoutMs: 1000, - OtelError: "", }, Spans: 1, }, @@ -126,14 +124,13 @@ var suites = []FixtureSuite{ SpanData: map[string]string{ "span_id": "*", "trace_id": "*", - "server_meta": "host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces", + "server_meta": "content-type=application/x-protobuf,host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces", }, Diagnostics: otelcli.Diagnostics{ IsRecording: true, NumArgs: 3, DetectedLocalhost: true, ParsedTimeoutMs: 1000, - OtelError: "", }, Spans: 1, }, @@ -445,4 +442,143 @@ var suites = []FixtureSuite{ }, }, }, + // validate OTEL_EXPORTER_OTLP_PROTOCOL / --protocol + { + // --protocol + { + Name: "--protocol grpc", + Config: FixtureConfig{ + ServerProtocol: grpcProtocol, + CliArgs: []string{"status", "--endpoint", "{{endpoint}}", "--protocol", "grpc"}, + TestTimeoutMs: 1000, + }, + Expect: Results{ + Config: otelcli.DefaultConfig().WithEndpoint("{{endpoint}}").WithProtocol("grpc"), + SpanData: map[string]string{ + "server_meta": "proto=grpc", + }, + Diagnostics: otelcli.Diagnostics{ + IsRecording: true, + NumArgs: 5, + DetectedLocalhost: true, + ParsedTimeoutMs: 1000, + }, + Spans: 1, + }, + }, + { + Name: "--protocol http/protobuf", + Config: FixtureConfig{ + ServerProtocol: httpProtocol, + CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}", "--protocol", "http/protobuf"}, + TestTimeoutMs: 1000, + }, + Expect: Results{ + Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}").WithProtocol("http/protobuf"), + SpanData: map[string]string{ + "server_meta": "content-type=application/x-protobuf,host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces", + }, + Diagnostics: otelcli.Diagnostics{ + IsRecording: true, + NumArgs: 5, + DetectedLocalhost: true, + ParsedTimeoutMs: 1000, + }, + Spans: 1, + }, + }, + { + Name: "protocol: bad config", + Config: FixtureConfig{ + CliArgs: []string{"status", "--endpoint", "{{endpoint}}", "--protocol", "xxx", "--verbose", "--fail"}, + TestTimeoutMs: 1000, + }, + Expect: Results{ + CommandFailed: true, + CliOutputRe: regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `), + CliOutput: "invalid protocol setting \"xxx\"\n", + Config: otelcli.DefaultConfig().WithEndpoint("{{endpoint}}"), + Diagnostics: otelcli.Diagnostics{ + IsRecording: false, + NumArgs: 7, + DetectedLocalhost: true, + ParsedTimeoutMs: 1000, + }, + Spans: 0, + }, + }, + // OTEL_EXPORTER_OTLP_PROTOCOL + { + Name: "OTEL_EXPORTER_OTLP_PROTOCOL grpc", + Config: FixtureConfig{ + ServerProtocol: grpcProtocol, + // validate protocol can be set to grpc with an http endpoint + CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}"}, + TestTimeoutMs: 1000, + Env: map[string]string{ + "OTEL_EXPORTER_OTLP_PROTOCOL": "grpc", + }, + }, + Expect: Results{ + Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}").WithProtocol("grpc"), + SpanData: map[string]string{ + "server_meta": "proto=grpc", + }, + Diagnostics: otelcli.Diagnostics{ + IsRecording: true, + NumArgs: 3, + DetectedLocalhost: true, + ParsedTimeoutMs: 1000, + }, + Spans: 1, + }, + }, + { + Name: "OTEL_EXPORTER_OTLP_PROTOCOL http/protobuf", + Config: FixtureConfig{ + ServerProtocol: httpProtocol, + CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}"}, + TestTimeoutMs: 1000, + Env: map[string]string{ + "OTEL_EXPORTER_OTLP_PROTOCOL": "http/protobuf", + }, + }, + Expect: Results{ + Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}").WithProtocol("http/protobuf"), + SpanData: map[string]string{ + "server_meta": "content-type=application/x-protobuf,host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces", + }, + Diagnostics: otelcli.Diagnostics{ + IsRecording: true, + NumArgs: 3, + DetectedLocalhost: true, + ParsedTimeoutMs: 1000, + }, + Spans: 1, + }, + }, + { + Name: "OTEL_EXPORTER_OTLP_PROTOCOL: bad config", + Config: FixtureConfig{ + CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}", "--fail", "--verbose"}, + TestTimeoutMs: 1000, + Env: map[string]string{ + "OTEL_EXPORTER_OTLP_PROTOCOL": "roflcopter", + }, + }, + Expect: Results{ + CommandFailed: true, + CliOutputRe: regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `), + CliOutput: "invalid protocol setting \"roflcopter\"\n", + Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}"), + Diagnostics: otelcli.Diagnostics{ + IsRecording: false, + NumArgs: 3, + DetectedLocalhost: true, + ParsedTimeoutMs: 1000, + }, + Spans: 0, + }, + }, + }, } diff --git a/go.sum b/go.sum index b563a73..abed92c 100644 --- a/go.sum +++ b/go.sum @@ -215,32 +215,18 @@ go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8= go.opencensus.io v0.22.2/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= go.opencensus.io v0.22.3/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= go.opencensus.io v0.22.4/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= -go.opentelemetry.io/otel v1.11.2 h1:YBZcQlsVekzFsFbjygXMOXSs6pialIZxcjfO/mBDmR0= -go.opentelemetry.io/otel v1.11.2/go.mod h1:7p4EUV+AqgdlNV9gL97IgUZiVR3yrFXYo53f9BM3tRI= go.opentelemetry.io/otel v1.12.0 h1:IgfC7kqQrRccIKuB7Cl+SRUmsKbEwSGPr0Eu+/ht1SQ= go.opentelemetry.io/otel v1.12.0/go.mod h1:geaoz0L0r1BEOR81k7/n9W4TCXYCJ7bPO7K374jQHG0= -go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.11.2 h1:htgM8vZIF8oPSCxa341e3IZ4yr/sKxgu8KZYllByiVY= -go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.11.2/go.mod h1:rqbht/LlhVBgn5+k3M5QK96K5Xb0DvXpMJ5SFQpY6uw= go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.12.0 h1:UfDENi+LTcLjQ/JhaXimjlIgn7wWjwbEMmdREm2Gyng= go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.12.0/go.mod h1:rqbht/LlhVBgn5+k3M5QK96K5Xb0DvXpMJ5SFQpY6uw= -go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.11.2 h1:fqR1kli93643au1RKo0Uma3d2aPQKT+WBKfTSBaKbOc= -go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.11.2/go.mod h1:5Qn6qvgkMsLDX+sYK64rHb1FPhpn0UtxF+ouX1uhyJE= go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.12.0 h1:ZVqtSAxrR4+ofzayuww0/EKamCjjnwnXTMRZzMudJoU= go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.12.0/go.mod h1:IlaGLENJkAl9+Xoo3J0unkdOwtL+rmqZ3ryMjUtYA94= -go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.11.2 h1:ERwKPn9Aer7Gxsc0+ZlutlH1bEEAUXAUhqm3Y45ABbk= -go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.11.2/go.mod h1:jWZUM2MWhWCJ9J9xVbRx7tzK1mXKpAlze4CeulycwVY= go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.12.0 h1:+tsVdWosoqDfX6cdHAeacZozjQS94ySBd+aUXFwnNKA= go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.12.0/go.mod h1:jSqjV+Knu1Jyvh+l3fx7V210Ev3HHgNQAi8YqpXaQP8= -go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.11.2 h1:Us8tbCmuN16zAnK5TC69AtODLycKbwnskQzaB6DfFhc= -go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.11.2/go.mod h1:GZWSQQky8AgdJj50r1KJm8oiQiIPaAX7uZCFQX9GzC8= go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.12.0 h1:L23MzcHDznr05xOM1Ng1F98L0nVd7hm/S7y2jW9IRB4= go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.12.0/go.mod h1:C+onYX2j5QH653b3wGJwowYr8jLMjBJw35QcaCQQK0U= -go.opentelemetry.io/otel/sdk v1.11.2 h1:GF4JoaEx7iihdMFu30sOyRx52HDHOkl9xQ8SMqNXUiU= -go.opentelemetry.io/otel/sdk v1.11.2/go.mod h1:wZ1WxImwpq+lVRo4vsmSOxdd+xwoUJ6rqyLc3SyX9aU= go.opentelemetry.io/otel/sdk v1.12.0 h1:8npliVYV7qc0t1FKdpU08eMnOjgPFMnriPhn0HH4q3o= go.opentelemetry.io/otel/sdk v1.12.0/go.mod h1:WYcvtgquYvgODEvxOry5owO2y9MyciW7JqMz6cpXShE= -go.opentelemetry.io/otel/trace v1.11.2 h1:Xf7hWSF2Glv0DE3MH7fBHvtpSBsjcBUe5MYAmZM/+y0= -go.opentelemetry.io/otel/trace v1.11.2/go.mod h1:4N+yC7QEz7TTsG9BSRLNAa63eg5E06ObSbKPmxQ/pKA= go.opentelemetry.io/otel/trace v1.12.0 h1:p28in++7Kd0r2d8gSt931O57fdjUyWxkVbESuILAeUc= go.opentelemetry.io/otel/trace v1.12.0/go.mod h1:pHlgBynn6s25qJ2szD+Bv+iwKJttjHSI3lUAyf0GNuQ= go.opentelemetry.io/proto/otlp v0.7.0/go.mod h1:PqfVotwruBrMGOCsRd/89rSnXhoiJIqeYNgFYFoEGnI= diff --git a/main_test.go b/main_test.go index 518df8e..3825c20 100644 --- a/main_test.go +++ b/main_test.go @@ -144,7 +144,7 @@ func checkAll(t *testing.T, fixture Fixture, endpoint string, results Results, s // many of the basic plumbing tests use status so it has its own set of checks // but these shouldn't run for testing the other subcommands - if len(fixture.Config.CliArgs) > 0 && fixture.Config.CliArgs[0] == "status" { + if len(fixture.Config.CliArgs) > 0 && fixture.Config.CliArgs[0] == "status" && !fixture.Expect.CommandFailed { checkStatusData(t, fixture, endpoint, results) } else { // checking the text output only makes sense for non-status paths @@ -296,7 +296,10 @@ func checkSpanData(t *testing.T, fixture Fixture, endpoint string, span otlpserv func runOtelCli(t *testing.T, fixture Fixture) (string, Results, otlpserver.CliEvent, otlpserver.CliEventList) { started := time.Now() - var results Results + results := Results{ + SpanData: map[string]string{}, + Env: map[string]string{}, + } var retSpan otlpserver.CliEvent var retEvents otlpserver.CliEventList @@ -409,7 +412,7 @@ func runOtelCli(t *testing.T, fixture Fixture) (string, Results, otlpserver.CliE // only try to parse status json if it was a status command // TODO: support variations on otel-cli where status isn't the first arg - if len(cliOut) > 0 && len(args) > 0 && args[0] == "status" { + if len(cliOut) > 0 && len(args) > 0 && args[0] == "status" && !fixture.Expect.CommandFailed { err = json.Unmarshal(cliOut, &results) if err != nil { t.Errorf("[%s] parsing otel-cli status output failed: %s", fixture.Name, err) diff --git a/otelcli/config.go b/otelcli/config.go index 78230d7..17e65d7 100644 --- a/otelcli/config.go +++ b/otelcli/config.go @@ -24,6 +24,7 @@ const spanBgSockfilename = "otel-cli-background.sock" func DefaultConfig() Config { return Config{ Endpoint: "", + Protocol: "", Timeout: "1s", Headers: map[string]string{}, Insecure: false, @@ -58,6 +59,7 @@ func DefaultConfig() Config { // Data structure is public so that it can serialize to json easily. type Config struct { Endpoint string `json:"endpoint" env:"OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"` + Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL"` Timeout string `json:"timeout" env:"OTEL_EXPORTER_OTLP_TIMEOUT,OTEL_EXPORTER_OTLP_TRACES_TIMEOUT"` Headers map[string]string `json:"otlp_headers" env:"OTEL_EXPORTER_OTLP_HEADERS"` // TODO: needs json marshaler hook to mask tokens Insecure bool `json:"insecure" env:"OTEL_EXPORTER_OTLP_INSECURE"` @@ -171,6 +173,7 @@ func (c *Config) LoadEnv(getenv func(string) string) error { func (c Config) ToStringMap() map[string]string { return map[string]string{ "endpoint": c.Endpoint, + "protocol": c.Protocol, "timeout": c.Timeout, "headers": flattenStringMap(c.Headers, "{}"), "insecure": strconv.FormatBool(c.Insecure), @@ -205,6 +208,12 @@ func (c Config) WithEndpoint(with string) Config { return c } +// WithProtocol returns the config with protocol set to the provided value. +func (c Config) WithProtocol(with string) Config { + c.Protocol = with + return c +} + // WithTimeout returns the config with Timeout set to the provided value. func (c Config) WithTimeout(with string) Config { c.Timeout = with diff --git a/otelcli/plumbing.go b/otelcli/plumbing.go index 1000d49..49d65df 100644 --- a/otelcli/plumbing.go +++ b/otelcli/plumbing.go @@ -33,6 +33,10 @@ func initTracer() (context.Context, func()) { return ctx, func() {} } + if config.Protocol != "" && config.Protocol != "grpc" && config.Protocol != "http/protobuf" { + softFail("invalid protocol setting %q", config.Protocol) + } + var exporter sdktrace.SpanExporter // allows overwrite in --test mode var err error @@ -41,8 +45,10 @@ func initTracer() (context.Context, func()) { // awkward for otel-cli so we break with the spec. otel-cli will only resolve // http(s):// to HTTP protocols, defaults bare host:port to gRPC, and supports // grpc:// to definitely use gRPC to connect out. - if strings.HasPrefix(config.Endpoint, "http://") || - strings.HasPrefix(config.Endpoint, "https://") { + if config.Protocol != "grpc" && + (strings.HasPrefix(config.Protocol, "http/") || + strings.HasPrefix(config.Endpoint, "http://") || + strings.HasPrefix(config.Endpoint, "https://")) { exporter, err = otlphttp.New(ctx, httpOptions()...) if err != nil { softFail("failed to configure OTLP/HTTP exporter: %s", err) @@ -97,7 +103,9 @@ func grpcOptions() []otlpgrpc.Option { grpcOpts := []otlpgrpc.Option{} // per comment in initTracer(), grpc:// is specific to otel-cli - if strings.HasPrefix(config.Endpoint, "grpc://") { + if strings.HasPrefix(config.Endpoint, "grpc://") || + strings.HasPrefix(config.Endpoint, "http://") || + strings.HasPrefix(config.Endpoint, "https://") { ep, err := url.Parse(config.Endpoint) if err != nil { softFail("error parsing provided gRPC URI '%s': %s", config.Endpoint, err) diff --git a/otelcli/root.go b/otelcli/root.go index f4b33f5..2ee39de 100644 --- a/otelcli/root.go +++ b/otelcli/root.go @@ -46,6 +46,8 @@ func addCommonParams(cmd *cobra.Command) { cmd.Flags().StringVarP(&config.CfgFile, "config", "c", defaults.CfgFile, "JSON configuration file") // --endpoint an endpoint to send otlp output to cmd.Flags().StringVar(&config.Endpoint, "endpoint", defaults.Endpoint, "host and port for the desired OTLP/gRPC or OTLP/HTTP endpoint (use http:// or https:// for OTLP/HTTP)") + // --protocol allows setting the OTLP protocol instead of relying on auto-detection from URI + cmd.Flags().StringVar(&config.Protocol, "protocol", defaults.Protocol, "desired OTLP protocol: grpc or http/protobuf") // --timeout a default timeout to use in all otel-cli operations (default 1s) cmd.Flags().StringVar(&config.Timeout, "timeout", defaults.Timeout, "timeout for otel-cli operations, all timeouts in otel-cli use this value") // --verbose tells otel-cli to actually log errors to stderr instead of failing silently diff --git a/otlpserver/httpserver.go b/otlpserver/httpserver.go index e88b0e2..7e2dff4 100644 --- a/otlpserver/httpserver.go +++ b/otlpserver/httpserver.go @@ -49,10 +49,11 @@ func (hs *HttpServer) ServeHTTP(rw http.ResponseWriter, req *http.Request) { } meta := map[string]string{ - "method": req.Method, - "proto": req.Proto, - "host": req.Host, - "uri": req.RequestURI, + "method": req.Method, + "proto": req.Proto, + "content-type": req.Header.Get("Content-Type"), + "host": req.Host, + "uri": req.RequestURI, } done := otelToCliEvent(hs.callback, &msg, meta)