adds suggested changes

Signed-off-by: JU4N98 <juanpablocabana2@gmail.com>
spiffe · Oct 19, 2023 · 894cae6 · 894cae6
1 parent ed6cb00
commit 894cae6
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 102 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,56 +19,39 @@ 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))
 			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
 }