Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Redis client (v9) and add TLS certificates #4979

Closed
wants to merge 1 commit into from
Closed

Update Redis client (v9) and add TLS certificates #4979

wants to merge 1 commit into from

Conversation

Lajule
Copy link

@Lajule Lajule commented Apr 26, 2023

Hi,

This PR contains an upgrade of the Redis client from v8 to v9 and some new config variables in order to specify TLS certificates (including CA) for the client.

Description

We use Redis 7.0.11 in cluster mode with TLS, to make Tyk works we add some config variables to provide TLS certificats to the TLSConfig of the Redis client in addition we have updated the client to the lasted version.

Related Issue

Could not connect to Redis in cluster mode with TLS

Motivation and Context

For security reasons we have to use TLS in every connections of ours infrastructure, that why we made this modifications

How This Has Been Tested

We build the container and test it directly in our test environment

Screenshots (if appropriate)

NA

Types of changes

  • [ x] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

@buger buger requested review from tbuchaillot and titpetric April 26, 2023 14:09
@titpetric
Copy link
Contributor

It used to be that v9 had breaking changes in the redis protocol: there is still an issue open, redis/go-redis#2152 (comment) but it seems that RESP2 support has been added and the client promoted to official meanwhile.

The PR is a strong merge candidate now that we upgraded to 1.19 and can bump the dependency for redis, along with #4211. That PR contains updates that improve error returns and enable custom storage drivers.

Apart from removing log.Info around ping to not log the expected value, the changes are not objectionable. 👍😊

@Lajule Lajule closed this by deleting the head repository Apr 26, 2023
@titpetric
Copy link
Contributor

@Lajule closed in error? The cert support imho is viable and we can include it. My comment was meant as your PR has value + the wip work has even more improvements. Happy to evaluate a merge here.

@Lajule
Copy link
Author

Lajule commented Apr 26, 2023

I'll reopen one with certificates only,
Comming in few hours

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants