You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I am using the latest version, or the latest version that corresponds to my Oxide installation.
There is no open issue that reports the same problem.
What was the expected behaviour
The expected behavior is that the Oxide API client will respect the full path of the provided base URL. This can be achieved using URL.JoinPath, but I wanted to open this issue instead of a pull request to discuss the desired behavior first.
Here's a test case showing the desired behavior and how the current code currently fails.
funcTestURLPath(t*testing.T) {
tt:=map[string]struct {
baseURLstringrelativePathstringexpectedFinalURLstring
}{
"api behind a context path": {
baseURL: "https://api.oxide.computer/some/api/path",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/some/api/path/v1/system/hardware/disks",
},
"api behind a context path with trailing slash": {
baseURL: "https://api.oxide.computer/some/api/path/",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/some/api/path/v1/system/hardware/disks",
},
"api behind a context path with query parameters": {
baseURL: "https://api.oxide.computer/some/api/path?foo=bar",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/some/api/path/v1/system/hardware/disks?foo=bar",
},
"api at the root path": {
baseURL: "https://api.oxide.computer",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/v1/system/hardware/disks",
},
"api at the root path with trailing slash": {
baseURL: "https://api.oxide.computer/",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/v1/system/hardware/disks",
},
"api at the root path with query parameters": {
baseURL: "https://api.oxide.computer?foo=bar",
relativePath: "/v1/system/hardware/disks",
expectedFinalURL: "https://api.oxide.computer/v1/system/hardware/disks?foo=bar",
},
}
fortestName, testCase:=rangett {
t.Run(testName, func(t*testing.T) {
baseURL, err:=parseBaseURL(testCase.baseURL)
iferr!=nil {
t.Fatalf("failed parsing base url: %v", err)
}
finalURL:=resolveRelative(baseURL, testCase.relativePath)
assert.Equal(t, testCase.expectedFinalURL, finalURL)
})
}
}
What is the current behaviour and what actions did you take to get there
A call to NewClient calls parseBaseURL to verify whether the passed Oxide API host is a valid URL.
Under the hood, resolveRelative uses URL.ResolveReference to build the final URL. When URL.ResolveReference is passed a path with a leading /, the resulting final URL is an absolute URL from the root of the base URL. This means that users of this Oxide API client cannot have their API served from any URL path other than the root.
For example, one cannot use a base URL of https://api.oxide.computer/some/api/path because an API call to /v1/system/hardware/disks would result in a final URL of https://api.oxide.computer/v1/system/hardware/disks, dropping the /some/api/path part of the URL.
Go SDK version
latest
Operating system
Fedora 39
Anything else you would like to add?
No response
The text was updated successfully, but these errors were encountered:
Hey there! Thanks a bunch for taking the time to open up this issue, and meticulously documenting the desired behaviour. This makes it very easy to understand what you're after :)
This functionality is indeed by design. While this feature may be desirable for testing a mock API or something similar, our product is currently designed in such a way that the API is always served from the root URL.
To avoid unintended errors, and to ensure that in the future we can enforce certain versions in the path (/v1/, /v2/, /v3/), it should remain this way for now.
Of course, if in the future we find that this feature is necessary, this issue can always be revisited!
This functionality is indeed by design. While this feature may be desirable for testing a mock API or something similar, our product is currently designed in such a way that the API is always served from the root URL.
Thank you for clarifying! My thought was that customers may place a web application firewall or similar in front of the Oxide API and serve it via a different URL path. The on-premises world is so unpredictable 😆.
Of course, if in the future we find that this feature is necessary, this issue can always be revisited!
Awesome, I'll leave this open then. Feel free to modify the issue as necessary. Thank you!
Preliminary checks
What was the expected behaviour
The expected behavior is that the Oxide API client will respect the full path of the provided base URL. This can be achieved using URL.JoinPath, but I wanted to open this issue instead of a pull request to discuss the desired behavior first.
Here's a test case showing the desired behavior and how the current code currently fails.
What is the current behaviour and what actions did you take to get there
A call to
NewClient
callsparseBaseURL
to verify whether the passed Oxide API host is a valid URL.oxide.go/oxide/lib.go
Line 91 in 20c490d
Each API method of this client calls
resolveRelative
to build the final URL for its API request.oxide.go/oxide/utils.go
Lines 11 to 20 in 20c490d
Under the hood,
resolveRelative
uses URL.ResolveReference to build the final URL. WhenURL.ResolveReference
is passed a path with a leading/
, the resulting final URL is an absolute URL from the root of the base URL. This means that users of this Oxide API client cannot have their API served from any URL path other than the root.For example, one cannot use a base URL of
https://api.oxide.computer/some/api/path
because an API call to/v1/system/hardware/disks
would result in a final URL ofhttps://api.oxide.computer/v1/system/hardware/disks
, dropping the/some/api/path
part of the URL.Go SDK version
latest
Operating system
Fedora 39
Anything else you would like to add?
No response
The text was updated successfully, but these errors were encountered: