pkg/prometheus: Add validation for proberSpec url field#4483
pkg/prometheus: Add validation for proberSpec url field#4483simonpasquier merged 1 commit intoprometheus-operator:mainfrom
Conversation
c98449e to
4ddb083
Compare
c5d8915 to
49c6238
Compare
|
@fpetkovski I updated, can you review again? |
simonpasquier
left a comment
There was a problem hiding this comment.
side-note: validateProberURL() is more strict than what Prometheus currently checks but it's fine for me.
pkg/prometheus/operator.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func validateProberURL(url string) bool { |
There was a problem hiding this comment.
IMHO it would be better if validateProberURL returns an error. This way the returned message would be clearer (e.g. "invalid host" vs. "invalid port") and it would be consistent with the other validate*() functions.
There was a problem hiding this comment.
I actually wrote with error as return but found it a bit tricky when checked with different types of inputs. Like the case url passed is http://blackbox-exporter.example.com:8080 in this case when we split the string on : it will take /blackbox-exporter.example.com as hostPort[1] and if we validate and return error as invalid port it could be more confusing
pkg/prometheus/operator.go
Outdated
| if strings.Contains(url, ":") { | ||
| hostPort := strings.Split(url, ":") | ||
| return govalidator.IsHost(hostPort[0]) && govalidator.IsPort(hostPort[1]) | ||
| } | ||
| return govalidator.IsHost(url) |
There was a problem hiding this comment.
Taking into account #4481, it would be more convenient if the function returns an error (only one place where the error message is constructed).
Changing the function to return an error instead of a bool could look like:
| if strings.Contains(url, ":") { | |
| hostPort := strings.Split(url, ":") | |
| return govalidator.IsHost(hostPort[0]) && govalidator.IsPort(hostPort[1]) | |
| } | |
| return govalidator.IsHost(url) | |
| hostPort := strings.Split(url, ":") | |
| if !govalidator.IsHost(hostPort[0]) { | |
| return errors.Errorf("invalid host: %q", hostPort[0]) | |
| } | |
| if len(hostPort[1]) > 1 && govalidator.IsPort(hostPort[1]) { | |
| return errors.Errorf("invalid port: %q", hostPort[1]) | |
| } | |
| return nil |
There was a problem hiding this comment.
Thank you. I added a case for handling url scheme specified as well. Else error may be reported as invalid port for a case like
proberSpec: monitoringv1.ProberSpec{
URL: "https://blackbox-exporter.example.com",
}
There was a problem hiding this comment.
discussed offline and decided we don't need to handle each case separately so removing handling the url scheme case separately
1d142e4 to
e52c9db
Compare
Fixes prometheus-operator#4476 Signed-off-by: Jayapriya Pai <slashpai9@gmail.com>
|
@simonpasquier I updated as per your suggestion, please review :) |
Fixes #4476
Signed-off-by: Jayapriya Pai slashpai9@gmail.com
Description
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Add validation for proberSpec url field to avoid invalid url being passed
Type of change
What type of changes does your code introduce to the Prometheus operator? Put an
xin the box that apply.CHANGE(fix or feature that would cause existing functionality to not work as expected)FEATURE(non-breaking change which adds functionality)BUGFIX(non-breaking change which fixes an issue)ENHANCEMENT(non-breaking change which improves existing functionality)NONE(if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)Changelog entry
Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.