fix(msk): allow both sasl/scram and iam auth #31743

msambol · 2024-10-14T05:09:10Z

Pointed out here and verified in the Console, both SASL/SCRAM and IAM can be enabled together.

It's a little confusing because CloudFormation groups Iam and Scram together under Sasl,
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/SCRAM and IAM cannot be enabled together.

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

msambol · 2024-10-14T05:16:20Z

packages/@aws-cdk/aws-msk-alpha/README.md

@@ -124,7 +124,7 @@ const cluster = new msk.Cluster(this, 'cluster', {
 });
 ```

-### SASL/IAM
+### IAM


Removing SASL here to avoid confusion.

packages/@aws-cdk/aws-msk-alpha/test/__snapshots__/cluster.test.ts.snap

packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts

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

nmussy

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.

packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts

packages/@aws-cdk/aws-msk-alpha/test/cluster.test.ts

packages/@aws-cdk/aws-msk-alpha/test/integ.cluster-authentication.ts

msambol · 2024-10-14T18:22:16Z

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.

Agree on adding that, perhaps in a follow-on PR. I also think Iam should be decoupled from the SaslProps.

packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts

aws-cdk-automation · 2024-12-22T00:12:30Z

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 · 2024-12-22T01:41:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.92%. Comparing base (cf9d9e2) to head (b4c64de).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #31743   +/-   ##
=======================================
  Coverage   80.92%   80.92%           
=======================================
  Files         236      236           
  Lines       14253    14253           
  Branches     2490     2490           
=======================================
  Hits        11534    11534           
  Misses       2434     2434           
  Partials      285      285

Flag	Coverage Δ
suite.unit	`80.92% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`79.73% <ø> (ø)`
packages/aws-cdk-lib/core	`82.20% <ø> (ø)`

aws-cdk-automation · 2025-01-15T00:11:09Z

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.

nmussy

LGTM thanks for the changes 👍

packages/@aws-cdk/aws-msk-alpha/test/__snapshots__/cluster.test.ts.snap

Pull request has been modified.

msambol · 2025-02-03T22:30:58Z

@gracelu0 fixed :)

msambol · 2025-02-07T15:54:41Z

@gracelu0 could we get this in today :)

gracelu0

LGTM, thanks!

mergify · 2025-02-08T01:15:01Z

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).

mergify · 2025-02-08T01:15:07Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

mergify · 2025-02-08T01:19:54Z

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).

Pull request has been modified.

aws-cdk-automation · 2025-02-08T01:53:57Z

AWS CodeBuild CI Report

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

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

mergify · 2025-02-08T01:54:17Z

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 · 2025-02-08T01:54:38Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

aws-cdk-automation requested a review from a team October 14, 2024 05:09

github-actions bot added distinguished-contributor p2 labels Oct 14, 2024

aws-cdk-automation previously requested changes Oct 14, 2024

View reviewed changes

msambol force-pushed the msk-iam branch from 06b733f to fe83b99 Compare October 14, 2024 05:13

msambol commented Oct 14, 2024

View reviewed changes

packages/@aws-cdk/aws-msk-alpha/test/__snapshots__/cluster.test.ts.snap Outdated Show resolved Hide resolved

msambol force-pushed the msk-iam branch from fe83b99 to 2153761 Compare October 14, 2024 05:17

msambol mentioned this pull request Oct 14, 2024

feat(msk): IAM access control for clusters #14647

Merged

msambol commented Oct 14, 2024

View reviewed changes

packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts Outdated Show resolved Hide resolved

msambol changed the title ~~fix(msk): allow both scram and iam auth~~ fix(msk): allow both sasl/scram and iam auth Oct 14, 2024

aws-cdk-automation added the pr/needs-community-review label Oct 14, 2024

nmussy suggested changes Oct 14, 2024

View reviewed changes

packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts Show resolved Hide resolved

packages/@aws-cdk/aws-msk-alpha/test/cluster.test.ts Outdated Show resolved Hide resolved

msambol force-pushed the msk-iam branch from bfb5fc0 to 40cc995 Compare October 14, 2024 17:01

msambol commented Oct 14, 2024

View reviewed changes

