-
Notifications
You must be signed in to change notification settings - Fork 8
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: fail on request errors for triggering retry #143
Open
merll
wants to merge
28
commits into
main
Choose a base branch
from
apl-397-keycloak-error-handling
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+282
−285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
merll
changed the title
APL-397: fail on request errors for triggering retry
fix: fail on request errors for triggering retry
Nov 18, 2024
@ferruhcihan I reproduced the issue that you mentioned. It was actually the same error that usually leads to the user not getting assigned, just that it was logged but never thrown again. Unfortunately it still does not resolve by itself, so I am checking on it. |
merll
force-pushed
the
apl-397-keycloak-error-handling
branch
from
November 25, 2024 16:20
98b14ed
to
b691756
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces all instances of
utils.doApiCall
, which adds errors to a list but never raises them. This causes rejected requests to external APIs to get logged, but never retried. As a consequence, the operator will on a temporary failure (e.g. connection glitch) never reconcile fully until it is restarted.The only additional functionality of this function wrapper is that it passes over 409 errors (conflict) which might not be the most sensible thing to do in all situations. Where appropriate, the retry after a failed request (i.e. due to a race condition of two operator instances) with this particular error should come to the same result – otherwise it is more likely that the operator needs to be refined for such cases instead of suppressing the error.
For reducing the chance of race conditions, some more functions in role setup are awaited, and a single connection / api object is used during one update cycle. The update cycle of user groups has been consolidated into one function, and some unused or overly nested function calls have been removed.
In that regard, some objects are now being checked if they need updating (client, realm, users) before posting an update request. This should also lead to less errors (e.g. a known issue with duplicate redirect URLs), which seem to be a result of the Keycloak API not being able to handle nested object updates in all cases. There might still be potential in extending this approach to further objects.