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.

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(idtoken): Allow format options #1665

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

Conversation

eisandbar
Copy link
Contributor

Added field CustomFormat to DialSettings in internal/settings.go
Added field format to computerIDTokenSource in idtoken/compute.go
Function computeTokenSource now sets field format to full, and if ds.CustomFormat != "" overwrites the field
Method Token now uses c.format instead of string literal "full"

Fixes #542

Added field CustomFormat to DialSettings in internal/settings.go
Added field format to computerIDTokenSource in idtoken/compute.go
Function computeTokenSource now sets field format to full, and if ds.CustomFormat != "" overwrites the field
Method Token now uses c.format instead of string literal "full"

Fixes googleapis#542
@eisandbar eisandbar requested review from a team and yoshi-approver as code owners August 16, 2022 22:49
@eisandbar eisandbar changed the title feat(idtoken):Allow format options feat(idtoken): Allow format options Aug 16, 2022
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I think I am going to hold off on this for now as we would need to figure out a way to limit the scope of these changes.

@@ -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.

@@ -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

CustomClaims map[string]interface{}

And the way it is set

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

idtoken: computeEngine should support format and license options
2 participants