Reload VaultConfig if CAFile, CertFile, KeyFile have changed

[Xopherus]

Fixes #4395, #6052

[notnoop]

Thanks for submitting the PR. Overall this is good, though need to be careful about mutations and tls files being updated in-place.

We, the nomad team, probably should consider having the internal structs contain the raw file bytes rather than file paths; this would ease reasoning around handling file modifications and detecting changes/drifts.

[notnoop]

Feels like we should use false as the fail value. Besides being the zero value, if we were to accidentally ignore the returned error, we'd fail by refreshing vault config, which is a noop if files didn't actually change.

Noticed that the TLS config has that behavior in

nomad/nomad/structs/config/tls.go

Lines 253 to 260 in 8e4a5f1

	// Set the checksum if it hasn't yet been set (this should happen when the
	// config is parsed but this provides safety in depth)
	if newConfig.Checksum == "" {
	err := newConfig.SetChecksum()
	if err != nil {
	return false, err
	}
	}

Suggested change

	return true, err
	return false, err

[notnoop]

This probably should be called by Merge() to avoid using a stale pre-merge checksum accidentally.

[notnoop]

Populating SetChecksum lazily causes missing the first update. Consider the case where the tls files are manipulated in place and SIGHUP sent to nomad. The checksums for both objects would reflect the latest and this function returns true unexpectedly.

We probably should call SetChecksum during config initialization.

[Xopherus]

@notnoop I appreciate the review. I did end up deploying a version of this to test in my stack and like you said, I found that this was incomplete. However I did leave a comment with some of my findings on #4593 (comment) and I think the scope is actually much larger than I initially expected. Certainly it's much larger of an effort that I can do by myself. That being said, I think this feature is sorely needed in order to run Nomad with Vault-backed TLS, and would be glad to help provide more information to the Nomad team to help make this happen.

If that's cool with you, then I think we can close this PR.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.