kubectl-debug: add more debugging profiles #110526

knight42 · 2022-06-12T12:26:21Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

I plan to implement all the remainging debugging profiles listed in kubernetes/kubectl#1108, but I am afraid that the volume of this PR will be too huge, hence I filed a PR with some basic tests to get early feedback from reviewers. If I was on the right track, I would continue to finish my work.

Does this PR introduce a user-facing change?

NONE

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

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/1441-kubectl-debug#debugging-profiles

k8s-ci-robot · 2022-06-12T12:27:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: knight42
To complete the pull request process, please assign apelisse after the PR has been reviewed.
You can assign the PR to them by writing /assign @apelisse in a comment when ready.

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

Needs approval from an approver in each of these files:

staging/src/k8s.io/kubectl/OWNERS

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

knight42 · 2022-06-12T13:01:18Z

/cc @verb
/triage accepted
/priority backlog

k8s-ci-robot · 2022-06-12T13:11:16Z

@knight42: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-kind	d26fc86b153e3b1429d8bdfc822d595f944a1233	link	true	`/test pull-kubernetes-e2e-kind`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

verb · 2022-06-13T09:04:06Z

/assign

knight42 · 2022-06-27T15:54:10Z

@verb Hi! Would you like to have an early review to see if I was on the right track?

verb · 2022-07-08T16:06:02Z

@knight42 sure thing, this is next on my list.

verb

I think this approach is fine. Note that I need to rebuild context about the implementation, so please point out when I've gotten a detail wrong or made a bad assumption.

Is there anything I can do to help with implementation?

verb · 2022-07-08T16:38:48Z

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

