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

fix(google): skip groups for service accounts #953

Merged
merged 6 commits into from
May 12, 2022

Conversation

perek
Copy link
Contributor

@perek perek commented May 10, 2022

Currently the google roles provider checks Google Groups for all service accounts, this is impossible to succeed because these are not real domains usable in Google Groups. This causes a couple of issues around rate limiting due to bad requests, and artificially increases the load on the Admin Directory API quota. By skipping over this without hitting Google we reduce the time needed to check permissions for this, and improve stability of this API

@perek perek requested a review from ttomsu as a code owner May 10, 2022 17:34
@ttomsu
Copy link
Member

ttomsu commented May 10, 2022

Hello, darkness, my old friend...

I'm not apart of this project anymore.

@ttomsu ttomsu removed their request for review May 10, 2022 17:41
@perek
Copy link
Contributor Author

perek commented May 10, 2022

We will never forget you @ttomsu

@perek
Copy link
Contributor Author

perek commented May 10, 2022

bitmoji

@perek
Copy link
Contributor Author

perek commented May 10, 2022

@dbyron-sf can you review this?

@dbyron-sf
Copy link
Contributor

Sorry @perek , I'm not familiar with this code.

@link108
Copy link
Member

link108 commented May 12, 2022

@perek this looks reasonable to me, thanks for adding tests as well 👍 I'm not super knowledgable re: google groups and service accounts, but it looks like they may have added the ability to use service accounts in groups: https://workspaceupdates.googleblog.com/2020/08/service-accounts-in-google-groups-beta.html

@perek
Copy link
Contributor Author

perek commented May 12, 2022

@perek this looks reasonable to me, thanks for adding tests as well 👍 I'm not super knowledgable re: google groups and service accounts, but it looks like they may have added the ability to use service accounts in groups: https://workspaceupdates.googleblog.com/2020/08/service-accounts-in-google-groups-beta.html

Yes, but that is not a spinnaker service account. Spinnaker service account don’t even have a domain, impossible to use on google

Copy link
Member

@link108 link108 left a comment

Choose a reason for hiding this comment

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

SGTM 👍

@link108 link108 added the ready to merge Approved and ready for merge label May 12, 2022
@mergify mergify bot added the auto merged label May 12, 2022
@mergify mergify bot merged commit 8972abd into spinnaker:master May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants