No issues found across 12 files

🤖 Kelos API Reviewer Agent @gjkim42

API Design Review

Verdict: APPROVE
Scope: Adds four optional fields to PodOverrides (volumes, volumeMounts, podSecurityContext, containerSecurityContext), enabling spawned Task pods to attach user-supplied volumes/mounts and declare security contexts so they can land in PSS-restricted namespaces.

The design is solid: changes are purely additive, each new field is +optional with a pointer or slice, JSON tags are camelCase, and the embedded corev1.* types are the right shape for these semantics. Generated artifacts (zz_generated.deepcopy.go, CRDs) are included. Tests cover the meaningful paths (append, reserved-name rejection, duplicate rejection, FSGroup preservation/override, container SC replacement). Below are documentation clarifications and a few non-blocking suggestions.

Findings

Documentation accuracy

• api/v1alpha1/task_types.go:103-108 — The PodSecurityContext doc says "fields set here override Kelos defaults; fields left unset retain Kelos defaults". The implementation only carries forward FSGroup (internal/controller/job_builder.go:621-630); every other field is taken verbatim from the user struct. This happens to be correct today only because FSGroup is the only field Kelos defaults (job_builder.go:365-367). If a future change adds another defaulted field on the pod SC, the doc will silently become wrong. Suggest restating the contract narrowly:

  PodSecurityContext replaces the pod-level security context. As a carve-out, if fsGroup is unset here, Kelos's default fsGroup is preserved so the agent user retains read/write access to the workspace volume. All other fields are taken as-is from this struct.

• api/v1alpha1/task_types.go:96-101 — VolumeMounts says names must reference a user-supplied volume "from Volumes or a Kelos-managed volume (workspace, kelos-plugin)". That is documenting the implementation, but kelos-plugin is only present when plugin support is active for the Task; mounting it from PodOverrides.VolumeMounts will succeed for Tasks that have plugins and fail (at pod admission) for those that don't. Worth either calling that out or recommending users only reference their own volumes entries.

Validation surface

• internal/controller/job_builder.go:610-619 — Reserved-name and duplicate-name validation runs at Build() time, so users find out about the problem only when the Job materialises (status moves to a controller error, not an apply-time rejection). Since the Task spec is immutable post-create, this is a one-shot UX cost, but consider a CRD-level CEL rule, e.g.:

  // +kubebuilder:validation:XValidation:rule="!self.exists(v, v.name == 'workspace' || v.name == 'kelos-plugin')",message="podOverrides.volumes name must not be 'workspace' or 'kelos-plugin'"
  // +kubebuilder:validation:XValidation:rule="size(self.map(v, v.name)) == size(self)",message="podOverrides.volumes names must be unique"
  

  on the field. This gives operators feedback at kubectl apply rather than at reconcile, and keeps the reserved-name list visible in the API surface (today it is hidden in job_builder.go:756-760).

• There is no controller-side check that each VolumeMounts[].name resolves to either a reserved volume or one of PodOverrides.Volumes. The kubelet/apiserver will reject the pod, but a clearer pre-flight error would shorten the debugging loop. Optional.

List semantics (minor)

• api/v1alpha1/task_types.go:94, 101 — []corev1.Volume and []corev1.VolumeMount are emitted as atomic lists in the CRD (internal/manifests/install-crd.yaml:1097, 3009), whereas upstream PodSpec.Volumes is +listType=map +listMapKey=name. For Task this is a no-op (spec is immutable), but for TaskSpawner.spec.taskTemplate.podOverrides it means any SSA touching volumes replaces the whole list rather than merging by name. Adding the markers is the more conventional shape; matching the existing Env []corev1.EnvVar style (also atomic) is acceptable too — flagging only for awareness, not as a blocker.

containerSecurityContext semantics

• internal/controller/job_builder.go:632-634 — When set, this completely replaces the agent container's SecurityContext. The agent container has no Kelos-set defaults today, so "replace" matches "set". That is fine, but document it explicitly on the field so future contributors don't accidentally introduce a Kelos default and silently lose it (mirror the wording used on PodSecurityContext).

Field-by-field convention check

• All four fields are camelCase JSON, marked +optional, use pointer/slice types, and have godoc — good.
• No raw primitives that should be resource.Quantity/metav1.Duration/metav1.Time — embedded corev1 types handle those internally.
• Prefix naming (podSecurityContext, containerSecurityContext) is justified to disambiguate at the same struct level.
• Backwards compatible: existing clients ignoring the four new fields see the same generated Job they would today.

