-
Notifications
You must be signed in to change notification settings - Fork 170
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
[resty.resolver] support ipv6 resolvers #511
Conversation
ce246d5
to
438c327
Compare
Hi @mikz ! I had a look and it seems to work on my machine ! 👍 Just one side note: this time there is no port at the end of the IPv6 address in my If I do the test with the ':53' at the end it does not seems correct.
Otherwise, without the port at the end of the IPv6 address, it works.
|
@nmasse-itix I think the |
@nmasse-itix thanks for the thorough investigation! And sorry it broke. Unfortunately IPv6 is not as widespread as it should be. Hopefully we can merge this soon. |
t/resolver.t
Outdated
|
||
=== TEST 3: can have ipv6 RESOLVER | ||
RESOLVER env variable can be IPv6 address | ||
--- ONLY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah! I'm going to fix the CI to catch this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. And verified it was failing.
4344391
to
c367185
Compare
gateway/src/resty/resolver.lua
Outdated
-- see https://github.com/3scale/apicast/issues/321 for more details | ||
local m | ||
|
||
m, err = re_match(resolver, '^(.+?)(?<!:)(?:\\:(\\d+))?$', 'oj') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we extract the regex to a constant, readers will be able to know what we need to match here without understanding the regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It matches anything optionally followed by just one :
and a series of numbers.
I'm not even sure how to call it :)
It needs to match 1.2.3.4:53
and [dead::beef]:53
but not dead::beef::53
.
Actually I see there is a bug as it matches the 3rd example as a port. http://rubular.com/r/j6rm3ObFIM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to extended regex with comments 9226b0b
9226b0b
to
1df4cf4
Compare
just print the most critical errors so we don't see ngx.log statements when running busted
* the second test does not need it * and init_worker can get variables set by the env directive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @mikz 👍
Thanks for your help @nmasse-itix !
Fixes #510
@nmasse-itix do you mind trying it out?
TODO