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

Jump to bottom

feat(idtoken): Allow format options #1665

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(idtoken): Allow format options #1665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62af93a
feat(idtoken): Allow format options
eisandbar Aug 16, 2022
9b53769
Merge branch 'googleapis:main' into idtoken
eisandbar Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion idtoken/compute.go
View file
Open in desktop
Expand Up @@ -23,6 +23,10 @@ func computeTokenSource(audience string, ds *internal.DialSettings) (oauth2.Toke
}
ts := computeIDTokenSource{
audience: audience,
format: "full",
Copy link
Member

Choose a reason for hiding this comment

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

This changes the default behaviour which is not desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default format was already full, so default behavior should be the same.

}
if ds.CustomFormat != "" {
ts.format = ds.CustomFormat
}
tok, err := ts.Token()
if err != nil {
Expand All @@ -33,12 +37,13 @@ func computeTokenSource(audience string, ds *internal.DialSettings) (oauth2.Toke

type computeIDTokenSource struct {
audience string
format string
}

func (c computeIDTokenSource) Token() (*oauth2.Token, error) {
v := url.Values{}
v.Set("audience", c.audience)
v.Set("format", "full")
v.Set("format", c.format)
urlSuffix := "instance/service-accounts/default/identity?" + v.Encode()
res, err := metadata.Get(urlSuffix)
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions internal/settings.go
View file
Open in desktop
Expand Up @@ -47,6 +47,7 @@ type DialSettings struct {
ImpersonationConfig *impersonate.Config
EnableDirectPath bool
AllowNonDefaultServiceAccount bool
CustomFormat string
Copy link
Member

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 want this plumbed to the DialSettings as this is a package things and not an over all client library thing.

Also, there is no mechanism to set this field. I think I have a pattern laying around somewhere for doing something like this but it involves exposing custom client options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now DialSettings seems to already have a field called CustomClaims that only package idtoken uses

google-api-go-client/internal/settings.go

Line 45 in d6ee425

CustomClaims map[string]interface{}

And the way it is set

google-api-go-client/idtoken/idtoken.go

Lines 146 to 154 in 62364a2

func WithCustomClaims(customClaims map[string]interface{}) ClientOption {
return withCustomClaims(customClaims)
}
type withCustomClaims map[string]interface{}
func (w withCustomClaims) Apply(o *internal.DialSettings) {
o.CustomClaims = w
}

Copy link
Member

Choose a reason for hiding this comment

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

You are right, but I don't think it should. This works but only because these two packages are in the same repo today, but in the future they may not be. DialSetting was used here out of convenience, but it muddies the waters of what DialSetting should be used for I think.

There are patterns we could adopt to allow any package to declare its own options without needing to touch DialOptions at all. I think we CustomClaims could be re-written in such a manner -- as well as format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a pattern in mind?

My first thought is have opts be of type interface{} and then type switch each opt

func NewClient(ctx context.Context, audience string, opts ...interface{}) (*http.Client, error) {
	var ds internal.DialSettings
	for _, opt := range opts {
                switch x := opt.(type) {
                case ClientOption: 
                        x.Apply(&ds)
                }
	}

Are there any immediate drawbacks you see with this?


// Google API system parameters. For more information please read:
// https://cloud.google.com/apis/docs/system-parameters
Expand Down