Repository navigation
Conversation
Building a reqwest client loads the system's TLS root certificates from disk, and fails if none can be read (e.g., when out of file descriptors). Control plane services talk to each other over plain HTTP, so add builders that trust no TLS roots and therefore do no I/O. See #10871.
Progenitor-generated Client::new builds a reqwest client that loads TLS root certificates from disk, and unwraps the result. For clients that talk plain HTTP to other control plane services, build the client with internal_http_client() instead, which does no I/O.
Clients that talk plain HTTP to other control plane services don't need TLS root certificates. Build them with internal_http_client_builder(), so building the client does no I/O.
Building a reqwest client can fail. Return the error rather than panicking, so that callers can handle it.
Handle failures to build HTTP clients the same way each call site already handles failures to reach the service in question: return the error where the caller is fallible, and otherwise log it and skip that backend (or record it as a per-target error), as these call sites already do for request failures.
Where a saga retries pantry requests with retry_operation_while_indefinitely, build the pantry client inside the retried operation and report a build failure as a communication error, so it is retried like any other transient failure. This keeps undo actions (e.g., detaching a disk from a pantry) from failing and leaving the saga stuck. Elsewhere, client construction failures become action failures, matching how those actions handle failed requests. Also include the underlying error when Nexus fails to get a sled agent client in instance_common.
sled-agent, oximeter, wicketd, wicket, installinator, and the CockroachDB admin crates: handle failures to build HTTP clients instead of panicking. Clients built once at startup propagate the error to startup; per-request clients report it through the request's existing error path; and polling or retry loops retry building the client.
reqwest::Error's Display impl prints only the error kind (e.g., "builder error"); the underlying cause is in its source. Use InlineErrorChain where we log or stringify these errors.
Return reqwest::Error from the helper, and map it to a (retryable) communication error at the call sites inside retry loops. Use the helper at the non-retrying pantry call sites as well.
- Describe the module as the central configuration for internal HTTP clients, and move the TLS-roots explanation to the tls_certs_only call. - Point callers that talk to services outside the rack at Nexus's external_client module. - Clarify that the remaining expect only fires on programmer error.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #11482.
#11482 stopped internal HTTP clients from doing I/O when they're built, but kept the existing
unwrap/expectcalls. This PR makesinternal_http_client()return aResultand handles construction failures instead of panicking. In general, each call site handles a client-construction failure the same way it already handles failing to reach the service in question:PrecheckError::SledAgentClient,ApplyUpdateError::MgsClient,TimeSyncError::NtpAdminClient, sled-agent'sinstance::Error::PropolisClient).retry_operation_while_indefinitely, the pantry client is now built inside the retried operation, and a build failure is reported as aCommunicationErrorso it is retried like other transient failures. This keeps undo actions (detaching a disk from a pantry) from leaving sagas stuck. Elsewhere, construction failures become action failures, matching how those actions treat failed requests.Two
expects remain, both in code that only runs at sled-agent startup, where propagating would require restructuring for little benefit (the result is the same: sled-agent exits and is restarted):ArtifactStore::new(called fromspawn_all_longrunning_tasks, which isn't fallible)Reconciler::new)Since #11482, building these clients does no I/O, so these could only fail due to a configuration error.
Part of #10871.