Merge pull request #133 from hashicorp/asheshvidyut/NET-2190/fix-log-…

…level-present-in-extra-args

fix: log level present in extra args using envoy-extra-args annotation : NET-2190

.changelog/133.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+* Add support for envoy-extra-args. Fixes [Envoy extra-args annotation crashing consul-dataplane container](https://github.com/hashicorp/consul-k8s/issues/1846).
+```

pkg/envoy/proxy.go

@@ -409,6 +409,13 @@ func (p *Proxy) buildCommand(ctx context.Context, cfgPath string) *exec.Cmd {
 		logLevel = "info"
 	}
 
+	// Updating loglevel value if --log-level is present in extra args
+	newExtraArgs, valOfLoggerInExtraArgs := removeArgAndGetValue(p.cfg.ExtraArgs, "--log-level")
+
+	if len(valOfLoggerInExtraArgs) > 0 {
+		logLevel = valOfLoggerInExtraArgs
+	}
+
 	args := append(
 		[]string{
 			"--config-path", cfgPath,
@@ -418,7 +425,7 @@ func (p *Proxy) buildCommand(ctx context.Context, cfgPath string) *exec.Cmd {
 			// TODO(NET-713): support hot restarts.
 			"--disable-hot-restart",
 		},
-		p.cfg.ExtraArgs...,
+		newExtraArgs...,
 	)
 
 	cmd := exec.CommandContext(ctx, p.cfg.ExecutablePath, args...)
@@ -427,3 +434,15 @@ func (p *Proxy) buildCommand(ctx context.Context, cfgPath string) *exec.Cmd {
 
 	return cmd
 }
+
+// removeArgAndGetValue Function to get new args after removing given key
+// and also returns the value of key
+func removeArgAndGetValue(stringAr []string, key string) ([]string, string) {
+	for index, item := range stringAr {
+		if item == key {
+			valueToReturn := stringAr[index+1]
+			return append(stringAr[:index], stringAr[index+2:]...), valueToReturn
+		}
+	}
+	return stringAr, ""
+}

pkg/envoy/proxy_test.go

@@ -146,3 +146,129 @@ func testOutputPath() string {
 		fmt.Sprintf("test-output-%x.json", time.Now().UnixNano()+int64(os.Getpid())),
 	)
 }
+
+func TestProxy_OverridingLoggerAndExtraArgs(t *testing.T) {
+	bootstrapConfig := []byte(`hello world`)
+
+	// Test checks we are starting proxy with only one argument of log-level
+	// When log-level is present in both ProxyConfig and ExtraArgs
+	// We override the log-level with that of ExtraArgs
+	outputPath := testOutputPath()
+	t.Cleanup(func() { _ = os.Remove(outputPath) })
+
+	p, err := NewProxy(ProxyConfig{
+		Logger:            hclog.New(&hclog.LoggerOptions{Level: hclog.Warn, Output: io.Discard}),
+		EnvoyErrorStream:  io.Discard,
+		EnvoyOutputStream: io.Discard,
+		ExecutablePath:    "testdata/fake-envoy",
+		ExtraArgs:         []string{"--test-output", outputPath, "--log-level", "debug"},
+		BootstrapConfig:   bootstrapConfig,
+	})
+	require.NoError(t, err)
+	require.NoError(t, p.Run(context.Background()))
+	t.Cleanup(func() { _ = p.Kill() })
+
+	// Read the output written by fake-envoy. It might take a while, so poll the
+	// file for a couple of second
+	var output struct {
+		Args       []byte
+		ConfigData []byte
+	}
+	require.Eventually(t, func() bool {
+		outputBytes, err := os.ReadFile(outputPath)
+		if err != nil {
+			t.Logf("failed to read output file: %v", err)
+			return false
+		}
+		if err := json.Unmarshal(outputBytes, &output); err != nil {
+			t.Logf("failed to unmarshal output file: %v", err)
+			return false
+		}
+		return true
+	}, 2*time.Second, 50*time.Millisecond)
+
+	// Check that fake-envoy was able to read the config from the pipe.
+	require.Equal(t, bootstrapConfig, output.ConfigData)
+
+	// Check that we're correctly configuring the log level.
+	require.Contains(t, string(output.Args), "--log-level debug")
+
+	// Check that we're removing the logger in Proxy config and overriding it with extra args
+	require.NotContains(t, string(output.Args), "--log-level warn")
+
+	// Check that we're disabling hot restarts.
+	require.Contains(t, string(output.Args), "--disable-hot-restart")
+
+	// Check the process is still running.
+	require.NoError(t, p.cmd.Process.Signal(syscall.Signal(0)))
+
+	// Ensure Stop kills and reaps the process.
+	require.NoError(t, p.Kill())
+
+	require.Eventually(t, func() bool {
+		return p.cmd.Process.Signal(syscall.Signal(0)) == os.ErrProcessDone
+	}, 2*time.Second, 50*time.Millisecond)
+}
+
+func TestProxy_EnvoyExtraArgs(t *testing.T) {
+	bootstrapConfig := []byte(`hello world`)
+
+	// Test checks we are starting proxy with only one argument of log-level
+	// When log-level is present in both ProxyConfig and ExtraArgs
+	// We override the log-level with that of ExtraArgs
+	outputPath := testOutputPath()
+	t.Cleanup(func() { _ = os.Remove(outputPath) })
+
+	p, err := NewProxy(ProxyConfig{
+		Logger:            hclog.New(&hclog.LoggerOptions{Level: hclog.Warn, Output: io.Discard}),
+		EnvoyErrorStream:  io.Discard,
+		EnvoyOutputStream: io.Discard,
+		ExecutablePath:    "testdata/fake-envoy",
+		ExtraArgs:         []string{"--test-output", outputPath, "--log-level", "debug", "--concurrency", "1"},
+		BootstrapConfig:   bootstrapConfig,
+	})
+	require.NoError(t, err)
+	require.NoError(t, p.Run(context.Background()))
+	t.Cleanup(func() { _ = p.Kill() })
+
+	// Read the output written by fake-envoy. It might take a while, so poll the
+	// file for a couple of second
+	var output struct {
+		Args       []byte
+		ConfigData []byte
+	}
+	require.Eventually(t, func() bool {
+		outputBytes, err := os.ReadFile(outputPath)
+		if err != nil {
+			t.Logf("failed to read output file: %v", err)
+			return false
+		}
+		if err := json.Unmarshal(outputBytes, &output); err != nil {
+			t.Logf("failed to unmarshal output file: %v", err)
+			return false
+		}
+		return true
+	}, 2*time.Second, 50*time.Millisecond)
+
+	// Check that fake-envoy was able to read the config from the pipe.
+	require.Equal(t, bootstrapConfig, output.ConfigData)
+
+	// Check that we're correctly configuring the log level.
+	require.Contains(t, string(output.Args), "--log-level debug")
+
+	// Check that we're removing the logger in Proxy config and overriding it with extra args
+	require.NotContains(t, string(output.Args), "--log-level warn")
+
+	// Check that we're disabling hot restarts.
+	require.Contains(t, string(output.Args), "--disable-hot-restart")
+
+	// Check the process is still running.
+	require.NoError(t, p.cmd.Process.Signal(syscall.Signal(0)))
+
+	// Ensure Stop kills and reaps the process.
+	require.NoError(t, p.Kill())
+
+	require.Eventually(t, func() bool {
+		return p.cmd.Process.Signal(syscall.Signal(0)) == os.ErrProcessDone
+	}, 2*time.Second, 50*time.Millisecond)
+}