Improve Redis configuration options to allow the use of non default user or connection string

[dimitar-hristov]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
• If you are interested in working on this issue or have submitted a pull request, please leave a comment.

---

• I'd be willing to implement this feature (contributing guide)

Describe the user story

As a developer, I would like to make use of the Redis ACL functionality which allows the use of non default users. In addition, I would like to be able to use connection strings instead of configurations object.

Atlantis uses the redis-go client, and as described in the Redis docs here, it suggests that we can either use options to configure a client on our own or use a connections string which is then parsed as can be seen here.

In Atlantis, the logic that creates the redis client starts here and then initialises the client here where it passes the following options.

rdb = redis.NewClient(&redis.Options{
		Addr:      fmt.Sprintf("%s:%d", hostname, port),
		Password:  password,
		DB:        db,
		TLSConfig: tlsConfig,
	})

As can be seen here, it doesn’t even have the username as an input option.

Then in the redis-go client, we create the initialise the new client here and create a new connection here. And ultimately, it ends up calling the methods here depending on redis version and if username is present. And when username is not present, it will use the default one as can be seen here (for Redis 6.0 and higher). This is also confirmed by the ACL docs

Here’s an example of the old form:
AUTH <password>
What happens is that the username used to authenticate is “default”, so just specifying the password implies that we want to authenticate against the default user. This provides backward compatibility.

Describe the solution you'd like

Extend the logic here to:

• Include the Redis username as an input option. Then extend the way we create Redis client to allow us to pass username.
• Allow for connection string to be passed as an input. Then parse it using the ParseURL method.

Describe the drawbacks of your solution

• More Redis logic to maintain within Atlantis.

Describe alternatives you've considered

• Use the current approach where we can only connect via the default user.

[marceloboeira]

@dimitar-hristov are you working on this one?

I think it is a low-friction one, I can take a shot. it would be great to get some community feedback for the "behavior".

Considering how most mature open-source projects deal with this, I'd love to delegate all options to .ParseUrl since it is safer, cleaner & easier to grasp.

e.g.: (from redis go docs)

--redis-url="redis://user:password@my.cloud.provider.com:6789/3?dial_timeout=3&read_timeout=6s&max_retries=2"

Instead of the more verbose:

--redis-host="my.cloud.provider.com" \
--redis-port=6789 \
--redis-user=user  \ 
--redis-password=pass \
--redis-db=3 \
--redis-dial-timeout="3s" \
--redis-read-timeout="6s"
--redis-max-retries=2
...

Or even via env:

ATLANTIS_REDIS_HOST="my.cloud.provider.com" 
ATLANTIS_REDIS_PORT=6789 
ATLANTIS_REDIS_USER=user
ATLANTIS_REDIS_PASSWORD=pass
ATLANTIS_REDIS_DB=3
ATLANTIS_REDIS_DIAL_TIMEOUT="3s"
ATLANTIS_REDIS_READ_TIMEOUT="6s"
ATLANTIS_REDIS_MAX_RETRIES=2
...

IMHO we should converge to a single option to avoid complex logics but also I don't want to break compatibility right away. So maybe the overall solution progression would go as follows:

• minor release supporting both options

  • when --redis-url is sent then everything else is ignored (warning/deprecation message is shown)
  • when --redis-url is NOT sent then it works as today

• another minor release deprecating the legacy

  • when --redis-url is sent then redis is configured with.
  • when --redis-url is NOT sent then redis is NOT configure

WDYT?

[jamengual]

yes , that would work.

…

On Tue, Aug 22, 2023, 7:23 a.m. MB ***@***.***> wrote: @dimitar-hristov <https://github.com/dimitar-hristov> are you working on this one? I think it is a low-friction one, I can take a shot. it would be great to get some community feedback for the "behavior". Considering how most mature open-source projects deal with this, I'd love to delegate all options to .ParseUrl since it is safer, cleaner & easier to grasp. e.g.: (from redis go docs) ***@***.***:6789/3?dial_timeout=3&read_timeout=6s&max_retries=2" Instead of the more verbose --redis-host="my.cloud.provider.com" \ --redis-port=6789 --redis-user=user \ --redis-password=pass \ --redis-db=3 \ --redis-dial-timeout=3 \ --redis-read-timeout=6s --redis-max-retries=2 ... IMHO we should converge to a single option to avoid complex logics but also I don't want to break compatibility right away. So maybe the overall solution progression would go as follows: - *minor release supporting both options* - when --redis-url is sent then everything else is ignored (warning/deprecation message is shown) - when --redis-url is NOT sent then it works as today - *another minor release deprecating the legacy* - when --redis-url is sent then redis is configured with. - when --redis-url is NOT sent then redis is NOT configure WDYT? — Reply to this email directly, view it on GitHub <#3610 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERDJU4Q76YSBZAZD3PTXWS6F7ANCNFSM6AAAAAA2NO2KOE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[dimitar-hristov]

Hey 👋 Yes, makes sense to me. I haven't implemented anything yet as I wanted to get some community feedback for the behavior as you said.

For the specific implementation, I was thinking of supporting both - the more verbose and less verbose options for backwards compatibility reasons. But if we think this will only introduce confusion, we can stick to only one of them and carefully introduce the breaking change.

If you already have work in progress, just open a PR and I can help with the reviews. Otherwise, I will try to find some spare time to implement it.

[marceloboeira]

@dimitar-hristov I haven't started it yet either, feel free to push it forward. My only concern with supporting both is precedence if folks send both... that's the confusing part (which one overrides the other?). If that's clear documentation wise I don't see much problem as everything else at this point is offered with the verbose option.