Refactor Keycloak #502
Refactor Keycloak #502
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe pull request introduces modifications to enhance the functionality of the Changes
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
hack/e2e.sh (2)
191-192: Simplify Waiting Mechanism for etcd MembersThe current commands for waiting on etcd members might be redundant and could be simplified for better readability and efficiency.
Consider combining the commands to streamline the waiting process:
-timeout 180 sh -c 'until timeout -s 9 2 talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 2>&1; do sleep 1; done' -timeout 60 sh -c 'while talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 2>&1 | grep "rpc error"; do sleep 1; done' +timeout 180 sh -c 'until talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 >/dev/null 2>&1; do sleep 1; done'
340-346: Add Validation After Enabling OIDCAfter patching the ConfigMap to enable OIDC, consider adding a validation step to confirm that OIDC is correctly configured and operational.
You might add a command to verify the OIDC setup:
# Verify that OIDC is enabled in the cluster kubectl get authentication oidcpackages/apps/tenant/templates/keycloakgroups.yaml (1)
Line range hint
1-51: Resolve YAML Syntax Error at Line 1There is a YAML syntax error at line 1: "expected the node content, but found '-'". This may be due to improper placement or formatting of the
ifstatement.Apply this diff to fix the syntax:
-{{- if .Capabilities.APIVersions.Has "v1.edp.epam.com/v1" }} +{{- if .Capabilities.APIVersions.Has "v1.edp.epam.com/v1" -}}Ensure that there is no content before the
{{- if ... -}}statement and that the block is properly closed with{{- end }}at the end of the file.🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/core/platform/bundles/paas-hosted.yaml (2)
2-10: Add validation for theoidc-enableparameterWhile the code correctly validates the presence of required parameters, consider adding validation for the
oidc-enableparameter's value to ensure it's a valid boolean.{{- $oidcEnable := index $cozyConfig.data "oidc-enable" }} +{{- if and $oidcEnable (ne $oidcEnable "true") (ne $oidcEnable "false") }} +{{- fail "ERROR oidc-enable in cozystack ConfigMap must be 'true' or 'false'" }} +{{- end }}
Line range hint
166-184: Consider adding readiness checks for Keycloak resourcesThe Keycloak resources have proper dependencies, but consider adding readiness/health checks to ensure robust deployment ordering.
Consider adding readiness probes in the respective Helm charts for these resources to ensure they're fully operational before dependent resources are deployed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
hack/e2e.sh(5 hunks)packages/apps/tenant/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/tenant/templates/keycloakgroups.yaml(2 hunks)packages/apps/tenant/templates/kubeconfig.yaml(3 hunks)packages/core/platform/bundles/distro-full.yaml(0 hunks)packages/core/platform/bundles/distro-hosted.yaml(0 hunks)packages/core/platform/bundles/paas-full.yaml(4 hunks)packages/core/platform/bundles/paas-hosted.yaml(3 hunks)packages/system/keycloak-configure/templates/configure-kk.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- packages/core/platform/bundles/distro-hosted.yaml
- packages/core/platform/bundles/distro-full.yaml
🧰 Additional context used
🪛 yamllint (1.35.1)
packages/apps/tenant/templates/dashboard-resourcemap.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
packages/apps/tenant/templates/keycloakgroups.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (8)
hack/e2e.sh (3)
213-214: Confirm Addition of root-host and api-server-endpoint in ConfigMap
The keys root-host and api-server-endpoint have been added to the ConfigMap. Ensure these keys are correctly used in the configurations and that they match the expected values required by the system.
313-315:
Validate JSON Syntax in Ingress Patch Command
Similarly, the JSON in the kubectl patch command for the ingress may have a syntax issue. Please verify the JSON formatting.
Apply this diff to correct the syntax:
kubectl patch -n tenant-root ingresses.apps.cozystack.io ingress --type=merge -p '{"spec":{
"dashboard": true
-}}'
+}}'Ensure all JSON structures are properly closed.
Likely invalid or redundant comment.
299-305:
Correct JSON Syntax in kubectl patch Command
There may be a syntax issue in the JSON used for the kubectl patch command. Ensure that the JSON string is correctly formatted to prevent parsing errors.
Apply this diff to fix the JSON syntax:
kubectl patch -n tenant-root tenants.apps.cozystack.io root --type=merge -p '{"spec":{
"host": "example.org",
"ingress": true,
"monitoring": true,
"etcd": true,
"isolated": true
-}}'
+}}'Double-check that all brackets and braces are properly closed.
Likely invalid or redundant comment.
packages/apps/tenant/templates/kubeconfig.yaml (1)
Line range hint 5-45: Verify Usage of $apiServerEndpoint in Kubeconfig
The variable $apiServerEndpoint replaces $apiServerAdress. Ensure that $apiServerEndpoint is defined correctly and that the api-server-endpoint key exists in the cozystack ConfigMap.
Run the following script to check if api-server-endpoint is present:
✅ Verification successful
api-server-endpoint is properly validated and used in the codebase
The codebase shows that api-server-endpoint is properly handled:
- The key is required and validated in both
paas-full.yamlandpaas-hosted.yamlbundles with explicit failure messages if not present - The variable
$apiServerEndpointis correctly defined from the ConfigMap data in all usages - The usage in
kubeconfig.yamlmatches the validation pattern used across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'api-server-endpoint' exists in the ConfigMap
# Expected result: The 'api-server-endpoint' key should be displayed
kubectl get configmap cozystack -n cozy-system -o yaml | grep 'api-server-endpoint'
Length of output: 181
Script:
#!/bin/bash
# Let's search for ConfigMap definitions containing api-server-endpoint
rg -l "api-server-endpoint" --type yaml
# Let's also check the actual content of these files
rg "api-server-endpoint" --type yaml -C 2
Length of output: 1897
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/keycloak-configure/templates/configure-kk.yaml (2)
84-84: Review Hardcoded webUrl in Keycloak Client
The webUrl for the Keycloak client kubernetes is set to https://localhost:8000/oauth2/callback. Confirm that this is the intended URL for your environment, as using localhost may not be appropriate in a production setting.
Line range hint 1-95: Ensure All References to $apiServerAdress Are Updated
The variable $apiServerAdress has been removed. Verify that all references have been updated to use the new variable or hardcoded values, and that there are no lingering dependencies on the old variable.
Run this script to check for any remaining references:
✅ Verification successful
No References to $apiServerAdress Found in the Codebase
The variable $apiServerAdress has been completely removed from the templates directory, and no variations of API server address references were found. The code appears to be clean of any lingering dependencies on the old variable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any leftover references to $apiServerAdress
# Expected result: No output if all references have been removed
grep -rn '\$apiServerAdress' packages/system/keycloak-configure/templates/
Length of output: 76
Script:
#!/bin/bash
# Let's check for any variations in spelling or case of the variable name
rg -i 'apiserver(ad+res+|address)' packages/system/keycloak-configure/templates/
# Also check for any references to API server URLs or configurations that might be related
rg -i '(kubernetes|k8s|kube).+(url|address|endpoint)' packages/system/keycloak-configure/templates/
Length of output: 184
packages/core/platform/bundles/paas-hosted.yaml (1)
156-164: Verify the creation order of ConfigMap dependencies
The dashboard's dependency on kubeapps-auth-config ConfigMap could lead to a race condition if the ConfigMap isn't created before the dashboard deployment.
✅ Verification successful
No race condition concern - ConfigMap creation is properly sequenced
The kubeapps-auth-config ConfigMap is created by the keycloak-configure component, and the dashboard correctly specifies dependsOn: [keycloak-configure] when OIDC is enabled. This ensures proper sequencing where the ConfigMap is created before the dashboard attempts to use it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if kubeapps-auth-config is created by any of the dependent resources
rg -l "kubeapps-auth-config" | grep "templates/"
Length of output: 109
Script:
#!/bin/bash
# Let's verify if keycloak-configure creates the ConfigMap and if it's properly listed as a dependency
cat packages/system/keycloak-configure/templates/configure-kk.yaml
Length of output: 4069
packages/core/platform/bundles/paas-full.yaml (1)
226-234:
Fix conflicting dependency declarations for dashboard
There's a conflict in the dashboard's dependencies:
- Line 226 declares
dependsOn: [cilium,kubeovn,keycloak-configure] - Lines 227-234 override this with either
[keycloak-configure]or[]
This results in losing the required dependencies on cilium and kubeovn.
- dependsOn: [cilium,kubeovn,keycloak-configure]
{{- if $oidcEnable }}
- dependsOn: [keycloak-configure]
+ dependsOn: [cilium,kubeovn,keycloak-configure]
valuesFrom:
- kind: ConfigMap
name: kubeapps-auth-config
valuesKey: values.yaml
{{- else }}
- dependsOn: []
+ dependsOn: [cilium,kubeovn]
{{- end }}Likely invalid or redundant comment.
| apiServer: | ||
| extraArgs: | ||
| oidc-issuer-url: "https://keycloak.example.org/realms/cozy" | ||
| oidc-client-id: "kubernetes" | ||
| oidc-username-claim: "preferred_username" | ||
| oidc-groups-claim: "groups" |
There was a problem hiding this comment.
Verify YAML Indentation for apiServer Configuration
The indentation of the apiServer block in the patch.yaml file may be incorrect, which can lead to YAML parsing errors. Ensure that the apiServer key is properly indented under the cluster key.
Apply this diff to correct the indentation:
cluster:
+ apiServer:
+ extraArgs:
+ oidc-issuer-url: "https://keycloak.example.org/realms/cozy"
+ oidc-client-id: "kubernetes"
+ oidc-username-claim: "preferred_username"
+ oidc-groups-claim: "groups"Committable suggestion skipped: line range outside the PR's diff.
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: Role | ||
| metadata: | ||
| name: {{ include "tenant.name" . }}-dashboard-resources | ||
| namespace: {{ .Release.namespace }} | ||
| rules: | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - secrets | ||
| resourceNames: | ||
| - kubeconfig-{{ include "tenant.name" . }} | ||
| verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
Fix YAML Syntax Error in Role Definition
A YAML syntax error is reported at line 4: "expected <block end>, but found <scalar>". This suggests there may be an issue with the indentation or structure of the metadata fields.
Apply this diff to fix the syntax error:
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
- name: {{ include "tenant.name" . }}-dashboard-resources
+ name: "{{ include "tenant.name" . }}-dashboard-resources"
namespace: {{ .Release.namespace }}
rules:Wrapping the name value with quotes can resolve issues when template functions are used.
📝 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.
| apiVersion: rbac.authorization.k8s.io/v1 | |
| kind: Role | |
| metadata: | |
| name: {{ include "tenant.name" . }}-dashboard-resources | |
| namespace: {{ .Release.namespace }} | |
| rules: | |
| - apiGroups: | |
| - "" | |
| resources: | |
| - secrets | |
| resourceNames: | |
| - kubeconfig-{{ include "tenant.name" . }} | |
| verbs: ["get", "list", "watch"] | |
| apiVersion: rbac.authorization.k8s.io/v1 | |
| kind: Role | |
| metadata: | |
| name: "{{ include "tenant.name" . }}-dashboard-resources" | |
| namespace: {{ .Release.namespace }} | |
| rules: | |
| - apiGroups: | |
| - "" | |
| resources: | |
| - secrets | |
| resourceNames: | |
| - kubeconfig-{{ include "tenant.name" . }} | |
| verbs: ["get", "list", "watch"] |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 4-4: syntax error: expected , but found ''
(syntax)
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $oidcEnable := index $cozyConfig.data "oidc-enable" }} | ||
| {{- $host := index $cozyConfig.data "root-host" }} | ||
| {{- if not $host }} | ||
| {{- fail "ERROR need root-host in cozystack ConfigMap" }} | ||
| {{- end }} | ||
| {{- $apiServerEndpoint := index $cozyConfig.data "api-server-endpoint" }} | ||
| {{- if not $apiServerEndpoint }} | ||
| {{- fail "ERROR need api-server-endpoint in cozystack ConfigMap" }} | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing code duplication with paas-hosted.yaml
The OIDC and Keycloak configuration logic is duplicated between paas-hosted.yaml and paas-full.yaml. Consider extracting common configuration into a shared template.
Create a shared template file (e.g., _oidc-config.tpl) with common OIDC and Keycloak configuration:
{{- define "cozystack.oidc-config" -}}
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
{{- $oidcEnable := index $cozyConfig.data "oidc-enable" }}
...
{{- end -}}Then include it in both files:
-{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
-{{- $oidcEnable := index $cozyConfig.data "oidc-enable" }}
+{{- include "cozystack.oidc-config" . }}Also applies to: 226-234, 270-288
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
kvaps
force-pushed
the
refactor-keycloak
branch
from
07023d1 to
4cb7cb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/apps/tenant/templates/keycloakgroups.yaml (1)
Line range hint
4-53: Document group permissions and make realm reference configurableTwo suggestions for improvement:
- Add comments documenting the purpose and permissions of each group level (view, use, admin, super-admin)
- The
realmRefname is hardcoded to "keycloakrealm-cozy". Consider making it configurable through values.yaml{{- if $oidcEnabled }} +# KeycloakRealmGroups define the RBAC hierarchy for tenant access: +# - view: Read-only access to tenant resources +# - use: Basic operational permissions +# - admin: Full administrative access within tenant +# - super-admin: Elevated privileges including tenant management +{{- $realmName := .Values.keycloak.realmName | default "keycloakrealm-cozy" }} apiVersion: v1.edp.epam.com/v1 kind: KeycloakRealmGroup metadata: name: {{ include "tenant.name" . }}-view namespace: {{ include "tenant.name" . }} spec: name: {{ include "tenant.name" . }}-view realmRef: - name: keycloakrealm-cozy + name: {{ $realmName }} kind: ClusterKeycloakRealmpackages/apps/tenant/templates/kubeconfig.yaml (1)
Let me analyze the search results and generate the final response.
Security improvements needed for OIDC configuration
The codebase shows that other OIDC implementations in the repository follow secure practices. Specifically:
- The password grant type is not used elsewhere in the codebase - other components like dashboard use authorization code flow
- TLS verification is properly handled in other components (e.g., dashboard's oauth2-proxy configuration)
- Browser interaction settings are configurable in other implementations
Recommended changes:
args: - oidc-login - get-token - --oidc-issuer-url=https://keycloak.{{ $host }}/realms/cozy - --oidc-client-id=kubernetes - --oidc-client-secret={{ $k8sClient }} - - --skip-open-browser - - --grant-type=password + - --grant-type=authorization_code + {{- if .Values.oidc.skipBrowser }} + - --skip-open-browser + {{- end }} + {{- if not .Values.oidc.skipTLSVerify }} + - --tls-insecure-skip-verify=false + {{- end }}🔗 Analysis chain
Line range hint
21-45: Review security configuration for OIDC authenticationSeveral security considerations:
- The password grant type is less secure than authorization code flow
- Missing TLS verification settings for the OIDC endpoint
- Consider making the authentication flow configurable instead of hardcoding
--skip-open-browserLet's verify the OIDC configuration across the codebase:
Consider updating the configuration:
args: - oidc-login - get-token - --oidc-issuer-url=https://keycloak.{{ $host }}/realms/cozy - --oidc-client-id=kubernetes - --oidc-client-secret={{ $k8sClient }} - - --skip-open-browser - - --grant-type=password + - --grant-type=authorization_code + {{- if .Values.oidc.skipBrowser }} + - --skip-open-browser + {{- end }} + - --insecure-skip-tls-verify={{ .Values.oidc.skipTLSVerify | default "false" }}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other OIDC configurations to ensure consistent security settings rg --type yaml 'oidc|oauth' -A 5Length of output: 71632
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
hack/e2e.sh(5 hunks)packages/apps/tenant/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/tenant/templates/keycloakgroups.yaml(2 hunks)packages/apps/tenant/templates/kubeconfig.yaml(3 hunks)packages/core/platform/bundles/distro-full.yaml(0 hunks)packages/core/platform/bundles/distro-hosted.yaml(0 hunks)packages/core/platform/bundles/paas-full.yaml(4 hunks)packages/core/platform/bundles/paas-hosted.yaml(3 hunks)packages/system/keycloak-configure/templates/configure-kk.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- packages/core/platform/bundles/distro-hosted.yaml
- packages/core/platform/bundles/distro-full.yaml
🧰 Additional context used
🪛 yamllint (1.35.1)
packages/apps/tenant/templates/dashboard-resourcemap.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
packages/apps/tenant/templates/keycloakgroups.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (10)
hack/e2e.sh (3)
127-132: Verify YAML Indentation for apiServer Configuration
The indentation in the patch.yaml file for the apiServer block may be incorrect, which can lead to YAML parsing errors. Ensure that the apiServer key is properly indented under the cluster key, and extraArgs and its contents are correctly nested.
Apply this diff to correct the indentation:
cluster:
+ apiServer:
+ extraArgs:
+ oidc-issuer-url: "https://keycloak.example.org/realms/cozy"
+ oidc-client-id: "kubernetes"
+ oidc-username-claim: "preferred_username"
+ oidc-groups-claim: "groups"191-192: Enhanced Error Handling for etcd Readiness Checks
The updated commands improve the reliability of the etcd readiness checks by ensuring that the talosctl etcd members command executes successfully before proceeding.
341-346: Ensure OIDC Configuration is Applied Correctly
The commands to enable OIDC in the ConfigMap and wait for the Keycloak Helm releases are appropriate. Confirm that the Keycloak services are fully operational before proceeding with dependent actions.
packages/apps/tenant/templates/dashboard-resourcemap.yaml (1)
4-4: Fix YAML Syntax Error: Wrap the name Value in Quotes
The name field uses a template function, which may produce a value requiring quotes. Wrapping the value with quotes can prevent YAML parsing errors, especially if the tenant name contains special characters.
Apply this diff to fix the syntax error:
- name: {{ include "tenant.name" . }}-dashboard-resources
+ name: "{{ include "tenant.name" . }}-dashboard-resources"🧰 Tools
🪛 yamllint (1.35.1)
[error] 4-4: syntax error: expected , but found ''
(syntax)
packages/system/keycloak-configure/templates/configure-kk.yaml (1)
84-84: Verify the Change of webUrl to localhost:8000
Changing the webUrl of the KeycloakClient to https://localhost:8000/oauth2/callback may affect authentication flows if clients are not running locally. Ensure this change aligns with your deployment environment and client access patterns.
packages/core/platform/bundles/paas-hosted.yaml (3)
1-10: LGTM: Configuration validation is well-implemented
The validation checks for required configuration parameters are clear and fail early with descriptive error messages.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
156-164: LGTM: Well-structured conditional OIDC configuration
The dashboard configuration correctly adapts based on the OIDC status, with appropriate dependencies and values configuration.
Line range hint 166-184: LGTM: Well-structured Keycloak resource definitions
The Keycloak resources are properly organized with correct dependency chains and consistent naming conventions.
packages/core/platform/bundles/paas-full.yaml (2)
Line range hint 270-288: Consider extracting Keycloak resources into a shared template
The Keycloak resource definitions are duplicated between paas-hosted.yaml and paas-full.yaml.
Create a shared template file (e.g., _keycloak-resources.tpl):
{{- define "cozystack.keycloak-resources" -}}
- name: keycloak
releaseName: keycloak
chart: cozy-keycloak
namespace: cozy-keycloak
dependsOn: [postgres-operator]
- name: keycloak-operator
releaseName: keycloak-operator
chart: cozy-keycloak-operator
namespace: cozy-keycloak
dependsOn: [keycloak]
- name: keycloak-configure
releaseName: keycloak-configure
chart: cozy-keycloak-configure
namespace: cozy-keycloak
dependsOn: [keycloak-operator]
{{- end -}}1-10: 🛠️ Refactor suggestion
Consider reducing code duplication with paas-hosted.yaml
The configuration retrieval and validation logic is identical between paas-hosted.yaml and paas-full.yaml. Consider extracting this common code into a shared template.
Create a shared template file (e.g., _oidc-config.tpl):
{{- define "cozystack.oidc-config" -}}
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
{{- $oidcEnabled := index $cozyConfig.data "oidc-enabled" }}
{{- $host := index $cozyConfig.data "root-host" }}
{{- if not $host }}
{{- fail "ERROR need root-host in cozystack ConfigMap" }}
{{- end }}
{{- $apiServerEndpoint := index $cozyConfig.data "api-server-endpoint" }}
{{- if not $apiServerEndpoint }}
{{- fail "ERROR need api-server-endpoint in cozystack ConfigMap" }}
{{- end }}
{{- end -}}🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
| {{- if $oidcEnabled }} | ||
| dependsOn: [keycloak-configure] | ||
| valuesFrom: | ||
| - kind: ConfigMap | ||
| name: kubeapps-auth-config | ||
| valuesKey: values.yaml | ||
| {{- else }} | ||
| dependsOn: [] | ||
| {{- end }} |
There was a problem hiding this comment.
Fix conflicting dependsOn declarations
There's a conflict between the initial dependsOn list [cilium,kubeovn,keycloak-configure] and the conditional block that overwrites it. This could lead to losing the cilium and kubeovn dependencies.
Suggested fix:
{{- if $oidcEnabled }}
- dependsOn: [keycloak-configure]
+ dependsOn: [cilium,kubeovn,keycloak-configure]
valuesFrom:
- kind: ConfigMap
name: kubeapps-auth-config
valuesKey: values.yaml
{{- else }}
- dependsOn: []
+ dependsOn: [cilium,kubeovn]
{{- end }}📝 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.
| {{- if $oidcEnabled }} | |
| dependsOn: [keycloak-configure] | |
| valuesFrom: | |
| - kind: ConfigMap | |
| name: kubeapps-auth-config | |
| valuesKey: values.yaml | |
| {{- else }} | |
| dependsOn: [] | |
| {{- end }} | |
| {{- if $oidcEnabled }} | |
| dependsOn: [cilium,kubeovn,keycloak-configure] | |
| valuesFrom: | |
| - kind: ConfigMap | |
| name: kubeapps-auth-config | |
| valuesKey: values.yaml | |
| {{- else }} | |
| dependsOn: [cilium,kubeovn] | |
| {{- end }} |
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $oidcEnabled := index $cozyConfig.data "oidc-enabled" }} | ||
| {{- if $oidcEnabled }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing ConfigMap
The ConfigMap lookup could fail if cozystack ConfigMap doesn't exist in the cozy-system namespace. Consider adding a fallback or error handling mechanism.
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
+{{- if not $cozyConfig }}
+ {{- fail "Required ConfigMap 'cozystack' not found in 'cozy-system' namespace" }}
+{{- end }}
{{- $oidcEnabled := index $cozyConfig.data "oidc-enabled" }}
{{- if $oidcEnabled }}📝 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.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- $oidcEnabled := index $cozyConfig.data "oidc-enabled" }} | |
| {{- if $oidcEnabled }} | |
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- if not $cozyConfig }} | |
| {{- fail "Required ConfigMap 'cozystack' not found in 'cozy-system' namespace" }} | |
| {{- end }} | |
| {{- $oidcEnabled := index $cozyConfig.data "oidc-enabled" }} | |
| {{- if $oidcEnabled }} |
🧰 Tools
🪛 yamllint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
|
|
||
| {{- if $k8sClientSecret }} | ||
| {{- $apiServerEndpoint := index $cozyConfig.data "api-server-endpoint" }} | ||
| {{- $k8sClient := index $k8sClientSecret.data "client-secret-key" | b64dec }} | ||
| {{- $rootSaConfigMap := lookup "v1" "ConfigMap" "kube-system" "kube-root-ca.crt" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required configuration values
The template assumes all required values exist in ConfigMaps/Secrets. Add validation to ensure required values are present and valid.
{{- if $k8sClientSecret }}
+{{- if not (index $cozyConfig.data "api-server-endpoint") }}
+ {{- fail "Required value 'api-server-endpoint' not found in cozystack ConfigMap" }}
+{{- end }}
{{- $apiServerEndpoint := index $cozyConfig.data "api-server-endpoint" }}
{{- $k8sClient := index $k8sClientSecret.data "client-secret-key" | b64dec }}
+{{- if not $k8sClient }}
+ {{- fail "Empty client secret found in k8s-client Secret" }}
+{{- end }}📝 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.
| {{- if $k8sClientSecret }} | |
| {{- $apiServerEndpoint := index $cozyConfig.data "api-server-endpoint" }} | |
| {{- $k8sClient := index $k8sClientSecret.data "client-secret-key" | b64dec }} | |
| {{- $rootSaConfigMap := lookup "v1" "ConfigMap" "kube-system" "kube-root-ca.crt" }} | |
| {{- if $k8sClientSecret }} | |
| {{- if not (index $cozyConfig.data "api-server-endpoint") }} | |
| {{- fail "Required value 'api-server-endpoint' not found in cozystack ConfigMap" }} | |
| {{- end }} | |
| {{- $apiServerEndpoint := index $cozyConfig.data "api-server-endpoint" }} | |
| {{- $k8sClient := index $k8sClientSecret.data "client-secret-key" | b64dec }} | |
| {{- if not $k8sClient }} | |
| {{- fail "Empty client secret found in k8s-client Secret" }} | |
| {{- end }} | |
| {{- $rootSaConfigMap := lookup "v1" "ConfigMap" "kube-system" "kube-root-ca.crt" }} |
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Integrated OpenID Connect (OIDC) for enhanced authentication. - Added dynamic Role resource for tenant-specific access to Kubernetes secrets. - Introduced new Keycloak realm groups for improved role management. - **Improvements** - Enhanced error handling for service readiness checks. - Streamlined configuration files for better clarity and management of OIDC settings. - Updated handling of API server address and improved configuration adaptability based on OIDC settings. - **Bug Fixes** - Removed deprecated configurations related to Keycloak, simplifying deployment. These updates aim to improve security, usability, and overall system performance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These updates aim to improve security, usability, and overall system performance.