-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix dns service SRV lookup when service address is a fqdn #6283
Conversation
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.
Overall this PR is amazing. The DNS code has been in need of breaking up into more isolated functions for some time so thank you for taking that on.
I have a number of comments and questions inline but one big request is that this PR be rebased against the release/1-6 branch. Not a whole lot has changed with the DNS server in that branch but there is one fundamental addition of service level tagged addressing which would definitely be relevant for this PR.
In general pretty much anywhere that you would access the node address you should call d.agent.TranslateAddress(dc, <node address>, <node tagged addresses>)
, when you need the service address you use d.agent.TranslateServiceAddress(dc, <service address>, <service tagged addresses>)
and when you need the port (such as for SRV RRs) you can call d.agent.TranslateServicePort(dc, <service port>, <service tagged addresses>)
.
@tionebsalocin gentle ping - would you be able to make the changes that came up during our review? |
@i0rek, @tionebsalocin is on vacation rn but we will look at this soon |
@mkeeler Should I create a new PR on release/1-6 instead? |
@tionebsalocin You should be able to edit the PR and change the target branch to release/1-6. No need for a separate PR. |
I just went ahead and retargeted myself. |
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 looks like the rebase may have lost a few things. There are replies where you said you updated things but the update doesn't seem to be there. Additionally there is one location where some git conflict stuff is still lingering.
@tionebsalocin Possible to have a look to fix this? |
63eb163
to
f9ee972
Compare
9792f26
to
ce51547
Compare
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.
Just a few more comments but this is super close to being ready to merge.
ce51547
to
8572ac1
Compare
@mkeeler @pierresouchay A new PR has been created to implement requested fixes (#6792 ) |
Hey there, This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days. If you are still experiencing problems, or still have questions, feel free to open a new one 👍. |
Refactor dns to have same behavior between A and SRV.
Current implementation returns the node name instead of the service
address.
With this fix when querying for SRV record service address is return in
the SRV record.
And when performing a simple dns lookup it returns a CNAME to the
service address.
(continuation of #6252)
Huge list of commits due to rebase on release/1-6