Don't set security context when on OpenShift (#521)

* Envoy container should not have security context set when on OpenShift and when tproxy is disabled because we want to let OpenShift set a random user/group. When tproxy is enabled though, we need to still set it on OpenShift because we require Envoy to have a specific user for the traffic redirection to work. * init-copy-container doesn't need to have security context set whenever running on OpenShift (regardless of whether tproxy is enabled or not) so that OpenShift can set its own random user/group.
hashicorp · May 25, 2021 · 0bc1f55 · 0bc1f55
1 parent 7ad32ba
commit 0bc1f55
Show file tree

Hide file tree

Showing 7 changed files with 153 additions and 42 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,8 @@
 IMPROVEMENTS:
 * Connect: Overwrite Kubernetes HTTP readiness and/or liveness probes to point to Envoy proxy when
   transparent proxy is enabled. [[GH-517](https://github.com/hashicorp/consul-k8s/pull/517)]
+* Connect: Don't set security context for the Envoy proxy when on OpenShift and transparent proxy is disabled.
+  [[GH-521](https://github.com/hashicorp/consul-k8s/pull/521)]
 
 BUG FIXES:
 * Connect: Process every Address in an Endpoints object before returning an error. This ensures an address that isn't reconciled successfully doesn't prevent the remaining addresses from getting reconciled. [[GH-519](https://github.com/hashicorp/consul-k8s/pull/519)]

diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go
@@ -64,12 +64,12 @@ type initContainerCommandData struct {
 	TProxyExcludeUIDs []string
 }
 
-// containerInitCopyContainer returns the init container spec for the copy container which places
+// initCopyContainer returns the init container spec for the copy container which places
 // the consul binary into the shared volume.
-func (h *Handler) containerInitCopyContainer() corev1.Container {
+func (h *Handler) initCopyContainer() corev1.Container {
 	// Copy the Consul binary from the image to the shared volume.
 	cmd := "cp /bin/consul /consul/connect-inject/consul"
-	return corev1.Container{
+	container := corev1.Container{
 		Name:      InjectInitCopyContainerName,
 		Image:     h.ImageConsul,
 		Resources: h.InitContainerResources,
@@ -80,14 +80,18 @@ func (h *Handler) containerInitCopyContainer() corev1.Container {
 			},
 		},
 		Command: []string{"/bin/sh", "-ec", cmd},
-		SecurityContext: &corev1.SecurityContext{
+	}
+	// If running on OpenShift, don't set the security context and instead let OpenShift set a random user/group for us.
+	if !h.EnableOpenShift {
+		container.SecurityContext = &corev1.SecurityContext{
 			// Set RunAsUser because the default user for the consul container is root and we want to run non-root.
 			RunAsUser:              pointerToInt64(copyContainerUserAndGroupID),
 			RunAsGroup:             pointerToInt64(copyContainerUserAndGroupID),
 			RunAsNonRoot:           pointerToBool(true),
 			ReadOnlyRootFilesystem: pointerToBool(true),
-		},
+		}
 	}
+	return container
 }
 
 // containerInit returns the init container spec for registering the Consul

diff --git a/connect-inject/container_init_test.go b/connect-inject/container_init_test.go
@@ -1,6 +1,7 @@
 package connectinject
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -684,19 +685,31 @@ func TestHandlerContainerInit_Resources(t *testing.T) {
 }
 
 // Test that the init copy container has the correct command and SecurityContext.
-func TestHandlerContainerInitCopyContainer(t *testing.T) {
-	require := require.New(t)
-	h := Handler{}
-	container := h.containerInitCopyContainer()
-	expectedSecurityContext := &corev1.SecurityContext{
-		RunAsUser:              pointerToInt64(copyContainerUserAndGroupID),
-		RunAsGroup:             pointerToInt64(copyContainerUserAndGroupID),
-		RunAsNonRoot:           pointerToBool(true),
-		ReadOnlyRootFilesystem: pointerToBool(true),
+func TestHandlerInitCopyContainer(t *testing.T) {
+	openShiftEnabledCases := []bool{false, true}
+
+	for _, openShiftEnabled := range openShiftEnabledCases {
+		t.Run(fmt.Sprintf("openshift enabled: %t", openShiftEnabled), func(t *testing.T) {
+			h := Handler{EnableOpenShift: openShiftEnabled}
+
+			container := h.initCopyContainer()
+
+			if openShiftEnabled {
+				require.Nil(t, container.SecurityContext)
+			} else {
+				expectedSecurityContext := &corev1.SecurityContext{
+					RunAsUser:              pointerToInt64(copyContainerUserAndGroupID),
+					RunAsGroup:             pointerToInt64(copyContainerUserAndGroupID),
+					RunAsNonRoot:           pointerToBool(true),
+					ReadOnlyRootFilesystem: pointerToBool(true),
+				}
+				require.Equal(t, expectedSecurityContext, container.SecurityContext)
+			}
+
+			actual := strings.Join(container.Command, " ")
+			require.Contains(t, actual, `cp /bin/consul /consul/connect-inject/consul`)
+		})
 	}
-	require.Equal(container.SecurityContext, expectedSecurityContext)
-	actual := strings.Join(container.Command, " ")
-	require.Contains(actual, `cp /bin/consul /consul/connect-inject/consul`)
 }
 
 var testNS = corev1.Namespace{

diff --git a/connect-inject/envoy_sidecar.go b/connect-inject/envoy_sidecar.go
@@ -10,7 +10,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 )
 
-func (h *Handler) envoySidecar(pod corev1.Pod) (corev1.Container, error) {
+func (h *Handler) envoySidecar(namespace corev1.Namespace, pod corev1.Pod) (corev1.Container, error) {
 	resources, err := h.envoySidecarResources(pod)
 	if err != nil {
 		return corev1.Container{}, err
@@ -21,21 +21,6 @@ func (h *Handler) envoySidecar(pod corev1.Pod) (corev1.Container, error) {
 		return corev1.Container{}, err
 	}
 
-	if pod.Spec.SecurityContext != nil {
-		// User container and Envoy container cannot have the same UID.
-		if pod.Spec.SecurityContext.RunAsUser != nil && *pod.Spec.SecurityContext.RunAsUser == envoyUserAndGroupID {
-			return corev1.Container{}, fmt.Errorf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID)
-		}
-	}
-	// Ensure that none of the user's containers have the same UID as Envoy. At this point in injection the handler
-	// has only injected init containers so all containers defined in pod.Spec.Containers are from the user.
-	for _, c := range pod.Spec.Containers {
-		// User container and Envoy container cannot have the same UID.
-		if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil && *c.SecurityContext.RunAsUser == envoyUserAndGroupID {
-			return corev1.Container{}, fmt.Errorf("container %q has runAsUser set to the same uid %q as envoy which is not allowed", c.Name, envoyUserAndGroupID)
-		}
-	}
-
 	container := corev1.Container{
 		Name:  "envoy-sidecar",
 		Image: h.ImageEnvoy,
@@ -55,13 +40,40 @@ func (h *Handler) envoySidecar(pod corev1.Pod) (corev1.Container, error) {
 			},
 		},
 		Command: cmd,
-		SecurityContext: &corev1.SecurityContext{
+	}
+
+	tproxyEnabled, err := transparentProxyEnabled(namespace, pod, h.EnableTransparentProxy)
+	if err != nil {
+		return corev1.Container{}, err
+	}
+
+	// If not running in transparent proxy mode and in an OpenShift environment,
+	// skip setting the security context and let OpenShift set it for us.
+	// When transparent proxy is enabled, then Envoy needs to run as our specific user
+	// so that traffic redirection will work.
+	if tproxyEnabled || !h.EnableOpenShift {
+		if pod.Spec.SecurityContext != nil {
+			// User container and Envoy container cannot have the same UID.
+			if pod.Spec.SecurityContext.RunAsUser != nil && *pod.Spec.SecurityContext.RunAsUser == envoyUserAndGroupID {
+				return corev1.Container{}, fmt.Errorf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID)
+			}
+		}
+		// Ensure that none of the user's containers have the same UID as Envoy. At this point in injection the handler
+		// has only injected init containers so all containers defined in pod.Spec.Containers are from the user.
+		for _, c := range pod.Spec.Containers {
+			// User container and Envoy container cannot have the same UID.
+			if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil && *c.SecurityContext.RunAsUser == envoyUserAndGroupID {
+				return corev1.Container{}, fmt.Errorf("container %q has runAsUser set to the same uid %q as envoy which is not allowed", c.Name, envoyUserAndGroupID)
+			}
+		}
+		container.SecurityContext = &corev1.SecurityContext{
 			RunAsUser:              pointerToInt64(envoyUserAndGroupID),
 			RunAsGroup:             pointerToInt64(envoyUserAndGroupID),
 			RunAsNonRoot:           pointerToBool(true),
 			ReadOnlyRootFilesystem: pointerToBool(true),
-		},
+		}
 	}
+
 	return container, nil
 }
 func (h *Handler) getContainerSidecarCommand(pod corev1.Pod) ([]string, error) {

diff --git a/connect-inject/envoy_sidecar_test.go b/connect-inject/envoy_sidecar_test.go
@@ -28,7 +28,7 @@ func TestHandlerEnvoySidecar(t *testing.T) {
 			},
 		},
 	}
-	container, err := h.envoySidecar(pod)
+	container, err := h.envoySidecar(testNS, pod)
 	require.NoError(err)
 	require.Equal(container.Command, []string{
 		"envoy",
@@ -43,6 +43,76 @@ func TestHandlerEnvoySidecar(t *testing.T) {
 	})
 }
 
+func TestHandlerEnvoySidecar_withSecurityContext(t *testing.T) {
+	cases := map[string]struct {
+		tproxyEnabled      bool
+		openShiftEnabled   bool
+		expSecurityContext *corev1.SecurityContext
+	}{
+		"tproxy disabled; openshift disabled": {
+			tproxyEnabled:    false,
+			openShiftEnabled: false,
+			expSecurityContext: &corev1.SecurityContext{
+				RunAsUser:              pointerToInt64(envoyUserAndGroupID),
+				RunAsGroup:             pointerToInt64(envoyUserAndGroupID),
+				RunAsNonRoot:           pointerToBool(true),
+				ReadOnlyRootFilesystem: pointerToBool(true),
+			},
+		},
+		"tproxy enabled; openshift disabled": {
+			tproxyEnabled:    true,
+			openShiftEnabled: false,
+			expSecurityContext: &corev1.SecurityContext{
+				RunAsUser:              pointerToInt64(envoyUserAndGroupID),
+				RunAsGroup:             pointerToInt64(envoyUserAndGroupID),
+				RunAsNonRoot:           pointerToBool(true),
+				ReadOnlyRootFilesystem: pointerToBool(true),
+			},
+		},
+		"tproxy disabled; openshift enabled": {
+			tproxyEnabled:      false,
+			openShiftEnabled:   true,
+			expSecurityContext: nil,
+		},
+		"tproxy enabled; openshift enabled": {
+			tproxyEnabled:    true,
+			openShiftEnabled: true,
+			expSecurityContext: &corev1.SecurityContext{
+				RunAsUser:              pointerToInt64(envoyUserAndGroupID),
+				RunAsGroup:             pointerToInt64(envoyUserAndGroupID),
+				RunAsNonRoot:           pointerToBool(true),
+				ReadOnlyRootFilesystem: pointerToBool(true),
+			},
+		},
+	}
+	for name, c := range cases {
+		t.Run(name, func(t *testing.T) {
+			h := Handler{
+				EnableTransparentProxy: c.tproxyEnabled,
+				EnableOpenShift:        c.openShiftEnabled,
+			}
+			pod := corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						annotationService: "foo",
+					},
+				},
+
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "web",
+						},
+					},
+				},
+			}
+			ec, err := h.envoySidecar(testNS, pod)
+			require.NoError(t, err)
+			require.Equal(t, c.expSecurityContext, ec.SecurityContext)
+		})
+	}
+}
+
 // Test that if the user specifies a pod security context with the same uid as `envoyUserAndGroupID` that we return
 // an error to the handler.
 func TestHandlerEnvoySidecar_FailsWithDuplicatePodSecurityContextUID(t *testing.T) {
@@ -60,7 +130,7 @@ func TestHandlerEnvoySidecar_FailsWithDuplicatePodSecurityContextUID(t *testing.
 			},
 		},
 	}
-	_, err := h.envoySidecar(pod)
+	_, err := h.envoySidecar(testNS, pod)
 	require.Error(err, fmt.Sprintf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID))
 }
 
@@ -89,7 +159,7 @@ func TestHandlerEnvoySidecar_FailsWithDuplicateContainerSecurityContextUID(t *te
 			},
 		},
 	}
-	_, err := h.envoySidecar(pod)
+	_, err := h.envoySidecar(testNS, pod)
 	require.Error(err, fmt.Sprintf("container %q has runAsUser set to the same uid %q as envoy which is not allowed", pod.Spec.Containers[1].Name, envoyUserAndGroupID))
 }
 
@@ -177,7 +247,7 @@ func TestHandlerEnvoySidecar_EnvoyExtraArgs(t *testing.T) {
 				EnvoyExtraArgs: tc.envoyExtraArgs,
 			}
 
-			c, err := h.envoySidecar(*tc.pod)
+			c, err := h.envoySidecar(testNS, *tc.pod)
 			require.NoError(t, err)
 			require.Equal(t, tc.expectedContainerCommand, c.Command)
 		})
@@ -351,7 +421,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) {
 					},
 				},
 			}
-			container, err := c.handler.envoySidecar(pod)
+			container, err := c.handler.envoySidecar(testNS, pod)
 			if c.expErr != "" {
 				require.NotNil(err)
 				require.Contains(err.Error(), c.expErr)

diff --git a/connect-inject/handler.go b/connect-inject/handler.go
@@ -134,6 +134,11 @@ type Handler struct {
 	// to point them to the Envoy proxy.
 	TProxyOverwriteProbes bool
 
+	// EnableOpenShift indicates that when tproxy is enabled, the security context for the Envoy and init
+	// containers should not be added because OpenShift sets a random user for those and will not allow
+	// those containers to be created otherwise.
+	EnableOpenShift bool
+
 	// Log
 	Log logr.Logger
 
@@ -199,7 +204,7 @@ func (h *Handler) Handle(ctx context.Context, req admission.Request) admission.R
 	}
 
 	// Add the init container which copies the Consul binary to /consul/connect-inject/.
-	initCopyContainer := h.containerInitCopyContainer()
+	initCopyContainer := h.initCopyContainer()
 	pod.Spec.InitContainers = append(pod.Spec.InitContainers, initCopyContainer)
 
 	// A user can enable/disable tproxy for an entire namespace via a label.
@@ -218,7 +223,7 @@ func (h *Handler) Handle(ctx context.Context, req admission.Request) admission.R
 	pod.Spec.InitContainers = append(pod.Spec.InitContainers, initContainer)
 
 	// Add the Envoy sidecar.
-	envoySidecar, err := h.envoySidecar(pod)
+	envoySidecar, err := h.envoySidecar(*ns, pod)
 	if err != nil {
 		h.Log.Error(err, "error configuring injection sidecar container", "request name", req.Name)
 		return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error configuring injection sidecar container: %s", err))

diff --git a/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go
@@ -92,6 +92,8 @@ type Command struct {
 	flagDefaultEnableTransparentProxy          bool
 	flagTransparentProxyDefaultOverwriteProbes bool
 
+	flagEnableOpenShift bool
+
 	flagSet *flag.FlagSet
 	http    *flags.HTTPFlags
 
@@ -158,6 +160,8 @@ func (c *Command) init() {
 		"Enable transparent proxy mode for all Consul service mesh applications by default.")
 	c.flagSet.BoolVar(&c.flagTransparentProxyDefaultOverwriteProbes, "transparent-proxy-default-overwrite-probes", true,
 		"Overwrite Kubernetes probes to point to Envoy by default when in Transparent Proxy mode.")
+	c.flagSet.BoolVar(&c.flagEnableOpenShift, "enable-openshift", false,
+		"Indicates that the command runs in an OpenShift cluster.")
 	c.flagSet.StringVar(&c.flagLogLevel, "log-level", zapcore.InfoLevel.String(),
 		fmt.Sprintf("Log verbosity level. Supported values (in order of detail) are "+
 			"%q, %q, %q, and %q.", zapcore.DebugLevel.String(), zapcore.InfoLevel.String(), zapcore.WarnLevel.String(), zapcore.ErrorLevel.String()))
@@ -447,6 +451,7 @@ func (c *Command) Run(args []string) int {
 			CrossNamespaceACLPolicy:    c.flagCrossNamespaceACLPolicy,
 			EnableTransparentProxy:     c.flagDefaultEnableTransparentProxy,
 			TProxyOverwriteProbes:      c.flagTransparentProxyDefaultOverwriteProbes,
+			EnableOpenShift:            c.flagEnableOpenShift,
 			Log:                        ctrl.Log.WithName("handler").WithName("connect"),
 		}})