Skip to content

fcli aviator: Fail fast with clear messages when commands cannot connect - #1145

Merged
rsenden merged 25 commits into
fortify:dev/v3.xfrom
kireetivar:p/cdatla/aviator-connectivity-failure-handling
Oct 28, 2026
Merged

rsenden merged 25 commits into
fortify:dev/v3.xfrom
kireetivar:p/cdatla/aviator-connectivity-failure-handling

Conversation

@kireetivar

@kireetivar kireetivar commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Ordinary Aviator admin, user-login, and admin-config commands no longer wait for the gRPC channel to become ready. A refused connection fails when the operating system reports the refusal. The 30-second request deadline is unchanged.

Audit, correlation, diagnostics, and the existing public createClient(url) constructors still wait. A new ordinary command gets fail-fast only when it uses createGrpcClient or passes AviatorRequestPolicy.FAIL_FAST.

The two-line "was refused" message is used for known English JDK and Netty refusal reasons. Other transport failures get a short unavailable, timeout, DNS, or proxy message. Terminal output and DEBUG logs omit URL userinfo and proxy credentials. DEBUG records the failure kind, the safe message, and cause class names.

A failed Aviator user-login or admin-config validation leaves the existing session or admin config in place. SSC and FoD login behavior is unchanged.

fix: `fcli aviator`: Fail fast with clear messages when ordinary commands cannot connect
fix: `fcli aviator`: Keep the existing user session and admin config when validation fails
fix: `fcli aviator`: Do not show credentials from the Aviator URL in messages and logs
fix: `fcli aviator`: Fail audit streams that do not initialize within 30 seconds

kireetivar and others added 9 commits October 22, 2026 12:41
…ails

AbstractSessionLoginCommand destroyed an existing session before login() ran. If Aviator token or admin-config validation then failed (for example because the server could not be reached), the previous valid session or configuration was lost.

Add the isReplaceExistingSessionBeforeLogin() hook. The default keeps the current behavior for all other modules. The Aviator user session login and admin-config create commands opt out, so they validate first and only replace the stored descriptor after validation succeeds.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add AbstractFcliException.getSafeDiagnosticString(). The default returns null, so FcliExecutionExceptionHandler keeps logging the full throwable at DEBUG level. An exception whose causes can hold sensitive text can return a safe description instead; the handler then logs only that description.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add the types for consistent Aviator connectivity failure reporting:

- AviatorRequestPolicy: ORDINARY (fail fast, shared messages) or UNCHANGED (current behavior)
- AviatorConnectionContext/AviatorEndpoint: safe route summary without URLs or credentials
- AviatorConnectivityFailureKind: one message per failure kind
- AviatorConnectivityFailureClassifier: maps UNAVAILABLE/DEADLINE_EXCEEDED to a kind using only exception types, never status descriptions or messages; TLS failures (SSLException, CertificateException) keep their current handling
- AviatorConnectivityException: prints only the user message and supplies a safe DEBUG description

The grpc package does not depend on the diagnose package. The types are not used yet.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GrpcUtil.executeGrpcCall() now accepts an AviatorRequestPolicy and a connection context. With the ORDINARY policy, unary calls run without wait-for-ready (through a per-call interceptor, because gRPC 1.76 stubs have no withoutWaitForReady()), and UNAVAILABLE/DEADLINE_EXCEEDED failures become AviatorConnectivityException. The 30-second deadline, the shared stubs, and streaming calls do not change.

AviatorGrpcClient and AviatorGrpcClientHelper get overloads that accept the policy. All existing constructors and createClient() methods keep the UNCHANGED policy, so no caller changes behavior yet. The existing status mapping is extracted into toAviatorException() without changes to messages or logging.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Aviator app, token, and entitlement commands, 'aviator session login', and 'aviator admin-config create' now use the ORDINARY request policy. When the Aviator server or the configured proxy refuses the connection, these commands now fail at once instead of waiting until the 30-second request deadline, and print a short message, for example:

  Connection to proxy localhost:33444 was refused
  Check that the proxy is running and that its host and port are correct

All admin commands get the policy through AbstractAviatorAdminSessionOutputCommand.createGrpcClient(). AviatorUserSessionHelper.validateToken() and AviatorAdminConfigHelper.validateConfig() get overloads with a policy; the existing overloads keep the UNCHANGED policy, so connection diagnostics, audit, and correlation commands keep their current behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
parseTarget() and normalizeUrl() included the raw Aviator URL in error messages, in a WARN log entry, and in UnsupportedAviatorUrlSchemeException (which connection diagnostics copy into their evidence). A URL with user info, such as https://user:password@host, exposed the password. These places now show the URL with the user-info part replaced by ***. URLs without user info are shown unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Consolidate classification and inline messages, retain the existing connection timeout, and recognize Windows connection refusals. Protect sensitive causes in terminal output and malformed URL errors.

