Feat/kinesis binding vmware go kcl v2 latest #4082

swatimodi-scout · 2025-10-31T10:53:13Z

Description

This PR includes two key improvements to the AWS Kinesis input binding:

Upgrade to vmware-go-kcl-v2
- Updated the Kinesis consumer dependency from the legacy vmware-go-kcl to vmware-go-kcl-v2.
- Motivation: The original library is no longer actively maintained and lacks critical bug fixes and performance improvements.
- Changes:
  - Updated go.mod to reference github.com/vmware/vmware-go-kcl-v2.
  - Adjusted import paths and updated worker configuration to align with the new v2 APIs.
  - Verified compatibility with existing Kinesis input binding behavior through unit and integration tests.
Fix for nil pointer reference in Kinesis input binding
- Resolved a bug where the binding attempted to resolve the stream ARN unconditionally, even when KinesisConsumerMode was set to SharedThroughput (default mode).
- Issue: The binding was calling authProvider.Kinesis().Stream(...) regardless of the configured consumer mode.
- Impact: When running in SharedThroughput mode (such as with LocalStack), this caused nil pointer errors if the stream was unavailable or uninitialized.
- Fix: Added a conditional check so that stream ARN resolution only occurs when KinesisConsumerMode is set to ExtendedFanout.

These changes improve stability and maintainability of the Kinesis input binding, ensuring compatibility with LocalStack and future AWS SDK updates.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[3980, 3985]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation
- Created the dapr/docs PR: <insert PR link here>

Note: We expect contributors to open a corresponding documentation PR in the dapr/docs repository. As the implementer, you are the best person to document your work! Implementation PRs will not be merged until the documentation PR is opened and ready for review.

sicoyle

thank you for your contribution to the Dapr project 🙌 - few comments :)

sicoyle · 2025-10-31T13:45:04Z

bindings/aws/kinesis/kinesis.go

+	closed          atomic.Bool
+	closeCh         chan struct{}
+	wg              sync.WaitGroup
+	applicationName string


can you please share some insights on why the additional field here?

can you please share some insights on why the additional field here?

applicationName is required for KCL (Kinesis Client Library) worker configuration in shared throughput mode. It identifies the consumer application and is used for DynamoDB table naming and checkpointing.

Without applicationName we were facing an error.

I'd move this field next to consumerMode. And the comment is not necessary if you ask me, I'd remove it for consistency, but feel free to keep if you think it's useful :)

I'd move this field next to consumerMode. And the comment is not necessary if you ask me, I'd remove it for consistency, but feel free to keep if you think it's useful :)

Done

sicoyle · 2025-10-31T13:45:53Z

common/authentication/aws/client.go

+			v1Creds, err := c.Credentials.Get()
+			if err != nil {
+				return nil


why are we ignoring the err here? If we are updating to use v2 then can you make the v2 creds provider be the default and if err then maybe try the v1 and then err from there? but pls reference the other aws components to see the auth flow so we can ensure that we standardize on this for when using v2 sdks.

why are we ignoring the err here? If we are updating to use v2 then can you make the v2 creds provider be the default and if err then maybe try the v1 and then err from there? but pls reference the other aws components to see the auth flow so we can ensure that we standardize on this for when using v2 sdks.

Done please verify this.

acroca · 2025-11-05T10:51:53Z

The git history of this PR seems odd, lots of those commits are unrelated. Do you mind cleaning it up so it only contains commits related to this change?

Signed-off-by: swatimodi-scout <[email protected]>

Signed-off-by: devendrapohekar-scout <[email protected]>

swatimodi-scout · 2025-11-11T10:11:23Z

The git history of this PR seems odd, lots of those commits are unrelated. Do you mind cleaning it up so it only contains commits related to this change?

@acroca Thanks for the feedback! I’ve cleaned up the branch history — the PR now contains only the relevant commit for this change. Please let me know if anything else needs adjustment.

Signed-off-by: swatimodi-scout <[email protected]>

bindings/aws/kinesis/kinesis.go

acroca · 2025-11-11T11:20:16Z

bindings/aws/kinesis/kinesis.go

+	closed          atomic.Bool
+	closeCh         chan struct{}
+	wg              sync.WaitGroup
+	applicationName string


I'd move this field next to consumerMode. And the comment is not necessary if you ask me, I'd remove it for consistency, but feel free to keep if you think it's useful :)

acroca · 2025-11-11T11:21:19Z

common/authentication/aws/client.go

+		/**
+		 * If the error is not nil, do not proceed to the next step
+		 * as it may cause a nil pointer error on stream.StreamDescription.StreamARN.
+		 */


I think the comment is not necessary

I think the comment is not necessary

Done

common/authentication/aws/client.go

acroca · 2025-11-11T11:29:17Z

common/authentication/aws/client.go

+				kclConfig := config.NewKinesisClientLibConfigWithCredential(applicationName, stream, region, "", v2Config.Credentials)
 				return kclConfig
 			}
