add support for SASL mechanism AWS_MSK_IAM. fixes #34

[kjellmartinfalk]

PR not completely done, more opening this for discussion.

Description of your changes

Enables users to authenticate towards AWS MSK Kafka cluster with injected AWS credentials.

Fixes #34

I have:

• [x] Read and followed Crossplane's contribution process.
• [x] Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Currently only tested locally by "simulating" an EKS cluster environment, i.e by "injecting" the token and set the corresponding env vars that would otherwise be "automagically" injected.

Plan is to do a "real" test on a EKS cluster before merge.

[kjellmartinfalk]

So, reason for opening this PR was to ask some questions and see where you guys stand.

What's done currently is to let the users choose either PLAIN or AWS_MSK_IAM to enable the IAM auth flow while also give the opportunity to assume a different role via the injected identity.

This currently looks like this.

   {
     "brokers":[
       "kafka-dev-0.kafka-dev-headless:9092"
      ],
      "sasl":{
        "mechanism":"AWS_MSK_IAM",
        "roleArn": "arn:aws:iam::account:role/role-name-with-path" (optional)
      }
   }

However, I wonder if there is a particular reason for having the brokers field in the secret? That kinda forces us to use a secret even in the case of injected identity, when otherwise that secret would be redundant and therefor omitted.

If we would to move the brokers to the ProviderConfig instead that would render something like this:

apiVersion: kafka.crossplane.io/v1alpha1
kind: ProviderConfig
metadata:
  ...
  annotations:
    eks.amazonaws.com/role-arn: arn:aws:iam::account:role/role-name-with-path
spec:
  brokers: [...]
  credentials:
    source: ... 
    roleArn: arn:aws:iam::account:role/my-cross-account-role (optional)
    ...

and the secret can be omitted for this use case.

Hope it's clear what I'm suggesting here. Also not suggesting this to be done on this issue since it's a bigger change.

[marshmallory]

Sorry @mdhwk. I didn't get notified of this change. Taking a look now and we'll get back to you shortly. :)

[marshmallory]

@mdhwk Thanks for your patience.

I ran your updated code, and everything appears to still be working as expected locally. Unfortunately, I don't think I can pull in a real EKS cluster to test, so we'll have to rely on your testing for that before merge.

For the broker question: In the first release, we decided to implement the seed broker and credentials all at once, as you've seen. We built this off of the crossplane provider template, and had assistance for much of the setup, since it was our first time working with crossplane and kafka. Honestly, we had not given it a lot of thought before discussing it now. We're not kafka experts, so it wasn't a question we would've known to ask.

We agree that it would be quite an undertaking to change it, and that it should be outside of this PR if we decide it's necessary. Happy to hear your thoughts on that too, if you have them. Feel free to set up an issue.

In terms of why it's done this way, I'm investigating further with Upbound. I think that it was for convenience of not having to separate the data sources, but I want to 100% verify that before giving a final answer. There is a definite possibility that I'm missing a reason for this configuration. I'll let you know what I find out!

[kjellmartinfalk]

No worries and thanks for your detailed answer, it answers my questions.

Relating to the brokers, my question was more if there was any specific reason behind it, that I am not aware of, but seems it wasn't.

Regarding being necessary, I would say no. It's perfectly fine for people to create a secret and only store the brokers urls in there, however it would maybe be a nice change since it would align with the Crossplane AWS provider for example. With that said, I will create an issue for it and I am happy to contribute the change when and if you deem it necessary.

I will also test this on a proper EKS cluster before making this PR a non-draft PR, so trusting it work won't be needed :)

[marshmallory]

No worries and thanks for your detailed answer, it answers my questions.

Relating to the brokers, my question was more if there was any specific reason behind it, that I am not aware of, but seems it wasn't.

Regarding being necessary, I would say no. It's perfectly fine for people to create a secret and only store the brokers urls in there, however it would maybe be a nice change since it would align with the Crossplane AWS provider for example. With that said, I will create an issue for it and I am happy to contribute the change when and if you deem it necessary.

I will also test this on a proper EKS cluster before making this PR a non-draft PR, so trusting it work won't be needed :)

I got another update from Upbound, and as I suspected the only reason for the brokers being in there was to keep the connection information together.

[marshmallory]

Hey there. It looks like there's another PR that will complete the MSK work, just as an FYI. :)

[marshmallory]

Closing, as this has been resolved by another PR.

[emelieakerstrom-sb1]

Hi @marshmallory,

We have been testing out your PR for IAM authentication with MSK in our own fork as we are looking to have this feature. If you closed this PR since you saw the notice that I merged this to our fork, please feel free to open this up again so that you can contribute with your code to the official repo! Thanks

[kjellmartinfalk]

Hi @emelieakerstrom-sb1,

That's awesome. Have you guys verified it working in an actual EKS cluster? That's where I left off and went on vacation and didn't really think of this after that...

[emelieakerstrom-sb1]

Hi @mdhwk,

We had some issues connecting to the provider to authenticate which we were trying to debug when there was a decision not to use IAM authentication but SALS/SCRAM. So we decided not to go forward with the testing of this code. I do not think it was an issue with your code, but something else in our implementation. But unfortunately that means we never got to the end of this before vacations either.

[jograca]

Reopening at your request @emelieakerstrom-sb1

CC @marshmallory