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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
9b758da
feat(debug): add more profiles
knight42 Jun 12, 2022
21f304e
feat(debug): implment serveral debugging profiles
knight42 Jun 12, 2022
85da9f2
test: add some basic tests
knight42 Jun 12, 2022
249bfac
chore: add some helper functions
knight42 Jul 14, 2022
c3e82ba
ensure pod copies always get their probes cleared
sding3 Dec 3, 2022
59bc349
ensure debug container in pod copy is added before the profile applic…
sding3 Dec 3, 2022
5c767f6
make switch over pod copy, ephemeral, or node more clear
sding3 Dec 3, 2022
5c41ee2
use helper functions
sding3 Dec 3, 2022
b3d2138
add tests for the debug profiles
sding3 Dec 5, 2022
194c8dd
document new debugging profiles in command line help text
sding3 Dec 5, 2022
1db7eb6
add file header to profiles_test.go
sding3 Dec 10, 2022
8ae1fb0
remove URL to KEP from help text
sding3 Jan 10, 2023
9e9c210
move probe removal to the profiles
sding3 Jan 10, 2023
385afff
remove mustNewProfileApplier in tests
sding3 Jan 10, 2023
0f35faa
remove extra whiteline from import block
sding3 Jan 10, 2023
507f9e7
remove isPodCopy helper func
sding3 Jan 10, 2023
34ec729
switch baselineProfile to using the modifyEphemeralContainer helper
sding3 Jan 10, 2023
777b04a
rename addCap to addCapability, and don't do deep copy
sding3 Jan 10, 2023
5ebd435
fix godoc on modifyEphemeralContainer
sding3 Jan 10, 2023
61b0b0c
export DebugOptions.Applier for extensibility
sding3 Jan 11, 2023
70bd5a4
fix unit test
sding3 Jan 11, 2023
0edd708
fix spelling on overriden
sding3 Jan 11, 2023
00be236
remove debugStyle facilities
sding3 Jan 11, 2023
29ada24
inline setHostNamespace helper func
sding3 Jan 11, 2023
1dbd839
remove modifyContainer, modifyEphemeralContainer, and remove probes
sding3 Jan 11, 2023
adb553b
remove DebugApplierFunc convenience facility
sding3 Jan 11, 2023
93050e8
fix baseline profile implementation
sding3 Jan 11, 2023
3b5527d
remove addCapability helper, in-lining at call sites
sding3 Jan 11, 2023
c1e70d6
address Arda's code review comments
sding3 Jan 13, 2023
457117b
remove tricky defer in generatePodCopyWithDebugContainer
sding3 Jan 24, 2023
c834ada
provide helper functions to make debug profiles more readable
sding3 Jan 24, 2023
914098f
add note to remind people about updating --profile's help text when a…
sding3 Jan 24, 2023
4472c8f
Implement helper functions with names that improve readability
sding3 Jan 27, 2023
5e61f87
add styleUnsupported to replace debugStyle(-1)
sding3 Feb 3, 2023
443d72d
fix godoc on modifyContainer
sding3 Feb 3, 2023
98878e9
drop style prefix from debugStyle values
sding3 Feb 7, 2023
5db0224
put VisitContainers in podutils & use that from debug
sding3 Feb 8, 2023
88d19f4
cite source for ContainerType and VisitContainers
sding3 Feb 8, 2023
d917ea7
pull in AllContainers ContainerType value
sding3 Feb 8, 2023
9980209
have VisitContainer take pod spec rather than pod
sding3 Feb 8, 2023
1624f5f
in-line modifyContainer
sding3 Feb 8, 2023
98f8ff5
unexport helper funcs
sding3 Feb 9, 2023
7f7ef70
put debugStyle at top of file
sding3 Feb 9, 2023
7ccbb6c
merge profile_applier.go into profile.go
sding3 Feb 9, 2023
f337511
tweak dropCapabilities
sding3 Feb 9, 2023
f604758
fix allowProcessTracing & add a test for it
sding3 Feb 9, 2023
d3583dc
drop mask param from help funcs, since we can already unambiguous ide…
sding3 Feb 9, 2023
d760b0c
fix grammar in code comment
sding3 Feb 9, 2023
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
33 changes: 18 additions & 15 deletions staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go
Expand Up @@ -121,6 +121,7 @@ type DebugOptions struct {
TargetContainer string
TTY bool
Profile string
Applier ProfileApplier
Copy link
Contributor

Choose a reason for hiding this comment

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

could you help me catch up on why this is now exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arda suggested this during review of this MR:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks for catching me up.

I don't think I quite understand what's going to set the Applier if not kubectl debug, but I support the use case.

Copy link
Member

Choose a reason for hiding this comment

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

This is basically for the other CLI's like oc to use kubectl debug. oc has it's own custom profiles and it will set Applier with custom profiles.

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm, happy for oc to innovate here (and hopefully push what works back upstream 😁)


attachChanged bool
shareProcessedChanged bool
Expand All @@ -129,8 +130,6 @@ type DebugOptions struct {

genericclioptions.IOStreams
WarningPrinter *printers.WarningPrinter

applier ProfileApplier
}

// NewDebugOptions returns a DebugOptions initialized with default values.
Expand Down Expand Up @@ -180,7 +179,7 @@ func addDebugFlags(cmd *cobra.Command, opt *DebugOptions) {
cmd.Flags().BoolVar(&opt.ShareProcesses, "share-processes", opt.ShareProcesses, i18n.T("When used with '--copy-to', enable process namespace sharing in the copy."))
cmd.Flags().StringVar(&opt.TargetContainer, "target", "", i18n.T("When using an ephemeral container, target processes in this container name."))
cmd.Flags().BoolVarP(&opt.TTY, "tty", "t", opt.TTY, i18n.T("Allocate a TTY for the debugging container."))
cmd.Flags().StringVar(&opt.Profile, "profile", ProfileLegacy, i18n.T("Debugging profile."))
cmd.Flags().StringVar(&opt.Profile, "profile", ProfileLegacy, i18n.T(`Debugging profile. Options are "legacy", "general", "baseline", or "restricted".`))
Copy link
Contributor

Choose a reason for hiding this comment

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

hard coding the list of profiles seems like it could be hard to keep in sync, both with new profiles and in translations. Should we instead have a flag that prints out the list of profiles and exits?

Copy link
Contributor Author

@sding3 sding3 Jan 24, 2023

Choose a reason for hiding this comment

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

I think that would be useful, but given that the KEP only specified a only handful of profiles - we should be okay?

In any case, I think we should defer adding a new flag which prints out the list of profiles to a future MR if possible.

For now, I've added a code comment to remind folks in 914098f

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll want something better, but maybe it'll come in the form of extensible profiles. I'm fine with hardcoding this to make progress.

}

// Complete finishes run-time initialization of debug.DebugOptions.
Expand Down Expand Up @@ -226,9 +225,13 @@ func (o *DebugOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []st
if o.WarningPrinter == nil {
o.WarningPrinter = printers.NewWarningPrinter(o.ErrOut, printers.WarningPrinterOptions{Color: term.AllowsColorOutput(o.ErrOut)})
}
o.applier, err = NewProfileApplier(o.Profile)
if err != nil {
return err

if o.Applier == nil {
applier, err := NewProfileApplier(o.Profile)
if err != nil {
return err
}
o.Applier = applier
}

return nil
Expand Down Expand Up @@ -536,10 +539,12 @@ func (o *DebugOptions) generateDebugContainer(pod *corev1.Pod) (*corev1.Pod, *co

copied := pod.DeepCopy()
copied.Spec.EphemeralContainers = append(copied.Spec.EphemeralContainers, *ec)
if err := o.applier.Apply(copied, name, copied); err != nil {
if err := o.Applier.Apply(copied, name, copied); err != nil {
return nil, nil, err
}

ec = &copied.Spec.EphemeralContainers[len(copied.Spec.EphemeralContainers)-1]
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason of this change with respect to the new debug profiles?. If this is not related to the new profiles, it would be better to open it separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this came from knight42 via commit bdb860490bfdf3db225020b2bc77168527394783. It ensures that the returned value ec takes into consideration any update that may have occurred to the ephemeral container due to profile.

Copy link
Member

Choose a reason for hiding this comment

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

Is this change related to new debug profiles or is it fixing something?. If it fixes a bug, it is better to fix that independent from this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is related to new debug profiles, because the only pre-existing legacy profile doesn't update EphemeralContainers.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the returned ec references is only used for two reasons:

  1. to print a debugging message, which could easily be changed
  2. for compatibility with pre-1.22 APIs, which we probably don't need anymore

Maybe we should just get rid of returning a pointer to the ephemeral container and just return the name instead?

The section might read a little more clearly with an immediate pod copy, something like:

name := o.computeDebugContainerName(pod)
copied := pod.DeepCopy()
i := len(copied.Spec.EphemeralContainers)
copied.Spec.EphemeralContainers = append(copied.Spec.EphemeralContainers, corev1.EphemeralContainer{
		EphemeralContainerCommon: corev1.EphemeralContainerCommon{
			Name:                     name,
			Env:                      o.Env,
			Image:                    o.Image,
			ImagePullPolicy:          o.PullPolicy,
			Stdin:                    o.Interactive,
			TerminationMessagePolicy: corev1.TerminationMessageReadFile,
			TTY:                      o.TTY,
		},
		TargetContainerName: o.TargetContainer,
	})
ec := &copied.Spec.EphemeralContainers[i]

Both of these suggestions are optional, feel free to disregard or fix in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I've made #115285 to track that. I'd be happy to circle back around on this in a future MR.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks


return copied, ec, nil
}

Expand Down Expand Up @@ -593,7 +598,7 @@ func (o *DebugOptions) generateNodeDebugPod(node *corev1.Node) (*corev1.Pod, err
p.Spec.Containers[0].Command = o.Args
}

if err := o.applier.Apply(p, cn, node); err != nil {
if err := o.Applier.Apply(p, cn, node); err != nil {
return nil, err
}

Expand All @@ -614,7 +619,7 @@ func (o *DebugOptions) generatePodCopyWithDebugContainer(pod *corev1.Pod) (*core
copied.Spec.EphemeralContainers = nil
// change ShareProcessNamespace configuration only when commanded explicitly
if o.shareProcessedChanged {
copied.Spec.ShareProcessNamespace = pointer.BoolPtr(o.ShareProcesses)
copied.Spec.ShareProcessNamespace = pointer.Bool(o.ShareProcesses)
}
if !o.SameNode {
copied.Spec.NodeName = ""
Expand Down Expand Up @@ -647,13 +652,11 @@ func (o *DebugOptions) generatePodCopyWithDebugContainer(pod *corev1.Pod) (*core
if len(name) == 0 {
name = o.computeDebugContainerName(copied)
}
c = &corev1.Container{
copied.Spec.Containers = append(copied.Spec.Containers, corev1.Container{
Name: name,
TerminationMessagePolicy: corev1.TerminationMessageReadFile,
}
defer func() {
copied.Spec.Containers = append(copied.Spec.Containers, *c)
}()
})
c = &copied.Spec.Containers[len(copied.Spec.Containers)-1]
}

if len(o.Args) > 0 {
Expand All @@ -676,7 +679,7 @@ func (o *DebugOptions) generatePodCopyWithDebugContainer(pod *corev1.Pod) (*core
c.Stdin = o.Interactive
c.TTY = o.TTY

err := o.applier.Apply(copied, c.Name, pod)
err := o.Applier.Apply(copied, c.Name, pod)
if err != nil {
return nil, "", err
}
Expand Down
84 changes: 75 additions & 9 deletions staging/src/k8s.io/kubectl/pkg/cmd/debug/debug_test.go
Expand Up @@ -55,6 +55,7 @@ func TestGenerateDebugContainer(t *testing.T) {
Container: "debugger",
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileLegacy,
},
expected: &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Expand All @@ -72,6 +73,7 @@ func TestGenerateDebugContainer(t *testing.T) {
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
TargetContainer: "myapp",
Profile: ProfileLegacy,
},
expected: &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Expand All @@ -90,6 +92,7 @@ func TestGenerateDebugContainer(t *testing.T) {
Container: "debugger",
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileLegacy,
},
expected: &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Expand All @@ -109,6 +112,7 @@ func TestGenerateDebugContainer(t *testing.T) {
Args: []string{"echo", "one", "two", "three"},
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileLegacy,
},
expected: &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Expand All @@ -125,6 +129,7 @@ func TestGenerateDebugContainer(t *testing.T) {
opts: &DebugOptions{
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileLegacy,
},
expected: &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Expand All @@ -140,6 +145,7 @@ func TestGenerateDebugContainer(t *testing.T) {
opts: &DebugOptions{
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileLegacy,
},
pod: &corev1.Pod{
Spec: corev1.PodSpec{
Expand All @@ -164,6 +170,7 @@ func TestGenerateDebugContainer(t *testing.T) {
opts: &DebugOptions{
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileLegacy,
},
pod: &corev1.Pod{
Spec: corev1.PodSpec{
Expand Down Expand Up @@ -196,6 +203,7 @@ func TestGenerateDebugContainer(t *testing.T) {
opts: &DebugOptions{
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileLegacy,
},
pod: &corev1.Pod{
Spec: corev1.PodSpec{
Expand Down Expand Up @@ -227,6 +235,65 @@ func TestGenerateDebugContainer(t *testing.T) {
},
},
},
{
name: "general profile",
opts: &DebugOptions{
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileGeneral,
},
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{"SYS_PTRACE"},
},
},
},
},
},
{
name: "baseline profile",
opts: &DebugOptions{
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileBaseline,
},
expected: &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: "debugger-1",
Image: "busybox",
ImagePullPolicy: corev1.PullIfNotPresent,
TerminationMessagePolicy: corev1.TerminationMessageReadFile,
},
},
},
{
name: "restricted profile",
opts: &DebugOptions{
Image: "busybox",
PullPolicy: corev1.PullIfNotPresent,
Profile: ProfileRestricted,
},
expected: &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: "debugger-1",
Image: "busybox",
ImagePullPolicy: corev1.PullIfNotPresent,
TerminationMessagePolicy: corev1.TerminationMessageReadFile,
SecurityContext: &corev1.SecurityContext{
RunAsNonRoot: pointer.Bool(true),
Capabilities: &corev1.Capabilities{
Drop: []corev1.Capability{"ALL"},
},
},
},
},
},
} {
t.Run(tc.name, func(t *testing.T) {
tc.opts.IOStreams = genericclioptions.NewTestIOStreamsDiscard()
Expand All @@ -236,11 +303,11 @@ func TestGenerateDebugContainer(t *testing.T) {
tc.pod = &corev1.Pod{}
}

applier, err := NewProfileApplier(ProfileLegacy)
Copy link
Member

Choose a reason for hiding this comment

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

As I suggested above, we can preserve this one

Copy link
Contributor Author

@sding3 sding3 Jan 10, 2023

Choose a reason for hiding this comment

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

yep, done in 8176751066f175f7bf53cbaf88a8c20ff2adc

applier, err := NewProfileApplier(tc.opts.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: Is there any reason of changing the order with if tc.pod == nil {

if err != nil {
t.Fatalf("fail to create %s profile", ProfileLegacy)
t.Fatalf("failed to create profile applier: %s: %v", tc.opts.Profile, err)
}
tc.opts.applier = applier
tc.opts.Applier = applier

_, debugContainer, err := tc.opts.generateDebugContainer(tc.pod)
if err != nil {
Expand Down Expand Up @@ -814,7 +881,7 @@ func TestGeneratePodCopyWithDebugContainer(t *testing.T) {
TerminationMessagePolicy: corev1.TerminationMessageReadFile,
},
},
ShareProcessNamespace: pointer.BoolPtr(true),
ShareProcessNamespace: pointer.Bool(true),
},
},
},
Expand Down Expand Up @@ -1017,7 +1084,7 @@ func TestGeneratePodCopyWithDebugContainer(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
var err error
tc.opts.applier, err = NewProfileApplier(ProfileLegacy)
tc.opts.Applier, err = NewProfileApplier(ProfileLegacy)
if err != nil {
t.Fatalf("Fail to create legacy profile: %v", err)
}
Expand Down Expand Up @@ -1212,7 +1279,7 @@ func TestGenerateNodeDebugPod(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
var err error
tc.opts.applier, err = NewProfileApplier(ProfileLegacy)
tc.opts.Applier, err = NewProfileApplier(ProfileLegacy)
if err != nil {
t.Fatalf("Fail to create legacy profile: %v", err)
}
Expand All @@ -1236,7 +1303,7 @@ func TestCompleteAndValidate(t *testing.T) {
cmpFilter := cmp.FilterPath(func(p cmp.Path) bool {
switch p.String() {
// IOStreams contains unexported fields
case "IOStreams":
case "IOStreams", "Applier":
return true
}
return false
Expand Down Expand Up @@ -1572,7 +1639,6 @@ func TestCompleteAndValidate(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
opts := NewDebugOptions(ioStreams)
var gotError error

cmd := &cobra.Command{
Run: func(cmd *cobra.Command, args []string) {
gotError = opts.Complete(tf, cmd, args)
Expand All @@ -1599,7 +1665,7 @@ func TestCompleteAndValidate(t *testing.T) {
}

if diff := cmp.Diff(tc.wantOpts, opts, cmpFilter, cmpopts.IgnoreFields(DebugOptions{},
"attachChanged", "shareProcessedChanged", "podClient", "WarningPrinter", "applier")); diff != "" {
"attachChanged", "shareProcessedChanged", "podClient", "WarningPrinter", "Applier")); diff != "" {
t.Error("CompleteAndValidate unexpected diff in generated object: (-want +got):\n", diff)
}
})
Expand Down
49 changes: 0 additions & 49 deletions staging/src/k8s.io/kubectl/pkg/cmd/debug/profile_applier.go

This file was deleted.