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

Jump to bottom

msk: CDK doesn't allow a cluster with multiple authentication methods #16980

Closed
nlongton opened this issue Oct 14, 2021 · 7 comments
Closed

msk: CDK doesn't allow a cluster with multiple authentication methods #16980

nlongton opened this issue Oct 14, 2021 · 7 comments
Labels
@aws-cdk/aws-msk Related to Amazon Managed Streaming for Apache Kafka (Amazon MSK) effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@nlongton
Copy link

General Issue

We want to use IAM auth but this is only for Java so we also need TLS for python etc.

The Question

We want to use IAM auth but this is only for Java so we also need TLS for python etc.
When i use the AWS Console to create a MSK cluster I can select none, TLS and IAM and get multiple connection string. eg:

TLS:
b-1..$$$$.kafka.us-west-2.amazonaws.com:9094,b-2..$$$$.kafka.us-west-2.amazonaws.com:9094
IAM:
b-1..$$$$.kafka.us-west-2.amazonaws.com:9098,b-2..$$$$.kafka.us-west-2.amazonaws.com:9098
Plaintext:
b-1..$$$$.kafka.us-west-2.amazonaws.com:9092,b-2..$$$$.kafka.us-west-2.amazonaws.com:9092

However the logic in the cluster.ts prevents creating a dual TLS and IAM credential properties class, and even if it did then this logic only acts on one type

let clientAuthentication;
if (props.clientAuthentication?.saslProps?.iam) {
  clientAuthentication = {
    sasl: { iam: { enabled: props.clientAuthentication.saslProps.iam } },
  };
} else if (props.clientAuthentication?.saslProps?.scram) {
  clientAuthentication = {
    sasl: {
      scram: {
        enabled: props.clientAuthentication.saslProps.scram,
      },
    },
  };
} else if (
  props.clientAuthentication?.tlsProps?.certificateAuthorities !== undefined
) {
  clientAuthentication = {
    tls: {
      certificateAuthorityArnList: props.clientAuthentication?.tlsProps?.certificateAuthorities.map(
        (ca) => ca.certificateAuthorityArn,
      ),
    },
  };
}

Is this constraint imposed by the SDK - havent had time to look there - and how does the console manage it?

CDK CLI Version

1.123.0

Framework Version

1.127.0

Node.js Version

14.17.0

OS

any

Language

Typescript

Language Version

3.9.7

Other information

No response
@nlongton nlongton added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Oct 14, 2021
@github-actions github-actions bot added the @aws-cdk/aws-msk Related to Amazon Managed Streaming for Apache Kafka (Amazon MSK) label Oct 14, 2021
@ldecaro
Copy link

ldecaro commented Oct 15, 2021

I wonder whether this is related to old codebase. According to this release, MSK recently added the ability to handle a combination of authentication modes.

@otaviomacedo
Copy link
Contributor

I wonder whether this is related to old codebase. According to this release, MSK recently added the ability to handle a combination of authentication modes.

Yes, I believe that's the issue. I'm marking this as a p2 feature request. We welcome contributions from the community. If you can submit with a PR to resolve this, I'll be happy to review it.

@otaviomacedo otaviomacedo added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 and removed guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Oct 15, 2021
@otaviomacedo otaviomacedo removed their assignment Oct 15, 2021
@nlongton
Copy link
Author

nlongton commented Oct 15, 2021 via email

@otaviomacedo
Copy link
Contributor

I’ll see If I can work out how to do it – would you want this in v1 or v2 of the CDK?

All PRs should be created against master. Both v1 and v2 are generated from there.

@dberry-trip
Copy link

any update on when this is going to be fixed?

the workaround of setting up one auth in the cdk, and then turning on the another auth in the console doesn't really work in an IAC (infrastructure as code) context where you're not allowed to use the console in production, hence the reliance on cdk.

fix in cdk v2 please.

@gmuslia
Copy link
Contributor

gmuslia commented Sep 14, 2022

@otaviomacedo I have added this initial PR to address this feature request: #22041 Looking forward to push this feature into CDK.

@gmuslia gmuslia mentioned this issue Sep 14, 2022
Merged
4 tasks
mergify bot pushed a commit that referenced this issue Sep 15, 2022
@gmuslia
Allow multiple clientAuthentication methods in MSK (IAM + TLS) (#22041)
77ea83c 
This PR adds a method (`saslTls `) to have both IAM And TLS for the ClientAuthentication given that this is already supported since October 2021 as feature in the Console as well as in the CloudFormation level. It addresses this issue: #16980 

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@stalbot15 stalbot15 mentioned this issue Nov 2, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
@aws-cdk/aws-msk Related to Amazon Managed Streaming for Apache Kafka (Amazon MSK) effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@otaviomacedo @ldecaro @nlongton @dberry-trip @gmuslia