[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #245: Introducing TLS in HTTP server. Resolves : apache/skywalking [BanyanDB]#9759

["hanahmily (via GitHub)" <gi...@apache.org>]

hanahmily commented on code in PR #245:
URL: https://github.com/apache/skywalking-banyandb/pull/245#discussion_r1089942598


##########
banyand/liaison/http/server.go:
##########
@@ -61,12 +61,17 @@ type service struct {
 	srv          *http.Server
 	listenAddr   string
 	grpcAddr     string
+	creds        credentials.TransportCredentials

Review Comment:
   You didn't initialize `creds`. 



##########
banyand/liaison/http/server.go:
##########
@@ -130,7 +134,7 @@ func (p *service) Serve() run.StopNotify {
 	p.mux.Mount("/api", http.StripPrefix("/api", gwMux))
 	go func() {
 		p.l.Info().Str("listenAddr", p.listenAddr).Msg("Start liaison http server")
-		if err := p.srv.ListenAndServe(); err != http.ErrServerClosed {
+		if err := http.ListenAndServeTLS(p.listenAddr, p.certFile, p.keyFile, p.srv.Handler); err != http.ErrServerClosed {

Review Comment:
   You had better add conditional branches to boot a TLS server or plain server based on `p.tls`.



##########
banyand/liaison/http/server.go:
##########
@@ -61,12 +61,17 @@ type service struct {
 	srv          *http.Server
 	listenAddr   string
 	grpcAddr     string
+	creds        credentials.TransportCredentials
+	keyFile      string
+	certFile     string
 }
 
 func (p *service) FlagSet() *run.FlagSet {
 	flagSet := run.NewFlagSet("")
 	flagSet.StringVar(&p.listenAddr, "http-addr", ":17913", "listen addr for http")
 	flagSet.StringVar(&p.grpcAddr, "grpc-addr", "localhost:17912", "the grpc addr")
+	flagSet.StringVarP(&p.certFile, "cert-file", "", "", "the TLS cert file")
+	flagSet.StringVarP(&p.keyFile, "key-file", "", "", "the TLS key file")

Review Comment:
   A new flag, `grpc-cert-file` should be added. Then you could build `p.cerds` based on this cert file.



##########
banyand/liaison/http/server.go:
##########
@@ -61,12 +61,17 @@ type service struct {
 	srv          *http.Server
 	listenAddr   string
 	grpcAddr     string
+	creds        credentials.TransportCredentials
+	keyFile      string
+	certFile     string
 }
 
 func (p *service) FlagSet() *run.FlagSet {
 	flagSet := run.NewFlagSet("")
 	flagSet.StringVar(&p.listenAddr, "http-addr", ":17913", "listen addr for http")
 	flagSet.StringVar(&p.grpcAddr, "grpc-addr", "localhost:17912", "the grpc addr")
+	flagSet.StringVarP(&p.certFile, "cert-file", "", "", "the TLS cert file")
+	flagSet.StringVarP(&p.keyFile, "key-file", "", "", "the TLS key file")

Review Comment:
   And another flag, `tls`, is needed. Its type is `bool`, turning on/off the TLS mode.



##########
banyand/liaison/http/server.go:
##########
@@ -130,7 +134,7 @@ func (p *service) Serve() run.StopNotify {
 	p.mux.Mount("/api", http.StripPrefix("/api", gwMux))
 	go func() {
 		p.l.Info().Str("listenAddr", p.listenAddr).Msg("Start liaison http server")
-		if err := p.srv.ListenAndServe(); err != http.ErrServerClosed {
+		if err := http.ListenAndServeTLS(p.listenAddr, p.certFile, p.keyFile, p.srv.Handler); err != http.ErrServerClosed {

Review Comment:
   use `p.svr.ListenAndServerTLS` in place of `http.ListenAndServerTLS`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org