From 1ba3330b2a1bb2b8e5aba21c3694eb5b2445994c Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Fri, 5 Apr 2024 14:08:39 -0600 Subject: [PATCH 1/2] feat(option): add support for env var GOOGLE_CLOUD_UNIVERSE_DOMAIN --- internal/cba_test.go | 22 +----- internal/settings.go | 49 ++++++++------ internal/settings_test.go | 71 ++++++++++++++++++++ option/internaloption/internaloption_test.go | 9 ++- option/option_test.go | 9 ++- 5 files changed, 117 insertions(+), 43 deletions(-) diff --git a/internal/cba_test.go b/internal/cba_test.go index 5d0dd2fa2b0..34cfd2eec9b 100644 --- a/internal/cba_test.go +++ b/internal/cba_test.go @@ -67,7 +67,6 @@ func TestGetEndpoint(t *testing.T) { Endpoint: tc.UserEndpoint, DefaultEndpoint: tc.DefaultEndpoint, DefaultEndpointTemplate: tc.DefaultEndpointTemplate, - DefaultUniverseDomain: "googleapis.com", }, nil) if tc.WantErr && err == nil { t.Errorf("want err, got nil err") @@ -121,10 +120,9 @@ func TestGetEndpointWithClientCertSource(t *testing.T) { for _, tc := range testCases { got, err := getEndpoint(&DialSettings{ - Endpoint: tc.UserEndpoint, - DefaultEndpoint: tc.DefaultEndpoint, - DefaultMTLSEndpoint: tc.DefaultMTLSEndpoint, - DefaultUniverseDomain: "googleapis.com", + Endpoint: tc.UserEndpoint, + DefaultEndpoint: tc.DefaultEndpoint, + DefaultMTLSEndpoint: tc.DefaultMTLSEndpoint, }, dummyClientCertSource) if tc.WantErr && err == nil { t.Errorf("want err, got nil err") @@ -201,7 +199,6 @@ func TestGetGRPCTransportConfigAndEndpoint(t *testing.T) { DefaultMTLSEndpoint: testMTLSEndpoint, DefaultEndpointTemplate: testEndpointTemplate, Endpoint: testOverrideEndpoint, - DefaultUniverseDomain: "googleapis.com", }, validConfigResp, testOverrideEndpoint, @@ -211,7 +208,6 @@ func TestGetGRPCTransportConfigAndEndpoint(t *testing.T) { &DialSettings{ DefaultMTLSEndpoint: "", DefaultEndpointTemplate: testEndpointTemplate, - DefaultUniverseDomain: "googleapis.com", }, validConfigResp, testRegularEndpoint, @@ -384,7 +380,6 @@ func TestGetHTTPTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultEndpoint: testRegularEndpoint, DefaultEndpointTemplate: testEndpointTemplate, DefaultMTLSEndpoint: testMTLSEndpoint, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testRegularEndpoint, }, @@ -395,7 +390,6 @@ func TestGetHTTPTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultEndpointTemplate: testEndpointTemplate, DefaultMTLSEndpoint: testMTLSEndpoint, ClientCertSource: dummyClientCertSource, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testMTLSEndpoint, }, @@ -406,7 +400,6 @@ func TestGetHTTPTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultEndpointTemplate: testEndpointTemplate, DefaultMTLSEndpoint: testMTLSEndpoint, UniverseDomain: testUniverseDomain, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testUniverseDomainEndpoint, }, @@ -418,7 +411,6 @@ func TestGetHTTPTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultMTLSEndpoint: testMTLSEndpoint, UniverseDomain: testUniverseDomain, ClientCertSource: dummyClientCertSource, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testUniverseDomainEndpoint, wantErr: errUniverseNotSupportedMTLS, @@ -457,7 +449,6 @@ func TestGetGRPCTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultEndpoint: testRegularEndpoint, DefaultEndpointTemplate: testEndpointTemplate, DefaultMTLSEndpoint: testMTLSEndpoint, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testRegularEndpoint, }, @@ -468,7 +459,6 @@ func TestGetGRPCTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultEndpointTemplate: testEndpointTemplate, DefaultMTLSEndpoint: testMTLSEndpoint, Endpoint: testOverrideEndpoint, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testOverrideEndpoint, }, @@ -479,7 +469,6 @@ func TestGetGRPCTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultEndpointTemplate: testEndpointTemplate, DefaultMTLSEndpoint: testMTLSEndpoint, ClientCertSource: dummyClientCertSource, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testMTLSEndpoint, }, @@ -491,7 +480,6 @@ func TestGetGRPCTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultMTLSEndpoint: testMTLSEndpoint, ClientCertSource: dummyClientCertSource, Endpoint: testOverrideEndpoint, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testOverrideEndpoint, }, @@ -502,7 +490,6 @@ func TestGetGRPCTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultEndpointTemplate: testEndpointTemplate, DefaultMTLSEndpoint: testMTLSEndpoint, UniverseDomain: testUniverseDomain, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testUniverseDomainEndpoint, }, @@ -514,7 +501,6 @@ func TestGetGRPCTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultMTLSEndpoint: testMTLSEndpoint, UniverseDomain: testUniverseDomain, Endpoint: testOverrideEndpoint, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testOverrideEndpoint, }, @@ -526,7 +512,6 @@ func TestGetGRPCTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { DefaultMTLSEndpoint: testMTLSEndpoint, UniverseDomain: testUniverseDomain, ClientCertSource: dummyClientCertSource, - DefaultUniverseDomain: "googleapis.com", }, wantErr: errUniverseNotSupportedMTLS, }, @@ -539,7 +524,6 @@ func TestGetGRPCTransportConfigAndEndpoint_UniverseDomain(t *testing.T) { UniverseDomain: testUniverseDomain, ClientCertSource: dummyClientCertSource, Endpoint: testOverrideEndpoint, - DefaultUniverseDomain: "googleapis.com", }, wantEndpoint: testOverrideEndpoint, }, diff --git a/internal/settings.go b/internal/settings.go index af4a038d3e2..3c3f95385a7 100644 --- a/internal/settings.go +++ b/internal/settings.go @@ -11,6 +11,7 @@ import ( "net/http" "os" "strconv" + "sync" "time" "golang.org/x/oauth2" @@ -20,8 +21,9 @@ import ( ) const ( - newAuthLibEnVar = "GOOGLE_API_GO_EXPERIMENTAL_USE_NEW_AUTH_LIB" - universeDomainDefault = "googleapis.com" + newAuthLibEnvVar = "GOOGLE_API_GO_EXPERIMENTAL_USE_NEW_AUTH_LIB" + universeDomainEnvVar = "GOOGLE_CLOUD_UNIVERSE_DOMAIN" + defaultUniverseDomain = "googleapis.com" ) // DialSettings holds information needed to establish a connection with a @@ -58,9 +60,10 @@ type DialSettings struct { EnableDirectPathXds bool EnableNewAuthLibrary bool AllowNonDefaultServiceAccount bool - UniverseDomain string DefaultUniverseDomain string + udMu sync.Mutex // guards universeDomain + UniverseDomain string // Google API system parameters. For more information please read: // https://cloud.google.com/apis/docs/system-parameters QuotaProject string @@ -94,7 +97,7 @@ func (ds *DialSettings) IsNewAuthLibraryEnabled() bool { if ds.EnableNewAuthLibrary { return true } - if b, err := strconv.ParseBool(os.Getenv(newAuthLibEnVar)); err == nil { + if b, err := strconv.ParseBool(os.Getenv(newAuthLibEnvVar)); err == nil { return b } return false @@ -165,31 +168,37 @@ func (ds *DialSettings) Validate() error { return nil } -// GetDefaultUniverseDomain returns the default service domain for a given Cloud -// universe, as configured with internaloption.WithDefaultUniverseDomain. -// The default value is "googleapis.com". +// GetDefaultUniverseDomain returns the Google default universe domain +// ("googleapis.com"). func (ds *DialSettings) GetDefaultUniverseDomain() string { - if ds.DefaultUniverseDomain == "" { - return universeDomainDefault - } - return ds.DefaultUniverseDomain + return defaultUniverseDomain } // GetUniverseDomain returns the default service domain for a given Cloud -// universe, as configured with option.WithUniverseDomain. -// The default value is the value of GetDefaultUniverseDomain, as configured -// with internaloption.WithDefaultUniverseDomain. +// universe, with the following precedence: +// +// 1. A non-empty option.WithUniverseDomain. +// 2. A non-empty environment variable GOOGLE_CLOUD_UNIVERSE_DOMAIN. +// 3. The default value "googleapis.com". func (ds *DialSettings) GetUniverseDomain() string { - if ds.UniverseDomain == "" { - return ds.GetDefaultUniverseDomain() + ds.udMu.Lock() + defer ds.udMu.Unlock() + if ds.UniverseDomain != "" { + return ds.UniverseDomain + } + envVarUniverseDomain := os.Getenv(universeDomainEnvVar) + if envVarUniverseDomain != "" { + ds.UniverseDomain = envVarUniverseDomain + } else { + ds.UniverseDomain = defaultUniverseDomain } return ds.UniverseDomain } // IsUniverseDomainGDU returns true if the universe domain is the default Google -// universe. +// universe ("googleapis.com"). func (ds *DialSettings) IsUniverseDomainGDU() bool { - return ds.GetUniverseDomain() == ds.GetDefaultUniverseDomain() + return ds.GetUniverseDomain() == defaultUniverseDomain } // GetUniverseDomain returns the default service domain for a given Cloud @@ -215,7 +224,7 @@ func GetUniverseDomain(creds *google.Credentials) (string, error) { case <-errors: // An error that is returned before the timer expires is likely to be // connection refused. Temporarily (2024-03-21) return the GDU domain. - return universeDomainDefault, nil + return defaultUniverseDomain, nil case res := <-results: return res, nil case <-timer.C: // Timer is expired. @@ -227,6 +236,6 @@ func GetUniverseDomain(creds *google.Credentials) (string, error) { // calls to creds.GetUniverseDomain() in grpc/dial.go and http/dial.go // and remove this method to close // https://github.com/googleapis/google-api-go-client/issues/2399. - return universeDomainDefault, nil + return defaultUniverseDomain, nil } } diff --git a/internal/settings_test.go b/internal/settings_test.go index 8b6e6d30f72..5ef1c1fa31c 100644 --- a/internal/settings_test.go +++ b/internal/settings_test.go @@ -79,3 +79,74 @@ func TestSettingsValidate(t *testing.T) { type dummyTS struct{} func (dummyTS) Token() (*oauth2.Token, error) { return nil, nil } + +func TestGetUniverseDomain(t *testing.T) { + testCases := []struct { + name string + ds *DialSettings + universeDomainEnvVar string + want string + }{ + { + name: "none", + ds: &DialSettings{}, + want: "googleapis.com", + }, + { + name: "settings", + ds: &DialSettings{ + UniverseDomain: "settings-example.goog", + }, + want: "settings-example.goog", + }, + { + name: "env var", + ds: &DialSettings{}, + universeDomainEnvVar: "env-example.goog", + want: "env-example.goog", + }, + { + name: "both", + ds: &DialSettings{ + UniverseDomain: "settings-example.goog", + }, + universeDomainEnvVar: "env-example.goog", + want: "settings-example.goog", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if tc.universeDomainEnvVar != "" { + t.Setenv("GOOGLE_CLOUD_UNIVERSE_DOMAIN", tc.universeDomainEnvVar) + } + + if got := tc.ds.GetUniverseDomain(); got != tc.want { + t.Errorf("got %s, want %s", got, tc.want) + } + if got, want := tc.ds.GetDefaultUniverseDomain(), "googleapis.com"; got != want { + t.Errorf("got %s, want %s", got, want) + } + }) + } +} + +func TestGetUniverseDomain_Race(t *testing.T) { + want := "example.com" + t.Setenv("GOOGLE_CLOUD_UNIVERSE_DOMAIN", want) + creds := &DialSettings{} + c := make(chan bool) + go func() { + got := creds.GetUniverseDomain() // First conflicting access. + if got != want { + t.Errorf("got %q, want %q", got, want) + } + c <- true + }() + got := creds.GetUniverseDomain() // Second conflicting access. + <-c + if got != want { + t.Errorf("got %q, want %q", got, want) + } + +} diff --git a/option/internaloption/internaloption_test.go b/option/internaloption/internaloption_test.go index 33037da25ce..90846616173 100644 --- a/option/internaloption/internaloption_test.go +++ b/option/internaloption/internaloption_test.go @@ -53,7 +53,12 @@ func TestDefaultApply(t *testing.T) { DefaultAudience: "audience", DefaultMTLSEndpoint: "http://mtls.example.com:445", } - if !cmp.Equal(got, want, cmpopts.IgnoreUnexported(grpc.ClientConn{}), cmpopts.IgnoreFields(google.Credentials{}, "udMu", "universeDomain")) { - t.Errorf(cmp.Diff(got, want, cmpopts.IgnoreUnexported(grpc.ClientConn{}), cmpopts.IgnoreFields(google.Credentials{}, "udMu", "universeDomain"))) + ignore := []cmp.Option{ + cmpopts.IgnoreUnexported(grpc.ClientConn{}), + cmpopts.IgnoreFields(internal.DialSettings{}, "udMu"), + cmpopts.IgnoreFields(google.Credentials{}, "udMu", "universeDomain"), + } + if !cmp.Equal(got, want, ignore...) { + t.Errorf(cmp.Diff(got, want, ignore...)) } } diff --git a/option/option_test.go b/option/option_test.go index 03ad71f7cff..60864766ed3 100644 --- a/option/option_test.go +++ b/option/option_test.go @@ -94,8 +94,13 @@ func TestApply(t *testing.T) { TelemetryDisabled: true, UniverseDomain: "universe.com", } - if !cmp.Equal(got, want, cmpopts.IgnoreUnexported(grpc.ClientConn{}), cmpopts.IgnoreFields(google.Credentials{}, "udMu", "universeDomain")) { - t.Errorf(cmp.Diff(got, want, cmpopts.IgnoreUnexported(grpc.ClientConn{}), cmpopts.IgnoreFields(google.Credentials{}, "udMu", "universeDomain"))) + ignore := []cmp.Option{ + cmpopts.IgnoreUnexported(grpc.ClientConn{}), + cmpopts.IgnoreFields(internal.DialSettings{}, "udMu"), + cmpopts.IgnoreFields(google.Credentials{}, "udMu", "universeDomain"), + } + if !cmp.Equal(got, want, ignore...) { + t.Errorf(cmp.Diff(got, want, ignore...)) } } From 6047d8e774a14e810ad69b5a6df7e3ad921c1e09 Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Fri, 5 Apr 2024 16:07:18 -0600 Subject: [PATCH 2/2] convert GetUniverseDomain to stateless --- internal/settings.go | 16 ++++------------ internal/settings_test.go | 20 -------------------- option/internaloption/internaloption_test.go | 1 - option/option_test.go | 1 - 4 files changed, 4 insertions(+), 34 deletions(-) diff --git a/internal/settings.go b/internal/settings.go index 3c3f95385a7..0415386f676 100644 --- a/internal/settings.go +++ b/internal/settings.go @@ -11,7 +11,6 @@ import ( "net/http" "os" "strconv" - "sync" "time" "golang.org/x/oauth2" @@ -61,9 +60,7 @@ type DialSettings struct { EnableNewAuthLibrary bool AllowNonDefaultServiceAccount bool DefaultUniverseDomain string - - udMu sync.Mutex // guards universeDomain - UniverseDomain string + UniverseDomain string // Google API system parameters. For more information please read: // https://cloud.google.com/apis/docs/system-parameters QuotaProject string @@ -181,18 +178,13 @@ func (ds *DialSettings) GetDefaultUniverseDomain() string { // 2. A non-empty environment variable GOOGLE_CLOUD_UNIVERSE_DOMAIN. // 3. The default value "googleapis.com". func (ds *DialSettings) GetUniverseDomain() string { - ds.udMu.Lock() - defer ds.udMu.Unlock() if ds.UniverseDomain != "" { return ds.UniverseDomain } - envVarUniverseDomain := os.Getenv(universeDomainEnvVar) - if envVarUniverseDomain != "" { - ds.UniverseDomain = envVarUniverseDomain - } else { - ds.UniverseDomain = defaultUniverseDomain + if envUD := os.Getenv(universeDomainEnvVar); envUD != "" { + return envUD } - return ds.UniverseDomain + return defaultUniverseDomain } // IsUniverseDomainGDU returns true if the universe domain is the default Google diff --git a/internal/settings_test.go b/internal/settings_test.go index 5ef1c1fa31c..09ccd2d4985 100644 --- a/internal/settings_test.go +++ b/internal/settings_test.go @@ -130,23 +130,3 @@ func TestGetUniverseDomain(t *testing.T) { }) } } - -func TestGetUniverseDomain_Race(t *testing.T) { - want := "example.com" - t.Setenv("GOOGLE_CLOUD_UNIVERSE_DOMAIN", want) - creds := &DialSettings{} - c := make(chan bool) - go func() { - got := creds.GetUniverseDomain() // First conflicting access. - if got != want { - t.Errorf("got %q, want %q", got, want) - } - c <- true - }() - got := creds.GetUniverseDomain() // Second conflicting access. - <-c - if got != want { - t.Errorf("got %q, want %q", got, want) - } - -} diff --git a/option/internaloption/internaloption_test.go b/option/internaloption/internaloption_test.go index 90846616173..8917f885dfa 100644 --- a/option/internaloption/internaloption_test.go +++ b/option/internaloption/internaloption_test.go @@ -55,7 +55,6 @@ func TestDefaultApply(t *testing.T) { } ignore := []cmp.Option{ cmpopts.IgnoreUnexported(grpc.ClientConn{}), - cmpopts.IgnoreFields(internal.DialSettings{}, "udMu"), cmpopts.IgnoreFields(google.Credentials{}, "udMu", "universeDomain"), } if !cmp.Equal(got, want, ignore...) { diff --git a/option/option_test.go b/option/option_test.go index 60864766ed3..a1e60418088 100644 --- a/option/option_test.go +++ b/option/option_test.go @@ -96,7 +96,6 @@ func TestApply(t *testing.T) { } ignore := []cmp.Option{ cmpopts.IgnoreUnexported(grpc.ClientConn{}), - cmpopts.IgnoreFields(internal.DialSettings{}, "udMu"), cmpopts.IgnoreFields(google.Credentials{}, "udMu", "universeDomain"), } if !cmp.Equal(got, want, ignore...) {