kubectl debug: add netadmin profile #115712

wedaly · 2023-02-12T17:21:07Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add the netadmin profile from KEP 1441
https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/1441-kubectl-debug#debugging-profiles

Which issue(s) this PR fixes:

xref kubernetes/kubectl#1108

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added "netadmin" debugging profiles for kubectl debug.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

KEP 1441

k8s-ci-robot · 2023-02-12T17:21:16Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2023-02-12T17:21:16Z

Hi @wedaly. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wedaly · 2023-02-12T17:22:58Z

/cc @verb @sding3

k8s-ci-robot · 2023-02-12T17:23:00Z

@wedaly: GitHub didn't allow me to request PR reviews from the following users: sding3.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @verb @sding3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sding3

Cross referencing the KEP, the profiles LGTM.

Left one nit on the placement of the allowNetadminCapability call.

sding3 · 2023-02-12T20:33:49Z

staging/src/k8s.io/kubectl/pkg/cmd/debug/profiles.go

 		return false
 	})
 }
+
+func addCapability(c *corev1.Container, capability corev1.Capability) {


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

+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.

sding3 · 2023-02-12T20:37:58Z

staging/src/k8s.io/kubectl/pkg/cmd/debug/profiles.go

+	switch style {
+	case node:
+		allowNetadminCapability(pod, containerName)
+		useHostNamespaces(pod)
+		setPrivileged(pod, containerName)
+
+	case podCopy:
+		allowNetadminCapability(pod, containerName)
+
+	case ephemeral:
+		allowNetadminCapability(pod, containerName)
+	}


Looks like allowNetadminCapability is used in all three debugging styles, so we could shift it up to before the switch:

Suggested change

switch style {

case node:

allowNetadminCapability(pod, containerName)

useHostNamespaces(pod)

setPrivileged(pod, containerName)

case podCopy:

allowNetadminCapability(pod, containerName)

case ephemeral:

allowNetadminCapability(pod, containerName)

}

allowNetadminCapability(pod, containerName)

switch style {

case node:

useHostNamespaces(pod)

setPrivileged(pod, containerName)

case podCopy, ephemeral:

// no additional modifications needed

}

verb

/lgtm
/ok-to-test

verb · 2023-02-13T15:57:46Z

staging/src/k8s.io/kubectl/pkg/cmd/debug/profiles.go

 		return false
 	})
 }
+
+func addCapability(c *corev1.Container, capability corev1.Capability) {


+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.

k8s-ci-robot · 2023-02-13T16:00:48Z

LGTM label has been added.

Git tree hash: 937ab9e471f94e9d7a93e463069952069146b6d3

wedaly · 2023-02-13T17:11:13Z

/assign @soltysh

Add the netadmin profile from KEP 1441 https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/1441-kubectl-debug#debugging-profiles Signed-off-by: Will Daly <widaly@microsoft.com>

wedaly · 2023-02-13T22:40:21Z

Rebased to address merge conflict.

verb · 2023-02-14T12:06:24Z

@ardaguclu Do you have time to review this for approval?

ardaguclu

Thanks for the new profile. I left a few comments but they are not blocker for this PR and can be done in followup.

/approve

ardaguclu · 2023-02-16T06:24:50Z

staging/src/k8s.io/kubectl/pkg/cmd/debug/profiles.go

+		if c.SecurityContext == nil {
+			c.SecurityContext = &corev1.SecurityContext{}
+		}
+		c.SecurityContext.Privileged = pointer.BoolPtr(true)


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

followup PR: #115837

ardaguclu · 2023-02-16T06:28:41Z

staging/src/k8s.io/kubectl/pkg/cmd/debug/profiles_test.go

+							SecurityContext: &corev1.SecurityContext{
+								Privileged: pointer.BoolPtr(true),
+								Capabilities: &corev1.Capabilities{
+									Add: []corev1.Capability{"NET_ADMIN"},


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

added in followup PR: #115839

ardaguclu · 2023-02-16T06:29:54Z

staging/src/k8s.io/kubectl/pkg/cmd/debug/debug_test.go

@@ -294,6 +294,27 @@ func TestGenerateDebugContainer(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "netadmin profile",


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

added in followup PR: #115839

k8s-ci-robot · 2023-02-16T06:31:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, sding3, verb, wedaly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/kubectl/OWNERS~~ [ardaguclu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ardaguclu · 2023-02-16T06:31:50Z

/lgtm

k8s-ci-robot · 2023-02-16T06:31:56Z

LGTM label has been added.

Git tree hash: 300b30552472fc3748ffd52a2057f853d238a90e

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 12, 2023

k8s-ci-robot requested review from mpuckett159 and soltysh February 12, 2023 17:22

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 12, 2023

k8s-ci-robot requested a review from verb February 12, 2023 17:22

sding3 approved these changes Feb 12, 2023

View reviewed changes

verb approved these changes Feb 13, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 13, 2023

k8s-ci-robot assigned verb Feb 13, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2023

k8s-ci-robot assigned soltysh Feb 13, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2023

k8s-ci-robot requested a review from verb February 13, 2023 22:34

k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Feb 13, 2023

kubectl debug: add netadmin profile

f5095bf

Add the netadmin profile from KEP 1441 https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/1441-kubectl-debug#debugging-profiles Signed-off-by: Will Daly <widaly@microsoft.com>

wedaly force-pushed the kubectl-netadmin-debug-profile branch from 454c846 to f5095bf Compare February 13, 2023 22:40

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Feb 13, 2023

ardaguclu reviewed Feb 16, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2023

k8s-ci-robot assigned ardaguclu Feb 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 16, 2023

k8s-ci-robot merged commit 6a5c88b into kubernetes:master Feb 16, 2023

k8s-ci-robot added this to the v1.27 milestone Feb 16, 2023

wedaly deleted the kubectl-netadmin-debug-profile branch February 16, 2023 14:31

This was referenced Feb 16, 2023

kubectl: use pointer.Bool instead of deprecated pointer.BoolPtr #115837

Merged

kubectl: add unit tests for kubectl debug profiles #115839

Merged

aecay mentioned this pull request Mar 24, 2023

KEP 1441 - kubectl debug kubernetes/enhancements#1441

Closed

4 tasks

This was referenced Jun 14, 2023

Add CAP_NET_RAW to netadmin debugging profile #118647

Merged

Add support for setting security context into ephemeral container specs nilic/kubectl-netshoot#9

Open

mochizuki875 mentioned this pull request Jul 7, 2023

NET_RAW isn't added to ephemeral container via netadmin debugging profile #118962

Closed

kubectl debug: add netadmin profile #115712

kubectl debug: add netadmin profile #115712

Uh oh!

Conversation

wedaly commented Feb 12, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Feb 12, 2023

Uh oh!

k8s-ci-robot commented Feb 12, 2023

Uh oh!

wedaly commented Feb 12, 2023

Uh oh!

k8s-ci-robot commented Feb 12, 2023

Uh oh!

sding3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

verb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 13, 2023

Uh oh!

wedaly commented Feb 13, 2023

Uh oh!

wedaly commented Feb 13, 2023

Uh oh!

verb commented Feb 14, 2023

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 16, 2023

Uh oh!

ardaguclu commented Feb 16, 2023

Uh oh!

k8s-ci-robot commented Feb 16, 2023

Uh oh!

Uh oh!