packages/@aws-cdk/aws-msk-alpha/test/cluster.test.ts Show resolved Hide resolved

msambol commented Oct 14, 2024

View reviewed changes

packages/@aws-cdk/aws-msk-alpha/test/integ.cluster-authentication.ts Show resolved Hide resolved

aws-cdk-automation removed the pr/needs-community-review label Oct 14, 2024

nmussy reviewed Oct 14, 2024

View reviewed changes

packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Nov 1, 2024

Monthly PR metrics report #31972

Closed

mazyu36 mentioned this pull request Jan 8, 2025

aws-msk-alpha: Cluster enforces incorrect authentication constraint #32779

Closed

1 task

github-actions bot added bug effort/small labels Jan 9, 2025

msambol force-pushed the msk-iam branch from e6d5eef to 8992d7c Compare January 15, 2025 00:34

nmussy approved these changes Jan 16, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review label Jan 16, 2025

gracelu0 previously requested changes Feb 3, 2025

View reviewed changes

packages/@aws-cdk/aws-msk-alpha/test/__snapshots__/cluster.test.ts.snap Outdated Show resolved Hide resolved

msambol added 7 commits February 3, 2025 13:15

fix(msk): allow both scram and iam auth

7bc13cc

Add integration test

f45d3a9

Update integration test, add feedback from Jimmy

77c3701

Fix snapshot

bd3dfca

move declaration

e373c6d

rebase on main

39726e6

Trigger build

db85da3

aws-cdk-automation removed the pr/needs-maintainer-review label Feb 3, 2025

msambol force-pushed the msk-iam branch from 64f81c4 to dd64cfd Compare February 3, 2025 21:19

Remove enabled for tls

5abd4e1

msambol force-pushed the msk-iam branch from dd64cfd to 5abd4e1 Compare February 3, 2025 21:57

aws-cdk-automation added the pr/needs-maintainer-review label Feb 3, 2025

Merge branch 'main' into msk-iam

faf3b56

gracelu0 previously approved these changes Feb 8, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review label Feb 8, 2025

Merge branch 'main' into msk-iam

b4c64de

gracelu0 approved these changes Feb 8, 2025

View reviewed changes

mergify bot merged commit fbcb732 into aws:main Feb 8, 2025
22 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(msk): allow both sasl/scram and iam auth #31743

fix(msk): allow both sasl/scram and iam auth #31743

msambol commented Oct 14, 2024 •

edited by kaizencc

Loading

aws-cdk-automation left a comment

msambol Oct 14, 2024

nmussy left a comment

msambol commented Oct 14, 2024

aws-cdk-automation commented Dec 22, 2024

codecov bot commented Dec 22, 2024 •

edited

Loading

aws-cdk-automation commented Jan 15, 2025

nmussy left a comment

msambol commented Feb 3, 2025

msambol commented Feb 7, 2025

gracelu0 left a comment

mergify bot commented Feb 8, 2025

mergify bot commented Feb 8, 2025

mergify bot commented Feb 8, 2025

aws-cdk-automation commented Feb 8, 2025

mergify bot commented Feb 8, 2025

github-actions bot commented Feb 8, 2025

fix(msk): allow both sasl/scram and iam auth #31743

fix(msk): allow both sasl/scram and iam auth #31743

Conversation

msambol commented Oct 14, 2024 • edited by kaizencc Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

msambol Oct 14, 2024

Choose a reason for hiding this comment

nmussy left a comment

Choose a reason for hiding this comment

msambol commented Oct 14, 2024

aws-cdk-automation commented Dec 22, 2024

codecov bot commented Dec 22, 2024 • edited Loading

Codecov Report

aws-cdk-automation commented Jan 15, 2025

nmussy left a comment

Choose a reason for hiding this comment

msambol commented Feb 3, 2025

msambol commented Feb 7, 2025

gracelu0 left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 8, 2025

mergify bot commented Feb 8, 2025

mergify bot commented Feb 8, 2025

aws-cdk-automation commented Feb 8, 2025

AWS CodeBuild CI Report

mergify bot commented Feb 8, 2025

github-actions bot commented Feb 8, 2025

msambol commented Oct 14, 2024 •

edited by kaizencc

Loading

codecov bot commented Dec 22, 2024 •

edited

Loading