You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "DevPJ9 (via GitHub)" <gi...@apache.org> on 2023/01/26 09:01:27 UTC
[GitHub] [skywalking-banyandb] DevPJ9 opened a new pull request, #245: Introducing TLS in HTTP server. Resolves : apache/skywalking [BanyanDB]#9759
DevPJ9 opened a new pull request, #245:
URL: https://github.com/apache/skywalking-banyandb/pull/245
**Description :**
Adding TLS in HTTP server.
**Implementation**
- Added keyFile and certFile strings in service struct.
- replaced `grpc.WithTransportCredentials(insecure.NewCredentials())` with `grpc.WithTransportCredentials(p.creds)`
- replaced `p.srv.ListenAndServe()` with `http.ListenAndServeTLS(p.listenAddr, p.certFile, p.keyFile, p.srv.Handler)`
**TODO:**
- Make Commands Executions
- Adding HTTP pk in flagset.
--
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
[GitHub] [skywalking-banyandb] hanahmily merged pull request #245: Introducing TLS in HTTP server. Resolves : apache/skywalking [BanyanDB]#9759
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #245:
URL: https://github.com/apache/skywalking-banyandb/pull/245
--
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
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #245: Introducing TLS in HTTP server. Resolves : apache/skywalking [BanyanDB]#9759
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on code in PR #245:
URL: https://github.com/apache/skywalking-banyandb/pull/245#discussion_r1091467965
##########
banyand/liaison/http/server.go:
##########
@@ -61,16 +62,30 @@ type service struct {
srv *http.Server
listenAddr string
grpcAddr string
+ creds credentials.TransportCredentials
+ keyFile string
+ certFile string
+ grpcCert string
+ tlsEnabled bool
}
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")
+ flagSet.StringVarP(&p.grpcCert, "grpc-cert-file", "", "", "the GRPC Certfile")
+ flagSet.BoolVarP(&p.tlsEnabled, "tlsEnabled", "", false, "the tls enabling option")
return flagSet
}
func (p *service) Validate() error {
+ creds, errTLS := credentials.NewClientTLSFromFile(p.grpcCert, "")
Review Comment:
Please check whether the grcpCert file exists before loading it. That's the main reason for testing failure.
##########
banyand/liaison/http/server.go:
##########
@@ -61,16 +62,30 @@ type service struct {
srv *http.Server
listenAddr string
grpcAddr string
+ creds credentials.TransportCredentials
+ keyFile string
+ certFile string
+ grpcCert string
+ tlsEnabled bool
}
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")
+ flagSet.StringVarP(&p.grpcCert, "grpc-cert-file", "", "", "the GRPC Certfile")
+ flagSet.BoolVarP(&p.tlsEnabled, "tlsEnabled", "", false, "the tls enabling option")
return flagSet
}
func (p *service) Validate() error {
+ creds, errTLS := credentials.NewClientTLSFromFile(p.grpcCert, "")
+ if errTLS != nil {
+ return errors.Wrap(errTLS, "failed to load cert and key")
Review Comment:
The error message needs to be more accurate. `"failed to load the grpc cert"` is better.
##########
banyand/liaison/http/server.go:
##########
@@ -130,9 +144,16 @@ 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 {
- p.l.Error().Err(err)
+ if p.tlsEnabled {
+ if err := p.srv.ListenAndServeTLS(p.certFile, p.keyFile); err != http.ErrServerClosed {
+ p.l.Error().Err(err)
+ }
+ } else {
+ if err := p.srv.ListenAndServe(); err != http.ErrServerClosed {
+ p.l.Error().Err(err)
+ }
}
Review Comment:
```suggestion
var err error
if p.tlsEnabled {
err = p.srv.ListenAndServeTLS(p.certFile, p.keyFile)
} else {
err = p.srv.ListenAndServe()
}
if err != http.ErrServerClosed {
p.l.Error().Err(err)
}
```
##########
banyand/liaison/http/server.go:
##########
@@ -61,16 +62,30 @@ type service struct {
srv *http.Server
listenAddr string
grpcAddr string
+ creds credentials.TransportCredentials
+ keyFile string
+ certFile string
+ grpcCert string
+ tlsEnabled bool
}
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")
+ flagSet.StringVarP(&p.grpcCert, "grpc-cert-file", "", "", "the GRPC Certfile")
+ flagSet.BoolVarP(&p.tlsEnabled, "tlsEnabled", "", false, "the tls enabling option")
Review Comment:
The flag's name should follow the grpc package's convention, which is "tls".
##########
banyand/liaison/http/server.go:
##########
@@ -61,16 +62,30 @@ type service struct {
srv *http.Server
listenAddr string
grpcAddr string
+ creds credentials.TransportCredentials
+ keyFile string
+ certFile string
+ grpcCert string
+ tlsEnabled bool
}
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")
+ flagSet.StringVarP(&p.grpcCert, "grpc-cert-file", "", "", "the GRPC Certfile")
+ flagSet.BoolVarP(&p.tlsEnabled, "tlsEnabled", "", false, "the tls enabling option")
Review Comment:
The flag's name should follow the grpc package's convention, which is "tls".
##########
banyand/liaison/http/server.go:
##########
@@ -102,8 +117,7 @@ func (p *service) Serve() run.StopNotify {
var ctx context.Context
ctx, p.clientCloser = context.WithCancel(context.Background())
opts := []grpc.DialOption{
- // TODO: add TLS
- grpc.WithTransportCredentials(insecure.NewCredentials()),
+ grpc.WithTransportCredentials(p.creds),
Review Comment:
```suggestion
opts := make([]grpc.DialOption, 0, 1)
if p.cerds == nil {
opts = append(opts, insecure.NewCredentials())
} else {
opts = append(opts, grpc.WithTransportCredentials(p.creds))
}
```
--
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
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #245: Introducing TLS in HTTP server. Resolves : apache/skywalking [BanyanDB]#9759
Posted by "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
[GitHub] [skywalking-banyandb] codecov-commenter commented on pull request #245: Introducing TLS in HTTP server. Resolves : apache/skywalking [BanyanDB]#9759
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #245:
URL: https://github.com/apache/skywalking-banyandb/pull/245#issuecomment-1414619372
# [Codecov](https://codecov.io/gh/apache/skywalking-banyandb/pull/245?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#245](https://codecov.io/gh/apache/skywalking-banyandb/pull/245?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8ffa04) into [main](https://codecov.io/gh/apache/skywalking-banyandb/commit/e98f6c6c25792692560c2e3b28dfdedd3a60327f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e98f6c6) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #245 +/- ##
=======================================
Coverage 45.88% 45.88%
=======================================
Files 87 87
Lines 8896 8896
=======================================
Hits 4082 4082
Misses 4430 4430
Partials 384 384
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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