-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(apigateway): rest api deployment does not depend on authorizers #23215
Changes from all commits
d0673a3
a632ece
b4d111d
11a2506
908e8a9
4086b08
1b4016d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
import * as cognito from '@aws-cdk/aws-cognito'; | ||
import { Duration, Lazy, Names, Stack } from '@aws-cdk/core'; | ||
import { Duration, FeatureFlags, Lazy, Names, Stack } from '@aws-cdk/core'; | ||
import { APIGATEWAY_AUTHORIZER_CHANGE_DEPLOYMENT_LOGICAL_ID } from '@aws-cdk/cx-api'; | ||
import { Construct } from 'constructs'; | ||
import { CfnAuthorizer } from '../apigateway.generated'; | ||
import { CfnAuthorizer, CfnAuthorizerProps } from '../apigateway.generated'; | ||
import { Authorizer, IAuthorizer } from '../authorizer'; | ||
import { AuthorizationType } from '../method'; | ||
import { IRestApi } from '../restapi'; | ||
|
@@ -64,18 +65,25 @@ export class CognitoUserPoolsAuthorizer extends Authorizer implements IAuthorize | |
|
||
private restApiId?: string; | ||
|
||
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); | ||
|
||
this.authorizerId = resource.ref; | ||
this.authorizerArn = Stack.of(this).formatArn({ | ||
|
@@ -96,6 +104,16 @@ export class CognitoUserPoolsAuthorizer extends Authorizer implements IAuthorize | |
} | ||
|
||
this.restApiId = restApi.restApiId; | ||
|
||
const addToLogicalId = FeatureFlags.of(this).isEnabled(APIGATEWAY_AUTHORIZER_CHANGE_DEPLOYMENT_LOGICAL_ID); | ||
|
||
const deployment = restApi.latestDeployment; | ||
if (deployment && addToLogicalId) { | ||
deployment.node.addDependency(this); | ||
deployment.addToLogicalId({ | ||
authorizer: this.authorizerProps, | ||
}); | ||
Comment on lines
+112
to
+115
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've made a change and added a new test which checks that the deployment changes when the lambda function changes name |
||
} | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import * as iam from '@aws-cdk/aws-iam'; | ||
import * as lambda from '@aws-cdk/aws-lambda'; | ||
import { Arn, ArnFormat, Duration, Lazy, Names, Stack } from '@aws-cdk/core'; | ||
import { Arn, ArnFormat, Duration, FeatureFlags, Lazy, Names, Stack } from '@aws-cdk/core'; | ||
import { APIGATEWAY_AUTHORIZER_CHANGE_DEPLOYMENT_LOGICAL_ID } from '@aws-cdk/cx-api'; | ||
import { Construct } from 'constructs'; | ||
import { CfnAuthorizer } from '../apigateway.generated'; | ||
import { CfnAuthorizer, CfnAuthorizerProps } from '../apigateway.generated'; | ||
import { Authorizer, IAuthorizer } from '../authorizer'; | ||
import { IRestApi } from '../restapi'; | ||
|
||
|
@@ -69,6 +70,8 @@ abstract class LambdaAuthorizer extends Authorizer implements IAuthorizer { | |
|
||
protected restApiId?: string; | ||
|
||
protected abstract readonly authorizerProps: CfnAuthorizerProps; | ||
|
||
protected constructor(scope: Construct, id: string, props: LambdaAuthorizerProps) { | ||
super(scope, id); | ||
|
||
|
@@ -90,6 +93,28 @@ abstract class LambdaAuthorizer extends Authorizer implements IAuthorizer { | |
} | ||
|
||
this.restApiId = restApi.restApiId; | ||
|
||
const deployment = restApi.latestDeployment; | ||
const addToLogicalId = FeatureFlags.of(this).isEnabled(APIGATEWAY_AUTHORIZER_CHANGE_DEPLOYMENT_LOGICAL_ID); | ||
|
||
if (deployment && addToLogicalId) { | ||
let functionName; | ||
|
||
if (this.handler instanceof lambda.Function) { | ||
// if not imported, attempt to get the function name, which | ||
// may be a token | ||
functionName = (this.handler.node.defaultChild as lambda.CfnFunction).functionName; | ||
} else { | ||
// if imported, the function name will be a token | ||
functionName = this.handler.functionName; | ||
} | ||
|
||
deployment.node.addDependency(this); | ||
deployment.addToLogicalId({ | ||
authorizer: this.authorizerProps, | ||
comcalvi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
authorizerToken: functionName, | ||
}); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -163,11 +188,14 @@ export class TokenAuthorizer extends LambdaAuthorizer { | |
|
||
public readonly authorizerArn: string; | ||
|
||
protected readonly authorizerProps: CfnAuthorizerProps; | ||
|
||
constructor(scope: Construct, id: string, props: TokenAuthorizerProps) { | ||
super(scope, id, props); | ||
|
||
const restApiId = this.lazyRestApiId(); | ||
const resource = new CfnAuthorizer(this, 'Resource', { | ||
|
||
const authorizerProps: CfnAuthorizerProps = { | ||
name: props.authorizerName ?? Names.uniqueId(this), | ||
restApiId, | ||
type: 'TOKEN', | ||
|
@@ -176,7 +204,11 @@ export class TokenAuthorizer extends LambdaAuthorizer { | |
authorizerResultTtlInSeconds: props.resultsCacheTtl?.toSeconds(), | ||
identitySource: props.identitySource || 'method.request.header.Authorization', | ||
identityValidationExpression: props.validationRegex, | ||
}); | ||
}; | ||
|
||
this.authorizerProps = authorizerProps; | ||
|
||
const resource = new CfnAuthorizer(this, 'Resource', authorizerProps); | ||
|
||
this.authorizerId = resource.ref; | ||
this.authorizerArn = Stack.of(this).formatArn({ | ||
|
@@ -221,6 +253,8 @@ export class RequestAuthorizer extends LambdaAuthorizer { | |
|
||
public readonly authorizerArn: string; | ||
|
||
protected readonly authorizerProps: CfnAuthorizerProps; | ||
|
||
constructor(scope: Construct, id: string, props: RequestAuthorizerProps) { | ||
super(scope, id, props); | ||
|
||
|
@@ -229,15 +263,20 @@ export class RequestAuthorizer extends LambdaAuthorizer { | |
} | ||
|
||
const restApiId = this.lazyRestApiId(); | ||
const resource = new CfnAuthorizer(this, 'Resource', { | ||
|
||
const authorizerProps: CfnAuthorizerProps = { | ||
name: props.authorizerName ?? Names.uniqueId(this), | ||
restApiId, | ||
type: 'REQUEST', | ||
authorizerUri: lambdaAuthorizerArn(props.handler), | ||
authorizerCredentials: props.assumeRole?.roleArn, | ||
authorizerResultTtlInSeconds: props.resultsCacheTtl?.toSeconds(), | ||
identitySource: props.identitySources.map(is => is.toString()).join(','), | ||
}); | ||
}; | ||
|
||
this.authorizerProps = authorizerProps; | ||
|
||
const resource = new CfnAuthorizer(this, 'Resource', authorizerProps); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we leave this in the more compact form? |
||
|
||
this.authorizerId = resource.ref; | ||
this.authorizerArn = Stack.of(this).formatArn({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
{"version":"20.0.0"} | ||
{"version":"22.0.0"} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,12 @@ | ||
{ | ||
"version": "20.0.0", | ||
"version": "22.0.0", | ||
"testCases": { | ||
"cognito-authorizer/DefaultTest": { | ||
"stacks": [ | ||
"CognitoUserPoolsAuthorizerInteg" | ||
], | ||
"assertionStack": "cognito-authorizer/DefaultTest/DeployAssert" | ||
"assertionStack": "cognito-authorizer/DefaultTest/DeployAssert", | ||
"assertionStackName": "cognitoauthorizerDefaultTestDeployAssert4551574C" | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...why? Can we not leave this as the original more compact form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to save
authorizerProps
to a private instance variable so that we can use it in the_attachToApi
function