fix(apigateway): rest api deployment does not depend on authorizers #23215

cecheta · 2022-12-03T00:28:10Z

Closes #16554 (formerly #22808)

The Rest API deployment needs to depend on all authorizers attached to the API, so there is a new deployment if any of the authorizers change. This is similar to what is already done for Methods. Includes trivial change to integ test.

Note - Because this will change the logical ID of existing deployments, this is technically a breaking change, so I am not sure if it requires a feature flag.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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

gitpod-io · 2022-12-03T00:28:19Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

comcalvi

I'm not sure this PR actually solves the underlying issue. I'm going to see how this pairs with the reproduction code posted by the issue opener, once I can successfully reproduce.

comcalvi · 2022-12-13T17:26:35Z

packages/@aws-cdk/aws-apigateway/lib/authorizers/cognito.ts

+ private readonly authorizerProps: CfnAuthorizerProps;
+
 constructor(scope: Construct, id: string, props: CognitoUserPoolsAuthorizerProps) {
 super(scope, id);

 const restApiId = this.lazyRestApiId();
- const resource = new CfnAuthorizer(this, 'Resource', {
+
+ const authorizerProps = {
 name: props.authorizerName ?? Names.uniqueId(this),
 restApiId,
 type: 'COGNITO_USER_POOLS',
 providerArns: props.cognitoUserPools.map(userPool => userPool.userPoolArn),
 authorizerResultTtlInSeconds: props.resultsCacheTtl?.toSeconds(),
 identitySource: props.identitySource || 'method.request.header.Authorization',
- });
+ };
+
+ this.authorizerProps = authorizerProps;
+
+ const resource = new CfnAuthorizer(this, 'Resource', authorizerProps);


...why? Can we not leave this as the original more compact form?

Need to save authorizerProps to a private instance variable so that we can use it in the _attachToApi function

comcalvi · 2022-12-13T19:00:40Z

packages/@aws-cdk/aws-apigateway/lib/authorizers/cognito.ts

+ deployment.node.addDependency(this);
+ deployment.addToLogicalId({
+ authorizer: this.authorizerProps,
+ });


As I understand it, the dependency just ensures resource deployment order, and it won't force an update to another resource. We really need a test to ensure that whenever the lambda function changes, the construct that needs to be updated is actually updated.

addToLogicalId() ensures that whenever any of the authoriser props change, the deployment will get a new logical ID and a new deployment will be created

addDependency() ensures that any changes to the authorizer are made before creating the new deployment

I've made a change and added a new test which checks that the deployment changes when the lambda function changes name

comcalvi · 2022-12-13T19:01:18Z

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

+
+ this.authorizerProps = authorizerProps;
+
+ const resource = new CfnAuthorizer(this, 'Resource', authorizerProps);


can we leave this in the more compact form?

aws-cdk-automation · 2023-01-04T00:08:14Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

Pull request has been modified.

comcalvi

Please add an integration test that verifies that addToLogicalId() is correctly used.

comcalvi · 2023-01-30T22:09:26Z

packages/@aws-cdk/aws-apigateway/test/authorizers/integ.request-authorizer.lit.ts

@@ -24,6 +24,7 @@ const restapi = new RestApi(stack, 'MyRestApi', { cloudWatchRole: true });
 const authorizer = new RequestAuthorizer(stack, 'MyAuthorizer', {
 handler: authorizerFn,
 identitySources: [IdentitySource.header('Authorization'), IdentitySource.queryString('allow')],
+ resultsCacheTtl: Duration.minutes(5),


why? This doesn't add anything to this integration test.

I edited this test to re-run this integration test and verify that addToLogicalId() worked correctly, however I will instead create a separate test

I think this is a breaking change so I also tried to add a feature flag

Pull request has been modified.

comcalvi

You are correct, this needs a feature flag. I think you've added it correctly.

comcalvi · 2023-02-06T19:24:57Z

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

+ if (!Token.isUnresolved(functionName)) {
+ authorizerToken = JSON.stringify({ functionName });
+ }


You can pass tokens to addToLogicalId(), why do we check for them here?

I have copied the existing logic from here

I am guessing it is because the token can change which could cause the logical ID to change, even though the function name hadn't changed

The other logic did not use this method, and addToLogicalId() has a particular way of dealing with tokens. Please remove this token check and just pass it through to addToLogicalId().

I think the other logic I linked to is used in the addToLogicalId() call here, however I have reworked the new code to handle unresolved tokens

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

aws-cdk-automation · 2023-02-11T00:08:17Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-02-12T00:09:25Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Pull request has been modified.

comcalvi · 2023-02-13T23:20:47Z

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

+ if (!Token.isUnresolved(functionName)) {
+ authorizerToken = JSON.stringify({ functionName });
+ }


The other logic did not use this method, and addToLogicalId() has a particular way of dealing with tokens. Please remove this token check and just pass it through to addToLogicalId().

comcalvi · 2023-02-13T23:22:27Z

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

@@ -90,6 +93,34 @@ abstract class LambdaAuthorizer extends Authorizer implements IAuthorizer {
 }

 this.restApiId = restApi.restApiId;
+
+ let functionName;


is there a reason to do any of this if the feature flag is disabled?

Pull request has been modified.

comcalvi

Thanks for the fix!

mergify · 2023-02-15T18:10:38Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-02-15T18:51:01Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 1b4016d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-02-15T18:51:14Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions bot added bug This issue is a bug. p2 labels Dec 3, 2022

aws-cdk-automation requested a review from a team December 3, 2022 00:28

github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Dec 3, 2022

cecheta changed the title ~~fix(apigateway): rest api does not depend on authorizers~~ fix(apigateway): rest api deployment does not depend on authorizers Dec 3, 2022

aws-cdk-automation previously requested changes Dec 3, 2022

View reviewed changes

comcalvi previously requested changes Dec 13, 2022

View reviewed changes

comcalvi self-assigned this Dec 13, 2022

github-actions bot added the effort/small Small work item – less than a day of effort label Dec 21, 2022

comcalvi previously requested changes Jan 30, 2023

View reviewed changes

comcalvi previously requested changes Feb 6, 2023

View reviewed changes

comcalvi previously requested changes Feb 13, 2023

View reviewed changes

cecheta added 6 commits February 14, 2023 23:42

fix(apigateway): rest api deployment does not depend on authorizers

d0673a3

Add authorizerToken to deployment

a632ece

Add tests

b4d111d

Update tests

11a2506

Add feature flag

908e8a9

Rework logic

4086b08

comcalvi approved these changes Feb 15, 2023

View reviewed changes

Merge branch 'main' into authorizer

1b4016d

mergify bot merged commit 12e13c1 into aws:main Feb 15, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apigateway): rest api deployment does not depend on authorizers #23215

fix(apigateway): rest api deployment does not depend on authorizers #23215

cecheta commented Dec 3, 2022 •

edited by peterwoodworth

Loading

gitpod-io bot commented Dec 3, 2022

aws-cdk-automation left a comment

comcalvi left a comment

comcalvi Dec 13, 2022

cecheta Jan 4, 2023

comcalvi Dec 13, 2022

cecheta Jan 4, 2023

cecheta Jan 5, 2023

comcalvi Dec 13, 2022

aws-cdk-automation commented Jan 4, 2023

comcalvi left a comment

comcalvi Jan 30, 2023

cecheta Feb 5, 2023

cecheta Feb 5, 2023

comcalvi left a comment

comcalvi Feb 6, 2023

cecheta Feb 12, 2023

comcalvi Feb 13, 2023

cecheta Feb 15, 2023 •

edited

Loading

aws-cdk-automation commented Feb 11, 2023

aws-cdk-automation commented Feb 12, 2023

comcalvi Feb 13, 2023

comcalvi Feb 13, 2023

cecheta Feb 15, 2023

comcalvi left a comment

mergify bot commented Feb 15, 2023

aws-cdk-automation commented Feb 15, 2023

mergify bot commented Feb 15, 2023


		this.authorizerProps = authorizerProps;

		const resource = new CfnAuthorizer(this, 'Resource', authorizerProps);

fix(apigateway): rest api deployment does not depend on authorizers #23215

fix(apigateway): rest api deployment does not depend on authorizers #23215

Conversation

cecheta commented Dec 3, 2022 • edited by peterwoodworth Loading

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Dec 3, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

comcalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 4, 2023

comcalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comcalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecheta Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 11, 2023

aws-cdk-automation commented Feb 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comcalvi left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 15, 2023

aws-cdk-automation commented Feb 15, 2023

AWS CodeBuild CI Report

mergify bot commented Feb 15, 2023

cecheta commented Dec 3, 2022 •

edited by peterwoodworth

Loading

cecheta Feb 15, 2023 •

edited

Loading