Skip to content
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

kubectl debug: add netadmin profile #115712

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go
Expand Up @@ -190,7 +190,7 @@ func (o *DebugOptions) AddFlags(cmd *cobra.Command) {
cmd.Flags().BoolVar(&o.ShareProcesses, "share-processes", o.ShareProcesses, i18n.T("When used with '--copy-to', enable process namespace sharing in the copy."))
cmd.Flags().StringVar(&o.TargetContainer, "target", "", i18n.T("When using an ephemeral container, target processes in this container name."))
cmd.Flags().BoolVarP(&o.TTY, "tty", "t", o.TTY, i18n.T("Allocate a TTY for the debugging container."))
cmd.Flags().StringVar(&o.Profile, "profile", ProfileLegacy, i18n.T(`Debugging profile. Options are "legacy", "general", "baseline", or "restricted".`))
cmd.Flags().StringVar(&o.Profile, "profile", ProfileLegacy, i18n.T(`Debugging profile. Options are "legacy", "general", "baseline", "netadmin", or "restricted".`))
}

// Complete finishes run-time initialization of debug.DebugOptions.
Expand Down
21 changes: 21 additions & 0 deletions staging/src/k8s.io/kubectl/pkg/cmd/debug/debug_test.go
Expand Up @@ -294,6 +294,27 @@ func TestGenerateDebugContainer(t *testing.T) {
},
},
},
{
name: "netadmin profile",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we need to add unit tests also in TestGenerateNodeDebugPod and TestGeneratePodCopyWithDebugContainer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in followup PR: #115839

opts: &DebugOptions{
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileNetadmin,
},
expected: &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: "debugger-1",
Image: "busybox",
ImagePullPolicy: corev1.PullIfNotPresent,
TerminationMessagePolicy: corev1.TerminationMessageReadFile,
SecurityContext: &corev1.SecurityContext{
Capabilities: &corev1.Capabilities{
Add: []corev1.Capability{"NET_ADMIN"},
},
},
},
},
},
} {
t.Run(tc.name, func(t *testing.T) {
tc.opts.IOStreams = genericclioptions.NewTestIOStreamsDiscard()
Expand Down
68 changes: 62 additions & 6 deletions staging/src/k8s.io/kubectl/pkg/cmd/debug/profiles.go
Expand Up @@ -52,6 +52,8 @@ const (
// ProfileRestricted is identical to "baseline" but adds configuration that's required
// under the restricted security profile, such as requiring a non-root user and dropping all capabilities.
ProfileRestricted = "restricted"
// ProfileNetadmin offers elevated privileges for network debugging.
ProfileNetadmin = "netadmin"
)

type ProfileApplier interface {
Expand All @@ -70,6 +72,8 @@ func NewProfileApplier(profile string) (ProfileApplier, error) {
return &baselineProfile{}, nil
case ProfileRestricted:
return &restrictedProfile{}, nil
case ProfileNetadmin:
return &netadminProfile{}, nil
}

return nil, fmt.Errorf("unknown profile: %s", profile)
Expand All @@ -87,6 +91,9 @@ type baselineProfile struct {
type restrictedProfile struct {
}

type netadminProfile struct {
}

func (p *legacyProfile) Apply(pod *corev1.Pod, containerName string, target runtime.Object) error {
switch target.(type) {
case *corev1.Pod:
Expand Down Expand Up @@ -183,6 +190,26 @@ func (p *restrictedProfile) Apply(pod *corev1.Pod, containerName string, target
return nil
}

func (p *netadminProfile) Apply(pod *corev1.Pod, containerName string, target runtime.Object) error {
style, err := getDebugStyle(pod, target)
if err != nil {
return fmt.Errorf("netadmin profile: %s", err)
}

allowNetadminCapability(pod, containerName)

switch style {
case node:
useHostNamespaces(pod)
setPrivileged(pod, containerName)

case podCopy, ephemeral:
// no additional modifications needed
}

return nil
}

// removeLabelsAndProbes removes labels from the pod and remove probes
// from all containers of the pod.
func removeLabelsAndProbes(p *corev1.Pod) {
Expand Down Expand Up @@ -239,6 +266,20 @@ func clearSecurityContext(p *corev1.Pod, containerName string) {
})
}

// setPrivileged configures the containers as privileged.
func setPrivileged(p *corev1.Pod, containerName string) {
podutils.VisitContainers(&p.Spec, podutils.AllContainers, func(c *corev1.Container, _ podutils.ContainerType) bool {
if c.Name != containerName {
return true
}
if c.SecurityContext == nil {
c.SecurityContext = &corev1.SecurityContext{}
}
c.SecurityContext.Privileged = pointer.BoolPtr(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: BoolPtr is deprecated, pointer.Bool is recommended.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

followup PR: #115837

return false
})
}

// disallowRoot configures the container to run as a non-root user.
func disallowRoot(p *corev1.Pod, containerName string) {
podutils.VisitContainers(&p.Spec, podutils.AllContainers, func(c *corev1.Container, _ podutils.ContainerType) bool {
Expand Down Expand Up @@ -274,13 +315,28 @@ func allowProcessTracing(p *corev1.Pod, containerName string) {
if c.Name != containerName {
return true
}
if c.SecurityContext == nil {
c.SecurityContext = &corev1.SecurityContext{}
}
if c.SecurityContext.Capabilities == nil {
c.SecurityContext.Capabilities = &corev1.Capabilities{}
addCapability(c, "SYS_PTRACE")
return false
})
}

// allowNetadminCapability grants NET_ADMIN capability to the container.
func allowNetadminCapability(p *corev1.Pod, containerName string) {
podutils.VisitContainers(&p.Spec, podutils.AllContainers, func(c *corev1.Container, _ podutils.ContainerType) bool {
if c.Name != containerName {
return true
}
c.SecurityContext.Capabilities.Add = append(c.SecurityContext.Capabilities.Add, "SYS_PTRACE")
addCapability(c, "NET_ADMIN")
return false
})
}

func addCapability(c *corev1.Container, capability corev1.Capability) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for factoring this out into its own function. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I'm wondering if we even need allowNetadminCapability() and allowProcessTracing(), or if we should just be using allowCapability(pod, containerName, capability) for both, but then I decided it's probably easier to make these decisions after all of the profiles have been added.

if c.SecurityContext == nil {
c.SecurityContext = &corev1.SecurityContext{}
}
if c.SecurityContext.Capabilities == nil {
c.SecurityContext.Capabilities = &corev1.Capabilities{}
}
c.SecurityContext.Capabilities.Add = append(c.SecurityContext.Capabilities.Add, capability)
}
138 changes: 138 additions & 0 deletions staging/src/k8s.io/kubectl/pkg/cmd/debug/profiles_test.go
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package debug

import (
"fmt"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -430,3 +431,140 @@ func TestRestrictedProfile(t *testing.T) {
})
}
}

func TestNetAdminProfile(t *testing.T) {
pod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "pod"},
Spec: corev1.PodSpec{EphemeralContainers: []corev1.EphemeralContainer{
{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: "dbg", Image: "dbgimage",
},
},
}},
}

tests := []struct {
name string
pod *corev1.Pod
containerName string
target runtime.Object
expectPod *corev1.Pod
expectErr error
}{
{
name: "nil target",
pod: pod,
containerName: "dbg",
target: nil,
expectErr: fmt.Errorf("netadmin profile: objects of type <nil> are not supported"),
},
{
name: "debug by ephemeral container",
pod: pod,
containerName: "dbg",
target: pod,
expectPod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "pod"},
Spec: corev1.PodSpec{EphemeralContainers: []corev1.EphemeralContainer{
{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: "dbg", Image: "dbgimage",
SecurityContext: &corev1.SecurityContext{
Capabilities: &corev1.Capabilities{
Add: []corev1.Capability{"NET_ADMIN"},
},
},
},
},
}},
},
},
{
name: "debug by pod copy",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "podcopy"},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{Name: "app", Image: "appimage"},
{Name: "dbg", Image: "dbgimage"},
},
},
},
containerName: "dbg",
target: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "podcopy"},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{Name: "app", Image: "appimage"},
},
},
},
expectPod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "podcopy"},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{Name: "app", Image: "appimage"},
{
Name: "dbg",
Image: "dbgimage",
SecurityContext: &corev1.SecurityContext{
Capabilities: &corev1.Capabilities{
Add: []corev1.Capability{"NET_ADMIN"},
},
},
},
},
},
},
},
{
name: "debug by node",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "pod"},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{Name: "dbg", Image: "dbgimage"},
},
},
},
containerName: "dbg",
target: testNode,
expectPod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "pod"},
Spec: corev1.PodSpec{
HostNetwork: true,
HostPID: true,
HostIPC: true,
Containers: []corev1.Container{
{
Name: "dbg",
Image: "dbgimage",
SecurityContext: &corev1.SecurityContext{
Privileged: pointer.BoolPtr(true),
Capabilities: &corev1.Capabilities{
Add: []corev1.Capability{"NET_ADMIN"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could you please add a test case to cover that already existed capability in the container is also shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in followup PR: #115839

},
},
},
},
},
},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
err := (&netadminProfile{}).Apply(test.pod, test.containerName, test.target)
if (err == nil) != (test.expectErr == nil) || (err != nil && test.expectErr != nil && err.Error() != test.expectErr.Error()) {
t.Fatalf("expect error: %v, got error: %v", test.expectErr, err)
}
if err != nil {
return
}
if diff := cmp.Diff(test.expectPod, test.pod); diff != "" {
t.Error("unexpected diff in generated object: (-want +got):\n", diff)
}
})
}
}