feat: signer using default creds #1

bruce-szalwinski-he · 2024-02-21T15:16:25Z

The msk iam sasl signer in Ruby, joining the family of msk iam sasl signers.

Waiting on OAuth support to be added to rdkafka-ruby

bruce-szalwinski-he · 2024-02-21T17:22:25Z

.rubocop.yml

@@ -73,6 +73,9 @@ Metrics/ParameterLists:
 Minitest/EmptyLineBeforeAssertionMethods:
  Enabled: false

+Minitest/MultipleAssertions:


I split up credential tests into groups and moved creation of token into setup. Increased MultipleAssertions from default of 3 to 20. With the increase, could also just put all of the assertions back together.

bruce-szalwinski-he · 2024-02-21T17:22:47Z

.rubocop.yml

+
+Naming/MethodParameterName:
+  AllowedNames:
+    - a


chose to add comparison helpers and needed to allow short names.

bruce-szalwinski-he · 2024-02-21T17:32:06Z

lib/aws/msk/iam/sasl/credentials.rb

+      module Sasl
+        module Signer
+          class Credentials
+            def load_default_credentials


put this here so that it could be stubbed with fake creds.

It makes better sense to have the client created in the presigner module. You can use the stub responses feature of the client for testing:

stubbed_client = Aws::Kafka::Client.new(stub_responses: true)

which gives stubbed credentials. You can do something like:

expect(Aws::Kafka::Client).to receive(:new).and_return(stubbed_client)

mullermp

Nice! Just some general comments and improvements - I didn't want to be a blocker to your progress.

mullermp · 2024-02-21T18:26:57Z

lib/aws/msk/iam/sasl/credentials.rb

+      module Sasl
+        module Signer
+          class Credentials
+            def load_default_credentials


It makes better sense to have the client created in the presigner module. You can use the stub responses feature of the client for testing:

stubbed_client = Aws::Kafka::Client.new(stub_responses: true)

which gives stubbed credentials. You can do something like:

expect(Aws::Kafka::Client).to receive(:new).and_return(stubbed_client)

mullermp · 2024-02-21T18:56:37Z

lib/aws/msk/iam/sasl/signer.rb

+            end
+
+            def endpoint_url
+              host = ENDPOINT_URL_TEMPLATE.gsub("{}", @region)


Instead of using the template here, use endpoint resolution pieces:

