Add audited AWS telemetry diagnostics - #435
Conversation
|
@codex review |
![superlog-app[bot]](https://avatars.githubusercontent.com/in/3485414?s=60&v=4){kind=small}
| } | ||
|
|
||
| export async function runAwsDiagnostics( | ||
| target: AwsDiagnosticTarget, |
There was a problem hiding this comment.
logs · blocking — Add an error log for unexpected probe failures in runAwsDiagnostics
Log the caught exception at error level before falling back to unavailableResult so operators can distinguish unexpected SDK/network failures from expected AccessDenied probe errors; without this, silent fallbacks are invisible in any log aggregator.
| target: AwsDiagnosticTarget, | |
| } catch (error) { | |
| if (!(error instanceof AwsDiagnosticProbeError)) { | |
| console.error( | |
| { err: error, connectionId: target.connectionId, projectId: target.projectId }, | |
| "aws diagnostics probe failed unexpectedly", | |
| ); | |
| } | |
| result = unavailableResult(error); | |
| } |
Useful? React with 👍 / 👎.
![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: 688de6ae78
ℹ️ 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".
| client: DiagnosticClient, | ||
| permissionGaps: string[], | ||
| ): Promise<AwsDiagnosticFacts["stack"]> { | ||
| for (const stackName of ["superlog-connect", "superlog-metrics-stream", "superlog-logs-stream"]) { |
There was a problem hiding this comment.
When a customer changes the stack name in AWS Quick Create—for example to another permitted superlog-* name—the resources can be fully deployed while this exact-name lookup misses every stack and reports that no Superlog stack exists. The launch flow supplies only a default name and does not persist the customer's final choice, so diagnostics should discover or record the actual stack name rather than restricting lookup to these three defaults.
Useful? React with 👍 / 👎.
| if (stack?.StackName && stack.StackStatus) { | ||
| return { name: stack.StackName, status: stack.StackStatus }; |
There was a problem hiding this comment.
When a connection uses the legacy setup, superlog-connect normally exists alongside separate superlog-metrics-stream and superlog-logs-stream stacks, but this return stops after the role stack. Consequently a streaming stack in ROLLBACK_COMPLETE can be ignored—and, if its retained stream resources still look active, the diagnostic can report the deployment as healthy based solely on superlog-connect; collect and evaluate all applicable stack statuses instead.
Useful? React with 👍 / 👎.
|
@codex review |
| ): Promise<AwsDiagnosticRun> { | ||
| let result: AwsDiagnosticResult; | ||
| try { | ||
| result = evaluateAwsDiagnostics(await deps.probe.inspect(target)); |
There was a problem hiding this comment.
logs · blocking — Log unexpected probe errors before converting to unavailableResult
Add an error-level log with the caught value before calling unavailableResult, so that novel AWS SDK errors (network timeouts, throttling, SDK bugs) that are not AwsDiagnosticProbeError are visible in the application log stream and not silently converted to a generic DiagnosticUnavailable status.
| result = evaluateAwsDiagnostics(await deps.probe.inspect(target)); | |
| } catch (error) { | |
| logger.error({ err: error, connectionId: target.connectionId, projectId: target.projectId }, "aws diagnostic probe failed unexpectedly"); | |
| result = unavailableResult(error); |
Useful? React with 👍 / 👎.
| requestedByUserId: user.id, | ||
| reason: parsed.data.reason || null, | ||
| }, | ||
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, |
There was a problem hiding this comment.
logs · warning — Add an info log when a diagnostic run completes
Emit an info-level structured log after runAwsDiagnostics returns, including connectionId, projectId, userId, runId, and status, so operators can correlate a specific run to a user action in the application log stream without querying the database.
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, | |
| const run = await runAwsDiagnostics( | |
| { | |
| connectionId: row.id, | |
| projectId: row.projectId, | |
| region: row.region, | |
| roleArn: row.scrapeRoleArn, | |
| externalId, | |
| expectedAccountId: row.accountId, | |
| requestedByUserId: user.id, | |
| reason: parsed.data.reason || null, | |
| }, | |
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, | |
| ); | |
| logger.info({ connectionId: row.id, projectId: row.projectId, userId: user.id, runId: run.id, status: run.status }, "aws diagnostic run completed"); | |
| return c.json(run); |
Useful? React with 👍 / 👎.
| } | ||
| return { | ||
| accessKeyId: credentials.AccessKeyId, | ||
| secretAccessKey: credentials.SecretAccessKey, |
There was a problem hiding this comment.
logs · blocking — Log the original AWS error code when role assumption fails
Before re-throwing as AwsDiagnosticProbeError, emit an error-level log with the original AWS error code and the target roleArn (without the externalId), so that role-assumption failures caused by SCPs, wrong trust policies, or expired external IDs are visible in the application log stream and not silently absorbed into the probe error.
| secretAccessKey: credentials.SecretAccessKey, | |
| } catch (error) { | |
| if (error instanceof AwsDiagnosticProbeError) throw error; | |
| const code = errorCode(error); | |
| logger.error({ err: error, errorCode: code, roleArn: target.roleArn, region: target.region }, "aws diagnostic role assumption failed"); | |
| throw new AwsDiagnosticProbeError(code); |
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd9627a4c2
ℹ️ 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".
| result = evaluateAwsDiagnostics(await deps.probe.inspect(target)); | ||
| } catch (error) { | ||
| result = unavailableResult(error); |
There was a problem hiding this comment.
When role assumption succeeds but a later AWS call fails for a reason not explicitly handled—such as throttling, a service outage, or a delivery stream disappearing between list and describe—probe.inspect() rejects into this catch, and unavailableResult() records a failed role check stating that the role could not be assumed. This produces an inaccurate audited result and directs operators to repair a working IAM role; preserve the successful role result or represent the downstream check as unavailable instead.
Useful? React with 👍 / 👎.
|
@codex review |
| }, | ||
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, | ||
| ); | ||
| return c.json(run); |
There was a problem hiding this comment.
logs · blocking — Add an error log when runAwsDiagnostics throws unexpectedly
Log at error level with connectionId, projectId, and the error code before re-throwing so operators can detect and alert on unexpected failures (e.g. DB unavailability, decryption errors) that are currently invisible in the log stream.
| return c.json(run); | |
| const run = await runAwsDiagnostics( | |
| { | |
| connectionId: row.id, | |
| projectId: row.projectId, | |
| region: row.region, | |
| roleArn: row.scrapeRoleArn, | |
| externalId, | |
| expectedAccountId: row.accountId, | |
| requestedByUserId: user.id, | |
| reason: parsed.data.reason || null, | |
| }, | |
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, | |
| ).catch((error: unknown) => { | |
| console.error("aws_diagnostics_run_failed", { | |
| connectionId: row.id, | |
| projectId: row.projectId, | |
| errorCode: (error as { code?: string })?.code ?? String(error), | |
| }); | |
| throw error; | |
| }); |
Useful? React with 👍 / 👎.
| return { | ||
| async record(run: AwsDiagnosticRunDraft) { | ||
| const [row] = await db.insert(schema.cloudConnectionDiagnosticRuns).values(run).returning(); | ||
| if (!row) throw new Error("failed to record AWS diagnostic"); |
There was a problem hiding this comment.
logs · blocking — Add an error log when the diagnostic DB insert fails
Log at error level with connectionId and projectId before throwing so operators can distinguish a DB failure from a probe failure; without it the diagnostic result is silently lost and the 500 response has no correlated server-side evidence.
| if (!row) throw new Error("failed to record AWS diagnostic"); | |
| const [row] = await db.insert(schema.cloudConnectionDiagnosticRuns).values(run).returning(); | |
| if (!row) { | |
| console.error("aws_diagnostics_record_failed", { | |
| connectionId: run.connectionId, | |
| projectId: run.projectId, | |
| status: run.status, | |
| }); | |
| throw new Error("failed to record AWS diagnostic"); | |
| } |
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| async function inspectIdentity( | ||
| target: AwsDiagnosticTarget, | ||
| credentials: TempCredentials, | ||
| factory: AwsDiagnosticClientFactory, | ||
| ): Promise<string> { | ||
| try { | ||
| const output = (await factory |
There was a problem hiding this comment.
logs · warning — Log at info level when AssumeRole succeeds for the diagnostic session
Emit a structured info log with connectionId, region, and identityAccountId after a successful role assumption so operators can confirm the diagnostic session was established and correlate it with downstream AWS API calls.
| } | |
| async function inspectIdentity( | |
| target: AwsDiagnosticTarget, | |
| credentials: TempCredentials, | |
| factory: AwsDiagnosticClientFactory, | |
| ): Promise<string> { | |
| try { | |
| const output = (await factory | |
| async function assumeDiagnosticRole( | |
| target: AwsDiagnosticTarget, | |
| factory: AwsDiagnosticClientFactory, | |
| ): Promise<TempCredentials> { | |
| try { | |
| const output = (await factory.sts({ region: target.region }).send( | |
| new AssumeRoleCommand({ | |
| RoleArn: target.roleArn, | |
| ExternalId: target.externalId, | |
| RoleSessionName: "superlog-diagnostics", | |
| DurationSeconds: 900, | |
| Policy: diagnosticSessionPolicy(target), | |
| }), | |
| )) as { | |
| Credentials?: { | |
| AccessKeyId?: string; | |
| SecretAccessKey?: string; | |
| SessionToken?: string; | |
| }; | |
| }; | |
| const credentials = output.Credentials; | |
| if (!credentials?.AccessKeyId || !credentials.SecretAccessKey || !credentials.SessionToken) { | |
| throw new AwsDiagnosticProbeError("MissingTemporaryCredentials"); | |
| } | |
| console.info("aws_diagnostics_role_assumed", { | |
| connectionId: target.connectionId, | |
| region: target.region, | |
| }); | |
| return { | |
| accessKeyId: credentials.AccessKeyId, | |
| secretAccessKey: credentials.SecretAccessKey, | |
| sessionToken: credentials.SessionToken, | |
| }; | |
| } catch (error) { | |
| if (error instanceof AwsDiagnosticProbeError) throw error; | |
| throw new AwsDiagnosticProbeError(errorCode(error)); | |
| } | |
| } |
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f19a47430f
ℹ️ 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".
| }; | ||
| for (const summary of output.StackSummaries ?? []) { | ||
| if ( | ||
| summary.StackName?.startsWith("superlog-") && |
There was a problem hiding this comment.
When a customer changes the editable CloudFormation stack name to something such as production-observability, this filter drops the stack before its ConnectionId parameter can be checked, so diagnostics incorrectly report that the stack is missing. The quick-create helper explicitly treats stackName as an editable default (cloud-connections-service.ts:31-32); the cloudformation:DescribeStacks resource patterns in the session and role policies must also permit such renamed stacks.
Useful? React with 👍 / 👎.
|
@codex review |
| status: "warning", | ||
| summary: "This check could not run.", | ||
| evidence: unavailableEvidence, | ||
| }, |
There was a problem hiding this comment.
logs · blocking — Log probe failures in runAwsDiagnostics before recording the error result
Add an error-level structured log when the probe throws so operators can detect and alert on diagnostic failures without querying the database. Include connectionId, projectId, and the sanitized errorCode for correlation; never log roleArn or externalId.
| }, | |
| } catch (error) { | |
| logger.error("aws_diagnostic_probe_failed", { | |
| connectionId: target.connectionId, | |
| projectId: target.projectId, | |
| region: target.region, | |
| errorCode: error instanceof AwsDiagnosticProbeError ? error.code : "DiagnosticUnavailable", | |
| roleAssumed: error instanceof AwsDiagnosticProbeError ? Boolean(error.context?.roleAssumed) : false, | |
| }); | |
| result = unavailableResult(error, target); |
Useful? React with 👍 / 👎.
| }, | ||
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, | ||
| ); | ||
| return c.json(run); |
There was a problem hiding this comment.
logs · warning — Add an info log on diagnostic run completion in the POST handler
Emit a structured info log after runAwsDiagnostics resolves so operators can correlate a user-triggered diagnostic run with its outcome (status, connectionId, runId) in the application log stream without relying solely on the database.
| return c.json(run); | |
| const run = await runAwsDiagnostics( | |
| { | |
| connectionId: row.id, | |
| projectId: row.projectId, | |
| region: row.region, | |
| roleArn: row.scrapeRoleArn, | |
| externalId, | |
| expectedAccountId: row.accountId, | |
| requestedByUserId: user.id, | |
| reason: parsed.data.reason || null, | |
| }, | |
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, | |
| ); | |
| logger.info("aws_diagnostic_run_completed", { | |
| runId: run.id, | |
| connectionId: run.connectionId, | |
| projectId: run.projectId, | |
| status: run.status, | |
| requestedByUserId: run.requestedByUserId, | |
| }); | |
| return c.json(run); |
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2842e00aa
ℹ️ 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".
| for (const stackName of [...new Set(candidateNames)]) { | ||
| try { | ||
| const output = (await client.send(new DescribeStacksCommand({ StackName: stackName }))) as { |
There was a problem hiding this comment.
In regions with hundreds or thousands of active or nested stacks, this loop makes one sequential DescribeStacks request for every stack before finding the few belonging to this connection. A diagnostic can therefore take minutes, exhaust the 15-minute assumed-role session, or abort entirely on a throttling error from any unrelated stack, causing the whole customer-facing run to be recorded as unavailable; discover matching stacks without a serial per-stack request, or batch/constrain this scan.
Useful? React with 👍 / 👎.
|
@codex review |
| }, | ||
| { | ||
| key: "metrics", | ||
| label: "CloudWatch metrics", |
There was a problem hiding this comment.
logs · warning — Log probe errors before recording the degraded result in runAwsDiagnostics
Add an error-level structured log inside the catch block that includes connectionId, projectId, and the resolved errorCode so operators can alert on and correlate probe failures without querying the database. Without it, every AWS-side failure (throttling, access denied, network) is silently converted to a stored error status with no server-side signal.
| label: "CloudWatch metrics", | |
| } catch (error) { | |
| const errorCode = error instanceof AwsDiagnosticProbeError ? error.code : "DiagnosticUnavailable"; | |
| logger.error("aws diagnostic probe failed", { | |
| connectionId: target.connectionId, | |
| projectId: target.projectId, | |
| region: target.region, | |
| errorCode, | |
| }); | |
| result = unavailableResult(error, target); | |
| } |
Useful? React with 👍 / 👎.
| reason: target.reason, | ||
| ...result, | ||
| }); | ||
| } |
There was a problem hiding this comment.
metrics · warning — Record a counter for diagnostic run outcomes in runAwsDiagnostics
Increment a module-scope counter (e.g. aws_diagnostic_runs_total with dimensions status ∈ {healthy, warning, error} and region) after deps.recorder.record(...) so operators can track run volume, error rates, and regional failure patterns without polling the database. The current code produces no metric signal on any outcome.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca5e8ec66e
ℹ️ 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".
| recordsDelivered: delivery.recordsDelivered, | ||
| minimumSuccessfulRecords: delivery.minimumSuccessfulRecords, | ||
| recentErrorCount: errors.length, | ||
| latestErrorCode: errors.at(0)?.code ?? null, |
There was a problem hiding this comment.
When multiple delivery errors occur within the inspection window, inspectDeliveryErrors preserves the events in chronological order and takes the last five, but this field selects the first of those entries. The persisted latestErrorCode therefore describes the oldest retained error rather than the latest one, which makes the audited evidence inaccurate when the error code changes; select the final matching error instead.
Useful? React with 👍 / 👎.
|
@codex review |
| }, | ||
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, | ||
| ); | ||
| return c.json(run); |
There was a problem hiding this comment.
logs · blocking — Add an error log when runAwsDiagnostics throws in the POST handler
Wrap the runAwsDiagnostics call in a try/catch and emit an error-level structured log with connectionId, projectId, and the error code so operators can distinguish a recorder failure from a probe failure; without it, a DB write error surfaces only as an unhandled 500 with no correlated evidence.
| return c.json(run); | |
| const run = await runAwsDiagnostics( | |
| { | |
| connectionId: row.id, | |
| projectId: row.projectId, | |
| region: row.region, | |
| roleArn: row.scrapeRoleArn, | |
| externalId, | |
| expectedAccountId: row.accountId, | |
| requestedByUserId: user.id, | |
| reason: parsed.data.reason || null, | |
| }, | |
| { probe: diagnosticProbe, recorder: diagnosticRecorder }, | |
| ).catch((err: unknown) => { | |
| logger.error("aws_diagnostics_run_failed", { | |
| connectionId: row.id, | |
| projectId: row.projectId, | |
| errorCode: (err instanceof Error ? err.name : String(err)), | |
| }); | |
| throw err; | |
| }); |
Useful? React with 👍 / 👎.
| status: "warning", | ||
| summary: "This check could not run.", | ||
| evidence: unavailableEvidence, | ||
| }, |
There was a problem hiding this comment.
logs · warning — Add an info log when a diagnostic run completes in runAwsDiagnostics
Emit an info-level structured log after deps.recorder.record(...) with runId, connectionId, projectId, status, and requestedByUserId so operators can reconstruct the audit trail from logs alone without querying the database.
| }, | |
| const saved = await deps.recorder.record({ | |
| connectionId: target.connectionId, | |
| projectId: target.projectId, | |
| region: target.region, | |
| requestedByUserId: target.requestedByUserId, | |
| reason: target.reason, | |
| ...result, | |
| }); | |
| // Log after persist so the runId is available for correlation. | |
| // Import your existing logger at the top of this file. | |
| logger.info("aws_diagnostics_run_recorded", { | |
| runId: saved.id, | |
| connectionId: target.connectionId, | |
| projectId: target.projectId, | |
| status: saved.status, | |
| requestedByUserId: target.requestedByUserId, | |
| }); | |
| return saved; |
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f07cc4cba2
ℹ️ 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".
| - "cloudformation:DescribeStacks" | ||
| Resource: "*" |
There was a problem hiding this comment.
When an unrelated customer stack has a plaintext parameter that is not marked NoEcho, this DescribeStacks grant with Resource: "*" exposes its ParameterValue to the assumed Superlog role. inspectStacks calls DescribeStacks without a StackName and iterates every stack's parameters to locate ConnectionId, so all stack parameters are read before the result is sanitized, and the sensitive-read denies do not block CloudFormation from returning them. Store or discover the integration stack identifier without enumerating every stack, then scope this permission in both templates and the session policy.
Useful? React with 👍 / 👎.
| if (!hasSource) { | ||
| return { | ||
| key: kind, | ||
| label, | ||
| status: "warning", |
There was a problem hiding this comment.
When a customer uses the template's valid EnableMetrics=false or EnableLogs=false configuration, the corresponding source and delivery stream are intentionally absent, but this branch always marks that signal as a warning and makes the whole connection report that it needs attention. Inventory-only and one-signal installations therefore can never receive an accurate healthy result; persist the selected enablement state or otherwise skip checks for signals that were deliberately disabled.
Useful? React with 👍 / 👎.
1 participant
Summary
Why
Connection health previously relied on local ingest-key activity and did not provide an on-demand view into the customer-controlled AWS delivery path. That made regional stack, stream, and Firehose failures difficult to distinguish without asking customers to manually inspect AWS.
Validation
Summary by cubic
Adds audited, customer-triggered AWS telemetry diagnostics to inspect CloudFormation, metric streams, Firehose, and CloudWatch Logs health, with sanitized evidence and a project-scoped history. Batches and paginates AWS calls to reduce throttling, supports custom/renamed/legacy Superlog stacks, and reports the latest Firehose delivery error without storing raw logs.
New Features
/api/projects/:projectId/cloud-connections/:id/diagnostics(run; optionalreason) and GET/api/projects/:projectId/cloud-connections/:id/diagnostics(history).GetMetricData; reads CFN stacks (matched byConnectionId), metric stream state, Firehose status/metrics, CW Logs subscription policy count, and the latest delivery error (code and time); preserves STS identity evidence on probe errors; no credentials or raw logs are persisted.Migration
cloud_connection_diagnostic_runs.superlog-connectand scrape-role templates to grant read‑only diagnostics permissions; re‑launch if needed.@aws-sdk/client-cloudformation,@aws-sdk/client-cloudwatch,@aws-sdk/client-cloudwatch-logs,@aws-sdk/client-firehose.Written for commit f07cc4c. Summary will update on new commits.