-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 sys/version-history endpoint and associated command #13766
Conversation
32f6c6c
to
277ebe4
Compare
|
||
resp, err := client.Logical().List("sys/version-history") | ||
if err != nil { | ||
c.UI.Error(fmt.Sprintf("Error reading version history: %s", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also mention that it was only added in 1.10 here? We could also use the seal-status endpoint to detect the server version and not bother with the version-history call if the server is <1.10, but I'm not sure I want to set that precedent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that I like falling back to the seal-status endpoint either. I think mentioning that the version-history endpoint was added in 1.10 as well makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add this warning directly to the error handling logic but instead to the existing warning at the top of the CLI output that mentions the earliest version that has been tracked.
See this commit 7417fbb. What do you think?
http/sys_version_history_test.go
Outdated
// to the endpoint must be authenticated. Without synthetically altering the | ||
// underlying core/versions storage entries, a single version entry should | ||
// exist. | ||
func TestSysVersionHistory_List(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we put only tests in the http
package when they're exercising code that lives in that package, i.e. "non-logical" endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to follow the example of testing other sys endpoints:
❯ find http -type f -name sys*_test.go
http/sys_rekey_test.go
http/sys_init_test.go
http/sys_version_history_test.go
http/sys_rotate_test.go
http/sys_generate_root_test.go
http/sys_in_flight_requests_test.go
http/sys_lease_test.go
http/sys_policy_test.go
http/sys_monitor_test.go
http/sys_auth_test.go
http/sys_mounts_test.go
http/sys_config_cors_test.go
http/sys_hostinfo_test.go
http/sys_health_test.go
http/sys_metrics_test.go
http/sys_mount_test.go
http/sys_internal_test.go
http/sys_seal_test.go
http/sys_audit_test.go
http/sys_config_state_test.go
http/sys_leader_test.go
http/sys_wrapping_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these are indeed non-logical endpoints, e.g. health, metrics, pprof, monitor. Most of the rest are just ancient tests that we probably wouldn't write this way nowadays. If you look for tests starting with "TestSystemBackend" you'll find a ton of other examples elsewhere that are more conventional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great context, thank you! I'll take a look and modify accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments but looks good!
vault/version_store.go
Outdated
// timestamps were initially stored in local time. UTC should be used. Existing | ||
// entries can be overwritten via the force flag. A bool will be returned | ||
// denoting whether the entry was updated | ||
func (c *Core) storeVersionTimestamp(ctx context.Context, version string, currentTime time.Time, force bool) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might you want to apply .UTC()
to currentTime
here, instead of having all calling functions require it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that's a great point. I'll keep the calls to UTC()
in the existing calling code as it doesn't hurt and provides a the example that time.Now().UTC()
is preferred over time.Now()
.
Co-authored-by: Nick Cabatoff <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text LGTM
Tracking version history was added in Vault 1.9.0. Version entries are stored in entries keyed by
core/versions/<version-string>
(e.g./core/versions/1.9.0
). The entries themselves contain the installation timestamp of the version. Originally, the timestamps stored were created using local time (i.e.time.Now()
). UTC is preferred and thus the logic has been modified to store new entries using UTC (i.e.time.Now().UTC()
). Consistency is achieved by adding self-heal logic upon the initial read to cache existing entries inCore.versionTimestamps
. If an existing timestamp'stime.Location
is nottime.UTC
then it is modified and rewritten.The
Core.storeVersionTimestamp
method originally checked for the existence of an entry for the providedversion
. If one existed, a write to storage would not occur. The has been modified to take aforce
argument in order to facilitate the self-heal process.Note: all of the
core/versions
logic used to reside incore_util_common.go
. This PR has moved it toversion_store.go
.Users currently only have the ability to determine the current version of the running Vault. This PR introduces a new authenticated
LIST
endpoint,sys/version-history
, that will return the version history of a Vault cluster. Thekeys
array will contain the versions in chronological order.key_info
will include entries indexed by the version strings found inkeys
with additional information including theprevious_version
andtimestamp_installed
.Example response:
An associated
vault version-history
CLI command has been added. By default, it will print table output as follows. The raw JSON response can be printed using-format=json
.