+			// Fallback to v1 credentials if v2 fails


In which cases would v2 fail, but v1 work?

In which cases would v2 fail, but v1 work?

@acroca

Good question! You're right to point that out. Looking at this more carefully, the v2 fallback to v1 scenario is actually quite rare and adds unnecessary complexity.

In practice, both v1 and v2 SDKs use the same underlying credential sources (env vars, credential files, IAM roles, etc.), so if v2 fails to load credentials, v1 would likely fail for the same reason.

Since we already have validated v1 credentials from the established session, I've simplified this to directly use those credentials and convert them to v2 format for KCL compatibility. This is more reliable and removes the redundant credential resolution logic.

Updated the code to use the existing session credentials directly - much cleaner approach! 👍

Co-authored-by: Albert Callarisa <[email protected]> Signed-off-by: swatimodi-scout <[email protected]>

Signed-off-by: swatimodi-scout <[email protected]>

acroca · 2025-11-12T11:40:59Z

common/authentication/aws/client.go

+		v1Creds, err := c.Credentials.Get()
+		if err != nil {
+			return nil
 		}
+		v2Creds := v2creds.NewStaticCredentialsProvider(v1Creds.AccessKeyID, v1Creds.SecretAccessKey, v1Creds.SessionToken)


It feels wrong to have both v1 and v2 living together. Can't we migrate completely to v2?

It feels wrong to have both v1 and v2 living together. Can't we migrate completely to v2?

Done please review it.

I don't think this works, it's not wired up to the refresh at all.
I think it was better before, but the conversion from v1 credentials to v2 should be done in the KinesisClients.New, which is called in the refresh operations.
This way, you can convert all kinesis to v2 and only do the v1->v2 in the New right?

thansks @swatimodi-scout! What if you fully migrate the aws client to v2? you do not need to migrate all other components, it would be something similar to this PR that @mikeee is working on.

thansks @swatimodi-scout! What if you fully migrate the aws client to v2? you do not need to migrate all other components, it would be something similar to this PR that @mikeee is working on.

@javier-aliaga Yes, that makes sense. However, we’re not migrating the entire AWS SDK version at this stage — my current change focuses only on updating vmware-go-kcl to vmware-go-kcl-v2.

Would you prefer that I continue implementing the AWS SDK v2 changes for Kinesis within this same PR, or should I move that work to a separate PR?

I don't think this works, it's not wired up to the refresh at all. I think it was better before, but the conversion from v1 credentials to v2 should be done in the KinesisClients.New, which is called in the refresh operations. This way, you can convert all kinesis to v2 and only do the v1->v2 in the New right?

@acroca I updated in KinesisClients.New. It is working i checked in local. Let me know if i need to revert back to previous version where we were supporting both v1 and v2.

I think you can do it in the same PR as changing to aws-sdk-v2 is related to update the vmware-go-kcl-v2