+	}
+	return applier
+}
+
 func TestGenerateDebugContainer(t *testing.T) {


I think it makes more sense to test the new profiles individually in a profiles_test.go

Did you mean we should move TestGenerateDebugContainer, TestGeneratePodCopyWithDebugContainer and TestGenerateNodeDebugPod to the new profiles_test.go?

No, I meant that we should create a profiles_test.go to test the methods in profiles.go individually, for example TestGeneralProfileApply, TestBaselineProfileApply, etc. My thinking is that we'll want multiple test cases per profile, so the table in debug_test.go would get really long.

verb · 2022-07-08T16:40:35Z

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

+
+func (p *generalProfile) Apply(pod *corev1.Pod, containerName string, target runtime.Object) error {
+	switch target.(type) {
+	case *corev1.Pod:


For the case of pod we'll want to detect whether pod and target are the same and have different behavior. I don't think we plan on stripping labels for debug by ephemeral container.

hmm I would like to double check, the proposal https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/1441-kubectl-debug#profile-general says that:

Probes and labels are stripped from Pod copies to ensure the copy isn't killed and doesn't receive traffic during debugging.

Do we need to strip labels?

When debugging a pod, there are two outcomes:

Creating a copy of the pod. This should strip labels & probes.

Adding an ephemeral container to an existing pod. This should not change labels & probes.

I should have been more clear in the KEP. I meant for the section you pasted to apply to "Pod copies" (i.e. the first case above)

verb · 2022-07-08T16:40:53Z

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

+				continue
+			}
+			// For copy of pod: sets SYS_PTRACE in debugging container, sets shareProcessNamespace
+			// Probes and labels are stripped from Pod copies.


We'll want helper routines for the common operations, I think.

Signed-off-by: Jian Zeng <[email protected]>

Including `general`, `baseline` and `restricted`. I plan to add more profiles afterwards, but I'd like to get early reviews. Signed-off-by: Jian Zeng <[email protected]>

Signed-off-by: Jian Zeng <[email protected]>

verb · 2022-07-27T13:52:47Z

@knight42 are you still working on this?

knight42 · 2022-07-28T15:40:30Z

It is strange that I cannot reply directly to the comment..

For the case of pod we'll want to detect whether pod and target are the same and have different behavior.

I think we already did it.

If the debug container is found in pod.Spec.Containers then pod is a copy of the target, then we strip labels and probes(BTW we don't copy labels when generating pod copy.

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

Lines 607 to 614 in 79a62d6

    
           func (o *DebugOptions) generatePodCopyWithDebugContainer(pod *corev1.Pod) (*corev1.Pod, string, error) { 
        
           	copied := &corev1.Pod{ 
        
           		ObjectMeta: metav1.ObjectMeta{ 
        
           			Name:        o.CopyTo, 
        
           			Namespace:   pod.Namespace, 
        
           			Annotations: pod.Annotations, 
        
           		}, 
        
           		Spec: *pod.Spec.DeepCopy(),

).

If the debug container is found in pod.Spec.EphemeralContainers, then pod is an existing pod containing the ephermal container. We don't change its labels & probes.

knight42 · 2022-07-28T15:41:39Z

@verb I am still working on it though I might be slow to respond recently due to the busy work 😢

k8s-triage-robot · 2022-10-26T15:51:24Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

sding3 · 2022-11-18T03:32:57Z

@knight42 - are you still working on this ?

I'm looking to use the kubectl debug profile that this PR adds, and I wish to avoid having to maintain a fork. So if you aren't currently working on this, I can pick this up.

knight42 · 2022-11-18T03:38:03Z

@knight42 - are you still working on this ?

I'm looking to use the kubectl debug profile that this PR adds, and I wish to avoid having to maintain a fork. So if you aren't currently working on this, I can pick this up.

Hi, I am unavilable to work on this recently, please feel free to take it.

k8s-ci-robot requested review from brianpursley and seans3 June 12, 2022 12:27

knight42 mentioned this pull request Jun 12, 2022

kubectl debug: allow users to specify debugging profile kubernetes/kubectl#1108

Open

7 tasks

k8s-ci-robot requested a review from verb June 12, 2022 13:02

knight42 marked this pull request as draft June 12, 2022 14:35

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 12, 2022

k8s-ci-robot assigned verb Jun 13, 2022

gautierdelorme mentioned this pull request Jul 4, 2022

debug: ability to disable health check probes on pod copies kubernetes/kubectl#1070

Closed

verb suggested changes Jul 8, 2022

View reviewed changes

knight42 added 4 commits July 14, 2022 21:40

feat(debug): add more profiles

6616c80

Signed-off-by: Jian Zeng <[email protected]>

feat(debug): implment serveral debugging profiles

b215d60

Including `general`, `baseline` and `restricted`. I plan to add more profiles afterwards, but I'd like to get early reviews. Signed-off-by: Jian Zeng <[email protected]>

test: add some basic tests

c8ee3a7

Signed-off-by: Jian Zeng <[email protected]>

chore: add some helper functions

c4a8795

Signed-off-by: Jian Zeng <[email protected]>

knight42 force-pushed the feat/add-more-debugging-profiles branch from d26fc86 to c4a8795 Compare July 14, 2022 14:01

ardaguclu mentioned this pull request Jul 26, 2022

Enhance kubectl debug to set security context kubernetes/kubectl#1252

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 26, 2022

knight42 closed this Nov 18, 2022

sding3 mentioned this pull request Dec 5, 2022

Implement kubectl debug profiles: general, baseline, and restricted #114280

Merged

kubectl-debug: add more debugging profiles #110526

kubectl-debug: add more debugging profiles #110526

Uh oh!

Conversation

knight42 commented Jun 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Jun 12, 2022

Uh oh!

knight42 commented Jun 12, 2022

Uh oh!

k8s-ci-robot commented Jun 12, 2022

Uh oh!

verb commented Jun 13, 2022

Uh oh!

knight42 commented Jun 27, 2022

Uh oh!

verb commented Jul 8, 2022

Uh oh!

verb left a comment

Choose a reason for hiding this comment

Uh oh!

verb Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

knight42 Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

verb Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

verb Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

knight42 Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

verb Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

verb Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

verb commented Jul 27, 2022

Uh oh!

knight42 commented Jul 28, 2022

Uh oh!

knight42 commented Jul 28, 2022

Uh oh!

k8s-triage-robot commented Oct 26, 2022

Uh oh!

sding3 commented Nov 18, 2022

Uh oh!

knight42 commented Nov 18, 2022

Uh oh!

Uh oh!

knight42 commented Jun 12, 2022 •

edited

Loading