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.

Sign up for GitHub

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

Allow for local dns resolution with a custom dialer #121

Merged
merged 4 commits into from
Jul 11, 2023
Merged

Allow for local dns resolution with a custom dialer #121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd2dcf7
Allow for local dns resolution with a custom dialer
stephaniehingtgen Jun 26, 2023
a9be1e0
Use a new dialer type
stephaniehingtgen Jun 27, 2023
9a3698b
fix unit test
stephaniehingtgen Jun 29, 2023
d376e9e
Add changelog & readme
stephaniehingtgen Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions protocol.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& 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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Screenshot 2023-06-27 at 1 25 46 PM

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added :)

stephaniehingtgen marked this conversation as resolved.
Show resolved Hide resolved
d := c.getDialer(p)
addr := net.JoinHostPort(p.Host, strconv.Itoa(int(resolveServerPort(p.Port))))
return d.DialContext(ctx, "tcp", addr)
}

ips, err = net.LookupIP(p.Host)
if err != nil {
return
Expand Down
41 changes: 41 additions & 0 deletions tds_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -971,3 +971,44 @@ func versionToHexString(v uint32) string {
binary.LittleEndian.PutUint32(b, v)
return hex.EncodeToString(b)
}

func TestDialSqlConnectionCustomDialer(t *testing.T) {
tl := testLogger{t: t}
defer tl.StopLogging()
SetLogger(&tl)

params := msdsn.Config{
Host: "nonexistant-dns.svc.cluster.local",
}
connector, err := NewConnector(params.URL().String())
if err != nil {
t.Error(err)
}

ctx, cancel := context.WithTimeout(context.Background(), time.Second*30)
defer cancel()

// if a dialer is specified, the dialer should be used to make the connection
mock := NewMockTransportDialer(
[]string{},
[]string{},
)
connector.Dialer = mock
if mock.count != 0 {
t.Error("expecting no connections")
}
sqlDialer, _ := msdsn.ProtocolDialers["tcp"].(MssqlProtocolDialer)
conn, err := sqlDialer.DialSqlConnection(ctx, connector, &params)
if err != nil {
t.Error(err)
}

if mock.count != 1 {
t.Error("expecting 1 connection")
}

err = conn.Close()
if err != nil {
t.Error(err)
}
}