-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Google: Implement groups fetch by default service account from metadata (support for GKE workload identity) #2989
Conversation
…Lists from Different Google Workspaces Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>
Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>
Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>
9e577b0
to
eba0f71
Compare
…#2911 Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, @vsychov. Thank you for opening this PR. For now, non of the maintainers has access to the Google provider, so testing changes for us is a special kind of pain.
Probably, we will hold with this PR for a while until we find a proper way to test it. Sorry for the inconvenience 😞
Hello @nabokihms, I have access to a Google provider. I'll start by building an image and use it to try it out. Please let me know if I can help out further with the review process. |
Signed-off-by: Maksim Nabokikh <maksim.nabokikh@flant.com>
Signed-off-by: Maksim Nabokikh <max.nabokih@gmail.com>
@nabokihms I notice this was added v2.38.0 milestone, anything we can do to help it progress? |
👋🏻 FWIW, we've recently tested a very similar change (with domain delegation) in another tool that we use, and things seem to work as expected! |
@nabokihms the documentation change merged in dexidp/website#138 references features that are not available yet as this PR was not merged. This caused some confusion for myself and my team. |
We just hit this too - relied on the docs and have only found this after debugging it not working :( |
There's a small merge conflict in the go.mod file, but after fixing that locally, I also get good results from this PR, after banging my head on the failure. (We disable service account key creation org-wide, so we either have to rely on workload identity for group retrieval, or give up on Dex and the Google connector.) There's a pile of moving parts to rule out whenever something related to Workload Identity goes wrong, so it took a frustrating couple of days to suspect and then prove that the code simply wasn't doing what it's documented to do. |
…dentity Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>
Hey @nabokihms , I've resolved conflicts with the master. Is there a chance this will be merged? This is a working PR that has been tested by several people under different conditions, as seen from the comments above. |
Hello @sagikazarmark , maybe you also can take a look? |
Is this going to be merged ? |
Signed-off-by: Maksim Nabokikh <maksim.nabokikh@flant.com>
Signed-off-by: Maksim Nabokikh <max.nabokih@gmail.com>
Signed-off-by: Maksim Nabokikh <max.nabokih@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while since PR was introduced. Thanks everyone for waiting. We decided to take this as it is now and hope everything will be fine.
@vsychov thanks you a lot for your effort and patience.
Thank you guys I've just tested this feature and it works well. |
…#2911 (#138) Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com> Signed-off-by: Maksim Nabokikh <maksim.nabokikh@flant.com> Co-authored-by: Maksim Nabokikh <maksim.nabokikh@flant.com>
Overview
Hello,
This pull request addresses the need to fetch groups using the default service account from metadata in the Dex Google Connector. It adds more robust support for Google Cloud Platform environments, particularly GKE Workload Identity, and increases the module's resilience and versatility.
What this PR does / why we need it
Fix #2676
Special notes for your reviewer
Does this PR introduce a user-facing change?
No
Docs PR: dexidp/website#138