Skip to content

feat(client): include request URI context in ApiError (#949)#1997

Open
goodbsw wants to merge 4 commits into
kube-rs:mainfrom
goodbsw:fix/issue-949-unhelpful-404-errors
Open

feat(client): include request URI context in ApiError (#949)#1997
goodbsw wants to merge 4 commits into
kube-rs:mainfrom
goodbsw:fix/issue-949-unhelpful-404-errors

Conversation

@goodbsw

@goodbsw goodbsw commented Jun 9, 2026

Copy link
Copy Markdown

Motivation

When handling API errors (such as 404 Not Found, 409 Conflict, etc.) returned by the Kubernetes API server, the current Error::Api variant only wraps the raw Status object. This makes debugging highly problematic in large-scale infrastructure environments because the error context does not indicate which specific URI/endpoint failed.

For instance, when a service mesh or external client receives an unhelpful API error, troubleshooting requires cross-referencing external logs since the exact request path is completely lost. This PR resolves #949 by injecting the missing request URI directly into the Error::Api variant.

Solution

To bypass the architectural constraint where the Response object independently lacks request context metadata, we implemented a sidecar metadata pocket mechanism utilizing HTTP response extensions:

  1. Error::Api Variant Refactoring (kube-client/src/error.rs)

    • Transformed the Api variant from a tuple struct to a struct variant: Api { source: Box<Status>, uri: http::Uri }.
    • Updated the error macro formatting to explicitly print the failed URI path along with the API status.
  2. Context Injection in Client Pipeline (kube-client/src/client/mod.rs)

    • In Client::send, cloned the original request.uri() before the request ownership is moved.
    • Inserted the cloned URI into the response metadata using response.extensions_mut().insert(req_uri).
    • In handle_api_errors, extracted the http::Uri type back from the extensions map and successfully packed it into the new Error::Api struct layout.
  3. Watch API Stream Support & Test Realignment

    • Fixed the asynchronous stream closure in request_events by applying the move keyword to propagate req_uri across async boundaries cleanly.
    • Fixed all pattern matchings and assertion breaks across unit/doc-tests (lib.rs, entry.rs, core_methods.rs).
    • Verified that all 50+ unit tests and 50+ doc-tests pass flawlessly (test result: ok).

Signed-off-by: Seungweon Baek <ops@caffy.io>
@goodbsw goodbsw force-pushed the fix/issue-949-unhelpful-404-errors branch from 56d493a to a3815a4 Compare June 9, 2026 15:58
…anges

Signed-off-by: Seungweon Baek <goodbsw@gmail.com>
@goodbsw goodbsw force-pushed the fix/issue-949-unhelpful-404-errors branch from a3815a4 to ec70b63 Compare June 9, 2026 22:31
@doxxx93

doxxx93 commented Jun 10, 2026

Copy link
Copy Markdown
Member

Thanks for picking this up, nice write-up! Quick heads-up before this goes further: there's already a PR for #949 with some maintainer review on it, #1973. Might be worth syncing up so we don't end up with two different takes on the same thing.

Couple of things from that thread that probably apply here too:

clux suggested reusing the existing BaseUri middleware for this. His review on #1973 (comment) basically says: hook into the existing BaseUri tower layer instead of adding a separate mechanism just for error context. The extensions sidecar here kind of goes the opposite way.

There's also a subtle gap with grabbing request.uri() at the client layer. BaseUri rewrites the URI inside the tower stack, so before the stack you only have the relative path. For a rancher-style cluster (https://example.com/foo/bar) the actual path is /foo/bar/api/v1/... but you'd capture /api/v1/..., and the host is gone too. So the URI on the error wouldn't quite match what got sent. BaseUri is the layer that already knows the final URL (rancher case included).

One more: this flips Error::Api from a tuple to a struct variant, which is a breaking change. #1973 keeps Error::Api(Status) as-is and just adds the URI to the debug logs, so probably worth a deliberate call on whether we want the breaking version or not.

Anyway, might be worth syncing with @AkashKumar7902 on #1973 and landing one approach. Happy to help review whichever way it goes 👍

@goodbsw

goodbsw commented Jun 10, 2026

Copy link
Copy Markdown
Author

Thanks for the incredibly detailed and insightful review @doxxx93!

You made an excellent point regarding the Rancher-style proxy clusters (/foo/bar/api/v1/...). Capturing request.uri() at the outer client layer would indeed truncate the host and base path context, leading to inaccurate diagnostic logs for SREs running multi-tenant topologies. I also completely agree on avoiding breaking changes for Error::Api to keep the upgrade path seamless.

Since #1973 is already making solid progress with maintainer feedback, I’ll sync up with @AkashKumar7902 over there. I’d love to see if we can ensure that #1973 hooks into the BaseUri tower layer properly to capture the full, rewritten URI context without breaking anything.

I'll head over to #1973 to collaborate and make sure we land the best approach for #949. Thanks again for guiding me in the right direction! 🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhelpful 404 error for missing API resources

3 participants