feat(lambda-event-sources): add `rootCACertificate` to `SelfManagedKafkaEventSource` by WtfJoke · Pull Request #21422 · aws/aws-cdk

WtfJoke · 2022-08-02T18:01:50Z

Co-authored-by: @abks90 alexander.backes@codecentric.de
Co-authored-by: @ccoltx 59687842+ccoltx@users.noreply.github.com

Description

In AWS its possible to configure a self hosted Kafka as an eventsource for a Lambda, described here.

Optional its possible to choose to reference the root certificate (CA) secret in the field encryption (see below screenshot 4.h)).

However in CDK in the SelfManagedKafkaEventSourceProps of SelfManagedKafkaEventSource its not possible to specify that encryption secret. Its only possible to specify a secret for the also optional authentication field.

This PR add the possibly to specify the encryption secret to reference a root ca for self signed certificates in SelfManagedKafkaEventSource.

Screenshot of docs explaining how to set secret for root ca (field encryption)

Screenshot of aws console, where the field encryption is visible while adding a self hosted kafka as lambda eventsource trigger

Technical Approach

We started by adding a test (extending the kafka.test.ts) which allows to pass an optional property encryption of type secret.
In case this property is set, we add it to the sourceAccessConfiguration in the AWS::Lambda::EventSourceMapping.

As a small bonus we also added the missing static variable for SERVER_ROOT_CA_CERTIFICATE in the class SourceAccessConfigurationType with the value taken from this documented list of possible values

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test? No, as there was no integration test there, but we extended the regular unit tests.
- 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

gitpod-io · 2022-08-02T18:01:57Z

property to SelfManagedKafkaEventSource Co-authored-by: abks90 <alexander.backes@codecentric.de> Co-authored-by: ccoltx <59687842+ccoltx@users.noreply.github.com>

TheRealAmazonKendra

Before we can provide a meaningful review we need the following:

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)
Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests)
Please add integration tests for this change. PRs with changes that are not statically guaranteed by the type system must have tests to make sure that (a) the new behavior works as intended and (b) it is not accidentally undone by future changes (See Contributing Guide, Work Your Magic).

WtfJoke · 2022-08-02T22:47:13Z

Hey @TheRealAmazonKendra

Thank you for taking a first look at our PR.
I tried my best to update the PR description with a meaningful description, the motivation and explaination of the technical approach.

Regarding integration tests:
We couldnt figure out how to write an integration test and there was no existing integration test.
I've made an experiment in another branch, which you can take a look here,
But as I dont have a kafka running, the integration test will fail, when trying it to deploy using the IntegTest Construct. If I dont add the IntegrationTest Construct yarn test will fail.

I would appreciate if you could guide me here in the right direction.

mrgrain · 2022-08-03T10:16:30Z

Thanks for giving this a go @WtfJoke ! We have specific docs for integration tests, but it looks like you've found them already.

Regarding the issue of the the failing integration test. With your branch I'm getting the following error. Is that the one you see as well? How come we need a custom Kafka cluster running to deploy this?

❌  lambda-event-source-kafka-self-managed failed: Error: The stack named lambda-event-source-kafka-self-managed failed to deploy: CREATE_FAILED (The following resource(s) failed to create: [FKafkaEventSource838c4d5ff3c99c1a617120adfca83e5bmytesttopic1E7A7798]. ): Resource handler returned message: "Invalid request provided: The secret value with ARN arn:aws:secretsmanager:us-east-1:123345678901:secret:SC0855C491-zF9t2Um9DSTH-LufM8z does not have the expected format of: {"certificate": "someCertificate", "privateKey": "somePrivateKey", (OPTIONAL) "privateKeyPassword": "somePrivateKeyPassword"}

Pull request has been modified.

WtfJoke · 2022-08-03T17:55:36Z

@mrgrain Thanks! I havent seen the error when I did that yesterday 🙈 . So I was assuming that part of the deployment (eg. creation of the trigger) would try to connect to the kafka broker as well. But luckily that isnt the case and it was just that the secrets werent filled.

