config: health checker timeout and interval should be greater than 0#3734
config: health checker timeout and interval should be greater than 0#3734mattklein123 merged 5 commits intoenvoyproxy:masterfrom dio:greater-than-0
Conversation
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thank you! Any chance of adding a test for this stuff?
| // health check attempt will be considered a failure. | ||
| google.protobuf.Duration timeout = 1 [(validate.rules).duration.required = true]; | ||
| google.protobuf.Duration timeout = 1 [ | ||
| (validate.rules).duration = {required: true, gt: {}}, |
There was a problem hiding this comment.
nit: IMO saying greater than 0 is a bit more clear here. Same below.
There was a problem hiding this comment.
I thought the same, but then I found that the others use gt: {} only (rather than gt: {seconds: 0}). E.g.
There was a problem hiding this comment.
mmm I still think that having the explicit value is more clear. I would rather add it here and update the other protos in a separate PR for explicitness.
There was a problem hiding this comment.
@mattklein123 @junr03 will do. I also think that we should automatically render that requirement into doc, e.g. REQUIRED, GREATER THAN 0 SEC. But I believe that should be down in a different PR. Related: #3731
|
@mattklein123 do you think I can just add config tests inside the |
|
@dio I would just add tests wherever is easiest. (Obviously just make sure the validator actually runs.) Thank you! |
|
@junr03 please take a pass on this once tests are added. Thank you! |
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
|
Test is added. PTAL. Thanks! |
junr03
left a comment
junr03
left a comment
There was a problem hiding this comment.
lgtm mod matt's nit, which I actually think we should do.
| // health check attempt will be considered a failure. | ||
| google.protobuf.Duration timeout = 1 [(validate.rules).duration.required = true]; | ||
| google.protobuf.Duration timeout = 1 [ | ||
| (validate.rules).duration = {required: true, gt: {}}, |
There was a problem hiding this comment.
mmm I still think that having the explicit value is more clear. I would rather add it here and update the other protos in a separate PR for explicitness.
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
3 participants
Description:
This patch adds value validation for interval and timeout fields to accept values greater than 0.
Risk Level: Low
Testing: Unit, manual by setting timeout or interval to 0s
Docs Changes: N/A
Release Notes: N/A
Fixes #3729
Signed-off-by: Dhi Aurrahman dio@rockybars.com