-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Allow paths without a leading slash in probes #15681
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15681 +/- ##
==========================================
+ Coverage 80.78% 80.83% +0.04%
==========================================
Files 222 222
Lines 18025 18025
==========================================
+ Hits 14561 14570 +9
+ Misses 3092 3085 -7
+ Partials 372 370 -2 ☔ View full report in Codecov by Sentry. |
I noticed this warning.
cc @dprotaso is this a false positive as the version is v4.0.2? |
/assign @dprotaso |
@dprotaso gentle ping. |
8821c8f
to
0f8e118
Compare
@@ -111,7 +112,7 @@ var transport = func() *http.Transport { | |||
}() | |||
|
|||
func getURL(config HTTPProbeConfigOptions) (*url.URL, error) { | |||
return url.Parse(string(config.Scheme) + "://" + net.JoinHostPort(config.Host, config.Port.String()) + config.Path) | |||
return url.Parse(string(config.Scheme) + "://" + net.JoinHostPort(config.Host, config.Port.String()) + "/" + strings.TrimPrefix(config.Path, "/")) |
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 should just construct a URL struct instead of building a url string and then parsing it.
eg. url.URL handles normalizing the url - see: https://go.dev/play/p/BC7UV63Drih
u1 = url.URL{
Scheme: "http",
Path: "/blah",
Host: net.JoinHostPort("host", "port"),
}
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.
oh and lets add a unit test so we don't regress in the future
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.
It is not that straightforward because you need then to validate the url itself. For example the path needs to be cleaned from the query part. The fact that you manually create a url does not mean it is valid. Here is the full url.parse logic and I don't want to copy that, that is the reason I didn't use that in the first place:
func parse(rawURL string, viaRequest bool) (*URL, error) {
var rest string
var err error
if stringContainsCTLByte(rawURL) {
return nil, errors.New("net/url: invalid control character in URL")
}
if rawURL == "" && viaRequest {
return nil, errors.New("empty url")
}
url := new(URL)
if rawURL == "*" {
url.Path = "*"
return url, nil
}
// Split off possible leading "http:", "mailto:", etc.
// Cannot contain escaped characters.
if url.Scheme, rest, err = getScheme(rawURL); err != nil {
return nil, err
}
url.Scheme = strings.ToLower(url.Scheme)
if strings.HasSuffix(rest, "?") && strings.Count(rest, "?") == 1 {
url.ForceQuery = true
rest = rest[:len(rest)-1]
} else {
rest, url.RawQuery, _ = strings.Cut(rest, "?")
}
if !strings.HasPrefix(rest, "/") {
if url.Scheme != "" {
// We consider rootless paths per RFC 3986 as opaque.
url.Opaque = rest
return url, nil
}
if viaRequest {
return nil, errors.New("invalid URI for request")
}
// Avoid confusion with malformed schemes, like cache_object:foo/bar.
// See golang.org/issue/16822.
//
// RFC 3986, §3.3:
// In addition, a URI reference (Section 4.1) may be a relative-path reference,
// in which case the first path segment cannot contain a colon (":") character.
if segment, _, _ := strings.Cut(rest, "/"); strings.Contains(segment, ":") {
// First path segment has colon. Not allowed in relative URL.
return nil, errors.New("first path segment in URL cannot contain colon")
}
}
if (url.Scheme != "" || !viaRequest && !strings.HasPrefix(rest, "///")) && strings.HasPrefix(rest, "//") {
var authority string
authority, rest = rest[2:], ""
if i := strings.Index(authority, "/"); i >= 0 {
authority, rest = authority[:i], authority[i:]
}
url.User, url.Host, err = parseAuthority(authority)
if err != nil {
return nil, err
}
} else if url.Scheme != "" && strings.HasPrefix(rest, "/") {
// OmitHost is set to true when rawURL has an empty host (authority).
// See golang.org/issue/46059.
url.OmitHost = true
}
// Set Path and, optionally, RawPath.
// RawPath is a hint of the encoding of Path. We don't want to set it if
// the default escaping of Path is equivalent, to help make sure that people
// don't rely on it in general.
if err := url.setPath(rest); err != nil {
return nil, err
}
return url, nil
}
/lgtm Can you add a follow up unit test? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, skonto The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sure. |
/cherry-pick release-1.16 |
/cherry-pick release-1.15 |
@dprotaso: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@dprotaso: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@dprotaso: #15681 failed to apply on top of branch "release-1.15":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@dprotaso: #15681 failed to apply on top of branch "release-1.16":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Fixes #15673
Proposed Changes
Release Note