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

PKI Health Check Command #17750

Merged
merged 13 commits into from
Nov 16, 2022
Merged

PKI Health Check Command #17750

merged 13 commits into from
Nov 16, 2022

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Oct 31, 2022

This pull requests adds initial support for the vault pki health-check command, with two health checks. More will be added in future PRs, but I wanted to get this opened for review before we get too far and add too many tests.


TODO:

  • Finish CRL HC implementation
  • Add tests for initial health checks
  • Add a little code reuse between PKI tests
  • Finish return code parsing

@cipherboy cipherboy added this to the 1.13.0-rc1 milestone Oct 31, 2022
@cipherboy cipherboy force-pushed the cipherboy-pki-health-check branch 2 times, most recently from 34536e8 to 8c36298 Compare November 1, 2022 14:34
@cipherboy cipherboy force-pushed the cipherboy-pki-health-check branch 2 times, most recently from f469c59 to 3949a6b Compare November 9, 2022 14:14
@cipherboy cipherboy marked this pull request as ready for review November 9, 2022 14:16
@cipherboy cipherboy force-pushed the cipherboy-pki-health-check branch from 5128818 to 3c89ff3 Compare November 9, 2022 14:46
@cipherboy cipherboy requested review from kitography, sgmiller and a team November 9, 2022 20:59
@hashicorp hashicorp deleted a comment from cipherboy Nov 15, 2022
Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small nit with the %w, but everything looks good. I'm assuming doc updates will come with a later PR?

command/healthcheck/common.go Outdated Show resolved Hide resolved
command/healthcheck/pki_crl_validity_period.go Outdated Show resolved Hide resolved
This command will be used to generate health check results for the PKI
engine.

Signed-off-by: Alexander Scheel <[email protected]>
These utilities will collect helpers not specific to PKI health checks,
such as formatting longer durations more legibly.

Signed-off-by: Alexander Scheel <[email protected]>
Many health checks will need issuer and/or CRL information in order to
execute. We've centrally located these helpers to avoid particular
health checks from needing to reimplement them each time.

Signed-off-by: Alexander Scheel <[email protected]>
This shifts the last of the logic difference between Read(...) and
ReadRaw(...) to a new helper, allowing ReadRaw(...) requests to be
parsed into the same response structure afterwards as Read(...); this
allows API callers to fetch the raw secret and inspect the raw response
object in case something went wrong (error code &c) -- and when the
request succeeds, they can still get the api.Secret out.

This will be used with the PKI health check functionality, making both
LIST and READ operations use ReadRaw, and optionally parsing the secret
afterwards.

Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
When reading raw objects, don't manually call the context cancellation:
this causes timeouts and/or EOF errors when attempting to read or parse
the response body. See message in client.RawRequestWithContext(...) for
more information.

This was causing the test suite to randomly fail, due to the context
cancelling. The test suite's client usually had a default timeout,
whereas the CLI didn't, and thus didn't exhibit the same issue.

Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy force-pushed the cipherboy-pki-health-check branch from 3c89ff3 to 57de19d Compare November 15, 2022 14:47
@cipherboy
Copy link
Contributor Author

Thanks! Pushed an update to those comments and will auto-merge.

@cipherboy cipherboy enabled auto-merge (squash) November 15, 2022 14:47
Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy force-pushed the cipherboy-pki-health-check branch from 57de19d to fbfd065 Compare November 16, 2022 14:08
@cipherboy cipherboy merged commit 02d265b into main Nov 16, 2022
@cipherboy cipherboy deleted the cipherboy-pki-health-check branch December 1, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants