Skip to content

HYPERFLEET-1111 - feat: automated orphan cleanup#55

Draft
rh-amarin wants to merge 1 commit into
openshift-hyperfleet:mainfrom
rh-amarin:clean-namespaces
Draft

HYPERFLEET-1111 - feat: automated orphan cleanup#55
rh-amarin wants to merge 1 commit into
openshift-hyperfleet:mainfrom
rh-amarin:clean-namespaces

Conversation

@rh-amarin

Copy link
Copy Markdown
Contributor

Summary

Introduces a namespace-cleaner Helm chart that runs a CronJob to periodically clean up stale HyperFleet namespaces and their associated Maestro resource bundles.

The cronjob will run every 1 hour and will delete:

  • Maestro bundles older than 3 hours
  • Namespaces with label key hyperfleet.io/cluster-id older than 3 hours
    • These labels are present in all resources created by adapters in the E2E repository

Why a custom job:

  • The original ticket mentions https://github.com/hjacobs/kube-janitor but the project was archived in 2020
  • But moreover, projects that only deal with k8s resources can not deal with maestro recreating resources if the bundles are not cleaned up from the DB

This makefile target is meant to execute manually when installing the prow cluster

What

  • New Helm chart helm/namespace-cleaner/ deploying a CronJob with full RBAC (ServiceAccount,
    ClusterRole, ClusterRoleBinding)
  • Cleaner script (scripts/namespace-cleaner.sh) embedded via ConfigMap and mounted into the CronJob container, making it independently testable
  • Two-phase cleanup per run:
    1. Delete Maestro resource bundles older than ageMinutes via the Maestro REST API, preventing Maestro from recreating namespaces
    2. Delete namespaces carrying the hyperfleet.io/cluster-id label that are older than ageMinutes, using kubectl delete --wait=false
  • DRY_RUN=true mode logs what would be deleted without taking action
  • Graceful degradation when Maestro is unreachable: logs a warning and proceeds to namespace cleanup
  • Cross-platform parse_timestamp() supporting GNU date (Linux), busybox -D flag (Alpine/alpine/k8s), and BSD date with TZ=UTC (macOS) to avoid timezone-skewed age calculation
  • New make install-cleaner / make uninstall-cleaner targets with configurable variables: CLEANER_SCHEDULE, CLEANER_LABEL_SELECTOR, CLEANER_AGE_MINUTES, CLEANER_MAESTRO_URL

Testing

To dry-run against a live cluster (port-forward Maestro first):

kubectl port-forward svc/maestro -n maestro 8000:8000 &
DRY_RUN=true AGE_MINUTES=30 MAESTRO_URL=http://localhost:8000 \
  bash helm/namespace-cleaner/scripts/namespace-cleaner.sh

To deploy:

HELMFILE_ENV=kind make install-cleaner
# trigger immediately without waiting for the next scheduled tick
kubectl create job --from=cronjob/namespace-cleaner namespace-cleaner-manual -n hyperfleet-local

@openshift-ci

openshift-ci Bot commented Jun 16, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci

openshift-ci Bot commented Jun 16, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ma-hill for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Excluded labels (none allowed) (2)
  • do-not-merge/work-in-progress
  • do-not-merge/hold

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c3f2b49c-aaf0-44cc-b719-6ea917fce70f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@helm/namespace-cleaner/scripts/namespace-cleaner.sh`:
- Around line 4-7: Add input validation immediately after the variable
assignments in lines 4-7 to ensure boundary safety before any delete operations.
Validate that LABEL_SELECTOR is not empty (fail if it is), AGE_MINUTES is a
positive integer (fail if zero, negative, or non-numeric), and DRY_RUN contains
only valid boolean values (true or false). If any validation fails, log a clear
error message and exit the script immediately with a non-zero status code. This
validation acts as a fail-closed guard at the script's startup, preventing
destructive delete operations at lines 10-10 and 61-62 from executing with
invalid or unintended selector/age values.
- Around line 28-30: The curl and jq operations at lines 28-30 (fetching and
parsing BUNDLES_JSON) and lines 36-38 (extracting data from BUNDLES_JSON) can
fail with non-JSON responses or missing fields, causing the script to exit due
to `set -eo pipefail` before namespace cleanup is performed. Wrap these
fetch/parse operations with error handling (such as || true or conditional
checks) to prevent script termination on failure, allowing the script to
continue to Step 2 (namespace cleanup) even when the Maestro API calls fail.

In `@helm/namespace-cleaner/values.yaml`:
- Around line 4-7: The image reference in values.yaml uses a mutable tag
(1.32.2) which creates a supply-chain security risk for a privileged CronJob
with cluster-wide delete permissions on namespaces. Replace the tag-based
reference with an immutable digest by changing the image.repository field to
include the SHA256 digest (format: alpine/k8s@sha256:<verified-digest>),
removing the tag field entirely, and then update the cronjob.yaml image field on
line 29 to reference only the repository value without separate tag rendering.

In `@Makefile`:
- Line 243: Update the help text comment for the install-cleaner target in the
Makefile to advertise the correct override variable names. Replace the
documented variables CLEANER_LABEL_FILTER and CLEANER_AGE_HOURS with the actual
implementation variable names CLEANER_LABEL_SELECTOR and CLEANER_AGE_MINUTES
respectively. This ensures operators follow the help text and pass the correct
variables that the implementation expects.
- Around line 248-250: The helm --set command on line 248 for labelSelector is
vulnerable to incorrect parsing when CLEANER_LABEL_SELECTOR contains commas,
since Helm treats commas as key separators in --set values, potentially altering
the label selector scope. Replace the --set flag with --set-string for the
labelSelector parameter to ensure the value is passed as a plain string literal,
preventing Helm from interpreting internal commas as separators.
- Around line 244-257: The unquoted recipe variables $(CLEANER_NAMESPACE),
$(HELM_DIR), and other variables in the install-cleaner and uninstall-cleaner
targets allow shell command injection attacks. Quote each variable reference
with single quotes (e.g., '$(CLEANER_NAMESPACE)' and '$(HELM_DIR)') in all helm
command invocations within these targets to prevent shell metacharacters from
breaking argument boundaries and executing arbitrary code. This applies to the
helm upgrade command in install-cleaner and the helm uninstall command in
uninstall-cleaner.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 59e10038-21d5-4281-a292-0a1d60673fa7

📥 Commits

Reviewing files that changed from the base of the PR and between a96f995 and 380bc4b.

📒 Files selected for processing (10)
  • Makefile
  • helm/namespace-cleaner/Chart.yaml
  • helm/namespace-cleaner/scripts/namespace-cleaner.sh
  • helm/namespace-cleaner/templates/_helpers.tpl
  • helm/namespace-cleaner/templates/clusterrole.yaml
  • helm/namespace-cleaner/templates/clusterrolebinding.yaml
  • helm/namespace-cleaner/templates/configmap.yaml
  • helm/namespace-cleaner/templates/cronjob.yaml
  • helm/namespace-cleaner/templates/serviceaccount.yaml
  • helm/namespace-cleaner/values.yaml
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • openshift-hyperfleet/architecture (manual)
  • openshift-hyperfleet/hyperfleet-api (manual)
  • openshift-hyperfleet/hyperfleet-sentinel (manual)
  • openshift-hyperfleet/hyperfleet-adapter (manual)
  • openshift-hyperfleet/hyperfleet-broker (manual)

Comment on lines +4 to +7
LABEL_SELECTOR="${LABEL_SELECTOR:-hyperfleet.io/cluster-id}"
AGE_MINUTES="${AGE_MINUTES:-180}"
MAESTRO_URL="${MAESTRO_URL:-http://maestro.maestro.svc.cluster.local:8000}"
DRY_RUN="${DRY_RUN:-false}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Validate destructive runtime inputs before any delete path (CWE-20).

LABEL_SELECTOR, AGE_MINUTES, and DRY_RUN are boundary inputs. Empty selector or non-positive/invalid age can expand deletion scope to unintended namespaces. Add strict guards at startup and fail closed.

Proposed fix
 LABEL_SELECTOR="${LABEL_SELECTOR:-hyperfleet.io/cluster-id}"
 AGE_MINUTES="${AGE_MINUTES:-180}"
 MAESTRO_URL="${MAESTRO_URL:-http://maestro.maestro.svc.cluster.local:8000}"
 DRY_RUN="${DRY_RUN:-false}"
 
+if [ -z "${LABEL_SELECTOR}" ]; then
+  echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) [ERROR] LABEL_SELECTOR must be non-empty"
+  exit 1
+fi
+
+if ! [[ "${AGE_MINUTES}" =~ ^[0-9]+$ ]] || [ "${AGE_MINUTES}" -le 0 ]; then
+  echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) [ERROR] AGE_MINUTES must be a positive integer"
+  exit 1
+fi
+
+case "${DRY_RUN}" in
+  true|false) ;;
+  *)
+    echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) [ERROR] DRY_RUN must be 'true' or 'false'"
+    exit 1
+    ;;
+esac
+
 NOW=$(date +%s)
 AGE_SECONDS=$((AGE_MINUTES * 60))

As per coding guidelines, "Validate input at system boundaries (HTTP handlers, CLI parsers, webhook receivers)."

Also applies to: 10-10, 61-62

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@helm/namespace-cleaner/scripts/namespace-cleaner.sh` around lines 4 - 7, Add
input validation immediately after the variable assignments in lines 4-7 to
ensure boundary safety before any delete operations. Validate that
LABEL_SELECTOR is not empty (fail if it is), AGE_MINUTES is a positive integer
(fail if zero, negative, or non-numeric), and DRY_RUN contains only valid
boolean values (true or false). If any validation fails, log a clear error
message and exit the script immediately with a non-zero status code. This
validation acts as a fail-closed guard at the script's startup, preventing
destructive delete operations at lines 10-10 and 61-62 from executing with
invalid or unintended selector/age values.

Source: Coding guidelines

Comment on lines +28 to +30
if curl -sf --connect-timeout 5 --max-time 10 "${MAESTRO_URL}/api/maestro/v1/consumers" > /dev/null 2>&1; then
BUNDLES_JSON=$(curl -sS --max-time 30 "${MAESTRO_URL}/api/maestro/v1/resource-bundles?size=500")
total=$(echo "${BUNDLES_JSON}" | jq '.total')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle Maestro list/JSON failures without aborting namespace cleanup (CWE-703).

With set -eo pipefail, a non-JSON response or missing .items causes jq to exit non-zero and terminates the script before namespace cleanup. Guard the fetch/parse path and continue to Step 2 on failure.

Proposed fix
-if curl -sf --connect-timeout 5 --max-time 10 "${MAESTRO_URL}/api/maestro/v1/consumers" > /dev/null 2>&1; then
-  BUNDLES_JSON=$(curl -sS --max-time 30 "${MAESTRO_URL}/api/maestro/v1/resource-bundles?size=500")
-  total=$(echo "${BUNDLES_JSON}" | jq '.total')
+if curl -sf --connect-timeout 5 --max-time 10 "${MAESTRO_URL}/api/maestro/v1/consumers" > /dev/null 2>&1; then
+  if BUNDLES_JSON=$(curl -fsS --max-time 30 "${MAESTRO_URL}/api/maestro/v1/resource-bundles?size=500"); then
+    total=$(echo "${BUNDLES_JSON}" | jq -er '.total') || {
+      echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) [WARN] Invalid Maestro bundle payload; skipping bundle cleanup"
+      total=0
+      BUNDLES_JSON='{"items":[]}'
+    }
+  else
+    echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) [WARN] Failed to fetch Maestro bundles; skipping bundle cleanup"
+    total=0
+    BUNDLES_JSON='{"items":[]}'
+  fi
   echo "$(date -u +%Y-%m-%dT%H:%M:%SZ) [INFO] Fetched ${total} Maestro resource bundles"
@@
-  echo "${BUNDLES_JSON}" | jq -r '.items[] | "\(.id) \(.created_at)"' \
+  echo "${BUNDLES_JSON}" | jq -r '.items[]? | "\(.id) \(.created_at)"' \

Also applies to: 36-38

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@helm/namespace-cleaner/scripts/namespace-cleaner.sh` around lines 28 - 30,
The curl and jq operations at lines 28-30 (fetching and parsing BUNDLES_JSON)
and lines 36-38 (extracting data from BUNDLES_JSON) can fail with non-JSON
responses or missing fields, causing the script to exit due to `set -eo
pipefail` before namespace cleanup is performed. Wrap these fetch/parse
operations with error handling (such as || true or conditional checks) to
prevent script termination on failure, allowing the script to continue to Step 2
(namespace cleanup) even when the Maestro API calls fail.

Comment on lines +4 to +7
image:
repository: alpine/k8s
tag: "1.32.2"
pullPolicy: IfNotPresent

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check structure and examine the namespace-cleaner Helm chart
find helm/namespace-cleaner -type f | head -20

Repository: openshift-hyperfleet/hyperfleet-infra

Length of output: 502


🏁 Script executed:

#!/bin/bash
# Read the values.yaml file mentioned in the review
cat -n helm/namespace-cleaner/values.yaml | head -30

Repository: openshift-hyperfleet/hyperfleet-infra

Length of output: 1054


🏁 Script executed:

#!/bin/bash
# Read the cronjob template to see how image is injected
cat -n helm/namespace-cleaner/templates/cronjob.yaml | head -50

Repository: openshift-hyperfleet/hyperfleet-infra

Length of output: 1996


🏁 Script executed:

#!/bin/bash
# Check for RBAC configuration (ServiceAccount, Role, RoleBinding, ClusterRole, ClusterRoleBinding)
find helm/namespace-cleaner -name "*.yaml" | xargs grep -l "ServiceAccount\|Role\|ClusterRole" 2>/dev/null

Repository: openshift-hyperfleet/hyperfleet-infra

Length of output: 241


🏁 Script executed:

#!/bin/bash
# Search for image pinning or digest standards in architecture repo
find . -name "*code-review*" -o -name "*standards*" | head -20

Repository: openshift-hyperfleet/architecture

Length of output: 135


🏁 Script executed:

#!/bin/bash
# Read the ClusterRole to verify what permissions are granted
cat -n helm/namespace-cleaner/templates/clusterrole.yaml

Repository: openshift-hyperfleet/hyperfleet-infra

Length of output: 431


🏁 Script executed:

#!/bin/bash
# Search for image pinning/digest standards in code-review
find hyperfleet/standards/code-review -type f | xargs cat

Repository: openshift-hyperfleet/architecture

Length of output: 50391


🏁 Script executed:

#!/bin/bash
# Read the namespace-cleaner script to understand what it does
cat -n helm/namespace-cleaner/scripts/namespace-cleaner.sh | head -80

Repository: openshift-hyperfleet/hyperfleet-infra

Length of output: 4507


🏁 Script executed:

#!/bin/bash
# Search for any standards related to image, container, or supply chain security
find . -type f -name "*.md" | xargs grep -l "image\|digest\|supply.chain" 2>/dev/null | head -10

Repository: openshift-hyperfleet/architecture

Length of output: 523


🏁 Script executed:

#!/bin/bash
# Search for Helm, container registry, or image deployment standards
find . -type f \( -name "*.md" -o -name "*.yaml" \) | xargs grep -l "helm\|container\|registry\|image.*digest\|image.*pin" 2>/dev/null | head -10

Repository: openshift-hyperfleet/architecture

Length of output: 560


🏁 Script executed:

#!/bin/bash
# Check other Helm charts to see image pinning patterns
find helm -name "values.yaml" -type f | head -5

Repository: openshift-hyperfleet/hyperfleet-infra

Length of output: 167


🏁 Script executed:

#!/bin/bash
# Examine other Helm charts' image configuration
for chart in helm/*/values.yaml; do 
  echo "=== $chart ===" 
  grep -A 4 "^image:" "$chart" 2>/dev/null || echo "No image section"
done

Repository: openshift-hyperfleet/hyperfleet-infra

Length of output: 372


🏁 Script executed:

#!/bin/bash
# Check ADR-0014 for Konflux build and release standards
cat hyperfleet/adrs/0014-konflux-build-and-release.md | head -100

Repository: openshift-hyperfleet/architecture

Length of output: 4337


🏁 Script executed:

#!/bin/bash
# Check ADR-0016 for Helm OCI distribution guidance
cat hyperfleet/adrs/0016-helm-oci-distribution.md | head -100

Repository: openshift-hyperfleet/architecture

Length of output: 3639


Pin the cleaner image by digest to prevent mutable-tag drift (CWE-494).

Line 6 uses a tag-only reference (alpine/k8s:1.32.2) for a CronJob with cluster-wide delete permissions on namespaces (clusterrole.yaml line 10). Mutable tags allow silent replacement if republished upstream, creating a supply-chain vector for a privileged, automated cleanup job. Use an immutable digest instead.

To fix:

  1. Obtain the digest for alpine/k8s:1.32.2: docker inspect --format='{{index .RepoDigests 0}}' alpine/k8s:1.32.2 or check the registry directly.
  2. Update image.repository to include the digest: repository: "alpine/k8s@sha256:<verified-digest>" and remove the tag field.
  3. Update cronjob.yaml line 29 to render only the repository: image: "{{ .Values.image.repository }}".

Aligns with ADR-0016 image reference standards for supply chain integrity.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@helm/namespace-cleaner/values.yaml` around lines 4 - 7, The image reference
in values.yaml uses a mutable tag (1.32.2) which creates a supply-chain security
risk for a privileged CronJob with cluster-wide delete permissions on
namespaces. Replace the tag-based reference with an immutable digest by changing
the image.repository field to include the SHA256 digest (format:
alpine/k8s@sha256:<verified-digest>), removing the tag field entirely, and then
update the cronjob.yaml image field on line 29 to reference only the repository
value without separate tag rendering.

Source: Coding guidelines

Comment thread Makefile

# ==== Namespace Cleaner Targets ====
.PHONY: install-cleaner
install-cleaner: check-helm check-kubectl ## Install namespace cleaner CronJob (CLEANER_SCHEDULE, CLEANER_LABEL_FILTER, CLEANER_AGE_HOURS)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Target help text advertises wrong override variables.

Line 243 documents CLEANER_LABEL_FILTER and CLEANER_AGE_HOURS, but the implementation uses CLEANER_LABEL_SELECTOR and CLEANER_AGE_MINUTES (Line 44-45, 248-249). Operators following help text will pass ignored variables and run with defaults.

As per coding guidelines, "Prioritize Critical and Major severity issues… mention repeated patterns once" and this mismatch can directly misconfigure cleanup behavior.

🧰 Tools
🪛 checkmake (0.3.2)

[warning] 243-243: Target body for "install-cleaner" exceeds allowed length of 5 lines (9).

(maxbodylength)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` at line 243, Update the help text comment for the install-cleaner
target in the Makefile to advertise the correct override variable names. Replace
the documented variables CLEANER_LABEL_FILTER and CLEANER_AGE_HOURS with the
actual implementation variable names CLEANER_LABEL_SELECTOR and
CLEANER_AGE_MINUTES respectively. This ensures operators follow the help text
and pass the correct variables that the implementation expects.

Source: Coding guidelines

Comment thread Makefile
Comment on lines +244 to +257
$(call check-namespace,$(CLEANER_NAMESPACE))
helm upgrade --install namespace-cleaner $(HELM_DIR)/namespace-cleaner \
--namespace $(CLEANER_NAMESPACE) \
--set-string "schedule=$$CLEANER_SCHEDULE" \
--set "labelSelector=$$CLEANER_LABEL_SELECTOR" \
--set "ageMinutes=$$CLEANER_AGE_MINUTES" \
--set "maestroURL=$$CLEANER_MAESTRO_URL" \
--wait
@echo "OK: namespace cleaner installed in namespace $(CLEANER_NAMESPACE)"

.PHONY: uninstall-cleaner
uninstall-cleaner: check-helm check-kubectl ## Uninstall namespace cleaner CronJob
helm uninstall namespace-cleaner --namespace $(CLEANER_NAMESPACE) || true
@echo "OK: namespace cleaner uninstalled"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Unquoted recipe variables allow shell command injection (CWE-78).

Line 244-257 interpolate $(CLEANER_NAMESPACE) / $(HELM_DIR) directly into shell commands. A crafted value (for example via CLI/env override) can break argument boundaries and execute arbitrary shell tokens.

Suggested hardening
 install-cleaner: check-helm check-kubectl ## Install namespace cleaner CronJob (CLEANER_SCHEDULE, CLEANER_LABEL_FILTER, CLEANER_AGE_HOURS)
-	$(call check-namespace,$(CLEANER_NAMESPACE))
-	helm upgrade --install namespace-cleaner $(HELM_DIR)/namespace-cleaner \
-		--namespace $(CLEANER_NAMESPACE) \
+	`@if` ! printf '%s' "$(CLEANER_NAMESPACE)" | grep -Eq '^[a-z0-9]([-a-z0-9]*[a-z0-9])?$$'; then \
+		echo "ERROR: invalid CLEANER_NAMESPACE='$(CLEANER_NAMESPACE)'"; exit 1; \
+	fi
+	$(call check-namespace,$(CLEANER_NAMESPACE))
+	helm upgrade --install namespace-cleaner "$(HELM_DIR)/namespace-cleaner" \
+		--namespace "$(CLEANER_NAMESPACE)" \
 		--set-string "schedule=$$CLEANER_SCHEDULE" \
 		--set "labelSelector=$$CLEANER_LABEL_SELECTOR" \
 		--set "ageMinutes=$$CLEANER_AGE_MINUTES" \
 		--set "maestroURL=$$CLEANER_MAESTRO_URL" \
 		--wait
 	`@echo` "OK: namespace cleaner installed in namespace $(CLEANER_NAMESPACE)"

 uninstall-cleaner: check-helm check-kubectl ## Uninstall namespace cleaner CronJob
-	helm uninstall namespace-cleaner --namespace $(CLEANER_NAMESPACE) || true
+	helm uninstall namespace-cleaner --namespace "$(CLEANER_NAMESPACE)" || true

As per coding guidelines, "**/Makefile: Flag shell injection via unquoted variables in recipes" and "SEC-01 to SEC-03: Validate input at system boundaries (CLI parsers)".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 244 - 257, The unquoted recipe variables
$(CLEANER_NAMESPACE), $(HELM_DIR), and other variables in the install-cleaner
and uninstall-cleaner targets allow shell command injection attacks. Quote each
variable reference with single quotes (e.g., '$(CLEANER_NAMESPACE)' and
'$(HELM_DIR)') in all helm command invocations within these targets to prevent
shell metacharacters from breaking argument boundaries and executing arbitrary
code. This applies to the helm upgrade command in install-cleaner and the helm
uninstall command in uninstall-cleaner.

Source: Coding guidelines

Comment thread Makefile
Comment on lines +248 to +250
--set "labelSelector=$$CLEANER_LABEL_SELECTOR" \
--set "ageMinutes=$$CLEANER_AGE_MINUTES" \
--set "maestroURL=$$CLEANER_MAESTRO_URL" \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

helm --set can split CLEANER_LABEL_SELECTOR on commas, changing deletion scope.

Line 248 uses --set "labelSelector=$$CLEANER_LABEL_SELECTOR". Kubernetes label selectors commonly contain commas; Helm treats commas as key separators in --set, which can silently alter the value and drive wrong namespace selection (cleanup overreach risk).

Safer value passing
-		--set "labelSelector=$$CLEANER_LABEL_SELECTOR" \
+		--set-string "labelSelector=$$CLEANER_LABEL_SELECTOR" \
 		--set "ageMinutes=$$CLEANER_AGE_MINUTES" \
-		--set "maestroURL=$$CLEANER_MAESTRO_URL" \
+		--set-string "maestroURL=$$CLEANER_MAESTRO_URL" \

As per coding guidelines, "SEC-01 to SEC-03: Validate input at system boundaries (CLI parsers, webhook receivers, HTTP handlers)" — this CLI boundary currently allows malformed selector parsing.

📝 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
--set "labelSelector=$$CLEANER_LABEL_SELECTOR" \
--set "ageMinutes=$$CLEANER_AGE_MINUTES" \
--set "maestroURL=$$CLEANER_MAESTRO_URL" \
--set-string "labelSelector=$$CLEANER_LABEL_SELECTOR" \
--set "ageMinutes=$$CLEANER_AGE_MINUTES" \
--set-string "maestroURL=$$CLEANER_MAESTRO_URL" \
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 248 - 250, The helm --set command on line 248 for
labelSelector is vulnerable to incorrect parsing when CLEANER_LABEL_SELECTOR
contains commas, since Helm treats commas as key separators in --set values,
potentially altering the label selector scope. Replace the --set flag with
--set-string for the labelSelector parameter to ensure the value is passed as a
plain string literal, preventing Helm from interpreting internal commas as
separators.

Source: Coding guidelines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant