Skip to content

fix: Add expired env checker job #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 31, 2024
Merged

fix: Add expired env checker job #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/ctrlplane/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: v2
name: ctrlplane
description: Ctrlplane Helm chart for Kubernetes
type: application
version: 0.2.6
version: 0.2.7
appVersion: "1.16.0"

maintainers:
Expand Down
46 changes: 46 additions & 0 deletions charts/ctrlplane/charts/jobs/templates/expired-env-checker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
apiVersion: batch/v1
kind: CronJob
metadata:
name: {{ include "jobs.fullname" . }}-expired-env-checker
labels:
{{- if .Values.cron.labels -}}
{{- toYaml .Values.cron.labels | nindent 4 }}
{{- end }}
annotations:
{{- if .Values.cron.annotations -}}
{{- toYaml .Values.cron.annotations | nindent 4 }}
{{- end }}
spec:
concurrencyPolicy: {{ .Values.cron.concurrencyPolicy }}
schedule: {{ .Values.cron.schedule | quote }}
Comment on lines +13 to +15
Copy link

@coderabbitai coderabbitai bot Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  • successfulJobsHistoryLimit
  • failedJobsHistoryLimit

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.

Suggested change
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
Comment on lines +27 to +42
Copy link

@coderabbitai coderabbitai bot Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. The secrets are used consistently across multiple job templates (expired-env-checker.yaml, policy-checker.yaml)
  2. There are no existing secret validation patterns in the chart
  3. 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_URL

And 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

{{- include "ctrlplane.extraEnv" . | nindent 16 }}
{{- include "ctrlplane.extraEnvFrom" (dict "root" $ "local" .) | nindent 16 }}
resources:
{{- toYaml .Values.resources | nindent 16 }}
Comment on lines +16 to +46
Copy link

@coderabbitai coderabbitai bot Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add security context and timeouts

The container configuration should include security best practices and job management features:

  1. Add security contexts to run as non-root
  2. Specify activeDeadlineSeconds to prevent stuck jobs
  3. 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.

Suggested change
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 }}

Loading