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

Warnings about ignored settings when daemon_mode=false #254

Merged
merged 6 commits into from
Feb 9, 2025

Conversation

ringerc
Copy link
Contributor

@ringerc ringerc commented Feb 3, 2025

Presently spiffe-helper ignores the settings cmd, pid_file_name and renew_signal silently if daemon_mode=false.

This could lead to mis-configuration where the helper is not doing what the user expects. It also makes it more difficult to give these options a meaning in non-daemon (one-shot) mode in future, if existing user configurations might contain invalid uses of them that presently have no effect.

Raise an error if pid_file_name is configured with daemon_mode=false. This option is new and isn't documented yet so the risk of people having configurations that set it and daemon_mode=false is minimal.

For cmd and renew_signal, log a warning if daemon_mode=false instead. There could be existing mis-configurations for these in the wild that we don't want to break by making working-but-incorrect configuration fail with a fatal validation error instead.

The docs PR #243 adds documentation for the various run modes, including that cmd, pid_file_name and renew_signal are nonsensical when daemon_mode=false.

Test

Unit test cover added for

  • valid pid_file_name nonempty + renew_signal nonempty configuration in daemon_mode=true (default) to show this wasn't broken by these changes
  • invalid config pid_file_name nonempty and renew_signal empty raises error
  • valid pid_file_name empty and renew_signal nonempty is not an error because it could be used for cmd
  • invalid config pid_file_name nonempty with !daemon_mode

A warning is emitted when cmd and/or renew_signal are set in !daemon_mode but execution continues (until it can't reach the agent since I'm not running one):

➜  spiffe-helper git:(warn-cmd-in-oneshot-mode) ✗ cat > helper_oneshot.conf <<'__END__'
daemon_mode=false
agent_address = "/tmp/spire-agent/public/api.sock"
svid_file_name = "svid.pem"
svid_key_file_name = "svid_key.pem"
svid_bundle_file_name = "svid_bundle.pem"
cert_dir = "certs"

cmd = "expect_warning"
renew_signal="SIGTERM"       
__END__

➜  spiffe-helper git:(warn-cmd-in-oneshot-mode) ✗ ./spiffe-helper -config helper_oneshot.conf
INFO[0000] Using configuration file: "helper_oneshot.conf"  system=spiffe-helper
WARN[0000] cmd is set but daemon_mode is false. cmd will be ignored. This may become an error in future.  system=spiffe-helper
WARN[0000] renew_signal is set but daemon_mode is false. renew_signal will be ignored. This may become an error in future.  system=spiffe-helper
INFO[0000] Daemon mode disabled                          system=spiffe-helper
ERRO[0000] Error fetching x509 certificates              error="rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial unix /tmp/spire-agent/public/api.sock: connect: no such file or directory\"" system=spiffe-helper

Setting pid_file_name with !daemon_mode becomes a fatal error as shown in the updated unit test cover, and

➜  spiffe-helper git:(warn-cmd-in-oneshot-mode) ✗ cat > helper_oneshot.conf <<'__END__'      
daemon_mode=false
agent_address = "/tmp/spire-agent/public/api.sock"
svid_file_name = "svid.pem"
svid_key_file_name = "svid_key.pem"
svid_bundle_file_name = "svid_bundle.pem"
cert_dir = "certs"

pid_file_name="pidfile"
renew_signal="SIGTERM"
__END__

➜  spiffe-helper git:(warn-cmd-in-oneshot-mode) ✗ ./spiffe-helper -config helper_oneshot.conf
INFO[0000] Using configuration file: "helper_oneshot.conf"  system=spiffe-helper
WARN[0000] renew_signal is set but daemon_mode is false. renew_signal will be ignored. This may become an error in future.  system=spiffe-helper
ERRO[0000] invalid configuration                         error="pid_file_name is set but daemon_mode is false. pid_file_name is only supported in daemon_mode" system=spiffe-helper

I did not add unit test cover for the new log messages, as the existing unit tests don't capture logs and I wasn't sure how best to do that. But the error has test cover.

@ringerc ringerc force-pushed the warn-cmd-in-oneshot-mode branch from e813483 to 01da7bd Compare February 3, 2025 21:31
Presently spiffe-helper ignores the settings cmd, pid_file_name
and renew_signal silently if daemon_mode=false.

This could lead to misconfiguration where the helper is not doing what
the user expects. It also makes it more difficult to give these options
a meaning in non-daemon (one-shot) mode in future, if existing user
configurations might contain invalid uses of them that presently have no
effect.

Raise an error if pid_file_name is configured with daemon_mode=false.
This option is new and isn't documented yet so the risk of people having
configurations that set it and daemon_mode=false is minimal.

For cmd and renew_signal, log a warning if daemon_mode=false instead.
There could be existing misconfigurations for these in the wild that we
don't want to break by making working-but-incorrect configuration fail
with a fatal validation error instead.
@ringerc ringerc force-pushed the warn-cmd-in-oneshot-mode branch from 01da7bd to 4e96a59 Compare February 3, 2025 21:35
@faisal-memon
Copy link
Collaborator

Thanks @ringerc for catching these.

@faisal-memon faisal-memon added this to the 0.10.0 milestone Feb 4, 2025
@faisal-memon faisal-memon merged commit f68cf0d into spiffe:main Feb 9, 2025
12 of 13 checks passed
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.

2 participants