feat: add TLS URL parameters #2076
Conversation
benweissmann
force-pushed
the
bsw/url-tls-options
branch
from
00b356c to
1d70944
Compare
benweissmann
force-pushed
the
bsw/url-tls-options
branch
from
1d70944 to
fae038d
Compare
benweissmann
changed the title
|
|
||
| o.TLSConfig.MinVersion = uint16(q.int("TLSMinVersion")) | ||
| o.TLSConfig.MaxVersion = uint16(q.int("TLSMaxVersion")) | ||
| o.TLSConfig.InsecureSkipVerify = q.bool("TLSInsecureSkipVerify") |
There was a problem hiding this comment.
Are these options TLSMinVersion/TLSMaxVersion/TLSInsecureSkipVerify documented anywhere?
|
Before merging this, we should check if these or similar options are already used anywhere. If not, we should consider using |
|
@benweissmann @vmihailenco Any updates on this ? |
|
@benweissmann @vmihailenco, any ETA for these changes to be released? |
|
This pull request is marked stale. It will be closed in 30 days if it is not updated. |
|
Is it really too much to ask to merge this PR? I think having TLS options is a no-brainer. |
|
@vlasky the option names in the PR are quite inconsistent with the rest of the option names which are |
There was a problem hiding this comment.
The option names in the PR are quite inconsistent with the rest of the option names which are snake_case and the ones in the pr are CamelCase. In the same time, the option ServerName is introduced without any meaningful prefix, which may lead users to think this is the actual ServerName that will be used for connecting to redis. Overall, this should be reworked. @benweissmann let me know if you would like to continue working on this or we can close this PR
Building on Ben Weissmann's original implementation, this commit adds: - Snake_case parameter names (addressing reviewer feedback): * tls_cert_file and tls_key_file (instead of TLSCertPEMFile/TLSKeyPEMFile) * tls_min_version and tls_max_version (instead of TLSMinVersion/TLSMaxVersion) * tls_server_name (instead of ServerName) - Improved error messages for better user experience - Updated test cases to use snake_case parameters - Removed redundant tls_insecure_skip_verify (use existing skip_verify) - Enhanced documentation with clear parameter descriptions This addresses all reviewer feedback from PR #2076 while maintaining the core functionality and comprehensive test coverage.
|
Hi @benweissmann , Thanks again for your work! |
Fixes #2024