Document pid_file_name and explain some other flags

[ringerc]

Update the README to:

• Explain the various modes in which spiffe-helper can be used and why each one might be useful
• Document the previously undocumented pid_file_name config option
• Add explanation of how cmd and cmd_args is interpreted, including:
  • double quoting to protect spaces
  • escape double quotes by pairing them not with backslashes
  • single quotes are not special and don't protect spaces or double quotes
  • shell metacharacters are not special
• Warn that stdin is not forwarded, and the exit code of the child process is not forwarded on exit (in fact, spiffe-helper does not exit at all in daemon mode, or run a command at all in non-daemon mode), so spiffe-helper cannot be used as a pass-through wrapper for another command
• Document that an exited cmd is re-launched only when the cert is rotated
• Explain how daemon_mode=false works and that it ignores cmd, pid_file_name and renew_signal

[faisal-memon]

@ringerc thanks for looking in to the docs.

[ringerc]

@faisal-memon @keeganwitt Rebased and suggested changes made. I also corrected a mistake where I incorrectly suggested that the cmd would be restarted if it was running; I meant if it was not running.

[ringerc]

@faisal-memon @keeganwitt I've revised this a bit more based on what I've learned working on the codebase in the last couple of days. I moved it to subheads rather than bullet points and tried to structure it into groups based on the different ways of using spiffe-helper.

You can view the rendered version here or the marked-up diff here.

[faisal-memon]

Is there anyway to support escaping the quotes?

[keeganwitt]

I'm not following what you mean by escape here. You're talking about single quotes? Or double quotes?

[ringerc]

The problem is that spiffe-helper uses a csv parser with the delimiter set to " " (space) to process its command-line args and split them to an argument vector.

This is the test case I wrote that this README text was based on: https://github.com/ringerc/spiffe-helper-PRs/blob/31a134b2438ea865739673779c23c99beb410815/pkg/sidecar/sidecar_test.go#L116-L131 . The arguments there would be correct as a shell argument string, but is not handled correctly by spiffe-helper because spiffe-helper only supports double-quoted arguments.

To confirm my understanding of its parser behaviour I've banged out some test cases here #256 - see in particular #256 (review) for this case. This also showed that double quotes are escaped by pairing them, not backslash-escaping them.

Using a csv parser produces surprising and confusing results, as the semantics of csv are not the same as the semantics of a command line argument string, particularly when nesting double quotes within single quotes (which in the shell's parser, protects the double quotes) and when escaping double quotes.

This could result in surprising and even dangerous behaviour, hence the documentation for it. I don't recommend trying to change or fix the parser as getting it right is too hard, and "right" depends on the OS anyway. It's much better to use an argument vector (array) instead and deprecate the string-based cmd_args.

I implemented an argument-vector replacement in my working tree but I haven't had time to extract it into a separate PR, and I'd rather wait until some of the current ones are merged so it doesn't get too hard to keep track of things and rebase.

So I documented the current behaviour in the meantime. Would it be clearer if I split this into whole hcl lines?

[ringerc]

Revised the explanation to hopefully make it clearer at the expense of making it longer: a558254

Please resolve if satisfied

[faisal-memon]

Thanks @ringerc this looks great. Just some minor comments and its ready to go.