Now that the secrets have the required content the integration test worked :)

TheRealAmazonKendra · 2022-08-04T00:24:17Z

I need clarification on a few things because I think our contract here is kind of rough and I'm having trouble understanding some of it. Full disclosure, I've never used Kafka so this is just going from the documentation, not my own user experience.

For Kafka self-managed clusters, it either seem like you pick a VPC (with subnet and security groups) OR you pick an AuthenticationMethod (which we have an enum of), not both. Is that correct? Or can you have both?
It also seems that you can only have one AuthenticationMethod, not multiple. Is that correct?

If the answer to both is yes, I'm think this takes the wrong approach. I would think we should add SERVER_ROOT_CA_CERTIFICATE as a value within AuthenticationMethod and then just use the value in secret for the uri of the encryption and add validation to say if AuthenticationMethod.SERVER_ROOT_CA_CERTIFICATE then encryption is mandatory or we throw an error?

mrgrain

@TheRealAmazonKendra How I understand it is that the root CA certificate in encryption is used in addition to any of the authentication methods [1]. Furthermore SourceAccessConfigurationType.SERVER_ROOT_CA_CERTIFICATE is coming in as a requirement from the AWS::Lambda::EventSourceMapping SourceAccessConfiguration definition.

If my understanding is correct, I'm basically happy with the current approach. There's a few things I'm not sure about:

Is encryption the right name for this property if what it represents is a SERVER_ROOT_CA_CERTIFICATE?
It seems like encryption is used in the UI and SERVER_ROOT_CA_CERTIFICATE is used in Cfn and the APIs. I'm not decided yet which conveys the meaning better. Currently I lean towards something like rootCaCertificate. But keen to hear other opinions and see prior art in the AWS CDK.
Can we fix the existing fuzzy contract about when authenticationMethod secret and encryption are used or not? At least the docstrings should be improved.
We don't seem to support CLIENT_CERTIFICATE_TLS_AUTH at the moment at all. This might be better as a follow up PR though.

[1] Technically as per the docs it doesn't apply to all authentication methods, but to the following list: "This setting applies to TLS encryption for SASL/SCRAM or SASL/PLAIN, and to mTLS authentication." That's all of them unless we're in a VPC. I guess we could proactively check if authentication is in the list and throw otherwise. However our existing contract is slightly off at the moment already. We say authenticationMethod is optional and defaults to AuthenticationMethod.SASL_SCRAM_512_AUTH. But in reality it's only used if secret is set.

WtfJoke · 2022-08-04T17:58:20Z

How I understand it is that the root CA certificate in encryption is used in addition to any of the authentication methods [1]. Furthermore SourceAccessConfigurationType.SERVER_ROOT_CA_CERTIFICATE is coming in as a requirement from the AWS::Lambda::EventSourceMapping SourceAccessConfiguration definition.

Exactly this. At least thats also my understanding and suits to my limited testing.

Full disclosure, I've never used Kafka so this is just going from the documentation, not my own user experience.

Appreciate the transparency :) I use Kafka, but im not really familiar with all the possible authentication methods.

Is encryption the right name for this property if what it represents is a SERVER_ROOT_CA_CERTIFICATE?
It seems like encryption is used in the UI and SERVER_ROOT_CA_CERTIFICATE is used in Cfn and the APIs. I'm not decided yet which conveys the meaning better. Currently I lean towards something like rootCaCertificate. But keen to hear other opinions and see prior art in the AWS CDK.

Yeah I'll agree here. encryption is not really a good name. I've choosen it simply for the fact that its called like that in the UI and it was already confusing enough that the authentication methods secret is simply called secret (which is a bit misleading imho).

TLDR: My personal favorite name currently is rootCA or rootCACertificate.

Can we fix the existing fuzzy contract about when authenticationMethod secret and encryption are used or not? At least the docstrings should be improved.

