From 894cae6e7ad5a2d42ffceca9623d1f80c6b9ce5c Mon Sep 17 00:00:00 2001 From: JU4N98 Date: Thu, 19 Oct 2023 15:30:55 -0300 Subject: [PATCH] adds suggested changes Signed-off-by: JU4N98 --- pkg/sidecar/config.go | 13 ++-------- pkg/sidecar/config_test.go | 28 ---------------------- pkg/sidecar/util_posix.go | 48 +++++++++++++------------------------ pkg/sidecar/util_windows.go | 48 +++++++++++++------------------------ 4 files changed, 35 insertions(+), 102 deletions(-) diff --git a/pkg/sidecar/config.go b/pkg/sidecar/config.go index 9cf1732e..9683a44e 100644 --- a/pkg/sidecar/config.go +++ b/pkg/sidecar/config.go @@ -32,9 +32,8 @@ type Config struct { RenewSignalDeprecated string `hcl:"renewSignal"` // JWT configuration - JwtAudience string `hcl:"audience"` - JSONFilename string `hcl:"json_filename"` - JSONFilenameDeprecated string `hcl:"jsonFilename"` + JwtAudience string `hcl:"audience"` + JSONFilename string `hcl:"json_filename"` // TODO: is there a reason for this to be exposed? and inside of config? ReloadExternalProcess func() error @@ -120,14 +119,6 @@ func ValidateConfig(c *Config) error { c.RenewSignal = c.RenewSignalDeprecated } - if c.JSONFilenameDeprecated != "" { - if c.JSONFilename != "" { - return errors.New("use of json_filename and jsonFilename found, use only json_filename") - } - c.Log.Warn(getWarning("jsonFilename", "json_filename")) - c.JSONFilename = c.JSONFilenameDeprecated - } - switch { case c.SvidFileName == "": return errors.New("svid_file_name is required") diff --git a/pkg/sidecar/config_test.go b/pkg/sidecar/config_test.go index 5aa02585..1a225052 100644 --- a/pkg/sidecar/config_test.go +++ b/pkg/sidecar/config_test.go @@ -159,19 +159,6 @@ func TestValidateConfig(t *testing.T) { }, expectError: "use of renew_signal and renewSignal found, use only renew_signal", }, - { - name: "Using JSONFilenameDeprecated", - config: &Config{ - AgentAddress: "path", - SvidFileName: "cert.pem", - SvidKeyFileName: "key.pem", - SvidBundleFileName: "bundle.pem", - RenewSignal: "SIGHUP", - JSONFilename: "cert.json", - JSONFilenameDeprecated: "cert.json", - }, - expectError: "use of jsonFilename and json_filename found, use only json_filename", - }, // Deprecated field warning: { name: "Using AgentAddressDeprecated", @@ -279,21 +266,6 @@ func TestValidateConfig(t *testing.T) { Message: "renewSignal will be deprecated, should be used as renew_signal", }}, }, - { - name: "Using JSONFilenameDeprecated", - config: &Config{ - AgentAddress: "path", - SvidFileName: "cert.pem", - SvidKeyFileName: "key.pem", - SvidBundleFileName: "bundle.pem", - RenewSignal: "SIGHUP", - JSONFilenameDeprecated: "cert.json", - }, - expectLogs: []shortEntry{{ - Level: logrus.WarnLevel, - Message: "jsonFilename will be deprecated, should be used as json_filename", - }}, - }, } { t.Run(tt.name, func(t *testing.T) { log, hook := test.NewNullLogger() diff --git a/pkg/sidecar/util_posix.go b/pkg/sidecar/util_posix.go index 3f52ffb4..f28db90b 100644 --- a/pkg/sidecar/util_posix.go +++ b/pkg/sidecar/util_posix.go @@ -10,6 +10,8 @@ import ( "github.com/spiffe/go-spiffe/v2/workloadapi" "golang.org/x/sys/unix" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // RunDaemon starts the main loop @@ -18,56 +20,38 @@ import ( // are stored in disk and a restart signal is sent to the proxy's process func (s *Sidecar) RunDaemon(ctx context.Context) error { var wg sync.WaitGroup - ctx, cancel := context.WithCancel(ctx) - defer func() { - cancel() - wg.Wait() - }() - tasks := 0 - if s.config.SvidFileName != "" && s.config.SvidKeyFileName != "" && s.config.SvidBundleFileName != "" { - tasks++ - } - if s.config.JSONFilename != "" { - tasks++ - } - if s.config.JSONFilename != "" && s.config.JwtAudience != "" { - tasks++ - } - wg.Add(tasks) - - errch := make(chan error, tasks) if s.config.SvidFileName != "" && s.config.SvidKeyFileName != "" && s.config.SvidBundleFileName != "" { + wg.Add(1) go func() { defer wg.Done() - errch <- workloadapi.WatchX509Context(ctx, &x509Watcher{sidecar: s}, workloadapi.WithAddr("unix://"+s.config.AgentAddress)) + err := workloadapi.WatchX509Context(ctx, &x509Watcher{sidecar: s}, workloadapi.WithAddr("unix://"+s.config.AgentAddress)) + if err != nil && status.Code(err) != codes.Canceled { + s.config.Log.Errorf("Error watching X.509 context: %w", err) + } }() } + if s.config.JSONFilename != "" { + wg.Add(1) go func() { defer wg.Done() - errch <- workloadapi.WatchJWTBundles(ctx, &JWTBundlesWatcher{sidecar: s}, workloadapi.WithAddr("unix://"+s.config.AgentAddress)) + err := workloadapi.WatchJWTBundles(ctx, &JWTBundlesWatcher{sidecar: s}, workloadapi.WithAddr("unix://"+s.config.AgentAddress)) + if err != nil && status.Code(err) != codes.Canceled { + s.config.Log.Fatalf("Error watching JWT bundles updates: %w", err) + } }() } + if s.config.JSONFilename != "" && s.config.JwtAudience != "" { + wg.Add(1) go func() { defer wg.Done() s.updateJWTSVID(ctx, workloadapi.WithAddr("unix://"+s.config.AgentAddress)) - errch <- nil }() } - for complete := 0; complete < tasks; { - select { - case <-ctx.Done(): - return ctx.Err() - case err := <-errch: - if err != nil { - return err - } - complete++ - } - } + wg.Wait() return nil } diff --git a/pkg/sidecar/util_windows.go b/pkg/sidecar/util_windows.go index 2c732f1c..43890600 100644 --- a/pkg/sidecar/util_windows.go +++ b/pkg/sidecar/util_windows.go @@ -6,8 +6,11 @@ package sidecar import ( "context" "errors" + "sync" "github.com/spiffe/go-spiffe/v2/workloadapi" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // RunDaemon starts the main loop @@ -16,38 +19,31 @@ import ( // are stored in disk and a restart signal is sent to the proxy's process func (s *Sidecar) RunDaemon(ctx context.Context) error { var wg sync.WaitGroup - ctx, cancel := context.WithCancel(ctx) - defer func() { - cancel() - wg.Wait() - }() - tasks := 0 - if s.config.SvidFileName != "" && s.config.SvidKeyFileName != "" && s.config.SvidBundleFileName != "" { - tasks++ - } - if s.config.JSONFilename != "" { - tasks++ - } - if s.config.JSONFilename != "" && s.config.JwtAudience != "" { - tasks++ - } - wg.Add(tasks) - - errch := make(chan error, tasks) if s.config.SvidFileName != "" && s.config.SvidKeyFileName != "" && s.config.SvidBundleFileName != "" { + wg.add(1) go func() { defer wg.Done() - errch <- workloadapi.WatchX509Context(ctx, &x509Watcher{sidecar: s}, workloadapi.WithNamedPipeName(s.config.AgentAddress)) + err := workloadapi.WatchX509Context(ctx, &x509Watcher{sidecar: s}, workloadapi.WithNamedPipeName(s.config.AgentAddress)) + if err != nil && status.Code(err) != codes.Canceled { + s.config.Log.Errorf("Error watching X.509 context: %w", err) + } }() } + if s.config.JSONFilename != "" { + wg.Add(1) go func() { defer wg.Done() - errch <- workloadapi.WatchJWTBundles(ctx, &JWTBundlesWatcher{sidecar: s}, workloadapi.WithNamedPipeName(s.config.AgentAddress)) + err := workloadapi.WatchJWTBundles(ctx, &JWTBundlesWatcher{sidecar: s}, workloadapi.WithNamedPipeName(s.config.AgentAddress)) + if err != nil && status.Code(err) != codes.Canceled { + s.config.Log.Fatalf("Error watching JWT bundles updates: %w", err) + } }() } + if s.config.JSONFilename != "" && s.config.JwtAudience != "" { + wg.Add(1) go func() { defer wg.Done() s.updateJWTSVID(ctx, workloadapi.WithNamedPipeName(s.config.AgentAddress)) @@ -55,17 +51,7 @@ func (s *Sidecar) RunDaemon(ctx context.Context) error { }() } - for complete := 0; complete < tasks; { - select { - case <-ctx.Done(): - return ctx.Err() - case err := <-errch: - if err != nil { - return err - } - complete++ - } - } + wg.Wait() return nil }