Implement kubectl debug profiles: general, baseline, and restricted by sding3 · Pull Request #114280 · kubernetes/kubernetes

sding3 · 2022-12-05T02:46:30Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements "general", "baseline", and "restricted" debugging profiles for kubectl debug as specified in KEP-1441.

Which issue(s) this PR fixes:

xref kubernetes/kubectl#1108

Special notes for your reviewer:

I had picked up where @knight42 had left off in #110526

Does this PR introduce a user-facing change?

Added "general", "baseline", and "restricted" debugging profiles for kubectl debug.

k8s-ci-robot · 2022-12-05T02:46:34Z

Please note that we're already in Test Freeze for the release-1.26 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.26.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sun Dec 4 21:52:06 UTC 2022.

k8s-ci-robot · 2022-12-05T02:46:39Z

Hi @sding3. 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.

Details

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 · 2022-12-05T02:52:34Z

/cc @verb @knight42

aimuz · 2022-12-05T05:49:06Z

/ok-to-test

aimuz

Thank you

aimuz · 2022-12-11T04:18:15Z

This looks like it changed his behavior

This is intentional to ensure that debug container in pod copy is added before the profile application. The way that the container list modification was defered caused the debug container to be added after the profile applier runs. We now make sure to have the container list modification happen before the profile applier runs.

I had placed this change in its own git commit and supplied a git commit message to explain the reason behind the change: 0545b223eb5edd447f7a143a8e57bb2500d8ae64

Thank you for your response

Same above. Is this change related to new debug profiles?

Yes, we need this to ensure the generated debug container is added the the container list prior to the o.Applier.Apply occurs on line 692 below or otherwise the generated debug container isn't available to o.Applier.

I'm worried this has gotten a little too tricky. Can we just append immediately?

c, ok := containerByName[name] if !ok { ... copied.Spec.Containers = append(copied.Spec.Containers, corev1.Container{...}) c = &copied.Spec.Containers[len(copied.Spec.Containers)-1] }

Nice idea - done in 457117b

sding3 · 2022-12-11T15:46:11Z

/cc @verb @knight42

…ntify the container by name

sding3 · 2023-02-09T15:57:34Z

I quickly tested these profiles on Kind cluster and did not encounter any problem. I left a few unblocker comments can be easily done in followup PR. It would be great if we can also add integration tests in a followup PR too.

Yep, that will be my top priority after this PR.

verb

/lgtm

k8s-ci-robot · 2023-02-09T15:59:19Z

LGTM label has been added.

Details

Git tree hash: 9a99e9c99044a43a3c014af9e6c418ebb5883f57

sding3 · 2023-02-09T16:02:42Z

@verb - sorry I pushed a grammar fix in d760b0c after you already tagged - could you retag? Thank you.

verb

/lgtm

k8s-ci-robot · 2023-02-09T17:17:27Z

LGTM label has been added.

Details

Git tree hash: 1445acc71d5102439c526f69227af3f47097bdc6

k8s-ci-robot · 2023-02-09T17:17:51Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

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

hariskmohd · 2023-03-25T08:42:23Z

Is this feature available now or when can we expect this to come?

…ubernetes#114280) * feat(debug): add more profiles Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * feat(debug): implment serveral debugging profiles 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 <anonymousknight96@gmail.com> * test: add some basic tests Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * chore: add some helper functions Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> * ensure pod copies always get their probes cleared not wanting probes to be present is something we want for all the debug profiles; so an easy place to implement this is at the time of pod copy generation. * ensure debug container in pod copy is added before the profile application The way that the container list modification was defered causes the debug container to be added after the profile applier runs. We now make sure to have the container list modification happen before the profile applier runs. * make switch over pod copy, ephemeral, or node more clear * use helper functions added a helper function to modify a container out of a list that matches the provided container name. also added a helper function that adds capabilities to container security. * add tests for the debug profiles * document new debugging profiles in command line help text * add file header to profiles_test.go * remove URL to KEP from help text * move probe removal to the profiles * remove mustNewProfileApplier in tests * remove extra whiteline from import block * remove isPodCopy helper func * switch baselineProfile to using the modifyEphemeralContainer helper * rename addCap to addCapability, and don't do deep copy * fix godoc on modifyEphemeralContainer * export DebugOptions.Applier for extensibility * fix unit test * fix spelling on overriden * remove debugStyle facilities * inline setHostNamespace helper func * remove modifyContainer, modifyEphemeralContainer, and remove probes their logic have been in-lined at call sites * remove DebugApplierFunc convenience facility * fix baseline profile implementation it shouldn't have SYS_PTRACE base on https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/1441-kubectl-debug#profile-baseline * remove addCapability helper, in-lining at call sites * address Arda's code review comments 1 use Bool instead of BoolPtr (now deprecated) 2 tweak for loop to continue when container name is not what we expect 3 use our knowledge on how the debug container is generated to simplify our modification to the security context 4 use our knowledge on how the pod for node debugging is generated to no longer explicit set pod's HostNework, HostPID and HostIPC fields to false * remove tricky defer in generatePodCopyWithDebugContainer * provide helper functions to make debug profiles more readable * add note to remind people about updating --profile's help text when adding new profiles * Implement helper functions with names that improve readability * add styleUnsupported to replace debugStyle(-1) * fix godoc on modifyContainer * drop style prefix from debugStyle values * put VisitContainers in podutils & use that from debug * cite source for ContainerType and VisitContainers * pull in AllContainers ContainerType value * have VisitContainer take pod spec rather than pod * in-line modifyContainer * unexport helper funcs * put debugStyle at top of file * merge profile_applier.go into profile.go * tweak dropCapabilities * fix allowProcessTracing & add a test for it * drop mask param from help funcs, since we can already unambiguous identify the container by name * fix grammar in code comment --------- Signed-off-by: Jian Zeng <anonymousknight96@gmail.com> Co-authored-by: Jian Zeng <anonymousknight96@gmail.com>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 5, 2022

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 5, 2022

