chore: update samples for new auth #4383
Conversation
codyoss
commented
Sep 16, 2024
•
codyoss
commented
- Remove a couple of old unused samples
- Switch apiary examples to use NewService variant or constructs that don't require users to pass a client in
- Update all refs of old auth library to new
product-auto-label
bot
added
the
samples
|
Here is the summary of changes. You are about to delete 4 region tags.
This comment is generated by snippet-bot.
|
|
Thanks for updating these samples Cody! Did you intend to remove several region tags? |
|
@grayside Yeah, should have called that out in the commit message. It is from the two files I deleted that were non-code and not associated with any code. I verified these relics where not being used anywhere :) |
| dts, err := downscope.NewTokenSource(ctx, downscope.DownscopingConfig{RootSource: rootSource, Rules: accessBoundary}) | ||
| // downscope.NewCredentials constructs the credentials with the | ||
| // configuration provided. | ||
| downscopedCreds, err := downscope.NewCredentials(&downscope.Options{ |
There was a problem hiding this comment.
observation: this is interesting that it's no longer necessary to provide a context object for new credentials.
There was a problem hiding this comment.
The context for the old libraries has one thing they were used for: a way to shove an http.Client into the context that was later used during token exchanges. This is now an explicit param on the options struct. Everything in auth is evaluated lazy so you don't need a context when creating objects as no network calls are made.
Noticed this was still needed in GoogleCloudPlatform#4383. Note we should also switch latest to 1.23, but I did not in this PR because the image has not been built yet.
| serviceAccountFile = flag.String("service-account-file", "", "Path to service account JSON file. Required.") | ||
| serviceAccountEmail = flag.String("service-account-email", "", "Path email associated with the service account. Required.") | ||
| url = flag.String("url", "", "The request url. Required.") | ||
| audience = flag.String("audience", "", "The audience for the JWT. equired") |
There was a problem hiding this comment.
I see that this was already an issue but I think this is supposed to be Required instead of equired.
| serviceAccountEmail = flag.String("service-account-email", "", "Path email associated with the service account. Required.") | ||
| url = flag.String("url", "", "The request url. Required.") | ||
| audience = flag.String("audience", "", "The audience for the JWT. equired") | ||
| serviceAccountFile = flag.String("service-account-file", "", "Path to service account JSON file. Required.") |
There was a problem hiding this comment.
I'm confused why we are dropping the serviceAccountEmail but still leaving it in what is line 40 below.
| ) | ||
|
|
||
| func main() { | ||
| flag.Parse() | ||
|
|
||
| if *audience == "" || *url == "" || *serviceAccountFile == "" || *serviceAccountEmail == "" { | ||
| if *audience == "" || *url == "" || *serviceAccountFile == "" { | ||
| fmt.Println("requires: --url, --audience, --service-account-file, --service-account-email") |
There was a problem hiding this comment.
I suspect you might have wanted to drop , --service-account-email" from this Println based on the removal of the serviceAccountEmail
|
|
||
| // Extract the RSA private key from the service account keyfile. | ||
| sa, err := ioutil.ReadFile(saKeyfile) | ||
| func generateJWT(saKeyfile, audience string) (string, error) { |
There was a problem hiding this comment.
This one has me stumped and I looked over here https://github.com/googleapis/google-cloud-go/blob/f847c75df3bda9d74257ab68e55ac01191c287fc/auth/credentials/detect.go#L75
It seems like it's changing what we're showing here and maybe it's all taken care of by the underlying changes in the library.
If you take a look at this page https://cloud.google.com/api-gateway/docs/authenticate-service-account#go and look at the different language implementations it's showing the same overall concept, but now this has changed and it might be confusing if someone was comparing between implementations as to whether it's showing the same thing?
I checked the design doc and. Ididn't see anything about this, so could you explain it a bit more to me ? (not in the code, but just trying to figure out whether the sample is the same just implementation has changed and differs between languages)
Noticed this was still needed in #4383. Note we should also switch latest to 1.23, but I did not in this PR because the image has not been built yet. Co-authored-by: Eric Schmidt <erschmid@google.com> Co-authored-by: Marc Dougherty <muncus@users.noreply.github.com>
