Fix: Update pvc/pv and initiate configs when deploy #51
Conversation
kubernetes/tacgui/values.yaml
Outdated
| database: tacgui | ||
| logDatabase: tgui_log | ||
| password: "" | ||
| password: "juniper@123" |
There was a problem hiding this comment.
Check lại giúp anh đoạn này đã tạo thành secret chưa nhé
There was a problem hiding this comment.
Password gen vào configmap thành file cấu hình được tacgui đọc. Em đang chưa xem phương án encrypt được ✌️
WalkthroughThis change set restructures the persistence configuration for the Tacgui Helm chart by introducing separate persistent volume claim and persistent volume sections, updates default values for several parameters, and adds new templates for managing configuration initialization using Kubernetes Jobs and ConfigMaps. Documentation is updated to reflect these changes, and some usage and troubleshooting content is removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes
participant PV (PersistentVolume)
participant PVC (PersistentVolumeClaim)
participant Job (InitConfigs)
participant ConfigMap (InitConfigs)
participant Deployment
User->>Helm: Install/Upgrade Chart
Helm->>Kubernetes: Create PersistentVolume (if enabled)
Helm->>Kubernetes: Create PersistentVolumeClaim (if enabled)
Helm->>Kubernetes: Create ConfigMap with TACACS+ config (if enabled)
Helm->>Kubernetes: Create Job to copy config to PVC (if enabled)
Job->>PVC: Copy tac_plus.cfg from ConfigMap to PVC
Helm->>Kubernetes: Deploy application with init container
Deployment->>PVC: Init container waits for config files
Deployment->>PVC: Main container starts after config is present
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
🔭 Outside diff range comments (2)
kubernetes/tacgui/values.yaml (1)
6-20: Parameter name drift – chart will not resolveglobal.storageClassName.Both
persistentVolumeClaim.ymlandpersistentVolume.ymlreference.Values.global.storageClassName, butvalues.yamlstill exposesglobal.storageClass.
Introduce the new key or revert the templates to the old one – otherwise rendered objects will missstorageClassName, breaking PVC/PV binding.global: imageRegistry: "" ... - storageClass: "" + storageClassName: ""kubernetes/tacgui/README.md (1)
78-88: Duplicateconfigurationentry in the image parameters tableThe parameter appears at Lines 78 and 87 with slightly different descriptions.
Drop one of them to avoid confusing users.
♻️ Duplicate comments (2)
kubernetes/tacgui/templates/persistentVolume.yml (1)
36-40: Wrong values key for storageClass fallback.Use
.Values.global.storageClass(as defined invalues.yaml) or rename the key invalues.yamltostorageClassNameto match the templates.kubernetes/tacgui/templates/persistentVolumeClaim.yml (1)
34-38: Same storageClass key mismatch as elsewhere.Replace
.Values.global.storageClassNamewith.Values.global.storageClassor rename the key invalues.yaml.
🧹 Nitpick comments (6)
kubernetes/tacgui/templates/deployment.yml (1)
109-118: Trailing whitespace & style-only – low impact.Two trailing-space lines flagged by YAML-lint; trim to avoid noisy pre-commit hooks.
kubernetes/tacgui/templates/persistentVolumeClaim.yml (1)
39-43: Redundant conditional duplicatesvolumeMode.The
else if .Values.persistentVolumeClaim.existingClaimbranch echoes the same expression; collapse to a single check to keep the template clean.kubernetes/tacgui/templates/configMaps/initConfigs.yml (2)
7-10: Label block indentation trips YAML lintersThe
labels:value is emitted vianindent 4, but the subsequentif .Values.commonLabelsblock is already indented 4 spaces, so the inner mapping appears 2 levels deeper than expected.
YAML parsers used by Helm ignore this, but many linters complain (see YAML-lint warnings).
Consider:- labels: {{- include "common.labels.standard" . | nindent 4 }} + labels: + {{- include "common.labels.standard" . | nindent 6 }}(or move the
labels:key into the included helper) to silence false-positive lint noise.
26-34: Trim superfluous trailing spaces inside literal linesLines 27-28 & 32 contain trailing blanks that bloat the diff every time an editor auto-strips them.
They’re inside the literal block, so you can remove them safely.kubernetes/tacgui/README.md (2)
79-80: Table cell breaks across linesThe description for
configurationConfigMapspans two physical lines, so Markdown renders the table incorrectly.
Join the text onto a single line.
181-181: Call-out: changed default forservice.externalTrafficPolicySwitching from
ClustertoLocalalters client-IP preservation and can affect load-balancer health checks.
Highlight this in the upgrade notes to avoid surprises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
kubernetes/tacgui/README.md(4 hunks)kubernetes/tacgui/templates/configMaps/initConfigs.yml(1 hunks)kubernetes/tacgui/templates/deployment.yml(3 hunks)kubernetes/tacgui/templates/jobInitConfigs.yml(1 hunks)kubernetes/tacgui/templates/persistentVolume.yml(1 hunks)kubernetes/tacgui/templates/persistentVolumeClaim.yml(1 hunks)kubernetes/tacgui/values.yaml(6 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
kubernetes/tacgui/templates/persistentVolumeClaim.yml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 9-9: wrong indentation: expected 2 but found 6
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
kubernetes/tacgui/templates/configMaps/initConfigs.yml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 9-9: wrong indentation: expected 2 but found 6
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 27-27: trailing spaces
(trailing-spaces)
[error] 28-28: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 70-70: no new line character at the end of file
(new-line-at-end-of-file)
kubernetes/tacgui/templates/jobInitConfigs.yml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
kubernetes/tacgui/templates/persistentVolume.yml
[error] 5-5: duplication of key "metadata" in mapping
(key-duplicates)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 9-9: wrong indentation: expected 2 but found 6
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 59-59: too many blank lines (1 > 0)
(empty-lines)
kubernetes/tacgui/templates/deployment.yml
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 117-117: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
kubernetes/tacgui/values.yaml (1)
110-113:initConfigsdefaults totrue– double-check upgrade path.Enabling the Job/ConfigMap workflow by default will create extra resources on upgrade and may restart pods unexpectedly.
Confirm this is acceptable for existing installations or flip the default tofalse.kubernetes/tacgui/templates/configMaps/initConfigs.yml (1)
1-1: Consider includingpre-upgradein the hook listOn Helm upgrades the ConfigMap won’t be regenerated, which may leave stale defaults on disk while the Job copies files from an older revision.
If you intend the config to be refreshable, append,pre-upgradeto the hook annotation.kubernetes/tacgui/README.md (1)
126-140: Possible doc/code mismatch forpersistentVolumeClaim.enableddefaultREADME shows the default as
false, while the PR summary says PVC is now enabled by default.
Please double-checkvalues.yamland align the documentation.
| annotations: | ||
| "helm.sh/hook": pre-install | ||
| "helm.sh/hook-weight": "-5" | ||
| {{- if .Values.commonAnnotations }} | ||
| {{- include "common.tplvalues.render" ( dict "value" .Values.commonAnnotations "context" $ ) | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a hook-delete policy to avoid orphaned ConfigMaps
Helm hooks stay around after a successful install/upgrade unless explicitly cleaned up.
Add the recommended annotation so repeated installs/upgrades don’t fail with “already exists”:
"helm.sh/hook-weight": "-5"
+"helm.sh/hook-delete-policy": "before-hook-creation,hook-succeeded"📝 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.
| annotations: | |
| "helm.sh/hook": pre-install | |
| "helm.sh/hook-weight": "-5" | |
| {{- if .Values.commonAnnotations }} | |
| {{- include "common.tplvalues.render" ( dict "value" .Values.commonAnnotations "context" $ ) | nindent 4 }} | |
| {{- end }} | |
| annotations: | |
| "helm.sh/hook": pre-install | |
| "helm.sh/hook-weight": "-5" | |
| "helm.sh/hook-delete-policy": "before-hook-creation,hook-succeeded" | |
| {{- if .Values.commonAnnotations }} | |
| {{- include "common.tplvalues.render" ( dict "value" .Values.commonAnnotations "context" $ ) | nindent 4 }} | |
| {{- end }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 11-11: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In kubernetes/tacgui/templates/configMaps/initConfigs.yml around lines 11 to 16,
add the annotation "helm.sh/hook-delete-policy" with the value
"before-hook-creation" to the annotations section. This will ensure Helm cleans
up the ConfigMap hooks before creating new ones, preventing errors about
resources already existing during repeated installs or upgrades.
| initContainers: | ||
| {{- if and .Values.initConfigs (.Values.persistentVolumeClaim.enabled) }} | ||
| - name: check-configuration | ||
| image: busybox | ||
| command: | ||
| - "/bin/sh" | ||
| - "-c" | ||
| - | | ||
| while [ ! -f /tmp/tacgui-data/tac_plus.cfg ] || [ ! -f /tmp/tacgui-data/tac_plus.cfg_test ]; do echo 'Waiting for configuration files...'; sleep 30; done; echo 'Configuration files are ready.' | ||
| volumeMounts: | ||
| - name: tacgui-data | ||
| mountPath: /tmp/tacgui-data | ||
| {{- end }} | ||
| {{- if .Values.initContainers }} | ||
| {{- include "common.tplvalues.render" (dict "value" .Values.initContainers "context" $) | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
initContainers: emitted with no items – renders invalid YAML when both feature flags are off.
Wrap the whole block in a conditional or build the list first.
- initContainers:
- {{- if ... }}
+ {{- if or (and .Values.initConfigs .Values.persistentVolumeClaim.enabled) .Values.initContainers }}
+ initContainers:
+ {{- if and .Values.initConfigs (.Values.persistentVolumeClaim.enabled) }}
- name: check-configuration
...
{{- end }}
{{- if .Values.initContainers }}
{{- include "common.tplvalues.render" (dict "value" .Values.initContainers "context" $) | nindent 8 }}
{{- end }}
+ {{- end }}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kubernetes/tacgui/templates/deployment.yml around lines 64 to 79, the
initContainers key is rendered even when no init containers are defined, causing
invalid YAML. To fix this, wrap the entire initContainers block in a conditional
that checks if either .Values.initConfigs with
.Values.persistentVolumeClaim.enabled or .Values.initContainers is set, so
initContainers is only emitted when there are actual items to include.
| cp /tmp/tac_plus.cfg /tmp/tacplus-data/tac_plus.cfg_test && \ | ||
| cp /tmp/tac_plus.cfg /tmp/tacplus-data/tac_plus.cfg && \ | ||
| chmod 777 /tmp/tacplus-data/tac_plus.cfg_test && \ | ||
| chmod 777 /tmp/tacplus-data/tac_plus.cfg && \ | ||
| mkdir -p /tmp/tacplus-data/backups && \ | ||
| chmod 777 /tmp/tacplus-data/backups |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid chmod 777 and world-writable backups.
Granting full RWX to everyone on configuration files defeats basic defence-in-depth.
Use the container’s UID/GID (or 0640/0750) instead.
-chmod 777 /tmp/tacplus-data/tac_plus.cfg_test && \
-chmod 777 /tmp/tacplus-data/tac_plus.cfg && \
+chmod 640 /tmp/tacplus-data/tac_plus.cfg_test && \
+chmod 640 /tmp/tacplus-data/tac_plus.cfg && \
...
-chmod 777 /tmp/tacplus-data/backups
+chmod 750 /tmp/tacplus-data/backups📝 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.
| cp /tmp/tac_plus.cfg /tmp/tacplus-data/tac_plus.cfg_test && \ | |
| cp /tmp/tac_plus.cfg /tmp/tacplus-data/tac_plus.cfg && \ | |
| chmod 777 /tmp/tacplus-data/tac_plus.cfg_test && \ | |
| chmod 777 /tmp/tacplus-data/tac_plus.cfg && \ | |
| mkdir -p /tmp/tacplus-data/backups && \ | |
| chmod 777 /tmp/tacplus-data/backups | |
| cp /tmp/tac_plus.cfg /tmp/tacplus-data/tac_plus.cfg_test && \ | |
| cp /tmp/tac_plus.cfg /tmp/tacplus-data/tac_plus.cfg && \ | |
| chmod 640 /tmp/tacplus-data/tac_plus.cfg_test && \ | |
| chmod 640 /tmp/tacplus-data/tac_plus.cfg && \ | |
| mkdir -p /tmp/tacplus-data/backups && \ | |
| chmod 750 /tmp/tacplus-data/backups |
🤖 Prompt for AI Agents
In kubernetes/tacgui/templates/jobInitConfigs.yml around lines 27 to 32, the use
of chmod 777 on configuration files and backup directories makes them
world-writable, which is insecure. Replace chmod 777 with more restrictive
permissions such as 0640 for files and 0750 for directories, and ensure
ownership is set to the container's UID/GID to maintain proper access control
without exposing files to all users.
| metadata: | ||
| metadata: | ||
| name: {{ include "common.names.fullname" . }}-pv | ||
| labels: {{- include "common.labels.standard" . | nindent 4 }} | ||
| {{- if .Values.commonLabels }} | ||
| {{- include "common.tplvalues.render" ( dict "value" .Values.commonLabels "context" $ ) | nindent 4 }} | ||
| {{- end }} | ||
| {{- if or .Values.persistentVolume.annotations .Values.commonAnnotations }} |
There was a problem hiding this comment.
Duplicate metadata: key breaks rendering.
Line 5 repeats metadata: causing invalid YAML.
Delete the second occurrence.
-4 metadata:
-5 metadata:
+4 metadata:📝 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.
| metadata: | |
| metadata: | |
| name: {{ include "common.names.fullname" . }}-pv | |
| labels: {{- include "common.labels.standard" . | nindent 4 }} | |
| {{- if .Values.commonLabels }} | |
| {{- include "common.tplvalues.render" ( dict "value" .Values.commonLabels "context" $ ) | nindent 4 }} | |
| {{- end }} | |
| {{- if or .Values.persistentVolume.annotations .Values.commonAnnotations }} | |
| metadata: | |
| name: {{ include "common.names.fullname" . }}-pv | |
| labels: {{- include "common.labels.standard" . | nindent 4 }} | |
| {{- if .Values.commonLabels }} | |
| {{- include "common.tplvalues.render" ( dict "value" .Values.commonLabels "context" $ ) | nindent 4 }} | |
| {{- end }} | |
| {{- if or .Values.persistentVolume.annotations .Values.commonAnnotations }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 5-5: duplication of key "metadata" in mapping
(key-duplicates)
[warning] 9-9: wrong indentation: expected 2 but found 6
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
🤖 Prompt for AI Agents
In kubernetes/tacgui/templates/persistentVolume.yml around lines 4 to 11, there
is a duplicate `metadata:` key on line 5 that breaks YAML rendering. Remove the
second `metadata:` line so that the metadata block is defined only once,
ensuring valid YAML structure.
| {{- if .Values.persistentVolume.annotations }} | ||
| annotations: | ||
| {{- include "common.tplvalues.render" ( dict "value" .Values.persistentVolume.annotations "context" $ ) | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
annotations not allowed under spec for PersistentVolume.
Move these annotations to the already-present metadata.annotations block or drop them; the current placement makes the PV object invalid.
🤖 Prompt for AI Agents
In kubernetes/tacgui/templates/persistentVolume.yml around lines 41 to 44, the
annotations are incorrectly placed under the spec section of the
PersistentVolume, which is invalid. Move the annotations block from under spec
to the metadata.annotations section where annotations are allowed, or remove
them if not needed, to ensure the PersistentVolume object is valid.
Sync with main
|
@ptthanh1511 Resolve request change để anh review lại đi |
Summary by CodeRabbit
New Features
Improvements
Documentation