fix(driver-k8s): add label selector to sandbox watch stream and list (#2211)#4
fix(driver-k8s): add label selector to sandbox watch stream and list (#2211)#4rhuss wants to merge 7 commits into
Conversation
Parent brainstorm (01) defines the layered measurement approach for evaluating Agent Sandbox warm pooling on OpenShift. Child documents cover cluster setup (02), measurements (03), and results synthesis (04). Assisted-By: 🤖 Claude Code
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
…VIDIA#2211) The Kubernetes driver's watch_sandboxes() crashed with a file descriptor leak when non-gateway Sandbox resources (e.g. from SandboxWarmPool) existed in the namespace. The unfiltered watch received events for all Sandbox objects, failed to extract IDs from warm pool objects, and propagated errors that triggered a 2-second reconnect loop leaking HTTP/2 connections until FD exhaustion. Two fixes: - Add openshell.ai/managed-by=openshell label selector to the watcher config and list params so only gateway-managed objects are received. - Defensively skip unrecognized objects with a debug log instead of sending errors to the channel. Signed-off-by: Roland Huß <roland@jolokia.org> Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
📝 WalkthroughWalkthroughThe PR introduces Kubernetes supervisor sidecar topology with authenticated control-channel coordination, sidecar network enforcement, topology-aware process sandboxing, image/configuration changes, provider secret handling, TUI status reporting, warm-pool planning documents, and operational documentation. ChangesKubernetes sidecar runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant KubernetesDriver
participant NetworkSidecar
participant ProcessSupervisor
participant SandboxProcess
KubernetesDriver->>NetworkSidecar: render sidecar pod and network-init configuration
NetworkSidecar->>ProcessSupervisor: authenticate control connection
ProcessSupervisor->>SandboxProcess: provide bootstrap policy and environment
NetworkSidecar->>ProcessSupervisor: publish policy and environment updates
ProcessSupervisor->>SandboxProcess: stream runtime updates
SandboxProcess->>ProcessSupervisor: report entrypoint lifecycle
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The debug vs warn question for the defensive skip is resolved by the implementation which uses debug! level logging. Signed-off-by: Roland Huß <roland@jolokia.org> Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
Applied fixes from bot review comments: - Comment #3563290363: log error context in Applied branch skip - Comment #3563290364: log error context in Deleted branch skip - Comment #3563290365: log error context in Restarted branch skip Refactored from if-let-else to let-else pattern for flatter code structure and clearer early-return semantics. Signed-off-by: Roland Huß <roland@jolokia.org> Assisted-By: 🤖 Claude Code Signed-off-by: Roland Huß <rhuss@redhat.com>
|
@copilot resolve the merge conflicts in this pull request |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| - Is KEP-753 (native sidecars) available on the target OpenShift version? (from #01, #02) | ||
| - How does pool exhaustion behave (cold fallback vs. Pending)? (from #03) | ||
| - Should findings be posted to upstream agent-sandbox repo? (from #04) | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
🟡 Unresolved git merge conflict marker left in a committed brainstorm document
A git merge conflict marker (<<<<<<< HEAD at brainstorm/00-overview.md:32) was committed into the file, so the document contains raw conflict syntax instead of resolved content.
Impact: The brainstorm overview document is malformed and renders the conflict marker literally for anyone reading it.
Merge conflict marker details
Line 32 of brainstorm/00-overview.md contains <<<<<<< HEAD. The corresponding ======= and >>>>>>> markers that normally delimit the two sides of a conflict are absent, suggesting the file was partially resolved but the opening marker was not removed. The lines after it (33-37) appear to be the intended content from one side of the merge.
| <<<<<<< HEAD |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/openshell-tui/src/lib.rs (1)
2414-2434: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
refresh_draft_chunkssilently swallows fetch errors/timeouts, unlike its sibling refresh functions.Every other
refresh_*function touched in this PR (refresh_providers,refresh_global_settings,refresh_sandboxes,refresh_sandbox_policy) now setsapp.status_textonOk(Err(_))/timeout. This function drops those branches entirely (the prior debug logging was removed with no replacement), so a user won't see any indication that the periodic draft-chunk refresh (called every tick on the Sandbox screen) is failing.🩹 Proposed fix to surface the failure
- if let Ok(Ok(resp)) = - tokio::time::timeout(Duration::from_secs(5), app.client.get_draft_policy(req)).await - { - let inner = resp.into_inner(); - app.draft_chunks = inner.chunks; - app.draft_version = inner.draft_version; - if app.draft_selected >= app.draft_chunks.len() && !app.draft_chunks.is_empty() { - app.draft_selected = app.draft_chunks.len() - 1; - } - } + match tokio::time::timeout(Duration::from_secs(5), app.client.get_draft_policy(req)).await { + Ok(Ok(resp)) => { + let inner = resp.into_inner(); + app.draft_chunks = inner.chunks; + app.draft_version = inner.draft_version; + if app.draft_selected >= app.draft_chunks.len() && !app.draft_chunks.is_empty() { + app.draft_selected = app.draft_chunks.len() - 1; + } + } + Ok(Err(e)) => { + app.status_text = format!("failed to fetch draft chunks: {}", e.message()); + } + Err(_) => { + app.status_text = "draft chunks request timed out".to_string(); + } + }🤖 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 `@crates/openshell-tui/src/lib.rs` around lines 2414 - 2434, Update refresh_draft_chunks to handle both RPC errors and timeout errors instead of silently ignoring them. Preserve the existing successful-response updates, and set app.status_text with an appropriate failure message in each unsuccessful branch, matching the behavior of the sibling refresh functions.
🧹 Nitpick comments (5)
crates/openshell-supervisor-network/src/proxy.rs (1)
1580-1614: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
network_binary_identity_required()instead of reading the env var per connection.
evaluate_opa_tcp(Linux and non-Linux) now callscrate::opa::network_binary_identity_required(), which doesstd::env::var(...), on every proxied TCP connection's decision path. Since this value is effectively static configuration for the process lifetime, consider caching it once (e.g.OnceLock<bool>/LazyLock<bool>) instead of re-reading the environment on every connection.♻️ Sketch
-if !crate::opa::network_binary_identity_required() { +static NETWORK_BINARY_IDENTITY_REQUIRED: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); +if !*NETWORK_BINARY_IDENTITY_REQUIRED + .get_or_init(crate::opa::network_binary_identity_required) +{Also applies to: 1730-1743
🤖 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 `@crates/openshell-supervisor-network/src/proxy.rs` around lines 1580 - 1614, Cache the result of crate::opa::network_binary_identity_required() for the process lifetime using the existing OPA configuration area and a OnceLock<bool> or LazyLock<bool>. Update both Linux and non-Linux evaluate_opa_tcp decision paths to reuse the cached value instead of reading the environment per connection, while preserving the current endpoint-only behavior when the value is false.crates/openshell-supervisor-network/src/opa.rs (1)
80-100: 🚀 Performance & Scalability | 🔵 TrivialHeads up: every
OpaEngineconstruction (including everyreload/reload_from_proto_with_pid) now emits an info log + OCSF event.
clone_engine_for_tunnelavoids this (it clones the built engine directly), so per-tunnel L7 evaluation isn't affected. But if the sandbox's policy poll loop reloads on a fixed interval regardless of whether the policy actually changed, this adds a steady stream of "Configured OPA runtime binary identity mode" log lines/OCSF events. Worth confirming the poll loop only reloads on detected change, or that the interval is coarse enough that this isn't operationally noisy.🤖 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 `@crates/openshell-supervisor-network/src/opa.rs` around lines 80 - 100, Verify the sandbox policy poll loop only invokes OpaEngine construction through reload or reload_from_proto_with_pid when the policy has actually changed; avoid periodic unconditional reloads that repeatedly trigger emit_binary_identity_mode. Preserve clone_engine_for_tunnel behavior, and if unconditional polling is required, suppress or appropriately coalesce the configuration log and OCSF event for unchanged policy state.crates/openshell-supervisor-process/src/netns/mod.rs (2)
556-616: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConntrack accept rule is unconditionally required in the iptables-legacy fallback, unlike its nft counterpart.
In
generate_sidecar_bypass_commandsthe analogousct state established,related acceptrule is markedrequired: falsebecause it needsnf_conntrack. Here, the equivalent-m conntrack --ctstate ESTABLISHED,RELATEDrule is bundled into the required, sequential command list — if the conntrack extension is unavailable, the fallback of last resort aborts entirely instead of degrading gracefully like the primary nft path does.🤖 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 `@crates/openshell-supervisor-process/src/netns/mod.rs` around lines 556 - 616, Update install_sidecar_iptables_legacy_family_rules so the conntrack ESTABLISHED,RELATED accept rule is optional, matching generate_sidecar_bypass_commands. Execute it separately and continue when the conntrack extension is unavailable, while preserving failure handling for the remaining required iptables rules.
690-720: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
run_nft_commands_current_namespaceandrun_nft_commands_netnsare near-duplicate implementations.Both iterate
NftCommands, log, execute, and apply the same required/non-required error handling; they differ only in prefixing withnsenter --net=. Extracting a shared helper (parameterized by an optional command prefix) would remove the duplication risk of the required/non-required logic silently drifting between the two copies.Also applies to: 763-809
🤖 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 `@crates/openshell-supervisor-process/src/netns/mod.rs` around lines 690 - 720, Extract the shared iteration, logging, execution, and required/non-required error handling from run_nft_commands_current_namespace and run_nft_commands_netns into one helper parameterized by an optional command prefix. Have each existing function supply its namespace-specific command invocation while preserving the current nsenter --net= behavior and error semantics.crates/openshell-supervisor-process/src/netns/nft_ruleset.rs (1)
36-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSignificant duplication between
generate_bypass_commandsandgenerate_sidecar_bypass_commands.Both functions repeat the same four reject-rule blocks (ipv4/ipv6 × tcp/udp) and the same two log-rule blocks almost verbatim, differing only in the table name and accept-rule preamble. Extracting small helpers (e.g.
reject_rule(table, nfproto, l4proto, icmp_type),log_rule(table, proto_match, prefix)) would cut this file roughly in half and reduce the risk of the two rulesets silently diverging on a future edit.🤖 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 `@crates/openshell-supervisor-process/src/netns/nft_ruleset.rs` around lines 36 - 367, Refactor the duplicated rule construction in generate_bypass_commands and generate_sidecar_bypass_commands into shared helpers for protocol logging and IPv4/IPv6 TCP/UDP rejection, parameterized by table and the relevant protocol or ICMP type. Replace both functions’ repeated blocks with these helpers while preserving command ordering, nft_cmd failure flags, and each function’s existing accept-rule preamble.
🤖 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 `@brainstorm/00-overview.md`:
- Around line 32-33: Remove the leftover `<<<<<<< HEAD` merge-conflict marker
from the overview entry, while retaining the resolved `debug!` decision text.
In `@crates/openshell-supervisor-network/data/sandbox-policy.rego`:
- Around line 22-24: Update the binary_identity_required rule to explicitly
compare the resolved object.get value to true, preserving the default-true
behavior when require_binary_identity is absent and allowing an explicit false
value to disable enforcement.
In `@telemetry/README.md`:
- Line 37: Update the provider name in the “Providers (last 2 weeks)” list from
“github” to the official capitalization “GitHub”; leave the other provider names
unchanged.
---
Outside diff comments:
In `@crates/openshell-tui/src/lib.rs`:
- Around line 2414-2434: Update refresh_draft_chunks to handle both RPC errors
and timeout errors instead of silently ignoring them. Preserve the existing
successful-response updates, and set app.status_text with an appropriate failure
message in each unsuccessful branch, matching the behavior of the sibling
refresh functions.
---
Nitpick comments:
In `@crates/openshell-supervisor-network/src/opa.rs`:
- Around line 80-100: Verify the sandbox policy poll loop only invokes OpaEngine
construction through reload or reload_from_proto_with_pid when the policy has
actually changed; avoid periodic unconditional reloads that repeatedly trigger
emit_binary_identity_mode. Preserve clone_engine_for_tunnel behavior, and if
unconditional polling is required, suppress or appropriately coalesce the
configuration log and OCSF event for unchanged policy state.
In `@crates/openshell-supervisor-network/src/proxy.rs`:
- Around line 1580-1614: Cache the result of
crate::opa::network_binary_identity_required() for the process lifetime using
the existing OPA configuration area and a OnceLock<bool> or LazyLock<bool>.
Update both Linux and non-Linux evaluate_opa_tcp decision paths to reuse the
cached value instead of reading the environment per connection, while preserving
the current endpoint-only behavior when the value is false.
In `@crates/openshell-supervisor-process/src/netns/mod.rs`:
- Around line 556-616: Update install_sidecar_iptables_legacy_family_rules so
the conntrack ESTABLISHED,RELATED accept rule is optional, matching
generate_sidecar_bypass_commands. Execute it separately and continue when the
conntrack extension is unavailable, while preserving failure handling for the
remaining required iptables rules.
- Around line 690-720: Extract the shared iteration, logging, execution, and
required/non-required error handling from run_nft_commands_current_namespace and
run_nft_commands_netns into one helper parameterized by an optional command
prefix. Have each existing function supply its namespace-specific command
invocation while preserving the current nsenter --net= behavior and error
semantics.
In `@crates/openshell-supervisor-process/src/netns/nft_ruleset.rs`:
- Around line 36-367: Refactor the duplicated rule construction in
generate_bypass_commands and generate_sidecar_bypass_commands into shared
helpers for protocol logging and IPv4/IPv6 TCP/UDP rejection, parameterized by
table and the relevant protocol or ICMP type. Replace both functions’ repeated
blocks with these helpers while preserving command ordering, nft_cmd failure
flags, and each function’s existing accept-rule preamble.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d0fb1df-03fb-4c6e-96ab-295002c80f37
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (78)
.agents/skills/create-github-issue/SKILL.md.agents/skills/debug-openshell-cluster/SKILL.md.agents/skills/helm-dev-environment/SKILL.md.agents/skills/triage-issue/SKILL.md.github/ISSUE_TEMPLATE/bug_report.yml.github/workflows/branch-e2e.yml.github/workflows/sync-docs.yml.packit.yamlREADME.mdarchitecture/build.mdarchitecture/compute-runtimes.mdbrainstorm/00-overview.mdcrates/openshell-cli/src/main.rscrates/openshell-cli/src/run.rscrates/openshell-cli/tests/provider_commands_integration.rscrates/openshell-core/src/config.rscrates/openshell-core/src/driver_mounts.rscrates/openshell-core/src/grpc_client.rscrates/openshell-core/src/provider_credentials.rscrates/openshell-core/src/sandbox_env.rscrates/openshell-driver-docker/src/lib.rscrates/openshell-driver-docker/src/tests.rscrates/openshell-driver-kubernetes/README.mdcrates/openshell-driver-kubernetes/src/config.rscrates/openshell-driver-kubernetes/src/driver.rscrates/openshell-driver-kubernetes/src/lib.rscrates/openshell-driver-kubernetes/src/main.rscrates/openshell-driver-podman/README.mdcrates/openshell-driver-podman/src/config.rscrates/openshell-driver-podman/src/container.rscrates/openshell-driver-podman/src/main.rscrates/openshell-ocsf/src/format/shorthand.rscrates/openshell-sandbox/Cargo.tomlcrates/openshell-sandbox/src/lib.rscrates/openshell-sandbox/src/main.rscrates/openshell-sandbox/src/sidecar_control.rscrates/openshell-supervisor-network/data/sandbox-policy.regocrates/openshell-supervisor-network/src/identity.rscrates/openshell-supervisor-network/src/l7/relay.rscrates/openshell-supervisor-network/src/opa.rscrates/openshell-supervisor-network/src/proxy.rscrates/openshell-supervisor-network/src/run.rscrates/openshell-supervisor-process/src/lib.rscrates/openshell-supervisor-process/src/netns/mod.rscrates/openshell-supervisor-process/src/netns/nft_ruleset.rscrates/openshell-supervisor-process/src/process.rscrates/openshell-supervisor-process/src/run.rscrates/openshell-supervisor-process/src/sandbox/linux/landlock.rscrates/openshell-supervisor-process/src/sandbox/linux/mod.rscrates/openshell-supervisor-process/src/ssh.rscrates/openshell-supervisor-process/src/supervisor_session.rscrates/openshell-supervisor-process/src/unix_socket.rscrates/openshell-tui/Cargo.tomlcrates/openshell-tui/src/event.rscrates/openshell-tui/src/lib.rsdeploy/docker/Dockerfile.supervisordeploy/helm/openshell/README.mddeploy/helm/openshell/README.md.gotmpldeploy/helm/openshell/ci/values-sidecar-kata.yamldeploy/helm/openshell/ci/values-sidecar.yamldeploy/helm/openshell/skaffold.yamldeploy/helm/openshell/templates/_helpers.tpldeploy/helm/openshell/templates/gateway-config.yamldeploy/helm/openshell/tests/gateway_config_test.yamldeploy/helm/openshell/values.yamldocs/kubernetes/access-control.mdxdocs/kubernetes/ingress.mdxdocs/kubernetes/managing-certificates.mdxdocs/kubernetes/openshift.mdxdocs/kubernetes/setup.mdxdocs/kubernetes/topology.mdxdocs/reference/gateway-config.mdxdocs/reference/sandbox-compute-drivers.mdxdocs/sandboxes/manage-providers.mdxe2e/with-kube-gateway.shtasks/helm.tomltasks/test.tomltelemetry/README.md
💤 Files with no reviewable changes (1)
- crates/openshell-tui/Cargo.toml
✅ Files skipped from review due to trivial changes (14)
- crates/openshell-supervisor-process/src/lib.rs
- docs/kubernetes/ingress.mdx
- deploy/helm/openshell/ci/values-sidecar.yaml
- .github/ISSUE_TEMPLATE/bug_report.yml
- deploy/helm/openshell/README.md.gotmpl
- crates/openshell-tui/src/event.rs
- .github/workflows/sync-docs.yml
- docs/kubernetes/access-control.mdx
- docs/kubernetes/managing-certificates.mdx
- README.md
- crates/openshell-driver-podman/README.md
- docs/kubernetes/setup.mdx
- .packit.yaml
- docs/reference/sandbox-compute-drivers.mdx
| <<<<<<< HEAD | ||
| - ~~Should the defensive skip use `debug!` or `warn!` level? (from #05)~~ Resolved: `debug!` used in implementation |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the unresolved merge-conflict marker.
Line 32 leaves <<<<<<< HEAD in the committed document, so the conflict is not fully resolved. Delete the marker and retain the resolved debug! entry on Line 33.
🧰 Tools
🪛 LanguageTool
[style] ~33-~33: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1386 characters long)
Context: ...r warn! level? (from #05)~~ Resolved: debug! used in implementation ## Parked Idea...
(EN_EXCESSIVE_EXCLAMATION)
🤖 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 `@brainstorm/00-overview.md` around lines 32 - 33, Remove the leftover `<<<<<<<
HEAD` merge-conflict marker from the overview entry, while retaining the
resolved `debug!` decision text.
| binary_identity_required if { | ||
| object.get(object.get(data, "runtime", {}), "require_binary_identity", true) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Compare the resolved value to true explicitly.
The rule body uses a bare object.get(...) term, which Regal flags (Non-boolean return value unassigned). A non-boolean value here would satisfy the rule in surprising ways. Make the boolean intent explicit so only a genuine true/absent value enforces binary identity and false relaxes it.
Proposed fix
binary_identity_required if {
- object.get(object.get(data, "runtime", {}), "require_binary_identity", true)
+ object.get(object.get(data, "runtime", {}), "require_binary_identity", true) == true
}📝 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.
| binary_identity_required if { | |
| object.get(object.get(data, "runtime", {}), "require_binary_identity", true) | |
| } | |
| binary_identity_required if { | |
| object.get(object.get(data, "runtime", {}), "require_binary_identity", true) == true | |
| } |
🧰 Tools
🪛 Regal (0.41.1)
[error] 23-23: Non-boolean return value unassigned
(bugs)
🤖 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 `@crates/openshell-supervisor-network/data/sandbox-policy.rego` around lines 22
- 24, Update the binary_identity_required rule to explicitly compare the
resolved object.get value to true, preserving the default-true behavior when
require_binary_identity is absent and allowing an explicit false value to
disable enforcement.
Source: Linters/SAST tools
|
|
||
| **Where sandboxes are created (last 2 weeks).** The United States leads by a wide margin, followed by Israel, Australia, Hong Kong, India, Singapore, South Korea, Germany, Japan, and China. | ||
|
|
||
| **Providers (last 2 weeks).** `custom` profiles lead (~30k), then `openai` (~21k), with nvidia, claude, anthropic, github, gitlab, opencode, codex, and copilot trailing. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the official capitalization: GitHub.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: The official name of this software platform is spelled with a capital “H”.
Context: ...(~21k), with nvidia, claude, anthropic, github, gitlab, opencode, codex, and copilot t...
(GITHUB)
🤖 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 `@telemetry/README.md` at line 37, Update the provider name in the “Providers
(last 2 weeks)” list from “github” to the official capitalization “GitHub”;
leave the other provider names unchanged.
Source: Linters/SAST tools
- Remove dead boolCount function that would fail golangci-lint (#1) - Emit EventAdded for the first watch event instead of EventModified, matching k8s watch semantics (#7) - Add mutex locking to all mock server methods that access the shared sandboxes map, fixing latent race conditions (#12) - Skip HealthCheck integration test that calls an unimplemented stub (#13) - Scope doc.go examples: mark sections for sub-clients not yet available in this PR with "available in a future release" (#4) - Document Config.Timeout/RetryPolicy/Logger and WatchOptions fields as reserved for future use (#2, #6) Signed-off-by: Roland Huß <rhuss@redhat.com>
Summary
openshell.ai/managed-by=openshelllabel selector to the K8s driver'swatch_sandboxes()andlist_sandboxes()so only gateway-managed Sandbox objects are received from the API serversandbox_id_from_objectandsandbox_from_objectextraction pathsRelated Issue
Fixes NVIDIA/OpenShell#2211
Changes
Single file change:
crates/openshell-driver-kubernetes/src/driver.rsLabel selector (primary fix):
watch_sandboxes():watcher::Config::default()→watcher::Config::default().labels("openshell.ai/managed-by=openshell")list_sandboxes():ListParams::default()→ListParams::default().labels("openshell.ai/managed-by=openshell")Defensive skip (defense-in-depth):
if let Ok(...)to process known objects anddebug!log + skip unknown objects, instead of sending errors to the channelTests:
test_sandbox_id_from_object_with_label- extraction via labeltest_sandbox_id_from_object_with_name_prefix- extraction via name fallbacktest_sandbox_id_from_object_unknown- rejection of warm pool objectstest_sandbox_id_from_object_managed_by_but_no_sandbox_id- edge case: managed-by present but no sandbox-idtest_sandbox_from_object_success- full Sandbox constructiontest_sandbox_from_object_unknown- rejection of unrecognized objectsTesting
cargo clippy -p openshell-driver-kubernetes --all-targets -- -D warnings: Cleancargo test -p openshell-driver-kubernetes: 99 passed (93 existing + 6 new)Checklist
Summary by CodeRabbit
New Features
proxyUidandprocessBinaryAwareNetworkPolicy, plus sidecar-focused CI/E2E overlays.openshell provider refresh configurewith--secret-material-envfor supplying secret material via environment variables.Bug Fixes
Tests
Documentation