Add TLS CONNECT proxy and address-fallback coverage with focused test helpers. Validation: 927 tests passed across common-core, aviator-common, and aviator; git diff --check passed.
@kireetivar kireetivar self-assigned this Oct 23, 2026
kireetivar and others added 3 commits October 23, 2026 11:16
The message is safe to show, so the standard fcli simple exception output is enough. Remove the cause-sensitive and safe-diagnostic hooks from AbstractFcliException, and log only cause types at DEBUG level.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Name the failing proxy or Aviator server, give one remediation, and state the 30-second request limit. Add a TLS failure kind for non-certificate TLS errors; certificate failures keep the fcli PKIX trust-store guidance.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The generic message no longer blames the server for failures that can be client-side, names the gRPC status, and tells the user how to log details.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@kireetivar
kireetivar marked this pull request as ready for review October 26, 2026 05:09
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@kireetivar
kireetivar requested a review from rsenden October 26, 2026 05:26
Rename the hook to shouldReplaceExistingSessionBeforeLogin and use a separate opt-out test subclass so the default lifecycle test exercises the production default.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kireetivar and others added 5 commits October 26, 2026 14:52
Keep unavailable responses neutral and mention server load for request deadlines. Cover safe cause-free statuses and preserve factory request policies.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Omit ignored paths, queries, fragments, and userinfo contents from display URLs. Remove the unused client factory overload and reuse connection-plan validation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pass WAIT_FOR_READY explicitly for diagnostic credential validation and remove unused validation overloads. Preserve ordinary fail-fast calls and existing validator interfaces.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Explain fail-fast call options, unchanged deadlines, and the pre-save session preservation boundary.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use neutral deadline guidance and cover an immediate server-returned DEADLINE_EXCEEDED response with a real Netty regression test. Keep the existing 30-second request deadline unchanged.
public Application createApplication(String name, String tenantName, String signature, String message) {
CreateApplicationRequest request = CreateApplicationRequest.newBuilder().setName(name).setTenantName(tenantName).setSignature(signature).setMessage(message).build();
return GrpcUtil.executeGrpcCall(blockingStub, ApplicationServiceGrpc.ApplicationServiceBlockingStub::createApplication, request, Constants.OP_CREATE_APP);
return GrpcUtil.executeGrpcCall(blockingStub,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: the question on scope. Commit 16a68cc4f9 only tweaks wording, so it isn't enough on its own. The line-wrapping of these executeGrpcCall call sites (here and ~15 others) landed inside the feature commits c9a5e11ede and 5534a7c787, and the RESOURCE_EXHAUSTED/default message rewrite is in an earlier feature commit as well. Please either move the reformatting and message rewrites into one standalone commit (so blame and bisect stay clean), or drop the reformatting. Since fcli merges with merge commits, the commit structure is what ends up in history.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped the reformatting. For the message rewrite, I think we squash-merge PRs in fcli, so the commit split wouldn't survive in history anyway

Comment thread doc-resources/repo-usage.md Outdated

For more information about available Docker images and usage instructions, see the [fcli-docker repository](https://github.com/fortify/fcli-docker).

## Aviator Connectivity

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is the template for the repo-level USAGE.md (Docker images, usage pointers), which is auto-generated from doc-resources and was not regenerated in this PR. Aviator command behaviour doesn't look like repo-usage content. Could you move it to the user documentation (or keep it in the PR description only)? If we do want it here, USAGE.md needs regenerating too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this got in by mistake I added this in the wrong place. I’ll remove it from the repo-usage template and keep the explanation in the PR description

public abstract boolean isSingular();

/** The caller must close the client. */
protected final AviatorGrpcClient createGrpcClient(AviatorAdminConfigDescriptor configDescriptor) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: ordinary commands only get fail-fast when they go through this helper, and the public createClient(url) still waits. Consider a short Javadoc line here such as "ordinary admin commands must use this; do not call AviatorGrpcClientHelper.createClient(url) directly", so a future command doesn't silently hang again. Not a blocker.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the factories that didn't take a policy, so every caller now has to pick one explicitly. Ordinary commands use FAIL_FAST and audit/diagnostics use WAIT_FOR_READY, so a new command can't end up waiting by accident.

kireetivar and others added 3 commits October 27, 2026 13:46
Remove policy-free factories and unused constructors. Audit and diagnostic callers explicitly select WAIT_FOR_READY; ordinary commands retain FAIL_FAST. Streaming configuration is unchanged. Test both policies through both public factory signatures.
Cancel pending RPCs and release SAST initialization waiters on timeout. Disarm the initialization timer when the stream initializes, preserving established-stream processing and existing recovery behavior. Leave total operation timeouts and SSC upload handling unchanged.

Add regression coverage for silent initialization, cancellation, late responses, and successful initialization. Both Aviator test suites and formatting checks pass (686 passed, 2 skipped).
@rsenden
rsenden merged commit bb10f54 into fortify:dev/v3.x Oct 28, 2026
10 checks passed
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.

3 participants