fix(sandbox): verify skipTLSVerify/caFile behavior of kubeconfig under Bun (#1849)#1919
fix(sandbox): verify skipTLSVerify/caFile behavior of kubeconfig under Bun (#1849)#1919larryro wants to merge 2 commits into
Conversation
…r Bun (#1849) @kubernetes/client-node@1.4.0 routes all API calls through node-fetch v2 (gen/http/isomorphic-fetch.js), which calls https.request() with an https.Agent — NOT Bun's native fetch(). Bun's node:https honours rejectUnauthorized and ca on the Agent, so skipTLSVerify and caFile are real, effective TLS knobs, not dead code. The limitation noted in the AUTH NOTE is specific to client-cert auth (cert/key options), which Bun's TLS stack does not support. skipTLSVerify (→ rejectUnauthorized:false) and caFile (→ ca buffer) are distinct and work as designed. Changes: - k8s-client.test.ts: add three tests under "kubeconfig TLS knobs under Bun" verifying applySecurityAuthentication propagates the correct options to the https.Agent for skipTLSVerify, caFile, and the no-override case - k8s-client.ts: rewrite AUTH NOTE to accurately describe what is and is not honoured (client certs inert; skipTLSVerify/caFile effective via node:https; NODE_EXTRA_CA_CERTS as belt-and-suspenders for native fetch paths) - docs/kubernetes.md: add TLS contract section with a knob table and guidance on SANDBOX_K8S_CAFILE vs NODE_EXTRA_CA_CERTS
📝 WalkthroughWalkthroughThe PR resolves issue Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@services/sandbox/docs/kubernetes.md`:
- Line 92: The NODE_EXTRA_CA_CERTS row in the environment variables table
currently implies that apiserver TLS trust depends on this variable, but this
contradicts the new TLS contract section which explains that
`@kubernetes/client-node` uses https.Agent and honors the kubeconfig caFile.
Revise the NODE_EXTRA_CA_CERTS row description to clarify that this environment
variable is specifically for native fetch() code paths only, and explicitly note
that it complements but is not the primary mechanism for apiserver TLS trust
(which is handled by kubeconfig caFile for client-node). Also review and update
the related text in the 126-149 range to ensure consistent messaging about which
components use which trust mechanisms.
In `@services/sandbox/src/backend/kubernetes/k8s-client.test.ts`:
- Around line 159-204: Add a new test case after the existing three tests that
verifies the error condition when caFile references a missing or unreadable file
path. Create a test that calls makeKc with a caFile pointing to a nonexistent
path, then invoke applySecurityAuthentication on the RequestContext and assert
that the promise rejects with an appropriate error. This completes the test
coverage by adding the required error-path condition alongside the existing
happy-path and edge-case tests.
- Around line 172-190: The test uses a hardcoded file path
`/tmp/k8s-client-test-ca.pem` for the caFile variable, which can cause race
conditions and flakiness when tests run concurrently across multiple workers or
retries due to file path collisions. Replace the hardcoded caPath string with a
dynamically generated unique temporary file path using Node.js built-in
utilities such as tmpNameSync or mkdtempSync, or an equivalent method that
ensures each test invocation gets a unique file path. Update the caPath variable
assignment in the test to use this unique temporary path generation approach.
- Around line 150-157: The getAgentOpts function currently uses an unsafe type
assertion chain (as unknown as) to access the options property, which violates
coding guidelines. Replace this unsafe cast with runtime property checks using
Reflect.get() to safely extract only the rejectUnauthorized and ca properties
that the tests depend on, and condense the multi-line documentation comment to a
single line per guidelines.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39b01c5b-9ece-49f7-bd0e-95b5e719d624
📒 Files selected for processing (3)
services/sandbox/docs/kubernetes.mdservices/sandbox/src/backend/kubernetes/k8s-client.test.tsservices/sandbox/src/backend/kubernetes/k8s-client.ts
| | `SANDBOX_SPAWNER_IMAGE` | yes | The spawner's **own** image ref, used for the `stage`/`harvest` containers. Must match the deployed spawner version. | | ||
| | `SANDBOX_RUNTIME_IMAGE` | yes | The `runner` image (`tale-sandbox-runtime:<tag>`). | | ||
| | `SANDBOX_K8S_NAMESPACE` | yes | Namespace the per-exec Pods/Secrets are created in (default `tale-sandbox`). | | ||
| | `NODE_EXTRA_CA_CERTS` | in-cluster | Point at the SA `ca.crt` (`/var/run/secrets/kubernetes.io/serviceaccount/ca.crt`). **Bun's fetch ignores the kubeconfig CA**, so without this the apiserver TLS isn't trusted. | |
There was a problem hiding this comment.
NODE_EXTRA_CA_CERTS note currently contradicts the new TLS contract text.
The env table says apiserver TLS is untrusted without NODE_EXTRA_CA_CERTS, but the new section explains @kubernetes/client-node uses https.Agent and honors kubeconfig caFile. Tighten the row wording so it describes NODE_EXTRA_CA_CERTS as complementary for native fetch() paths, not the primary apiserver trust source for client-node calls.
Suggested doc wording update
-| `NODE_EXTRA_CA_CERTS` | in-cluster | Point at the SA `ca.crt` (`/var/run/secrets/kubernetes.io/serviceaccount/ca.crt`). **Bun's fetch ignores the kubeconfig CA**, so without this the apiserver TLS isn't trusted. |
+| `NODE_EXTRA_CA_CERTS` | in-cluster | Point at the SA `ca.crt` (`/var/run/secrets/kubernetes.io/serviceaccount/ca.crt`). Recommended as defense-in-depth for native `fetch()` paths under Bun; client-node apiserver trust still comes from kubeconfig CA/agent settings. |Also applies to: 126-149
🤖 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 `@services/sandbox/docs/kubernetes.md` at line 92, The NODE_EXTRA_CA_CERTS row
in the environment variables table currently implies that apiserver TLS trust
depends on this variable, but this contradicts the new TLS contract section
which explains that `@kubernetes/client-node` uses https.Agent and honors the
kubeconfig caFile. Revise the NODE_EXTRA_CA_CERTS row description to clarify
that this environment variable is specifically for native fetch() code paths
only, and explicitly note that it complements but is not the primary mechanism
for apiserver TLS trust (which is handled by kubeconfig caFile for client-node).
Also review and update the related text in the 126-149 range to ensure
consistent messaging about which components use which trust mechanisms.
| function getAgentOpts(ctx: RequestContext): AgentOptions | undefined { | ||
| const agent = ctx.getAgent(); | ||
| if (!agent) return undefined; | ||
| // https.Agent stores its constructor options in agent.options; the property | ||
| // is present at runtime but narrower than the union type suggests. | ||
| // oxlint-disable-next-line typescript-eslint/no-unsafe-type-assertion | ||
| return (agent as unknown as { options: AgentOptions }).options; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the file mentioned in the review
find . -name "k8s-client.test.ts" -type fRepository: tale-project/tale
Length of output: 122
🏁 Script executed:
# Also search for the main implementation file (non-test version)
find . -name "k8s-client.ts" -type f | grep -v testRepository: tale-project/tale
Length of output: 117
🏁 Script executed:
# Search for AgentOptions definition
rg "type AgentOptions|interface AgentOptions" --type ts --type tsxRepository: tale-project/tale
Length of output: 89
🏁 Script executed:
# Search for RequestContext definition
rg "class RequestContext|interface RequestContext|type RequestContext" --type ts --type tsx -A 5Repository: tale-project/tale
Length of output: 89
🏁 Script executed:
# Read the test file to see the actual code
cat -n ./services/sandbox/src/backend/kubernetes/k8s-client.test.ts | sed -n '145,165p'Repository: tale-project/tale
Length of output: 1187
🏁 Script executed:
# Read the implementation file to see AgentOptions usage
cat -n ./services/sandbox/src/backend/kubernetes/k8s-client.ts | head -100Repository: tale-project/tale
Length of output: 4864
🏁 Script executed:
# Search for AgentOptions definition
rg "type AgentOptions|interface AgentOptions" --type tsRepository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Search for RequestContext definition more broadly
rg "RequestContext" ./services/sandbox/src/backend/kubernetes/ -A 3 -B 1Repository: tale-project/tale
Length of output: 3587
🏁 Script executed:
# Search for AgentOptions definition
rg "AgentOptions|interface.*Agent|type.*Agent" ./services/sandbox/src/backend/kubernetes/ --type ts -B 2 -A 5Repository: tale-project/tale
Length of output: 3374
🏁 Script executed:
# Check how getAgentOpts is being used in the tests
cat -n ./services/sandbox/src/backend/kubernetes/k8s-client.test.ts | sed -n '145,220p'Repository: tale-project/tale
Length of output: 3043
🏁 Script executed:
# Check imports to understand AgentOptions source
head -40 ./services/sandbox/src/backend/kubernetes/k8s-client.test.tsRepository: tale-project/tale
Length of output: 1456
🏁 Script executed:
# Search across the repo for AgentOptions definition
rg "type AgentOptions|interface AgentOptions" --type tsRepository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Check if getAgentOpts appears anywhere else in the codebase
rg "getAgentOpts" --type tsRepository: tale-project/tale
Length of output: 517
🏁 Script executed:
# Look for similar patterns with unsafe assertions in the main implementation
rg "as unknown as" ./services/sandbox/src/backend/kubernetes/ --type tsRepository: tale-project/tale
Length of output: 479
🏁 Script executed:
# Check if there are any other unsafe type assertions in the file
rg "no-unsafe-type-assertion|no-explicit-any" ./services/sandbox/src/backend/kubernetes/k8s-client.test.ts -A 2 -B 2Repository: tale-project/tale
Length of output: 361
🏁 Script executed:
# Verify the context of where getAgentOpts is defined - is it inside a test describe block?
cat -n ./services/sandbox/src/backend/kubernetes/k8s-client.test.ts | sed -n '130,160p'Repository: tale-project/tale
Length of output: 1703
🏁 Script executed:
# Check all test assertions using getAgentOpts to verify proposed Pick type
cat -n ./services/sandbox/src/backend/kubernetes/k8s-client.test.ts | sed -n '159,203p'Repository: tale-project/tale
Length of output: 2260
Replace the as unknown as cast chain with runtime property checks using Reflect.get().
This test helper currently violates the coding guideline against unsafe type assertions. The function accesses the options property on https.Agent, which exists at runtime but is narrower than the union type. The current documentation spans multiple lines where the exception comment should be one line per guidelines.
Replace the unsafe cast with runtime narrowing that safely extracts only the rejectUnauthorized and ca properties that the tests actually depend on:
Proposed rewrite without `as`/`unknown`
-function getAgentOpts(ctx: RequestContext): AgentOptions | undefined {
+function getAgentOpts(
+ ctx: RequestContext,
+): Pick<AgentOptions, 'rejectUnauthorized' | 'ca'> | undefined {
const agent = ctx.getAgent();
if (!agent) return undefined;
- // https.Agent stores its constructor options in agent.options; the property
- // is present at runtime but narrower than the union type suggests.
- // oxlint-disable-next-line typescript-eslint/no-unsafe-type-assertion
- return (agent as unknown as { options: AgentOptions }).options;
+ const options = Reflect.get(agent, 'options');
+ if (!options || typeof options !== 'object') return undefined;
+
+ const rejectUnauthorized = Reflect.get(options, 'rejectUnauthorized');
+ const ca = Reflect.get(options, 'ca');
+ return {
+ ...(typeof rejectUnauthorized === 'boolean' ? { rejectUnauthorized } : {}),
+ ...(ca !== undefined ? { ca } : {}),
+ };
}🤖 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 `@services/sandbox/src/backend/kubernetes/k8s-client.test.ts` around lines 150
- 157, The getAgentOpts function currently uses an unsafe type assertion chain
(as unknown as) to access the options property, which violates coding
guidelines. Replace this unsafe cast with runtime property checks using
Reflect.get() to safely extract only the rejectUnauthorized and ca properties
that the tests depend on, and condense the multi-line documentation comment to a
single line per guidelines.
Source: Coding guidelines
| test('skipTLSVerify:true → rejectUnauthorized:false on the per-request https.Agent', async () => { | ||
| const kc = makeKc({ skipTLSVerify: true }); | ||
| const ctx = new RequestContext( | ||
| 'https://k8s.local:6443/api/v1/pods', | ||
| HttpMethod.GET, | ||
| ); | ||
| await kc.applySecurityAuthentication(ctx); | ||
| // applySecurityAuthentication calls createAgent(cluster, { rejectUnauthorized: false, … }) | ||
| // and sets the resulting https.Agent on the context. The agent stores | ||
| // constructor options in agent.options (Node.js / Bun node:https parity). | ||
| expect(getAgentOpts(ctx)?.rejectUnauthorized).toBe(false); | ||
| }); | ||
|
|
||
| test('caFile → ca buffer present on the per-request https.Agent', async () => { | ||
| const caPath = '/tmp/k8s-client-test-ca.pem'; | ||
| // Content is a placeholder; bufferFromFileOrString reads the bytes as-is. | ||
| writeFileSync(caPath, 'placeholder-pem-content'); | ||
| try { | ||
| const kc = makeKc({ caFile: caPath }); | ||
| const ctx = new RequestContext( | ||
| 'https://k8s.local:6443/api/v1/pods', | ||
| HttpMethod.GET, | ||
| ); | ||
| await kc.applySecurityAuthentication(ctx); | ||
| // applyHTTPSOptions calls fs.readFileSync(caFile) and copies the result | ||
| // to agentOptions.ca before constructing the https.Agent. | ||
| const agentOpts = getAgentOpts(ctx); | ||
| expect(agentOpts?.ca).toBeDefined(); | ||
| expect(Buffer.isBuffer(agentOpts?.ca)).toBe(true); | ||
| } finally { | ||
| unlinkSync(caPath); | ||
| } | ||
| }); | ||
|
|
||
| test('neither skipTLSVerify nor caFile → default TLS (no overrides on the agent)', async () => { | ||
| const kc = makeKc({}); | ||
| const ctx = new RequestContext( | ||
| 'https://k8s.local:6443/api/v1/pods', | ||
| HttpMethod.GET, | ||
| ); | ||
| await kc.applySecurityAuthentication(ctx); | ||
| const agentOpts = getAgentOpts(ctx); | ||
| expect(agentOpts?.rejectUnauthorized).toBeUndefined(); | ||
| expect(agentOpts?.ca).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add one explicit error-path TLS test to complete the contract.
The new block has happy + edge coverage, but no error condition. Add a failing caFile case (missing/unreadable path) and assert applySecurityAuthentication rejects.
As per coding guidelines: “Every feature and fix must carry its test: happy path, one edge case, one error condition minimum.”
Minimal error-case addition
describe('kubeconfig TLS knobs under Bun', () => {
@@
test('neither skipTLSVerify nor caFile → default TLS (no overrides on the agent)', async () => {
@@
expect(agentOpts?.ca).toBeUndefined();
});
+
+ test('missing caFile → applySecurityAuthentication rejects', async () => {
+ const kc = makeKc({ caFile: '/tmp/does-not-exist.pem' });
+ const ctx = new RequestContext(
+ 'https://k8s.local:6443/api/v1/pods',
+ HttpMethod.GET,
+ );
+ await expect(kc.applySecurityAuthentication(ctx)).rejects.toThrow();
+ });
});📝 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.
| test('skipTLSVerify:true → rejectUnauthorized:false on the per-request https.Agent', async () => { | |
| const kc = makeKc({ skipTLSVerify: true }); | |
| const ctx = new RequestContext( | |
| 'https://k8s.local:6443/api/v1/pods', | |
| HttpMethod.GET, | |
| ); | |
| await kc.applySecurityAuthentication(ctx); | |
| // applySecurityAuthentication calls createAgent(cluster, { rejectUnauthorized: false, … }) | |
| // and sets the resulting https.Agent on the context. The agent stores | |
| // constructor options in agent.options (Node.js / Bun node:https parity). | |
| expect(getAgentOpts(ctx)?.rejectUnauthorized).toBe(false); | |
| }); | |
| test('caFile → ca buffer present on the per-request https.Agent', async () => { | |
| const caPath = '/tmp/k8s-client-test-ca.pem'; | |
| // Content is a placeholder; bufferFromFileOrString reads the bytes as-is. | |
| writeFileSync(caPath, 'placeholder-pem-content'); | |
| try { | |
| const kc = makeKc({ caFile: caPath }); | |
| const ctx = new RequestContext( | |
| 'https://k8s.local:6443/api/v1/pods', | |
| HttpMethod.GET, | |
| ); | |
| await kc.applySecurityAuthentication(ctx); | |
| // applyHTTPSOptions calls fs.readFileSync(caFile) and copies the result | |
| // to agentOptions.ca before constructing the https.Agent. | |
| const agentOpts = getAgentOpts(ctx); | |
| expect(agentOpts?.ca).toBeDefined(); | |
| expect(Buffer.isBuffer(agentOpts?.ca)).toBe(true); | |
| } finally { | |
| unlinkSync(caPath); | |
| } | |
| }); | |
| test('neither skipTLSVerify nor caFile → default TLS (no overrides on the agent)', async () => { | |
| const kc = makeKc({}); | |
| const ctx = new RequestContext( | |
| 'https://k8s.local:6443/api/v1/pods', | |
| HttpMethod.GET, | |
| ); | |
| await kc.applySecurityAuthentication(ctx); | |
| const agentOpts = getAgentOpts(ctx); | |
| expect(agentOpts?.rejectUnauthorized).toBeUndefined(); | |
| expect(agentOpts?.ca).toBeUndefined(); | |
| }); | |
| }); | |
| test('skipTLSVerify:true → rejectUnauthorized:false on the per-request https.Agent', async () => { | |
| const kc = makeKc({ skipTLSVerify: true }); | |
| const ctx = new RequestContext( | |
| 'https://k8s.local:6443/api/v1/pods', | |
| HttpMethod.GET, | |
| ); | |
| await kc.applySecurityAuthentication(ctx); | |
| // applySecurityAuthentication calls createAgent(cluster, { rejectUnauthorized: false, … }) | |
| // and sets the resulting https.Agent on the context. The agent stores | |
| // constructor options in agent.options (Node.js / Bun node:https parity). | |
| expect(getAgentOpts(ctx)?.rejectUnauthorized).toBe(false); | |
| }); | |
| test('caFile → ca buffer present on the per-request https.Agent', async () => { | |
| const caPath = '/tmp/k8s-client-test-ca.pem'; | |
| // Content is a placeholder; bufferFromFileOrString reads the bytes as-is. | |
| writeFileSync(caPath, 'placeholder-pem-content'); | |
| try { | |
| const kc = makeKc({ caFile: caPath }); | |
| const ctx = new RequestContext( | |
| 'https://k8s.local:6443/api/v1/pods', | |
| HttpMethod.GET, | |
| ); | |
| await kc.applySecurityAuthentication(ctx); | |
| // applyHTTPSOptions calls fs.readFileSync(caFile) and copies the result | |
| // to agentOptions.ca before constructing the https.Agent. | |
| const agentOpts = getAgentOpts(ctx); | |
| expect(agentOpts?.ca).toBeDefined(); | |
| expect(Buffer.isBuffer(agentOpts?.ca)).toBe(true); | |
| } finally { | |
| unlinkSync(caPath); | |
| } | |
| }); | |
| test('neither skipTLSVerify nor caFile → default TLS (no overrides on the agent)', async () => { | |
| const kc = makeKc({}); | |
| const ctx = new RequestContext( | |
| 'https://k8s.local:6443/api/v1/pods', | |
| HttpMethod.GET, | |
| ); | |
| await kc.applySecurityAuthentication(ctx); | |
| const agentOpts = getAgentOpts(ctx); | |
| expect(agentOpts?.rejectUnauthorized).toBeUndefined(); | |
| expect(agentOpts?.ca).toBeUndefined(); | |
| }); | |
| test('missing caFile → applySecurityAuthentication rejects', async () => { | |
| const kc = makeKc({ caFile: '/tmp/does-not-exist.pem' }); | |
| const ctx = new RequestContext( | |
| 'https://k8s.local:6443/api/v1/pods', | |
| HttpMethod.GET, | |
| ); | |
| await expect(kc.applySecurityAuthentication(ctx)).rejects.toThrow(); | |
| }); | |
| }); |
🤖 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 `@services/sandbox/src/backend/kubernetes/k8s-client.test.ts` around lines 159
- 204, Add a new test case after the existing three tests that verifies the
error condition when caFile references a missing or unreadable file path. Create
a test that calls makeKc with a caFile pointing to a nonexistent path, then
invoke applySecurityAuthentication on the RequestContext and assert that the
promise rejects with an appropriate error. This completes the test coverage by
adding the required error-path condition alongside the existing happy-path and
edge-case tests.
Source: Coding guidelines
| test('caFile → ca buffer present on the per-request https.Agent', async () => { | ||
| const caPath = '/tmp/k8s-client-test-ca.pem'; | ||
| // Content is a placeholder; bufferFromFileOrString reads the bytes as-is. | ||
| writeFileSync(caPath, 'placeholder-pem-content'); | ||
| try { | ||
| const kc = makeKc({ caFile: caPath }); | ||
| const ctx = new RequestContext( | ||
| 'https://k8s.local:6443/api/v1/pods', | ||
| HttpMethod.GET, | ||
| ); | ||
| await kc.applySecurityAuthentication(ctx); | ||
| // applyHTTPSOptions calls fs.readFileSync(caFile) and copies the result | ||
| // to agentOptions.ca before constructing the https.Agent. | ||
| const agentOpts = getAgentOpts(ctx); | ||
| expect(agentOpts?.ca).toBeDefined(); | ||
| expect(Buffer.isBuffer(agentOpts?.ca)).toBe(true); | ||
| } finally { | ||
| unlinkSync(caPath); | ||
| } |
There was a problem hiding this comment.
Use a unique temp file for the caFile test.
The fixed /tmp/k8s-client-test-ca.pem path can collide across concurrent workers/retries and make this test flaky.
Suggested temp-path hardening
-import { writeFileSync, unlinkSync } from 'node:fs';
+import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
@@
- test('caFile → ca buffer present on the per-request https.Agent', async () => {
- const caPath = '/tmp/k8s-client-test-ca.pem';
+ test('caFile → ca buffer present on the per-request https.Agent', async () => {
+ const dir = mkdtempSync(join(tmpdir(), 'k8s-client-ca-'));
+ const caPath = join(dir, 'ca.pem');
@@
- } finally {
- unlinkSync(caPath);
- }
+ } finally {
+ rmSync(dir, { recursive: true, force: true });
+ }
});🤖 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 `@services/sandbox/src/backend/kubernetes/k8s-client.test.ts` around lines 172
- 190, The test uses a hardcoded file path `/tmp/k8s-client-test-ca.pem` for the
caFile variable, which can cause race conditions and flakiness when tests run
concurrently across multiple workers or retries due to file path collisions.
Replace the hardcoded caPath string with a dynamically generated unique
temporary file path using Node.js built-in utilities such as tmpNameSync or
mkdtempSync, or an equivalent method that ensures each test invocation gets a
unique file path. Update the caPath variable assignment in the test to use this
unique temporary path generation approach.
Code Review: fix(sandbox): verify skipTLSVerify/caFile behavior of kubeconfig under Bun (#1849)CI: All required checks passed (Unit ✅, UI ✅, Browser ✅, Migrations ✅, Smoke test ✅, Build sandbox ✅, Opengrep ✅, CodeRabbit ✅, Analyze JS/TS ✅). Fork-PR skips are expected. Tests run locally: Verdict: NOT READY — 1 blocking issueBlocking1.
|
- Fix internal contradiction in kubernetes.md: NODE_EXTRA_CA_CERTS row
previously stated "Bun's fetch ignores the kubeconfig CA" which directly
contradicted the new TLS contract section. Updated to explain that k8s API
calls are trusted via caFile -> Agent, while native fetch() calls (e.g.
harvest presigned-URL uploads) bypass the Agent and require this variable.
- Add console.warn when SANDBOX_K8S_CAFILE is unset and skipTLSVerify:true
fallback is used, so operators get a visible signal in the live log.
- Verify caFile buffer contents in test (not just that ca is a Buffer).
- Use rmSync({ force: true }) instead of unlinkSync to avoid ENOENT on
concurrent test runs.
- Add expect(agentOpts).toBeDefined() guard in the default-TLS test.
- Add caData test: covers the base64-inline CA path as a distinct code branch.
Desk Reviewer Report — PR #1919Branch: CI StateAll applicable CI checks pass. The "skipping" jobs (Docs container test, Smoke test (fork PR), Storybook, Validate images (fork PR), Web container test) are fork-PR-specific jobs skipped by design — not failures.
Test Run (local)After installing dependencies: The three new TLS-knob tests pass. FindingsBLOCKING — must be fixed before merge1. The expect(agentOpts?.ca).toEqual(Buffer.from('placeholder-pem-content'));The expect(agentOpts?.ca).toBeDefined();
expect(Buffer.isBuffer(agentOpts?.ca)).toBe(true);A bug where expect(agentOpts?.ca).toEqual(Buffer.from('placeholder-pem-content'));This mirrors the 2. The path import { randomUUID } from 'node:crypto';
const caPath = `/tmp/k8s-client-test-ca-${randomUUID()}.pem`;ADVISORY — not blockingA. If B. The comment block explains library internals already covered verbatim in AssessmentThe approach is sound: testing Both blocking items are one-line fixes to the test file. VERDICT: NOT READY — CHANGES REQUIRED
|
|
Three open PRs address the same question, and two reach opposite conclusions about whether Bun honors kubeconfig TLS knobs:
This is security-critical: merging the wrong conclusion could leave TLS verification silently disabled in production, or remove code that was actually doing the verification. This cannot be resolved by review alone — it needs an empirical test under the real Bun version (issue a request with a kubeconfig carrying |
1 participant
Summary
Resolves #1849.
@kubernetes/client-node@1.4.0routes every API call through node-fetch v2 (gen/http/isomorphic-fetch.js: import fetch from "node-fetch"), which callshttps.request()with anhttps.Agent— not Bun's nativefetch(). Bun'snode:httpsimplementation honoursrejectUnauthorizedandcaon the Agent, soskipTLSVerifyandcaFilein the kubeconfig are effective and carry real security semantics — they are not dead code.The limitation documented in the AUTH NOTE is specific to client-cert auth (
cert/keyoptions on the Agent), which Bun's TLS stack does not support.skipTLSVerifyandcaFileare distinct knobs that work as intended.Changes
k8s-client.test.ts— three new tests underkubeconfig TLS knobs under Bunthat callapplySecurityAuthenticationagainst a testRequestContextand assert thehttps.Agentoptions (rejectUnauthorized,ca) are set correctly forskipTLSVerify:true,caFile, and the no-override case.k8s-client.ts— AUTH NOTE rewritten to be precise: client certs are inert;skipTLSVerify/caFileare effective vianode:https;NODE_EXTRA_CA_CERTSserves as belt-and-suspenders for nativefetch()paths (harvest presigned-URL uploads).docs/kubernetes.md— new TLS contract under Bun section with a table of kubeconfig knobs, their Bun-effective status, and guidance onSANDBOX_K8S_CAFILEvsNODE_EXTRA_CA_CERTS.Test plan
bun run --filter '@tale/sandbox' test— 345 pass, 0 fail (3 new tests added)bun run --filter '@tale/sandbox' lint— 0 warnings, 0 errorsbun run --filter '@tale/sandbox' typecheck— cleanbun run --filter '@tale/sandbox' format:check— cleanbun run lint:sast -- --include services/sandbox/— 0 findingsSummary by CodeRabbit
Documentation
Tests