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

custom-resources: Custom resource provider framework not passing ResponseURL to user function #21058

Closed
okonos opened this issue Jul 8, 2022 · 18 comments · Fixed by #21065 or #21117
Closed
Assignees
Labels
@aws-cdk/custom-resources Related to AWS CDK Custom Resources bug This issue is a bug. effort/small Small work item – less than a day of effort p0

Comments

@okonos
Copy link

okonos commented Jul 8, 2022

Describe the bug

According to AWS custom resource reference, ResponseURL is a required request field.
The behavior of custom resource provider framework was changed in 2.31.0 to not pass it to user function. It's discussed in #20899.
Without ResponseURL, the provider function fails trying to send the response to CloudFormation.

It works as expected in version 2.30.0.

Expected Behavior

Custom resource should be created without errors.

Current Behavior

Custom resource creation fails with following error (Golang function, the ResponseURL in the event it receives is an empty string):

Received response status [FAILED] from custom resource. Message returned: Error: Put "": unsupported protocol scheme ""

Reproduction Steps

Sample custom resource function from:
https://pkg.go.dev/github.com/aws/aws-lambda-go/cfn#readme-sample-function

import { Stack, StackProps, CustomResource } from 'aws-cdk-lib';
import { Construct } from 'constructs';
import {
    Code,
    Function,
    Runtime,
} from 'aws-cdk-lib/aws-lambda';
import {
    Provider,
} from 'aws-cdk-lib/custom-resources';

export class DeploymentStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const fn = new Function(this, 'MyFunc', {
      runtime: Runtime.GO_1_X,
      memorySize: 256,
      code: Code.fromAsset('build/'),
      handler: 'func',
    });

    const provider = new Provider(this, 'Provider', {
        onEventHandler: fn,
    });

    new CustomResource(this, 'CustomResource', { serviceToken: provider.serviceToken });
  }
}

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.31.0 (build b67950d)

Framework Version

No response

Node.js Version

v16.15.1

OS

Ubuntu

Language

Typescript

Language Version

No response

Other information

No response

