VAULT-20669: Add New Authenticated Endpoint for Version #23740
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
|
Build Results: |
|
CI Results: |
marcboudreau
force-pushed
the
marcboudreau/VAULT-20669/add-new-ui-version-endpoint
branch
from
870416c to
f39ddd9
Compare
marcboudreau
force-pushed
the
marcboudreau/VAULT-20669/add-new-ui-version-endpoint
branch
from
f39ddd9 to
78ff999
Compare
|
@marcboudreau Neither sys/health nor sys/seal-status are authenticated endpoints. Why does this new endpoint need to be authenticated? |
|
Is it worth adding a test that verifies this new endpoint outputs the right thing? |
@raskchanky I believe the idea is so that when the Vault platform operator has configured redaction to occur on the unauthenticated endpoints ( We recently added this in a few PRs but the docs might explain the redaction settings better. 😄 |
There was a problem hiding this comment.
Looks good to me, but @raskchanky made a good point in a previous comment that it might be reassuring to have some tests to make sure we get what we're expecting from the endpoint.
Failing that some receipts in the PR summary to show the output? 🤷🏼
| Operations: map[logical.Operation]framework.OperationHandler{ | ||
| logical.ReadOperation: &framework.PathOperation{ | ||
| Callback: b.pathInternalUIVersion, | ||
| Summary: "Backwards compatibility is not guaranteed for this API", |
There was a problem hiding this comment.
❓ I wonder why we just use the same summary message for multiple internal API endpoints, do you think it would be useful to include what this endpoint is for (even if it also says that it doesn't guarantee backwards compatibility)?
The handler function only has a single branch of execution and so I don't think adding a test for the handler provides much value, however I looked at the version package and there's plenty of untested conditional branching going on there, so I'll add tests there. My thinking is that if we are confident that version.GetVersion().VersionNumber() works correctly, then we can also be confident that the pathInternalUIVersion handler will work as well. |
|
@peteski22 Thanks for clarifying! Makes sense. |
|
My latest two commits add tests for the functions exported from the version package, this will give us the confidence that |
|
❓ I definitely understand the intent of moving it to an authenticated endpoint. Is there is a purpose to prefixing it with |
|
The version information is still available from the unauthenticated endpoints |
Co-authored-by: Sarah Chavis <[email protected]>
Co-authored-by: Sarah Chavis <[email protected]>
Co-authored-by: Sarah Chavis <[email protected]>
marcboudreau
deleted the
marcboudreau/VAULT-20669/add-new-ui-version-endpoint
branch
…)" This reverts commit 550c99a.
* VAULT-24469 use sys/seal-status instead of internal version endpoint * Update tests and mirage handlers * Revert "VAULT-20669: Add New Authenticated Endpoint for Version (#23740)" This reverts commit 550c99a. * Readded version_test.go * Reverted any old changes on versionlgo --------- Co-authored-by: divyaac <[email protected]>
This PR adds a new endpoint
sys/internal/ui/versionwhich requires a Vault Token that has the read capability granted for its path. The change also adds a new rule that grants the read capability to this path in the default policy. This endpoint is used to obtain the Vault version.This new path is being introduced to provide a guaranteed mechanism for the Vault UI to retrieve the Vault version, since the current endpoints:
sys/healthandsys/seal-statuscan have the version number redacted if the redact_version TCP listener configuration parameter is set totrue.The Vault UI will be changed to use this endpoint in a later change.