Add staged connection diagnosis for opaque dial failures - #1114
Add staged connection diagnosis for opaque dial failures#1114rossnelson wants to merge 13 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 👍 / 👎.
|
I think the codex review is worth taking a look at but everything else lgtm. IMO we are free to change the error output at will. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f2ebe7436
ℹ️ 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".
| if cfg.ServerName == "" && !cfg.InsecureSkipVerify { | ||
| cfg.ServerName = host |
There was a problem hiding this comment.
When --client-authority differs from the address host and no explicit --tls-server-name is set, gRPC uses that authority as the server name for its authentication handshake, but this probe replaces the empty ServerName with the address host. Thus a certificate valid for the configured authority can pass the real TLS handshake and then, after an unrelated gRPC failure, be reported as a hostname mismatch. The generated CLI help confirms that --client-authority controls the gRPC :authority value (cliext/flags.gen.go:91); pass the effective authority into the probe so it follows the same TLS path.
Useful? React with 👍 / 👎.
| if strings.Contains(msg, "certificate required") || | ||
| strings.Contains(msg, "bad certificate") { |
There was a problem hiding this comment.
When the client already supplies a certificate that the server rejects, TLS alert 42 is rendered as bad certificate; treating that alert as causeClientCertRequired produces the action to configure --tls-cert-path and --tls-key-path even though both may already be configured. Split the rejected-certificate case from the certificate required alert so users are told to validate or replace the configured certificate rather than merely enable mTLS.
Useful? React with 👍 / 👎.
| if net.ParseIP(host) == nil { | ||
| addrs, err := net.DefaultResolver.LookupHost(ctx, host) |
There was a problem hiding this comment.
When the address is a valid scoped IPv6 endpoint such as [fe80::1%eth0]:7233, SplitHostPort returns fe80::1%eth0, but net.ParseIP rejects the zone suffix. The code consequently sends the literal to LookupHost, reports a DNS failure, and stops before the valid TCP endpoint can be tested. Parse zoned addresses with net/netip or strip and validate the zone before deciding that the host requires DNS.
Useful? React with 👍 / 👎.
| case causeServerPlaintext: | ||
| return &displayAction{Label: fmt.Sprintf("The server at %s does not appear to use TLS. Remove --tls and related TLS flags, or check the address.", meta.Address)} |
There was a problem hiding this comment.
When TLS was enabled implicitly by --api-key or by profile/environment configuration, users may not have any --tls or related TLS flags to remove, and retrying as suggested leaves the effective TLS configuration unchanged. The generated help explicitly says TLS is auto-enabled by API keys and TLS options (cliext/flags.gen.go:95), so the action should explain how to disable or correct the effective setting rather than assuming it came from removable flags.
Useful? React with 👍 / 👎.
| // to its own error handling logic, and just copy the exit code through. | ||
| os.Exit(exitError.ExitCode()) | ||
| } | ||
| if writeConnectionError(c.Options.Stderr, err, !color.NoColor) { |
There was a problem hiding this comment.
With the default --color auto, this passes the process-wide color.NoColor state even though the report is written to CommandOptions.Stderr. If stdout is a terminal while stderr is redirected, or an embedding supplies a non-terminal buffer while the process has terminal OS streams, color.NoColor can remain false and the redirected error report receives ANSI escape sequences. Determine automatic coloring from the actual configured stderr writer, while preserving the explicit always and never policies.
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.