OLS-3200: implement generic LIGHTSPEED_* env var contract (operator)#27
OLS-3200: implement generic LIGHTSPEED_* env var contract (operator)#27onmete wants to merge 5 commits into
Conversation
…200) Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@onmete: This pull request references OLS-3200 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
More reviews will be available in 39 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR unifies LLM credential mounting and environment variable configuration across all provider types. The specification defines the new ChangesUnified LLM Credential Mounting Architecture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Remove brittle assertNoSDKEnvVars denylist — positive assertions on LIGHTSPEED_* vars already verify correct behavior. Add doc comments to each patchLLMCredentials test describing what it exercises. Co-authored-by: Cursor <cursoragent@cursor.com>
- providerTypeString lowercases model provider via strings.ToLower - Remove LIGHTSPEED_MODE from spec rule 16a (per OLS-3204) - Replace vague "When applicable" with explicit provider conditions Co-authored-by: Cursor <cursoragent@cursor.com>
![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
🤖 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/sandbox_templates_test.go`:
- Around line 245-267: Update the assertCredentialVolumeMount helper to validate
the full credential contract: in addition to verifying the volume mount named
llmCredsVolumeName at llmCredsMountPath and that it is readOnly (current checks
in assertCredentialVolumeMount), also assert the container has an envFrom entry
that references the secret (inspect container["envFrom"] for a secretRef with a
name) to ensure unconditional secret injection, and assert the container's env
does NOT include legacy SDK-specific variables (e.g. OPENAI_API_KEY,
AZURE_OPENAI_KEY, BEDROCK_API_KEY) so only the LIGHTSPEED_* / secret-based
config is present; locate these checks inside assertCredentialVolumeMount (which
reads containers via unstructured.NestedSlice) and fail the test with
t.Error/t.Fatalf when the envFrom secret is missing or legacy env vars are
present.
In `@controller/proposal/sandbox_templates.go`:
- Around line 31-33: The llmCredsMountPath constant currently uses
"/var/run/secrets/llm-credentials" but the spec requires the trailing slash
"/var/run/secrets/llm-credentials/"; update the llmCredsMountPath declaration to
include the trailing slash and ensure any related uses/tests referencing
llmCredsMountPath (and the new tests added) are aligned to the exact literal
with the trailing slash so the operator/spec/sandbox contract matches precisely.
- Around line 267-334: Replace inline error message strings in the fmt.Errorf
calls with package-level constant labels and wrap the original error using
fmt.Errorf("%s: %w", Err..., err); add const declarations (e.g.
ErrSetLightspeedProvider, ErrSetLightspeedModel, ErrAddCredentialsEnvFrom,
ErrAddCredentialsVolume, ErrMountCredentials, ErrSetProviderURL,
ErrSetModelProvider, ErrSetProviderProject, ErrSetProviderRegion,
ErrSetProviderAPIVersion) and update all occurrences in the code that call
setEnvVar, addEnvFromSecret, addSecretVolume, addVolumeMount and the
provider-specific branches to use fmt.Errorf("%s: %w", <const>, err) instead of
inline strings so the error labels are consistent with the repository guideline.
🪄 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: e9e60bfc-3ae5-4983-b4f9-09c9e61eae33
📒 Files selected for processing (3)
.ai/spec/what/sandbox-execution.mdcontroller/proposal/sandbox_templates.gocontroller/proposal/sandbox_templates_test.go
| func assertCredentialVolumeMount(t *testing.T, tmpl *unstructured.Unstructured) { | ||
| t.Helper() | ||
| containers, _, _ := unstructured.NestedSlice(tmpl.Object, "spec", "podTemplate", "spec", "containers") | ||
| if len(containers) == 0 { | ||
| t.Fatal("no containers") | ||
| } | ||
| container := containers[0].(map[string]any) | ||
| mounts, _, _ := unstructured.NestedSlice(container, "volumeMounts") | ||
| found := false | ||
| for _, m := range mounts { | ||
| mount := m.(map[string]any) | ||
| if mount["name"] == llmCredsVolumeName && mount["mountPath"] == llmCredsMountPath { | ||
| found = true | ||
| if mount["readOnly"] != true { | ||
| t.Error("credential volume mount should be readOnly") | ||
| } | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| t.Errorf("missing credential volume mount at %s", llmCredsMountPath) | ||
| } | ||
| } |
There was a problem hiding this comment.
Broaden the shared helper to assert the full credential contract.
Right now it only checks the volume mount, so the new OpenAI/Azure/Bedrock cases no longer verify the unconditional envFrom secret injection. It also leaves room for a regression where legacy SDK-specific env vars come back alongside LIGHTSPEED_* and the tests still pass.
🤖 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/sandbox_templates_test.go` around lines 245 - 267, Update
the assertCredentialVolumeMount helper to validate the full credential contract:
in addition to verifying the volume mount named llmCredsVolumeName at
llmCredsMountPath and that it is readOnly (current checks in
assertCredentialVolumeMount), also assert the container has an envFrom entry
that references the secret (inspect container["envFrom"] for a secretRef with a
name) to ensure unconditional secret injection, and assert the container's env
does NOT include legacy SDK-specific variables (e.g. OPENAI_API_KEY,
AZURE_OPENAI_KEY, BEDROCK_API_KEY) so only the LIGHTSPEED_* / secret-based
config is present; locate these checks inside assertCredentialVolumeMount (which
reads containers via unstructured.NestedSlice) and fail the test with
t.Error/t.Fatalf when the envFrom secret is missing or legacy env vars are
present.
There was a problem hiding this comment.
Fixed in 57f7333. Folded envFrom check into renamed assertCredentialMounts — all 6 provider tests now verify both envFrom and volume mount unconditionally. Skipped the SDK denylist suggestion: a static blocklist rots and the positive LIGHTSPEED_* assertions already catch regressions.
| llmCredsMountPath = "/var/run/secrets/llm-credentials" | ||
| llmCredsVolumeName = "llm-credentials" | ||
| mcpHeadersMountRoot = "/var/secrets/mcp" |
There was a problem hiding this comment.
Align the credential mount path literal with the documented contract.
The spec describes /var/run/secrets/llm-credentials/, but this constant and the new tests use /var/run/secrets/llm-credentials. Even if the filesystem behavior is equivalent, the operator/spec/sandbox contract should use one exact literal before the companion PR merges.
🤖 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/sandbox_templates.go` around lines 31 - 33, The
llmCredsMountPath constant currently uses "/var/run/secrets/llm-credentials" but
the spec requires the trailing slash "/var/run/secrets/llm-credentials/"; update
the llmCredsMountPath declaration to include the trailing slash and ensure any
related uses/tests referencing llmCredsMountPath (and the new tests added) are
aligned to the exact literal with the trailing slash so the
operator/spec/sandbox contract matches precisely.
There was a problem hiding this comment.
Skipping. The operator constant is a Kubernetes volumeMounts.mountPath where trailing slash is irrelevant — the kubelet treats both identically. The sandbox uses the same path as a prefix for f"{path}/filename" where no trailing slash is correct. The spec trailing slash is a documentation convention, not a contract violation.
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER", providerTypeString(llm.Spec.Type)); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER: %w", err) | ||
| } | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_MODEL", model); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_MODEL: %w", err) | ||
| } | ||
| if err := addEnvFromSecret(tmpl, secretName); err != nil { | ||
| return fmt.Errorf("add credentials envFrom: %w", err) | ||
| } | ||
| if err := setEnvVar(tmpl, "ANTHROPIC_MODEL", model); err != nil { | ||
| return fmt.Errorf("set ANTHROPIC_MODEL: %w", err) | ||
| if err := addSecretVolume(tmpl, llmCredsVolumeName, secretName); err != nil { | ||
| return fmt.Errorf("add credentials volume: %w", err) | ||
| } | ||
|
|
||
| if u := providerURL(llm); u != "" { | ||
| if err := setEnvVar(tmpl, providerURLEnvVar(llm.Spec.Type), u); err != nil { | ||
| return fmt.Errorf("set provider URL: %w", err) | ||
| } | ||
| if err := addVolumeMount(tmpl, llmCredsVolumeName, llmCredsMountPath, true); err != nil { | ||
| return fmt.Errorf("mount credentials: %w", err) | ||
| } | ||
|
|
||
| switch llm.Spec.Type { | ||
| case agenticv1alpha1.LLMProviderAnthropic: | ||
| if u := providerURL(llm); u != "" { | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_URL", u); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_URL: %w", err) | ||
| } | ||
| } | ||
| case agenticv1alpha1.LLMProviderGoogleCloudVertex: | ||
| cfg := llm.Spec.GoogleCloudVertex | ||
| if err := setEnvVar(tmpl, "CLAUDE_CODE_USE_VERTEX", "1"); err != nil { | ||
| return fmt.Errorf("set CLAUDE_CODE_USE_VERTEX: %w", err) | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_MODEL_PROVIDER", strings.ToLower(string(cfg.ModelProvider))); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_MODEL_PROVIDER: %w", err) | ||
| } | ||
| if err := setEnvVar(tmpl, "GCP_PROJECT", cfg.ProjectID); err != nil { | ||
| return fmt.Errorf("set GCP_PROJECT: %w", err) | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_PROJECT", cfg.ProjectID); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_PROJECT: %w", err) | ||
| } | ||
| if err := setEnvVar(tmpl, "GCP_REGION", cfg.Region); err != nil { | ||
| return fmt.Errorf("set GCP_REGION: %w", err) | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_REGION", cfg.Region); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_REGION: %w", err) | ||
| } | ||
| if err := setEnvVar(tmpl, "GOOGLE_APPLICATION_CREDENTIALS", vertexCredsMountPath+"/"+vertexCredsFileName); err != nil { | ||
| return fmt.Errorf("set GOOGLE_APPLICATION_CREDENTIALS: %w", err) | ||
| } | ||
| if err := addSecretVolume(tmpl, llmCredsVolumeName, secretName); err != nil { | ||
| return fmt.Errorf("add Vertex credentials volume: %w", err) | ||
| if u := providerURL(llm); u != "" { | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_URL", u); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_URL: %w", err) | ||
| } | ||
| } | ||
| if err := addVolumeMount(tmpl, llmCredsVolumeName, vertexCredsMountPath, true); err != nil { | ||
| return fmt.Errorf("mount Vertex credentials: %w", err) | ||
| case agenticv1alpha1.LLMProviderOpenAI: | ||
| if u := providerURL(llm); u != "" { | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_URL", u); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_URL: %w", err) | ||
| } | ||
| } | ||
| case agenticv1alpha1.LLMProviderAzureOpenAI: | ||
| cfg := llm.Spec.AzureOpenAI | ||
| if err := setEnvVar(tmpl, "AZURE_OPENAI_ENDPOINT", cfg.Endpoint); err != nil { | ||
| return fmt.Errorf("set AZURE_OPENAI_ENDPOINT: %w", err) | ||
| providerURLValue := cfg.Endpoint | ||
| if u := cfg.URL; u != "" { | ||
| providerURLValue = u | ||
| } | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_URL", providerURLValue); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_URL: %w", err) | ||
| } | ||
| if cfg.APIVersion != "" { | ||
| if err := setEnvVar(tmpl, "AZURE_OPENAI_API_VERSION", cfg.APIVersion); err != nil { | ||
| return fmt.Errorf("set AZURE_OPENAI_API_VERSION: %w", err) | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_API_VERSION", cfg.APIVersion); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_API_VERSION: %w", err) | ||
| } | ||
| } | ||
| case agenticv1alpha1.LLMProviderAWSBedrock: | ||
| cfg := llm.Spec.AWSBedrock | ||
| if err := setEnvVar(tmpl, "CLAUDE_CODE_USE_BEDROCK", "1"); err != nil { | ||
| return fmt.Errorf("set CLAUDE_CODE_USE_BEDROCK: %w", err) | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_REGION", cfg.Region); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_REGION: %w", err) | ||
| } | ||
| if err := setEnvVar(tmpl, "AWS_REGION", cfg.Region); err != nil { | ||
| return fmt.Errorf("set AWS_REGION: %w", err) | ||
| if u := providerURL(llm); u != "" { | ||
| if err := setEnvVar(tmpl, "LIGHTSPEED_PROVIDER_URL", u); err != nil { | ||
| return fmt.Errorf("set LIGHTSPEED_PROVIDER_URL: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use constant error labels for the new fmt.Errorf branches.
These added paths introduce new inline error strings ("set LIGHTSPEED_PROVIDER", "add credentials volume", etc.). The repo guideline for Go files requires defining the labels as constants and wrapping with fmt.Errorf("%s: %w", Err..., err) for consistent context.
As per coding guidelines, **/*.go: Define errors as constants using const ErrFoo = "…" and wrap them with fmt.Errorf("%s: %w", …) for context.
🤖 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/sandbox_templates.go` around lines 267 - 334, Replace
inline error message strings in the fmt.Errorf calls with package-level constant
labels and wrap the original error using fmt.Errorf("%s: %w", Err..., err); add
const declarations (e.g. ErrSetLightspeedProvider, ErrSetLightspeedModel,
ErrAddCredentialsEnvFrom, ErrAddCredentialsVolume, ErrMountCredentials,
ErrSetProviderURL, ErrSetModelProvider, ErrSetProviderProject,
ErrSetProviderRegion, ErrSetProviderAPIVersion) and update all occurrences in
the code that call setEnvVar, addEnvFromSecret, addSecretVolume, addVolumeMount
and the provider-specific branches to use fmt.Errorf("%s: %w", <const>, err)
instead of inline strings so the error labels are consistent with the repository
guideline.
There was a problem hiding this comment.
Skipping — pre-existing pattern across sandbox_templates.go. All existing fmt.Errorf calls use inline strings. Extracting error constants for every wrapper would be a separate refactor touching the entire file, not scoped to this PR.
…(OLS-3200) Fold envFrom check into assertCredentialMounts so all 6 provider tests verify both envFrom and volume mount unconditionally. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@onmete: 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. |
2 participants
Summary
patchLLMCredentialsto set only genericLIGHTSPEED_*env vars instead of SDK-specific varsLIGHTSPEED_PROVIDER,LIGHTSPEED_MODEL, plus optionalLIGHTSPEED_MODEL_PROVIDER,LIGHTSPEED_PROVIDER_URL,LIGHTSPEED_PROVIDER_PROJECT,LIGHTSPEED_PROVIDER_REGION,LIGHTSPEED_PROVIDER_API_VERSIONenvFrom+ volume at/var/run/secrets/llm-credentials/(previously Vertex-only at/var/secrets/google)ANTHROPIC_MODEL,CLAUDE_CODE_USE_VERTEX,GCP_PROJECT,GCP_REGION,GOOGLE_APPLICATION_CREDENTIALS,AZURE_OPENAI_ENDPOINT,OPENAI_BASE_URL,ANTHROPIC_BASE_URL, etc.urloverridesendpointforLIGHTSPEED_PROVIDER_URLCompanion PR: sandbox env var mapping in openshift/lightspeed-agentic-sandbox#62 (must merge together)
Jira: OLS-3200
Supersedes: OLS-3044, OLS-3051
Test plan
LIGHTSPEED_*vars and absence of SDK-specific varsMade with Cursor
Summary by CodeRabbit
Release Notes
Documentation
Refactor
Tests