search

skynetservices / skydns1

DNS for skynet or any other service discovery
MIT License
528 stars 54 forks source link

Skydns is case sensitive #82

Closed crosbymichael closed 10 years ago

crosbymichael commented 10 years ago

Queries with difference case fail with skydns.

ping @miekg

crosbymichael commented 10 years ago

Any idea of a good place to make this change?

erikstmartin commented 10 years ago

Looks like an oversight, when the key to store it is created everything is lowercased https://github.com/skynetservices/skydns/blob/master/registry/registry.go#L432

but when the search is performed it takes the casing as requested. The best i'd recommend making the change is https://github.com/skynetservices/skydns/blob/master/registry/registry.go#L223

If strings.ToLower(domain) is called at the beginning of the function that should solve the issue you're seeing.

I can make that change here in a few mins if you'd like

crosbymichael commented 10 years ago

Thanks for fixing this so quickly.

erikstmartin commented 10 years ago

No problem, as soon as I saw the email I knew exactly what it was, just had to wait to be near a computer to fix it :)

crosbymichael commented 10 years ago

@erikstmartin it looks like the domain still needs to be checked for case. redis.dev.docKer fails

erikstmartin commented 10 years ago

Oh you're right. This logic takes place after the anchor domain has been trimmed off. I'll reopen this and i'll get a fix in for that as well.

erikstmartin commented 10 years ago

Should be all set now

miekg commented 10 years ago

Lgtm, I'll double check the dns package. On Mar 28, 2014 3:57 AM, "Erik St. Martin" notifications@github.com wrote:

Should be all set now

— Reply to this email directly or view it on GitHubhttps://github.com/skynetservices/skydns/issues/82#issuecomment-38886851 .

bketelsen commented 10 years ago

@miekg feel free to merge when you're happy.