feature: add redis token refresh support #3238

sadath-12 · 2023-11-20T05:58:09Z

Description

Please explain the changes you've made

The changes proposed will support token refresh of redis using auth command every hour and maintains the connection without terminating as before

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: #3088

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 issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

Signed-off-by: sadath-12 <[email protected]>

daixiang0 · 2023-11-20T06:24:24Z

Please fix CI.

ping @johnewart @ItalyPaleAle

Signed-off-by: sadath-12 <[email protected]>

sadath-12 · 2023-11-20T06:43:14Z

Sure, Thank you @daixiang0

Signed-off-by: sadath-12 <[email protected]>

JoshVanL

Some comments from me 🙂

common/component/redis/v9client.go

Signed-off-by: sadath-12 <[email protected]>

sadath-12 · 2023-11-20T16:55:08Z

Push the changes @JoshVanL . In order to make sure that background goroutine is terminated I have used closeCh . As you mentioned in comments about wait group ? do we have to use it here necessarily ? Since after close that goroutine will either listen to the incoming channel and terminate or if it does not listen means it was terminated anyways

common/component/redis/v8client.go

common/component/redis/v9client.go

Signed-off-by: sadath-12 <[email protected]>

…trib into redis-1

Signed-off-by: sadath-12 <[email protected]>

sadath-12 · 2023-11-20T18:40:17Z

loop must run every "a little bit less than 1hr" ? why and how ? please

ItalyPaleAle · 2023-11-20T18:57:07Z

loop must run every "a little bit less than 1hr" ? why and how ? please

So in here the person from the Azure Redis Cache team explained that the service expects a new AUTH command, with a new token, sent every hour at most.

If you run the loop every 1h exactly, you may be sending the updated command too late, maybe even by a few ms, so this would cause the session to expire.

When there's something that must be renewed periodically, the common pattern is to do that a little bit before the deadline. In this case, if the deadline is 1h, maybe do it every 50m. This also leaves us some buffer, so if requesting the token fails, it can be retried 1m later, and we are still < 1h so the session is kept active.

Signed-off-by: sadath-12 <[email protected]>

sadath-12 · 2023-11-20T19:14:08Z

loop must run every "a little bit less than 1hr" ? why and how ? please

So in here the person from the Azure Redis Cache team explained that the service expects a new AUTH command, with a new token, sent every hour at most.

If you run the loop every 1h exactly, you may be sending the updated command too late, maybe even by a few ms, so this would cause the session to expire.

When there's something that must be renewed periodically, the common pattern is to do that a little bit before the deadline. In this case, if the deadline is 1h, maybe do it every 50m. This also leaves us some buffer, so if requesting the token fails, it can be retried 1m later, and we are still < 1h so the session is kept active.

Thank you 😊👌

common/component/redis/v8client.go

common/component/redis/v9client.go

sadath-12 · 2023-12-06T17:14:29Z

thank you @ItalyPaleAle , following that

Signed-off-by: sadath-12 <[email protected]>

…trib into redis-1

Signed-off-by: sadath-12 <[email protected]>

common/component/redis/redis.go

Signed-off-by: sadath-12 <[email protected]>

berndverst · 2024-01-25T20:23:28Z

@sadath-12 you need to run your changed files through an autoformatter. gofumpt

Signed-off-by: sadath-12 <[email protected]>

DeepanshuA · 2024-01-30T02:13:26Z

I did following to verify this:

Enabled Managed identity on my already existing AKS cluster: az aks update -g -n --enable-managed-identity
Created "Azure cache for Redis" - Here, Enabled "Managed Identity"
In Authentication tab of my Redis (Azure Portal), I provided my Service Principal (my AKS cluster ObjectId)
Created new dapr Build with PR 3238 and deployed that on my AKS cluster
Created a new redis component:

kind: Component
metadata:
  name: statestore
spec:
  type: state.redis
  version: v1
  metadata:
  - name: useAzureAD
    value: true

Applied nodeapp from hello-kubernetes as it would use redis statestore, but it fails with this error:

time="2024-01-29T12:50:50.191942482Z" level=fatal msg="Fatal error from runtime: process component statestore error: [INIT_COMPONENT_FAILURE]: initialization error occurred for statestore (state.redis/v1): [INIT_COMPONENT_FAILURE]: initialization error occurred for statestore (state.redis/v1): redis store: error connecting to redis at : dial tcp [::1]:6379: connect: connection refused" app_id=nodeapp instance=nodeapp-577cc79c45-4b7rs scope=dapr.runtime type=log ver=edge

