Skip to content

GCP CLI support: server-side request handler#19789

Merged
Tener merged 15 commits intomasterfrom
tener/gcloud-cli-handler
Jan 11, 2023
Merged

GCP CLI support: server-side request handler#19789
Tener merged 15 commits intomasterfrom
tener/gcloud-cli-handler

Conversation

@Tener
Copy link
Copy Markdown
Contributor

@Tener Tener commented Jan 3, 2023

Server-side request handler forwarding the requests to GCP, required for #17257

To be applied on top of #19788.

@Tener Tener mentioned this pull request Jan 3, 2023
2 tasks
@Tener Tener marked this pull request as ready for review January 3, 2023 11:50
@github-actions github-actions Bot requested review from LKozlowski and r0mant January 3, 2023 11:51
@Tener Tener force-pushed the tener/gcloud-cli-handler branch 2 times, most recently from 987e04c to d92542c Compare January 3, 2023 13:12
Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread lib/srv/app/azure/handler.go Outdated
Comment thread lib/srv/app/azure/handler_test.go Outdated
Comment thread lib/srv/app/gcp/handler_test.go Outdated
@Tener Tener force-pushed the tener/gcloud-cli-auth branch from b1543cb to b09f034 Compare January 4, 2023 10:25
@Tener Tener force-pushed the tener/gcloud-cli-handler branch from d92542c to 71e3a69 Compare January 4, 2023 10:30
@smallinsky smallinsky requested a review from greedy52 January 4, 2023 10:34
Copy link
Copy Markdown
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

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

First pass.

Comment thread lib/srv/app/gcp/handler.go Outdated
Comment thread lib/srv/app/gcp/handler.go Outdated
Comment thread lib/srv/app/gcp/handler.go Outdated
Comment thread lib/srv/app/gcp/handler.go Outdated
Comment thread lib/srv/app/gcp/handler.go Outdated
Comment thread lib/srv/app/azure/handler.go Outdated
Comment thread lib/srv/app/azure/handler_test.go Outdated
@Tener
Copy link
Copy Markdown
Contributor Author

Tener commented Jan 4, 2023

@smallinsky

I have moved this section to #19790

TODO: Testing:

  • Google Storage interaction with gsutil tool
  • Terraform GCP provider.

@Tener Tener force-pushed the tener/gcloud-cli-handler branch from d5a27d2 to f8a2788 Compare January 4, 2023 15:36
@Tener Tener force-pushed the tener/gcloud-cli-auth branch from b09f034 to ae22885 Compare January 5, 2023 10:24
@Tener Tener force-pushed the tener/gcloud-cli-handler branch from f8a2788 to aa7754b Compare January 5, 2023 10:59
@Tener Tener requested a review from smallinsky January 5, 2023 11:16
@Tener Tener force-pushed the tener/gcloud-cli-auth branch from ae22885 to fdceb53 Compare January 5, 2023 11:30
@Tener Tener force-pushed the tener/gcloud-cli-handler branch from aa7754b to adf88aa Compare January 5, 2023 11:32
@Tener Tener force-pushed the tener/gcloud-cli-auth branch from fdceb53 to 53e16a2 Compare January 5, 2023 12:20
@Tener Tener force-pushed the tener/gcloud-cli-handler branch from adf88aa to 54ccb28 Compare January 5, 2023 12:21
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

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

LGTM.

Only concern is on the pre-defined list of scopes. Not sure how often users need something else.

Comment thread lib/srv/app/gcp/handler.go
Comment thread lib/srv/app/gcp/handler.go
Comment thread lib/srv/app/gcp/handler.go
@Tener
Copy link
Copy Markdown
Contributor Author

Tener commented Jan 11, 2023

LGTM.

Only concern is on the pre-defined list of scopes. Not sure how often users need something else.

Thanks!

Regarding pre-defined scopes: I took it from the default scope list used by gcloud. In general, there is no harm in the list being too large (you configure access for service accounts individually anyway), and if needed we can expand the list (but I don't think it should be needed).

Tener added 11 commits January 11, 2023 12:21
- make debug log entry in appropriate place
- document the default scope list
- replace `context.WithTimeout` with `s.Clock.After` to avoid `time.Sleep` in tests.
- cloud client is now being mocked, making `generateAccessToken` testeable.
- update tests
- replace `context.WithTimeout` with `s.Clock.After` to avoid `time.Sleep` in tests.
@Tener Tener force-pushed the tener/gcloud-cli-auth branch from 53e16a2 to 2e7c71e Compare January 11, 2023 11:40
@Tener Tener force-pushed the tener/gcloud-cli-handler branch from 54ccb28 to 46d1ddc Compare January 11, 2023 11:40
Base automatically changed from tener/gcloud-cli-auth to master January 11, 2023 14:41
@Tener Tener enabled auto-merge (squash) January 11, 2023 14:56
@Tener Tener merged commit 4c7c660 into master Jan 11, 2023
@Tener Tener deleted the tener/gcloud-cli-handler branch January 11, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants