diff --git a/pkg/sidecar/sidecar.go b/pkg/sidecar/sidecar.go index d8dcb52d..e6383698 100644 --- a/pkg/sidecar/sidecar.go +++ b/pkg/sidecar/sidecar.go @@ -11,7 +11,6 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "time" "github.com/spiffe/go-spiffe/v2/bundle/jwtbundle" @@ -28,10 +27,12 @@ type Sidecar struct { config *Config client *workloadapi.Client jwtSource *workloadapi.JWTSource - processRunning int32 + processRunning bool process *os.Process certReadyChan chan struct{} health Health + + mu sync.Mutex } type Health struct { @@ -232,7 +233,10 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { // signalProcessCMD sends the renew signal to the process or starts it if its first time func (s *Sidecar) signalProcess() error { - if atomic.LoadInt32(&s.processRunning) == 0 { + s.mu.Lock() + defer s.mu.Unlock() + + if !s.processRunning { cmdArgs, err := getCmdArgs(s.config.CmdArgs) if err != nil { return fmt.Errorf("error parsing cmd arguments: %w", err) @@ -245,6 +249,7 @@ func (s *Sidecar) signalProcess() error { return fmt.Errorf("error executing process \"%v\": %w", s.config.Cmd, err) } s.process = cmd.Process + s.processRunning = true go s.checkProcessExit() } else { if err := SignalProcess(s.process, s.config.RenewSignal); err != nil { @@ -276,13 +281,13 @@ func (s *Sidecar) signalPID() error { } func (s *Sidecar) checkProcessExit() { - atomic.StoreInt32(&s.processRunning, 1) - _, err := s.process.Wait() - if err != nil { + if _, err := s.process.Wait(); err != nil { s.config.Log.Errorf("error waiting for process exit: %v", err) } - atomic.StoreInt32(&s.processRunning, 0) + s.mu.Lock() + s.processRunning = false + s.mu.Unlock() } func (s *Sidecar) fetchJWTSVIDs(ctx context.Context, jwtAudience string, jwtExtraAudiences []string) (*jwtsvid.SVID, error) { diff --git a/pkg/sidecar/sidecar_test.go b/pkg/sidecar/sidecar_test.go index 473676ec..abea363e 100644 --- a/pkg/sidecar/sidecar_test.go +++ b/pkg/sidecar/sidecar_test.go @@ -269,3 +269,29 @@ func TestGetCmdArgs(t *testing.T) { }) } } + +// TestSignalProcess makes sure only one copy of the process is started. It uses a small script that creates a file +// where the name is the process ID of the script. If more then one file exists, then multiple processes were started +func TestSignalProcess(t *testing.T) { + tempDir := t.TempDir() + config := &Config{ + Cmd: "./sidecar_test.sh", + CmdArgs: tempDir, + RenewSignal: "SIGWINCH", + } + sidecar := New(config) + require.NotNil(t, sidecar) + + // Run signalProcess() twice. The second should only signal the process with SIGWINCH which is basically a no op. + err := sidecar.signalProcess() + require.NoError(t, err) + err = sidecar.signalProcess() + require.NoError(t, err) + + // Give the script some time to run + time.Sleep(1 * time.Second) + + files, err := os.ReadDir(tempDir) + require.NoError(t, err) + require.Equal(t, 1, len(files)) +} diff --git a/pkg/sidecar/sidecar_test.sh b/pkg/sidecar/sidecar_test.sh new file mode 100755 index 00000000..374e7da0 --- /dev/null +++ b/pkg/sidecar/sidecar_test.sh @@ -0,0 +1,3 @@ +#!/bin/bash + +touch $1/$$