fix: disable config cache, trigger reconcile if generation changes#649
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBumps controller-gen annotation in CRD manifests, adjusts KeycloakRealm controller reconciliation and reconciler pod image/tag selection, sets Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller (reconciler)
participant K8sAPI as Kubernetes API
participant Keycloak as Keycloak server
Controller->>K8sAPI: Fetch KeycloakRealm
alt ObservedGeneration matches and skip conditions met
Controller->>Controller: Skip reconciliation
else Proceed with reconcile
Controller->>Keycloak: (optional) detect server version
Keycloak-->>Controller: Return version (major)
Controller->>Controller: Compute image tag (realm.Spec.Version or "latest" or "latest-<major>")
Controller->>K8sAPI: Create/Update reconciler Pod (ENV IMPORT_CACHE_ENABLED="false", image tag)
K8sAPI-->>Controller: Pod created/updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 30 minutes and 56 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controllers/keycloakrealm_controller.go (1)
337-346:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRate limiter now blocks legitimate drift reconciliations.
On Line 337, removing the checksum guard makes the controller skip while
Ready=Truepurely based on interval timing (or always when interval is nil). That can suppress reconciles after watched resource changes when no reconciler pod is active, leaving drift unapplied.💡 Suggested fix
- if readyCondition != nil && readyCondition.Status == metav1.ConditionTrue && (realm.Spec.Interval == nil || time.Since(readyCondition.LastTransitionTime.Time) < realm.Spec.Interval.Duration) { + if readyCondition != nil && readyCondition.Status == metav1.ConditionTrue && + (realm.Spec.Interval == nil || time.Since(readyCondition.LastTransitionTime.Time) < realm.Spec.Interval.Duration) && + checksum == realm.Status.ObservedSHA256 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/keycloakrealm_controller.go` around lines 337 - 346, The skip logic around readyCondition and realm.Spec.Interval is too broad and suppresses reconciles after watched changes; modify the condition in the Reconcile (KeycloakRealmReconciler) so you only skip when the realm's readiness and interval criteria AND the resource has not changed since last apply — e.g., require that realm.Status.LastAppliedChecksum (or realm.Status.ObservedGeneration) equals the current calculated checksum (or realm.GetGeneration()) in addition to readyCondition.Status == True and the Interval timing check; if the checksum/observedGeneration differs, fall through and perform reconciliation instead of returning early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/controllers/keycloakrealm_controller.go`:
- Around line 337-346: The skip logic around readyCondition and
realm.Spec.Interval is too broad and suppresses reconciles after watched
changes; modify the condition in the Reconcile (KeycloakRealmReconciler) so you
only skip when the realm's readiness and interval criteria AND the resource has
not changed since last apply — e.g., require that
realm.Status.LastAppliedChecksum (or realm.Status.ObservedGeneration) equals the
current calculated checksum (or realm.GetGeneration()) in addition to
readyCondition.Status == True and the Interval timing check; if the
checksum/observedGeneration differs, fall through and perform reconciliation
instead of returning early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57685705-b499-4cbb-9039-8ab29acd02cb
📒 Files selected for processing (8)
chart/keycloak-controller/crds/keycloak.infra.doodle.com_keycloakclients.yamlchart/keycloak-controller/crds/keycloak.infra.doodle.com_keycloakrealms.yamlchart/keycloak-controller/crds/keycloak.infra.doodle.com_keycloakusers.yamlconfig/base/crd/bases/keycloak.infra.doodle.com_keycloakclients.yamlconfig/base/crd/bases/keycloak.infra.doodle.com_keycloakrealms.yamlconfig/base/crd/bases/keycloak.infra.doodle.com_keycloakusers.yamlinternal/controllers/keycloakrealm_controller.gointernal/controllers/keycloakrealm_controller_test.go
raffis
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controllers/keycloakrealm_controller.go (2)
457-472:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the dead
tagdefault or make it an actual fallback.
tag := "latest"is overwritten on both branches, so the current change still fails the reportedineffassigncheck. If the intent was to fall back tolatest, that fallback is not reachable because the version-detection error path returns immediately.Suggested fix
- tag := "latest" + var tag string username, password, err := getSecret(ctx, r.Client, realm, usernameField, passwordField) if err != nil { return realm, ctrl.Result{}, err } if realm.Spec.Version == "" { version, err := r.getKeycloakVersion(ctx, realm, username, password) if err != nil { return realm, reconcile.Result{}, err } tag = fmt.Sprintf("latest-%s", getMajor(version)) logger.Info("keycloak version detected", "version", version, "tag", tag) } else { tag = realm.Spec.Version }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/keycloakrealm_controller.go` around lines 457 - 472, The local variable tag is initialized to "latest" but that value is never used because both branches overwrite it (and the getKeycloakVersion error path returns), triggering the ineffassign warning; fix by either removing the dead initialization and declare var tag string then set tag in the two branches (use realm.Spec.Version when non-empty else set tag = fmt.Sprintf("latest-%s", getMajor(version)) after successful getKeycloakVersion), or if you want a real fallback, change the getKeycloakVersion error handling to not return immediately and instead log the error and leave tag as "latest" so the fallback is used; update the code around tag, getKeycloakVersion, and realm.Spec.Version accordingly.
337-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the generation/checksum guards outside the
spec.interval == nilfast path.Lines 337-345 still skip every reconcile once the realm is
Ready=Trueandspec.intervalis nil, becauserealm.Spec.Interval == nilshort-circuits the rest of the expression. That means a later generation-only update can still be ignored after the first successful reconcile, which misses the PR’s trigger-on-generation-change goal.Suggested fix
- if readyCondition != nil && readyCondition.Status == metav1.ConditionTrue && (realm.Spec.Interval == nil || time.Since(readyCondition.LastTransitionTime.Time) < realm.Spec.Interval.Duration && checksum == realm.Status.ObservedSHA256 && realm.Generation == realm.Status.ObservedGeneration) { + skipDueToInterval := realm.Spec.Interval == nil || time.Since(readyCondition.LastTransitionTime.Time) < realm.Spec.Interval.Duration + if readyCondition != nil && + readyCondition.Status == metav1.ConditionTrue && + skipDueToInterval && + checksum == realm.Status.ObservedSHA256 && + realm.Generation == realm.Status.ObservedGeneration {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controllers/keycloakrealm_controller.go` around lines 337 - 345, The current fast-path combines readyCondition, realm.Spec.Interval == nil and the generation/checksum guards into one conditional, so when realm.Spec.Interval is nil it short-circuits and skips reconciliation even if checksum or generation changed; modify the logic in the reconciliation block around readyCondition to first check Ready==True and then separately evaluate the generation/checksum guards (checksum vs realm.Status.ObservedSHA256 and realm.Generation vs realm.Status.ObservedGeneration) before deciding to return early, keeping the realm.Spec.Interval == nil fast-path only for the interval timing (requeue behavior via realm.Spec.Interval.Duration) and ensuring logger.V(1).Info and the return paths only run when both interval timing and checksum/generation conditions are satisfied.
🧹 Nitpick comments (1)
config/tests/cases/keycloak-v26/kustomization.yaml (1)
47-47: Pin to an immutable tag instead oflatest-26for test determinism.
latest-26is a moving tag that tracks the latest keycloak-config-cli release built for Keycloak 26, which can cause CI drift over time. Use an immutable alternative such aslatest-26.5.4,6.5.0-26, or pin by digest (e.g.,sha256:45fe580854ef20f6fbfe31d809265c3530248e75aa5f5843f2e66702cab06f18for amd64) to keep test outcomes reproducible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/tests/cases/keycloak-v26/kustomization.yaml` at line 47, The kustomization.yaml currently uses a moving tag value "version: latest-26"; change this to an immutable tag or digest to ensure deterministic tests by replacing the version field's value (the "version" key in kustomization.yaml) with a specific release like "latest-26.5.4" or "6.5.0-26" or a pinned image digest (e.g., the provided sha256) so CI runs use a fixed Keycloak image/version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/controllers/keycloakrealm_controller.go`:
- Around line 457-472: The local variable tag is initialized to "latest" but
that value is never used because both branches overwrite it (and the
getKeycloakVersion error path returns), triggering the ineffassign warning; fix
by either removing the dead initialization and declare var tag string then set
tag in the two branches (use realm.Spec.Version when non-empty else set tag =
fmt.Sprintf("latest-%s", getMajor(version)) after successful
getKeycloakVersion), or if you want a real fallback, change the
getKeycloakVersion error handling to not return immediately and instead log the
error and leave tag as "latest" so the fallback is used; update the code around
tag, getKeycloakVersion, and realm.Spec.Version accordingly.
- Around line 337-345: The current fast-path combines readyCondition,
realm.Spec.Interval == nil and the generation/checksum guards into one
conditional, so when realm.Spec.Interval is nil it short-circuits and skips
reconciliation even if checksum or generation changed; modify the logic in the
reconciliation block around readyCondition to first check Ready==True and then
separately evaluate the generation/checksum guards (checksum vs
realm.Status.ObservedSHA256 and realm.Generation vs
realm.Status.ObservedGeneration) before deciding to return early, keeping the
realm.Spec.Interval == nil fast-path only for the interval timing (requeue
behavior via realm.Spec.Interval.Duration) and ensuring logger.V(1).Info and the
return paths only run when both interval timing and checksum/generation
conditions are satisfied.
---
Nitpick comments:
In `@config/tests/cases/keycloak-v26/kustomization.yaml`:
- Line 47: The kustomization.yaml currently uses a moving tag value "version:
latest-26"; change this to an immutable tag or digest to ensure deterministic
tests by replacing the version field's value (the "version" key in
kustomization.yaml) with a specific release like "latest-26.5.4" or "6.5.0-26"
or a pinned image digest (e.g., the provided sha256) so CI runs use a fixed
Keycloak image/version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05160fe9-fff7-402b-b5e7-d99495930386
📒 Files selected for processing (5)
Makefileconfig/tests/base/keycloakrealm.yamlconfig/tests/cases/keycloak-v26/kustomization.yamlinternal/controllers/keycloakrealm_controller.gointernal/controllers/keycloakrealm_controller_test.go
💤 Files with no reviewable changes (1)
- config/tests/base/keycloakrealm.yaml
✅ Files skipped from review due to trivial changes (1)
- Makefile
2 participants
Current situation
keycloak-config-client by default caches the keycloakrealm. This is counter intuitive if something changes at keycloak and should be reverted by the reconciler. Before this change if this was not wanted one had to apply
IMPORT_CACHE_ENABLED: falseto the reconciler pod.Moreover a reconciliation was not triggered by any change outside of keycloak specs. For instance changing spec.suspend to true and back to false did not trigger a reconciliation, it awaited spec.interval as well which is confusing for users.
Proposal
Remove checksum gate and disable "IMPORT_CACHE_ENABLED" by default.
Trigger a reconciliation if the keycloakrealm generation changes.
Summary by CodeRabbit
Improvements
Updates
Chores