Issue 30660043: code review 30660043: goauth2/compute/serviceaccount: add service account helpers.
Descriptiongoauth2/compute/serviceaccount: add service account helpers.
Patch Set 1 #Patch Set 2 : diff -r 8eeae4635bf6 https://code.google.com/p/goauth2 #Patch Set 3 : diff -r 8eeae4635bf6 https://code.google.com/p/goauth2 #
Total comments: 8
Patch Set 4 : diff -r 8eeae4635bf6 https://code.google.com/p/goauth2 #
Total comments: 2
Patch Set 5 : diff -r 8eeae4635bf6 https://code.google.com/p/goauth2 #Patch Set 6 : diff -r 8eeae4635bf6 https://code.google.com/p/goauth2 #
Total comments: 4
Patch Set 7 : diff -r 8eeae4635bf6 https://code.google.com/p/goauth2 #MessagesTotal messages: 19
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/goauth2
Sign in to reply to this message.
On 2013/11/22 01:53:08, ukai wrote: > Hello mailto:golang-dev@googlegroups.com (cc: mailto:golang-dev@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/goauth2 What is the high level purpose of the goauth2 library? Personally, I'd prefer to see it remain just "OAuth 2.0 for Go clients", and have a separate home for specific service provider implementations. I see that there's already an App Engine-specific package in goauth2, though, so perhaps that ship may already have sailed. -josh
Sign in to reply to this message.
https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... File compute/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:6: // HTTP requests from Google Compute Engine instance using service accounts. "instances" https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:37: // Options is a service account options. Options configures a service account Client. https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:39: // Underlying roundtripper of service account client. Underlying transport of service account client. https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:112: func (t *transport) FetchToken() error { this needn't be exported
Sign in to reply to this message.
On 22 November 2013 13:57, <josharian@gmail.com> wrote: > What is the high level purpose of the goauth2 library? Personally, I'd > prefer to see it remain just > "OAuth 2.0 for Go clients", and have a separate home for specific > service provider implementations. > I see that there's already an App Engine-specific package in goauth2, > though, so perhaps that ship > may already have sailed. > I'm happy for goauth2 to be broadly useful in a variety of ways. I'm not quite happy with the state of the oauth and jwt packages. They are not very convenient to use, and it has been on my plate to try to figure out a better design. So far I haven't come up with anything, though. I think these helpers (appengine and this new compute/serviceaccount package) go a long way to relieving pain points when performing typical actions. I'd welcome more packages of this kind. Andrew
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, josharian@gmail.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... File compute/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:6: // HTTP requests from Google Compute Engine instance using service accounts. On 2013/11/22 03:07:31, adg wrote: > "instances" Done. https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:37: // Options is a service account options. On 2013/11/22 03:07:31, adg wrote: > Options configures a service account Client. Done. https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:39: // Underlying roundtripper of service account client. On 2013/11/22 03:07:31, adg wrote: > Underlying transport of service account client. Done. https://codereview.appspot.com/30660043/diff/40001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:112: func (t *transport) FetchToken() error { On 2013/11/22 03:07:31, adg wrote: > this needn't be exported Done. appengine/serviceaccount exports FetchToken. do we fix it as well. and don't we need to export Refresh too?
Sign in to reply to this message.
LGTM https://codereview.appspot.com/30660043/diff/60001/compute/serviceaccount/ser... File compute/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/30660043/diff/60001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:112: func (t *transport) fetchToken() error { actually you might as well leave it as FetchToken just to be consistent.
Sign in to reply to this message.
>> What is the high level purpose of the goauth2 library? Personally, I'd >> prefer to see it remain just >> "OAuth 2.0 for Go clients", and have a separate home for specific >> service provider implementations. >> I see that there's already an App Engine-specific package in goauth2, >> though, so perhaps that ship >> may already have sailed. > > > I'm happy for goauth2 to be broadly useful in a variety of ways. > I'm not quite happy with the state of the oauth and jwt packages. > They are not very convenient to use, and it has been on my plate to try to > figure out a better design. > So far I haven't come up with anything, though. I expect to use oauth/jwt a fair amount in the near future, so just maybe I'll have something useful to contribute on this front. I'd love to know (in just a few words) what dissatisfies you with the current state. > I think these helpers (appengine and this new compute/serviceaccount > package) go a long way to relieving pain points when performing typical > actions. I'd welcome more packages of this kind. Ok; thanks for clarifying. -josh
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, josharian@gmail.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 22 Nov 2013 14:58, "Josh Bleecher Snyder" <josharian@gmail.com> wrote: > > >> What is the high level purpose of the goauth2 library? Personally, I'd > >> prefer to see it remain just > >> "OAuth 2.0 for Go clients", and have a separate home for specific > >> service provider implementations. > >> I see that there's already an App Engine-specific package in goauth2, > >> though, so perhaps that ship > >> may already have sailed. > > > > > > I'm happy for goauth2 to be broadly useful in a variety of ways. > > I'm not quite happy with the state of the oauth and jwt packages. > > They are not very convenient to use, and it has been on my plate to try to > > figure out a better design. > > So far I haven't come up with anything, though. > > I expect to use oauth/jwt a fair amount in the near future, so just > maybe I'll have something useful to contribute on this front. I'd love > to know (in just a few words) what dissatisfies you with the current > state. Basically I don't understand it personally, the example seems wrong, and it all seems more complicated than it should be. I'd love your input here. > > > I think these helpers (appengine and this new compute/serviceaccount > > package) go a long way to relieving pain points when performing typical > > actions. I'd welcome more packages of this kind. > > Ok; thanks for clarifying. > > > -josh > > -- > > --- > You received this message because you are subscribed to the Google Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
https://codereview.appspot.com/30660043/diff/60001/compute/serviceaccount/ser... File compute/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/30660043/diff/60001/compute/serviceaccount/ser... compute/serviceaccount/serviceaccount.go:112: func (t *transport) fetchToken() error { On 2013/11/22 03:29:59, adg wrote: > actually you might as well leave it as FetchToken just to be consistent. Ok. I'll leave it as FetchToken just to be consistent.
Sign in to reply to this message.
Anyway, I'm wondering we should protect Token with mutex in transport, so that we could use the Client created by serviceaccount.NewClient concurrently? or should user create new client on each goroutine?
Sign in to reply to this message.
On 2013/11/22 07:30:58, ukai wrote: > Anyway, I'm wondering we should protect Token with mutex in transport, so that > we could use the Client created by serviceaccount.NewClient concurrently? > or should user create new client on each goroutine? Yes, this is a good point. On App Engine we didn't worry because you in that context you need to create a new client for every request. But in this case, I can see this being useful. Please add the lock, so that only one refresh is in flight at once.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, josharian@gmail.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/30660043/diff/100001/compute/serviceaccount/se... File compute/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/30660043/diff/100001/compute/serviceaccount/se... compute/serviceaccount/serviceaccount.go:86: mu sync.Mutex make this type transport struct { Transport http.RoundTripper Account string mu sync.Mutex token *oauth.Token } this is the convention to signify which fields are protected by the mutex https://codereview.appspot.com/30660043/diff/100001/compute/serviceaccount/se... compute/serviceaccount/serviceaccount.go:89: func refresh(t *transport) error { add a comment that t.mu should be held when this is called this can still be a method of *transport
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, josharian@gmail.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/30660043/diff/100001/compute/serviceaccount/se... File compute/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/30660043/diff/100001/compute/serviceaccount/se... compute/serviceaccount/serviceaccount.go:86: mu sync.Mutex On 2013/11/25 01:34:55, adg wrote: > make this > > type transport struct { > Transport http.RoundTripper > Account string > > mu sync.Mutex > token *oauth.Token > } > > this is the convention to signify which fields are protected by the mutex Done. https://codereview.appspot.com/30660043/diff/100001/compute/serviceaccount/se... compute/serviceaccount/serviceaccount.go:89: func refresh(t *transport) error { On 2013/11/25 01:34:55, adg wrote: > add a comment that t.mu should be held when this is called > this can still be a method of *transport Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/goauth2/source/detail?r=5877582f0cb0 *** goauth2/compute/serviceaccount: add service account helpers. R=golang-dev, josharian, adg CC=golang-dev https://codereview.appspot.com/30660043 Committer: Andrew Gerrand <adg@golang.org>
Sign in to reply to this message.
|
