Add staged connection diagnosis for opaque dial failures - #1114
Add staged connection diagnosis for opaque dial failures#1114rossnelson wants to merge 12 commits into
Conversation
|
|
Covers classifyGRPCError, connectSummary's grep-compatibility contract, and an end-to-end case where the failing address comes from a config profile (exercising the new cliext builder metadata).
- errors.Is(err, syscall.ECONNREFUSED) doesn't match Windows' WSAECONNREFUSED; fall back to matching the error message. - The plaintext test server closed with the client's ClientHello unread, sending an RST that on Windows discards the buffered HTTP response before the probe reads it; drain before closing.
…nosis # Conflicts: # internal/temporalcli/commands.go
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9e46c124
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case errors.As(err, &hostnameErr): | ||
| d.fail("TLS handshake failed: server certificate is not valid for this host: " + shortErr(err)) | ||
| d.Cause = causeHostnameMismatch | ||
| case errors.As(err, &unknownAuthErr), errors.As(err, &certErr): |
There was a problem hiding this comment.
When the TLS handshake fails because a trusted certificate is expired, not yet valid, or has an incompatible usage, Go wraps the underlying x509.CertificateInvalidError in *tls.CertificateVerificationError, so this broad match classifies it as causeCAVerify. suggestAction then incorrectly tells the user to configure --tls-ca-path, which cannot fix those certificate-validity failures. Reserve causeCAVerify for unknown-authority errors, or classify other verification errors separately so the diagnostic preserves the actual remedy.
Useful? React with 👍 / 👎.
| if runtime.GOOS == "windows" { | ||
| shell = displayShellPowerShell | ||
| } |
There was a problem hiding this comment.
When the CLI is launched from cmd.exe, runtime.GOOS is still windows, so the suggested command is rendered as PowerShell syntax such as & 'temporal' 'server' 'start-dev'. Command Prompt treats those single quotes as literal characters and cannot execute that command, making the proposed recovery step unusable for cmd users. Detect the active shell or render the current safe invocation in syntax accepted by both Windows shells.
Useful? React with 👍 / 👎.
| // (or rejects us at the certificate step, which still means it spoke TLS), the | ||
| // mismatch is the likely root cause. | ||
| func probeServerSpeaksTLS(ctx context.Context, conn net.Conn, host string) (connectCause, string) { | ||
| tlsConn := tls.Client(conn, &tls.Config{InsecureSkipVerify: true, ServerName: host}) |
There was a problem hiding this comment.
When the target TLS endpoint requires ALPN negotiation for h2, this raw crypto/tls probe sends no NextProtos, so the server can reject it with no application protocol. The real gRPC TLS transport advertises h2, meaning the diagnostic follows a different handshake path and can fail to detect that a plaintext-configured client is actually contacting a TLS server; the configured-TLS probe has the same omission and may report a spurious TLS-handshake failure. Add h2 to the cloned/probe TLS configurations so the checks reproduce the gRPC transport.
Useful? React with 👍 / 👎.
3 participants
Related issues
Related to #224 and #851.
What changed?
This PR adds bounded, evidence-based diagnosis after a failed connection, turning opaque dial errors into supported next steps when the evidence allows.
Before this change, failed connections could end as opaque dial errors. After the real failure, the CLI runs bounded DNS, TCP, and TLS checks in sequence. It gives advice only when the evidence supports it. Diagnosis stops after three seconds or cancellation.
TEMPORAL_CLI_DISABLE_CONNECT_DIAGNOSISdisables it. A generic TLS handshake failure does not prove mTLS, so the CLI gives no certificate advice.This PR adds one connection-specific check to the existing
CommandOptions.Failpath. It leaves all other commands, generated command flow, Activity errors, extensions, stdout, status, and usage behavior unchanged.Checklist
Stability
-o json/-o jsonl) are treated as breaking changesDesign
temporal <noun> <verb>structure (e.g.temporal workflow start)--search-attribute, bad:--index-field)(Experimental)incommands.yamlHelp text (see style guide at the top of
commands.yaml)--namespace, not-n), one flag per lineYourXxxform (YourWorkflowId,YourNamespace)Behavior
Tests
SharedServerSuite)func TestXxx) where applicableManual tests
Setup
No manual setup was used.
Happy path
No manual happy-path run was used. After merge, the full suite passed in
internal/temporalcliin 166.201s:Error case
Connection-focused
go test -racetests passed. The concurrent development-server race still reports the existing upstream global color race in unchangedcommands.golines. This PR does not claim that the full race suite passes.Composition
No manual composition test was used.