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

Google: Implement groups fetch by default service account from metadata (support for GKE workload identity) #2989

Merged
merged 10 commits into from
May 29, 2024

Conversation

vsychov
Copy link
Contributor

@vsychov vsychov commented Jun 8, 2023

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

Implement groups fetch by default service account from metadata (support for GKE workload identity)

Docs PR: dexidp/website#138

@vsychov vsychov changed the title Implement groups fetch by default service account from metadata (support for GKE workload identity) Google: Implement groups fetch by default service account from metadata (support for GKE workload identity) Jun 8, 2023
Copy link
Member

@nabokihms nabokihms left a 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 😞

@LaloLoop
Copy link

LaloLoop commented Aug 5, 2023

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.
Thanks!

@nabokihms nabokihms added the release-note/enhancement Release note: Enhancements label Oct 24, 2023
Signed-off-by: Maksim Nabokikh <[email protected]>
@nabokihms nabokihms added this to the v2.38.0 milestone Oct 24, 2023
@milesarmstrong
Copy link

@nabokihms I notice this was added v2.38.0 milestone, anything we can do to help it progress?

@jace-ys
Copy link

jace-ys commented Dec 11, 2023

👋🏻 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!

gocardless/theatre#330

nabokihms added a commit to dexidp/website that referenced this pull request Jan 10, 2024
…#2911 (#138)

Signed-off-by: Viacheslav Sychov <[email protected]>
Signed-off-by: Maksim Nabokikh <[email protected]>
Co-authored-by: Maksim Nabokikh <[email protected]>
@sagikazarmark sagikazarmark modified the milestones: v2.38.0, v2.39.0 Jan 25, 2024
@czuares
Copy link

czuares commented Feb 1, 2024

@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.

@mikebryant
Copy link

We just hit this too - relied on the docs and have only found this after debugging it not working :(

@irons
Copy link

irons commented Mar 28, 2024

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.

@vsychov
Copy link
Contributor Author

vsychov commented Mar 29, 2024

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.

@vsychov
Copy link
Contributor Author

vsychov commented Mar 29, 2024

Hello @sagikazarmark , maybe you also can take a look?

@nightwatch92
Copy link

Is this going to be merged ?

nabokihms and others added 3 commits May 29, 2024 13:31
Signed-off-by: Maksim Nabokikh <[email protected]>
Signed-off-by: Maksim Nabokikh <[email protected]>
Copy link
Member

@nabokihms nabokihms left a 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.

@nabokihms nabokihms merged commit b057594 into dexidp:master May 29, 2024
9 checks passed
@nabokihms nabokihms modified the milestones: v2.39.0, v2.40.0 May 29, 2024
@StepanKuksenko
Copy link

Thank you guys I've just tested this feature and it works well.
The only thing left is to bring the documentation dexidp/website#166 back to the website.

kerberos727 added a commit to kerberos727/dex-project that referenced this pull request Aug 23, 2024
…#2911 (#138)

Signed-off-by: Viacheslav Sychov <[email protected]>
Signed-off-by: Maksim Nabokikh <[email protected]>
Co-authored-by: Maksim Nabokikh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/enhancement Release note: Enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google connector with ADC: unexpected end of JSON input