Repository navigation
fix(mainhandler): re-render persisted schedules from the current template at startup - #447
Conversation
…late at startup Signed-off-by: Youssef Omar Bouden <youssef.bouden2002@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
matthyx
left a comment
There was a problem hiding this comment.
Approve — no blocking findings in the three-file migration at 8891d833d2f47c993de588b59fc59bdba0a4c0d7, targeting main at c29b7dae93f3c5e44e64b428a4d6162e3b200d7c (PR merge base 6bd65ac2c08716930d00313bcab897ebdde928ab).
The fix is needed: #446 and the upgrade finding on helm-charts#971 describe persisted HTTP schedules. Current main's update handlers still change only schedule/annotations; it has no equivalent startup migration. Replacing template ConfigMaps in merged helm-charts#964 cannot update those persisted CronJobs. This implementation replaces the complete execution PodSpec for all three legacy tiers, retains schedule/time zone/metadata and the schedule's request ConfigMap, skips current jobs and old/missing templates, and continues after individual failures. No dependencies or unrelated changes are introduced.
History: searched operator PRs across open, closed and merged states and related issues using schedule, cronjob, triggerAction, http-request, persisted, migration, MigrateScheduleCronJobs and cronjobmigration, up to 100 results/query. No duplicate or superseding persisted-schedule migration surfaced. The broad http-request query reached the 100-result cap; that cap and search indexing/body coverage limit completeness. #431 concerns listener configuration; #435 supplies the already-merged parent-tracking repair. #411 was explicitly rejected because its allowlist broke intended CLI remediation. That objection does not apply: this PR leaves the HTTP endpoint available. Earlier #964 image/command and tracking objections were addressed before its approval; #447 supplies the subsequent persisted-resource repair requested on #971.
Validation: in a credential-free, network-isolated Bubblewrap sandbox with read-only source/dependencies and disposable writable build cache, go test -race ./mainhandler/ passed; go build -o /tmp/operator . and go vet . ./mainhandler/ passed on linux/amd64 with Go 1.26.9. git diff --check passed. Tests cover three tiers, preservation, compatibility skips and continuation after update failure. Independent code and architecture reviews found no material blocker. No live-cluster upgrade, real scheduler execution, or failing-before regression run was performed. Startup cancellation and retry improvements are optional follow-ups to the documented best-effort migration.
At the final context check there were no existing reviews or inline findings on #447. Integration build and reported security/compliance checks passed; the cross-platform build remained running. Pending CI alone is not a blocker. The chart still needs a released operator containing this fix before removing legacy ingress, as its author already acknowledged on #971.
|
Thanks for the review and the merge; the three tiers and the kept request ConfigMap were the parts I most wanted a second pair of eyes on. |
Overview
A backend schedule CronJob is rendered from the chart's template ConfigMap once, when it is created;
updateVulnScanCronJobandupdateKubescapeCronJobrewrite only the schedule and the annotations. kubescape/helm-charts#964 moved the templates from anhttp-requestpod posting tooperator:4002/v1/triggerActionto a pod that creates an OperatorCommand, and kubescape/helm-charts#971 drops the trigger-port ingress rule, so a schedule created before #964 would keep posting to a port the NetworkPolicy no longer admits (matthyx's P2 on #971).MigrateScheduleCronJobsruns once at startup, after the main handler is set up. It lists the CronJobs in the operator namespace and, for each whose pods carry thearmo.tierlabel of a schedule (vuln-scan,kubescape-scan,registry-scan) and whose container args still namev1/triggerAction, re-renders the PodSpec from that tier's template ConfigMap with the request volume pointing at the schedule's own ConfigMap, assetCronJobForTriggerRequestdoes at creation. Name, schedule, time zone, labels, annotations and the request ConfigMap stay as they are. A template that still renders the HTTP call (operator restarted before the chart upgrade) or that is not installed (component disabled) leaves its schedules alone. Every schedule is tried; the first failed update is returned and logged as a warning inmain.How to Test
TestMigrateScheduleCronJobsRerendersTheSchedulesThatStillPostToTheOperator: a fake cluster with the three current templates, one pre-#964 schedule and one current schedule per tier, plus a CronJob without the schedule label; only the three old schedules are updated, each gets the current template's PodSpec with its own request ConfigMap, and its schedule, time zone, labels and annotations are unchanged.TestMigrateScheduleCronJobsLeavesSchedulesAloneWhenTheTemplateCannotHelp: an old schedule stays when its template still posts to the operator and when the template is not installed.TestMigrateScheduleCronJobsReportsTheFirstFailureAfterTryingEverySchedule: a failing update is returned and the other schedule is still migrated.main.gobuilds and vets for linux/amd64 (the root package does not build on macOS because of inspektor-gadget's build constraints).Related issues/PRs:
Summary by CodeRabbit
New Features
Bug Fixes