When I had tried to add principalId in user identity section of "azure redis", then it was not able to recognize that.
So, I had added to Authentication.

Do we need to enable managed identity some other way to verify this? @shpathak-msft if you could please confirm here?

github-actions · 2024-02-29T02:34:38Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

berndverst · 2024-02-29T03:43:30Z

I did following to verify this:

Enabled Managed identity on my already existing AKS cluster: az aks update -g -n --enable-managed-identity

Created "Azure cache for Redis" - Here, Enabled "Managed Identity"

In Authentication tab of my Redis (Azure Portal), I provided my Service Principal (my AKS cluster ObjectId)

Created new dapr Build with PR 3238 and deployed that on my AKS cluster

Created a new redis component:
kind: Component
metadata:
  name: statestore
spec:
  type: state.redis
  version: v1
  metadata:
  - name: useAzureAD
    value: true
Applied nodeapp from hello-kubernetes as it would use redis statestore, but it fails with this error:

time="2024-01-29T12:50:50.191942482Z" level=fatal msg="Fatal error from runtime: process component statestore error: [INIT_COMPONENT_FAILURE]: initialization error occurred for statestore (state.redis/v1): [INIT_COMPONENT_FAILURE]: initialization error occurred for statestore (state.redis/v1): redis store: error connecting to redis at : dial tcp [::1]:6379: connect: connection refused" app_id=nodeapp instance=nodeapp-577cc79c45-4b7rs scope=dapr.runtime type=log ver=edge

When I had tried to add principalId in user identity section of "azure redis", then it was not able to recognize that. So, I had added to Authentication.

Do we need to enable managed identity some other way to verify this? @shpathak-msft if you could please confirm here?

@DeepanshuA please use Azure Workload Identity instead. For this you first create a managed identity - user assigned or system assigned. Obviously you need to give the correct Redis permissions (role assignment) to this identity.

Then you must follow these steps for your Dapr enabled deployment:
https://learn.microsoft.com/en-us/azure/aks/workload-identity-overview?tabs=dotnet

azure.workload.identity/use: true
and
azure.workload.identity/client-id which must be set to the client ID of your managed identity you wish to use.

If the AKS cluster has workload identity enabled what will happen is that a special service account token gets mounted in each container of the annotated deployment and env vars are also injected. This allows the Azure Identity SDK to then make a request to authenticate has the managed identity.

I suggest first making sure with another Azure Dapr component that you have workload identity working correctly if you run into issues.

berndverst · 2024-02-29T03:51:11Z

common/component/redis/v9client.go

