Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use FnJoin in bucketName property #624

Closed
xdodocodex opened this issue Aug 24, 2018 · 2 comments · Fixed by #628
Closed

use FnJoin in bucketName property #624

xdodocodex opened this issue Aug 24, 2018 · 2 comments · Fixed by #628
Labels
bug This issue is a bug.

Comments

@xdodocodex
Copy link

Hi everyone,
I am using a cdk.Parameter object to define the stage of my CloudFormation stack and I would like to use this object to define a bucket name, doing so I'll have a bucket related to production, dev, test, etc...

In order to achieve this result I am doing as follow:

new Bucket(this, this.name.toLowerCase(), {
            bucketName: new FnJoin("", [this.name.toLowerCase(), "-", DefaultParameters.getParameter(this.parent, DefaultParameters.PARAMETERS.stage)]).toString(),
            versioned: true
        });

where

DefaultParameters.getParameter(this.parent, DefaultParameters.PARAMETERS.stage)

return a cdk.Parameter object.

When I run

cdk synth --output tmp

I receive this error:

/Users/dodo/Desktop/Dev/Code/Projects/pvideosmart/pvideosmart-aws-infrastructure/node_modules/@aws-cdk/aws-s3/lib/util.js:9
        throw new Error('Bucket name contains invalid characters: ' + bucketName);
        ^

Error: Bucket name contains invalid characters: ${Token[TOKEN.5]}
    at Object.validateBucketName (/Users/dodo/Desktop/Dev/Code/Projects/pvideosmart/pvideosmart-aws-infrastructure/node_modules/@aws-cdk/aws-s3/lib/util.js:9:15)
    at new Bucket (/Users/dodo/Desktop/Dev/Code/Projects/pvideosmart/pvideosmart-aws-infrastructure/node_modules/@aws-cdk/aws-s3/lib/bucket.js:172:16)
    at new TestBucket (/Users/dodo/Desktop/Dev/Code/Projects/pvideosmart/pvideosmart-aws-infrastructure/build/lib/s3/testBucket.js:10:23)
    at new MainStack (/Users/dodo/Desktop/Dev/Code/Projects/pvideosmart/pvideosmart-aws-infrastructure/build/stacks/main_stack.js:23:25)
    at new MyApp (/Users/dodo/Desktop/Dev/Code/Projects/pvideosmart/pvideosmart-aws-infrastructure/build/index.js:22:9)
    at Object.<anonymous> (/Users/dodo/Desktop/Dev/Code/Projects/pvideosmart/pvideosmart-aws-infrastructure/build/index.js:25:22)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)

I have tried to comment the following line from @aws-cdk/aws-s3/lib/bucket.js:

util_1.validateBucketName(props && props.bucketName);

Doing so I obtain the desired result in the output template which is:

pvideosmartweb94A59098:
        Type: 'AWS::S3::Bucket'
        Properties:
            BucketName:
                'Fn::Join':
                    - ""
                    -
                        - pvideo-smart-web
                        - '-'
                        -
                            Ref: stage
            VersioningConfiguration:
                Status: Enabled

Am I missing something or there is a problem in the s3 bucket's name verifcation process?

Thank you.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 24, 2018

You are correct. The S3 bucket name validation was not written with the use of Tokens in mind.

We wanted to validate that you weren't making any mistakes against this, but the fact is that with Tokens we cannot actually check this (since the value may be unavailable until deployment time), and now that Tokens may be embedded into strings it doesn't actually make any sense to validate anymore.

The only thing to do for now is get rid of that validation.

@rix0rrr rix0rrr added the bug This issue is a bug. label Aug 24, 2018
@xdodocodex
Copy link
Author

I agree with you since there are other objects that have no name validation like roles and IMHO at the moment is more useful being able to create referenced name than having the validation in place since in any case failing it will result in a CloudFormation error during the deployment.

Of course having both the validation and the token would be the best, but for me getting rid of the name validation on buckets is enough for now.

Thank you for the replay, and keep up the good work!

rix0rrr pushed a commit that referenced this issue Aug 27, 2018
Now that Tokens can be stringified, they can be inserted into parameters
like `bucketName`, which can lead to an input string like
`"bucket-name-${Token[TOKEN.0]}"`. We can't properly validate these
strings anymore.

Fixes #624.
rix0rrr added a commit that referenced this issue Sep 3, 2018
Now that Tokens can be stringified, they can be inserted into parameters
like `bucketName`, which can lead to an input string like
`"bucket-name-${Token[TOKEN.0]}"`. We can't properly validate these
strings anymore.

Fixes #624.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants