-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 live loader flag for configuring http endpoint for pprof. #3846
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ A review job has been created and sent to the PullRequest network.
Check the status or cancel PullRequest code review here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this submission.
The code currently in place implements default server and mux are not suitable for this use case. I believe it creates a vector for users to obtain information about the profiled code that could be exploited further. I think this code can be improved by creating a server manually.
Consider this example, which achieve, I believe, the same outcome:
addr := ":" + strconv.Itoa(port)
ln, err := net.Listen("tcp", addr)
server := http.Server{
Addr: addr,
ReadTimeout: 5 * time.Second,
WriteTimeout: 10 * time.Second,
Handler: nil,
MaxHeaderBytes: 1024 * 1024,
}
go func() {
if err := server.Serve(ln); err != nil {
glog.Errorf("Error while starting HTTP server: %+v", err)
}
}()
Reviewed with ❤️ by PullRequest
} | ||
go func() { | ||
if err := http.ListenAndServe("localhost:6060", nil); err != nil { | ||
glog.Errorf("Error while starting HTTP server in port 6060: %+v", err) | ||
if err := http.ListenAndServe(opt.httpAddr, nil); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach can be refactored, and expose the components of the method for greater flexibility, particularly in environments that do not use the default mux but wish to participate in this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it creates a vector for users to obtain information about the profiled code that could be exploited further.
Can you elaborate about this point? I'm not sure how exposing pprof with the default server and mux could cause an exploit.
dgraph/cmd/live/run.go
Outdated
@@ -63,6 +63,7 @@ type options struct { | |||
useCompression bool | |||
newUids bool | |||
verbose bool | |||
httpAddr string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order fields alphabetically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @danielmai, @manishrjain, and @pullrequest[bot])
This change adds an
--http
flag todgraph live
to configure the HTTP port that is exposed for pprof (e.g., http://localhost:6060/debug/pprof). Live loader was already set up to expose localhost:6060 as the pprof endpoint, but this was not configurable. If two live loaders were running on the same machine, you'd see the following error and you could only profile one of the live loaders:--http
flag to live loader. This matches the flag name and description asdgraph bulk
.localhost:6060
as the default value for the--http
flag, which is the same addr:port used in live loader already.This change is
Fixes #3985