Suggestions (optional)

• Consider +kubebuilder:validation:MaxItems on Volumes and VolumeMounts to bound API blast radius (small numbers — most realistic use cases need < 10).
• The reserved-volume-name list (workspace, kelos-plugin) is referenced in three places: the field godoc, the validator (job_builder.go:758-760), and tests. Consider exporting ReservedVolumeNames from the controller package (or moving it to the api package) so the list is single-sourced — handy if more managed volumes get added later.
• The internal/helmchart/render.go fix (replacing strings.Split(..., "---\n") with yamlutil.NewYAMLReader) is the right call and not strictly part of the API change, but is well-motivated by the new CRD content. Nice catch.

/kelos needs-input

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds volumes, volumeMounts, podSecurityContext, and containerSecurityContext to PodOverrides so spawned Task pods can attach extra volumes/mounts and declare security contexts; also fixes a latent helmchart-render YAML splitter that the new corev1 types tripped.

Findings Overview

Findings

Documentation accuracy (Conventions)

• [P3] api/v1alpha1/task_types.go:103-108 — PodSecurityContext's godoc reads "fields set here override Kelos defaults; fields left unset retain Kelos defaults". The implementation in internal/controller/job_builder.go:621-630 only carries FSGroup forward; every other field is taken verbatim from the user struct. This happens to be accurate today only because FSGroup is the sole field Kelos defaults (job_builder.go:365-367); if a future change defaults another pod-SC field, the doc silently becomes wrong. Restate the carve-out narrowly so the contract is explicit, e.g. "Replaces the pod-level security context. As a carve-out, when fsGroup is unset here Kelos's default fsGroup is preserved so the agent user retains workspace access; all other fields are taken as-is."
• [P3] api/v1alpha1/task_types.go:96-101 — VolumeMounts doc says names must reference a user-supplied volume "from Volumes or a Kelos-managed volume (workspace, kelos-plugin)". kelos-plugin only exists when the Task's AgentConfig declares plugins or skills (job_builder.go:508-519); referencing it from a Task without those will succeed at validation and fail at pod admission. Either drop kelos-plugin from the user-facing doc or add the conditional.

PSS-restricted scope (Correctness/Documentation)

• [P2] api/v1alpha1/task_types.go:110-115 and internal/controller/job_builder.go:632-634 — ContainerSecurityContext only applies to the agent container, but PSS-restricted is enforced on every container in the pod, including init containers. The pre-existing init containers git-clone (job_builder.go:388-397), plugin-setup (job_builder.go:527-535), and skills-install (job_builder.go:543-553) set at most RunAsUser and do not set allowPrivilegeEscalation=false or capabilities.drop=[ALL]. The result is that for any Task with a workspace or with an AgentConfig that includes plugins/skills — i.e., the common case — the spawned pod will still be rejected by a PSS-restricted namespace despite the user setting containerSecurityContext. The headline rationale of the PR ("the pod can land in a PSS-restricted namespace") therefore only holds for Tasks with no workspace and no plugin/skill setup. Either apply restricted-compatible defaults to the init containers (allowPrivilegeEscalation=false, capabilities.drop=[ALL], readOnlyRootFilesystem=true) — they are all simple shell entrypoints that don't need privileged capabilities — or document this limitation in the new field's godoc and in docs/reference.md so operators understand the boundary.

Suggestions (optional)

• [P3] Reserved-name validation runs in JobBuilder.Build(), so a bad volume name surfaces as a controller reconcile error rather than a kubectl apply rejection. A CRD-level CEL rule on podOverrides.volumes (name not in ['workspace','kelos-plugin'] and uniqueness by name) would shorten the feedback loop and put the reserved-name list in the API surface; the validator can stay as a defense-in-depth layer.
• [P3] Consider exporting the reserved-volume-name set or moving it to the api package so the godoc, the validator (job_builder.go:756-760), and the tests all reference one source — handy if more managed volumes are added.
• [P3] The splitYAMLDocs switch from strings.Split(content, "---\n") to yamlutil.NewYAMLReader is the correct fix and is well-motivated by the new PodSecurityContext.fsGroup description, which contains the literal rw-rw----\n (4 dashes + newline) that the naive split treated as a document boundary. Nice catch.

Key takeaways

• Implementation, deepcopy, CRDs, and tests are coherent and the YAML-split fix is correct.
• The PSS-restricted promise in the PR description is partially broken for Tasks that have a workspace or a plugin/skill-bearing AgentConfig; tightening the godoc or extending the security context to init containers would close the gap.