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.

Sign up for GitHub

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(redshift-alpha): use same role for database-query singleton function #32363

Merged
merged 13 commits into from
Jan 3, 2025
Merged

fix(redshift-alpha): use same role for database-query singleton function #32363

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
daece0a
fix(redshift-alpha): extract tableName from custom resource functions
5d Dec 10, 2024
2500815
fix unit test cases
5d Dec 11, 2024
a998017
revert some integ file changes
5d Dec 11, 2024
8f89f02
fix(redshift-alpha): use same role for database-query singleton function
5d Dec 12, 2024
06e8851
add additional integ test case
5d Dec 12, 2024
4331a07
update integ tests
5d Dec 12, 2024
8723099
Merge branch 'main' into 5d/redshift-iam-role
5d Dec 13, 2024
e89b32b
Merge branch 'main' into 5d/redshift-iam-role
5d Dec 13, 2024
6c0f908
Merge branch 'main' into 5d/redshift-iam-role
5d Jan 2, 2025
13b536f
add more unit test cases
5d Jan 3, 2025
af94e56
Merge remote-tracking branch 'origin/main' into 5d/redshift-iam-role
5d Jan 3, 2025
03a3946
update integ tests
5d Jan 3, 2025
751a303
remove auto generated feature flag file
5d Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/@aws-cdk/aws-redshift-alpha/lib/private/database-query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export class DatabaseQuery<HandlerProps> extends Construct implements iam.IGrant

const provider = new customresources.Provider(this, 'Provider', {
onEventHandler: handler,
role: this.getProviderRole(handler),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use handler.role (so everything about the singleton lambda is nicely scoped inside the SingletonFunction instance and you don't need to add the constructName)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handler in this case is the singleton lambda function. However, the role is not associated to the handler. Instead, the role is assumed by the custom resource provider, which is responsible for triggering the handler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! Thank you.

});

const queryHandlerProps: DatabaseQueryHandlerProps & HandlerProps = {
Expand Down Expand Up @@ -116,4 +117,19 @@ export class DatabaseQuery<HandlerProps> extends Construct implements iam.IGrant
}
return adminUser;
}

/**
* Get or create the IAM role for the singleton lambda function.
* We only need one function since it's just acting as a trigger.
* */
private getProviderRole(handler: lambda.SingletonFunction): iam.IRole {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. nit: Since the method returns either an existing role or creates a new role, i think we can rename the method name to be getOrCreateLambdaProviderRole for better readability.
  2. Can we have a unit test for this method to get an existing role and create a new role?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will update the function name. I will add the unit test case for it.

const id = handler.constructName + 'ProviderRole';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could get the function name like below

const id = handler.functionName + 'ProviderRole';

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

const existing = cdk.Stack.of(this).node.tryFindChild(id);
return existing != null
? existing as iam.Role
: new iam.Role(cdk.Stack.of(this), id, {
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole')],
});
}
}
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-redshift-alpha/lib/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ abstract class UserBase extends Construct implements IUser {
...this.databaseProps,
user: this,
});

this.privileges.node.addDependency(table);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for my understanding, why do we need this to be add as dependency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've encountered errors during stack deletion when the table is removed before its associated privileges are deleted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding the reason as a code comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will add a comment to it.

}

this.privileges.addPrivileges(table, ...actions);
Expand Down
Binary file removed ...e.js.snapshot/.cache/746da84b10e215c552e68b6d2061024e4429f0386f43a35ef5e4d2940655692e.zip
View file
Open in desktop
Binary file not shown.
4 changes: 2 additions & 2 deletions ...-alpha/test/integ.cluster-distkey.js.snapshot/aws-cdk-redshift-distkey-create.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

160 changes: 80 additions & 80 deletions ...lpha/test/integ.cluster-distkey.js.snapshot/aws-cdk-redshift-distkey-create.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,83 +604,6 @@
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"TableProviderframeworkonEventServiceRoleC3128F67": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "lambda.amazonaws.com"
}
}
],
"Version": "2012-10-17"
},
"ManagedPolicyArns": [
{
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
]
]
}
]
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"TableProviderframeworkonEventServiceRoleDefaultPolicyAD08715D": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyDocument": {
"Statement": [
{
"Action": "lambda:InvokeFunction",
"Effect": "Allow",
"Resource": [
{
"Fn::GetAtt": [
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5f3DF81997",
"Arn"
]
},
{
"Fn::Join": [
"",
[
{
"Fn::GetAtt": [
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5f3DF81997",
"Arn"
]
},
":*"
]
]
}
]
}
],
"Version": "2012-10-17"
},
"PolicyName": "TableProviderframeworkonEventServiceRoleDefaultPolicyAD08715D",
"Roles": [
{
"Ref": "TableProviderframeworkonEventServiceRoleC3128F67"
}
]
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"TableProviderframeworkonEvent97F3951A": {
"Type": "AWS::Lambda::Function",
"Properties": {
Expand All @@ -704,7 +627,7 @@
"Handler": "framework.onEvent",
"Role": {
"Fn::GetAtt": [
"TableProviderframeworkonEventServiceRoleC3128F67",
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5fProviderRoleB78E4FFA",
"Arn"
]
},
Expand All @@ -720,8 +643,8 @@
"Timeout": 900
},
"DependsOn": [
"TableProviderframeworkonEventServiceRoleDefaultPolicyAD08715D",
"TableProviderframeworkonEventServiceRoleC3128F67"
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5fProviderRoleDefaultPolicy8C45E853",
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5fProviderRoleB78E4FFA"
],
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
Expand Down Expand Up @@ -865,6 +788,83 @@
],
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5fProviderRoleB78E4FFA": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "lambda.amazonaws.com"
}
}
],
"Version": "2012-10-17"
},
"ManagedPolicyArns": [
{
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
]
]
}
]
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5fProviderRoleDefaultPolicy8C45E853": {
"Type": "AWS::IAM::Policy",
"Properties": {
"PolicyDocument": {
"Statement": [
{
"Action": "lambda:InvokeFunction",
"Effect": "Allow",
"Resource": [
{
"Fn::GetAtt": [
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5f3DF81997",
"Arn"
]
},
{
"Fn::Join": [
"",
[
{
"Fn::GetAtt": [
"QueryRedshiftDatabase3de5bea727da479686625efb56431b5f3DF81997",
"Arn"
]
},
":*"
]
]
}
]
}
],
"Version": "2012-10-17"
},
"PolicyName": "QueryRedshiftDatabase3de5bea727da479686625efb56431b5fProviderRoleDefaultPolicy8C45E853",
"Roles": [
{
"Ref": "QueryRedshiftDatabase3de5bea727da479686625efb56431b5fProviderRoleB78E4FFA"
}
]
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
}
},
"Mappings": {
Expand Down
4 changes: 2 additions & 2 deletions ...-alpha/test/integ.cluster-distkey.js.snapshot/aws-cdk-redshift-distkey-update.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading