fix: return error with conflicting credential opts

enocom · enocom · commit 6a033d83012c · 2025-08-23T12:54:02.000-06:00
This commit ensures conflicting credential options result in a user-facing error. This commit moves all the dialer config initialization into a single function in options.go. This will help the migration to supporting auth.Credentials and makes the relationship between all the options clear. Previously, there was a mix of option functions setting various fields, possibly returning errors, and the dialer initializer doing additional configuration work. This made it hard to understand how the different credential options related to one another and hid the bug of allowing callers to set conflicting options. In addition, this commit adds end to end tests to verify the credential-related options work as intended. This is a belated port of GoogleCloudPlatform/cloud-sql-go-connector#460. Related to #646
diff --git a/credentials_test.go b/credentials_test.go
@@ -0,0 +1,131 @@
+// Copyright 2025 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package alloydbconn_test
+
+import (
+	"context"
+	"fmt"
+	"net"
+	"os"
+	"testing"
+	"time"
+
+	"cloud.google.com/go/alloydbconn"
+	"github.com/jackc/pgx/v5/pgxpool"
+	"golang.org/x/oauth2"
+	"golang.org/x/oauth2/google"
+)
+
+// removeAuthEnvVar retrieves an OAuth2 token and a path to a service account key
+// and then unsets GOOGLE_APPLICATION_CREDENTIALS. It returns a cleanup function
+// that restores the original setup.
+func removeAuthEnvVar(t *testing.T) (*oauth2.Token, string, func()) {
+	ts, err := google.DefaultTokenSource(context.Background(),
+		"https://www.googleapis.com/auth/cloud-platform",
+	)
+	if err != nil {
+		t.Errorf("failed to resolve token source: %v", err)
+	}
+
+	tok, err := ts.Token()
+	if err != nil {
+		t.Errorf("failed to get token: %v", err)
+	}
+	path, ok := os.LookupEnv("GOOGLE_APPLICATION_CREDENTIALS")
+	if !ok {
+		t.Fatalf("GOOGLE_APPLICATION_CREDENTIALS was not set in the environment")
+	}
+	if err := os.Unsetenv("GOOGLE_APPLICATION_CREDENTIALS"); err != nil {
+		t.Fatalf("failed to unset GOOGLE_APPLICATION_CREDENTIALS")
+	}
+	return tok, path, func() {
+		os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", path)
+	}
+}
+
+func keyfile(t *testing.T) string {
+	path := os.Getenv("GOOGLE_APPLICATION_CREDENTIALS")
+	if path == "" {
+		t.Fatal("GOOGLE_APPLICATION_CREDENTIALS not set")
+	}
+	creds, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("io.ReadAll(): %v", err)
+	}
+	return string(creds)
+}
+
+func TestPostgresAuthentication(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping credential integration tests")
+	}
+	requireAlloyDBVars(t)
+	creds := keyfile(t)
+
+	tok, path, cleanup := removeAuthEnvVar(t)
+	defer cleanup()
+
+	tcs := []struct {
+		desc string
+		opt alloydbconn.Option
+	}{
+		{
+			desc: "with token",
+			opt: alloydbconn.WithTokenSource(oauth2.StaticTokenSource(tok)),
+		},
+		{
+			desc: "with credentials file",
+			opt: alloydbconn.WithCredentialsFile(path),
+		},
+		{
+			desc: "with credentials JSON",
+			opt: alloydbconn.WithCredentialsJSON([]byte(creds)),
+		},
+	}
+	for _, tc := range tcs {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx := context.Background()
+
+			d, err := alloydbconn.NewDialer(ctx, tc.opt, alloydbconn.WithIAMAuthN())
+			if err != nil {
+				t.Fatalf("failed to init Dialer: %v", err)
+			}
+			defer d.Close()
+
+			dsn := fmt.Sprintf("user=%s dbname=%s sslmode=disable", alloydbIAMUser, alloydbDB)
+			config, err := pgxpool.ParseConfig(dsn)
+			if err != nil {
+				t.Fatalf("failed to parse pgx config: %v", err)
+			}
+
+			config.ConnConfig.DialFunc = func(ctx context.Context, _ string, _ string) (net.Conn, error) {
+				return d.Dial(ctx, alloydbInstanceName)
+			}
+
+			pool, err := pgxpool.NewWithConfig(ctx, config)
+			if err != nil {
+				t.Fatalf("failed to create pool: %s", err)
+			}
+			defer pool.Close()
+
+			var now time.Time
+			err = pool.QueryRow(context.Background(), "SELECT NOW()").Scan(&now)
+			if err != nil {
+				t.Fatalf("QueryRow failed: %s", err)
+			}
+			t.Log(now)
+		})
+	}
+}
diff --git a/dialer.go b/dialer.go
@@ -36,9 +36,7 @@ import (
 	"cloud.google.com/go/alloydbconn/internal/alloydb"
 	"cloud.google.com/go/alloydbconn/internal/tel"
 	"github.com/google/uuid"
-	"golang.org/x/net/proxy"
 	"golang.org/x/oauth2"
-	"golang.org/x/oauth2/google"
 	"google.golang.org/api/option"
 	"google.golang.org/protobuf/proto"
 
@@ -189,34 +187,9 @@ func (nullLogger) Debugf(context.Context, string, ...any) {}
 // RSA keypair is performed. Calls with a WithRSAKeyPair DialOption or after a default
 // RSA keypair is generated will be faster.
 func NewDialer(ctx context.Context, opts ...Option) (*Dialer, error) {
-	cfg := &dialerConfig{
-		refreshTimeout: alloydb.RefreshTimeout,
-		dialFunc:       proxy.Dial,
-		logger:         nullLogger{},
-		userAgents:     []string{userAgent},
-	}
-	for _, opt := range opts {
-		opt(cfg)
-		if cfg.err != nil {
-			return nil, cfg.err
-		}
-	}
-	if cfg.disableMetadataExchange && cfg.useIAMAuthN {
-		return nil, errors.New("incompatible options: WithOptOutOfAdvancedConnection " +
-			"check cannot be used with WithIAMAuthN")
-	}
-	userAgent := strings.Join(cfg.userAgents, " ")
-	// Add user agent to the end to make sure it's not overridden.
-	cfg.clientOpts = append(cfg.clientOpts, option.WithUserAgent(userAgent))
-
-	// If no token source is configured, use ADC's token source.
-	ts := cfg.tokenSource
-	if ts == nil {
-		var err error
-		ts, err = google.DefaultTokenSource(ctx, CloudPlatformScope)
-		if err != nil {
-			return nil, err
-		}
+	cfg, err := newDialerConfig(ctx, opts...)
+	if err != nil {
+		return nil, err
 	}
 
 	cOpts := append(cfg.alloydbClientOpts, cfg.clientOpts...)
@@ -261,7 +234,7 @@ func NewDialer(ctx context.Context, opts ...Option) (*Dialer, error) {
 		metricRecorders:         map[alloydb.InstanceURI]telv2.MetricRecorder{},
 		dialFunc:                cfg.dialFunc,
 		useIAMAuthN:             cfg.useIAMAuthN,
-		iamTokenSource:          ts,
+		iamTokenSource:          cfg.tokenSource,
 		userAgent:               userAgent,
 		buffer:                  newBuffer(),
 	}
diff --git a/options.go b/options.go
@@ -17,18 +17,21 @@ package alloydbconn
 import (
 	"context"
 	"crypto/rsa"
+	"errors"
 	"io"
 	"net"
 	"net/http"
 	"os"
+	"strings"
 	"time"
 
 	"cloud.google.com/go/alloydbconn/debug"
 	"cloud.google.com/go/alloydbconn/errtype"
 	"cloud.google.com/go/alloydbconn/internal/alloydb"
+	"golang.org/x/net/proxy"
 	"golang.org/x/oauth2"
 	"golang.org/x/oauth2/google"
-	apiopt "google.golang.org/api/option"
+	"google.golang.org/api/option"
 )
 
 // CloudPlatformScope is the default OAuth2 scope set on the API client.
@@ -37,23 +40,109 @@ const CloudPlatformScope = "https://www.googleapis.com/auth/cloud-platform"
 // An Option is an option for configuring a Dialer.
 type Option func(d *dialerConfig)
 
+func newDialerConfig(ctx context.Context, opts ...Option) (*dialerConfig, error) {
+	d := &dialerConfig{
+		refreshTimeout: alloydb.RefreshTimeout,
+		dialFunc:       proxy.Dial,
+		logger:         nullLogger{},
+		userAgents:     []string{userAgent},
+	}
+	for _, opt := range opts {
+		opt(d)
+	}
+
+	incompatibleAuthOpts := d.disableMetadataExchange && d.useIAMAuthN
+	if incompatibleAuthOpts {
+		return nil, errors.New("incompatible options: WithOptOutOfAdvancedConnectionCheck " +
+			"cannot be used with WithIAMAuthN")
+	}
+	incompatibleAuthOpts = d.credentialsFile != "" && d.credentialsJSON != nil
+	if incompatibleAuthOpts {
+		return nil, errors.New("incompatible options: WithCredentialsFile " +
+			"cannot be used with WithCredentialsJSON")
+	}
+	incompatibleAuthOpts = d.credentialsFile != "" && d.tokenSource != nil
+	if incompatibleAuthOpts {
+		return nil, errors.New("incompatible options: WithCredentialsFile " +
+			"cannot be used with WithTokenSource")
+	}
+	incompatibleAuthOpts = d.credentialsJSON != nil && d.tokenSource != nil
+	if incompatibleAuthOpts {
+		return nil, errors.New("incompatible options: WithCredentialsJSON " +
+			"cannot be used with WithTokenSource")
+	}
+
+	switch {
+	case d.credentialsFile != "":
+		b, err := os.ReadFile(d.credentialsFile)
+		if err != nil {
+			return nil, errtype.NewConfigError(err.Error(), "n/a")
+		}
+		// TODO: Use AlloyDB-specfic scope
+		c, err := google.CredentialsFromJSON(context.Background(), b, CloudPlatformScope)
+		if err != nil {
+			return nil, errtype.NewConfigError(err.Error(), "n/a")
+		}
+		d.tokenSource = c.TokenSource
+		d.clientOpts = append(d.clientOpts, option.WithCredentials(c))
+	case d.credentialsJSON != nil:
+		// TODO: Use AlloyDB-specfic scope
+		c, err := google.CredentialsFromJSON(context.Background(), d.credentialsJSON, CloudPlatformScope)
+		if err != nil {
+			return nil, errtype.NewConfigError(err.Error(), "n/a")
+		}
+		d.tokenSource = c.TokenSource
+		d.clientOpts = append(d.clientOpts, option.WithCredentials(c))
+	case d.tokenSource != nil:
+		d.clientOpts = append(d.clientOpts, option.WithTokenSource(d.tokenSource))
+	default:
+		// If a credentials file, credentials JSON, or a token soruce was not provided,
+		// default to Application Default Credentials.
+		var err error
+		ts, err := google.DefaultTokenSource(ctx, CloudPlatformScope)
+		if err != nil {
+			return nil, err
+		}
+		d.tokenSource = ts
+	}
+
+	if d.httpClient != nil {
+		d.clientOpts = append(d.clientOpts, option.WithHTTPClient(d.httpClient))
+	}
+
+	if d.adminAPIEndpoint != "" {
+		d.alloydbClientOpts = append(d.alloydbClientOpts, option.WithEndpoint(d.adminAPIEndpoint))
+	}
+
+	userAgent := strings.Join(d.userAgents, " ")
+	// Add user agent to the end to make sure it's not overridden.
+	d.clientOpts = append(d.clientOpts, option.WithUserAgent(userAgent))
+
+	return d, nil
+}
+
 type dialerConfig struct {
 	rsaKey *rsa.PrivateKey
 	// alloydbClientOpts are options to configure only the AlloyDB Rest API
 	// client. Configuration that should apply to all Google Cloud API clients
 	// should be included in clientOpts.
-	alloydbClientOpts []apiopt.ClientOption
+	alloydbClientOpts []option.ClientOption
 	// clientOpts are options to configure any Google Cloud API client. They
 	// should not include any AlloyDB-specific configuration.
-	clientOpts     []apiopt.ClientOption
-	dialOpts       []DialOption
-	dialFunc       func(ctx context.Context, network, addr string) (net.Conn, error)
-	refreshTimeout time.Duration
-	tokenSource    oauth2.TokenSource
-	userAgents     []string
-	useIAMAuthN    bool
-	logger         debug.ContextLogger
-	lazyRefresh    bool
+	clientOpts       []option.ClientOption
+	dialOpts         []DialOption
+	dialFunc         func(ctx context.Context, network, addr string) (net.Conn, error)
+	refreshTimeout   time.Duration
+	userAgents       []string
+	useIAMAuthN      bool
+	logger           debug.ContextLogger
+	lazyRefresh      bool
+	adminAPIEndpoint string
+
+	tokenSource     oauth2.TokenSource
+	credentialsFile string
+	credentialsJSON []byte
+	httpClient      *http.Client
 
 	// disableMetadataExchange is a temporary addition and will be removed in
 	// future versions.
@@ -62,8 +151,6 @@ type dialerConfig struct {
 	disableBuiltInTelemetry bool
 
 	staticConnInfo io.Reader
-	// err tracks any dialer options that may have failed.
-	err error
 }
 
 // WithOptions turns a list of Option's into a single Option.
@@ -80,28 +167,15 @@ func WithOptions(opts ...Option) Option {
 // authentication.
 func WithCredentialsFile(filename string) Option {
 	return func(d *dialerConfig) {
-		b, err := os.ReadFile(filename)
-		if err != nil {
-			d.err = errtype.NewConfigError(err.Error(), "n/a")
-			return
-		}
-		opt := WithCredentialsJSON(b)
-		opt(d)
+		d.credentialsFile = filename
 	}
 }
 
 // WithCredentialsJSON returns an Option that specifies a service account
 // or refresh token JSON credentials to be used as the basis for authentication.
 func WithCredentialsJSON(b []byte) Option {
 	return func(d *dialerConfig) {
-		// TODO: Use AlloyDB-specfic scope
-		c, err := google.CredentialsFromJSON(context.Background(), b, CloudPlatformScope)
-		if err != nil {
-			d.err = errtype.NewConfigError(err.Error(), "n/a")
-			return
-		}
-		d.tokenSource = c.TokenSource
-		d.clientOpts = append(d.clientOpts, apiopt.WithCredentials(c))
+		d.credentialsJSON = b
 	}
 }
 
@@ -125,7 +199,6 @@ func WithDefaultDialOptions(opts ...DialOption) Option {
 func WithTokenSource(s oauth2.TokenSource) Option {
 	return func(d *dialerConfig) {
 		d.tokenSource = s
-		d.clientOpts = append(d.clientOpts, apiopt.WithTokenSource(s))
 	}
 }
 
@@ -150,15 +223,15 @@ func WithRefreshTimeout(t time.Duration) Option {
 // advanced use-cases.
 func WithHTTPClient(client *http.Client) Option {
 	return func(d *dialerConfig) {
-		d.clientOpts = append(d.clientOpts, apiopt.WithHTTPClient(client))
+		d.httpClient = client
 	}
 }
 
 // WithAdminAPIEndpoint configures the underlying AlloyDB Admin API client to
 // use the provided URL.
 func WithAdminAPIEndpoint(url string) Option {
 	return func(d *dialerConfig) {
-		d.alloydbClientOpts = append(d.alloydbClientOpts, apiopt.WithEndpoint(url))
+		d.adminAPIEndpoint = url
 	}
 }
 
diff --git a/options_test.go b/options_test.go
