Fix webhook error when metrics service address uses env var expansion #3531
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
This change looks good to me (minus the gosec warning), but it doesn't fully address #3513. What we should do in addition is emit a warning when failing to parse the port instead of returning an error. |
|
@swiatekm will add the warning and fix the gosec issue in a few mins. Thanks! |
Now, when there would be an error it gets logged and the default values are returned. With this refactor the method encapsulates all defaulting logic that was slightly spread around different places.
|
@swiatekm @yuriolisa: I just updated the PR with the changes you suggested:
On top of this, I could modify the code to support IPv6 and added a bunch of tests to ensure we don't have regressions. Thank you for your review. 🙇 |
Although I see the IPv6 support as valid, I suggest putting it in another PR to avoid any misleading debugging. |
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: | | ||
| This should allow the metrics service address to have the host portion expanded from an environment variable, |
There was a problem hiding this comment.
Please inform that you enabled the IPv6.
There was a problem hiding this comment.
@yuriolisa I didn’t necessarily enable ipv6. I only refactored that ‘Service.MetricsEndpoint’ method in a way that it also works with ipv6 addresses. I do not know if the project has everything else that is needed for full ipv6 support. Do you know about this?
There was a problem hiding this comment.
I don't think there's a need to specifically call out IPv6. We only care about the port to begin with.
swiatekm
added
the
discuss-at-sig
There was a problem hiding this comment.
Your changes look good to me overall @douglascamata, and thank you for being patient with the review process! I'm blocking this from being merged for now, because I'd like to get wider consensus on how we should deal with this issue. I'll do a proper review once we have clarity on that.
|
@jaronoff97: I took care of your feedback (plus another small pending change) in a batch on a6744fd |
Description:
Use a naive port parsing in the operator's webhook to ensure it doesn't error out when the metrics service address uses environment variable expansion for the host part.
Link to tracking Issue(s):
spec.config.service.telemetry.metrics.addressdoesn't work #3513Testing:
Added a unit test covering the IPv4 case.
Documentation:
None.