Can you please elaborate a bit more on that @mrgrain?
Do you mean the contract between authenticationMethod + secret? In my opinion this should be an object instead, consisting of method and secret. But this would result in some sort of breaking change.

We don't seem to support CLIENT_CERTIFICATE_TLS_AUTH at the moment at all. This might be better as a follow up PR though.

In our company we use exactly this combination. CLIENT_CERTIFICATE_TLS_AUTH + rootCA and it works (using the workaround by using eventsourcemapping instead of selfhostedkafkaprops).

But in reality it's only used if secret is set.

Yes. Also secret is required if you arent in a vpc.

TheRealAmazonKendra · 2022-08-04T19:17:01Z

Oof. I kind of hate their contract for this (not your implementation, this does the best it can with a bad contract). I'll leave the rest of the reviewing of this one to @mrgrain since it sounds like he has a much better understanding of this than me. Thanks for all the clarifications on this.

What I will say, however, is that if you have an idea for a better overall contract and it's breaking change, put it under a feature flag and we'll gladly take a look at it. Personally, I'd prefer to create a better user experience that requires a feature flag than the kind of wonky contract required for updating this without breaking changes. If you and @mrgrain disagree, however, I'll defer to your judgement.

Pull request has been modified.

mrgrain · 2022-08-04T19:51:50Z

Let's got with rootCACertificate

Can you please elaborate a bit more on that @mrgrain?
Do you mean the contract between authenticationMethod + secret? In my opinion this should be an object instead, consisting of method and secret. But this would result in some sort of breaking change.

Yes that's exactly what I meant.

Personally, I'd prefer to create a better user experience that requires a feature flag than the kind of wonky contract required for updating this without breaking changes.

I do as well, but I also like small single purpose PRs. I vote we get this one over the line. If you or anyone else would like to tackle the wonky contract and/or CLIENT_CERTIFICATE_TLS_AUTH in separate PRs, we will definitely look at them though.

mrgrain · 2022-08-05T11:36:19Z

@WtfJoke Looks like something messed up the tests. Once that's resolved I think we are good to go.

packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts

Pull request has been modified.

Co-authored-by: Momo Kornher <mail@moritzkornher.de>

WtfJoke · 2022-08-05T15:57:28Z

Lets see if the build turns green now.

Some rough thoughts about how improve the usability (with breaking change/feature flag)....
We could introduce an object, which either gets populated by vpc or authentication method and use that in the SelfManagedKafkaProps, the object could look like that: https://gist.github.com/WtfJoke/bd20e0bf74d77a158f41970204560ee4

That might improve the situation/usability. We also can take this up to a new PR. I dont know how you handle breaking changes/feature flags.

TheRealAmazonKendra · 2022-08-05T16:23:13Z

Lets see if the build turns green now.

Some rough thoughts about how improve the usability (with breaking change/feature flag).... We could introduce an object, which either gets populated by vpc or authentication method and use that in the SelfManagedKafkaProps, the object could look like that: https://gist.github.com/WtfJoke/bd20e0bf74d77a158f41970204560ee4

That might improve the situation/usability. We also can take this up to a new PR. I dont know how you handle breaking changes/feature flags.

Let's take those improvements to a new PR. First please open an issue for it so we can discuss further there. @mrgrain and I both think this is good to go and my gripe is with their contract, not yours. So, let's merge this and then go from there if you're interested in making this a better user experience. We really appreciate all your work on this. Thanks for taking on this issue!

mrgrain

Ready to ship 🚢 🙌🏻

mergify · 2022-08-05T16:26:08Z

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

aws-cdk-automation · 2022-08-05T17:02:30Z

AWS CodeBuild CI Report

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

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

mergify · 2022-08-05T17:02:34Z

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

