feat: Support configure connections using DNS by hessjcg · Pull Request #843 · GoogleCloudPlatform/cloud-sql-go-connector

hessjcg · 2024-07-16T22:49:07Z

The dialer may be configured to use a DNS name to look up the instance
name instead of configuring the connector with the instance name directly.

Add a DNS TXT record for the Cloud SQL instance to a private DNS server
or a private Google Cloud DNS Zone used by your application. For example:

Record type: TXT
Name: prod-db.mycompany.example.com – This is the domain name used by the application
Value: my-project:region:my-instance – This is the instance connection name

Configure the dialer with the cloudsqlconn.WithDNSResolver() option.

Open a database connection using the DNS name:

db, err := sql.Open(
    "cloudsql-mysql",
    "myuser:mypass@cloudsql-mysql(prod-db.mycompany.example.com)/mydb",
)

Part of #842

enocom · 2024-07-17T15:16:10Z

 	if err != nil {
 		return nil, err
 	}
+	var r NetResolver


r := net.DefaultResolver if cfg.resolver != nil { r = cfg.resolver }

https://go.dev/wiki/CodeReviewComments#indent-error-flow

enocom · 2024-07-17T15:17:20Z

+	cn, err := d.resolveInstanceName(ctx, icn)
 	if err != nil {
-		return nil, err
+		// The connection name was not project:region:instance


I think we don't need this block of code since it's part of resolveInstanceName?

Fixed. I missed this during a refactor.

enocom · 2024-07-17T15:17:50Z

 	return c, nil
 }
+
+func (d *Dialer) queryDNS(ctx context.Context, domainName string) (instance.ConnName, error) {


Since this is a non-trivial method, let's add a godoc (even though it's private) for future readers.

Method comment added.

enocom · 2024-07-17T15:18:47Z

