work around otel reading envvars #181 (#208)

When I rewrote the envvar handling for otel-cli and removed Viper, one
of the problems I tried to solve was envvars being read by both otel-cli
and the otel collector backend it uses. Otel-cli allows some things that
the collector doesn't, and besides, otel-cli can't make any guarantees
for code paths that it doesn't test. So, envvars have to be deleted from
the os environment to prevent that, but we still need to pass the
original env through.

The easy path forward is to back up the env early at startup, which
happens now, and then have exec and status use that backup instead of
os.Environ(). This commit implements that.

Squashed commitlog follows:

* add a private field to config to backup env

* make a copy of env before envvar configuration

We have to delete envvars during configuration that might also be read
by the OTel collector client. Also, users of otel-cli exec expect
envvars including OTEL stuff to pass through  so they don't have to do
weird backflips to configure embedded exec usage, which is one of the
earliest documented features of otel-cli.

To accomplish this, refactored the PersistentPreRun func to be a full
func instead of inline. Modified it to inject os.Environ() into the
private config field before calling the envvar config func.

* persist env through to exec and status commands

When I rewrote the envvar handling for otel-cli and removed Viper, one
of the problems I tried to solve was envvars being read by both otel-cli
and the otel collector backend it uses. Otel-cli allows some things that
the collector doesn't, and besides, otel-cli can't make any guarantees
for code paths that it doesn't test. So, envvars have to be deleted from
the os environment to prevent that, but we still need to pass the
original env through.

The easy path forward is to back up the env early at startup, which
happens now, and then have exec and status use that backup instead of
os.Environ(). This commit implements that.

status was updated to also pass through so it's easier to test.

Added a test for pass-through, as well as updated tests that use
`status` so they now expect envvars to pass through again.

data_for_test.go

@@ -363,6 +363,34 @@ var suites = []FixtureSuite{
 				},
 			},
 		},