+var v9logger = logger.NewLogger("dapr.components.redisv9")
+
+const (
+	tokenRefreshInterval = 50 * time.Minute


Apparently we do not have to do this every hour -- instead we should examine the most recent token expiration / duration, and then refresh the token say 10 minutes before it expires.

The token validity is variable and can be longer than one hour.

There should be a function you can use to examine the validity of the token.

shpathak-msft · 2024-02-29T03:51:43Z

@sadath-12 and @ItalyPaleAle apologies for the confusion in the other issue, adding some clarifications here. Redis server does not care what is the lifetime of the token - it can be 1 hour or 24 hours. The only expectation is that the redis server receives an auth command before the previous token expires. I would recommend not hard coding the "re-auth" loop to run every 50 minutes but it should run ~10 minutes before the token expiry.

common/component/redis/redis.go

github-actions · 2024-03-30T04:44:09Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2024-04-06T04:46:03Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

mikeee · 2024-04-17T14:39:59Z

@sadath-12 This PR is being tracked for v1.14, a few comments are outstanding - do you have the bandwidth to pick this up still?

Signed-off-by: Bernd Verst <[email protected]>

berndverst · 2024-05-02T00:04:59Z

common/component/redis/redis.go

+	ticker := time.NewTicker(tokenRefreshInterval)
+	defer ticker.Stop()
+
+	if !s.useAzureAD {


I would like to see this exit condition instead be used in the v8client and v9client only.

if s.useAzureAd { authenticateWithAzureRedis() }

or something like that

berndverst · 2024-05-02T00:05:26Z

common/component/redis/redis.go

@@ -252,3 +255,65 @@ type RedisError string
 func (e RedisError) Error() string { return string(e) }

 func (RedisError) RedisError() {}
+
+func (s *Settings) refreshTokenRoutineForRedis(ctx context.Context, redisClient RedisClient, version string, meta map[string]string, logger logger.Logger) {


I'd rename this to

Suggested change

func (s *Settings) refreshTokenRoutineForRedis(ctx context.Context, redisClient RedisClient, version string, meta map[string]string, logger logger.Logger) {

func (s *Settings) authenticateWithAzureRedis(ctx context.Context, redisClient RedisClient, version string, meta map[string]string, logger logger.Logger) {

berndverst · 2024-05-02T00:07:06Z

common/component/redis/redis.go

@@ -252,3 +255,65 @@ type RedisError string
 func (e RedisError) Error() string { return string(e) }

 func (RedisError) RedisError() {}
+
+func (s *Settings) refreshTokenRoutineForRedis(ctx context.Context, redisClient RedisClient, version string, meta map[string]string, logger logger.Logger) {
+	ticker := time.NewTicker(tokenRefreshInterval)


The ticker value must be reset every time you get a new token. The value must be token expiration time minus 10 minutes (so that we refresh 10 minutes before expiration).

berndverst · 2024-05-02T00:15:12Z

common/component/redis/redis.go

+
+				// Authenticate with Redis using the refreshed token
+				var authErr error
+				if version == "v8" {


No, this is bad.

Instead the Auth method should be added to the interface in redis.go, then an implementation should be added for the v8client.go and v9client.go

That way you can then simply call client.Auth without using the Pipeline client and without using version specific clients in this code. The version should not be required here.

References:
https://pkg.go.dev/github.com/go-redis/redis/v8#Conn.Auth
https://pkg.go.dev/github.com/go-redis/redis/v9#Conn.Auth

berndverst · 2024-05-02T00:16:56Z

common/component/redis/redis.go

+				}
+				at, err := tokenCred.GetToken(ctx, policy.TokenRequestOptions{
+					Scopes: []string{
+						env.Cloud.Services[azure.ServiceOSSRDBMS].Audience + "/.default",


@ItalyPaleAle does this dapr.io/oss-rdbms environment setting always exist? I don't think so?

github-actions · 2024-06-01T19:17:43Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2024-06-08T19:24:20Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

sadath-12 added 3 commits November 17, 2023 12:52

feat: redis refresh token

20e6f07

Signed-off-by: sadath-12 <[email protected]>

feat: azure aad with redis token

5fb1c79

Signed-off-by: sadath-12 <[email protected]>

feat: redis azure aad supprot

a2f7680

Signed-off-by: sadath-12 <[email protected]>

sadath-12 requested review from a team as code owners November 20, 2023 05:58

fix client

0ff785c

Signed-off-by: sadath-12 <[email protected]>

generic

9cd23cb

Signed-off-by: sadath-12 <[email protected]>

JoshVanL suggested changes Nov 20, 2023

View reviewed changes

logger and closeCh

a5c961e

Signed-off-by: sadath-12 <[email protected]>

Merge branch 'main' into redis-1

c88df66

ItalyPaleAle reviewed Nov 20, 2023

View reviewed changes

sadath-12 added 3 commits November 21, 2023 00:04

minor fixes

b9fb8e2

Signed-off-by: sadath-12 <[email protected]>

Merge branch 'redis-1' of https://github.com/sadath-12/components-con…

287851d

…trib into redis-1

undo binding/redis

607cacf

Signed-off-by: sadath-12 <[email protected]>

chore: tokenRefreshInterval

2ebeaf0

Signed-off-by: sadath-12 <[email protected]>

yaron2 and others added 2 commits December 1, 2023 10:05

Merge branch 'main' into redis-1

c043100

Merge branch 'main' into redis-1

93ff135

ItalyPaleAle suggested changes Dec 6, 2023

View reviewed changes

common/component/redis/v8client.go Outdated Show resolved Hide resolved

common/component/redis/v9client.go Outdated Show resolved Hide resolved

common/component/redis/v9client.go Outdated Show resolved Hide resolved

sadath-12 and others added 4 commits December 6, 2023 23:39

azureAD added

2c800c1

Signed-off-by: sadath-12 <[email protected]>

Merge branch 'redis-1' of https://github.com/sadath-12/components-con…

0eb9e36

…trib into redis-1

Merge branch 'main' into redis-1

82c1b54

goimports lint fix

faeab92

Signed-off-by: sadath-12 <[email protected]>

ItalyPaleAle reviewed Dec 18, 2023

View reviewed changes

common/component/redis/redis.go Outdated Show resolved Hide resolved

common/component/redis/redis.go Outdated Show resolved Hide resolved

common/component/redis/redis.go Outdated Show resolved Hide resolved

sadath-12 and others added 3 commits December 19, 2023 22:06

replace deps

e9f7064

Signed-off-by: sadath-12 <[email protected]>

Merge branch 'main' into redis-1

24645ce

Merge branch 'main' into redis-1

b81216b

berndverst requested a review from ItalyPaleAle January 25, 2024 00:06

format

5bb9248

Signed-off-by: sadath-12 <[email protected]>

github-actions bot added the stale label Feb 29, 2024

github-actions bot removed the stale label Feb 29, 2024

berndverst reviewed Feb 29, 2024

View reviewed changes

philon-msft reviewed Feb 29, 2024

View reviewed changes

common/component/redis/redis.go Show resolved Hide resolved

github-actions bot added the stale label Mar 30, 2024

github-actions bot closed this Apr 6, 2024

ItalyPaleAle reopened this Apr 17, 2024

github-actions bot removed the stale label Apr 17, 2024

berndverst added 2 commits April 30, 2024 16:10

Update dapr/kit to the same version used in runtime

fbf5a15

Signed-off-by: Bernd Verst <[email protected]>

Merge branch 'main' into redis-1

7f43880

berndverst requested changes May 2, 2024

View reviewed changes

berndverst force-pushed the main branch from fbf5a15 to 1451363 Compare May 2, 2024 19:12

berndverst mentioned this pull request May 24, 2024

Add support for Azure Cache for Redis in all Redis components #3088

Closed

github-actions bot added the stale label Jun 1, 2024

github-actions bot closed this Jun 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add redis token refresh support #3238

feature: add redis token refresh support #3238

sadath-12 commented Nov 20, 2023 •

edited

Loading

daixiang0 commented Nov 20, 2023 •

edited

Loading

sadath-12 commented Nov 20, 2023

JoshVanL left a comment

sadath-12 commented Nov 20, 2023

sadath-12 commented Nov 20, 2023

ItalyPaleAle commented Nov 20, 2023

sadath-12 commented Nov 20, 2023

sadath-12 commented Dec 6, 2023

berndverst commented Jan 25, 2024

DeepanshuA commented Jan 30, 2024 •

edited

Loading

github-actions bot commented Feb 29, 2024

berndverst commented Feb 29, 2024

berndverst Feb 29, 2024

shpathak-msft commented Feb 29, 2024

github-actions bot commented Mar 30, 2024

github-actions bot commented Apr 6, 2024

mikeee commented Apr 17, 2024

berndverst May 2, 2024

berndverst May 2, 2024

berndverst May 2, 2024

berndverst May 2, 2024

berndverst May 2, 2024

github-actions bot commented Jun 1, 2024

github-actions bot commented Jun 8, 2024

	func (s *Settings) refreshTokenRoutineForRedis(ctx context.Context, redisClient RedisClient, version string, meta map[string]string, logger logger.Logger) {
	func (s *Settings) authenticateWithAzureRedis(ctx context.Context, redisClient RedisClient, version string, meta map[string]string, logger logger.Logger) {

feature: add redis token refresh support #3238

feature: add redis token refresh support #3238

Conversation

sadath-12 commented Nov 20, 2023 • edited Loading

Description

Issue reference

Checklist

daixiang0 commented Nov 20, 2023 • edited Loading

sadath-12 commented Nov 20, 2023

JoshVanL left a comment

Choose a reason for hiding this comment

sadath-12 commented Nov 20, 2023

sadath-12 commented Nov 20, 2023

ItalyPaleAle commented Nov 20, 2023

sadath-12 commented Nov 20, 2023

sadath-12 commented Dec 6, 2023

berndverst commented Jan 25, 2024

DeepanshuA commented Jan 30, 2024 • edited Loading

github-actions bot commented Feb 29, 2024

berndverst commented Feb 29, 2024

berndverst Feb 29, 2024

Choose a reason for hiding this comment

shpathak-msft commented Feb 29, 2024

github-actions bot commented Mar 30, 2024

github-actions bot commented Apr 6, 2024

mikeee commented Apr 17, 2024

berndverst May 2, 2024

Choose a reason for hiding this comment

berndverst May 2, 2024

Choose a reason for hiding this comment

berndverst May 2, 2024

Choose a reason for hiding this comment

berndverst May 2, 2024

Choose a reason for hiding this comment

berndverst May 2, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 1, 2024

github-actions bot commented Jun 8, 2024

sadath-12 commented Nov 20, 2023 •

edited

Loading

daixiang0 commented Nov 20, 2023 •

edited

Loading

DeepanshuA commented Jan 30, 2024 •

edited

Loading