✨ Introduce pprof server to manager

[zqzten]

This PR introduces a configurable standalone server for serving pprof to manager, just like the health and metrics server.

fixes #1779

Additional thoughts:
I've been aware of metricsExtraHandlers introduced in #824 which might be an alternative for end users to serve pprof, but I don't think it's quite appropriate to do so, mainly for these two reasons:

1. pprof is a language built-in feature which is widely used, so it's more appropriate to make its server built-in in the library rather than let end users construct and run themselves in every controller.
2. metricsExtraHandlers bind all handlers to metrics listener which is not that general and flexible, it cannot meet cases that user want to expose metrics and pprof by different listeners (common use case: a 0.0.0.0: listener for serving metrics and a 127.0.0.1: listener for serving pprof).

[k8s-ci-robot]

Hi @zqzten. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

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.

[AlmogBaku]

TBH, I'm against this one. This is very tempting to add functionality such as this to your solution, but enabling it on production might cause a sensitive information leakage.

I suggest adding a "hint" in your "DEVELOPMENT.MD" file as I did here:
https://github.com/natun-ai/natun/blob/master/CONTRIBUTING.md#profiling

[FillZpp]

This is very tempting to add functionality such as this to your solution, but enabling it on production might cause a sensitive information leakage.

That's true if we enable pprof and bind it to 0.0.0.0:<port> on production. But I think this PR is ok, since it just provides an option which is disabled by default and we can add doc comments to recommend ppl only bind it to localhost.

[zqzten]

@AlmogBaku Thanks for the security remind, but I don't think it as a blocking issue as we have ways to protect it:

• By binding its listener to 127.0.0.1:, as @FillZpp suggested, which is a very common and naive solution. This is also why seperated listener is introduced instead of reusing metricsExtraHandlers.
• By protecting pprof server by kube-rbac-proxy, which is also the default solution to protect metrics server in kubebuilder. This is a sophisticated solution which is similarly used by kube-controller-manager (it does the rbac proxy by itself though).

Security is very important indeed, I'll add some doc comments on it, also we can put best practice usage in kubebuilder after this feature is added.

[FillZpp]

/ok-to-test

/cc @alvaroaleman

[AlmogBaku]

what about adding a build tag as a solution?

[zqzten]

what about adding a build tag as a solution?

I'm not sure whether this solution is appropriate in this project. Let's just wait for opinions from other maintainers.

[alvaroaleman]

Kind of torn here. Not having pprof is extremely annoying in production, but it definitely exposes sensitive information.

@AlmogBaku what is the reason why you would prefer build tags over off-by-default, i.E. what is the problem you see from having it compiled in but disabled?

[AlmogBaku]

Exposing this setting using a build tag is like having a “remote debugger” - possible, but should be allowed implicitly and temporary on production mode

[alvaroaleman]

Exposing this setting using a build tag is like having a “remote debugger” - possible, but should be allowed implicitly and temporary on production mode

I actually disagree with that. Pprof needs to always be enabled or it is useless. If it requires a binary restart of any kind it becomes impossible to debug in situations where there is no clear reproducer. The remaining concern here is that we can't tell if ppl properly wall off connectivity to their controllers, which is why it might be problematic to enable by default.

[zqzten]

@alvaroaleman @vincepri @FillZpp Mind give some further points on this? It would be very appreciated. There're many ppl around calling for this feature.

[vincepri]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri, zqzten

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:

• OWNERS [vincepri]

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

[fovgrubby128]

A dumb question here, it would be released in the next version, right?

[sbueringer]

Yes, that's the plan

[fovgrubby128]

Yes, that's the plan

Could I know the next release date please since we want to introduce this change in our code for some deadline? thanks!

[sbueringer]

There is no fixed date as far as I know. But I think we want to get #2189 in first