feat: add PodSpecBuilder for typed pod-spec assembly#85
Open
thoraxe wants to merge 1 commit into
Open
Conversation
Extract pod-spec construction logic into a reusable PodSpecBuilder struct that produces corev1.PodSpec. Handles LLM env vars, credential mounts, skills OCI volumes, MCP server config, required secrets, probes, and security context. Shared foundation for both SandboxClaim and bare pod sandbox modes. Ref: OLS-3242
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces ChangesPodSpec builder implementation and tests
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 tasks
|
@thoraxe: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![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
🧹 Nitpick comments (4)
controller/proposal/podspec_builder.go (3)
20-25: ⚡ Quick winUnused
stepparameter.The
step stringparameter is accepted but never used. Remove it or use it.func (b *PodSpecBuilder) Build( agent *agenticv1alpha1.Agent, llm *agenticv1alpha1.LLMProvider, tools *agenticv1alpha1.ToolsSpec, - step string, ) (*corev1.PodSpec, error) {🤖 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 `@controller/proposal/podspec_builder.go` around lines 20 - 25, The Build method on PodSpecBuilder declares an unused parameter "step string" — remove the unused parameter from the PodSpecBuilder.Build signature and update any callers to stop passing the step argument (or if the step is intended to influence pod creation, thread it into the function body and use it where appropriate); locate the method by name PodSpecBuilder.Build and any call sites that pass a step value and either delete the argument at those call sites or implement usage inside Build (e.g., selecting containers/labels/annotations based on step) so the parameter is no longer unused.
26-31: ⚡ Quick winError strings should use const pattern.
Per coding guidelines: define errors using
const ErrFoo = "…"and wrap withfmt.Errorf("%s: %w", …).Proposed fix
+const ( + ErrAgentRequired = "agent is required" + ErrLLMProviderRequired = "LLMProvider is required" +) + func (b *PodSpecBuilder) Build( ... if agent == nil { - return nil, fmt.Errorf("agent is required") + return nil, fmt.Errorf(ErrAgentRequired) } if llm == nil { - return nil, fmt.Errorf("LLMProvider is required") + return nil, fmt.Errorf(ErrLLMProviderRequired) }🤖 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 `@controller/proposal/podspec_builder.go` around lines 26 - 31, Define constant error messages (e.g. const ErrAgentRequired = "agent is required" and const ErrLLMRequired = "LLMProvider is required") and replace the inline fmt.Errorf calls in the nil checks for agent and llm with returns that use those constants (e.g. return nil, fmt.Errorf("%s", ErrAgentRequired) or errors.New(ErrAgentRequired)); update the nil-checks referencing variables agent and llm in podspec_builder (the constructor/initializer function containing those checks) to use the new consts so error strings follow the const pattern.
160-164: ⚡ Quick winMultiple
skillsentries aren’t a bug here—they’re intentionally “first non-empty only.”
buildSkillsonly usesskills[0], and the project spec explicitly documents that when multipleskillsentries exist inToolsSpec, template derivation applies image/path patching based on the first non-empty skills source (current behavior). Multi-image support is tracked as a planned change (OLS-2894).🤖 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 `@controller/proposal/podspec_builder.go` around lines 160 - 164, buildSkills currently assumes the first element is the active source by directly using skills[0], which hides the intended "first non-empty wins" behavior; update buildSkills to explicitly iterate the skills slice to find the first agenticv1alpha1.SkillsSource with a non-empty Image (assign that to s) and fall back to returning nil,nil if none found, leaving the overall semantics as "first non-empty only" (no multi-image support) so callers and future readers see the intent clearly; refer to the buildSkills function and the SkillsSource type when making this change.controller/proposal/podspec_builder_test.go (1)
12-19: 💤 Low value
envToMapsilently ignoresValueFromenv vars.This helper only captures
env.Value, notenv.ValueFrom. Tests relying on this helper won't catch secret-backed env vars. TheTestPodSpecBuilder_RequiredSecrets_EnvVartest correctly iterates manually, but adding a comment or separate helper would prevent future misuse.🤖 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 `@controller/proposal/podspec_builder_test.go` around lines 12 - 19, The helper envToMap currently only reads env.Value and ignores env.ValueFrom, which hides secret-backed env vars; update envToMap (or add a new helper envToMapIncludeValueFrom) to detect when corev1.EnvVar.ValueFrom is non-nil and represent it in the map (e.g., set the map entry to a deterministic descriptive string containing the ValueFrom specifics such as SecretKeyRef.Name and SecretKeyRef.Key or a fixed tag like "<ValueFrom:Secret:NAME:KEY>") so tests like TestPodSpecBuilder_RequiredSecrets_EnvVar can assert secret-backed env vars rather than missing them. Ensure you reference the envToMap (or new helper) and ValueFrom/SecretKeyRef fields when locating the code to change.
🤖 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 `@controller/proposal/podspec_builder.go`:
- Around line 176-189: When s.Paths is empty you currently create the skills
volume but never add any VolumeMounts, so the container can't access the image
content; update the podspec_builder logic that builds mounts (the mounts slice
loop around s.Paths and the baseMountPath variable) to add a fallback
VolumeMount that mounts the entire skills volume at baseMountPath (e.g.,
MountPath set to baseMountPath, Name "skills", no SubPath or empty SubPath,
ReadOnly true) when len(s.Paths) == 0 so the whole image is available to the
container.
- Around line 210-223: The code appends a corev1.Volume for each MCP header that
uses a secret, causing duplicate volumes when multiple headers reference the
same secret; fix by adding a seen set (e.g., map[string]bool or
map[string]struct{}) keyed by the secret name before the header loop and only
append to volumes when the secret name (h.ValueFrom.Secret.Name) has not been
seen yet, then mark it seen; likewise avoid duplicate mounts by checking the
volName or secret name against a seenMounts set before appending to mounts;
apply this logic where MCPHeaderSourceTypeSecret is checked (symbols: h, he,
volumes, mounts, volName, mcpHeadersMountRoot, MCPHeaderSourceTypeSecret) so
duplicate volume/mount entries are not created.
- Around line 264-270: The code hardcodes SecretKeySelector.Key = "token"
causing silent misses; update the API and usage so the secret data key is
configurable: add a Key string field to SecretMountEnvVarConfig in
api/v1alpha1/tools_types.go (with CRD/json tags and a sensible default), change
the podspec builder to read that field instead of the literal "token" (fall back
to "token" when the field is empty), and update any constructors/decoding sites
that create SecretMountEnvVarConfig so the new field is populated or defaulted
appropriately.
---
Nitpick comments:
In `@controller/proposal/podspec_builder_test.go`:
- Around line 12-19: The helper envToMap currently only reads env.Value and
ignores env.ValueFrom, which hides secret-backed env vars; update envToMap (or
add a new helper envToMapIncludeValueFrom) to detect when
corev1.EnvVar.ValueFrom is non-nil and represent it in the map (e.g., set the
map entry to a deterministic descriptive string containing the ValueFrom
specifics such as SecretKeyRef.Name and SecretKeyRef.Key or a fixed tag like
"<ValueFrom:Secret:NAME:KEY>") so tests like
TestPodSpecBuilder_RequiredSecrets_EnvVar can assert secret-backed env vars
rather than missing them. Ensure you reference the envToMap (or new helper) and
ValueFrom/SecretKeyRef fields when locating the code to change.
In `@controller/proposal/podspec_builder.go`:
- Around line 20-25: The Build method on PodSpecBuilder declares an unused
parameter "step string" — remove the unused parameter from the
PodSpecBuilder.Build signature and update any callers to stop passing the step
argument (or if the step is intended to influence pod creation, thread it into
the function body and use it where appropriate); locate the method by name
PodSpecBuilder.Build and any call sites that pass a step value and either delete
the argument at those call sites or implement usage inside Build (e.g.,
selecting containers/labels/annotations based on step) so the parameter is no
longer unused.
- Around line 26-31: Define constant error messages (e.g. const ErrAgentRequired
= "agent is required" and const ErrLLMRequired = "LLMProvider is required") and
replace the inline fmt.Errorf calls in the nil checks for agent and llm with
returns that use those constants (e.g. return nil, fmt.Errorf("%s",
ErrAgentRequired) or errors.New(ErrAgentRequired)); update the nil-checks
referencing variables agent and llm in podspec_builder (the
constructor/initializer function containing those checks) to use the new consts
so error strings follow the const pattern.
- Around line 160-164: buildSkills currently assumes the first element is the
active source by directly using skills[0], which hides the intended "first
non-empty wins" behavior; update buildSkills to explicitly iterate the skills
slice to find the first agenticv1alpha1.SkillsSource with a non-empty Image
(assign that to s) and fall back to returning nil,nil if none found, leaving the
overall semantics as "first non-empty only" (no multi-image support) so callers
and future readers see the intent clearly; refer to the buildSkills function and
the SkillsSource type when making this change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6723b519-c2ed-40f7-a405-0de520fabcc0
📒 Files selected for processing (2)
controller/proposal/podspec_builder.gocontroller/proposal/podspec_builder_test.go
Comment on lines
+176
to
+189
| var mounts []corev1.VolumeMount | ||
| if len(s.Paths) > 0 { | ||
| baseMountPath := "/app/skills" | ||
| for _, p := range s.Paths { | ||
| subPath := strings.TrimPrefix(p, "/") | ||
| skillName := path.Base(p) | ||
| mounts = append(mounts, corev1.VolumeMount{ | ||
| Name: "skills", | ||
| MountPath: path.Join(baseMountPath, skillName), | ||
| SubPath: subPath, | ||
| ReadOnly: true, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
No volume mounts when Paths is empty.
When skills[0].Paths is empty, a volume is created but no mounts are added. The container can't access the skills image content. Either mount the entire image at a base path, or document this as intentional.
🤖 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 `@controller/proposal/podspec_builder.go` around lines 176 - 189, When s.Paths
is empty you currently create the skills volume but never add any VolumeMounts,
so the container can't access the image content; update the podspec_builder
logic that builds mounts (the mounts slice loop around s.Paths and the
baseMountPath variable) to add a fallback VolumeMount that mounts the entire
skills volume at baseMountPath (e.g., MountPath set to baseMountPath, Name
"skills", no SubPath or empty SubPath, ReadOnly true) when len(s.Paths) == 0 so
the whole image is available to the container.
Comment on lines
+210
to
+223
| if h.ValueFrom.Type == agenticv1alpha1.MCPHeaderSourceTypeSecret { | ||
| he.SecretName = h.ValueFrom.Secret.Name | ||
| volName := "mcp-header-" + h.ValueFrom.Secret.Name | ||
| volumes = append(volumes, corev1.Volume{ | ||
| Name: volName, | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Secret: &corev1.SecretVolumeSource{SecretName: h.ValueFrom.Secret.Name}, | ||
| }, | ||
| }) | ||
| mounts = append(mounts, corev1.VolumeMount{ | ||
| Name: volName, | ||
| MountPath: mcpHeadersMountRoot + "/" + h.ValueFrom.Secret.Name, | ||
| ReadOnly: true, | ||
| }) |
There was a problem hiding this comment.
Duplicate volumes if same secret used in multiple MCP headers.
If two MCP servers reference the same secret for headers, this creates duplicate volume entries with the same name, causing pod creation to fail.
Proposed fix: deduplicate by tracking seen secrets
+ seenSecrets := make(map[string]bool)
for _, s := range servers {
entry := mcpServerEnvEntry{...}
for _, h := range s.Headers {
he := mcpHeaderEnvEntry{...}
if h.ValueFrom.Type == agenticv1alpha1.MCPHeaderSourceTypeSecret {
he.SecretName = h.ValueFrom.Secret.Name
volName := "mcp-header-" + h.ValueFrom.Secret.Name
+ if !seenSecrets[h.ValueFrom.Secret.Name] {
+ seenSecrets[h.ValueFrom.Secret.Name] = true
volumes = append(volumes, corev1.Volume{...})
mounts = append(mounts, corev1.VolumeMount{...})
+ }
}
entry.Headers = append(entry.Headers, he)
}
}🤖 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 `@controller/proposal/podspec_builder.go` around lines 210 - 223, The code
appends a corev1.Volume for each MCP header that uses a secret, causing
duplicate volumes when multiple headers reference the same secret; fix by adding
a seen set (e.g., map[string]bool or map[string]struct{}) keyed by the secret
name before the header loop and only append to volumes when the secret name
(h.ValueFrom.Secret.Name) has not been seen yet, then mark it seen; likewise
avoid duplicate mounts by checking the volName or secret name against a
seenMounts set before appending to mounts; apply this logic where
MCPHeaderSourceTypeSecret is checked (symbols: h, he, volumes, mounts, volName,
mcpHeadersMountRoot, MCPHeaderSourceTypeSecret) so duplicate volume/mount
entries are not created.
Comment on lines
+264
to
+270
| SecretKeyRef: &corev1.SecretKeySelector{ | ||
| LocalObjectReference: corev1.LocalObjectReference{Name: s.Name}, | ||
| Key: "token", | ||
| Optional: &optional, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the SecretMountEnvVarConfig type definition for available fields
ast-grep --pattern 'type SecretMountEnvVarConfig struct {
$$$
}'Repository: openshift/lightspeed-agentic-operator
Length of output: 944
Hardcoded "token" secret data key for env-var mounts (potential silent env injection failure)
controller/proposal/podspec_builder.go always sets SecretKeyRef.Key to "token" for env-var secret mounts, but api/v1alpha1/tools_types.go’s SecretMountEnvVarConfig only exposes Name (no configurable secret data key). With Optional: true, a missing/mismatched "token" entry won’t fail—env vars may not be populated. Add a configurable key field (or clearly document that secret data["token"] is mandatory).
🤖 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 `@controller/proposal/podspec_builder.go` around lines 264 - 270, The code
hardcodes SecretKeySelector.Key = "token" causing silent misses; update the API
and usage so the secret data key is configurable: add a Key string field to
SecretMountEnvVarConfig in api/v1alpha1/tools_types.go (with CRD/json tags and a
sensible default), change the podspec builder to read that field instead of the
literal "token" (fall back to "token" when the field is empty), and update any
constructors/decoding sites that create SecretMountEnvVarConfig so the new field
is populated or defaulted appropriately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PodSpecBuilderstruct that assembles a typedcorev1.PodSpecfrom agent container image and resolved step configJira: OLS-3242
Stacked PR: 1 of 5 — targets
mainTest plan
🤖 Generated with Claude Code