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.

Sign up for GitHub

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

Jump to bottom

pki health-check fails to read in int config values #19265

Merged
merged 2 commits into from
Feb 21, 2023
Merged

pki health-check fails to read in int config values #19265

merged 2 commits into from
Feb 21, 2023

Conversation

stevendpclark
Copy link
Contributor

Issue Description

While testing the vault pki health-check feature, providing it a configuration file with integer value of say 100 for root_issued_leaves.root_issued_leaves led to the following error.

Failed to build health check configuration: error saving merged config for root_issued_leaves: error parsing root_issued_leaves.certs_to_fetch: could not parse value from input

Fix

  • Go's default behavior when decoding numbers to an interface{} is to use a float64 type which parseutil.SafeParseIntRange does not handle. Switch to having the JSON decoder use json.Number which parseutil methods properly handle.
  • Tested configuration values as strings, integers work
  • Also tested we get proper error messages for out of bounds integers and float values

@stevendpclark
pki health-check fails to read in int config values
f816f7d 
 - Go's default behavior when decoding numbers to an interface{} is to use a float64 type which parseutil.SafeParseIntRange does not handle.
 - Switch to having the JSON decoder use json.Number which our parseutil library
  properly handles.
@stevendpclark stevendpclark added this to the 1.13.0 milestone Feb 20, 2023
victorr
victorr approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@victorr victorr left a comment

Choose a reason for hiding this comment

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

LGTM!

cipherboy
cipherboy approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@cipherboy cipherboy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I figured this out just recently too elsewhere, really need a warning about it. :D

Thank you, and nice find!

@stevendpclark stevendpclark merged commit 4ea5c58 into main Feb 21, 2023
@stevendpclark stevendpclark deleted the stevendpclark/pki-healthcheck-config branch February 21, 2023 13:52
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cipherboy cipherboy cipherboy approved these changes

@victorr victorr victorr approved these changes

@kitography kitography Awaiting requested review from kitography

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@stevendpclark @victorr @cipherboy