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

feat(transport): add universe domain support #2355

Merged
merged 7 commits into from
Jan 19, 2024
Merged

feat(transport): add universe domain support #2355

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
348f707
feat(transport): add universe domain support
quartzmo Jan 17, 2024
6714ecc
Merge branch 'main' into transport-universe-domain
quartzmo Jan 18, 2024
d1d2082
add test
quartzmo Jan 18, 2024
ab7d34a
use %q instead of %s
quartzmo Jan 18, 2024
85db9f2
use explicit credentials in test
quartzmo Jan 18, 2024
8ae967d
Merge branch 'main' into transport-universe-domain
quartzmo Jan 19, 2024
49ad088
update test name
quartzmo Jan 19, 2024
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
17 changes: 17 additions & 0 deletions transport/http/dial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"context"
"crypto/tls"
"errors"
"fmt"
"net"
"net/http"
"time"
Expand Down Expand Up @@ -88,6 +89,13 @@ func newTransport(ctx context.Context, base http.RoundTripper, settings *interna
if err != nil {
return nil, err
}
credsUniverseDomain, err := creds.GetUniverseDomain()
if err != nil {
return nil, err
}
if settings.GetUniverseDomain() != credsUniverseDomain {
Copy link
Member

Choose a reason for hiding this comment

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

Won't this always happen if someone sets the value with WithUniverseDomain and creds returns the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct.

The verification can be done by comparing the universe_domain from the end-user configuration to the universe_domain from the credentials. If they match, the credentials can be used, if not the client libraries must raise an error instead of making a service call to the user-specified universe.

Copy link
Member

Choose a reason for hiding this comment

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

What is the point of the option in that case? They will never match unless it is set in the creds. Because we return a default value: https://cs.opensource.google/go/x/oauth2/+/refs/tags/v0.16.0:google/default.go;l=92

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing this creds method directly, do we have cases where creds have no universe embedded? Do we need to check for a non-empty universe from creds?

Copy link
Member Author

Choose a reason for hiding this comment

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

They will never match unless it is set in the creds.

Here's more spec:

Credentials are universe-specific. Whether a user provides the credentials or they are received via ADC, the credentials object (or equivalent) is expected to have the universe_domain property. The Client Libraries must read this property in order to verify that the credentials are valid for the user-supplied universe domain.

Copy link
Member Author

Choose a reason for hiding this comment

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

@shollyman

I'm not seeing this creds method directly, do we have cases where creds have no universe embedded? Do we need to check for a non-empty universe from creds?

See behavior of creds at https://github.com/golang/oauth2/blob/master/google/default.go#L91-L92

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the reference. I think my largest concern is related to non-cloud resolution (e.g. a service not in GDU currently). With the current implementation, both settings and creds yield up the GDU as the default value so I don't think it's an issue, but it is something for future consideration.

Copy link
Member Author

Choose a reason for hiding this comment

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

@shollyman This would be good to bring up with the spec authors. Afaik, the Go implementation adheres to the spec.

Copy link
Member

@codyoss codyoss Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

After looking at this with fresh eyes and thinking though it I think this is fine here, but really feels like it should be in the oauth2 layer. If we set the the override that is plumbed through to the oauth2 layer and is the value used by the credential I think? So it feels like this should never err perhaps? If that is the case it should be a test and not a in code validation. Approving.

Copy link
Member Author

@quartzmo quartzmo Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The WithUniverseDomain override is in fact NOT plumbed through to the oauth2 layer, it is only used to construct the endpoint in the client, pending a number of error condition checks like the one discussed here. The creds instance here needs to have the same universe domain value as is provided to settings, but only settings gets it via the WithUniverseDomain override. The creds instance gets it either from credentials JSON, the compute MDS, or oauth2's google.CredentialsParams. If the two separately-sourced values do not match, we need to return an error here.

return nil, errUniverseNotMatch(settings.GetUniverseDomain(), credsUniverseDomain)
}
paramTransport.quotaProject = internal.GetQuotaProject(creds, settings.QuotaProject)
ts := creds.TokenSource
if settings.ImpersonationConfig == nil && settings.TokenSource != nil {
Expand All @@ -101,6 +109,15 @@ func newTransport(ctx context.Context, base http.RoundTripper, settings *interna
return trans, nil
}

func errUniverseNotMatch(settingsUD, credsUD string) error {
return fmt.Errorf(
"the configured universe domain (%q) does not match the universe "+
"domain found in the credentials (%q). If you haven't configured "+
"WithUniverseDomain explicitly, googleapis.com is the default",
settingsUD,
credsUD)
}

func newSettings(opts []option.ClientOption) (*internal.DialSettings, error) {
var o internal.DialSettings
for _, opt := range opts {
Expand Down
16 changes: 16 additions & 0 deletions transport/http/dial_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (

"go.opencensus.io/plugin/ochttp"
"golang.org/x/oauth2"
"golang.org/x/oauth2/google"
"google.golang.org/api/option"
)

func TestNewClient(t *testing.T) {
Expand All @@ -37,3 +39,17 @@ func TestNewClient(t *testing.T) {
t.Fatalf("got %s, want: %s", got, want)
}
}

func TestNewClient_UniverseDomain(t *testing.T) {
quartzmo marked this conversation as resolved.
Show resolved Hide resolved
rootTokenScope := "https://www.googleapis.com/auth/cloud-platform"
universeDomain := "example.com"
universeDomainDefault := "googleapis.com"
creds := &google.Credentials{} // universeDomainDefault
wantErr := errUniverseNotMatch(universeDomain, universeDomainDefault)
_, _, err := NewClient(context.Background(), option.WithUniverseDomain(universeDomain),
option.WithCredentials(creds), option.WithScopes(rootTokenScope))

if err.Error() != wantErr.Error() {
t.Fatalf("got: %v, want: %v", err, wantErr)
}
}