+
+	// If resolve failed and no records were found, return the error.
+	if err != nil {
+		return instance.ConnName{}, fmt.Errorf("unable to resolve SRV record for %s: %v", domainName, err)


%q might be nice here for the SRV record -- it will wrap the value in quotes.

enocom · 2024-07-17T15:20:02Z

 // An Option is an option for configuring a Dialer.
 type Option func(d *dialerConfig)

+// NetResolver groups the methods on net.Resolver that are used by the DNS


If we're adding something to the public API for tests only, we should change our approach (maybe by patching an unexported field).

If on the other hand we think this is a useful interface for callers, we should remove "This allows the default net.Resolver instance to be overridden from tests" and elaborate on why a person might want to provide their own interface.

Also, nit if we're keeping this interface let's move it down to just above where it's used.

Moved to just above the WithResolver() function.

I think this makes sense as a public API. I can imagine a customer keeping their DNS entries in a separate, private DNS server. Then they would need to configure the connector to use that DNS server.

enocom · 2024-07-17T15:23:05Z

+
+#### Configure your DNS Records
+
+Add a DNS SRV record for the Cloud SQL instance to the DNS used by your 


People will do this for public IP. We should warn them that it leaks PII if they do that. This should be recommended for private contexts only.

I added a warning and clarified that they should use private dns.

enocom · 2024-07-17T20:19:54Z

+// application may need to connect to a specific DNS server using a specially
+// configured instance of net.Resolver.
+type NetResolver interface {
+	LookupSRV(ctx context.Context, service, proto, name string) (cname string, addrs []*net.SRV, err error)


The more I think about this, the more I think this is the wrong interface. What we want generically is a way to go from some name (DNS record or key value store path or whatever the user wants) to an instance connection name. This would be more future proof for when we want to support alternate resolution techniques

type InstanceConnectionNameResolver interface { Resolve(ctx context.Context, name string) (string, error) }

Then we could provide a default DNS resolver that does the equivalent of the SRV lookup. I think this is a small change, but will unlock many more use cases (especially as we discover the shortcomings of DNS polling).

Good insight, I agree. The Java implementation started heading in this direction too. I will update the PR accordingly.

hessjcg

I created a new public interface InstanceConnectionNameResolver and an internal implementation called DNSInstanceConnectionNameResolver.

enocom · 2024-07-22T19:09:15Z

+Cloud SQL instances to a public DNS server. This would allow anyone on the
+internet to discover the Cloud SQL instance name. 
+
+For example: suppose you wanted to use the domain name 


"For example, suppose ... to your database instance my-project:region:my-instance, you would create the following DNS record:

Updated. Switching to TXT records.

enocom · 2024-07-22T19:09:56Z

+)
+
+func connect() {
+    cleanup, err := mysql.RegisterDriver("cloudsql-mysql", cloudsqlconn.WithCredentialsFile("key.json"))


I think we should have an explicit opt-in to this DNS feature, e.g.,

WithDNSResolution()

wdyt?

enocom · 2024-07-22T19:11:37Z

 	if err != nil {
 		return nil, err
 	}
+	var r InstanceConnectionNameResolver = cloudsql.DefaultInstanceConnectionNameResolver


What if we used a shorter name, e.g., DefaultResolver? We have a public instance package where we could put this, the interface, and the SRVResolver.

Yes. See review comment.

enocom · 2024-07-22T19:12:33Z


+// InstanceConnectionNameResolver resolves  This allows an application to replace the default
+// DNSInstanceConnectionNameResolver with a custom implementation.
+type InstanceConnectionNameResolver interface {


This should probably go in the instance package next to the code that parses instance connection names. And that way we could just call it Resolver.

Yes. See review comment.

enocom · 2024-07-22T19:12:50Z

+// InstanceConnectionNameResolver resolves  This allows an application to replace the default
+// DNSInstanceConnectionNameResolver with a custom implementation.
+type InstanceConnectionNameResolver interface {
+	Lookup(ctx context.Context, name string) (instanceName instance.ConnName, err error)


Resolver -> Resolve would be more Go-like.

Yes. See review comment.

enocom · 2024-07-22T19:13:52Z

+// net.DefaultResolver with a custom implementation. For example: the
+// application may need to connect to a specific DNS server using a specially
+// configured instance of net.Resolver.
+type netResolver interface {


Could we remove the duplicate interface and test this e2e? Or alternatively, what would it take to assemble a mock DNS server in unit tests?

I think we need a mock for practicality. We need to write test cases covering our code when the DNS records are misconfigured. Also, we need to do this across all our supported connector languages. Mocking the DNS response is the most reliably way to implement these test cases.

hessjcg

I did a bit of refactoring as recommended.

I moved the ConnectionNameResolver interface into the exported instance package and renamed it's method from Lookup() to Resolve()

I created two option methods: WithResolver(r ConnectionNameResolver) and WithDnsResolver().

I configured the default behavior of the dialer to only accept instance names.

hessjcg · 2024-08-02T16:01:06Z

+Cloud SQL instances to a public DNS server. This would allow anyone on the
+internet to discover the Cloud SQL instance name. 
+
+For example: suppose you wanted to use the domain name 


Updated. Switching to TXT records.

hessjcg · 2024-08-02T16:06:00Z

+// net.DefaultResolver with a custom implementation. For example: the
+// application may need to connect to a specific DNS server using a specially
+// configured instance of net.Resolver.
+type netResolver interface {


I think we need a mock for practicality. We need to write test cases covering our code when the DNS records are misconfigured. Also, we need to do this across all our supported connector languages. Mocking the DNS response is the most reliably way to implement these test cases.

hessjcg · 2024-08-02T16:06:35Z

 	if err != nil {
 		return nil, err
 	}
+	var r InstanceConnectionNameResolver = cloudsql.DefaultInstanceConnectionNameResolver


Yes. See review comment.

hessjcg · 2024-08-02T16:06:47Z


+// InstanceConnectionNameResolver resolves  This allows an application to replace the default
+// DNSInstanceConnectionNameResolver with a custom implementation.
+type InstanceConnectionNameResolver interface {


Yes. See review comment.

hessjcg · 2024-08-02T16:06:51Z

+// InstanceConnectionNameResolver resolves  This allows an application to replace the default
+// DNSInstanceConnectionNameResolver with a custom implementation.
+type InstanceConnectionNameResolver interface {
+	Lookup(ctx context.Context, name string) (instanceName instance.ConnName, err error)


Yes. See review comment.

enocom

Looking good. A few more changes to make here.

enocom · 2024-08-05T17:35:58Z

+
+func connect() {
+    cleanup, err := mysql.RegisterDriver("cloudsql-mysql",
+			cloudsqlconn.WithDnsResolver(),


WithDNSResolver()

Also, let's do the equivalent of gofmt on this sample code.

enocom · 2024-08-05T17:39:45Z

+
+func (r *fakeResolver) Resolve(_ context.Context, name string) (instance.ConnName, error) {
+	// For TestDialerSuccessfullyDialsDnsTxtRecord
+	if name == "db.example.com" {


Instead of relying on hardcoded values for particular tests, you could set fields in fakeResolver which makes this more future proof and easy to extend:

type fakeResolve struct { instances map[string]struct{ connName instance.ConnName err error } }

And then this implementation becomes:

func (r *fakeResolver) Resolve(_ context.Context, name string) (instance.ConnName, error) { icn, ok := r.instances[name] if !ok { return instance.ConnName{}, errors.New("not found") } return icn, nil }

This seems excessive. After simplifying obsolete test cases, fakeResolver only ever needs to resolve one domain name.

I think it helps test readability. Otherwise, a person has to go searching for what's special about db.example.com.

If we're not supporting multiple instances anymore this also works:

WithResolver(&fakeResolver{validName: "db.example.com"}),

type fakeResolve struct { validName string } func (r *fakeResolver) Resolve(_ context.Context, name string) (instance.ConnName, error) { if name != r.validName { // return error } // return connection name }

The point is to make the fake extensible -- not just for this use, but for future use.

Actually this is what you do below in the resolver test -- let's do the same thing here.

enocom

This will be a well-liked feature.

The connector may be configured to use a DNS name to look up the instance name instead of configuring the connector with the instance name directly. Add a DNS TXT record for the Cloud SQL instance to a private DNS server or a private Google Cloud DNS Zone used by your application. For example: - Record type: TXT - Name: prod-db.mycompany.example.com – This is the domain name used by the application - Value: my-project:region:my-instance – This is the instance connection name Configure the dialer with the cloudsqlconn.WithDNSResolver() option. Open a database connection using the DNS name: ``` const clientOpts = await connector.getOptions({ domainName: "db.example.com", }); ``` Part of #421 See also: GoogleCloudPlatform/cloud-sql-go-connector#843

hessjcg requested a review from a team as a code owner July 16, 2024 22:49

blunderbuss-gcf Bot assigned jackwotherspoon Jul 16, 2024

hessjcg force-pushed the gh-842-configure-with-dns branch 5 times, most recently from dd0c834 to a04a6f9 Compare July 17, 2024 15:22

enocom requested changes Jul 17, 2024

View reviewed changes

hessjcg force-pushed the gh-842-configure-with-dns branch from a04a6f9 to 87d1d3b Compare July 17, 2024 16:10

hessjcg requested a review from enocom July 17, 2024 16:11

hessjcg force-pushed the gh-842-configure-with-dns branch from 87d1d3b to de69d61 Compare July 17, 2024 17:56

enocom requested changes Jul 17, 2024

View reviewed changes

feat: Automatially configure connections using DNS

4c2b080

hessjcg force-pushed the gh-842-configure-with-dns branch from de69d61 to 4c2b080 Compare July 19, 2024 16:16

hessjcg commented Jul 19, 2024

View reviewed changes

hessjcg requested review from enocom and jackwotherspoon July 19, 2024 16:18

enocom requested changes Jul 22, 2024

View reviewed changes

hessjcg commented Aug 2, 2024

View reviewed changes

hessjcg added 3 commits August 2, 2024 10:50

chore(refactor): Move ConnectionNameResolver to instance package.

8f8138d

WIP: Readme edits for TXT record

a53c120

WIP: changes for commit rr

b6823f8

hessjcg requested a review from enocom August 2, 2024 17:22

hessjcg mentioned this pull request Aug 5, 2024

feat: support Cloud SQL CAS instances. #850

Merged

enocom requested changes Aug 5, 2024

View reviewed changes

wip: eno review changes

3f9ba9d

hessjcg requested a review from enocom August 6, 2024 20:17

wip: more Eno comments

35526b7

enocom changed the title ~~feat: Automatially configure connections using DNS~~ feat: Support configure connections using DNS Aug 8, 2024

enocom approved these changes Aug 8, 2024

View reviewed changes

hessjcg merged commit ec6b3a0 into main Aug 8, 2024

hessjcg deleted the gh-842-configure-with-dns branch August 8, 2024 19:12

release-please Bot mentioned this pull request Aug 8, 2024

chore(main): release 1.12.0 #857

Merged

hessjcg mentioned this pull request Aug 8, 2024

feat: Configure connections using DNS domain names GoogleCloudPlatform/cloud-sql-jdbc-socket-factory#2047

Merged

hessjcg mentioned this pull request Mar 5, 2025

feat: Configure the connector using a DNS name. GoogleCloudPlatform/cloud-sql-nodejs-connector#422

Merged


		#### Configure your DNS Records

		Add a DNS SRV record for the Cloud SQL instance to the DNS used by your

Conversation

hessjcg commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enocom Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hessjcg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hessjcg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enocom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hessjcg commented Jul 16, 2024 •

edited

Loading

enocom Jul 17, 2024 •

edited

Loading