…ret` Follow up to #21422 Type was missed there.

…ret` (#21555) Follow up to #21422 Type was missed there. ---- ### All Submissions: * [x] 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 * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*

aws-cdk-automation requested a review from a team August 2, 2022 18:02

github-actions bot added the p2 label Aug 2, 2022

feat(lambda-event-sources): add encryption..

14745e9

property to SelfManagedKafkaEventSource Co-authored-by: abks90 <alexander.backes@codecentric.de> Co-authored-by: ccoltx <59687842+ccoltx@users.noreply.github.com>

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

WtfJoke changed the title ~~feat(lambda-event-sources): add encryption..~~ feat(lambda-event-sources): add encryption property to SelfManagedKafkaEventSource Aug 2, 2022

WtfJoke changed the title ~~feat(lambda-event-sources): add encryption property to SelfManagedKafkaEventSource~~ feat(lambda-event-sources): add encryption property to selfmanagedkafkaeventsource Aug 2, 2022

WtfJoke requested a review from TheRealAmazonKendra August 3, 2022 21:49

mrgrain previously requested changes Aug 4, 2022

View reviewed changes

mrgrain previously requested changes Aug 5, 2022

View reviewed changes

packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts Outdated Show resolved Hide resolved

WtfJoke and others added 3 commits August 5, 2022 17:42

test(lambda-event-sources): Add integ. test

07e6614

chore(lambda-event-sources): rename encryption to rootCACertificate

bd101cd

docs(lambda-event-sources) move hint up from default

af6fd25

Co-authored-by: Momo Kornher <mail@moritzkornher.de>

WtfJoke changed the title ~~feat(lambda-event-sources): add encryption property to selfmanagedkafkaeventsource~~ feat(lambda-event-sources): add rootCACertificate to selfmanagedkafkaeventsource Aug 5, 2022

mrgrain changed the title ~~feat(lambda-event-sources): add rootCACertificate to selfmanagedkafkaeventsource~~ feat(lambda-event-sources): add rootCACertificate to SelfManagedKafkaEventSource Aug 5, 2022

mrgrain approved these changes Aug 5, 2022

View reviewed changes

Merge branch 'main' into addEncryptionParameterToSelfManagedKafka

52fb7ed

mergify bot merged commit 82a597a into aws:main Aug 5, 2022

WtfJoke deleted the addEncryptionParameterToSelfManagedKafka branch August 6, 2022 22:16

WtfJoke mentioned this pull request Aug 6, 2022

(lambda-event-sources): (Improve configuring authentication in SelfManagedKafkaEventSourceProps) #21489

Open

2 tasks

mrgrain added a commit that referenced this pull request Aug 11, 2022

fix(lambda-event-sources): rootCACertificate does not support `ISec…

eff7b29

…ret` Follow up to #21422 Type was missed there.

mrgrain mentioned this pull request Aug 11, 2022

fix(lambda-event-sources): rootCACertificate does not support ISecret #21555

Merged

4 tasks

mrgrain added the effort/small Small work item – less than a day of effort label Aug 17, 2022

Conversation

WtfJoke commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Technical Approach

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Aug 2, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

WtfJoke commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrgrain commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WtfJoke commented Aug 3, 2022

Uh oh!

TheRealAmazonKendra commented Aug 4, 2022

Uh oh!

mrgrain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WtfJoke commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheRealAmazonKendra commented Aug 4, 2022

Uh oh!

mrgrain commented Aug 4, 2022

Uh oh!

mrgrain commented Aug 5, 2022

Uh oh!

Uh oh!

WtfJoke commented Aug 5, 2022

Uh oh!

TheRealAmazonKendra commented Aug 5, 2022

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 5, 2022

Uh oh!

aws-cdk-automation commented Aug 5, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

WtfJoke commented Aug 2, 2022 •

edited

Loading

WtfJoke commented Aug 2, 2022 •

edited

Loading

mrgrain commented Aug 3, 2022 •

edited

Loading

mrgrain left a comment •

edited

Loading

WtfJoke commented Aug 4, 2022 •

edited

Loading