Bugfix/fix kk configure reconciliation #589
Conversation
WalkthroughThe pull request introduces a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
bot
added
the
size:XS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/platform/bundles/paas-full.yaml (1)
302-304: Consider extracting common code to a template.The
cozyValuesimplementation is identical in bothpaas-hosted.yamlandpaas-full.yaml. To maintain consistency and ease maintenance, consider extracting this to a shared template.Create a new template file (e.g.,
_keycloak_helpers.tpl):{{- define "keycloak.cozyValues" -}} {{- $config := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} {{- if not $config }} {{- fail "ERROR: cozystack ConfigMap not found in cozy-system namespace" }} {{- end }} {{ $config.data | toJson | sha256sum }} {{- end -}}Then use it in both files:
values: cozyValues: | - {{ (lookup "v1" "ConfigMap" "cozy-system" "cozystack").data | toJson | sha256sum }} + {{ include "keycloak.cozyValues" . }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/platform/bundles/paas-full.yaml(1 hunks)packages/core/platform/bundles/paas-hosted.yaml(1 hunks)
🔇 Additional comments (2)
packages/core/platform/bundles/paas-hosted.yaml (1)
198-200: Verify the impact on Keycloak configuration reconciliation.The SHA256 checksum of the ConfigMap data is used to trigger reconciliation. Let's verify if this approach effectively solves the configuration update issue.
✅ Verification successful
The SHA256 checksum approach effectively triggers Keycloak configuration reconciliation
The implementation correctly uses Helm's value injection mechanism to detect ConfigMap changes and trigger reconciliation. Recent PRs (#508, #509) have addressed configuration drift issues, and ongoing improvements (#589) demonstrate active maintenance of this functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any reported issues or discussions about Keycloak configuration not updating # when ConfigMap changes. # Search for related issues or discussions gh issue list --search "keycloak configuration update" --state all gh pr list --search "keycloak configuration" --state all # Check for patterns indicating configuration update issues rg -i "keycloak.*config.*update" rg -i "keycloak.*reconcil"Length of output: 2795
Script:
#!/bin/bash # Search for Keycloak configuration and reconciliation implementation rg -i "configmap.*cozystack" -A 5 rg -i "sha256.*config" -A 5 # Find Keycloak controller or reconciliation logic ast-grep --pattern 'func $_($$$ *Reconcile* $$$) { $$$ }' # Check for any Helm templates or values files related to Keycloak configuration fd -e yaml -e tpl . packages/system/keycloak --exec grep -l -i "config"Length of output: 19366
packages/core/platform/bundles/paas-full.yaml (1)
302-304: Verify template usage across the codebase.Let's check if there are other similar patterns that could benefit from templating.
✅ Verification successful
Template usage is consistent and follows best practices
The codebase already demonstrates a consistent pattern of storing the ConfigMap lookup in
$cozyConfigvariable across multiple files, and the SHA256 usage is appropriately limited to bundle files where needed. No additional templating improvements are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for similar patterns of ConfigMap lookups that could be templated # Search for ConfigMap lookups rg 'lookup.*ConfigMap.*cozystack' # Search for SHA256 usage rg 'toJson \| sha256sum' # Look for existing template patterns rg '{{- define.*}}'Length of output: 3500
| values: | ||
| cozyValues: | | ||
| {{ (lookup "v1" "ConfigMap" "cozy-system" "cozystack").data | toJson | sha256sum }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for ConfigMap lookup.
The code assumes that the cozystack ConfigMap exists in the cozy-system namespace. While this is likely true given the context, it's better to handle potential errors gracefully.
Consider adding a check:
values:
cozyValues: |
- {{ (lookup "v1" "ConfigMap" "cozy-system" "cozystack").data | toJson | sha256sum }}
+ {{- $config := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
+ {{- if not $config }}
+ {{- fail "ERROR: cozystack ConfigMap not found in cozy-system namespace" }}
+ {{- end }}
+ {{ $config.data | toJson | sha256sum }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| values: | |
| cozyValues: | | |
| {{ (lookup "v1" "ConfigMap" "cozy-system" "cozystack").data | toJson | sha256sum }} | |
| values: | |
| cozyValues: | | |
| {{- $config := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- if not $config }} | |
| {{- fail "ERROR: cozystack ConfigMap not found in cozy-system namespace" }} | |
| {{- end }} | |
| {{ $config.data | toJson | sha256sum }} |
Co-authored-by: Andrei Kvapil <kvapss@gmail.com>
Co-authored-by: Andrei Kvapil <kvapss@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/platform/bundles/paas-full.yaml (1)
302-304: Consider restructuring the values section as previously suggested.The current structure differs from the previously suggested format. Consider using the nested structure to maintain consistency:
values: - cozystack: - configHash: {{ $cozyConfig | toJson | sha256sum }} + cozystack: + configHash: {{ $cozyConfig | toJson | sha256sum }}This maintains better alignment with Helm's value structure conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/platform/bundles/paas-full.yaml(1 hunks)packages/core/platform/bundles/paas-hosted.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/platform/bundles/paas-hosted.yaml
🔇 Additional comments (1)
packages/core/platform/bundles/paas-full.yaml (1)
302-304: Review impact on dependent services.The addition of the
configHashwill trigger reconciliation of thekeycloak-configurerelease whenever the ConfigMap changes. This could cascade to dependent services likedashboard. Ensure this is the intended behavior and that the hash computation only includes configuration fields that should trigger a reconciliation.Let's check the dependency chain:
✅ Verification successful
Implementation follows configuration management best practices
The
configHashimplementation is consistent across files and properly scoped to$cozyConfig. The dependency chain is well-structured with explicit declarations and appropriate conditional logic for OIDC-enabled scenarios. The cascading reconciliation behavior is intentional and follows standard Kubernetes configuration management patterns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check services that depend on keycloak-configure # Find all releases that depend on keycloak-configure rg -l "dependsOn.*keycloak-configure" --type yamlLength of output: 145
Script:
#!/bin/bash # Get the dependency context and configHash usage # Show dependencies on keycloak-configure with context rg "dependsOn.*keycloak-configure" --type yaml -B 2 -A 2 # Check for similar configHash patterns rg "configHash.*sha256sum" --type yamlLength of output: 1398
| @@ -299,4 +299,7 @@ releases: | |||
| chart: cozy-keycloak-configure | |||
| namespace: cozy-keycloak | |||
| dependsOn: [keycloak-operator] | |||
| values: | |||
| cozystack: | |||
| configHash: {{ $cozyConfig | toJson | sha256sum }} | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider using a deterministic approach for configuration hashing
The current implementation hashes the entire ConfigMap using toJson, which may lead to inconsistent hashes due to non-deterministic key ordering in JSON serialization. This could trigger unnecessary reconciliations of the keycloak-configure release.
Recommended solutions:
- Extract and sort only the relevant configuration fields before hashing
- Use a template function to create an ordered string representation of the required fields
Example approach:
configHash: {{ printf "%s:%s:%s" $cozyConfig.data.oidc-enabled $cozyConfig.data.root-host $cozyConfig.data.api-server-endpoint | sha256sum }}🔗 Analysis chain
Verify the hash computation's stability across reconciliations.
The toJson conversion of the entire ConfigMap might lead to inconsistent hashes if the fields are not ordered consistently. Consider either:
- Sorting the keys before hashing
- Hashing only specific fields that affect the Keycloak configuration
Let's verify the ConfigMap structure and potential hash variations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the ConfigMap structure and field ordering
# Get the ConfigMap structure
kubectl get configmap cozystack -n cozy-system -o yaml
# Check if there are any other consumers of this hash
rg -l "configHash" --type yaml
Length of output: 279
Script:
#!/bin/bash
# Check how $cozyConfig is constructed and used
rg -A 5 -B 5 "\$cozyConfig" --type yaml
# Look for any helper functions or templates that might handle JSON processing
fd "helpers|functions" --type yaml --exec cat {}
# Get full content of both files to compare usage
cat packages/core/platform/bundles/paas-hosted.yaml packages/core/platform/bundles/paas-full.yaml
Length of output: 13942
Summary by CodeRabbit
configHashfield in thekeycloak-configurerelease for bothpaas-fullandpaas-hostedconfigurations.cozyConfigdata to enhance configuration integrity checks.