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

Add configuration for turning profiling on/off #12045

Closed
1 of 3 tasks
VitalyGushin opened this issue May 23, 2024 · 11 comments
Closed
1 of 3 tasks

Add configuration for turning profiling on/off #12045

VitalyGushin opened this issue May 23, 2024 · 11 comments
Assignees

Comments

@VitalyGushin
Copy link
Contributor

VitalyGushin commented May 23, 2024

  • Stage 1: build additional images with profiler only for the most important apps (nsmgr, forwarder, registry) only for release images
  • Stage 2: support for CI images
  • Stage 3: support for all other applications
@denis-tingaikin
Copy link
Member

Hi @VitalyGushin, @szvincze 

I just got a thought that probably it is better to use build flags for enabling or disabling pprof. The flags can be used for CI and RC images. This approach allows to exclude http and pproff deps in the release binaries. 

Thoughts?

@VitalyGushin
Copy link
Contributor Author

@szvincze When importing the net/http/pprof package, the profiler's HTTP endpoints are automatically exposed, which causes gosec warning G108: "Profiling endpoint is automatically exposed on /debug/pprof", which I disable with the #nosec tag in this task. This doesn't matter when the HTTP server is not running, but it may still matter if someone adds a wrapper with their own HTTP server. Is this not critical for you?

@szvincze
Copy link
Contributor

it may still matter if someone adds a wrapper with their own HTTP server. Is this not critical for you?

@VitalyGushin Can you please elaborate a bit more when and how it could cause a problem?

@VitalyGushin
Copy link
Contributor Author

@szvincze You can read more about the G108 threat here. This is a security hole for which we are turning off the warning. The threat may appear if someone adds an http server that will run constantly. It is difficult to say what specific problems this may cause (or may not). In the current implementation, if the profiler is turned off, then there is no threat.

@szvincze
Copy link
Contributor

@VitalyGushin I have read that page and considered all the listed potential security issues. By default the profiling will be disabled, it will only be enabled on our request to the customer and the profiling will be done as a controlled procedure. So, I think we can go and live with it.

@denis-tingaikin
Copy link
Member

denis-tingaikin commented May 29, 2024

@szvincze, @VitalyGushin,

It looks like the issue is with NSM apps that rely on http. You can actually find a list of all http-based apps on GitHub https://github.com/search?q=org%3Anetworkservicemesh%20%22net%2Fhttp%22&type=code

These could be things like webhook, dashboard backend and etc.

We need to head up though, if we will use pprof for these apps, the profile endpoint will always be on. That's because importing packages with import _ "package" automatically runs the init function from that package.

Basically, this approach wouldn't be ideal since HTTP-based apps would always have a gap that we can't turn off.

@szvincze
Copy link
Contributor

@denis-tingaikin Good point! I think we just added pprof to the most important apps. For the rest we can control it by build flags and use the proper image on demand.

@denis-tingaikin denis-tingaikin moved this from Under review to Todo in Release v1.14.0 Jun 25, 2024
@VitalyGushin
Copy link
Contributor Author

@szvincze We have several tag name options for additional images with profiling enabled:

cmd-nsmgr:v1.13.1-rc.1-debug
cmd-nsmgr:v1.13.1-rc.1-profiler
cmd-nsmgr:v1.13.1-rc.1-pprof

Which one do you like best? Or maybe you have another option?

@szvincze
Copy link
Contributor

I would vote for cmd-nsmgr:v1.13.1-rc.1-pprof because it clearly represents the tool we should use.

@Ex4amp1e
Copy link
Contributor

Current plan:
Do not import "net/http/pprof" with _ to avoid auto run of init function and add required handlers manually in case pprof is enabled in env config

This was referenced Aug 14, 2024
@Ex4amp1e Ex4amp1e moved this from In Progress to Done in Release v1.14.0 Aug 16, 2024
@denis-tingaikin
Copy link
Member

denis-tingaikin commented Aug 16, 2024

@szvincze , @edwarnicke FYI: Now we're able to enable the pprof server in each nsm application via env NSM_PPROF_ENABLED=true

Going to close the issue; let us know if you have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

4 participants