Skip to content

feat: add TLS URL parameters #2076

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: add TLS URL parameters #2076

wants to merge 1 commit into from

Conversation

benweissmann
Copy link

Fixes #2024

@benweissmann benweissmann changed the title Add TLS URL parameters feat: Add TLS URL parameters Apr 23, 2022
@benweissmann benweissmann changed the title feat: Add TLS URL parameters feat: add TLS URL parameters Apr 25, 2022
vmihailenco
vmihailenco reviewed Jun 4, 2022
View reviewed changes
options.go

o.TLSConfig.MinVersion = uint16(q.int("TLSMinVersion"))
o.TLSConfig.MaxVersion = uint16(q.int("TLSMaxVersion"))
o.TLSConfig.InsecureSkipVerify = q.bool("TLSInsecureSkipVerify")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these options TLSMinVersion/TLSMaxVersion/TLSInsecureSkipVerify documented anywhere?

@vmihailenco
Copy link
Collaborator

Before merging this, we should check if these or similar options are already used anywhere. If not, we should consider using ?tls_min_version instead of ?TLSMinVersion.

@vmihailenco vmihailenco added the wait Can’t be processed temporarily for other reasons label Oct 6, 2022
@gaby gaby mentioned this pull request Sep 10, 2023
Closed
3 tasks
@gaby
Copy link

gaby commented Sep 10, 2023

@benweissmann @vmihailenco Any updates on this ?

@gaby gaby mentioned this pull request Sep 10, 2023
Open
@chayim chayim requested a review from ofekshenawa September 10, 2023 07:11
@apurvasaraiya
Copy link

@benweissmann @vmihailenco, any ETA for these changes to be released?

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 7, 2025

This pull request is marked stale. It will be closed in 30 days if it is not updated.

@github-actions github-actions bot added the Stale label Jul 7, 2025
@vlasky
Copy link

vlasky commented Jul 21, 2025

Is it really too much to ask to merge this PR? I think having TLS options is a no-brainer.

@github-actions github-actions bot removed the Stale label Jul 22, 2025
@ndyakov
Copy link
Member

ndyakov commented Aug 5, 2025

@vlasky 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. If you are up for it, please feel free to open a PR.

ndyakov
ndyakov requested changes Aug 5, 2025
View reviewed changes
Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vmihailenco vmihailenco vmihailenco left review comments

@ndyakov ndyakov ndyakov requested changes

@ofekshenawa ofekshenawa Awaiting requested review from ofekshenawa

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
wait Can’t be processed temporarily for other reasons
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support specifying TLS options in connection string
6 participants
@benweissmann @vmihailenco @gaby @apurvasaraiya @vlasky @ndyakov