@okonos okonos added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 8, 2022
@github-actions github-actions bot added the @aws-cdk/custom-resources Related to AWS CDK Custom Resources label Jul 8, 2022
@comcalvi comcalvi added the p1 label Jul 8, 2022
@peterwoodworth peterwoodworth added effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jul 8, 2022
@comcalvi comcalvi added p0 and removed p1 labels Jul 8, 2022
@mergify mergify bot closed this as completed in #21065 Jul 8, 2022
mergify bot pushed a commit that referenced this issue Jul 8, 2022
… `ResponseURL` to user function (#21065)

#20889 included a change that broke the custom resource framework by not including the necessary presigned URL. This includes the presigned URL, and fixes the issue. This PR does not include test changes because this is the runtime code.

Closes #21058

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Jul 8, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

comcalvi added a commit that referenced this issue Jul 8, 2022
… `ResponseURL` to user function (#21065)

#20889 included a change that broke the custom resource framework by not including the necessary presigned URL. This includes the presigned URL, and fixes the issue. This PR does not include test changes because this is the runtime code.

Closes #21058

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
comcalvi added a commit that referenced this issue Jul 8, 2022
… `ResponseURL` to user function (#21065)

#20889 included a change that broke the custom resource framework by not including the necessary presigned URL. This includes the presigned URL, and fixes the issue. This PR does not include test changes because this is the runtime code.

Closes #21058

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@rix0rrr rix0rrr reopened this Jul 9, 2022
@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 9, 2022

Note that there is a difference between Custom Resources written directly for CloudFormation, and Lambda Functions written for the Custom Resource Provider Framework!

They look similar, but they are different.


When you use the Custom Resource Provider Framework, your Lambda Function is not responsible for writing the response object. The framework does. You only have to make your Lambda function succeed or fail, as usual.

See the documentation:

image

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources-readme.html#handling-lifecycle-events-onevent

The documentation page proceeds to list a set of properties that are in the input. You will see that ResponseURL is not listed in that table.

Yes, we included ResponseURL by accident. That still does not mean you should have taken a dependency on that ResponseURL parameter.

We are going to remove ResponseURL because it can only be misused.

daschaa pushed a commit to daschaa/aws-cdk that referenced this issue Jul 9, 2022
… `ResponseURL` to user function (aws#21065)

aws#20889 included a change that broke the custom resource framework by not including the necessary presigned URL. This includes the presigned URL, and fixes the issue. This PR does not include test changes because this is the runtime code.

Closes aws#21058

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@jnawk
Copy link

jnawk commented Jul 10, 2022

You've taken CloudFormation's own interface, changed it in a breaking way, but actually forgot to make said change, and now you're blaming everyone else for following one set of documentation over another set which until now was entirely redundant.

Anyone who knows how to write a lambda backed custom resource without CDK can fall into this trap. Summarily neutering this functionality is definitely a shark-jumping breaking change.

(In fact, learning about this today explains heaps about mysterious behaviour I didn't have the brain cells to fully investigate previously).

@mrpackethead
Copy link

Exisiting Custom resources are breaking in 2.31.0. This is a BREAKING change to CDK. @rix0rrr , you need to think about how to resolve this in a different way, becuase this BREAKS a lot of code.

@mrpackethead
Copy link

CDK 2.31.0 has a breaking change that prevents Lambda backed Custom resources from working..
@Rico Huijbers
, this is a serious issue that is killing production.
If you've used cfn-reponse in your lambdas this breaks.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/cfn-lambda-function-code-cfnresponsemodule.html

@mrpackethead
Copy link

While i agree in principal that there is a security weakness, its impact is minor to trivial. and compared to the serious impact that this is causing with a breaking change that this needs to be reverted.

Perhaps a property in the custom resource handler that lets you set the behavior not to send the URL? It might in fact be something better set as a flag.

@mrpackethead
Copy link

Ok.. trouble over..; this has been rolled back in 2.31.1 Thanks.. I guess now we can have a chat about how to work through this.

@okonos
Copy link
Author

okonos commented Jul 11, 2022

@rix0rrr Thanks. Having spent a lot of time writing custom resources in the past, I didn't read the docs carefully enough.

The change breaking backwards compatibility is a valid concern, though.

@mrpackethead It has not actually been rolled back, the input is still sanitized in 2.31.1. The only thing that changed is that now the function is being passed ... instead of undefined as the value of ResponseURL.

@mrpackethead
Copy link

@okonos , i have to go and have a look closer.. This might still be a problem for me. if its not returning the URL i've got a lot of Lambdas that are going to break. so, i'll be stuck at 2.30.0

@comcalvi
Copy link
Contributor

@okonos is correct, the ResponseURL is just passed ... instead of undefined. This doesn't actually fix the issue for anyone, and instead it gives the illusion that ResponseURL is available, but it actually isn't. Leaving it as undefined is much clearer. The rollback is being rolled back here: #21109

We will update the docs to clarify this soon.

@jnawk
Copy link

jnawk commented Jul 12, 2022

even that "rollback" doesn't fix the problem. just makes it undefined" again. guess it's time to peg cdk to 2.30.0, since you can't be trusted to fix this properly.

@mrpackethead
Copy link

@rix0rrr and @comcalvi .. This is a breaking change. that is anti the no breaking change promise of CDK2.0.

While i agree that perhaps there is a better way to do this, we need to do this in a staged sensible way. There are thousands of custom lambdas that Just dont' work now with 2.31.0.. Irrispective of the docs being wrong or right.

@mrpackethead
Copy link

https://aws.amazon.com/blogs/developer/announcing-aws-cloud-development-kit-v2-developer-preview/

In AWS CDK v2, we no longer make intentional breaking changes to any APIs in minor version releases. Instead, we’re introducing a new lifecycle in which new, experimental construct libraries go through an incubation period as a library completely independent from the main aws-cdk-lib library. The packages are distributed with a name clearly indicating their experimental status, like @aws-cdk-experiments/aws-xxx, and are versioned with a version number 0.x to clearly indicate their pre-release status. Only after the new package has matured and has been put through its paces in several real-world scenarios will we include it into aws-cdk-lib as a stable module.

This means that every method, class, and property in aws-cdk-lib is guaranteed to exist until the next major version update, and you can update to new minor versions at any time without having to touch your code.

mrgrain added a commit to mrgrain/aws-cdk that referenced this issue Jul 13, 2022
… `ResponseURL` to user function (aws#21065)

aws#20889 included a change that broke the custom resource framework by not including the necessary presigned URL. We attempted to fix this with aws#21065 but it didn't resolve the issue and aws#21109 reverted the attempted fix.

This changes explicitly includes the presigned URL, as well as adding tests to ensure the URL is passed to the downstream Lambda Function.

Closes aws#21058
@mergify mergify bot closed this as completed in #21117 Jul 13, 2022
mergify bot pushed a commit that referenced this issue Jul 13, 2022
… `ResponseURL` to user function (#21117)

#20889 included a change that broke the custom resource framework by not including the necessary presigned URL. We attempted to fix this with #21065 but it didn't resolve the issue and #21109 reverted the attempted fix.

This changes explicitly includes the presigned URL, as well as adding tests to ensure the URL is passed to the downstream Lambda Function.

Closes #21058


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@mrgrain
Copy link
Contributor

mrgrain commented Jul 13, 2022

Apologies for the confusion everyone. We have just merged another fix for this (see #21117) and are expecting to ship it with the next release.

mergify bot pushed a commit that referenced this issue Jul 13, 2022
… `ResponseURL` to user function (#21117)

aws#20889 included a change that broke the custom resource framework by not including the necessary presigned URL. We attempted to fix this with #21065 but it didn't resolve the issue and #21109 reverted the attempted fix.

This changes explicitly includes the presigned URL, as well as adding tests to ensure the URL is passed to the downstream Lambda Function.

Closes #21058

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

(cherry picked from commit ddfca48)

# Conflicts:
#	packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts
kaizencc pushed a commit that referenced this issue Jul 13, 2022
… `ResponseURL` to user function (#21117)

#20889 included a change that broke the custom resource framework by not including the necessary presigned URL. We attempted to fix this with #21065 but it didn't resolve the issue and #21109 reverted the attempted fix.

This changes explicitly includes the presigned URL, as well as adding tests to ensure the URL is passed to the downstream Lambda Function.

Closes #21058

----

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@kaizencc
Copy link
Contributor

Hi everyone, we released CDK versions 2.31.2 and 1.163.2 with #21117 to address this issue. It should now be fixed. Let us know if that works and apologies for the breaking change!

@mrpackethead
Copy link

@kaizencc , that looks like its working in a trivial test case i have. Theres some good ideas here that need exploring though, and it might be possible to improve how we use the custom resources moving forward.

@kaizencc
Copy link
Contributor

@mrpackethead, happy that it looks like we have rolled back any breaking changes with the latest release. We have some internal ideas for improvements here (keeping it vague because I actually wasn't involved in those discussions :)) but I think it would be awesome if you could open a new github issue describing some of the improvements listed in this thread -- this thread is closed, so there's less visibility, and there's a lot of clutter regarding our treatment of breaking changes :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/custom-resources Related to AWS CDK Custom Resources bug This issue is a bug. effort/small Small work item – less than a day of effort p0
Projects
None yet
8 participants