hanahmily commented on code in PR #642:
URL:
https://github.com/apache/skywalking-banyandb/pull/642#discussion_r2036902574
##########
banyand/liaison/http/server.go:
##########
@@ -134,27 +136,52 @@ func (p *server) GetPort() *uint32 {
func (p *server) PreRun(_ context.Context) error {
p.l = logger.GetLogger(p.Name())
+ p.l.Info().Str("level", p.l.GetLevel().String()).Msg("Logger
initialized")
+
+ // Log flag values after parsing
+ p.l.Debug().Bool("tls", p.tls).Str("certFile",
p.certFile).Str("keyFile", p.keyFile).Msg("Flag values after parsing")
+
+ // Initialize TLSReloader if TLS is enabled
+ p.l.Debug().Bool("tls", p.tls).Msg("HTTP TLS flag is set")
+ if p.tls {
+ p.l.Debug().Str("certFile", p.certFile).Str("keyFile",
p.keyFile).Msg("Initializing TLSReloader for HTTP")
+ var err error
+ p.tlsReloader, err = pkgtls.NewReloader(p.certFile, p.keyFile,
p.l)
+ if err != nil {
+ p.l.Error().Err(err).Msg("Failed to initialize
TLSReloader for HTTP")
+ return err
+ }
+ } else {
+ p.l.Warn().Msg("HTTP TLS is disabled, skipping TLSReloader
initialization")
+ }
+
p.mux = chi.NewRouter()
if err := p.setRootPath(); err != nil {
return err
}
+
+ // Configure the HTTP server with dynamic TLS if enabled
p.srv = &http.Server{
Addr: p.listenAddr,
Handler: p.mux,
ReadHeaderTimeout: 3 * time.Second,
}
+ if p.tls {
+ p.srv.TLSConfig = p.tlsReloader.GetTLSConfig()
Review Comment:
Please load the creds dynamically as the gRPC did.
##########
pkg/tls/reloader_test.go:
##########
@@ -0,0 +1,163 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package tls
+
+import (
+ "crypto/rand"
+ "crypto/rsa"
+ "crypto/x509"
+ "crypto/x509/pkix"
+ "encoding/pem"
+ "math/big"
+ "os"
+ "path/filepath"
+ "testing"
+ "time"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+
+ "github.com/apache/skywalking-banyandb/pkg/logger"
+)
+
+func generateSelfSignedCert(t *testing.T, commonName string) (certPEM, keyPEM
[]byte) {
+ t.Helper()
+
+ privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
+ require.NoError(t, err)
+
+ template := x509.Certificate{
+ SerialNumber: big.NewInt(1),
+ Subject: pkix.Name{
+ CommonName: commonName,
+ },
+ DNSNames: []string{commonName},
+ NotBefore: time.Now(),
+ NotAfter: time.Now().Add(time.Hour * 24),
+ KeyUsage: x509.KeyUsageKeyEncipherment |
x509.KeyUsageDigitalSignature,
+ ExtKeyUsage:
[]x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+ BasicConstraintsValid: true,
+ }
+
+ certDER, err := x509.CreateCertificate(rand.Reader, &template,
&template, &privateKey.PublicKey, privateKey)
+ require.NoError(t, err)
+
+ certPEM = pem.EncodeToMemory(&pem.Block{
+ Type: "CERTIFICATE",
+ Bytes: certDER,
+ })
+
+ keyPEM = pem.EncodeToMemory(&pem.Block{
+ Type: "RSA PRIVATE KEY",
+ Bytes: x509.MarshalPKCS1PrivateKey(privateKey),
+ })
+
+ return certPEM, keyPEM
+}
+
+func TestReloader_CertificateRotation(t *testing.T) {
+ tempDir, err := os.MkdirTemp("", "tls-test-")
+ require.NoError(t, err)
+ defer os.RemoveAll(tempDir)
+
+ certFile := filepath.Join(tempDir, "cert.pem")
+ keyFile := filepath.Join(tempDir, "key.pem")
+
+ certPEM1, keyPEM1 := generateSelfSignedCert(t, "test1.local")
+ err = os.WriteFile(certFile, certPEM1, 0o600)
+ require.NoError(t, err)
+ err = os.WriteFile(keyFile, keyPEM1, 0o600)
+ require.NoError(t, err)
+
+ log := logger.GetLogger("tls-test")
+ reloader, err := NewReloader(certFile, keyFile, log)
+ require.NoError(t, err)
+
+ defer reloader.Stop()
+
+ initialCert, err := reloader.GetCertificate(nil)
+ require.NoError(t, err)
+ leafCert, err := x509.ParseCertificate(initialCert.Certificate[0])
+ require.NoError(t, err)
+ assert.Equal(t, "test1.local", leafCert.Subject.CommonName)
+
+ certPEM2, keyPEM2 := generateSelfSignedCert(t, "test2.local")
+ err = os.WriteFile(certFile, certPEM2, 0o600)
+ require.NoError(t, err)
+ err = os.WriteFile(keyFile, keyPEM2, 0o600)
+ require.NoError(t, err)
+
+ err = reloader.reloadCertificate()
+ require.NoError(t, err)
+
+ updatedCert, err := reloader.GetCertificate(nil)
+ require.NoError(t, err)
+ leafCert, err = x509.ParseCertificate(updatedCert.Certificate[0])
+ require.NoError(t, err)
+ assert.Equal(t, "test2.local", leafCert.Subject.CommonName)
+
+ tlsConfig := reloader.GetTLSConfig()
+ configCert, err := tlsConfig.GetCertificate(nil)
+ require.NoError(t, err)
+ leafCert, err = x509.ParseCertificate(configCert.Certificate[0])
+ require.NoError(t, err)
+ assert.Equal(t, "test2.local", leafCert.Subject.CommonName)
+}
+
+func TestReloader_FileWatcher(t *testing.T) {
Review Comment:
This case is exactly the same as "TestReloader_CertificateRotation," with
the only difference being the method used to get the certificate. Since I
suggest converting getCert to an unexported method, you can remove this function
##########
pkg/tls/reloader_test.go:
##########
@@ -0,0 +1,163 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package tls
+
+import (
+ "crypto/rand"
+ "crypto/rsa"
+ "crypto/x509"
+ "crypto/x509/pkix"
+ "encoding/pem"
+ "math/big"
+ "os"
+ "path/filepath"
+ "testing"
+ "time"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+
+ "github.com/apache/skywalking-banyandb/pkg/logger"
+)
+
+func generateSelfSignedCert(t *testing.T, commonName string) (certPEM, keyPEM
[]byte) {
+ t.Helper()
+
+ privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
+ require.NoError(t, err)
+
+ template := x509.Certificate{
+ SerialNumber: big.NewInt(1),
+ Subject: pkix.Name{
+ CommonName: commonName,
+ },
+ DNSNames: []string{commonName},
+ NotBefore: time.Now(),
+ NotAfter: time.Now().Add(time.Hour * 24),
+ KeyUsage: x509.KeyUsageKeyEncipherment |
x509.KeyUsageDigitalSignature,
+ ExtKeyUsage:
[]x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+ BasicConstraintsValid: true,
+ }
+
+ certDER, err := x509.CreateCertificate(rand.Reader, &template,
&template, &privateKey.PublicKey, privateKey)
+ require.NoError(t, err)
+
+ certPEM = pem.EncodeToMemory(&pem.Block{
+ Type: "CERTIFICATE",
+ Bytes: certDER,
+ })
+
+ keyPEM = pem.EncodeToMemory(&pem.Block{
+ Type: "RSA PRIVATE KEY",
+ Bytes: x509.MarshalPKCS1PrivateKey(privateKey),
+ })
+
+ return certPEM, keyPEM
+}
+
+func TestReloader_CertificateRotation(t *testing.T) {
Review Comment:
You only test updating file. But you didn't cover following cases:
1. Remove the old files and create new files
2. Create files after the reloader starts
3. Remove files and don't create new files.
4. Create invalid files.
##########
banyand/liaison/grpc/server.go:
##########
@@ -232,18 +244,24 @@ func (s *server) Validate() error {
if s.keyFile == "" {
return errServerKey
}
- creds, errTLS := credentials.NewServerTLSFromFile(s.certFile, s.keyFile)
- if errTLS != nil {
- return errors.Wrap(errTLS, "failed to load cert and key")
- }
- s.creds = creds
return nil
}
func (s *server) Serve() run.StopNotify {
var opts []grpclib.ServerOption
if s.tls {
- opts = []grpclib.ServerOption{grpclib.Creds(s.creds)}
+ if err := s.tlsReloader.Start(); err != nil {
+ s.log.Error().Err(err).Msg("Failed to start TLSReloader
for gRPC")
+ close(s.stopCh)
+ return s.stopCh
+ }
+ s.log.Info().Str("certFile", s.certFile).Str("keyFile",
s.keyFile).Msg("Starting TLS file monitoring")
+ tlsConfig := &tls.Config{
Review Comment:
You can use tlsReloader.GetTLSConfig() here
##########
test/stress/istio/testdata/metrics/data.csv:
##########
@@ -151,4 +151,4 @@
242130464.000000,278732800.000000,892029384.000000,2228224.000000,1812.150000,63812001792.000000,132011507712.000000,114327.000000,29935.000000,926433.000000,956611.000000,3129891328.000000,91122723840.000000,461544.000000,6676148.000000
243407136.000000,280125440.000000,892029384.000000,2228224.000000,1812.210000,63812001792.000000,132011507712.000000,114327.000000,29935.000000,926484.000000,956637.000000,3129891328.000000,91123096576.000000,461547.000000,6676190.000000
243978768.000000,280608768.000000,892029384.000000,2228224.000000,1812.260000,63812001792.000000,132011507712.000000,114327.000000,29935.000000,926505.000000,956670.000000,3129891328.000000,91123317760.000000,461550.000000,6676209.000000
-371007152.000000,387579904.000000,892029384.000000,2293760.000000,1850.930000,63828652032.000000,132011507712.000000,114327.000000,29935.000000,926789.000000,957052.000000,3129891328.000000,91145636864.000000,461655.000000,6676573.000000
Review Comment:
Resort this file.
##########
pkg/tls/reloader.go:
##########
@@ -0,0 +1,200 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Package tls provides common TLS utilities for HTTP and gRPC servers.
+package tls
+
+import (
+ "crypto/tls"
+ "sync"
+
+ "github.com/fsnotify/fsnotify"
+ "github.com/pkg/errors"
+ "google.golang.org/grpc/credentials"
+
+ "github.com/apache/skywalking-banyandb/pkg/logger"
+)
+
+// Config contains TLS configuration options.
+type Config struct {
+ CertFile string
+ KeyFile string
+ Enabled bool
+}
+
+// Reloader manages dynamic reloading of TLS certificates and keys for servers.
+type Reloader struct {
+ watcher *fsnotify.Watcher
+ cert *tls.Certificate
+ log *logger.Logger
+ certFile string
+ keyFile string
+ mu sync.RWMutex
+}
+
+// NewReloader creates a new TLSReloader instance.
+func NewReloader(certFile, keyFile string, log *logger.Logger) (*Reloader,
error) {
+ if certFile == "" || keyFile == "" {
+ return nil, errors.New("certFile and keyFile must be provided")
+ }
+ if log == nil {
+ return nil, errors.New("logger must not be nil")
+ }
+
+ watcher, err := fsnotify.NewWatcher()
+ if err != nil {
+ return nil, errors.Wrap(err, "failed to create fsnotify
watcher")
+ }
+
+ cert, err := tls.LoadX509KeyPair(certFile, keyFile)
+ if err != nil {
+ watcher.Close()
+ return nil, errors.Wrap(err, "failed to load initial TLS
certificate")
+ }
+
+ log.Info().Str("certFile", certFile).Str("keyFile",
keyFile).Msg("Successfully loaded initial TLS certificates")
+
+ tr := &Reloader{
+ certFile: certFile,
+ keyFile: keyFile,
+ cert: &cert,
+ log: log,
+ watcher: watcher,
+ }
+
+ return tr, nil
+}
+
+// Start begins monitoring the TLS certificate and key files for changes.
+func (r *Reloader) Start() error {
+ r.log.Info().Str("certFile", r.certFile).Str("keyFile",
r.keyFile).Msg("Starting TLS file monitoring")
+
+ err := r.watcher.Add(r.certFile)
+ if err != nil {
+ return errors.Wrapf(err, "failed to watch cert file: %s",
r.certFile)
+ }
+
+ err = r.watcher.Add(r.keyFile)
+ if err != nil {
+ return errors.Wrapf(err, "failed to watch key file: %s",
r.keyFile)
+ }
+
+ go r.watchFiles()
+
+ return nil
+}
+
+func (r *Reloader) watchFiles() {
+ r.log.Info().Msg("TLS file watcher loop started")
+ for {
+ select {
+ case event, ok := <-r.watcher.Events:
+ if !ok {
+ r.log.Warn().Msg("Watcher events channel closed
unexpectedly")
+ return
+ }
+
+ r.log.Debug().Str("file", event.Name).Str("op",
event.Op.String()).Msg("Detected file event")
+
+ if event.Op&fsnotify.Remove == fsnotify.Remove {
+ r.log.Info().Str("file", event.Name).Msg("File
removed, re-adding to watcher")
+ if event.Name == r.certFile {
+ if err := r.watcher.Add(r.certFile);
err != nil {
+
r.log.Error().Err(err).Str("file", r.certFile).Msg("Failed to re-add cert file
to watcher")
+ } else {
+ r.log.Debug().Str("file",
r.certFile).Msg("Re-added cert file to watcher")
+ }
+ } else if event.Name == r.keyFile {
+ if err := r.watcher.Add(r.keyFile); err
!= nil {
+
r.log.Error().Err(err).Str("file", r.keyFile).Msg("Failed to re-add key file to
watcher")
+ } else {
+ r.log.Debug().Str("file",
r.keyFile).Msg("Re-added key file to watcher")
+ }
+ }
+ }
+
+ if event.Op&fsnotify.Write == fsnotify.Write ||
+ event.Op&fsnotify.Rename == fsnotify.Rename ||
+ event.Op&fsnotify.Create == fsnotify.Create {
+ r.log.Info().Str("file",
event.Name).Msg("Detected certificate change")
+ if err := r.reloadCertificate(); err != nil {
+ r.log.Error().Err(err).Str("file",
event.Name).Msg("Failed to reload TLS certificate")
+ } else {
+ r.log.Info().Str("file",
event.Name).Msg("Successfully updated TLS certificate")
+ }
+ }
+
+ case err, ok := <-r.watcher.Errors:
+ if !ok {
+ r.log.Warn().Msg("Watcher errors channel closed
unexpectedly")
+ return
+ }
+ r.log.Error().Err(err).Msg("Error in file watcher")
+ }
+ }
+}
+
+func (r *Reloader) reloadCertificate() error {
+ r.log.Debug().Msg("Attempting to reload TLS certificate")
+ newCert, err := tls.LoadX509KeyPair(r.certFile, r.keyFile)
+ if err != nil {
+ return errors.Wrap(err, "failed to reload TLS certificate")
+ }
+
+ r.mu.Lock()
+ r.cert = &newCert
+ r.mu.Unlock()
+
+ r.log.Debug().Msg("TLS certificate updated in memory")
+ return nil
+}
+
+// GetCertificate returns the current TLS certificate for TLS Config's
GetCertificate callback.
+func (r *Reloader) GetCertificate(*tls.ClientHelloInfo) (*tls.Certificate,
error) {
Review Comment:
```suggestion
func (r *Reloader) getCertificate(*tls.ClientHelloInfo) (*tls.Certificate,
error) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
