ruby-kafka icon indicating copy to clipboard operation
ruby-kafka copied to clipboard

add support for aws assume role credentials

Open hnaoto opened this issue 3 years ago • 9 comments
trafficstars

Description

  • The existing implementation doesn't support temporary IAM credentials. A user has to create an IAM user, get iam_access_key and iam_secret_key and then use those values to create a Ruby Kafka client.
  • This MR will add support for aws iam assume role credentials. User will be able to retrieve AssumeRoleCredentials from a STS client and use the AssumeRoleCredentials to create a Ruby Kafka client in the following fashion.
sts = Aws::STS::Client.new
# to get more information about how to create AssumeRoleCredentials
# please refer to https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/AssumeRoleCredentials.html
role_credentials = Aws::AssumeRoleCredentials.new(
  client: sts,
  role_arn: "role_arn",
  role_session_name: "role_session_name"
  
)
kafka_client = Kafka.new(
  ["broker_address"],
  aws_iam_assume_role_credentials: role_credentials,
  sasl_aws_msk_iam_aws_region: "us-east-2",
  ssl_ca_certs_from_system: true,
)

Implementation Details

  • Although the AssumeRoleCredentials are temporary credentials and will expire in 1 hour by default. The code change in this MR won't need to refresh AssumeRoleCredentials.
  • The AssumeRoleCredentials will be refreshed by AWS SDK in the background automatically (For more details, please refer to https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-core/lib/aws-sdk-core/refreshing_credentials.rb). The AssumeRoleCredentials is passed to the Kafka client as an object and it will get updated in the background as well.
  • Either assume_role_credentials object or the combination of access_key_id and secret_key_id has to be supplied. If neither of those parameters are supplied, an AwsMskIamCredentialsException error will be raised internally and an error message will be logged. The Kafka client will crash eventually due to ConnectionError.

Discussions

Discussions regarding implementation details and testing can be found in https://github.com/zendesk/ruby-kafka/issues/944

Documentations and Comments

  • [x] add comments
  • [x] add descriptions to readme
  • [x] add sample code to example directory

Testing

  • [x] test with a small MSK cluster and a skeleton ruby program...
  • [x] add unit tests
  • [x] test in working environment

hnaoto avatar Jun 15 '22 00:06 hnaoto

Hi @garrett528 and @jobeus, would you mind taking a look at the MR? Please let me know if there is anything appears to be unclear to you.

(I will try to loop in the maintainers of this repo later.

hnaoto avatar Jun 15 '22 00:06 hnaoto

will take a look this week!

garrett528 avatar Jun 15 '22 12:06 garrett528

lgtm in general but changed jobs and don't have access to a kafka cluster in AWS right now to test on, etc..

jobeus avatar Jun 16 '22 16:06 jobeus

Thanks @garrett528 😄! really appreciate your help. Thanks for joining the discussion and sharing your insights

hnaoto avatar Jun 16 '22 18:06 hnaoto

Thanks for reviewing the MR @jobeus! No worry. I will try to test the code change in a working environment

hnaoto avatar Jun 16 '22 18:06 hnaoto

Hi @dasch and @leonmaia, would you mind taking a look at this MR?

Please let me know if you need any further information or if there is anything that I can help with.

hnaoto avatar Jun 17 '22 00:06 hnaoto

Hey @hnaoto, this looks great and is something my company is interested in. Wondering what the next steps are to get this approved and merged? I'm happy to help out with testing/finalizing the PR/etc.

jordanfbrown avatar Aug 09 '22 21:08 jordanfbrown

Hi @jordanfbrown, if you are interested in testing the MR, please give it a try. If you encounter any issue, please let me know. really appreciate that.

Please noted that MSK currently has connection constraints when one uses IAM authentication. If a large number of Kafka clients trying to establish connections through IAM authentication at the same time, there will be some serious connection issues. However, it depends on the number of the connections that one attempts to establish simultaneously, and your clients could work perfectly fine if they don't hit the bottleneck.

To get the MR merged we need to have it reviewed by @dasch.

hnaoto avatar Aug 31 '22 17:08 hnaoto

I would love to see users try this out before merging.

dasch avatar Sep 02 '22 08:09 dasch

I don't know if this would be expected to work or not, but I tried this earlier with an instance of Aws::InstanceProfileCredentials and it didn't work. I'm pretty new to the AWS world, so take that with a grain of salt.

We'd like to be able to use AssumeRole or instance creds at some point, but can get by with permanent credentials.

gaustin avatar Nov 07 '22 21:11 gaustin

Pull request has been marked as stale due to a lack of activity.

github-actions[bot] avatar Jun 16 '23 00:06 github-actions[bot]