feat(core): logRetention + throttling + accessLogging stack defaults (Infrastructure Options) - #385
Conversation
🦋 Changeset detectedLatest commit: ead10cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
sarayev
left a comment
sarayev
left a comment
There was a problem hiding this comment.
Reviewed as a senior engineer — focused on correctness, edge cases, security, scaling, and tests (skipping pure style). Overall a clean, well-documented change with good synth coverage. Inline notes below: 3 majors worth resolving before merge (Logger retention clobber, the production throttle default as a silent behavior change, and the per-stack ApiGateway::Account singleton risk), plus a few minors/nits. None are hard blockers on their own — flagging for discussion.
| // asks for a specific value; otherwise leave the stack-wide default in | ||
| // place. Applied via the L1 escape hatch because a per-block option must | ||
| // reconfigure the framework-owned group, not spawn a competing one. | ||
| const retention = options?.retention ?? this.defaults.logRetention; |
There was a problem hiding this comment.
[major] Every Logger reconfigures the same shared group, so this unconditional write makes the last-constructed Logger win. A bare new Logger(scope, 'x') computes defaults.logRetention and overwrites a retention that another Logger set earlier — silently and order-dependent. Could we write only when the caller explicitly asked, so a no-op Logger doesn't stomp an explicit override?
if (options?.retention) {
const cfnLogGroup = this.handlerLogGroup.node.defaultChild as CfnLogGroup | undefined;
if (cfnLogGroup) cfnLogGroup.retentionInDays = options.retention;
}The shared group already carries defaults.logRetention from setupBlocksInfra, so writing the default back is redundant anyway.
There was a problem hiding this comment.
Good catch — fixed in 784657c. Logger now writes retention only when options.retention is explicitly set; a bare new Logger(...) no longer writes defaults.logRetention back, so it can't clobber an explicit value from another Logger (and the redundant default write is gone). Added a test that pins this: an explicit retention: 14 survives a later bare Logger.
| // place. Applied via the L1 escape hatch because a per-block option must | ||
| // reconfigure the framework-owned group, not spawn a competing one. | ||
| const retention = options?.retention ?? this.defaults.logRetention; | ||
| const cfnLogGroup = this.handlerLogGroup.node.defaultChild as CfnLogGroup | undefined; |
There was a problem hiding this comment.
[minor] If defaultChild is ever undefined (e.g. the group resolves to an imported ILogGroup), the retention request is silently dropped and you'd end up with unexpectedly-infinite retention and no signal. It's always a concrete LogGroup today, so this is defensive — but would an else that throws at synth be safer than a silent skip?
There was a problem hiding this comment.
Fixed in 784657c. When retention is explicitly requested but the shared group's defaultChild isn't a concrete LogGroup (e.g. an imported ILogGroup), it now throws an actionable synth error rather than silently dropping the request. (In the no-retention path there's nothing to write, so no throw.)
| removalPolicy: RemovalPolicy.RETAIN, | ||
| deletionProtection: true, | ||
| logRetention: RetentionDays.ONE_YEAR, | ||
| throttling: DEFAULT_THROTTLING, |
There was a problem hiding this comment.
[major] This applies a 200 req/s (400 burst) cap to production by default. Before this PR the REST stage had no stage-level throttle, so existing deployments ran at the account default (~10k rps). On upgrade, anything serving >200 rps would start getting 429s with no change on their side. Is 200 intentional for production, or should prod get a higher ceiling? Either way, since it can throttle live traffic, could we call this out as a behavior change in the changeset (it currently reads as a routine minor)?
There was a problem hiding this comment.
Addressed in 784657c. Production now defaults to 1000/2000 (sandbox stays 200/400) so the new stage cap doesn't 429 real prod traffic, and it's overridable per-stack via throttling. I also called this out explicitly as a behavior change in the changeset (before: no stage throttle → account default ~10k rps) with upgrade guidance.
| ], | ||
| }); | ||
|
|
||
| const account = new CfnAccount(stack, 'BlocksApiGatewayAccount', { |
There was a problem hiding this comment.
[major] AWS::ApiGateway::Account is a region singleton, but this emits one per stack. The comment notes "last deploy wins", but the teardown case is sharper: if two Blocks stacks in the same account+region both enable access logging and the one that deployed last is later destroyed, its BlocksApiGatewayCloudWatchRole is deleted and the account setting can end up pointing at a role that no longer exists — silently breaking logging (or blocking CreateStage) for the surviving stack. Should the account be importable/shared (fromExisting), or at least documented as a single-Blocks-stack-per-region assumption?
There was a problem hiding this comment.
Documented in 784657c. ensureApiGatewayAccount() now spells out the region-singleton teardown hazard you describe (later stack's role deleted → account setting points at a dead role → breaks logging / blocks CreateStage for the survivor) and states the single-Blocks-stack-per-region assumption for accessLogging. A shared/importable account for true multi-stack support is deferred; noted that multi-stack users should manage the account role out-of-band. Also flagged in the changeset.
| // retention follows the stack-wide default instead of AWS's infinite default, | ||
| // and gives bb-logger one group to reconfigure rather than a second, colliding | ||
| // one. Torn down with the stack (logs are not durable state). | ||
| const handlerLogGroup = new logs.LogGroup(scope, 'HandlerLogGroup', { |
There was a problem hiding this comment.
[minor] On an existing deployment the handler previously logged to Lambda's auto-created /aws/lambda/<fn> group (infinite retention). Pointing the function at this new CDK group leaves the old one orphaned in CloudWatch — unmanaged and still infinite, i.e. the exact cost leak this PR targets. Worth a migration note in the changeset? (The old bb-logger group also gets DESTROY'd on upgrade, dropping its history.)
There was a problem hiding this comment.
Good point — added an upgrade/migration note to the changeset in 784657c: on upgrade the handler's old auto-created /aws/lambda/<fn> group (and any prior bb-logger retention group) is left orphaned/unmanaged in CloudWatch and should be deleted manually to reclaim the cost/close the infinite-retention leak.
| apiGatewayAccount = ensureApiGatewayAccount(cdk.Stack.of(scope)); | ||
| accessLogGroup = new logs.LogGroup(scope, 'ApiAccessLogs', { | ||
| retention: props.defaults.logRetention, | ||
| removalPolicy: cdk.RemovalPolicy.DESTROY, |
There was a problem hiding this comment.
[minor] In production these access logs are the audit trail, but the group is RemovalPolicy.DESTROY, so they vanish on stack teardown. Consistent with treating logs as disposable — but is that the intent for a production audit log? Might warrant RETAIN for the access-log group specifically.
There was a problem hiding this comment.
Changed in 784657c. The access-log groups (core REST + realtime WS) now use removalPolicy: defaults.removalPolicy, so production RETAINs the audit trail on teardown while sandbox still DESTROYs. The handler's operational stdout group stays DESTROY (not an audit trail). Reflected in the changeset.
| * the stack-wide `defaults.logRetention`; when omitted the log group uses the | ||
| * CDK `LogGroup` default retention. | ||
| */ | ||
| logRetention?: cdk.aws_logs.RetentionDays; |
There was a problem hiding this comment.
[nit] materialize is exported, and when logRetention is omitted the group falls back to CDK's default (TWO_YEARS), not the Blocks default. Database always passes it so this is fine today — just flagging the escape-hatch path for a direct caller.
There was a problem hiding this comment.
Acknowledged — Database always passes logRetention from defaults, so the escape-hatch (TWO_YEARS) path is only reachable by a direct materialize caller. The option's JSDoc already notes it falls back to the CDK LogGroup default when omitted, so a direct caller is on notice. Left as-is.
| timeout: cdk.Duration.minutes(MIGRATION_LAMBDA_TIMEOUT_MINUTES), | ||
| // Own the migration Lambda's log group so its retention follows the | ||
| // stack-wide default instead of AWS's infinite retention. | ||
| logGroup: new LogGroup(stack, `${this.fullId}DsqlMigrationLogs`, { |
There was a problem hiding this comment.
[minor] Every other adopter got a CDK retention test (bb-data, bb-distributed-table, bb-app-setting, bb-logger, realtime, core) — this DSQL migration group is the one change without one. A one-liner mirroring the bb-data test would close the parity gap:
template.hasResourceProperties('AWS::Logs::LogGroup', { RetentionInDays: 7 });There was a problem hiding this comment.
Added in 784657c — a parity test in index.cdk.test.ts asserting the DSQL migration Lambda's log group adopts defaults.logRetention (RetentionInDays: 7 under sandbox), matching the other adopters.
|
Thanks @sarayev — addressed the review in Majors
Minors/nits
|
… adopt in core + data BBs + realtime Extends BlocksDefaults (PR #302) with three additive fields, each read independently via `option ?? scope.defaults.field` (never derived from another): - logRetention: RetentionDays — sandbox ONE_WEEK / production ONE_YEAR - throttling: { rateLimit, burstLimit } — 200/400 in both presets - accessLogging: boolean — sandbox false / production true Adoption (hosting is intentionally deferred to a separate PR): logRetention — Blocks-managed log groups now follow the default instead of AWS's infinite retention: - core shared handler Lambda (setupBlocksInfra owns the group, exposed as scope.handlerLogGroup) - bb-logger reconfigures that shared group (option ?? default) instead of creating a colliding /aws/lambda/<fn> group - bb-distributed-table GSI-manager Lambdas - bb-distributed-data DSQL migration Lambda throttling — applied to every Blocks API Gateway stage: - core REST API stage (supersedes #163 and #326) - bb-realtime WebSocket stage (throttle unit is messages/sec across the connection) accessLogging — when enabled, structured JSON access logs to a dedicated CloudWatch log group (retention = defaults.logRetention, RemovalPolicy.DESTROY) on the core REST API and bb-realtime WebSocket stages. Provisions the account-level API Gateway CloudWatch Logs role once per stack via a shared ensureApiGatewayAccount() helper (Symbol.for-keyed so co-located stages reuse one AWS::ApiGateway::Account); RestApi auto-cloudWatchRole disabled to avoid a duplicate. Synth-tested; needs a clean-sandbox deploy to confirm log delivery before merge. Adds synth tests (aws-cdk-lib/assertions) for all sites + preset assertions.
- bb-logger DESIGN/README/types: shared handler log group is reconfigured (option ?? default), no second /aws/lambda group. - bb-realtime DESIGN: WebSocket stage throttling (messages/sec unit) + access logging + shared account-level CloudWatch role. - Changeset covering core (minor), blocks (minor), and the adopting BBs (patch). Hosting adoption is deferred to a separate PR.
…/init Lambdas Two more Blocks-managed Lambdas defaulted to AWS's infinite log retention: - bb-data: the Aurora migration Lambda (direct analog of bb-distributed-data) - bb-app-setting: the secret-init custom-resource Lambda Both now own explicit log groups whose retention follows defaults.logRetention (RemovalPolicy.DESTROY), with synth tests. The wrapped custom-resources.Provider framework Lambdas still use AWS defaults — the L2 Provider exposes no override.
…og retain, account-singleton docs, DSQL test - Production throttle default raised to 1000/2000 (sandbox stays 200/400) so the new stage cap doesn't 429 real prod traffic; documented as a behavior change. - bb-logger only writes retention when an explicit per-Logger `retention` is set (a bare Logger no longer clobbers another's value); throws if the shared group isn't a concrete LogGroup instead of silently dropping the request. - Access-log groups (core REST + realtime WS) follow defaults.removalPolicy, so production RETAINs the audit trail on teardown. - ensureApiGatewayAccount: documented the region-singleton teardown hazard and the single-Blocks-stack-per-region assumption. - Added the missing DSQL migration-Lambda log-retention synth test (parity). - Changeset: throttle values + upgrade/behavior notes (throttle 429 risk, orphaned legacy handler log group, access-log RETAIN).
2 participants
Summary
Extends the stack-wide
BlocksDefaultsmechanism (#302) with three additive fields and adopts them across Blocks-managed infrastructure. Each field is read independently viaoption ?? scope.defaults.field— a per-block option always wins, and no field is derived from another (a #302 reviewer blocker).New
BlocksDefaultsfields (and bothBlocksPresets):logRetentionRetentionDaysONE_WEEKONE_YEARthrottling{ rateLimit; burstLimit }200 / 400200 / 400accessLoggingbooleanfalsetrueAlso newly exported from
@aws-blocks/core/cdk: theBlocksThrottlingtype andensureApiGatewayAccount();Scopegains ahandlerLogGroupgetter.🔎 For API-BR review
Please sanity-check the three property names/shapes before this is widely adopted:
logRetention: RetentionDays(CDK enum — appears only in the/cdktype)throttling: { rateLimit: number; burstLimit: number }(newBlocksThrottlingtype)accessLogging: booleanAdoption sites
Log retention — Blocks-managed log groups now follow
defaults.logRetentioninstead of AWS's infinite default:Core shared handler Lambda —
setupBlocksInfranow owns a single log group, exposed asscope.handlerLogGroup.bb-logger— reconfigures that shared group's retention (retention ?? defaults.logRetention) instead of creating a colliding/aws/lambda/<fn>group.bb-distributed-table— GSI-manager Lambdas.bb-distributed-data— DSQL migration Lambda.bb-data— Aurora migration Lambda.bb-app-setting— secret-init custom-resource Lambda.(The framework
custom-resources.ProviderLambdas these BBs wrap still use AWS's default retention — the L2Providerexposes no log-group override.)Throttling — applied to the core REST API stage and the
bb-realtimeWebSocket stage (on a WS stage the throttle unit is messages/sec across the connection; noted inbb-realtime/DESIGN.md).Access logging — when enabled, structured JSON access logs to a dedicated CloudWatch log group (retention =
defaults.logRetention,RemovalPolicy.DESTROY) on the core REST API andbb-realtimeWebSocket stages. The account-level API Gateway CloudWatch Logs role is provisioned once per stack via a sharedensureApiGatewayAccount()helper (Symbol.for-keyed so co-located stages reuse oneAWS::ApiGateway::Account); the RestApi's autocloudWatchRoleis disabled to avoid a duplicate. Each access-logging stage takes an explicit dependency on the account so a clean-account first deploy applies the setting before the stage is created.#163 / #326 reconciliation
Both #163 and #326 add throttling to the core REST API directly (near-duplicates). This PR is the single source of truth for API Gateway throttling and supersedes them.
Deferred to a follow-up PR
secretsStore,pointInTimeRecovery, VPC, compute, sandbox template cleanup.Testing
npm run build && npm run lint && npm run lint:deps && npm testall green (3709 pass / 0 fail).aws-cdk-lib/assertions) cover: preset values; sandbox vs production retention on the handler group; 200/400 throttle on the REST and WS stages; per-block/per-stack overrides winning; access-log group + singleAWS::ApiGateway::Accountpresent when enabled and absent when disabled; GSI log-group retention.Notes for reviewers
check:apiproduces no diff — api-extractor tracks the mock/types entry, not the/cdkentry where these types live.Sandbox deploy verification (done)
Deployed
test-apps/comprehensiveto a sandbox (account 444209914584, us-west-2) withaccessLoggingenabled, exercising the core REST API,bb-realtimeWebSocket,bb-data,bb-distributed-data/-table, andbb-app-setting. Verified end-to-end, then torn down (DELETE_COMPLETE):CreateStagefailure. OurBlocksApiGatewayAccount+ CloudWatch role were created andapigateway get-accountpointed at our stack's role (explicit stage→account dependency ordered it correctly).POST /aws-blocks/apirequests (HTTP 200) produced structured JSON entries in theApiAccessLogsCloudWatch group (requestId/httpMethod/resourcePath/status…).accessLogSettings→ JSON format +ApiAccessLogsgroup; throttle 200/400 confirmed live.AccessLogSettings→BlocksRtAccessLogsgroup; throttle 200/400.Caveat observed: the test account already had an API Gateway CloudWatch role (from a prior hosting deploy), so this wasn't a truly clean account — our stack provisioned and re-pointed the account role (the documented region-global "last-deploy-wins" behavior), but the clean-account
CreateStagefailure itself couldn't be reproduced here.