fix(msk): allow both sasl/scram and iam auth #31743
Conversation
github-actions
bot
added
distinguished-contributor
There was a problem hiding this comment.
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.
| @@ -124,7 +124,7 @@ const cluster = new msk.Cluster(this, 'cluster', { | |||
| }); | |||
| ``` | |||
|
|
|||
| ### SASL/IAM | |||
| ### IAM | |||
There was a problem hiding this comment.
Removing SASL here to avoid confusion.
| @@ -422,6 +422,7 @@ exports[`MSK Cluster Snapshot test with all values set 1`] = ` | |||
| "CertificateAuthorityArnList": [ | |||
| "arn:aws:acm-pca:us-west-2:1234567890:certificate-authority/11111111-1111-1111-1111-111111111111", | |||
| ], | |||
| "Enabled": true, | |||
There was a problem hiding this comment.
Enabled was missing previously. See here.
msambol
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-community-review
There was a problem hiding this comment.
LGTM overall, just some missing regression tests and a style suggestion.
This is probably outside the scope of this PR, but it might be a good idea to expose Enabled if a user wants to set the props value but disable the feature. I'm just assuming here, but switching from Enabled: false to Enabled: true might be faster than updating CertificateAuthorityArnList for instance.
| }, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@nmussy I added some additional unit tests
| testCases: [stack], | ||
| }); | ||
|
|
||
| app.synth(); |
There was a problem hiding this comment.
@nmussy I created a new integration test that covers all 3 authentication types. The existing test is costly/timely to deploy 😓
aws-cdk-automation
removed
the
pr/needs-community-review
Agree on adding that, perhaps in a follow-on PR. I also think |
|
This PR has been in the MERGE CONFLICTS 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #31743 +/- ##
=======================================
Coverage 80.52% 80.52%
=======================================
Files 106 106
Lines 6948 6948
Branches 1287 1287
=======================================
Hits 5595 5595
Misses 1175 1175
Partials 178 178
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Pointed out here and verified in the Console, both
SASL/SCRAMandIAMcan be enabled together.It's a little confusing because CloudFormation groups
IamandScramtogether underSasl,but the Console separates the two and allows both at the same time. I'd like to refactor this further but
this change unblocks the issue where
SASL/SCRAMandIAMcannot be enabled together.