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

Support ignoring 404s when user is not found in GitHub instance #28

Merged
merged 7 commits into from
Dec 15, 2023

Conversation

jasonmacgowan
Copy link
Contributor

This PR introduces a new configuration option, ignore_not_found, which allows entitlements to ignore HTTP 404 errors when trying to perform organization and team membership operations on a GitHub instance where the user doesn't exist.

Sometimes this option may be desirable when GitHub instance users are ultimately provisioned by other entitlements plugins and there is a race condition where this plugin may try to add a member before they exist.

For example, users may be provisioned in a GitHub instance via Enterprise Managed Users backed by an IdP which is also fed by entitlements. If the SCIM requests don't complete before this plugin is executed, the member will not exist in the instance and the run will fail. Since entitlements is designed to be run often, it may be acceptable to allow an "eventually consistent" state where entitlements would add the user the next time it runs once it's provisioned by the IdP.

@jasonmacgowan jasonmacgowan marked this pull request as draft December 14, 2023 16:48
@jasonmacgowan jasonmacgowan changed the title Support ignoring 404s when user is not found in GitHub instance [WIP] Support ignoring 404s when user is not found in GitHub instance Dec 14, 2023
@jar349
Copy link

jar349 commented Dec 14, 2023

I think that this is the only choice you have if you are sick and tired of these race conditions causing an entitlements deployment to fail.

Since entitlements is designed to be run often, it may be acceptable to allow an "eventually consistent" state where entitlements would add the user the next time it runs once it's provisioned by the IdP.

Entitlements may not always be designed this way. At our recent offsite, we discussed changing Entitlements to naturally be an eventually consistent system. In which case, this problem you describe would be okay - Entitlements would naturally handle that.

Entitlements isn't like that today. What will you do in the case where the user gets "stuck" - meaning that it continually does not get created? How would today's Entitlements system notice that since it has no knowledge of prior runs? It doesn't have the capacity to know "hey: I've tried adding this person like 6 times now, what gives?".

How can we handle that scenario such that a human eventually gets notified?

@jasonmacgowan
Copy link
Contributor Author

It's not perfect, but the "stuck" cases should be very rare. I'd expect the user to notice their access isn't working as expected and we'd have this fail condition well documented.

@jasonmacgowan
Copy link
Contributor Author

Confirmed that the methods to remove organization membership and remove organization team membership do not raise Oktokit::NotFound when the user is not found.

@jasonmacgowan jasonmacgowan changed the title [WIP] Support ignoring 404s when user is not found in GitHub instance Support ignoring 404s when user is not found in GitHub instance Dec 14, 2023
@jasonmacgowan jasonmacgowan marked this pull request as ready for review December 14, 2023 20:12
@jasonmacgowan jasonmacgowan merged commit 94f8e48 into main Dec 15, 2023
7 checks passed
@jasonmacgowan jasonmacgowan deleted the jasonmacgowan/ignore_not_found branch December 15, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants