Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Cache] phpredis: Added full TLS support for RedisCluster#40882

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
jackthomasatl wants to merge0 commits intosymfony:4.4fromjackthomasatl:feature/RedisCluster-tls
Closed

[Cache] phpredis: Added full TLS support for RedisCluster#40882

jackthomasatl wants to merge0 commits intosymfony:4.4fromjackthomasatl:feature/RedisCluster-tls

Conversation

@jackthomasatl
Copy link
Contributor

@jackthomasatljackthomasatl commentedApr 20, 2021•
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets-
LicenseMIT
Doc PRn/a

This Pr bridges the gap for full TLS support when using phpredis driver implementation of TLS.

Adds the 'ssl' options array for cache configuration when using RedisCluster
https://www.php.net/manual/en/context.ssl.php

Switches directed node commands from using individual \Redis connections to using the recommended implementation from the phpredis documentation:
https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#directed-node-commands

This pr will enable compatibility with Amazon ElastiCache redis cluster mode using In Transit encryption (TLS) using the phpredis driver, Supports tagging & binary data types.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@jackthomasatljackthomasatl changed the titleAdded RedisClusterNodeProxyphpredis: Added full TLS support for RedisClusterApr 20, 2021
Copy link
Member

@jderussejderusse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do you think it's possible to add test for this?

I'm not sureRedisClusterNodeProxy is related to adding tls support?

@jackthomasatl
Copy link
ContributorAuthor

Sure. I think I can come up with tests.

The issue this is addressing is that when you're using redis cluster, and getHosts gets called, it makes a new Redis object for each redis cluster node and runs commands intended just for that node via this secondary connection.

The problem with that, is that when you call _masters on a RedisCluster, it does not indicate if tls is used or not. The secondary connections will be setup without tls. I've looked over the phpredis code and it does not expose in any way if its using tls or not.

The easy fix for this is to just reuse the connections that RedisCluster already has to each node, using the method described in the phpredis documentation.

RedisClusterNodeProxy provides a structure to encapsulate the method calls to \RedisCluster using the same interface that \Redis presents.

@nicolas-grekasnicolas-grekas added this to the4.4 milestoneApr 21, 2021
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good catch. Can you please rebase+target 4.4? This is a bugfix.
Here are some comments before merging.

@carsonbotcarsonbot changed the titlephpredis: Added full TLS support for RedisCluster[Cache] phpredis: Added full TLS support for RedisClusterApr 21, 2021
@jackthomasatljackthomasatl changed the base branch from5.x to4.4April 22, 2021 03:10
@jackthomasatl
Copy link
ContributorAuthor

Rebased against 4.4

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the smooth cooperation, that's a nice fix, I learned a few things about phpredis :)

@jackthomasatl
Copy link
ContributorAuthor

Very welcome!

@jackthomasatl
Copy link
ContributorAuthor

@nicolas-grekas What about psalm? I'm not sure what its upset about.

@nicolas-grekas
Copy link
Member

psalm is drunk here, it can be ignored :)

@jackthomasatl
Copy link
ContributorAuthor

What is remaining?

@jackthomasatl
Copy link
ContributorAuthor

Can someone approve workflows?

nicolas-grekas reacted with thumbs up emoji

@jackthomasatl
Copy link
ContributorAuthor

I switched the default value for ssl back to null. I start seeing client disconnects / timeouts when the value provided is an empty array. In the C++ code its default null. I think the presence of the array at all forces the driver into ssl mode.

nicolas-grekas reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Thank you@jackthomasatl.

nicolas-grekas added a commit that referenced this pull requestApr 23, 2021
…(jackthomasatl)This PR was squashed before being merged into the 4.4 branch.Discussion----------[Cache] phpredis: Added full TLS support for RedisCluster| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | n/aThis Pr bridges the gap for full TLS support when using phpredis driver implementation of TLS.Adds the 'ssl' options array for cache configuration when using RedisClusterhttps://www.php.net/manual/en/context.ssl.phpSwitches directed node commands from using individual \Redis connections to using the recommended implementation from the phpredis documentation:https://github.com/phpredis/phpredis/blob/develop/cluster.markdown#directed-node-commandsThis pr will enable compatibility with Amazon ElastiCache redis cluster mode using In Transit encryption (TLS) using the phpredis driver, Supports tagging & binary data types.Commits-------a1e0408 [Cache] phpredis: Added full TLS support for RedisCluster
@nicolas-grekas
Copy link
Member

(dunno why github displays this has having no commits, but the merge is confirmed inf8518ca)

@jackthomasatl
Copy link
ContributorAuthor

Ah, I did an empty merge commit trying to trigger the test suite to run again, it was stuck.

This was referencedMay 1, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@jderussejderussejderusse left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@jackthomasatl@carsonbot@nicolas-grekas@stof@jderusse
[8]ページ先頭
©2009-2025 Movatter.jp