From 5b9bd8e3ccc30e5fbb039008327935e7ea2797f7 Mon Sep 17 00:00:00 2001 From: Kevin Fox Date: Tue, 19 Dec 2023 07:48:52 -0800 Subject: [PATCH 1/7] Add support for signaling an external process via pid file Signed-off-by: Kevin Fox --- cmd/spiffe-helper/config/config.go | 1 + pkg/sidecar/config.go | 3 ++ pkg/sidecar/sidecar.go | 68 ++++++++++++++++++++---------- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/cmd/spiffe-helper/config/config.go b/cmd/spiffe-helper/config/config.go index f3f1b5c..1ef4ac0 100644 --- a/cmd/spiffe-helper/config/config.go +++ b/cmd/spiffe-helper/config/config.go @@ -26,6 +26,7 @@ type Config struct { AgentAddressDeprecated string `hcl:"agentAddress"` Cmd string `hcl:"cmd"` CmdArgs string `hcl:"cmd_args"` + PidFileName string `hcl:"pid_file_name"` CmdArgsDeprecated string `hcl:"cmdArgs"` CertDir string `hcl:"cert_dir"` CertDirDeprecated string `hcl:"certDir"` diff --git a/pkg/sidecar/config.go b/pkg/sidecar/config.go index 6de560c..8ffaa44 100644 --- a/pkg/sidecar/config.go +++ b/pkg/sidecar/config.go @@ -20,6 +20,9 @@ type Config struct { // The arguments of the process to launch. CmdArgs string + // Signal extral process via PID file + PidFileName string + // The directory name to store the x509s and/or JWTs. CertDir string diff --git a/pkg/sidecar/sidecar.go b/pkg/sidecar/sidecar.go index b56ef57..15bd614 100644 --- a/pkg/sidecar/sidecar.go +++ b/pkg/sidecar/sidecar.go @@ -1,11 +1,14 @@ package sidecar import ( + "bytes" "context" "encoding/csv" "fmt" "os" "os/exec" + "path" + "strconv" "strings" "sync" "sync/atomic" @@ -170,10 +173,12 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { } s.config.Log.Info("X.509 certificates updated") - if s.config.Cmd != "" { - if err := s.signalProcess(); err != nil { - s.config.Log.WithError(err).Error("Unable to signal process") - } + if err := s.signalProcess(); err != nil { + s.config.Log.WithError(err).Error("Unable to signal process") + } + + if s.config.ExitWhenReady { + os.Exit(0) } select { @@ -185,27 +190,46 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { // signalProcess sends the configured Renew signal to the process running the proxy // to reload itself so that the proxy uses the new SVID func (s *Sidecar) signalProcess() (err error) { + if s.config.PidFileName != "" { + byts, err := os.ReadFile(s.config.PidFileName) + if err != nil { + return fmt.Errorf("failed to read pid file: %s\n%w", s.config.PidFileName, err) + } + pid, err := strconv.Atoi(string(bytes.TrimSpace(byts))) + if err != nil { + return fmt.Errorf("failed to parse pid file: %s\n%w", s.config.PidFileName, err) + } + s.process, err = os.FindProcess(pid) + if err != nil { + return fmt.Errorf("failed to find process: %d\n%w", pid, err) + } + if err := s.SignalProcess(); err != nil { + return err + } + } // TODO: is ReloadExternalProcess still used? switch s.config.ReloadExternalProcess { case nil: - if atomic.LoadInt32(&s.processRunning) == 0 { - cmdArgs, err := getCmdArgs(s.config.CmdArgs) - if err != nil { - return fmt.Errorf("error parsing cmd arguments: %w", err) - } - - cmd := exec.Command(s.config.Cmd, cmdArgs...) // #nosec - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - err = cmd.Start() - if err != nil { - return fmt.Errorf("error executing process: %v\n%w", s.config.Cmd, err) - } - s.process = cmd.Process - go s.checkProcessExit() - } else { - if err := s.SignalProcess(); err != nil { - return err + if s.config.Cmd != "" { + if atomic.LoadInt32(&s.processRunning) == 0 { + cmdArgs, err := getCmdArgs(s.config.CmdArgs) + if err != nil { + return fmt.Errorf("error parsing cmd arguments: %w", err) + } + + cmd := exec.Command(s.config.Cmd, cmdArgs...) // #nosec + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + err = cmd.Start() + if err != nil { + return fmt.Errorf("error executing process: %v\n%w", s.config.Cmd, err) + } + s.process = cmd.Process + go s.checkProcessExit() + } else { + if err := s.SignalProcess(); err != nil { + return err + } } } From 38567208f5bd1a262b17f6f2b6cb59e8941cf4cb Mon Sep 17 00:00:00 2001 From: Faisal Memon Date: Tue, 24 Sep 2024 12:17:19 -0700 Subject: [PATCH 2/7] Fix rebase errors Signed-off-by: Faisal Memon --- pkg/sidecar/sidecar.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/sidecar/sidecar.go b/pkg/sidecar/sidecar.go index 15bd614..cc882a9 100644 --- a/pkg/sidecar/sidecar.go +++ b/pkg/sidecar/sidecar.go @@ -7,7 +7,6 @@ import ( "fmt" "os" "os/exec" - "path" "strconv" "strings" "sync" @@ -177,10 +176,6 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { s.config.Log.WithError(err).Error("Unable to signal process") } - if s.config.ExitWhenReady { - os.Exit(0) - } - select { case s.certReadyChan <- struct{}{}: default: From 0d53e759ae2ca6e9e013677e9c7fbc95e7396e49 Mon Sep 17 00:00:00 2001 From: Faisal Memon Date: Tue, 24 Sep 2024 12:30:26 -0700 Subject: [PATCH 3/7] Pass PID file name Signed-off-by: Faisal Memon --- cmd/spiffe-helper/config/config.go | 1 + pkg/sidecar/config.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/spiffe-helper/config/config.go b/cmd/spiffe-helper/config/config.go index 1ef4ac0..29aa14a 100644 --- a/cmd/spiffe-helper/config/config.go +++ b/cmd/spiffe-helper/config/config.go @@ -204,6 +204,7 @@ func NewSidecarConfig(config *Config, log logrus.FieldLogger) *sidecar.Config { AgentAddress: config.AgentAddress, Cmd: config.Cmd, CmdArgs: config.CmdArgs, + PidFileName: config.PidFileName, CertDir: config.CertDir, CertFileMode: fs.FileMode(config.CertFileMode), //nolint:gosec,G115 KeyFileMode: fs.FileMode(config.KeyFileMode), //nolint:gosec,G115 diff --git a/pkg/sidecar/config.go b/pkg/sidecar/config.go index 8ffaa44..8b667f6 100644 --- a/pkg/sidecar/config.go +++ b/pkg/sidecar/config.go @@ -20,7 +20,7 @@ type Config struct { // The arguments of the process to launch. CmdArgs string - // Signal extral process via PID file + // Signal external process via PID file PidFileName string // The directory name to store the x509s and/or JWTs. From eb234f11a271d6b6c07590a00ab14288145b6f17 Mon Sep 17 00:00:00 2001 From: Faisal Memon Date: Tue, 15 Oct 2024 16:20:24 -0700 Subject: [PATCH 4/7] Fix caps --- cmd/spiffe-helper/config/config.go | 4 ++-- pkg/sidecar/config.go | 2 +- pkg/sidecar/sidecar.go | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cmd/spiffe-helper/config/config.go b/cmd/spiffe-helper/config/config.go index 29aa14a..c8b1c34 100644 --- a/cmd/spiffe-helper/config/config.go +++ b/cmd/spiffe-helper/config/config.go @@ -26,7 +26,7 @@ type Config struct { AgentAddressDeprecated string `hcl:"agentAddress"` Cmd string `hcl:"cmd"` CmdArgs string `hcl:"cmd_args"` - PidFileName string `hcl:"pid_file_name"` + PIDFileName string `hcl:"pid_file_name"` CmdArgsDeprecated string `hcl:"cmdArgs"` CertDir string `hcl:"cert_dir"` CertDirDeprecated string `hcl:"certDir"` @@ -204,7 +204,7 @@ func NewSidecarConfig(config *Config, log logrus.FieldLogger) *sidecar.Config { AgentAddress: config.AgentAddress, Cmd: config.Cmd, CmdArgs: config.CmdArgs, - PidFileName: config.PidFileName, + PIDFileName: config.PIDFileName, CertDir: config.CertDir, CertFileMode: fs.FileMode(config.CertFileMode), //nolint:gosec,G115 KeyFileMode: fs.FileMode(config.KeyFileMode), //nolint:gosec,G115 diff --git a/pkg/sidecar/config.go b/pkg/sidecar/config.go index 8b667f6..9b95e32 100644 --- a/pkg/sidecar/config.go +++ b/pkg/sidecar/config.go @@ -21,7 +21,7 @@ type Config struct { CmdArgs string // Signal external process via PID file - PidFileName string + PIDFileName string // The directory name to store the x509s and/or JWTs. CertDir string diff --git a/pkg/sidecar/sidecar.go b/pkg/sidecar/sidecar.go index cc882a9..8e19413 100644 --- a/pkg/sidecar/sidecar.go +++ b/pkg/sidecar/sidecar.go @@ -185,14 +185,14 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { // signalProcess sends the configured Renew signal to the process running the proxy // to reload itself so that the proxy uses the new SVID func (s *Sidecar) signalProcess() (err error) { - if s.config.PidFileName != "" { - byts, err := os.ReadFile(s.config.PidFileName) + if s.config.PIDFileName != "" { + byts, err := os.ReadFile(s.config.PIDFileName) if err != nil { - return fmt.Errorf("failed to read pid file: %s\n%w", s.config.PidFileName, err) + return fmt.Errorf("failed to read pid file: %s\n%w", s.config.PIDFileName, err) } pid, err := strconv.Atoi(string(bytes.TrimSpace(byts))) if err != nil { - return fmt.Errorf("failed to parse pid file: %s\n%w", s.config.PidFileName, err) + return fmt.Errorf("failed to parse pid file: %s\n%w", s.config.PIDFileName, err) } s.process, err = os.FindProcess(pid) if err != nil { From f6358cc55d42b7955e7f63547f615e50193619be Mon Sep 17 00:00:00 2001 From: Faisal Memon Date: Tue, 15 Oct 2024 17:34:07 -0700 Subject: [PATCH 5/7] Reorg code Signed-off-by: Faisal Memon --- cmd/spiffe-helper/config/config.go | 12 ++-- pkg/sidecar/sidecar.go | 97 +++++++++++++++--------------- pkg/sidecar/util_posix.go | 17 ++---- pkg/sidecar/util_windows.go | 2 +- 4 files changed, 65 insertions(+), 63 deletions(-) diff --git a/cmd/spiffe-helper/config/config.go b/cmd/spiffe-helper/config/config.go index c8b1c34..2c190b4 100644 --- a/cmd/spiffe-helper/config/config.go +++ b/cmd/spiffe-helper/config/config.go @@ -163,6 +163,10 @@ func (c *Config) ValidateConfig(log logrus.FieldLogger) error { } } + if (c.Cmd != "" || c.PIDFileName != "") && c.RenewSignal == "" { + return errors.New("Must specify renew_signal when using cmd or pid_file_name") + } + x509Enabled, err := validateX509Config(c) if err != nil { return err @@ -206,10 +210,10 @@ func NewSidecarConfig(config *Config, log logrus.FieldLogger) *sidecar.Config { CmdArgs: config.CmdArgs, PIDFileName: config.PIDFileName, CertDir: config.CertDir, - CertFileMode: fs.FileMode(config.CertFileMode), //nolint:gosec,G115 - KeyFileMode: fs.FileMode(config.KeyFileMode), //nolint:gosec,G115 - JWTBundleFileMode: fs.FileMode(config.JWTBundleFileMode), //nolint:gosec,G115 - JWTSVIDFileMode: fs.FileMode(config.JWTSVIDFileMode), //nolint:gosec,G115 + CertFileMode: fs.FileMode(config.CertFileMode), //nolint:gosec + KeyFileMode: fs.FileMode(config.KeyFileMode), //nolint:gosec + JWTBundleFileMode: fs.FileMode(config.JWTBundleFileMode), //nolint:gosec + JWTSVIDFileMode: fs.FileMode(config.JWTSVIDFileMode), //nolint:gosec IncludeFederatedDomains: config.IncludeFederatedDomains, JWTBundleFilename: config.JWTBundleFilename, Log: log, diff --git a/pkg/sidecar/sidecar.go b/pkg/sidecar/sidecar.go index 8e19413..e88c50f 100644 --- a/pkg/sidecar/sidecar.go +++ b/pkg/sidecar/sidecar.go @@ -172,8 +172,23 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { } s.config.Log.Info("X.509 certificates updated") - if err := s.signalProcess(); err != nil { - s.config.Log.WithError(err).Error("Unable to signal process") + if s.config.PIDFileName != "" { + if err := s.signalPID(); err != nil { + s.config.Log.WithError(err).Error("Unable to signal PID file") + } + } + + if s.config.Cmd != "" { + if err := s.signalProcess(); err != nil { + s.config.Log.WithError(err).Error("Unable to signal process") + } + } + + // TODO: is ReloadExternalProcess still used? + if s.config.ReloadExternalProcess != nil { + if err := s.config.ReloadExternalProcess(); err != nil { + s.config.Log.WithError(err).Error("Unable to reload external process") + } } select { @@ -182,57 +197,45 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { } } -// signalProcess sends the configured Renew signal to the process running the proxy -// to reload itself so that the proxy uses the new SVID -func (s *Sidecar) signalProcess() (err error) { - if s.config.PIDFileName != "" { - byts, err := os.ReadFile(s.config.PIDFileName) - if err != nil { - return fmt.Errorf("failed to read pid file: %s\n%w", s.config.PIDFileName, err) - } - pid, err := strconv.Atoi(string(bytes.TrimSpace(byts))) +// signalPID sends the renew signal to the PID file +func (s *Sidecar) signalPID() error { + fileBytes, err := os.ReadFile(s.config.PIDFileName) + if err != nil { + return fmt.Errorf("failed to read pid file \"%s\": %w", s.config.PIDFileName, err) + } + pid, err := strconv.Atoi(string(bytes.TrimSpace(fileBytes))) + if err != nil { + return fmt.Errorf("failed to parse pid file \"%s\": %w", s.config.PIDFileName, err) + } + pidProcess, err := os.FindProcess(pid) + if err != nil { + return fmt.Errorf("failed to find process id %d: %w", pid, err) + } + + return SignalProcess(pidProcess, s.config.RenewSignal) +} + +// 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 { + cmdArgs, err := getCmdArgs(s.config.CmdArgs) if err != nil { - return fmt.Errorf("failed to parse pid file: %s\n%w", s.config.PIDFileName, err) + return fmt.Errorf("error parsing cmd arguments: %w", err) } - s.process, err = os.FindProcess(pid) - if err != nil { - return fmt.Errorf("failed to find process: %d\n%w", pid, err) + + cmd := exec.Command(s.config.Cmd, cmdArgs...) // #nosec + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + if err := cmd.Start(); err != nil { + return fmt.Errorf("error executing process \"%v\": %w", s.config.Cmd, err) } - if err := s.SignalProcess(); err != nil { + s.process = cmd.Process + go s.checkProcessExit() + } else { + if err := SignalProcess(s.process, s.config.RenewSignal); err != nil { return err } } - // TODO: is ReloadExternalProcess still used? - switch s.config.ReloadExternalProcess { - case nil: - if s.config.Cmd != "" { - if atomic.LoadInt32(&s.processRunning) == 0 { - cmdArgs, err := getCmdArgs(s.config.CmdArgs) - if err != nil { - return fmt.Errorf("error parsing cmd arguments: %w", err) - } - - cmd := exec.Command(s.config.Cmd, cmdArgs...) // #nosec - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - err = cmd.Start() - if err != nil { - return fmt.Errorf("error executing process: %v\n%w", s.config.Cmd, err) - } - s.process = cmd.Process - go s.checkProcessExit() - } else { - if err := s.SignalProcess(); err != nil { - return err - } - } - } - - default: - if err = s.config.ReloadExternalProcess(); err != nil { - return fmt.Errorf("error reloading external process: %w", err) - } - } return nil } diff --git a/pkg/sidecar/util_posix.go b/pkg/sidecar/util_posix.go index 5e754dc..6d9e44d 100644 --- a/pkg/sidecar/util_posix.go +++ b/pkg/sidecar/util_posix.go @@ -5,6 +5,7 @@ package sidecar import ( "fmt" + "os" "github.com/spiffe/go-spiffe/v2/workloadapi" "golang.org/x/sys/unix" @@ -14,20 +15,14 @@ func (s *Sidecar) getWorkloadAPIAddress() workloadapi.ClientOption { return workloadapi.WithAddr("unix://" + s.config.AgentAddress) } -func (s *Sidecar) SignalProcess() error { - // Signal to reload certs - if s.config.RenewSignal == "" { - // no signal provided - return nil - } - sig := unix.SignalNum(s.config.RenewSignal) +func SignalProcess(process *os.Process, renewSignal string) error { + sig := unix.SignalNum(renewSignal) if sig == 0 { - return fmt.Errorf("error getting signal: %v", s.config.RenewSignal) + return fmt.Errorf("error getting signal: %v", renewSignal) } - err := s.process.Signal(sig) - if err != nil { - return fmt.Errorf("error signaling process with signal: %v\n%w", sig, err) + if err := process.Signal(sig); err != nil { + return fmt.Errorf("error signaling process with signal num %v: %w", sig, err) } return nil diff --git a/pkg/sidecar/util_windows.go b/pkg/sidecar/util_windows.go index 15bf6ba..878db7f 100644 --- a/pkg/sidecar/util_windows.go +++ b/pkg/sidecar/util_windows.go @@ -13,7 +13,7 @@ func (s *Sidecar) getWorkloadAPIAddress() workloadapi.ClientOption { return workloadapi.WithNamedPipeName(s.config.AgentAddress) } -func (s *Sidecar) SignalProcess() error { +func SignalProcess(_ *os.Process, _ string) error { // Signal to reload certs // TODO: it is not possible to get signal by name on windows, // we must provide int here From 26aa9090fb9c4745d3a8485814d1914a24178a80 Mon Sep 17 00:00:00 2001 From: Faisal Memon Date: Tue, 15 Oct 2024 17:39:58 -0700 Subject: [PATCH 6/7] clean up --- pkg/sidecar/sidecar.go | 49 +++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/pkg/sidecar/sidecar.go b/pkg/sidecar/sidecar.go index e88c50f..5fffd73 100644 --- a/pkg/sidecar/sidecar.go +++ b/pkg/sidecar/sidecar.go @@ -143,6 +143,7 @@ func (s *Sidecar) CertReadyChan() <-chan struct{} { return s.certReadyChan } +// setupClients create the necessary workloadapi clients func (s *Sidecar) setupClients(ctx context.Context) error { if s.x509Enabled() || s.jwtBundleEnabled() { client, err := workloadapi.New(ctx, s.getWorkloadAPIAddress()) @@ -172,18 +173,18 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { } s.config.Log.Info("X.509 certificates updated") - if s.config.PIDFileName != "" { - if err := s.signalPID(); err != nil { - s.config.Log.WithError(err).Error("Unable to signal PID file") - } - } - if s.config.Cmd != "" { if err := s.signalProcess(); err != nil { s.config.Log.WithError(err).Error("Unable to signal process") } } + if s.config.PIDFileName != "" { + if err := s.signalPID(); err != nil { + s.config.Log.WithError(err).Error("Unable to signal PID file") + } + } + // TODO: is ReloadExternalProcess still used? if s.config.ReloadExternalProcess != nil { if err := s.config.ReloadExternalProcess(); err != nil { @@ -197,24 +198,6 @@ func (s *Sidecar) updateCertificates(svidResponse *workloadapi.X509Context) { } } -// signalPID sends the renew signal to the PID file -func (s *Sidecar) signalPID() error { - fileBytes, err := os.ReadFile(s.config.PIDFileName) - if err != nil { - return fmt.Errorf("failed to read pid file \"%s\": %w", s.config.PIDFileName, err) - } - pid, err := strconv.Atoi(string(bytes.TrimSpace(fileBytes))) - if err != nil { - return fmt.Errorf("failed to parse pid file \"%s\": %w", s.config.PIDFileName, err) - } - pidProcess, err := os.FindProcess(pid) - if err != nil { - return fmt.Errorf("failed to find process id %d: %w", pid, err) - } - - return SignalProcess(pidProcess, s.config.RenewSignal) -} - // 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 { @@ -240,6 +223,24 @@ func (s *Sidecar) signalProcess() error { return nil } +// signalPID sends the renew signal to the PID file +func (s *Sidecar) signalPID() error { + fileBytes, err := os.ReadFile(s.config.PIDFileName) + if err != nil { + return fmt.Errorf("failed to read pid file \"%s\": %w", s.config.PIDFileName, err) + } + pid, err := strconv.Atoi(string(bytes.TrimSpace(fileBytes))) + if err != nil { + return fmt.Errorf("failed to parse pid file \"%s\": %w", s.config.PIDFileName, err) + } + pidProcess, err := os.FindProcess(pid) + if err != nil { + return fmt.Errorf("failed to find process id %d: %w", pid, err) + } + + return SignalProcess(pidProcess, s.config.RenewSignal) +} + func (s *Sidecar) checkProcessExit() { atomic.StoreInt32(&s.processRunning, 1) _, err := s.process.Wait() From 30a062f3175c3cb3a6e4c46c16522b57b8010aac Mon Sep 17 00:00:00 2001 From: Faisal Memon Date: Tue, 15 Oct 2024 17:46:35 -0700 Subject: [PATCH 7/7] Spacing Signed-off-by: Faisal Memon --- pkg/sidecar/sidecar.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/sidecar/sidecar.go b/pkg/sidecar/sidecar.go index 5fffd73..47995c2 100644 --- a/pkg/sidecar/sidecar.go +++ b/pkg/sidecar/sidecar.go @@ -229,10 +229,12 @@ func (s *Sidecar) signalPID() error { if err != nil { return fmt.Errorf("failed to read pid file \"%s\": %w", s.config.PIDFileName, err) } + pid, err := strconv.Atoi(string(bytes.TrimSpace(fileBytes))) if err != nil { return fmt.Errorf("failed to parse pid file \"%s\": %w", s.config.PIDFileName, err) } + pidProcess, err := os.FindProcess(pid) if err != nil { return fmt.Errorf("failed to find process id %d: %w", pid, err)