br: Add TLS support for PProf of BR

[YuJuncen]

What problem does this PR solve?

Fixed #817

What is changed and how it works?

Get TLS config and apply it when starting proof.

Check List

Tests

• Integration test

Release Note

• Fixed a bug that caused, even BR started with TLS config, the pprof endpoints won't be served with TLS.

[overvenus]

Prefer pass arguments explicitly instead of a closure.

[YuJuncen]

This function is an internal function, I think if we pass arguments here, we probably need to provide TLS config explicitly here, like StartDynamicPProfListener(cert, key string). At the implementation might would be like:

if cert == "" || key == "" {
  StartPProfListener("0.0.0.0:0")
} else {
  StartPProfListener("0.0.0.0:0", cert, key)
}

We got more args, and function overloading-like magic... Without more information, I'm afraid I would prefer the current style...

[overvenus]

We can move the if condition into the StartPProfListener.

func StartPProfListener(addr, cert, key string) error {
    if cert == "" || key == "" {
    } else {
    }
}

[YuJuncen]

Yep, I think those would make a if statement within the function, whose condition is the new parameter we added. And in my opinion this is also control coupling or another form of function overloading.

I think make another function(StartPProfListenerTLS) and let user to decide how to start the pprof listener by callback in the StartDynamicPProfListener would be more flexible.

I'm wondering if there are some style guides preferring plain arguments or some bad things happened with closures...? If so, I will surely remove those.

[3pointer]

/lgtm

[kennytm]

Why not, um, reuse the method from (*Lightning).goServe?

br/pkg/lightning/lightning.go

Lines 95 to 172 in 688c835

	func (l *Lightning) GoServe() error {
	handleSigUsr1(func() {
	l.serverLock.Lock()
	statusAddr := l.globalCfg.App.StatusAddr
	shouldStartServer := len(statusAddr) == 0
	if shouldStartServer {
	l.globalCfg.App.StatusAddr = ":"
	}
	l.serverLock.Unlock()
	if shouldStartServer {
	// open a random port and start the server if SIGUSR1 is received.
	if err := l.goServe(":", os.Stderr); err != nil {
	log.L().Warn("failed to start HTTP server", log.ShortError(err))
	}
	} else {
	// just prints the server address if it is already started.
	log.L().Info("already started HTTP server", zap.Stringer("address", l.serverAddr))
	}
	})
	l.serverLock.Lock()
	statusAddr := l.globalCfg.App.StatusAddr
	l.serverLock.Unlock()
	if len(statusAddr) == 0 {
	return nil
	}
	return l.goServe(statusAddr, ioutil.Discard)
	}
	func (l *Lightning) goServe(statusAddr string, realAddrWriter io.Writer) error {
	mux := http.NewServeMux()
	mux.Handle("/", http.RedirectHandler("/web/", http.StatusFound))
	mux.Handle("/metrics", promhttp.Handler())
	mux.HandleFunc("/debug/pprof/", pprof.Index)
	mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline)
	mux.HandleFunc("/debug/pprof/profile", pprof.Profile)
	mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol)
	mux.HandleFunc("/debug/pprof/trace", pprof.Trace)
	handleTasks := http.StripPrefix("/tasks", http.HandlerFunc(l.handleTask))
	mux.Handle("/tasks", handleTasks)
	mux.Handle("/tasks/", handleTasks)
	mux.HandleFunc("/progress/task", handleProgressTask)
	mux.HandleFunc("/progress/table", handleProgressTable)
	mux.HandleFunc("/pause", handlePause)
	mux.HandleFunc("/resume", handleResume)
	mux.HandleFunc("/loglevel", handleLogLevel)
	mux.Handle("/web/", http.StripPrefix("/web", httpgzip.FileServer(web.Res, httpgzip.FileServerOptions{
	IndexHTML: true,
	ServeError: func(w http.ResponseWriter, req *http.Request, err error) {
	if os.IsNotExist(err) && !strings.Contains(req.URL.Path, ".") {
	http.Redirect(w, req, "/web/", http.StatusFound)
	} else {
	httpgzip.NonSpecific(w, req, err)
	}
	},
	})))
	listener, err := net.Listen("tcp", statusAddr)
	if err != nil {
	return err
	}
	l.serverAddr = listener.Addr()
	log.L().Info("starting HTTP server", zap.Stringer("address", l.serverAddr))
	fmt.Fprintln(realAddrWriter, "started HTTP server on", l.serverAddr)
	l.server.Handler = mux
	listener = l.globalTLS.WrapListener(listener)
	go func() {
	err := l.server.Serve(listener)
	log.L().Info("stopped HTTP server", log.ShortError(err))
	}()
	return nil
	}

[YuJuncen]

@kennytm Seems there is strong coupling between this method and lightning... I'm afraid reuse it needs more refactor...

[kennytm]

i mean using this to handle TLS instead of if/else.

 	listener, err := net.Listen("tcp", statusAddr) 
        ...
 	listener = l.globalTLS.WrapListener(listener) 

 	go func() { 
 		err := l.server.Serve(listener) 
 		log.L().Info("stopped HTTP server", log.ShortError(err)) 
 	}()        

[YuJuncen]

i mean using this to handle TLS instead of if/else.

 	listener, err := net.Listen("tcp", statusAddr) 
        ...
 	listener = l.globalTLS.WrapListener(listener) 

 	go func() { 
 		err := l.server.Serve(listener) 
 		log.L().Info("stopped HTTP server", log.ShortError(err)) 
 	}()        

I'm not sure whether I fully understand you... You mean, TLS config as a global variable, and let it to wrap the listener? (Since there isn't a BR struct here, and the config of BR hasn't been exposed as a global variable too.)

Well, I think I got it.

[kennytm]

LGTM

[YuJuncen]

/merge

[ti-srebot]

/run-all-tests

[ti-srebot]

cherry pick to release-4.0 in PR #839