@mikeee I updated the aws-sdk-v2 issue to track it
#3896

I think you can do it in the same PR as changing to aws-sdk-v2 is related to update the vmware-go-kcl-v2

@javier-aliaga Please review the code.

Signed-off-by: swatimodi-scout <[email protected]>

Signed-off-by: swatimodi-scout <[email protected]> Signed-off-by: rideshnath-scout <[email protected]>

Signed-off-by: rideshnath-scout <[email protected]>

swatimodi-scout requested review from a team as code owners October 31, 2025 10:53

sicoyle reviewed Oct 31, 2025

View reviewed changes

swatimodi-scout requested a review from sicoyle November 3, 2025 12:43

swatimodi-scout and others added 2 commits November 11, 2025 15:14

make the v2 creds provider be the default

2eddb15

Signed-off-by: swatimodi-scout <[email protected]>

feat: Upgrade kinesis github.com/vmware/vmware-go-kcl-v1 to v2.

cabcec5

Signed-off-by: devendrapohekar-scout <[email protected]>

swatimodi-scout force-pushed the feat/kinesis-binding-vmware-go-kcl-v2-latest branch from e6bc920 to cabcec5 Compare November 11, 2025 10:09

Resolved conflicts

112fd8d

Signed-off-by: swatimodi-scout <[email protected]>

acroca reviewed Nov 11, 2025

View reviewed changes

swatimodi-scout and others added 3 commits November 11, 2025 17:42

Update common/authentication/aws/client.go

de8646f

Co-authored-by: Albert Callarisa <[email protected]> Signed-off-by: swatimodi-scout <[email protected]>

Update bindings/aws/kinesis/kinesis.go

90e071c

Co-authored-by: Albert Callarisa <[email protected]> Signed-off-by: swatimodi-scout <[email protected]>

Resolved comments given by acroca

648a318

Signed-off-by: swatimodi-scout <[email protected]>

swatimodi-scout requested a review from acroca November 12, 2025 06:31

acroca reviewed Nov 12, 2025

View reviewed changes

Removed v1 credentials for kinesis

b686d94

Signed-off-by: swatimodi-scout <[email protected]>

javier-aliaga mentioned this pull request Nov 13, 2025

Upgrading the rest of the components using AWS to V2 #3896

Open

10 tasks

swatimodi-scout force-pushed the feat/kinesis-binding-vmware-go-kcl-v2-latest branch from 01bf923 to 45d9390 Compare November 13, 2025 13:05

swatimodi-scout and others added 2 commits November 14, 2025 12:28

updated client.go

9a70082

Signed-off-by: swatimodi-scout <[email protected]> Signed-off-by: rideshnath-scout <[email protected]>

Update AWS SDK to v2 and refactor Kinesis integration

9dafc6d

Signed-off-by: rideshnath-scout <[email protected]>

rideshnath-scout force-pushed the feat/kinesis-binding-vmware-go-kcl-v2-latest branch from 2c85b67 to 9dafc6d Compare November 14, 2025 06:59

Merge branch 'main' into feat/kinesis-binding-vmware-go-kcl-v2-latest

8b93022

swatimodi-scout requested review from acroca and javier-aliaga November 14, 2025 07:02

Merge branch 'main' into feat/kinesis-binding-vmware-go-kcl-v2-latest

2ee207c

Feat/kinesis binding vmware go kcl v2 latest #4082

Are you sure you want to change the base?

Feat/kinesis binding vmware go kcl v2 latest #4082

Uh oh!

Conversation

swatimodi-scout commented Oct 31, 2025

Description

Issue reference

Checklist

Uh oh!

sicoyle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swatimodi-scout Nov 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acroca commented Nov 5, 2025

Uh oh!

swatimodi-scout commented Nov 11, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swatimodi-scout Nov 13, 2025 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

swatimodi-scout Nov 3, 2025 •

edited

Loading

swatimodi-scout Nov 13, 2025 •

edited

Loading