Avoid accessing missing interfaces on other nodes #7392
Conversation
…_precise_redirect
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…_precise_redirect
…face_assumptions2
…face_assumptions2
There was a problem hiding this comment.
Pull Request Overview
This PR improves redirect handling for node RPC endpoints by ensuring redirects only target interfaces that exist on the destination node. The main change is refactoring the get_redirect_address_for_node helper to be used consistently across multiple endpoints (/node/join, /node/primary, and /node/snapshot), preventing attempts to redirect to non-existent interfaces.
Key changes:
- Unified redirect logic using
get_redirect_address_for_nodeacross multiple endpoints - Changed return type from
std::optional<std::string>tostd::variant<std::string, JsonAdapterResponse>to support both json_adapter and non-json_adapter endpoints - Added test coverage to verify error handling when redirecting to interfaces that don't exist on the target node
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/infra/interfaces.py | Added from_str static method to RPCInterface for creating instances from string addresses |
| tests/e2e_common_endpoints.py | Added test case that creates a node with a unique interface and verifies proper error handling when redirecting to missing interfaces |
| src/snapshots/fetch.h | Cleared response buffer after redirect to avoid carrying over redirect response bodies |
| src/node/rpc/node_frontend.h | Refactored redirect logic to use centralized helper function consistently across endpoints, converted several endpoints to use json_adapter |
| return ccf::make_error( | ||
| HTTP_STATUS_PERMANENT_REDIRECT, | ||
| ccf::errors::NodeCannotHandleRequest, | ||
| "Redirecting to primary"); |
There was a problem hiding this comment.
The error message 'Redirecting to primary' is misleading in the context of the /snapshot endpoint. This message suggests redirection to the primary node, but this endpoint redirects to the current node's snapshot. Consider changing the message to something like 'Redirecting to snapshot' to accurately reflect the operation.
| "Redirecting to primary"); | |
| "Redirecting to snapshot"); |
| // Ignore any body from redirect responses | ||
| response_body->buffer.clear(); |
There was a problem hiding this comment.
Explanation: When we make a GET request, we don't know whether it's going to be a redirect or a partial body. We use the same response_body object for all calls, and it blindly pastes the body onto the end of an internal buffer. That's exactly the behaviour that we want when we're calling subsequent range requests, but we don't want to include the body of a redirect response. Previously they had no body so this wasn't a problem, now they do.
Ideally we'd be deciding how to handle the body based on the headers, but that's awkward with curl. Perhaps we should create a fresh ResponseBody per response, and glue together the ones we want at the end? That implies some extra resizing and probably a copy, so this is faster, just ugly.
|
Superceded by #7395, which makes some different choices to make the diff a little tidier. |
1 participant
Aiming to use the
get_redirect_address_for_nodehelper added in #7373 for othernode_frontend.hredirects, to make sure they don't try to redirect to an interface that doesn't exist on the target node.That should be a tiny diff, but because some of the endpoints use
json_adapterand others don't, it's extremely awkward and unwieldy. I may take another stab at this because the diff is so ugly, but if that fails we should merge this to get a fix in.