fix: Add expired env checker job #13
Conversation
adityachoudhari26
requested review from
jsbroks and
zacharyblasczyk
as code owners
adityachoudhari26
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
WalkthroughThe changes include an update to the 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 (
|
|
@coderabbitai should we add activeDeadlineSeconds to the spec? |
|
Tip For best results, initiate chat on the files or code changes. @zacharyblasczyk: Yes, adding spec:
activeDeadlineSeconds: <max_duration_in_seconds>
template:
spec:
# your job spec hereReplace Let me know if you need any further assistance! 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Find all YAML files that define a Kubernetes Job
rg --files-with-matches 'kind:\s*Job' --glob '*.yaml' --glob '*.yml' > job_files.txt
# For each Job YAML file, check if 'activeDeadlineSeconds' is present
while read -r file; do
echo "Processing $file"
if rg -q 'activeDeadlineSeconds' "$file"; then
echo "'activeDeadlineSeconds' is present in $file"
else
echo "'activeDeadlineSeconds' is NOT present in $file"
fi
done < job_files.txt
Length of output: 522 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
charts/ctrlplane/Chart.yaml(1 hunks)charts/ctrlplane/charts/jobs/templates/expired-env-checker.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- charts/ctrlplane/Chart.yaml
🧰 Additional context used
🪛 yamllint
charts/ctrlplane/charts/jobs/templates/expired-env-checker.yaml
[warning] 7-7: wrong indentation: expected 2 but found 4
(indentation)
[warning] 7-7: too many spaces after hyphen
(hyphens)
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: too many spaces after hyphen
(hyphens)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[error] 4-4: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (1)
charts/ctrlplane/charts/jobs/templates/expired-env-checker.yaml (1)
1-12: LGTM: Well-structured resource metadata
The metadata section follows Kubernetes best practices and provides good flexibility through Helm values for custom labels and annotations.
🧰 Tools
🪛 yamllint
[warning] 7-7: wrong indentation: expected 2 but found 4
(indentation)
[warning] 7-7: too many spaces after hyphen
(hyphens)
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: too many spaces after hyphen
(hyphens)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[error] 4-4: syntax error: expected , but found ''
(syntax)
| spec: | ||
| concurrencyPolicy: {{ .Values.cron.concurrencyPolicy }} | ||
| schedule: {{ .Values.cron.schedule | quote }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding job history limits
While the basic CronJob configuration is correct, consider adding the following fields to better manage job history and prevent resource accumulation:
successfulJobsHistoryLimitfailedJobsHistoryLimit
Apply this diff:
spec:
concurrencyPolicy: {{ .Values.cron.concurrencyPolicy }}
schedule: {{ .Values.cron.schedule | quote }}
+ successfulJobsHistoryLimit: {{ .Values.cron.successfulJobsHistoryLimit | default 3 }}
+ failedJobsHistoryLimit: {{ .Values.cron.failedJobsHistoryLimit | default 1 }}📝 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.
| spec: | |
| concurrencyPolicy: {{ .Values.cron.concurrencyPolicy }} | |
| schedule: {{ .Values.cron.schedule | quote }} | |
| spec: | |
| concurrencyPolicy: {{ .Values.cron.concurrencyPolicy }} | |
| schedule: {{ .Values.cron.schedule | quote }} | |
| successfulJobsHistoryLimit: {{ .Values.cron.successfulJobsHistoryLimit | default 3 }} | |
| failedJobsHistoryLimit: {{ .Values.cron.failedJobsHistoryLimit | default 1 }} |
| jobTemplate: | ||
| spec: | ||
| template: | ||
| spec: | ||
| restartPolicy: OnFailure | ||
| containers: | ||
| - name: expired-env-checker | ||
| image: {{ .Values.image.repository }}:{{ .Values.image.tag }} | ||
| imagePullPolicy: {{ .Values.image.pullPolicy }} | ||
| command: ["node"] | ||
| args: ["index.js", "-r", "-j expired-env-checker"] | ||
| env: | ||
| - name: REDIS_URL | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Release.Name }}-connections | ||
| key: REDIS_URL | ||
| - name: POSTGRES_URL | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Release.Name }}-connections | ||
| key: POSTGRES_URL | ||
| - name: VARIABLES_AES_256_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Release.Name }}-encryption-key | ||
| key: AES_256_KEY | ||
| {{- include "ctrlplane.extraEnv" . | nindent 16 }} | ||
| {{- include "ctrlplane.extraEnvFrom" (dict "root" $ "local" .) | nindent 16 }} | ||
| resources: | ||
| {{- toYaml .Values.resources | nindent 16 }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security context and timeouts
The container configuration should include security best practices and job management features:
- Add security contexts to run as non-root
- Specify activeDeadlineSeconds to prevent stuck jobs
- Add pod security context
Apply these security improvements:
spec:
template:
spec:
restartPolicy: OnFailure
+ activeDeadlineSeconds: {{ .Values.cron.activeDeadlineSeconds | default 600 }}
+ securityContext:
+ runAsNonRoot: true
+ runAsUser: 1000
+ runAsGroup: 1000
+ fsGroup: 1000
containers:
- name: expired-env-checker
image: {{ .Values.image.repository }}:{{ .Values.image.tag }}
imagePullPolicy: {{ .Values.image.pullPolicy }}
+ securityContext:
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop:
+ - ALL
+ readOnlyRootFilesystem: true📝 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.
| jobTemplate: | |
| spec: | |
| template: | |
| spec: | |
| restartPolicy: OnFailure | |
| containers: | |
| - name: expired-env-checker | |
| image: {{ .Values.image.repository }}:{{ .Values.image.tag }} | |
| imagePullPolicy: {{ .Values.image.pullPolicy }} | |
| command: ["node"] | |
| args: ["index.js", "-r", "-j expired-env-checker"] | |
| env: | |
| - name: REDIS_URL | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Release.Name }}-connections | |
| key: REDIS_URL | |
| - name: POSTGRES_URL | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Release.Name }}-connections | |
| key: POSTGRES_URL | |
| - name: VARIABLES_AES_256_KEY | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Release.Name }}-encryption-key | |
| key: AES_256_KEY | |
| {{- include "ctrlplane.extraEnv" . | nindent 16 }} | |
| {{- include "ctrlplane.extraEnvFrom" (dict "root" $ "local" .) | nindent 16 }} | |
| resources: | |
| {{- toYaml .Values.resources | nindent 16 }} | |
| jobTemplate: | |
| spec: | |
| template: | |
| spec: | |
| restartPolicy: OnFailure | |
| activeDeadlineSeconds: {{ .Values.cron.activeDeadlineSeconds | default 600 }} | |
| securityContext: | |
| runAsNonRoot: true | |
| runAsUser: 1000 | |
| runAsGroup: 1000 | |
| fsGroup: 1000 | |
| containers: | |
| - name: expired-env-checker | |
| image: {{ .Values.image.repository }}:{{ .Values.image.tag }} | |
| imagePullPolicy: {{ .Values.image.pullPolicy }} | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| capabilities: | |
| drop: | |
| - ALL | |
| readOnlyRootFilesystem: true | |
| command: ["node"] | |
| args: ["index.js", "-r", "-j expired-env-checker"] | |
| env: | |
| - name: REDIS_URL | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Release.Name }}-connections | |
| key: REDIS_URL | |
| - name: POSTGRES_URL | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Release.Name }}-connections | |
| key: POSTGRES_URL | |
| - name: VARIABLES_AES_256_KEY | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ .Release.Name }}-encryption-key | |
| key: AES_256_KEY | |
| {{- include "ctrlplane.extraEnv" . | nindent 16 }} | |
| {{- include "ctrlplane.extraEnvFrom" (dict "root" $ "local" .) | nindent 16 }} | |
| resources: | |
| {{- toYaml .Values.resources | nindent 16 }} |
| env: | ||
| - name: REDIS_URL | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Release.Name }}-connections | ||
| key: REDIS_URL | ||
| - name: POSTGRES_URL | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Release.Name }}-connections | ||
| key: POSTGRES_URL | ||
| - name: VARIABLES_AES_256_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Release.Name }}-encryption-key | ||
| key: AES_256_KEY |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add secret validation to prevent deployment failures
The review suggestion is valid. The templates use several required secrets but lack validation, which could lead to silent failures during deployment. Based on the codebase analysis:
- The secrets are used consistently across multiple job templates (expired-env-checker.yaml, policy-checker.yaml)
- There are no existing secret validation patterns in the chart
- The chart has helper templates that could be extended to include validation
Add the following validation:
valueFrom:
secretKeyRef:
name: {{ required "Connection secrets are required for database access" (printf "%s-connections" .Release.Name) }}
key: REDIS_URLAnd similar validation for the encryption key secret:
name: {{ required "Encryption key secret is required for secure operations" (printf "%s-encryption-key" .Release.Name) }}🔗 Analysis chain
Verify required secrets exist
The job depends on several required secrets. Consider adding checks in the template to validate their existence.
Add validation using Helm's required function:
valueFrom:
secretKeyRef:
- name: {{ .Release.Name }}-connections
+ name: {{ required "A connections secret is required" (printf "%s-connections" .Release.Name) }}
key: REDIS_URL🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the template contains any validation for required secrets
rg -l "required" charts/ctrlplane/charts/jobs/templates/
Length of output: 56
Script:
#!/bin/bash
# Let's check the broader context of secret usage in Helm templates
rg -A 5 -B 5 "secretKeyRef" charts/ctrlplane/charts/jobs/templates/
# Also check if there are any other templates that might have secret validation patterns
rg -A 5 -B 5 "required.*secret" charts/ctrlplane/charts/
Length of output: 4523
Script:
#!/bin/bash
# Let's check if there are any helper templates or common patterns defined
fd . charts/ctrlplane -e yaml -e tpl | xargs rg -l "_helpers|common"
# Also check for any existing validation patterns in the chart
fd . charts/ctrlplane -e yaml -e tpl | xargs rg -A 3 "required|fail"
Length of output: 923
Summary by CodeRabbit
New Features
Version Update
ctrlplaneHelm chart from 0.2.6 to 0.2.7.