Skip to content

feat(aws): CloudFormation-compatible custom resources, and bucket notifications on imported / shared buckets - #163

Open
so0k wants to merge 4 commits into
mainfrom
feat/cfncompat-custom-resource
Open

feat(aws): CloudFormation-compatible custom resources, and bucket notifications on imported / shared buckets#163
so0k wants to merge 4 commits into
mainfrom
feat/cfncompat-custom-resource

Conversation

@so0k

@so0k so0k commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

feat(aws): CloudFormation-compatible custom resources, and bucket notifications on imported / shared buckets

Summary

Adds a CloudFormation-shaped custom-resource primitive to TerraConstructs, backed by the
cdktn-io/cfncompat Terraform provider, and uses it to lift the long-standing restriction
that S3 bucket notifications can only be managed by the stack that owns the bucket.

Two commits, on top of origin/main:

  1. feat(aws): CustomResource and CustomResourceHandler on @cdktn/provider-cfncompat
    aws.CustomResource (a port of aws-cdk-lib/core/lib/custom-resource.ts, wrapping
    cfncompat_custom_resource) and aws.CustomResourceHandler (a stack-singleton Lambda
    backing one or more custom resources). AwsStack grows a lazy cfncompatProvider
    singleton, an optional cfncompatProviderConfig prop, and a lazy per-stack
    customResourceResponseBucket.
  2. feat(storage): custom-resource bucket notifications for imported and shared buckets
    NotificationsResourceHandler (AWS CDK's Python handler, verbatim) and
    BucketNotificationsResource (Custom::S3BucketNotifications, Managed: "false").
    BucketBase.addEventNotification / enableEventBridgeNotification now pick between the
    native aws_s3_bucket_notification resource and the custom resource.

Imported buckets (Bucket.fromBucketName and friends) always use the custom resource — it is
the only way to attach a notification to a bucket the stack does not own. Owned buckets keep
the native resource unless the @terraconstructs/aws-s3:keepNotificationInImportedBucket
context key is set, matching AWS CDK's feature-flag name.

Docs: new src/aws/storage/README.md ("Notifications on imported / shared buckets"), and the
new integ target documented in integ/aws/storage/README.md.

Design decisions

  • Merge, never overwrite. The custom resource always runs the handler unmanaged
    (Managed: "false"). On every apply the handler reads the bucket's existing notification
    configuration, keeps everything it does not recognise, and merges in only this stack's own
    entries, identified by an Id prefixed with the stack id. That is what lets several stacks
    — the owning stack included — attach notifications to one bucket.
  • stackId = gridUUID. The handler distinguishes its own entries by StackId prefix, so
    the value must be stable across applies. gridUUID is; a construct path is not.
  • Context key, not a FeatureFlags port. Read with node.tryGetContext; jsii has no
    exported constants, so the literal key string is the public API and
    S3_KEEP_NOTIFICATION_IN_IMPORTED_BUCKET in cx-api.ts stays internal (same treatment as
    TARGET_PARTITIONS).
  • Migration hazard, documented not automated. Flipping the key on an already-deployed
    owned bucket destroys the native resource — which wipes the bucket's whole notification
    configuration — unordered against the custom resource's Put. Called out in
    addEventNotification's JSDoc, in cx-api.ts, and in the storage README.
  • Response transport. cfncompat_custom_resource delivers handler responses through a
    pre-signed S3 URL. Each stack lazily creates one CustomResourceResponsesBucket with
    force_destroy (response objects are written at apply time and are not tracked in state, so
    destroy would otherwise fail on a non-empty bucket). Setting
    cfncompatProviderConfig.customResourceBucket opts out and defers to the provider's bucket.
  • Lazy provider and bucket. Stacks that never create a custom resource never synthesize a
    cfncompat provider block or a response bucket. AwsStack requires storage/bucket lazily
    to break the aws-stackbucket import cycle.
  • Handler source inlined. Upstream loads the Python handler from an aws-cdk-lib build
    asset; that pipeline does not exist here, so the source is inlined verbatim (precedent:
    compute/ecs/drain-hook). Upstream's comment-stripping is dropped — it only exists to fit
    CloudFormation's 4 KiB inline ZipFile limit, and Code.fromInline renders a
    data.archive_file.
  • No async provider framework. No onEvent/isComplete state machine: Terraform applies
    are synchronous, so a plain Lambda invoked by cfncompat_custom_resource suffices.

Testing evidence

Unit (pnpm exec jest test/aws/storage test/aws/custom-resource): 18 suites, 375 passed, 2
skipped, 0 failed
. New: test/aws/custom-resource.test.ts (13),
test/aws/custom-resource-handler.test.ts (6),
test/aws/storage/notification-custom-resource.test.ts (15), plus imported-bucket cases in
test/aws/storage/bucket.test.ts.

Also green: pnpm compile, pnpm exec eslint on every touched file, pnpm exec projen (no
drift). The repo sets docgen: false and ships no API.md, so there is no generated
reference to regenerate.

Live AWS run — TestTcons PASS (1195s), account 694710432912 / us-east-1, using
terraconstructs@0.0.0.jsii.tgz built from this branch. Three stacks share one bucket; A owns
it, B and C import it by name:

Stage Result
deploy A → {a} PASS
deploy B → {a,b} PASS (b live after 7 warm-up probes)
deploy C → {a,b,c} PASS
re-deploy A → {a,b,c} PASS — No changes. Your infrastructure matches the configuration.
destroy B → {a,c} PASS
cleanup PASS

Re-applying the owning stack leaving B and C's entries intact is the property the whole change
exists for.

OpenTofu registry limitation. The in-repo integ target
make bucket-notifications-cross-stack (three stacks, entry-set assertions after every stage
plus an end-to-end delivery check per owner) could not be run. Terratest invokes tofu
(hardcoded in integ/aws/util.go) and registry.opentofu.org does not serve
cdktn-io/cfncompat, so init fails with Failed to query available provider packages. The
scenario was therefore validated by the equivalent standalone harness run above. Running the
in-repo target today requires a filesystem_mirror for cdktn-io/cfncompat in the OpenTofu
CLI configuration, or pointing terratest at a terraform binary; this is documented in
integ/aws/storage/README.md.

Follow-ups

  • Publish cdktn-io/cfncompat to the OpenTofu registry (or make the integ helpers' terraform
    binary configurable) so bucket-notifications-cross-stack runs in CI.
  • The imported-bucket handler-role tests upstream depends on machinery not ported here
    (recorded as not ported: lines in notification-custom-resource.test.ts).
  • Snapshot cases are omitted, matching the sibling test/aws/storage/notification.test.ts,
    where they are commented out.
  • Other constructs that need out-of-stack mutation (bucket policies on imported buckets, for
    example) can now reuse aws.CustomResource.

Update — in-repo integ under OpenTofu

cdktn-io/cfncompat is now on registry.opentofu.org (opentofu/registry #5338 + key #5340). make bucket-notifications-cross-stack (tofu, unmodified) → TestBucketNotificationsCrossStack PASS (472s, account 694710432912 / us-east-1, 2026-08-29). The OpenTofu limitation noted above no longer applies.

so0k added 4 commits August 28, 2026 23:10
…r-cfncompat

Ports aws-cdk-lib/core custom-resource.ts (renderResourceType/uppercaseProperties verbatim)
onto cfncompat_custom_resource; AwsStack gains a lazy cfncompat provider singleton and a
per-stack force_destroy response bucket used unless the provider config names one.
CustomResourceHandler is a getOrCreate stack singleton built from iam.Role + LambdaFunction.
…shared buckets

Ports aws-s3 notifications-resource (v2.233.0) onto the cfncompat CustomResource core:
NotificationsResourceHandler (stack singleton running the CDK Python handler verbatim) and
BucketNotificationsResource (Custom::S3BucketNotifications, Managed=false). Imported buckets
always use it; owned buckets keep the native aws_s3_bucket_notification unless the context key
@terraconstructs/aws-s3:keepNotificationInImportedBucket is set. Adds the three-stack
bucket-notifications-cross-stack integ app and TestBucketNotificationsCrossStack.
@so0k

so0k commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

In-repo integ now runs unmodified under OpenTofu: cdktn-io/cfncompat landed on registry.opentofu.org (opentofu/registry#5338, key opentofu/registry#5340). make bucket-notifications-cross-stackTestBucketNotificationsCrossStack PASS (472s, 2026-08-29). Integ README updated in the latest commit.

@sakul-learning sakul-learning left a comment

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.

Review: changes requested

I reviewed head 021f6779dcf86a5bc899e4b9843332a70e9aa485, ran the configured checks, and ran the new cross-stack integration target against AWS account 694710432912 in us-east-1.

Blocking correctness findings

  1. src/aws/storage/notifications-resource-handler.ts:55-93 — an unmanaged Update rewrites notification IDs owned by other stacks.

    get_id(n) clears and replaces n['Id'] in place. The Update path at line 71 calls it for every existing queue, topic, and Lambda notification while deciding which entries are external. A notification owned by stack B can remain in external_notifications, but its dictionary has already been mutated using stack A's stack_id; lines 89-93 then PUT that changed ID back to S3. A later Delete from B searches for the B prefix and can no longer recognize/remove its notification.

    Please derive comparison IDs without mutating the existing notification, and add a handler-level regression that changes A's notification properties, verifies B/C IDs remain byte-for-byte unchanged, then deletes B and verifies only B's entry disappears. The current redeploy_a integration stage reapplies unchanged Terraform and reports No changes, so it never sends the custom resource an Update event and does not cover this path.

  2. src/aws/storage/index.ts:3 exports BucketNotificationsResource despite its internal-only contract.

    bucket-notifications-resource.ts says only one instance may exist for a bucket/stack pair and that the construct is not exported publicly, but the storage barrel exports it. The generated .jsii assembly confirms terraconstructs.aws.storage.BucketNotificationsResource and BucketNotificationsResourceProps as stable public cross-language types. Please remove the public barrel export or deliberately support and document direct construction; retracting an accidentally published JSII API later is a breaking change.

Live integration result and teardown findings

make bucket-notifications-cross-stack ultimately passed with full A/B/C deployment, notification/delivery validation, B removal, A/C revalidation, and teardown:

--- PASS: TestBucketNotificationsCrossStack (427.80s)
PASS
ok github.com/terraconstructs/base/integ/aws/storage 427.836s

The target is valuable standalone/on-demand coverage for real multi-stack notification ownership and delivery, but the run exposed three cleanup/isolation problems:

  • Deterministic fixture identities collide across runs. integ/aws/storage/apps/bucket-notifications-cross-stack.ts:37-44 hard-codes g${owner}12345678-1234, so every run reuses /aws/lambda/{ga,gb,gc}12345678-1234-buc5f094a3db834Handler. Two zero-byte groups left from an earlier run caused the first two attempts to fail with ResourceAlreadyExistsException, even though the bucket and result resources use a random suffix. After explicit cleanup of those stale groups and manual destruction of partial state, the third run passed. The handler stack identity should be unique per test run while remaining stable across that run.

  • An early failure can prevent cleanup of resources that did deploy. storage_test.go:95-105 unconditionally starts deferred teardown with C. util.UndeployUsingTerraform fatally loads C's saved TerraformOptions; if deployment failed before C was synthesized/deployed, the missing c/.test-data/TerraformOptions.json aborts cleanup before B and A. Each stack cleanup should be conditional on saved state/options, and one absent/failing stack should not suppress cleanup of the others.

  • A successful destroy can still leave the custom-resource handler log group. The final run's generated Terraform has the correct dependency graph: the custom resource depends on the handler Lambda and its explicit log group. The actual stack-C destroy sequence was also correctly ordered: custom resource Delete completed at 03:55:37Z, Lambda deletion at 03:55:43Z, and log-group deletion at 03:55:45Z. Nevertheless, after OpenTofu reported empty A/B/C states, AWS contained a new zero-byte /aws/lambda/gc12345678-1234-buc5f094a3db834Handler with creationTime=2026-08-29T03:55:45.350Z. I removed it manually and verified the test bucket, Lambdas, queues, response buckets, IAM roles, fixed handler log groups, and all three OpenTofu states were empty.

    I do not think the evidence shows a missing dependency or a straightforward cfncompat implementation error. cfncompat v0.2.0 invokes Lambda with InvocationTypeEvent, then waits for the handler's S3 response. AWS documents that CloudFormation also invokes custom-resource Lambdas asynchronously. The likely race is that the handler sends its success response before Lambda's delayed runtime logging is quiescent; Terraform then deletes the explicitly managed log group, while AWSLambdaBasicExecutionRole still permits Lambda to recreate /aws/lambda/<function-name>. AWS notes that Lambda log delivery can lag by minutes: https://docs.aws.amazon.com/lambda/latest/dg/monitoring-cloudwatchlogs.html.

    This is still a product/test cleanup defect worth addressing here: prevent the explicitly managed group from being recreated (for example, avoid granting logs:CreateLogGroup when the group is pre-created, with appropriately scoped stream/write permissions), or otherwise make teardown tolerate and verify this asynchronous logging lifecycle. The integration target should assert that the fixed handler groups are absent after teardown so this cannot silently poison the next run.

Checks

  • pnpm compile — PASS (0 errors; existing JSII warnings only)
  • pnpm eslint — PASS
  • make bucket-notifications-cross-stack — PASS in 427.80s after removing stale prior-run groups
  • GitHub CI — green at this head
  • git diff --check origin/main...HEAD — reports trailing whitespace at src/aws/storage/notifications-resource-handler.ts:71

Ponytail artifact-value pass: the real-AWS integration scenario earns its maintenance cost, but it currently misses the handler Update regression and needs the isolation/cleanup hardening above to remain repeatable.

@sakul-learning

Copy link
Copy Markdown
Contributor

Review correction and refocus

After comparing src/aws/storage/notifications-resource-handler.ts directly with AWS CDK v2.233.0, I want to correct and refocus my earlier review.

The get_id behavior is inherited from AWS CDK

The implementation at notifications-resource-handler.ts:55-71, including get_id() mutating its input by setting n['Id'] = '', is copied verbatim from @aws-cdk/custom-resource-handlers v2.233.0. I should not have presented this as logic newly designed by this PR.

I also need to correct the precise mechanism stated in my first review: during unmanaged Update filtering, get_id(existingNotification) leaves the retained object with Id: ""; it does not assign the current stack's prefix to that retained object. Because that same object is later included in the S3 PUT payload, ownership information can still be lost, but this is an inherited upstream behavior. It remains worth a focused regression because this PR relies heavily on unmanaged, multi-stack operation, but it should not be the primary PR-specific architectural finding.

Primary concern: a new exported handler abstraction without the AWS CDK provider contract

src/aws/custom-resource-handler.ts introduces CustomResourceHandler as a generic stack-singleton Lambda abstraction and src/aws/index.ts:11 exports it. The generated JSII assembly therefore publishes terraconstructs.aws.CustomResourceHandler and CustomResourceHandlerProps as stable cross-language API.

AWS CDK does not expose an equivalent generic CustomResourceHandler. Its S3 notification implementation uses a service-specific internal singleton Lambda with an inline handler: notifications-resource-handler.ts. The local InLineLambda exists largely because the aws-s3 module cannot depend on the Lambda L2 without a dependency cycle; it is not the AWS CDK custom-resource provider abstraction.

AWS CDK's public abstraction for building providers is custom-resources.Provider, whose contract includes framework-owned response handling, handler return validation, physical-ID defaults/replacement behavior, error-to-FAILED responses, and optional asynchronous onEvent/isComplete stabilization. Its documentation recommends that framework unless there is a deliberate reason to use the raw protocol: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib-readme.html#the-custom-resource-provider-framework.

This PR instead publishes a class that provisions a Lambda and calls it a custom-resource handler, while explicitly omitting the Provider contract. That is an API and layering commitment beyond what the S3 port requires. Before merge, I would either:

  1. keep the S3 NotificationsResourceHandler and any Lambda-construction helper private, exporting only the low-level CustomResource primitive; or
  2. split out and first design a public provider abstraction with an intentional compatibility contract, rather than later evolving an already-published JSII CustomResourceHandler into something different.

The same applies to the service-specific BucketNotificationsResource: its source says it is internal, but src/aws/storage/index.ts currently exports it as stable JSII API.

Custom-resource compatibility needs a broader lifecycle matrix

The live test demonstrates that basic direct-Lambda Create/Delete and cross-stack notification behavior works, but it also exposed lifecycle behavior that unit synthesis tests do not cover:

  • A successful destroy left a zero-byte custom-resource handler log group even though the generated dependency graph and actual destroy ordering were correct. The custom resource completed Delete, then Terraform deleted the Lambda and explicit log group, but delayed Lambda logging recreated /aws/lambda/gc12345678-1234-buc5f094a3db834Handler. Two stale deterministic handler groups then caused subsequent creates to fail with ResourceAlreadyExistsException.
  • An early deployment failure before stack C saved TerraformOptions.json caused deferred cleanup to abort at C and skip already-deployed B/A resources.
  • The integration test's unchanged redeploy_a produced No changes, so it did not exercise a custom-resource Update, rollback Update, or replacement cleanup.

The log-group race is not evidence of a missing Terraform dependency: the graph and observed order were correct. It is evidence that the combination of asynchronous custom-resource invocation, response completion, Lambda logging, and Terraform-owned handler infrastructure needs a defined and tested teardown contract before these capabilities are exposed as general-purpose public APIs.

I think the foundational cfncompat custom-resource work needs more time and a documented compatibility/test matrix covering the implementation choices and trade-offs described by AWS:

  • direct Lambda-backed and SNS-backed service tokens (CloudFormation accepts Lambda or SNS; SQS is not itself a service token, though SNS can fan out to queue-backed processing);
  • raw protocol handlers that own the presigned-URL response versus framework-managed handlers that return/throw;
  • synchronous onEvent and asynchronous onEvent + isComplete stabilization;
  • Create failure and partial cleanup, failed Update rollback, Delete failure, and replacement when PhysicalResourceId changes;
  • Data/NoEcho, response-size and timeout behavior, idempotency/retries, duplicate delivery, and old property formats;
  • response transport/networking requirements, including access to the response S3 endpoint;
  • lifecycle and retention of framework Lambdas, roles, log groups, response objects/buckets, and any waiter resources.

A cleaner stack would be:

1. cfncompat provider plumbing + low-level CustomResource, with lifecycle conformance tests
2a. private direct-handler S3 notifications implementation
2b. separate public Provider-framework implementation when its contract is ready

Given the new stable JSII surface and the observed teardown race, my requested-changes verdict remains, but the main reason is now premature public abstraction and insufficiently demonstrated custom-resource lifecycle robustness, not that this PR independently invented the inherited get_id implementation.

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.

2 participants