k8s-ci-robot requested a review from ardaguclu December 5, 2022 02:50

k8s-ci-robot added the area/kubectl label Dec 5, 2022

k8s-ci-robot requested a review from seans3 December 5, 2022 02:50

k8s-ci-robot added 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 Dec 5, 2022

k8s-ci-robot requested review from knight42 and verb December 5, 2022 02:52

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 Dec 5, 2022

aimuz reviewed Dec 11, 2022

View reviewed changes

sding3 requested review from aimuz and removed request for ardaguclu, knight42, seans3 and verb December 11, 2022 15:45

k8s-ci-robot requested review from knight42 and verb December 11, 2022 15:46

sding3 added 3 commits February 9, 2023 09:34

tweak dropCapabilities

f337511

fix allowProcessTracing & add a test for it

f604758

drop mask param from help funcs, since we can already unambiguous ide…

d3583dc

…ntify the container by name

verb approved these changes Feb 9, 2023

View reviewed changes

k8s-ci-robot assigned verb Feb 9, 2023

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

fix grammar in code comment

d760b0c

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

k8s-ci-robot requested review from ardaguclu and verb February 9, 2023 16:00

verb approved these changes Feb 9, 2023

View reviewed changes

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

k8s-ci-robot merged commit d35da34 into kubernetes:master Feb 9, 2023

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

sding3 mentioned this pull request Feb 9, 2023

kubectl debug: ensure profile application occurs after pod spec mutation in the CopyTo case #113992

Closed

This was referenced Apr 17, 2023

Debugging Profile dosen't work well with PSA #117405

Closed

Fix: Restricted profile comply with PSS #117543

Merged

mochizuki875 mentioned this pull request Jun 14, 2023

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

Open

alban mentioned this pull request Jul 10, 2023

ig container image: docs and fixes for kubectl-debug and Docker Desktop on Windows inspektor-gadget/inspektor-gadget#1809

Merged

P1ng-W1n mentioned this pull request Sep 21, 2023

[enhancement] kubectl alpha debug add SYS_PTRACE for debug container #97103

Closed

ChrisKujawa mentioned this pull request Jan 11, 2024

Ephemeral container debug is not working for 8.3.x ChrisKujawa/zdb#332

Closed

Uh oh!

Conversation

sding3 commented Dec 5, 2022

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?

Uh oh!

k8s-ci-robot commented Dec 5, 2022

Uh oh!

k8s-ci-robot commented Dec 5, 2022

Uh oh!

sding3 commented Dec 5, 2022

Uh oh!

aimuz commented Dec 5, 2022

Uh oh!

aimuz left a comment

Choose a reason for hiding this comment

Uh oh!

aimuz Dec 11, 2022

Choose a reason for hiding this comment

Uh oh!

sding3 Dec 11, 2022

Choose a reason for hiding this comment

Uh oh!

aimuz Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

ardaguclu Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

sding3 Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

verb Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

sding3 Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sding3 commented Dec 11, 2022

Uh oh!

sding3 commented Feb 9, 2023

Uh oh!

verb left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 9, 2023

Uh oh!

sding3 commented Feb 9, 2023

Uh oh!

verb left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 9, 2023

Uh oh!

k8s-ci-robot commented Feb 9, 2023

Uh oh!

hariskmohd commented Mar 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

sding3 Jan 24, 2023 •

edited

Loading