awslabs / aws-solutions-constructs

The AWS Solutions Constructs Library is an open-source extension of the AWS Cloud Development Kit (AWS CDK) that provides multi-service, well-architected patterns for quickly defining solutions
https://docs.aws.amazon.com/solutions/latest/constructs/
Apache License 2.0
1.23k stars 246 forks source link

Unable to use existing bucket object from aws_s3.Bucket.from_bucket_name #958

Closed Gamecock closed 1 year ago

Gamecock commented 1 year ago

fails to create stack

Reproduction Steps

    exiting_bucket = s3.Bucket.from_bucket_name(self, 'existing_bucket', bucket_name="existing_bucket")

    s3tolambda = S3ToLambda(self, 'test_s3_lambda',
                        lambda_function_props=_lambda.FunctionProps(
                            code=_lambda.Code.from_asset('_lambda'),
                            runtime=_lambda.Runtime.PYTHON_3_9,
                            handler='index.handler'
                            ),
                        existing_bucket_obj=existing_bucket)

Error Log

TypeError: type of argument existing_bucket_obj must be one of (aws_cdk.aws_s3.Bucket, NoneType); got aws_cdk.aws_s3._BucketBaseProxy instead

Environment

Other

I think this is a documentation error, and error message is misleading. exsting_bucket_object appears to want the L1 construct.

existing_bucket_obj=raw_bucket.node.default_child resolves issue

More elegantly would be for the CDK to accept an IBucket in the constructor.

I can make the first change if desired, the second is beyond my abilities.


This is :bug: Bug Report

Gamecock commented 1 year ago

Even with the modification, the stack still creates a new bucket, and triggers events off the new bucket. Existing bucket object not used.

biffgaut commented 1 year ago

The existing_bucket_obj prop is a Bucket Class, s3.Bucket.from_bucket_name returns an interface, IBucket. The external bucket is outside the stack, so CDK has much less ability to modify it - the IBucket interface reflects these lesser capabilities.

We set up the event source using the CDK S3EventSource class, which requires a Bucket as input (not an IBucket). So this construct cannot work with an IBucket obtained from from_bucket_name().

Gamecock commented 1 year ago

Is there a different way to get the bucket from an existing bucket?

biffgaut commented 1 year ago

There's 2 static methods on Bucket that can get a bucket - by arn and by bucket name - but both return an IBucket. I believe the constraint is that CloudFormation cannot alter a bucket outside of the current stack, and not a CDK limitation, although I can't locate documentation explicitly stating this.

Gamecock commented 1 year ago

Is the right answer to just strip the existing_bucket option from the solution. I can do that.

biffgaut commented 1 year ago

If you drop that, it will create a new S3 bucket - which will be functional if it still supports your use case. Constructs will by default create the bucket encrypted, access logging enabled, versioning enabled and with a lifecycle policy that moves objects to Glacier after 30(I think) days. Those are all best practices, but you can override then with the bucketProperties prop.

Gamecock commented 1 year ago

I meant remove from the pattern, not my instance of it. Since the functionality does not work.

Mike

On Sat, May 20, 2023 at 2:59 PM biffgaut @.***> wrote:

If you drop that, it will create a new S3 bucket - which will be functional if it still supports your use case. Constructs will by default create the bucket encrypted, access logging enabled, versioning enabled and with a lifecycle policy that moves objects to Glacier after 30(I think) days. Those are all best practices, but you can override then with the bucketProperties prop.

— Reply to this email directly, view it on GitHub https://github.com/awslabs/aws-solutions-constructs/issues/958#issuecomment-1555972493, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMSCSUL3R4FU3DVKTONB2LXHEH7ZANCNFSM6AAAAAAXM5VLDA . You are receiving this because you authored the thread.Message ID: @.***>

biffgaut commented 1 year ago

It works if you provide a Bucket object created elsewhere in the same stack. For instance, the following code sets up a Lambda Function that is triggered by events in a bucket created in a separate Construct:

const dataIntake = new LambdaToS3(this, 'LambdaToS3Pattern', {
    lambdaFunctionProps: {
        runtime: lambda.Runtime.NODEJS_14_X,
        handler: 'index.handler',
        code: lambda.Code.fromAsset(`lambda`)
    }
});

new S3ToLambda(this, 'test-s3-lambda', {
  lambdaFunctionProps: {
    code: lambda.Code.fromAsset(`lambda`),
    runtime: lambda.Runtime.NODEJS_14_X,
    handler: 'index.handler'
  },
  existingBucket = dataIntake. s3Bucket
});

Linking together multiple constructs is the primary purpose of the existingObject prop values.

Gamecock commented 1 year ago

Got it, thanks.

On Mon, May 22, 2023, 12:53 PM biffgaut @.***> wrote:

It works if you provide a Bucket object created elsewhere in the same stack. For instance, the following code sets up a Lambda Function that is triggered by events in a bucket created in a separate Construct:

const dataIntake = new LambdaToS3(this, 'LambdaToS3Pattern', { lambdaFunctionProps: { runtime: lambda.Runtime.NODEJS_14_X, handler: 'index.handler', code: lambda.Code.fromAsset(lambda) } });

new S3ToLambda(this, 'test-s3-lambda', { lambdaFunctionProps: { code: lambda.Code.fromAsset(lambda), runtime: lambda.Runtime.NODEJS_14_X, handler: 'index.handler' }, existingBucket = dataIntake. s3Bucket });

Linking together multiple constructs is the primary purpose of the existing Object prop values.

— Reply to this email directly, view it on GitHub https://github.com/awslabs/aws-solutions-constructs/issues/958#issuecomment-1557569437, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMSCSRJHQBFWTG7WUY6LUDXHOKZRANCNFSM6AAAAAAXM5VLDA . You are receiving this because you authored the thread.Message ID: @.***>

Gamecock commented 1 year ago

@biffgaut Do you want me to submit a PR to clarify that in the documentation?

biffgaut commented 1 year ago

What clarification would you add? There are many constructs that accept an object rather than an interface for existing resources.

Gamecock commented 1 year ago

If this is S3 specific that an existing_bucket_object has to be defined in this stack. Then there should be words to that effect here. If in general existing _X_object cannot access an object already deployed in an account I don't know where that should be documented. But if I lost a couple days, I'd like to find some words to keep the next person from having the same problem. But maybe this is just general Mke not reading docs closely enough.

On Tue, Jul 11, 2023 at 4:44 PM biffgaut @.***> wrote:

What clarification would you add? There are many constructs that accept an object rather than an interface for existing resources.

— Reply to this email directly, view it on GitHub https://github.com/awslabs/aws-solutions-constructs/issues/958#issuecomment-1631484609, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMSCSQ6HG4UOOKBXQ2GFHTXPW3LFANCNFSM6AAAAAAXM5VLDA . You are receiving this because you authored the thread.Message ID: @.***>

biffgaut commented 1 year ago

It's a general CDK/CloudFormation trait - CloudFormation can't update resources outside it's stack. Because of this, CDK getExistingResourceByAttribute calls all return an Interface rather than an Object. At one point we started more clearly delineating one from the other by accepting existingObj or existingInterface props - but constructs created before we adopted this naming pattern aren't consistent.