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

Removed endpoint IP validation as it can also take hostnames #10

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ribejara-te
Copy link

Hi, while using this plugin we noticed it only takes IP addresses as endpoints, but we need to use a hostname to point it to a Redis cluster with a dynamic IP address.

This is strictly caused by validation. The plugin validates whether the endpoint is a valid IP address, but it doesn't really need that as the underlying radix client takes hostnames too.

The radix client ultimately calls net.Dial which says:

The host must be a literal IP address, or a host name that can be resolved to IP addresses.

As such, I have removed endpoint IP validation to allow for hostnames.

I considered adding hostname validation but that's a completely different beast, and I don't want to get into that. If the user does something wrong, they'll figure it out during runtime a few seconds later anyway.

Thanks!

@ribejara-te
Copy link
Author

Actually, this fails for two reasons (1) h declared but not used and (2) a test I removed, but with the GitHub incident my commits show up if I clone the repo, but don't appear on either the repo's web UI nor this PR.

Will wait for them to fix it otherwise I'll re-raise the PR.

@miekg
Copy link
Owner

miekg commented May 17, 2023 via email

@ribejara-te
Copy link
Author

ribejara-te commented May 18, 2023

The reason we needed this is so that we can plug this into AWS Elasticache. Elasticache Redis gives you a name, which resolves to the many IP addresses of your cluster's nodes. Without name support, this plugin is limited to extremely simple Redis deployments.

We ended up writing our own external cache plugin with the much newer go-redis/v9 client, which supports many of the things we needed to do this natively.

I can finish it up though, if you think it's worth it. I think it is.

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