Revert "Merge pull request hashicorp#378 from hashicorp/extra-envoy-a…

…rgs" This reverts commit fde8f5f, reversing changes made to 32ade6c.
ndhanushkodi · Nov 6, 2020 · 9561c2e · 9561c2e
1 parent fbad215
commit 9561c2e
Show file tree

Hide file tree

Showing 7 changed files with 4 additions and 157 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,13 +1,5 @@
 ## UNRELEASED
 
-IMPROVEMENTS:
-* Connect: support passing extra arguments to the envoy binary. [[GH-378](https://github.com/hashicorp/consul-k8s/pull/378)]
-    Arguments can be passed in 2 ways:
-    * via a flag to the consul-k8s inject-connect command:
-      e.g. `consul-k8s inject-connect -envoy-extra-args="--log-level debug --disable-hot-restart"`
-    * via pod annotations
-      e.g. `consul.hashicorp.com/envoy-extra-args: "--log-level debug --disable-hot-restart"`
-
 BUG FIXES:
 * Federation: ensure replication ACL token can replicate policies and tokens in Consul namespaces other than `default` (Consul-enterprise only). [[GH-364](https://github.com/hashicorp/consul-k8s/issues/364)]
 * CRDs: validate custom resources can only set namespace fields if Consul namespaces are enabled [[GH-375](https://github.com/hashicorp/consul-k8s/pull/375)]

diff --git a/connect-inject/envoy_sidecar.go b/connect-inject/envoy_sidecar.go
@@ -3,11 +3,9 @@ package connectinject
 import (
 	"bytes"
 	"fmt"
-	"strconv"
 	"strings"
 	"text/template"
 
-	"github.com/google/shlex"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 )
@@ -37,11 +35,6 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con
 		return corev1.Container{}, err
 	}
 
-	cmd, err := h.getContainerSidecarCommand(pod)
-	if err != nil {
-		return corev1.Container{}, err
-	}
-
 	container := corev1.Container{
 		Name:  "consul-connect-envoy-sidecar",
 		Image: h.ImageEnvoy,
@@ -71,7 +64,10 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con
 				},
 			},
 		},
-		Command: cmd,
+		Command: []string{
+			"envoy",
+			"--config-path", "/consul/connect-inject/envoy-bootstrap.yaml",
+		},
 	}
 	if h.ConsulCACert != "" {
 		caCertEnvVar := corev1.EnvVar{
@@ -91,38 +87,6 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con
 	}
 	return container, nil
 }
-func (h *Handler) getContainerSidecarCommand(pod *corev1.Pod) ([]string, error) {
-	cmd := []string{
-		"envoy",
-		"--config-path", "/consul/connect-inject/envoy-bootstrap.yaml",
-	}
-
-	extraArgs, annotationSet := pod.Annotations[annotationEnvoyExtraArgs]
-
-	if annotationSet || h.EnvoyExtraArgs != "" {
-
-		extraArgsToUse := h.EnvoyExtraArgs
-
-		// Prefer args set by pod annotation over the flag to the consul-k8s binary (h.EnvoyExtraArgs).
-		if annotationSet {
-			extraArgsToUse = extraArgs
-		}
-
-		// Split string into tokens.
-		// e.g. "--foo bar --boo baz" --> ["--foo", "bar", "--boo", "baz"]
-		tokens, err := shlex.Split(extraArgsToUse)
-		if err != nil {
-			return []string{}, err
-		}
-		for _, t := range tokens {
-			if strings.Contains(t, " ") {
-				t = strconv.Quote(t)
-			}
-			cmd = append(cmd, t)
-		}
-	}
-	return cmd, nil
-}
 
 func (h *Handler) envoySidecarResources(pod *corev1.Pod) (corev1.ResourceRequirements, error) {
 	resources := corev1.ResourceRequirements{

diff --git a/connect-inject/envoy_sidecar_test.go b/connect-inject/envoy_sidecar_test.go
@@ -60,97 +60,6 @@ func TestHandlerEnvoySidecar(t *testing.T) {
 	})
 }
 
-// Test that we can pass extra args to envoy via the extraEnvoyArgs flag
-// or via pod annotations. When arguments are passed in both ways, the
-// arguments set via pod annotations are used.
-func TestHandlerEnvoySidecar_EnvoyExtraArgs(t *testing.T) {
-	cases := []struct {
-		name                     string
-		envoyExtraArgs           string
-		pod                      *corev1.Pod
-		expectedContainerCommand []string
-	}{
-		{
-			name:           "no extra options provided",
-			envoyExtraArgs: "",
-			pod:            &corev1.Pod{},
-			expectedContainerCommand: []string{
-				"envoy",
-				"--config-path", "/consul/connect-inject/envoy-bootstrap.yaml",
-			},
-		},
-		{
-			name:           "via flag: extra log-level option",
-			envoyExtraArgs: "--log-level debug",
-			pod:            &corev1.Pod{},
-			expectedContainerCommand: []string{
-				"envoy",
-				"--config-path", "/consul/connect-inject/envoy-bootstrap.yaml",
-				"--log-level", "debug",
-			},
-		},
-		{
-			name:           "via flag: multiple arguments with quotes",
-			envoyExtraArgs: "--log-level debug --admin-address-path \"/tmp/consul/foo bar\"",
-			pod:            &corev1.Pod{},
-			expectedContainerCommand: []string{
-				"envoy",
-				"--config-path", "/consul/connect-inject/envoy-bootstrap.yaml",
-				"--log-level", "debug",
-				"--admin-address-path", "\"/tmp/consul/foo bar\"",
-			},
-		},
-		{
-			name:           "via annotation: multiple arguments with quotes",
-			envoyExtraArgs: "",
-			pod: &corev1.Pod{
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						annotationEnvoyExtraArgs: "--log-level debug --admin-address-path \"/tmp/consul/foo bar\"",
-					},
-				},
-			},
-			expectedContainerCommand: []string{
-				"envoy",
-				"--config-path", "/consul/connect-inject/envoy-bootstrap.yaml",
-				"--log-level", "debug",
-				"--admin-address-path", "\"/tmp/consul/foo bar\"",
-			},
-		},
-		{
-			name:           "via flag and annotation: should prefer setting via the annotation",
-			envoyExtraArgs: "this should be overwritten",
-			pod: &corev1.Pod{
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						annotationEnvoyExtraArgs: "--log-level debug --admin-address-path \"/tmp/consul/foo bar\"",
-					},
-				},
-			},
-			expectedContainerCommand: []string{
-				"envoy",
-				"--config-path", "/consul/connect-inject/envoy-bootstrap.yaml",
-				"--log-level", "debug",
-				"--admin-address-path", "\"/tmp/consul/foo bar\"",
-			},
-		},
-	}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			h := Handler{
-				ImageConsul:    "hashicorp/consul:latest",
-				ImageEnvoy:     "hashicorp/consul-k8s:latest",
-				EnvoyExtraArgs: tc.envoyExtraArgs,
-			}
-
-			c, err := h.envoySidecar(tc.pod, k8sNamespace)
-			require.NoError(t, err)
-			require.Equal(t, tc.expectedContainerCommand, c.Command)
-		})
-	}
-}
-
 // Test that if AuthMethod is set
 // the preStop command includes a token
 func TestHandlerEnvoySidecar_AuthMethod(t *testing.T) {

diff --git a/connect-inject/handler.go b/connect-inject/handler.go
@@ -85,13 +85,6 @@ const (
 	annotationSidecarProxyMemoryLimit   = "consul.hashicorp.com/sidecar-proxy-memory-limit"
 	annotationSidecarProxyMemoryRequest = "consul.hashicorp.com/sidecar-proxy-memory-request"
 
-	// annotationEnvoyExtraArgs is a space-separated list of arguments to be passed to the
-	// envoy binary. See list of args here: https://www.envoyproxy.io/docs/envoy/latest/operations/cli
-	// e.g. consul.hashicorp.com/envoy-extra-args: "--log-level debug --disable-hot-restart"
-	// The arguments passed in via this annotation will take precendence over arguments
-	// passed via the -envoy-extra-args flag.
-	annotationEnvoyExtraArgs = "consul.hashicorp.com/envoy-extra-args"
-
 	// injected is used as the annotation value for annotationInjected
 	injected = "injected"
 
@@ -123,10 +116,6 @@ type Handler struct {
 	// This image is used for the lifecycle-sidecar container.
 	ImageConsulK8S string
 
-	// Optional: set when you need extra options to be set when running envoy
-	// See a list of args here: https://www.envoyproxy.io/docs/envoy/latest/operations/cli
-	EnvoyExtraArgs string
-
 	// RequireAnnotation means that the annotation must be given to inject.
 	// If this is false, injection is default.
 	RequireAnnotation bool

diff --git a/go.mod b/go.mod
@@ -8,7 +8,6 @@ require (
 	github.com/go-logr/logr v0.1.0
 	github.com/google/go-cmp v0.4.0
 	github.com/google/go-querystring v1.0.0 // indirect
-	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
 	github.com/hashicorp/consul/api v1.4.1-0.20201007080954-aa0f5ff839c5
 	github.com/hashicorp/consul/sdk v0.6.0
 	github.com/hashicorp/go-discover v0.0.0-20200812215701-c4b85f6ed31f

diff --git a/go.sum b/go.sum
@@ -229,8 +229,6 @@ github.com/google/gofuzz v1.1.0 h1:Hsa8mG0dQ46ij8Sl2AYJDUv1oA9/d6Vk+3LG99Oe02g=
 github.com/google/gofuzz v1.1.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs=
 github.com/google/pprof v0.0.0-20181206194817-3ea8567a2e57/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
-github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaUGG7oYTSPP8MxqL4YI3kZKwcP4=
-github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ=
 github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/google/uuid v1.1.1 h1:Gkbcsh/GbpXz7lPftLA3P6TYMwjCLYm83jiFQZF/3gY=
 github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=

diff --git a/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go
@@ -47,7 +47,6 @@ type Command struct {
 	flagWriteServiceDefaults bool   // True to enable central config injection
 	flagDefaultProtocol      string // Default protocol for use with central config
 	flagConsulCACert         string // [Deprecated] Path to CA Certificate to use when communicating with Consul clients
-	flagEnvoyExtraArgs       string // Extra envoy args when starting envoy
 
 	// Flags to support namespaces
 	flagEnableNamespaces           bool     // Use namespacing on all components
@@ -110,8 +109,6 @@ func (c *Command) init() {
 		"Docker image for Envoy. Defaults to envoyproxy/envoy-alpine:v1.13.0.")
 	c.flagSet.StringVar(&c.flagConsulK8sImage, "consul-k8s-image", "",
 		"Docker image for consul-k8s. Used for the connect sidecar.")
-	c.flagSet.StringVar(&c.flagEnvoyExtraArgs, "envoy-extra-args", "",
-		"Extra envoy command line args to be set when starting envoy (e.g \"--log-level debug --disable-hot-restart\").")
 	c.flagSet.StringVar(&c.flagACLAuthMethod, "acl-auth-method", "",
 		"The name of the Kubernetes Auth Method to use for connectInjection if ACLs are enabled.")
 	c.flagSet.BoolVar(&c.flagWriteServiceDefaults, "enable-central-config", false,
@@ -308,7 +305,6 @@ func (c *Command) Run(args []string) int {
 		ConsulClient:               c.consulClient,
 		ImageConsul:                c.flagConsulImage,
 		ImageEnvoy:                 c.flagEnvoyImage,
-		EnvoyExtraArgs:             c.flagEnvoyExtraArgs,
 		ImageConsulK8S:             c.flagConsulK8sImage,
 		RequireAnnotation:          !c.flagDefaultInject,
 		AuthMethod:                 c.flagACLAuthMethod,