pulumi-cdk icon indicating copy to clipboard operation
pulumi-cdk copied to clipboard

Escape double-colons (::)

Open karakter98 opened this issue 2 years ago • 16 comments

Escape double-colons (::) from AWS custom resource naming convention, so the resource URNs are compatible with Pulumi's format.

Allows users to define their own dynamic providers for custom resources, which partially solves #60.

With these minimal changes, I was able to implement a generic dynamic provider for custom resources, which seems to work fine for S3BucketDeployment and S3AutoDeleteObjects resources. I'll brush it up a bit and will open another PR.

karakter98 avatar Aug 27 '23 21:08 karakter98

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions[bot] avatar Aug 27 '23 21:08 github-actions[bot]

Hi @karakter98. Thanks for the PR! It would be really helpful if you could post an example of what works with this change, but didn't work without it. A quick test would be even better.

iwahbe avatar Aug 28 '23 22:08 iwahbe

/run-acceptance-tests

iwahbe avatar Aug 28 '23 22:08 iwahbe

Please view the PR build - https://github.com/pulumi/pulumi-cdk/actions/runs/6005610699

github-actions[bot] avatar Aug 28 '23 22:08 github-actions[bot]

@iwahbe I tried to write a test, but it seems the pulumi runtime isn't actually called to register the resource, so the URN error never shows up in the test file. I believe it's because of the mocks, but I'm not familiar enough with the codebase to figure it out further.

What we can do now and couldn't do before: register custom resources from CDK that contain :: in their names, like Custom::S3AutoDeleteObjects from the cdk Bucket class.

karakter98 avatar Aug 29 '23 10:08 karakter98

A quick example:

const bucket = new Bucket(this, "Bucket", {
    enforceSSL: true,
    removalPolicy: RemovalPolicy.DESTROY,
    autoDeleteObjects: true,
});

new BucketDeployment(this, "Deployment", {
    sources: [Source.asset(path.join("..", "build", "web"))],
    destinationBucket: bucket,
    retainOnDelete: false,
});

Both the S3AutoDeleteObjects and S3BucketDeployment custom resources created by the above code can't be registered by Pulumi, because of double-colons in the resource names (of the form Custom::S3AutoDeleteObjects or similar). This clashes with Pulumi's URN format when it tries to register the resources.

karakter98 avatar Aug 29 '23 12:08 karakter98

Thanks for the example @karakter98. I have asked someone familiar with pulumi-ckd to take a look at your PR.

iwahbe avatar Aug 29 '23 17:08 iwahbe

@lukehoban Yes, I'm currently using this to deploy the frontend for a new project, so I can confirm it allows the use of custom resources.

The actual complete example is a bit complicated, because it uses:

  • remapCloudControlResource to handle each custom resource separately
  • A dynamic provider that calls the Lambdas behind the custom resources according to the CloudFormation contract (and provides them with S3 presigned URLs so they can upload the execution status, just like with CloudFormation)
  • Changes to the default remapCloudControlResource for Lambda and IAM Role resources, because the generated names when nested into custom constructs get longer than 64 characters, so I had to also limit this length to get custom resources to work

I think it makes sense to split this into multiple PRs, because so much setup is needed on the user side that the test code would be pretty huge.

I'll test out your suggestion with %3A encoding and see if Pulumi accepts that format, thanks for the idea!

karakter98 avatar Aug 29 '23 21:08 karakter98

I think it makes sense to split this into multiple PRs, because so much setup is needed on the user side that the test code would be pretty huge.

Got it. Yeah - I tried out the original example and ran into all those same problems as well. I’ll open some issues to track the remaining things we’ll need to address here. If you already have PRs in progress, great!

lukehoban avatar Aug 29 '23 22:08 lukehoban

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions[bot] avatar Sep 01 '23 07:09 github-actions[bot]

@lukehoban I changed the encoding according to your feedback, can confirm Pulumi registers the names correctly with the %3A (ran pulumi preview on the project I'm working on without errors)

karakter98 avatar Sep 01 '23 07:09 karakter98

/run-acceptance-tests

iwahbe avatar Sep 01 '23 16:09 iwahbe

Please view the PR build - https://github.com/pulumi/pulumi-cdk/actions/runs/6051638879

github-actions[bot] avatar Sep 01 '23 16:09 github-actions[bot]

Is this fine to be merged? I already opened other PRs and am ready to start working on the dynamic provider PR for custom resources, but this needs to be merged first

karakter98 avatar Sep 04 '23 12:09 karakter98

@lukehoban could you take a look when you get some time?

karakter98 avatar Sep 19 '23 09:09 karakter98

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions[bot] avatar Jan 22 '24 07:01 github-actions[bot]