-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(rds): Kerberos authentication support in Aurora Database Clusters #28559
Conversation
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.
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.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
4087725
to
2d83f35
Compare
2d83f35
to
681d1d4
Compare
681d1d4
to
bd37ee1
Compare
da04a78
to
9e10cc9
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
this.domainRole = props.domainRole ?? new iam.Role(this, 'RDSClusterDirectoryServiceRole', { | ||
assumedBy: new iam.CompositePrincipal( | ||
new iam.ServicePrincipal('rds.amazonaws.com'), | ||
new iam.ServicePrincipal('directoryservice.rds.amazonaws.com'), |
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.
In the implementation of the instance, directoryservice.rds.amazonaws.com
is not set as a principal, which is a bug and causes the deployment to fail.
I will address this in a separate issue(#28600).
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.
Nice catch!
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.
Thanks for your contribution. I made some comments about README.
new iam.ServicePrincipal('directoryservice.rds.amazonaws.com'), | ||
), | ||
managedPolicies: [ | ||
iam.ManagedPolicy.fromManagedPolicyArn(this, 'RdsRole', 'arn:aws:iam::aws:policy/service-role/AmazonRDSDirectoryServiceAccess'), |
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 not use fromAwsManagedPolicyName
?
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 was I using the fromManagerdPolicyArn
? 😂
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.
Let's fix the same in an integ test and an unit test so that contributors who see these codes will not misunderstand!
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.
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.
I was using it elsewhere too.. Thanks for pointing that out. I've made the correction!
Co-authored-by: k.goto <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto <24818752+go-to-k@users.noreply.github.com>
@go-to-k Thank you for your review! I've addressed your comments. |
Could you update snapshots for the integ test? |
@go-to-k |
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.
Looks good.
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.
Left some comments
/** | ||
* The Active Directory directory ID, required for setting up Kerberos authentication, to create the DB cluster in. | ||
* | ||
* @default - Do not join domain |
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.
This feels a bit vague to me but it could just be that I'm missing some context. Is this saying that the cluster will be created but just not within a domain (or within the Active Directory)?
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.
I understand, you're right; the cluster will be created but not associated with any domain. I've revised the comment section for clarity. Could you please check it again?
(I have limited English skills, so I would appreciate it if you could appropriately adjust the text for me.)
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.
The new description is a lot clearer to me now! And no worries at all, I really appreciate that you do your best to address the description changes!
* The IAM role to be used when making API calls to the Directory Service. The role needs the AWS-managed policy | ||
* AmazonRDSDirectoryServiceAccess or equivalent. | ||
* | ||
* @default - The role will be created for you if `DatabaseClusterBaseProps#domain` is specified |
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.
Would be nice to know what permissions are granted with the default role. Also is DatabaseClusterBaseProps#domain
supposed to be DatabaseClusterBaseProps.domain
?
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.
I've updated the sentence. Please confirm it.
this.domainRole = props.domainRole ?? new iam.Role(this, 'RDSClusterDirectoryServiceRole', { | ||
assumedBy: new iam.CompositePrincipal( | ||
new iam.ServicePrincipal('rds.amazonaws.com'), | ||
new iam.ServicePrincipal('directoryservice.rds.amazonaws.com'), |
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.
Nice catch!
Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>
Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>
Pull request has been modified.
Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>
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.
Thank you for updating the descriptions! They're a lot clearer to me now.
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.
Great work on this @badmintoncryer!
And thank you for reviewing @go-to-k!
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
I have added the arguments domain and domainRole to support Kerberos authentication for the Aurora Database cluster. The specifications for these arguments are the same as the existing domain and domainRole in the Instance.
Closes #28050.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license