-
Notifications
You must be signed in to change notification settings - Fork 371
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
Stricter validation for integer and float configuration #3332
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.0 #3332 +/- ##
=======================================
Coverage 98.10% 98.10%
=======================================
Files 1252 1252
Lines 72342 72348 +6
Branches 3387 3387
=======================================
+ Hits 70974 70980 +6
Misses 1368 1368 ☔ View full report in Codecov by Sentry. |
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'm a bit short on time today so I didn't look at this very thoroughly but here's a quick pass :)
rescue ArgumentError => e | ||
# Try also parsing as a whole floating point numbers | ||
begin | ||
f = Float(value) | ||
|
||
# Check for whole numbers | ||
raise ArgumentError, "#{value} is not a whole number" unless f.truncate == f |
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.
To be honest... This seems like a bit of a surprising semantics. If I'm in a rush and see something on my config that I want to update:
DD_PROFILING_HOW_MUCH_IT_ROCKS=100.0
and then I go, oh, I want to make it rock a bit less
DD_PROFILING_HOW_MUCH_IT_ROCKS=95.5
and it blows up -- this seems weird. That .0 that someone on my team left misled me into thinking this value was supposed to be a float, and then it turns out it isn't and it doesn't work.
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.
What do you think about allowing whole floats as integer @TonyCTHsu @ekump? (e.g. 100.0
is allowed for :int
configuration options). Or should be strict and say that this is not allowed (e.g. 100.0
would be rejected, and the user should replace it with an explicit 100
)
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.
@ivoanjo, how about a warning on whole floats? I think that might be a good compromise.
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.
how about a warning on whole floats?
I would still kinda prefer not to accept 'em, but I can live with that as a compromise ;)
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.
We had discussed moving this to a warning, which I think is better than the current raise. If we do keep the raise, I would suggest making it a bit clear what needs to be fixed in the current error -- e.g. "Setting expects only integer settings, you've provided #{value}"?
"The option #{@definition.name} is using an unsupported type option for env coercion `#{@definition.type}`" | ||
end | ||
end | ||
|
||
def coerce_env_var_int(value) | ||
# Strict integer parsing | ||
Integer(value) |
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 learned (depressingly) the other day that Integer(value)
actually supports hex and octal literals => e.g. Integer("010") => 8
and Integer("0x10") => 16
.
TL;DR the correct form is Integer(value, 10)
to specifically ask it to parse only as base 10. Now I'll go back to wondering why this isn't the default >_>
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 think if only 0x10
(and base 10) were allowed, we wouldn't bat an eye, but octals ruin it for everyone.
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.
Fixed 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.
Yeah, it's hard to accidentally have hex, but so easy to accidentally have octal >_>
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.
Left a few final notes :)
rescue ArgumentError => e | ||
# Try also parsing as a whole floating point numbers | ||
begin | ||
f = Float(value) | ||
|
||
# Check for whole numbers | ||
raise ArgumentError, "#{value} is not a whole number" unless f.truncate == f |
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.
We had discussed moving this to a warning, which I think is better than the current raise. If we do keep the raise, I would suggest making it a bit clear what needs to be fixed in the current error -- e.g. "Setting expects only integer settings, you've provided #{value}"?
b3e8f7c
to
90a7f10
Compare
@ivoanjo I talked to @TonyCTHsu and I was convinced to have stricter Integer validation, which aligns with your original suggestion. |
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 think we should still allow users to provide Integer
to float settings when doing it via the configure block, but otherwise this looks good to me 👍
when :float | ||
value.is_a?(Float) || value.is_a?(Rational) |
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 think allowing users to provide integer values for floats is a nice UX touch -- e.g. if you're providing a percentage, often you may want to say 50 and not 50.0 or something like that.
So I don't think we should lose that.
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 mention the removal of ENV['DD_EXPERIMENTAL_SKIP_CONFIGURATION_VALIDATION']
in the upgrade guide?
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.
👍 LGTM
2.0 Upgrade Guide notes
Configuration options of types ':int' and ':float' now have stricter type validation:
This applies to both environment variable and programmatic configuration.
Here's the list of all affected options:
[Insert all :int and :float options at release time!!!]
The environment variable
DD_EXPERIMENTAL_SKIP_CONFIGURATION_VALIDATION
has been removed.Motivation:
It was possible to configure options with empty values (e.g.
DD_TRACE_SAMPLE_RATE=
) and end up with a sampling rate of0.0
, because of the logicvalue.to_f
. The same applies to integer options (value.to_i
).Non-numeric values would also be converted to zero.
This means users do not know what their effective configuration will be, and are not given feedback on how to address configuration issues.
How to test the change?
There are unit tests for all the changes.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!