-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use rootless containers where possible #493
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,10 +192,11 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { | |
Capabilities: &corev1.Capabilities{ | ||
Add: []corev1.Capability{netAdminCapability}, | ||
}, | ||
RunAsNonRoot: pointerToBool(false), | ||
} | ||
expectedCmd := `/consul/connect-inject/consul connect redirect-traffic \ | ||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-proxy-uid=0` | ||
-proxy-uid=5995` | ||
container, err := h.containerInit(*pod, k8sNamespace) | ||
require.NoError(t, err) | ||
actualCmd := strings.Join(container.Command, " ") | ||
|
@@ -248,7 +249,6 @@ func TestHandlerContainerInit_namespacesEnabled(t *testing.T) { | |
Pod func(*corev1.Pod) *corev1.Pod | ||
Handler Handler | ||
Cmd string // Strings.Contains test | ||
CmdNot string // Not contains | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused in this test. |
||
}{ | ||
{ | ||
"whole template, default namespace", | ||
|
@@ -271,7 +271,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-namespace="default" \ | ||
-bootstrap > /consul/connect-inject/envoy-bootstrap.yaml`, | ||
"", | ||
}, | ||
|
||
{ | ||
|
@@ -295,7 +294,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-namespace="non-default" \ | ||
-bootstrap > /consul/connect-inject/envoy-bootstrap.yaml`, | ||
"", | ||
}, | ||
|
||
{ | ||
|
@@ -325,7 +323,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
-token-file="/consul/connect-inject/acl-token" \ | ||
-namespace="non-default" \ | ||
-bootstrap > /consul/connect-inject/envoy-bootstrap.yaml`, | ||
"", | ||
}, | ||
{ | ||
"Whole template, auth method, non-default namespace, mirroring enabled", | ||
|
@@ -355,7 +352,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
-token-file="/consul/connect-inject/acl-token" \ | ||
-namespace="k8snamespace" \ | ||
-bootstrap > /consul/connect-inject/envoy-bootstrap.yaml`, | ||
"", | ||
}, | ||
{ | ||
"whole template, default namespace, tproxy enabled", | ||
|
@@ -384,8 +380,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
/consul/connect-inject/consul connect redirect-traffic \ | ||
-namespace="default" \ | ||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-proxy-uid=0`, | ||
"", | ||
-proxy-uid=5995`, | ||
}, | ||
|
||
{ | ||
|
@@ -415,8 +410,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
/consul/connect-inject/consul connect redirect-traffic \ | ||
-namespace="non-default" \ | ||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-proxy-uid=0`, | ||
"", | ||
-proxy-uid=5995`, | ||
}, | ||
|
||
{ | ||
|
@@ -453,8 +447,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
/consul/connect-inject/consul connect redirect-traffic \ | ||
-namespace="k8snamespace" \ | ||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-proxy-uid=0`, | ||
"", | ||
-proxy-uid=5995`, | ||
}, | ||
} | ||
|
||
|
@@ -463,14 +456,10 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
require := require.New(t) | ||
|
||
h := tt.Handler | ||
|
||
container, err := h.containerInit(*tt.Pod(minimal()), k8sNamespace) | ||
require.NoError(err) | ||
actual := strings.Join(container.Command, " ") | ||
require.Equal(actual, tt.Cmd) | ||
if tt.CmdNot != "" { | ||
require.NotContains(actual, tt.CmdNot) | ||
} | ||
require.Equal(tt.Cmd, actual) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🍺 |
||
}) | ||
} | ||
} | ||
|
@@ -598,16 +587,18 @@ func TestHandlerContainerInit_Resources(t *testing.T) { | |
}, container.Resources) | ||
} | ||
|
||
// Test that the init copy container has the correct command. | ||
// 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), | ||
} | ||
require.Equal(container.SecurityContext, expectedSecurityContext) | ||
actual := strings.Join(container.Command, " ") | ||
require.Contains(actual, `cp /bin/consul /consul/connect-inject/consul`) | ||
} | ||
|
||
// pointerToBool takes a boolean and returns a pointer to it. | ||
func pointerToBool(b bool) *bool { | ||
return &b | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package connectinject | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
@@ -27,7 +28,7 @@ func TestHandlerEnvoySidecar(t *testing.T) { | |
}, | ||
}, | ||
} | ||
container, err := h.envoySidecar(pod, k8sNamespace) | ||
container, err := h.envoySidecar(pod) | ||
require.NoError(err) | ||
require.Equal(container.Command, []string{ | ||
"envoy", | ||
|
@@ -42,6 +43,56 @@ func TestHandlerEnvoySidecar(t *testing.T) { | |
}) | ||
} | ||
|
||
// 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) { | ||
require := require.New(t) | ||
h := Handler{} | ||
pod := corev1.Pod{ | ||
Spec: corev1.PodSpec{ | ||
Containers: []corev1.Container{ | ||
{ | ||
Name: "web", | ||
}, | ||
}, | ||
SecurityContext: &corev1.PodSecurityContext{ | ||
RunAsUser: pointerToInt64(envoyUserAndGroupID), | ||
}, | ||
}, | ||
} | ||
_, err := h.envoySidecar(pod) | ||
require.Error(err, fmt.Sprintf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID)) | ||
} | ||
|
||
// Test that if the user specifies a container with security context with the same uid as `envoyUserAndGroupID` | ||
// that we return an error to the handler. | ||
func TestHandlerEnvoySidecar_FailsWithDuplicateContainerSecurityContextUID(t *testing.T) { | ||
require := require.New(t) | ||
h := Handler{} | ||
pod := corev1.Pod{ | ||
Spec: corev1.PodSpec{ | ||
Containers: []corev1.Container{ | ||
{ | ||
Name: "web", | ||
// Setting RunAsUser: 1 should succeed. | ||
SecurityContext: &corev1.SecurityContext{ | ||
RunAsUser: pointerToInt64(1), | ||
}, | ||
}, | ||
{ | ||
Name: "app", | ||
// Setting RunAsUser: 5995 should fail. | ||
SecurityContext: &corev1.SecurityContext{ | ||
RunAsUser: pointerToInt64(envoyUserAndGroupID), | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
_, err := h.envoySidecar(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)) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 good test coverage. |
||
// 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. | ||
|
@@ -126,7 +177,7 @@ func TestHandlerEnvoySidecar_EnvoyExtraArgs(t *testing.T) { | |
EnvoyExtraArgs: tc.envoyExtraArgs, | ||
} | ||
|
||
c, err := h.envoySidecar(*tc.pod, k8sNamespace) | ||
c, err := h.envoySidecar(*tc.pod) | ||
require.NoError(t, err) | ||
require.Equal(t, tc.expectedContainerCommand, c.Command) | ||
}) | ||
|
@@ -300,7 +351,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { | |
}, | ||
}, | ||
} | ||
container, err := c.handler.envoySidecar(pod, k8sNamespace) | ||
container, err := c.handler.envoySidecar(pod) | ||
if c.expErr != "" { | ||
require.NotNil(err) | ||
require.Contains(err.Error(), c.expErr) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,8 +191,7 @@ func (h *Handler) Handle(_ context.Context, req admission.Request) admission.Res | |
container.Env = append(container.Env, containerEnvVars...) | ||
} | ||
|
||
// TODO: rename both of these initcontainers appropriately | ||
// Add the consul-init container | ||
// Add the init container which copies the Consul binary to /consul/connect-inject/. | ||
initCopyContainer := h.containerInitCopyContainer() | ||
pod.Spec.InitContainers = append(pod.Spec.InitContainers, initCopyContainer) | ||
|
||
|
@@ -205,8 +204,8 @@ func (h *Handler) Handle(_ context.Context, req admission.Request) admission.Res | |
} | ||
pod.Spec.InitContainers = append(pod.Spec.InitContainers, initContainer) | ||
|
||
// Add the Envoy and Consul sidecars. | ||
envoySidecar, err := h.envoySidecar(pod, req.Namespace) | ||
// Add the Envoy sidecar. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙏 |
||
envoySidecar, err := h.envoySidecar(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)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also call out that we will error out if you have the same user as envoy as a breaking change. Unless we want to error out only when tproxy is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point. But now I have a FEATURE + BREAKING CHANGE + BUG FIX entry in the changelog against the same PR, can we consolidate this somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yes that's true. Maybe change the non-bug fix to a breaking change and add a note about what's breaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ishustava how does this look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!