Fix aws_msk_iam_v2 module Authentication failure bug #977
Conversation
kikyomits
force-pushed
the
fix/msk-iam-v2-token-expiration
branch
from
0aae3eb to
264bc93
Compare
sasl/aws_msk_iam_v2/msk_iam.go
Outdated
| // The aws.Credentials of aws-sdk-go-v2. Required. | ||
| Credentials aws.Credentials | ||
| // The aws.CredentialsProvider of aws-sdk-go-v2. Required. | ||
| CredentialsProvider aws.CredentialsProvider |
There was a problem hiding this comment.
Credentials is shorter and clearly communicates the purpose, and the type information already expresses that we expect a provider; how about keeping the original field name?
It's also the name used in the aws.Config type, so it would remain consistent.
sasl/aws_msk_iam_v2/msk_iam.go
Outdated
| sig := signer.NewSigner() | ||
| return &Mechanism{ | ||
| Signer: sig, | ||
| CredentialsProvider: awsCfg.Credentials, | ||
| Region: awsCfg.Region, | ||
| } |
There was a problem hiding this comment.
It seems we could do without the intermediary variable here?
| sig := signer.NewSigner() | |
| return &Mechanism{ | |
| Signer: sig, | |
| CredentialsProvider: awsCfg.Credentials, | |
| Region: awsCfg.Region, | |
| } | |
| return &Mechanism{ | |
| Signer: signer.NewSigner(), | |
| CredentialsProvider: awsCfg.Credentials, | |
| Region: awsCfg.Region, | |
| } |
sasl/aws_msk_iam_v2/msk_iam.go
Outdated
| @@ -32,15 +32,15 @@ const ( | |||
| queryExpiryKey = "X-Amz-Expires" | |||
| ) | |||
|
|
|||
| var signUserAgent = fmt.Sprintf("kafka-go/sasl/aws_msk_iam/%s", runtime.Version()) | |||
| var signUserAgent = fmt.Sprintf("kafka-go/sasl/aws_msk_iam_v2/%s", runtime.Version()) | |||
There was a problem hiding this comment.
Very minor and unrelated to your change, but since it's being modified. This could be a simpler way to express the concatenation:
| var signUserAgent = fmt.Sprintf("kafka-go/sasl/aws_msk_iam_v2/%s", runtime.Version()) | |
| var signUserAgent = "kafka-go/sasl/aws_msk_iam_v2/" + runtime.Version() |
kikyomits
force-pushed
the
fix/msk-iam-v2-token-expiration
branch
2 times, most recently
from
0cd64d8 to
54fe6c1
Compare
|
@achille-roussel thanks, I have reflected your comments. Would you mind to tell me how to re-trigger the CI pipeline? Do we have a way to rerun the CI job from PR comment like |
Use `CredentialsProvider` instead of static `Credentials`. See segmentio#976
kikyomits
force-pushed
the
fix/msk-iam-v2-token-expiration
branch
from
54fe6c1 to
4455e21
Compare
|
hi @achille-roussel , I have re-ran the CI and confirmed it passed all checks. Can you please review this? |
|
Thanks for the review! Pleasure to contribute to this great project. |
Use
CredentialsProviderinstead of staticCredentials. See #976