+		{
+			Name: "#181 OTEL_ envvars should persist through to otel-cli exec",
+			Config: FixtureConfig{
+				CliArgs: []string{"status"},
+				Env: map[string]string{
+					"OTEL_FAKE_VARIABLE":             "fake value",
+					"OTEL_EXPORTER_OTLP_ENDPOINT":    "{{endpoint}}",
+					"OTEL_EXPORTER_OTLP_CERTIFICATE": "{{tls_ca_cert}}",
+					"X_WHATEVER":                     "whatever",
+				},
+			},
+			Expect: Results{
+				SpanCount: 1,
+				Config:    otelcli.DefaultConfig().WithEndpoint("{{endpoint}}").WithTlsCACert("{{tls_ca_cert}}"),
+				Env: map[string]string{
+					"OTEL_FAKE_VARIABLE":             "fake value",
+					"OTEL_EXPORTER_OTLP_ENDPOINT":    "{{endpoint}}",
+					"OTEL_EXPORTER_OTLP_CERTIFICATE": "{{tls_ca_cert}}",
+					"X_WHATEVER":                     "whatever",
+				},
+				Diagnostics: otelcli.Diagnostics{
+					IsRecording:       true,
+					DetectedLocalhost: true,
+					NumArgs:           1,
+					ParsedTimeoutMs:   1000,
+				},
+			},
+		},
 	},
 	// otel-cli span with no OTLP config should do and print nothing
 	{
@@ -393,6 +421,9 @@ var suites = []FixtureSuite{
 					NumArgs:         3,
 					ParsedTimeoutMs: 1000,
 				},
+				Env: map[string]string{
+					"OTEL_EXPORTER_OTLP_ENDPOINT": "{{endpoint}}",
+				},
 				Config: otelcli.DefaultConfig().
 					WithEndpoint("{{endpoint}}"). // tells the test framework to ignore/overwrite
 					WithTimeout("1s").
@@ -751,6 +782,9 @@ var suites = []FixtureSuite{
 				SpanData: map[string]string{
 					"server_meta": "proto=grpc",
 				},
+				Env: map[string]string{
+					"OTEL_EXPORTER_OTLP_PROTOCOL": "grpc",
+				},
 				Diagnostics: otelcli.Diagnostics{
 					IsRecording:       true,
 					NumArgs:           3,
@@ -775,6 +809,9 @@ var suites = []FixtureSuite{
 				SpanData: map[string]string{
 					"server_meta": "content-type=application/x-protobuf,host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces",
 				},
+				Env: map[string]string{
+					"OTEL_EXPORTER_OTLP_PROTOCOL": "http/protobuf",
+				},
 				Diagnostics: otelcli.Diagnostics{
 					IsRecording:       true,
 					NumArgs:           3,

otelcli/config.go

@@ -98,6 +98,9 @@ type Config struct {
 	CfgFile string `json:"config_file" env:"OTEL_CLI_CONFIG_FILE"`
 	Verbose bool   `json:"verbose" env:"OTEL_CLI_VERBOSE"`
 	Fail    bool   `json:"fail" env:"OTEL_CLI_FAIL"`
+
+	// private things for internals
+	envBackup []string // used to back up envvars for otel-cli exec
 }
 
 // LoadFile reads the file specified by -c/--config and overwrites the

otelcli/exec.go

@@ -69,7 +69,9 @@ func doExec(cmd *cobra.Command, args []string) {
 	child.Env = []string{}
 
 	// grab everything BUT the TRACEPARENT envvar
-	for _, env := range os.Environ() {
+	// read the cached env from config, because os.Environ() has to be modified
+	// to work around OTel libs reading envvars directly
+	for _, env := range config.envBackup {
 		if !strings.HasPrefix(env, "TRACEPARENT=") {
 			child.Env = append(child.Env, env)
 		}

otelcli/root.go

@@ -8,23 +8,10 @@ import (
 
 // rootCmd represents the base command when called without any subcommands
 var rootCmd = &cobra.Command{
-	Use:   "otel-cli",
-	Short: "CLI for creating and sending OpenTelemetry spans and events.",
-	Long:  `A command-line interface for generating OpenTelemetry data on the command line.`,
-	PersistentPreRun: func(cmd *cobra.Command, args []string) {
-		if err := config.LoadFile(); err != nil {
-			softFail("Error while loading configuration file %s: %s", config.CfgFile, err)
-		}
-		if err := config.LoadEnv(os.Getenv); err != nil {
-			// will need to specify --fail --verbose flags to see these errors
-			softFail("Error while loading environment variables: %s", err)
-		}
-
-		// plug a copy of the completed config into diagnostics
-		// so the otel error handler can check --fail/--verbose config
-		// this should go away after rewriting the otel exporter
-		diagnostics.config = config
-	},
+	Use:              "otel-cli",
+	Short:            "CLI for creating and sending OpenTelemetry spans and events.",
+	Long:             `A command-line interface for generating OpenTelemetry data on the command line.`,
+	PersistentPreRun: ConfigPreRun,
 }
 
 // Execute adds all child commands to the root command and sets flags appropriately.
@@ -45,6 +32,28 @@ func init() {
 	}
 }
 
+// ConfigPreRun is called by Cobra right after reading CLI args, and will load
+// the config file, then environment.
+func ConfigPreRun(cmd *cobra.Command, args []string) {
+	// because the OTel collector client code directly reads envvars, the OTEL_
+	// variables are deleted during config.LoadEnv(). This breaks expectations
+	// for otel-cli exec users, so we save a copy to pass to exec
+	config.envBackup = os.Environ()
+
+	if err := config.LoadFile(); err != nil {
+		softFail("Error while loading configuration file %s: %s", config.CfgFile, err)
+	}
+	if err := config.LoadEnv(os.Getenv); err != nil {
+		// will need to specify --fail --verbose flags to see these errors
+		softFail("Error while loading environment variables: %s", err)
+	}
+
+	// plug a copy of the completed config into diagnostics
+	// so the otel error handler can check --fail/--verbose config
+	// this should go away after rewriting the otel exporter
+	diagnostics.config = config
+}
+
 // addCommonParams adds the --config and --endpoint params to the command.
 func addCommonParams(cmd *cobra.Command) {
 	defaults := DefaultConfig()

otelcli/status.go

@@ -49,7 +49,7 @@ func doStatus(cmd *cobra.Command, args []string) {
 	span.Kind = tracepb.Span_SPAN_KIND_INTERNAL
 
 	env := make(map[string]string)
-	for _, e := range os.Environ() {
+	for _, e := range config.envBackup {
 		parts := strings.SplitN(e, "=", 2)
 		if len(parts) == 2 {
 			// TODO: this is just enough so I can sleep tonight.