Allow for local dns resolution with a custom dialer #121

stephaniehingtgen · 2023-06-26T19:56:13Z

If the custom dialer updates the transport to proxy the mssql connection, and only the proxy can reach the network that mssql is running in, then any local DNS resolution will fail.

To address this, this PR ensures the custom dialer is used, if defined, to resolve the DNS.

We've been using this logic on Grafana's forked repo for the past few months.

codecov-commenter · 2023-06-26T20:24:20Z

Codecov Report

Merging #121 (d376e9e) into main (dd3e503) will decrease coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
- Coverage   74.11%   73.86%   -0.26%     
==========================================
  Files          29       29              
  Lines        5613     5620       +7     
==========================================
- Hits         4160     4151       -9     
- Misses       1206     1216      +10     
- Partials      247      253       +6

Impacted Files	Coverage Δ
mssql.go	`86.27% <ø> (ø)`
protocol.go	`94.05% <100.00%> (+0.44%)`	⬆️

... and 2 files with indirect coverage changes

shueybubbles · 2023-06-26T21:27:31Z

protocol.go

@@ -51,6 +51,14 @@ func (t tcpDialer) DialSqlConnection(ctx context.Context, c *Connector, p *msdsn
 	var ips []net.IP
 	ip := net.ParseIP(p.Host)
 	if ip == nil {
+		// if the dialer has been updated, the dialer may be proxying to a different network, and so the
+		// dialer should be used to connect so the DNS is resolved within the right network
+		if c != nil && c.Dialer != nil {


&& c.Dialer != nil {

might there be value in defining an interface to determine if the custom dialer handles DNS lookups? Do we know that all of them do?

My initial thought is that if the custom dialer sends the final request to a different network, that they would want to use the DNS lookup where the actual requests will later be sent to.

For example, if the driver is being run in a k8s namespace that has a MSSQL instance with the local DNS of mssql, but we want to proxy the request to a separate MSSQL instance in another cluster with the local DNS of mssql2, we'd want the DNS resolution to happen on the other k8s cluster, otherwise we may get false positives that the DNS is correct if we are using mssql, when it won't work (Or vice versa if the DNS is switched):

But if there is a use case for it, I'd be happy to implement this.

I don't think we're in disagreement about the end result. I'm asking if there are multiple types of custom dialers - some that proxy to other networks and some that rely on the current DNS lookup and if we'd need to distinguish between the two. EG, your dialer is clearly of the former. Anyone using the existing model, though, is getting the DNS lookup "for free" and we risk breaking them.
Maybe a better name for Dialer would have been IPDialer since the driver has always called it with an IP address up to now.
Your dialer could be HostDialer which takes a host name.
eg

h, ok := c.Dialer.(HostDialer)

Sounds good! I'll work on implementing the second HostDialer interface

protocol.go

shueybubbles · 2023-06-26T21:35:53Z

thank you for submitting a PR!

stephaniehingtgen · 2023-06-27T18:19:58Z

@microsoft-github-policy-service agree company="Grafana"

shueybubbles

Please update the README and the CHANGELOG. Once I get a second approver we will merge.
thx!

apoorvdeshmukh

@stephaniehingtgen Can you look into the test failure?

stephaniehingtgen · 2023-06-29T21:49:32Z

@shueybubbles updated the README & Changelog! Thanks for the review!

@apoorvdeshmukh I updated the unit test, but I'm unable to kick off running the tests. Would you be able to kick it off for me?

stephaniehingtgen · 2023-07-10T18:38:14Z

Hey! Just wanted to check if there is anything I need to do here to get this merged. Thanks!

shueybubbles · 2023-07-10T19:12:49Z

@apoorvdeshmukh it's waiting for you to approve then we can merge

apoorvdeshmukh

LGTM! Thanks for the fix!

Allow for local dns resolution with a custom dialer

fd2dcf7

shueybubbles requested review from apoorvdeshmukh, stuartpa and JyotikaGargg June 26, 2023 21:24

shueybubbles reviewed Jun 26, 2023

View reviewed changes

protocol.go Outdated Show resolved Hide resolved

stephaniehingtgen requested a review from shueybubbles June 27, 2023 18:40

Use a new dialer type

a9be1e0

shueybubbles approved these changes Jun 28, 2023

View reviewed changes

apoorvdeshmukh requested changes Jun 28, 2023

View reviewed changes

stephaniehingtgen added 2 commits June 29, 2023 16:03

fix unit test

9a3698b

Add changelog & readme

d376e9e

stephaniehingtgen requested a review from apoorvdeshmukh June 29, 2023 21:49

apoorvdeshmukh approved these changes Jul 11, 2023

View reviewed changes

shueybubbles merged commit 45ecf99 into microsoft:main Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for local dns resolution with a custom dialer #121

Allow for local dns resolution with a custom dialer #121

stephaniehingtgen commented Jun 26, 2023 •

edited

Loading

codecov-commenter commented Jun 26, 2023 •

edited

Loading

shueybubbles Jun 26, 2023

stephaniehingtgen Jun 27, 2023

shueybubbles Jun 27, 2023

stephaniehingtgen Jun 27, 2023

stephaniehingtgen Jun 27, 2023

shueybubbles commented Jun 26, 2023

stephaniehingtgen commented Jun 27, 2023

shueybubbles left a comment

apoorvdeshmukh left a comment

stephaniehingtgen commented Jun 29, 2023

stephaniehingtgen commented Jul 10, 2023

shueybubbles commented Jul 10, 2023

apoorvdeshmukh left a comment

Allow for local dns resolution with a custom dialer #121

Allow for local dns resolution with a custom dialer #121

Conversation

stephaniehingtgen commented Jun 26, 2023 • edited Loading

codecov-commenter commented Jun 26, 2023 • edited Loading

Codecov Report

shueybubbles Jun 26, 2023

Choose a reason for hiding this comment

stephaniehingtgen Jun 27, 2023

Choose a reason for hiding this comment

shueybubbles Jun 27, 2023

Choose a reason for hiding this comment

stephaniehingtgen Jun 27, 2023

Choose a reason for hiding this comment

stephaniehingtgen Jun 27, 2023

Choose a reason for hiding this comment

shueybubbles commented Jun 26, 2023

stephaniehingtgen commented Jun 27, 2023

shueybubbles left a comment

Choose a reason for hiding this comment

apoorvdeshmukh left a comment

Choose a reason for hiding this comment

stephaniehingtgen commented Jun 29, 2023

stephaniehingtgen commented Jul 10, 2023

shueybubbles commented Jul 10, 2023

apoorvdeshmukh left a comment

Choose a reason for hiding this comment

stephaniehingtgen commented Jun 26, 2023 •

edited

Loading

codecov-commenter commented Jun 26, 2023 •

edited

Loading