endpoint_params = Aws::Kafka::EndpointParameters.new( region: region, use_dual_stack: true/false # can be options use_fips: true/false # can be options ) endpoint = client.config.endpoint_provider.resolve_endpoint(endpoint_params)

I was following the pattern that the go and python signers use. Will look at EndpointParameters.

mullermp · 2024-02-21T18:59:33Z

lib/aws/msk/iam/sasl/signer.rb

+            def endpoint_url
+              host = ENDPOINT_URL_TEMPLATE.gsub("{}", @region)
+              query_params = {
+                Action: "kafka-cluster:Connect"


Is this the only query param ever supported for this to work?

Yes. Once you connect, then MSK Cluster policy along with your IAM Policy governs what you can do in the cluster.

mullermp · 2024-02-21T19:00:41Z

lib/aws/msk/iam/sasl/signer.rb

+            ENDPOINT_URL_TEMPLATE = "kafka.{}.amazonaws.com".freeze
+            DEFAULT_TOKEN_EXPIRY_SECONDS = 900
+            ACTION_TYPE = "Action".freeze
+            ACTION_NAME = "kafka-cluster:Connect".freeze


Some of these aren't used

mullermp · 2024-02-21T19:01:06Z

aws-msk-iam-sasl-signer.gemspec

+  spec.require_paths = ["lib"]
+
+  # Runtime dependencies
+  # spec.add_dependency "thor", "~> 1.2"


You'll want a dependency on aws-sdk-kafka.

HarlemSquirrel

I definitely like the single module name AwsMskIamSaslSigner better 👍

Got a few comments

HarlemSquirrel · 2024-02-27T19:38:48Z

Gemfile

@@ -1,11 +1,16 @@
 source "https://rubygems.org"
 gemspec

+gem "aws-sdk-kafka", "~> 1.68"


Don't put this here since it will be read from gemspec above

HarlemSquirrel · 2024-02-27T19:38:52Z

Gemfile

 gem "rake", "~> 13.0"
 gem "rubocop", "1.60.2"
 gem "rubocop-minitest", "0.34.5"
 gem "rubocop-packaging", "0.5.2"
 gem "rubocop-performance", "1.20.2"
 gem "rubocop-rake", "0.6.0"
+gem "simplecov", require: false, group: :test
+gem "thor", "~> 1.3"


Don't put this here since it will be read from gemspec above

HarlemSquirrel · 2024-02-27T19:46:44Z

lib/aws-msk-iam-sasl-signer/msk_token_provider.rb

+        sts.get_caller_identity.arn
+      )
+
+      puts "Credentials Identity: #{caller_identity.to_h.to_json}"


I would avoid puts in the lib since it's not good practice with Rails apps. If we need to output something, we should look for Rails.logger to be defined, allow overriding it in a config, and have a default logger for non-Rails apps

…o with it

HarlemSquirrel

HarlemSquirrel · 2024-02-29T04:25:46Z

bin/console

@@ -1,7 +1,7 @@
 #!/usr/bin/env ruby

 require "bundler/setup"
-require "example"
+require "aws/msk/iam/sasl/signer"


This needs to point to the flattened file now, right?

bruce-szalwinski-he added 10 commits February 21, 2024 09:13

signer using default creds

f7c908d

pin depdendencies

85ecb0d

assert_equal, expected on the left

d4fb4bf

sort by gem

6edda84

allow a and b as arg names

55884af

rubocop

69c2383

fix trailing comma

bed1983

map to transform_values

d314b28

allow for more assertions

563f53e

no need to parse url

7c89a14

bruce-szalwinski-he commented Feb 21, 2024

View reviewed changes

remove activesupport

f36d3a7

bruce-szalwinski-he commented Feb 21, 2024

View reviewed changes

bruce-szalwinski-he added 2 commits February 21, 2024 11:36

remove activesupport

ca9e2b0

add base64 because it is removed from core in 3.4

a373372

bruce-szalwinski-he requested a review from mullermp February 21, 2024 18:57

mullermp reviewed Feb 21, 2024

View reviewed changes

bruce-szalwinski-he added 11 commits February 21, 2024 13:07

remove unused

bab905d

aws-sdk-kafka as run time dependency

71ffb6c

add exe, move msk token provider to its own file

ffd667a

organize like a pro

2fdaafc

can create token from cli

9d19eb0

example -> signer

95cf193

flatten

45cd8c6

lint

494c65f

towards aws-profile

c63957b

token from role arn

cd83616

fix abc

abad79e

bruce-szalwinski-he requested a review from HarlemSquirrel February 21, 2024 23:04

bruce-szalwinski-he added 4 commits February 23, 2024 15:20

codeclimate

3e3a7a4

simiplecov report

2c13b05

codecoverage

3dfb799

save code coverage for later

157873c

bruce-szalwinski-he marked this pull request as ready for review February 23, 2024 21:47

bruce-szalwinski-he added 13 commits February 23, 2024 15:54

remove unneeded

837f3bb

flatten namespace

803767b

flatten namespace

c277798

version test

2bcc0c4

test all of the generators

cfebb2d

names are hard

fd5bfff

found the s

bd7a420

frozen

50333e3

found the s

15f6b4b

names are hard

d673608

refactor

f291087

flow

e366137

fix typo

086a6df

HarlemSquirrel reviewed Feb 27, 2024

View reviewed changes

bruce-szalwinski-he added 6 commits February 27, 2024 18:15

let gemspec include the deps

0adc0a7

optionally return caller identity and let caller figure out what to d…

8418f50

…o with it

update docs

fddd5af

update docs

6bbfb2e

return a struct

bfd964e

return a struct

25e1e3f

bruce-szalwinski-he requested a review from HarlemSquirrel February 28, 2024 21:57

HarlemSquirrel approved these changes Feb 29, 2024

View reviewed changes

flatten namespace

55d1288

bruce-szalwinski-he merged commit bef3893 into main Feb 29, 2024
7 checks passed

bruce-szalwinski-he deleted the sasl-signer branch February 29, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: signer using default creds #1

feat: signer using default creds #1

bruce-szalwinski-he commented Feb 21, 2024 •

edited

Loading

bruce-szalwinski-he Feb 21, 2024

bruce-szalwinski-he Feb 21, 2024

bruce-szalwinski-he Feb 21, 2024

mullermp Feb 21, 2024

mullermp left a comment

mullermp Feb 21, 2024

mullermp Feb 21, 2024

bruce-szalwinski-he Feb 21, 2024

mullermp Feb 21, 2024

bruce-szalwinski-he Feb 21, 2024

mullermp Feb 21, 2024

mullermp Feb 21, 2024

HarlemSquirrel left a comment

HarlemSquirrel Feb 27, 2024

bruce-szalwinski-he Feb 28, 2024

HarlemSquirrel Feb 27, 2024

bruce-szalwinski-he Feb 28, 2024

HarlemSquirrel Feb 27, 2024

bruce-szalwinski-he Feb 28, 2024

HarlemSquirrel left a comment

HarlemSquirrel Feb 29, 2024

feat: signer using default creds #1

feat: signer using default creds #1

Conversation

bruce-szalwinski-he commented Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullermp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarlemSquirrel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarlemSquirrel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruce-szalwinski-he commented Feb 21, 2024 •

edited

Loading