Skip to content
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

Respect the full path of the Oxide API base URL #194

Open
2 tasks done
sudomateo opened this issue Feb 19, 2024 · 2 comments
Open
2 tasks done

Respect the full path of the Oxide API base URL #194

sudomateo opened this issue Feb 19, 2024 · 2 comments
Labels
enhancement Improvements to the current code that are not breaking current functionality

Comments

@sudomateo
Copy link
Contributor

sudomateo commented Feb 19, 2024

Preliminary checks

  • 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.

func TestURLPath(t *testing.T) {
        tt := map[string]struct {
                baseURL          string
                relativePath     string
                expectedFinalURL string
        }{
                "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",
                },
        }

        for testName, testCase := range tt {
                t.Run(testName, func(t *testing.T) {
                        baseURL, err := parseBaseURL(testCase.baseURL)
                        if err != 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.

host, err := parseBaseURL(host)

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

// resolveRelative combines a url base with a relative path.
func resolveRelative(basestr, relstr string) string {
u, _ := url.Parse(basestr)
rel, _ := url.Parse(relstr)
u = u.ResolveReference(rel)
us := u.String()
us = strings.Replace(us, "%7B", "{", -1)
us = strings.Replace(us, "%7D", "}", -1)
return us
}

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

@sudomateo sudomateo added the bug label Feb 19, 2024
@karencfv
Copy link
Collaborator

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!

@karencfv karencfv added enhancement Improvements to the current code that are not breaking current functionality and removed bug labels Feb 19, 2024
@sudomateo
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to the current code that are not breaking current functionality
Projects
None yet
Development

No branches or pull requests

2 participants