kubectl debug: Add --keep-* flag to control the removal of probes, labels, annotations, and initContainers from copy pod #123149

mochizuki875 · 2024-02-06T07:49:13Z

What type of PR is this?

/kind feature
/cleanup

What this PR does / why we need it:

When using --copy-to option with some debugging profile(legacy, restricted, netadmin sysadmin), the probe configuration remains in the copy pod .
Therefore, if the probe condition is not met by debugging operation on the copy pod, the main container will be restarted.
This may prevent debugging.

So in this PR, I've added flags(--keep-liveness,--keep-readiness,--keep-startup) to control the removal of probes from copy pod.
In addition, I've added --keep-labels, --keep-annotations and --keep-init-containers to control the removal of labels, annotations and initContainers too.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add `--keep-*` flags to `kubectl debug`, which enables to control the removal of probes, labels, annotations and initContainers from copy pod.

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

mochizuki875 · 2024-02-06T07:49:29Z

/sig cli

ardaguclu · 2024-02-14T14:53:06Z

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


 	switch style {
 	case podCopy:
+		removeLabelsAndProbes(pod)


I agree with that but I think we need to manage this with on/off flag as we did in https://github.com/openshift/oc/blob/cf7b09cd29f1c0f56b2c6cd8193092c57c7730ff/pkg/cli/debug/debug.go#L252-L255 and default would be to removal of probes. WDYT? @verb

@ardaguclu

Thank you for your comment and I agree that.
oc debug seems to be implemented to be able to control the removal of probes, labels, annotations and initContainers.
On the other hands in kubectl debug, labels will be removed in all profiles and probes will be removed in some profiles(general and baseline).

So I'm wondering which should be targeted for removal control（only probes, probes and labels or all?）.
How do you think?

Current implementations of kubectl debug are as below.

probes

removeLabelsAndProbes() which remove probes already has been implemented and used in some profiles(general and baseline).

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

Lines 245 to 254 in 91ee300

// removeLabelsAndProbes removes labels from the pod and remove probes

// from all containers of the pod.

func removeLabelsAndProbes(p *corev1.Pod) {

p.Labels = nil

for i := range p.Spec.Containers {

p.Spec.Containers[i].LivenessProbe = nil

p.Spec.Containers[i].ReadinessProbe = nil

p.Spec.Containers[i].StartupProbe = nil

}

}

labels

removeLabelsAndProbes() also implemented to remove labels, but copy pod labels will be removed when copy pod is created before removeLabelsAndProbes() will be called.

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

Lines 580 to 589 in 91ee300

// generatePodCopyWithDebugContainer takes a Pod and returns a copy and the debug container name of that copy

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(),

}

annotations and initContainers

There seems to be not implemented the logic to remove annotations and initContainers.

For now, I've added --keep-* flags to be able to control the removal of probes, labels, annotations and initContainers like oc debug.

mochizuki875 · 2024-03-25T06:57:03Z

/remove-kind bug
/kind feature

mochizuki875 · 2024-03-25T07:29:37Z

/retest

mochizuki875 · 2024-04-11T17:20:28Z

PTAL?

mochizuki875 · 2024-05-10T17:43:20Z

@ardaguclu
Thank you for your RVs and I've fixed them.
Please check again.

ardaguclu

Thanks for spending time and effort on this PR. That would be great, if we can add some integration tests cases also in here https://github.com/kubernetes/kubernetes/blob/master/test/cmd/debug.sh

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

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

ardaguclu · 2024-05-13T06:13:25Z

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

+		useHostNamespaces(pod)
+
+	case podCopy:
+		p.keeps.removeLabels(pod)


I think this is a behavioral change for legacyProfile because we were not removing as default and now we are.
I think, we can leave it as is and even keep flags don't change it. As it is a legacy profile.

Do you mean copy pod applied legacyProfile should keep annotations, probes and initContainers and doesn’t keep labels(it is not kept now as mentioned here) regardless of --keep-* flags?

Yes that is true. Because it is a legacy profile anyway and we shouldn't change the current behavior, unless it is fully broken, etc.

Thanks!
So should we add additional flags descriptions like "it does not work in legacyProfile" related that?

Let's don't add it for now. I think, adding more text just for a a legacy profile seems to be unnecessary. Isn't the recommended path is using the other profiles other than legacy?

We can say that if you want to manage these resources, please use the other recommended paths.

Let's don't add it for now.

Ok, I've got it.

Isn't the recommended path is using the other profiles other than legacy?

By the way, how many users recognize it recommended to use other profiles than legacy profile and the existence of debugging profile?

The official document doesn't seem to mention debugging profiles.(It's mentioned only in KEP now as I know.)

"kubectl debug --help" doesn't seem to show any recommended paths regarding profiles and show default profile is legacy.

So IMO, I think many users don't know the existence of debugging profile and recommendation.
Shouldn't we add any description about debugging profile somewhere?

In addition, it is said that in KEP:

Default Profile and Automation Selection
In order to provide a seamless experience and encourage use of PodSecurity, the "auto" profile will automatically choose a profile that's compatible with the current security profile by examining the pod-security.kubernetes.io/enforce annotation on the namespace and selecting the most permissive of "general", "baseline", and "restricted" that the controller will allow.

This will become the default behavior, but in order to maintain backwards compatibility the "legacy" profile will be the default profile until the 1.25 release. When --profile is not specified kubectl debug will print a warning about the upcoming change in behavior.

Though auto profile is WIP(#120687) now, shouldn't we print any warning or recommendation about profile as a first step?

These are outside the scope of this PR, but I was a little curious.

I assume that legacy name is self explanatory but I see your comments and it all makes sense. We should reconsider what is the path for legacy profile.

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

mochizuki875 · 2024-05-15T17:00:47Z

@ardaguclu
Ok, I've fixed.

Thanks for spending time and effort on this PR. That would be great, if we can add some integration tests cases also in here https://github.com/kubernetes/kubernetes/blob/master/test/cmd/debug.sh

I've added test pod manifest(hack/testdata/pod-with-metadata-and-probes.yaml) for added test cases.
If there is some good idea instead of adding the manifest, please comment that.

mochizuki875 · 2024-05-15T17:25:20Z

/retest

ardaguclu

tests look great. Thanks.

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

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

test/cmd/debug.sh

mochizuki875 · 2024-05-16T16:32:53Z

@ardaguclu
I've fixed.
In addition, I've renamed KeepFields to keepFlags because previous name seems to be not intuitive.

ardaguclu

One last comment and after that I think we are good to go

ardaguclu · 2024-05-16T16:46:09Z

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


 	if o.Applier == nil {
-		applier, err := NewProfileApplier(o.Profile)
+		kflags := keepFlags{


Agree that keepFlags is better naming and sorry, if I wasn't clear enough: Could you please export this, it's functions and it's fields?.

Could you please export this, it's functions and it's fields?.

It's ok.
Could you tell me the reason?
As you said here, they are not used by anywhere so I thought we need not to export...

Could you please export this, it's functions and it's fields?.

It's ok. Could you tell me the reason? As you said here, they are not used by anywhere so I thought we need not to export...

Basically, in case some other tools depend on kubectl debug functionality, they would be able to customize the behavior. Otherwise, keep flag functionality will be black box. We can also may want to intentionally unexport them. However, they would fully be relying on the flags and it may better to export them in my opinion. Hope that it makes sense.

Another reason would be ProfileApplier is exported function and can be customizable. On the other hand, it only gets an unexported type which drastically hurts the usability of ProfileApplier.

Thank you for your explanation.
I've understood.

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

mochizuki875 · 2024-05-16T17:11:59Z

@ardaguclu
I've fixed and rebased.

ardaguclu · 2024-05-16T17:18:46Z

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

-		return fmt.Errorf("the %s profile doesn't support objects of type %T", ProfileLegacy, target)
+// KeepFlags holds the flag set that determine which fields to keep in the copy pod.
+type KeepFlags struct {
+	labels         bool


Lastly fields to be exported?

Sorry, I overlooked it...
I've fixed.

ardaguclu · 2024-05-16T17:31:51Z

Great, thanks!
/approve
/lgtm

k8s-ci-robot · 2024-05-16T17:31:58Z

LGTM label has been added.

Git tree hash: 0a826e71ead3de72941f4581360fe22684c131a5

k8s-ci-robot · 2024-05-16T17:31:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, mochizuki875

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:

~~hack/testdata/OWNERS~~ [ardaguclu]
~~staging/src/k8s.io/kubectl/OWNERS~~ [ardaguclu]
~~test/cmd/OWNERS~~ [ardaguclu]

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

mochizuki875 · 2024-05-16T17:33:38Z

@ardaguclu
I'm very appreciate your many comments!

k8s-triage-robot · 2024-05-16T21:57:18Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

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

k8s-ci-robot requested review from ardaguclu and seans3 February 6, 2024 07:49

ardaguclu reviewed Feb 14, 2024

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 19, 2024

mochizuki875 force-pushed the remove_probe_from_copy_pod branch 2 times, most recently from 446744b to d252ebc Compare February 19, 2024 07:43

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2024

Sayrus mentioned this pull request Mar 6, 2024

Allow removing fields from kubectl debug copied pods or containers kubernetes/kubectl#1570

Closed

mochizuki875 force-pushed the remove_probe_from_copy_pod branch from d252ebc to 0dd901d Compare March 25, 2024 04:17

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2024

mochizuki875 force-pushed the remove_probe_from_copy_pod branch 2 times, most recently from 30858a2 to 4b3544e Compare March 25, 2024 06:31

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Mar 25, 2024

mochizuki875 changed the title ~~kubectl debug: Remove probe from copy pod~~ kubectl debug: Add --keep-* flag to control the removal of probes, labels, annotations, and initContainers from copy pod Mar 25, 2024

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label May 10, 2024

ardaguclu reviewed May 13, 2024

View reviewed changes

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 15, 2024

ardaguclu reviewed May 15, 2024

View reviewed changes

ardaguclu reviewed May 16, 2024

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/debug/profiles.go Outdated Show resolved Hide resolved

mochizuki875 force-pushed the remove_probe_from_copy_pod branch from 301dc1b to e56ce82 Compare May 16, 2024 17:10

ardaguclu reviewed May 16, 2024

View reviewed changes

Add keep options

b63fa13

mochizuki875 force-pushed the remove_probe_from_copy_pod branch from e56ce82 to b63fa13 Compare May 16, 2024 17:26

k8s-ci-robot assigned ardaguclu May 16, 2024

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

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

k8s-ci-robot merged commit 6d4250e into kubernetes:master May 17, 2024

k8s-ci-robot added this to the v1.31 milestone May 17, 2024

mochizuki875 deleted the remove_probe_from_copy_pod branch May 17, 2024 17:04

mochizuki875 mentioned this pull request May 17, 2024

kubectl debug: add auto profile #120687

Closed

mochizuki875 mentioned this pull request Jun 27, 2024

Add description about debugging profile kubernetes/website#46993

Merged

mochizuki875 mentioned this pull request Sep 9, 2024

kubectl debug: warning message about legacy profile #127230

Merged

	// removeLabelsAndProbes removes labels from the pod and remove probes
	// from all containers of the pod.
	func removeLabelsAndProbes(p *corev1.Pod) {
	p.Labels = nil
	for i := range p.Spec.Containers {
	p.Spec.Containers[i].LivenessProbe = nil
	p.Spec.Containers[i].ReadinessProbe = nil
	p.Spec.Containers[i].StartupProbe = nil
	}
	}

	// generatePodCopyWithDebugContainer takes a Pod and returns a copy and the debug container name of that copy
	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(),
	}

kubectl debug: Add --keep-* flag to control the removal of probes, labels, annotations, and initContainers from copy pod #123149

kubectl debug: Add --keep-* flag to control the removal of probes, labels, annotations, and initContainers from copy pod #123149

Uh oh!

Conversation

mochizuki875 commented Feb 6, 2024 • 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!

mochizuki875 commented Feb 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

probes

labels

annotations and initContainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mochizuki875 commented Mar 25, 2024

Uh oh!

mochizuki875 commented Mar 25, 2024

Uh oh!

mochizuki875 commented Apr 11, 2024

Uh oh!

mochizuki875 commented May 10, 2024

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mochizuki875 commented May 15, 2024

Uh oh!

mochizuki875 commented May 15, 2024

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mochizuki875 commented May 16, 2024

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

mochizuki875 commented Feb 6, 2024 •

edited

Loading

mochizuki875 May 16, 2024 •

edited

Loading