Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a test suite for sidecar cmd and signals #258

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ringerc
Copy link
Contributor

@ringerc ringerc commented Feb 5, 2025

Add test cover for:

  • long-running and short-lived cmd and cmd_args invocations
  • signalling via pid_file_name and renew_signal
  • windows cmd and cmd_args execution

If rebased onto the 'fix-windows' branch for windows test fixes, these pass for windows test runs too.

To make these tests possible the following changes are made to the sidecar code:

  • Sidecar.certReadyChan yields *workloadapi.X509Context not struct{} so the context can be observed. This could also be useful for code extending spiffe-helper.
  • A new Sidecar.cmdExitChan channel reports whenever a process managed by spiffe-helper exits, and the exit state. This allows tests to wait for process exits and see the exit code, etc.
  • A new Sidecar.pidFileSignalledChan channel reports whenever signalling the file in pid_file_name is attempted, the pid signalled, and the outcome. This allows signal based tests to see what spiffe-helper tried to signal.

Channels with values are used to ensure that there's no race between what might be present in the Sidecar struct state when the test observes it and what was there when the event of interest occurred.

It might be handy to have a channel for observing when the managed process is started too, but the tests just busy-wait polling Sidecar.processRunning to detect this for now.

I considered combining these channels into some kind of unified observer channel that emits a union struct depending on the event type, but it seemed pointless to do this when we can just select{} on multiple channels of interest and keep each channel simple.

Extracted from #252, with linter complaints addressed, and the test for pid signalling adapted so it asserts that there are no retries.

Add test cover for:

* long-running and short-lived cmd and cmd_args invocations
* signalling via pid_file_name and renew_signal
* windows cmd and cmd_args execution

If rebased onto the 'fix-windows' branch for windows test fixes, these
pass for windows test runs too.

To make these tests possible the following changes are made to the
sidecar code:

* Sidecar.certReadyChan yields *workloadapi.X509Context not struct{}
  so the context can be observed. This could also be useful for
  code extending spiffe-helper.
* A new Sidecar.cmdExitChan channel reports whenever a process
  managed by spiffe-helper exits, and the exit state. This allows
  tests to wait for process exits and see the exit code, etc.
* A new Sidecar.pidFileSignalledChan channel reports whenever
  signalling the file in pid_file_name is attempted, the pid
  signalled, and the outcome. This allows signal based tests
  to see what spiffe-helper tried to signal.

Channels with values are used to ensure that there's no race between
what might be present in the Sidecar struct state when the test observes
it and what was there when the event of interest occurred.

It might be handy to have a channel for observing when the managed
process is started too, but the tests just busy-wait polling
Sidecar.processRunning to detect this for now.

I considered combining these channels into some kind of unified observer
channel that emits a union struct depending on the event type, but it
seemed pointless to do this when we can just select{} on multiple
channels of interest and keep each channel simple.

Signed-off-by: Craig Ringer <[email protected]>
@ringerc ringerc force-pushed the test-suite-for-cmd-and-signal-file branch from e8d8c5e to 3c51386 Compare February 5, 2025 01:04
ringerc added a commit to ringerc/spiffe-helper-PRs that referenced this pull request Feb 5, 2025
… into retry-signal-pid-file

These are tracked by PRs

* spiffe#258 for test suite
* spiffe#257 for windows

Once they are merged the upcoming commit can be merged in its own PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant