components-contrib icon indicating copy to clipboard operation
components-contrib copied to clipboard
verified publisher icon dapr

Feat/kinesis binding vmware go kcl v2 latest

Open swatimodi-scout opened this issue 2 months ago • 18 comments

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:

  • [X] Code compiles correctly
  • [X] Created/updated tests
  • [ ] Extended the documentation

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.

swatimodi-scout avatar Oct 31 '25 10:10 swatimodi-scout

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 avatar Nov 05 '25 10:11 acroca

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.

swatimodi-scout avatar Nov 11 '25 10:11 swatimodi-scout

@mikeee @javier-aliaga Requested changes are done. Please review it.

swatimodi-scout avatar Nov 20 '25 08:11 swatimodi-scout

@swatimodi-scout linting is failing, can you fix it?

javier-aliaga avatar Nov 20 '25 10:11 javier-aliaga

@swatimodi-scout linting is failing, can you fix it?

@javier-aliaga I have fixed the lint issue, please check

rideshnath-scout avatar Nov 20 '25 10:11 rideshnath-scout

/ok-to-test

mikeee avatar Nov 20 '25 12:11 mikeee

Complete Build Matrix

The build status is currently not updated here. Please visit the action run below directly.

🔗 Link to Action run

Commit ref: d8f510305dd07f147b3486f5630a088aa91ec4b4

dapr-bot avatar Nov 20 '25 12:11 dapr-bot

Components certification test

🔗 Link to Action run

Commit ref: d8f510305dd07f147b3486f5630a088aa91ec4b4

❌ Some certification tests failed

These tests failed:

  • bindings.azure.blobstorage
  • bindings.azure.cosmosdb
  • bindings.azure.eventhubs
  • bindings.azure.servicebusqueues
  • bindings.azure.storagequeues
  • bindings.aws.s3
  • bindings.cron
  • bindings.dubbo
  • bindings.zeebe.command
  • bindings.zeebe.jobworker
  • bindings.kafka
  • bindings.kitex
  • bindings.localstorage
  • bindings.postgres
  • bindings.rabbitmq
  • bindings.redis
  • configuration.postgres
  • configuration.redis
  • middleware.http.bearer
  • middleware.http.ratelimit
  • middleware.http.opa
  • pubsub.aws.snssqs
  • pubsub.gcp.pubsub
  • pubsub.azure.eventhubs
  • pubsub.azure.servicebus.topics
  • pubsub.kafka
  • pubsub.mqtt3
  • pubsub.pulsar
  • pubsub.rabbitmq
  • secretstores.azure.keyvault
  • secretstores.hashicorp.vault
  • secretstores.local.env
  • secretstores.local.file
  • state.aws.dynamodb
  • state.azure.blobstorage
  • state.azure.cosmosdb
  • state.azure.tablestorage
  • state.cassandra
  • state.cockroachdb.v1
  • state.memcached
  • state.mongodb
  • state.mysql
  • state.postgresql.v1
  • state.postgresql.v2
  • state.redis
  • state.sqlite
  • state.sqlserver
  • state.sqlserver.v2
  • state.gcp.firestore

Additionally, some tests did not report a status:

  • bindings.azure.blobstorage
  • bindings.azure.cosmosdb
  • bindings.azure.eventhubs
  • bindings.azure.servicebusqueues
  • bindings.azure.storagequeues
  • bindings.aws.s3
  • bindings.cron
  • bindings.dubbo
  • bindings.zeebe.command
  • bindings.zeebe.jobworker
  • bindings.kafka
  • bindings.kitex
  • bindings.localstorage
  • bindings.postgres
  • bindings.rabbitmq
  • bindings.redis
  • configuration.postgres
  • configuration.redis
  • middleware.http.bearer
  • middleware.http.ratelimit
  • middleware.http.opa
  • pubsub.aws.snssqs
  • pubsub.gcp.pubsub
  • pubsub.azure.eventhubs
  • pubsub.azure.servicebus.topics
  • pubsub.kafka
  • pubsub.mqtt3
  • pubsub.pulsar
  • pubsub.rabbitmq
  • secretstores.azure.keyvault
  • secretstores.hashicorp.vault
  • secretstores.local.env
  • secretstores.local.file
  • state.aws.dynamodb
  • state.azure.blobstorage
  • state.azure.cosmosdb
  • state.azure.tablestorage
  • state.cassandra
  • state.cockroachdb.v1
  • state.memcached
  • state.mongodb
  • state.mysql
  • state.postgresql.v1
  • state.postgresql.v2
  • state.redis
  • state.sqlite
  • state.sqlserver
  • state.sqlserver.v2
  • state.gcp.firestore

dapr-bot avatar Nov 20 '25 12:11 dapr-bot

Components conformance test

🔗 Link to Action run

Commit ref: d8f510305dd07f147b3486f5630a088aa91ec4b4

❌ Some conformance tests failed

These tests failed:

  • bindings.azure.blobstorage
  • bindings.azure.cosmosdb
  • bindings.azure.eventgrid
  • bindings.azure.eventhubs
  • bindings.azure.servicebusqueues
  • bindings.azure.storagequeues
  • bindings.aws.s3.terraform
  • bindings.cron
  • bindings.http
  • bindings.influx
  • bindings.kafka-confluent
  • bindings.kafka-wurstmeister
  • bindings.kubemq
  • bindings.mqtt3-emqx
  • bindings.mqtt3-mosquitto
  • bindings.mqtt3-vernemq
  • bindings.postgresql.docker
  • bindings.postgresql.azure
  • bindings.rabbitmq
  • bindings.redis.v6
  • bindings.redis.v7
  • configuration.postgresql.docker
  • configuration.postgresql.azure
  • configuration.redis.v6
  • configuration.redis.v7
  • crypto.azure.keyvault
  • crypto.localstorage
  • crypto.jwks
  • lock.redis.v6
  • lock.redis.v7
  • pubsub.aws.snssqs.terraform
  • pubsub.gcp.pubsub.terraform
  • pubsub.azure.eventhubs
  • pubsub.azure.servicebus.queues
  • pubsub.azure.servicebus.topics
  • pubsub.in-memory
  • pubsub.jetstream
  • pubsub.kafka-confluent
  • pubsub.kafka-wurstmeister
  • pubsub.kubemq
  • pubsub.mqtt3-emqx
  • pubsub.mqtt3-vernemq
  • pubsub.pulsar
  • pubsub.rabbitmq
  • pubsub.redis.v6
  • pubsub.solace
  • secretstores.azure.keyvault.certificate
  • secretstores.azure.keyvault.serviceprincipal
  • secretstores.hashicorp.vault
  • secretstores.kubernetes
  • secretstores.local.env
  • secretstores.local.file
  • secretstores.aws.secretsmanager.terraform
  • secretstores.aws.secretsmanager.docker
  • state.aws.dynamodb.terraform
  • state.azure.blobstorage.v2
  • state.azure.blobstorage.v1
  • state.azure.cosmosdb
  • state.azure.sql
  • state.azure.tablestorage.cosmosdb
  • state.azure.tablestorage.storage
  • state.cassandra
  • state.cloudflare.workerskv
  • state.cockroachdb.v1
  • state.etcd.v1
  • state.etcd.v2
  • state.in-memory
  • state.memcached
  • state.mongodb
  • state.mysql.mariadb
  • state.mysql.mysql
  • state.oracledatabase
  • state.postgresql.v1.docker
  • state.postgresql.v1.azure
  • state.postgresql.v2.docker
  • state.postgresql.v2.azure
  • state.redis.v6
  • state.redis.v7
  • state.rethinkdb
  • state.sqlite
  • state.sqlserver
  • state.sqlserver.v2
  • state.sqlserver.docker
  • state.sqlserver.v2.docker
  • state.gcp.firestore.cloud

Additionally, some tests did not report a status:

  • bindings.azure.blobstorage
  • bindings.azure.cosmosdb
  • bindings.azure.eventgrid
  • bindings.azure.eventhubs
  • bindings.azure.servicebusqueues
  • bindings.azure.storagequeues
  • bindings.aws.s3.terraform
  • bindings.cron
  • bindings.http
  • bindings.influx
  • bindings.kafka-confluent
  • bindings.kafka-wurstmeister
  • bindings.kubemq
  • bindings.mqtt3-emqx
  • bindings.mqtt3-mosquitto
  • bindings.mqtt3-vernemq
  • bindings.postgresql.docker
  • bindings.postgresql.azure
  • bindings.rabbitmq
  • bindings.redis.v6
  • bindings.redis.v7
  • configuration.postgresql.docker
  • configuration.postgresql.azure
  • configuration.redis.v6
  • configuration.redis.v7
  • crypto.azure.keyvault
  • crypto.localstorage
  • crypto.jwks
  • lock.redis.v6
  • lock.redis.v7
  • pubsub.aws.snssqs.terraform
  • pubsub.gcp.pubsub.terraform
  • pubsub.azure.eventhubs
  • pubsub.azure.servicebus.queues
  • pubsub.azure.servicebus.topics
  • pubsub.in-memory
  • pubsub.jetstream
  • pubsub.kafka-confluent
  • pubsub.kafka-wurstmeister
  • pubsub.kubemq
  • pubsub.mqtt3-emqx
  • pubsub.mqtt3-vernemq
  • pubsub.pulsar
  • pubsub.rabbitmq
  • pubsub.redis.v6
  • pubsub.solace
  • secretstores.azure.keyvault.certificate
  • secretstores.azure.keyvault.serviceprincipal
  • secretstores.hashicorp.vault
  • secretstores.kubernetes
  • secretstores.local.env
  • secretstores.local.file
  • secretstores.aws.secretsmanager.terraform
  • secretstores.aws.secretsmanager.docker
  • state.aws.dynamodb.terraform
  • state.azure.blobstorage.v2
  • state.azure.blobstorage.v1
  • state.azure.cosmosdb
  • state.azure.sql
  • state.azure.tablestorage.cosmosdb
  • state.azure.tablestorage.storage
  • state.cassandra
  • state.cloudflare.workerskv
  • state.cockroachdb.v1
  • state.etcd.v1
  • state.etcd.v2
  • state.in-memory
  • state.memcached
  • state.mongodb
  • state.mysql.mariadb
  • state.mysql.mysql
  • state.oracledatabase
  • state.postgresql.v1.docker
  • state.postgresql.v1.azure
  • state.postgresql.v2.docker
  • state.postgresql.v2.azure
  • state.redis.v6
  • state.redis.v7
  • state.rethinkdb
  • state.sqlite
  • state.sqlserver
  • state.sqlserver.v2
  • state.sqlserver.docker
  • state.sqlserver.v2.docker
  • state.gcp.firestore.cloud

dapr-bot avatar Nov 20 '25 12:11 dapr-bot

@mikeee @acroca Is there anything required from our side now?

swatimodi-scout avatar Nov 20 '25 14:11 swatimodi-scout

@mikeee @acroca Could you please merge this PR if everything looks good to you?

swatimodi-scout avatar Nov 25 '25 07:11 swatimodi-scout

The changes to /common/authentication/aws should also be reverted if the package is not being referenced in this PR.

Are you talking about this change? - The changes to /common/authentication/aws should also be reverted if the package is not being referenced in this PR.

swatimodi-scout avatar Nov 25 '25 14:11 swatimodi-scout

/common/authentication/aws

@mikeee I have reverted the changes.

rideshnath-scout avatar Nov 26 '25 06:11 rideshnath-scout

My last review comment still stands

Done please review the changes

swatimodi-scout avatar Nov 26 '25 11:11 swatimodi-scout

Lgtm, just noting that I haven't got an environment set up to test this and we don't have certification/conformance test coverage for this binding. A bonus definitely would be if you could write some e2e tests I can set up the resources on our aws account for them.

https://github.com/dapr/components-contrib/tree/main/tests/certification/bindings/aws/s3

@mikeee Thanks for the review. We’ve tested the changes locally, and everything is working as expected. Would it be possible to merge this PR at the earliest? We can follow up by adding the e2e test cases separately afterward.

swatimodi-scout avatar Nov 27 '25 13:11 swatimodi-scout

@javier-aliaga Can we now merge this PR?

swatimodi-scout avatar Dec 01 '25 10:12 swatimodi-scout

LGTM. Still this will not be available until 1.17 (planned for January)

Got it, thanks for the update.

swatimodi-scout avatar Dec 01 '25 12:12 swatimodi-scout

@acroca and @javier-aliaga any last feedback here before merge?

sicoyle avatar Dec 09 '25 22:12 sicoyle