Add support to disable /debug/pprof and /debug/flags/v endpoint

[SaranBalaji90]

Co-authored-by: xiaofei.sun sunxiaofei@kuaishou.com
Co-authored-by: SaranBalaji90 srisaranbalaji@gmail.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds "EnableProfilingHandler" and "EnableDebugFlagsHandler" flag to kubelet configuration similar to "EnableSystemLogHandler". Based on the value of EnableProfilingHandler, /debug/pprof endpoint will be made available in kubelet and similarly based on "EnableDebugFlagsHandler", /debug/flags will be enabled . Default value for these flag is true.

Special notes for your reviewer:
This change is similar to #87273

Special credit to xiaoanyunfei@ for the initial commit.

Which issue(s) this PR fixes:
Fixes #84165

Does this PR introduce a user-facing change?:

Cluster admins can now turn off /debug/pprof and /debug/flags/v endpoint in kubelet by setting enableProfilingHandler and enableDebugFlagsHandler to false in their kubelet configuration file. enableProfilingHandler and enableDebugFlagsHandler can be set to true only when enableDebuggingHandlers is also set to true.

/sig node

Testing result:

Updated /etc/kubernetes/kubelet/kubelet-config.json with the following values

  "enableDebuggingHandlers": true,
  "enableSystemLogHandler": false,
  "enableDebugFlagsHandler": false,
  "enableProfilingHandler": false,

[ec2-user@ip-192-168-30-66 ~]$ curl -k https://localhost:10250/debug/pprof/symbol
profiling endpoint is disabled.
[ec2-user@ip-192-168-30-66 ~]$ curl -k https://localhost:10250/logs/
logs endpoint is disabled.
[ec2-user@ip-192-168-30-66 ~]$ curl -X PUT -k https://localhost:10250/debug/flags/v -d "2"
flags endpoint is disabled

After updating the values to true again

[ec2-user@ip-192-168-30-66 ~]$ curl -k https://localhost:10250/debug/pprof/symbol
num_symbols: 1
[ec2-user@ip-192-168-30-66 ~]$ curl -k https://localhost:10250/logs/
<pre>
<a href="alternatives.log">alternatives.log</a>
<a href="audit/">audit/</a>

/sig node
/area kubelet
/assign @derekwaynecarr
/assign @dchen1107

[fejta-bot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[SaranBalaji90]

Second commit is mostly to improve readability and make future enhancements easier - a880668

[ehashman]

/triage accepted
/priority important-longterm

[ehashman]

/retest

[lavalamp]

Release note claims:

Cluster admins can now turn off /debug/pprof and /debug/flags/v endpoint in kubelet by setting enableProfilingHandler and enableDebugFlagsHandler to false in their kubelet configuration file. enableProfilingHandler and enableDebugFlagsHandler can be set to true only when enableDebuggingHandlers is also set to true.

However I don't see any code enforcing that. Maybe the implementation happens to work this way but it's not easy at all to verify. Apart from this the api is fine. I think it's important that these things don't suddenly turn on for people who have them off currently.

[SaranBalaji90]

Release note claims:

Cluster admins can now turn off /debug/pprof and /debug/flags/v endpoint in kubelet by setting enableProfilingHandler and enableDebugFlagsHandler to false in their kubelet configuration file. enableProfilingHandler and enableDebugFlagsHandler can be set to true only when enableDebuggingHandlers is also set to true.

However I don't see any code enforcing that. Maybe the implementation happens to work this way but it's not easy at all to verify. Apart from this the api is fine. I think it's important that these things don't suddenly turn on for people who have them off currently.

Thank you for reviewing the PR @lavalamp :)

We are using enableDebuggingHandlers as the parent knob to turn on/off all the sub handlers like /logs or /pprof and other handlers. If at present if admin has enableDebuggingHandlers set to false then it wont turn on these handlers. Like you mentioned we decided this route earlier just to avoid turning on the feature accidentally and also to make it backward compatible.

we enforce that here - https://github.com/kubernetes/kubernetes/pull/98458/files#diff-c3e7f724d1b0dcee40df80716ed57d90d2649710150fa92bf9822bdad35e0429R239 enableDebuggingHandlers should be set to true to enable/disable other child handlers.

[lavalamp]

we enforce that here

I guess I'm not sure why you install a "not turned on" handler if it is off? I think it would be easier to audit if the section of code you linked me to was just if statements.

I also don't really like that this is so inconsistent with how apiserver handles this, but I don't know that anything can be done about that.

Anyway, kubelet is not my binary so I don't really have a strong opinion, if you all are certain enough then that's fine.

For the API:
/approve

[SaranBalaji90]

I guess I'm not sure why you install a "not turned on" handler if it is off? I think it would be easier to audit if the section of code you linked me to was just if statements.

If its off, Kubelet just return messages like "logs(profiling) endpoint is disabled."

[SaranBalaji90]

I also don't really like that this is so inconsistent with how apiserver handles this, but I don't know that anything can be done about that.

@lavalamp is it due to the fact that single --profiling cmd line argument takes care of turning off /debug/pprof and /debug/flags/v endpoints in ApiServer but we are turning off in Kubelet with two different flags? (our thought process is documented here for reference #84165 (comment)) or is it in general how Kubelet handles these handlers.

[lavalamp]

I guess I'm not sure why you install a "not turned on" handler if it is off? I think it would be easier to audit if the section of code you linked me to was just if statements.

If its off, Kubelet just return messages like "logs(profiling) endpoint is disabled."

Yes, I understood, I just am not sure why that's desirable.

[SaranBalaji90]

I guess I'm not sure why you install a "not turned on" handler if it is off? I think it would be easier to audit if the section of code you linked me to was just if statements.

If its off, Kubelet just return messages like "logs(profiling) endpoint is disabled."

Yes, I understood, I just am not sure why that's desirable.

Ah ok.. I can remove if we don't need to return anything.. I added with assumption that it provides better wording(UX) when disabled.

[ehashman]

From what I can see the latest update has changed quite a bit from the last... @sjenning can you PTAL?

[SaranBalaji90]

actually @ehashman I didn't make any change though between 3944e1d and 51c5401 but for some reason robot reported - "New changes are detected"

[lavalamp]

I added with assumption that it provides better wording(UX) when disabled.

It's up to the Kubelet owners. :)

[ehashman]

actually @ehashman I didn't make any change though between 3944e1d and 51c5401 but for some reason robot reported - "New changes are detected"

When I tried to diff the branches the diff numbers didn't match... I didn't do the initial review but I am not the right person to LGTM this :)

[SaranBalaji90]

actually @ehashman I didn't make any change though between 3944e1d and 51c5401 but for some reason robot reported - "New changes are detected"

When I tried to diff the branches the diff numbers didn't match... I didn't do the initial review but I am not the right person to LGTM this :)

looks like there was some log format changes in master which was missed after rebase. Fixed it now :)

[derekwaynecarr]

/approve
/lgtm

@andrewsykim maybe you can help restructure this in a follow-on to make the code path clearer per @lavalamp comments as part of overall /pkg/kubelet/server cleanup?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, dims, lavalamp, SaranBalaji90, sjenning

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:

• cmd/kubelet/OWNERS [derekwaynecarr,lavalamp,sjenning]
• pkg/kubelet/OWNERS [derekwaynecarr,lavalamp,sjenning]
• pkg/kubelet/apis/config/OWNERS [lavalamp]
• staging/src/k8s.io/kubelet/config/OWNERS [lavalamp]

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