diff --git a/.agents/skills/update-docs/SKILL.md b/.agents/skills/update-docs/SKILL.md index 8229ada34b..20d3e47d11 100644 --- a/.agents/skills/update-docs/SKILL.md +++ b/.agents/skills/update-docs/SKILL.md @@ -108,6 +108,7 @@ Write the doc update following the rules in `docs/CONTRIBUTING.mdx`. Key reminde - **Use `sidebar-title` for short nav labels**. For explicit navigation entries, keep relative `slug` values in `docs/index.yml` instead of page frontmatter. - **Keep explicit `page:` entries in `docs/index.yml`**. Fern still requires them. If the page defines `sidebar-title`, set `page:` to that value. Otherwise set `page:` to the page frontmatter `title`. - **Use `skip-slug: true` in `docs/index.yml`** when a child page should live at the parent section path. +- **Keep each page URL equal to its file path** under `docs/`. Rename the file when you rename a page, add a `fern/docs.yml` redirect for the old URL, and set a relative `slug:` when the nav label does not produce the file name. `mise run docs` runs `docs:nav`, which fails otherwise. - **Use `keywords` as a comma-separated string**. - **Do not add a duplicate H1**. Fern renders the page title from frontmatter. - **Always write NVIDIA in all caps.** Wrong: Nvidia, nvidia. diff --git a/.github/workflows/branch-docs.yml b/.github/workflows/branch-docs.yml index 9333a6ee1e..139b14f75b 100644 --- a/.github/workflows/branch-docs.yml +++ b/.github/workflows/branch-docs.yml @@ -7,6 +7,9 @@ on: - "fern/**" - "mise.toml" - "tasks/docs.toml" + - "tasks/test.toml" + - "tasks/scripts/check_docs_nav.py" + - "tasks/scripts/check_docs_nav_test.py" - ".github/workflows/branch-docs.yml" - ".github/workflows/release-tag.yml" @@ -47,6 +50,18 @@ jobs: working-directory: ./fern run: fern check + - name: Install uv + uses: astral-sh/setup-uv@ae62891fec2bb8e7d6c99fc78c9fec3a63790f8d # v10.0.0 + with: + version: "0.10.12" + python-version: "3.13" + + - name: Test docs navigation check + run: uv run --no-project --with pytest --with pytest-asyncio --with pyyaml pytest tasks/scripts/check_docs_nav_test.py + + - name: Check docs navigation matches file paths + run: uv run tasks/scripts/check_docs_nav.py + - name: Generate preview URL if: ${{ steps.fern-preview.outputs.enabled == 'true' }} id: generate-docs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4213239b35..60a9d3f15c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -487,7 +487,7 @@ field design, and schema-evolution rules. ## Documentation -If your change affects user-facing behavior (new flags, changed defaults, new features, bug fixes that contradict existing docs), update the relevant pages under `docs/` in the same PR and adjust `docs/index.yml` if navigation changes. For explicit navigation entries, keep `page:` aligned with `sidebar-title` when present and put relative `slug:` values in `docs/index.yml`. Reserve frontmatter `slug` for folder-discovered pages or absolute URL overrides. +If your change affects user-facing behavior (new flags, changed defaults, new features, bug fixes that contradict existing docs), update the relevant pages under `docs/` in the same PR and adjust `docs/index.yml` if navigation changes. For explicit navigation entries, keep `page:` aligned with `sidebar-title` when present and put relative `slug:` values in `docs/index.yml`. Reserve frontmatter `slug` for folder-discovered pages or absolute URL overrides. Keep every page URL equal to its file path under `docs/`; `mise run docs` checks this with `docs:nav`. To ensure your doc changes follow NVIDIA documentation style, use the `update-docs-from-commits` skill. It scans commits, identifies doc pages that need updates, and drafts content that follows the style guide in `docs/CONTRIBUTING.mdx`. diff --git a/architecture/build.md b/architecture/build.md index 11765e2706..b5c05881b9 100644 --- a/architecture/build.md +++ b/architecture/build.md @@ -584,8 +584,10 @@ See `CI.md` for the contributor workflow, labels, and maintainer merge-queue wor Published docs live in `docs/`. Navigation lives in `docs/index.yml`. Fern site configuration, components, theme assets, and publish settings live in `fern/`. -Use `mise run docs` for strict validation and `mise run docs:serve` for local -preview. PR previews are produced by `.github/workflows/branch-docs.yml` when +Use `mise run docs` for Fern validation and navigation-to-file-path consistency, +and `mise run docs:serve` for local preview. The docs PR workflow also runs the +navigation check's unit tests (`mise run test:docs-nav`). +PR previews are produced by `.github/workflows/branch-docs.yml` when Fern credentials are available. Production docs publish from the release tag workflow. Redirect rules follow the mutable snapshot that owns their source URL (or destination for unversioned aliases). Syncing replaces that channel's rules, diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 71a43bce2a..ec6fff115a 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -107,6 +107,11 @@ whose compute resource is still provisioning without exposing contradictory publ readiness signals. When the driver reports runtime readiness, its ready condition is published without waiting for a supervisor session. +The supervisor keeps retrying session establishment while the gateway is unavailable. +Its control readiness socket remains absent until the gateway accepts a session and +is removed if that session disconnects. A transient gateway delay during startup +therefore leaves the sandbox provisioning without terminating the supervisor. + **Session precedence over lagging driver snapshots:** A supervisor session can only be established by a running workload. When `set_supervisor_session_state` promotes the store record to `Ready` on session connect, a driver watch event may still arrive diff --git a/architecture/sandbox.md b/architecture/sandbox.md index b1169ca64e..a8a09310d0 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -131,6 +131,11 @@ When the admitted main process exits, its status and retained terminal output remain available. The confirmed sandbox and supervisor-owned access plane continue to serve policy-authorized exec and loopback forwarding until explicit stop or delete tears down the boundary and terminates any remaining workload processes. +When the sandbox is PID 1, it reaps adopted workload children after child-exit +notifications, with a low-frequency recovery sweep. Children owned by an +explicit process waiter remain registered and are never consumed by the orphan +reaper. This keeps idle sandboxes from scanning procfs continuously while +preserving wait results and eventual zombie cleanup. Completed exec output handles can be reclaimed, but execution request IDs remain reserved for the boundary generation. The sandbox accepts at most 4,096 exec diff --git a/crates/openshell-sandbox/src/managed_children.rs b/crates/openshell-sandbox/src/managed_children.rs index d224176e97..adbfdb62de 100644 --- a/crates/openshell-sandbox/src/managed_children.rs +++ b/crates/openshell-sandbox/src/managed_children.rs @@ -9,9 +9,11 @@ //! from incidental zombies. #![cfg(target_os = "linux")] +#![allow(unsafe_code)] use std::collections::{HashMap, HashSet}; use std::io; +use std::sync::atomic::AtomicI32; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{LazyLock, Mutex, MutexGuard}; use std::time::Duration; @@ -19,6 +21,22 @@ use std::time::Duration; static MANAGED_CHILDREN: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); static NEXT_GENERATION: AtomicU64 = AtomicU64::new(1); +static REAPER_EVENT_FD: AtomicI32 = AtomicI32::new(-1); +const REAPER_RECOVERY_INTERVAL: Duration = Duration::from_secs(30); +#[cfg(test)] +static REAPER_SCAN_COUNT: AtomicU64 = AtomicU64::new(0); + +extern "C" fn notify_reaper_on_sigchld(_: libc::c_int) { + let fd = REAPER_EVENT_FD.load(Ordering::Acquire); + if fd >= 0 { + let wakeup = 1_u64; + // SAFETY: write is async-signal-safe; the process-lifetime eventfd is + // nonblocking, so a full counter cannot stall the signal handler. + unsafe { + libc::write(fd, (&raw const wakeup).cast(), size_of::()); + } + } +} /// Identity of one registry entry. The generation prevents an old waiter from /// removing a newer child that reused the same numeric PID after reap. @@ -110,22 +128,87 @@ pub fn wait_until_terminal(pid: u32) -> io::Result<()> { /// Explicitly managed children remain owned by their normal waiters. Only /// unregistered children adopted from the workload process tree are reaped. pub fn start_orphan_reaper() -> io::Result<()> { + // This is installed before the boundary starts its runtime threads or + // workload children. The descriptor and handler live until process exit. + // SAFETY: eventfd has scalar arguments and returns a new descriptor. + let fd = unsafe { libc::eventfd(0, libc::EFD_CLOEXEC | libc::EFD_NONBLOCK) }; + if fd < 0 { + return Err(io::Error::last_os_error()); + } + REAPER_EVENT_FD.store(fd, Ordering::Release); + let action = nix::sys::signal::SigAction::new( + nix::sys::signal::SigHandler::Handler(notify_reaper_on_sigchld), + nix::sys::signal::SaFlags::SA_RESTART | nix::sys::signal::SaFlags::SA_NOCLDSTOP, + nix::sys::signal::SigSet::empty(), + ); + // SAFETY: the handler only reads a lock-free atomic and writes to the + // nonblocking eventfd, both valid for the remaining process lifetime. + unsafe { nix::sys::signal::sigaction(nix::sys::signal::Signal::SIGCHLD, &action) } + .map_err(io::Error::other)?; std::thread::Builder::new() .name("openshell-orphan-reaper".to_string()) - .spawn(|| { + .spawn(move || { loop { if let Err(error) = reap_unmanaged_children_once() { tracing::debug!(%error, "orphan reaper scan failed"); } - std::thread::sleep(Duration::from_millis(50)); + if let Err(error) = wait_for_reaper_event(fd) { + tracing::debug!(%error, "orphan reaper notification failed"); + std::thread::sleep(REAPER_RECOVERY_INTERVAL); + } } }) .map(|_| ()) } +fn wait_for_reaper_event(fd: libc::c_int) -> io::Result<()> { + let mut notification = libc::pollfd { + fd, + events: libc::POLLIN, + revents: 0, + }; + // SAFETY: poll receives one valid pollfd for the process-lifetime eventfd. + let ready = unsafe { + libc::poll( + &raw mut notification, + 1, + i32::try_from(REAPER_RECOVERY_INTERVAL.as_millis()).unwrap(), + ) + }; + if ready < 0 { + let error = io::Error::last_os_error(); + if error.kind() == io::ErrorKind::Interrupted { + return Ok(()); + } + return Err(error); + } + if ready == 0 { + return Ok(()); + } + if notification.revents & libc::POLLIN == 0 { + return Err(io::Error::other("orphan reaper eventfd is not readable")); + } + let mut count = 0_u64; + // Drain before scanning: a SIGCHLD arriving during the next scan remains + // queued and triggers another pass. A single read drains coalesced writes. + // SAFETY: read writes one u64 into valid storage; the fd is nonblocking. + let read = unsafe { libc::read(fd, (&raw mut count).cast(), size_of::()) }; + if read == isize::try_from(size_of::()).expect("u64 size fits isize") + || (read < 0 && io::Error::last_os_error().kind() == io::ErrorKind::WouldBlock) + { + Ok(()) + } else if read < 0 { + Err(io::Error::last_os_error()) + } else { + Err(io::Error::other("short orphan reaper eventfd read")) + } +} + fn reap_unmanaged_children_once() -> io::Result { use nix::sys::wait::{WaitPidFlag, WaitStatus, waitpid}; + #[cfg(test)] + REAPER_SCAN_COUNT.fetch_add(1, Ordering::Relaxed); let children = direct_child_pids()?; let registry = lock(); let mut reaped = 0; @@ -170,7 +253,89 @@ mod tests { use nix::unistd::Pid; use std::process::Command; use std::sync::mpsc; - use std::time::Duration; + use std::time::{Duration, Instant}; + + fn fork_exiting_child() -> i32 { + // SAFETY: the child does not run Rust code after fork; it exits through + // libc immediately, avoiding inherited test-harness locks. + let pid = unsafe { libc::fork() }; + if pid == 0 { + // SAFETY: terminate without unwinding or touching Rust state. + unsafe { libc::_exit(0) }; + } + assert!(pid > 0, "fork failed: {}", io::Error::last_os_error()); + pid + } + + fn wait_until(mut condition: impl FnMut() -> bool) { + let deadline = Instant::now() + Duration::from_secs(5); + while !condition() { + assert!(Instant::now() < deadline, "condition did not become true"); + std::thread::sleep(Duration::from_millis(10)); + } + } + + #[test] + fn orphan_reaper_waits_while_idle_and_reaps_only_unmanaged_children() { + const HELPER_ENV: &str = "OPENSHELL_ORPHAN_REAPER_TEST_HELPER"; + if std::env::var_os(HELPER_ENV).is_none() { + let output = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "managed_children::tests::orphan_reaper_waits_while_idle_and_reaps_only_unmanaged_children", + "--nocapture", + ]) + .env(HELPER_ENV, "1") + .output() + .expect("start isolated reaper test process"); + assert!( + output.status.success(), + "reaper test failed: {}", + String::from_utf8_lossy(&output.stdout) + ); + return; + } + + start_orphan_reaper().expect("start orphan reaper"); + wait_until(|| REAPER_SCAN_COUNT.load(Ordering::Relaxed) > 0); + let idle_scan_count = REAPER_SCAN_COUNT.load(Ordering::Relaxed); + std::thread::sleep(Duration::from_millis(150)); + assert_eq!(REAPER_SCAN_COUNT.load(Ordering::Relaxed), idle_scan_count); + + let mut registry = lock(); + let managed_pid = fork_exiting_child(); + let managed = registry + .register(u32::try_from(managed_pid).unwrap()) + .unwrap(); + drop(registry); + wait_until(|| { + matches!( + waitid( + Id::Pid(Pid::from_raw(managed_pid)), + WaitPidFlag::WEXITED | WaitPidFlag::WNOHANG | WaitPidFlag::WNOWAIT + ), + Ok(WaitStatus::Exited(..)) + ) + }); + wait_until(|| REAPER_SCAN_COUNT.load(Ordering::Relaxed) > idle_scan_count); + assert!(matches!( + waitpid(Pid::from_raw(managed_pid), None), + Ok(WaitStatus::Exited(..)) + )); + unregister(managed); + + let orphan_pids: Vec = (0..8).map(|_| fork_exiting_child()).collect(); + wait_until(|| { + let children = direct_child_pids().expect("inspect direct children"); + orphan_pids.iter().all(|pid| !children.contains(pid)) + }); + for pid in orphan_pids { + assert_eq!( + waitpid(Pid::from_raw(pid), Some(WaitPidFlag::WNOHANG)), + Err(nix::errno::Errno::ECHILD) + ); + } + } #[test] fn fast_child_remains_waitable_after_orphan_reap_attempt() { diff --git a/crates/openshell-supervisor-process/src/delegated.rs b/crates/openshell-supervisor-process/src/delegated.rs index dde9a7812e..87101f62a2 100644 --- a/crates/openshell-supervisor-process/src/delegated.rs +++ b/crates/openshell-supervisor-process/src/delegated.rs @@ -156,7 +156,7 @@ pub async fn start_boundary_access( let (session_task, session_readiness) = match (openshell_endpoint, sandbox_id) { (Some(endpoint), Some(id)) => { - let (task, mut accepted) = crate::supervisor_session::spawn_with_readiness( + let (task, accepted) = crate::supervisor_session::spawn_with_readiness( endpoint.to_string(), id.to_string(), ssh_socket_path, @@ -168,25 +168,10 @@ pub async fn start_boundary_access( session_id_updates: supervisor_session_updates, }, ); - let accepted_result = - tokio::time::timeout(Duration::from_secs(10), accepted.wait_for(|ready| *ready)) - .await - .map(|result| result.map(|_| ())); - match accepted_result { - Ok(Ok(())) => (Some(task), Some(accepted)), - Ok(Err(_)) => { - task.abort(); - return Err(miette::miette!( - "supervisor session ended before gateway acceptance" - )); - } - Err(_) => { - task.abort(); - return Err(miette::miette!( - "gateway did not accept supervisor session within 10 seconds" - )); - } - } + // Session establishment retries through gateway restarts. The + // readiness socket remains absent until the gateway accepts the + // session, so a transient delay cannot kill the supervisor. + (Some(task), Some(accepted)) } _ => (None, None), }; diff --git a/crates/openshell-supervisor/src/lib.rs b/crates/openshell-supervisor/src/lib.rs index fd5ed08a48..8dc3f97600 100644 --- a/crates/openshell-supervisor/src/lib.rs +++ b/crates/openshell-supervisor/src/lib.rs @@ -100,21 +100,24 @@ impl ControlReadiness { path: std::path::PathBuf, mut session_readiness: Option>, ) -> Result { - if session_readiness + prepare_control_readiness_path(&path)?; + let listener = if session_readiness .as_ref() .is_some_and(|readiness| !*readiness.borrow()) { - return Err(miette::miette!( - "supervisor session is not ready when starting health listener" - )); - } - prepare_control_readiness_path(&path)?; - let listener = tokio::net::UnixListener::bind(&path) - .into_diagnostic() - .wrap_err_with(|| format!("bind supervisor readiness socket on {}", path.display()))?; + None + } else { + Some( + tokio::net::UnixListener::bind(&path) + .into_diagnostic() + .wrap_err_with(|| { + format!("bind supervisor readiness socket on {}", path.display()) + })?, + ) + }; let task_path = path.clone(); let task = tokio::spawn(async move { - let mut listener = Some(listener); + let mut listener = listener; loop { let session_unready = session_readiness .as_ref() @@ -4754,10 +4757,19 @@ mod tests { async fn control_readiness_tracks_supervisor_session() { let root = tempfile::tempdir().unwrap(); let path = root.path().join("health.sock"); - let (session_tx, session_rx) = tokio::sync::watch::channel(true); + let (session_tx, session_rx) = tokio::sync::watch::channel(false); let _readiness = ControlReadiness::start(path.clone(), Some(session_rx)) .expect("start readiness listener"); - check_control_readiness(&path).expect("accepted session is ready"); + assert!(check_control_readiness(&path).is_err()); + + session_tx.send_replace(true); + timeout(Duration::from_secs(1), async { + while check_control_readiness(&path).is_err() { + tokio::task::yield_now().await; + } + }) + .await + .expect("first accepted session creates readiness socket"); session_tx.send_replace(false); timeout(Duration::from_secs(1), async { diff --git a/deploy/helm/openshell/README.md b/deploy/helm/openshell/README.md index 4d09c32de8..bfb60ddb80 100644 --- a/deploy/helm/openshell/README.md +++ b/deploy/helm/openshell/README.md @@ -17,6 +17,114 @@ namespace. The gateway and workspace releases can then be upgraded and removed independently. Use Kubernetes `operator` workspace mode when one gateway serves multiple pre-provisioned workspace namespaces. +## Cluster-scoped vs namespaced objects + +Most objects in this chart are namespaced and land in the release namespace. +Only two are cluster-scoped: + +| Object | Default name | +| --- | --- | +| `ClusterRole` | `-node-reader-` | +| `ClusterRoleBinding` | `-node-reader-` | + +By default the release creates both, so an install by a cluster-admin is +unchanged. On clusters where cluster-scoped RBAC is owned by a different team, +split the install in two. + +A cluster-admin applies the cluster-scoped objects once per gateway +ServiceAccount, rendered from the same values the release uses: + +```shell +helm template openshell oci://ghcr.io/nvidia/openshell/helm-chart --version \ + --namespace openshell -f my-values.yaml \ + --set rbac.create=true \ + --set rbac.clusterScoped.create=true \ + --set agentSandbox.preflight.enabled=false \ + --show-only templates/clusterrole.yaml \ + --show-only templates/clusterrolebinding.yaml | kubectl apply -f - +``` + +A namespace-admin then installs and upgrades the release with cluster-scoped +objects omitted, using [`ci/values-namespace-admin.yaml`](ci/values-namespace-admin.yaml) +or the equivalent `--set`: + +```shell +helm upgrade --install openshell oci://ghcr.io/nvidia/openshell/helm-chart --version \ + --namespace openshell -f my-values.yaml \ + --set rbac.clusterScoped.create=false +``` + +The gateway ServiceAccount name and namespace do not change, so the +pre-created `ClusterRoleBinding` keeps matching the release. This works with +`serviceAccount.create=false` too: the `ClusterRoleBinding` subject follows +`serviceAccount.name`, so render the admin step with the same values. + +### Which flag the installer needs + +In the default `shared` workspace mode the release also creates a namespaced +sandbox `Role` granting Agent Sandbox (`agents.x-k8s.io`) permissions. +Kubernetes forbids granting permissions you do not hold, and the built-in +`admin` ClusterRole does not cover that CRD, so an installer holding only +`admin` cannot create it. `rbac.clusterScoped.create=false` alone is then not +enough and the install fails with `attempting to grant RBAC permissions not +currently held`. + +| Workspace mode | Installer holds | Use | +| --- | --- | --- | +| `shared` | built-in `admin` only | `rbac.create=false`, cluster-admin pre-creates all gateway RBAC | +| `shared` | `admin` plus the sandbox permissions in the namespace | `rbac.clusterScoped.create=false` | +| `managed`, `operator` | built-in `admin` only | `rbac.clusterScoped.create=false` | + +`managed` and `operator` render no namespaced sandbox `Role`, so no extra grant +is needed there. + +With `rbac.create=false` the cluster-admin applies the namespaced RBAC too, +adding it to the same render: + +```shell +helm template openshell oci://ghcr.io/nvidia/openshell/helm-chart --version \ + --namespace openshell -f my-values.yaml \ + --set rbac.create=true \ + --set rbac.clusterScoped.create=true \ + --set agentSandbox.preflight.enabled=false \ + --show-only templates/clusterrole.yaml \ + --show-only templates/clusterrolebinding.yaml \ + --show-only templates/role.yaml \ + --show-only templates/rolebinding.yaml \ + --show-only templates/peer-role.yaml | kubectl apply -f - +``` + +To grant the installer the sandbox permissions instead, bind it to a Role +carrying the same rules as the chart's `openshell-sandbox` Role. + +The certgen hook and credential driver RBAC keep their own flags +(`pkiInitJob.enabled` and +`server.credentialDrivers.kubernetesSecrets.rbac.create`). + +### Migrating an existing release + +Helm deletes objects that leave a release manifest, so setting +`rbac.clusterScoped.create=false` on a release that already owns the +`ClusterRole` and `ClusterRoleBinding` deletes them. The gateway then loses +TokenReview until a cluster-admin re-applies them. Hand ownership over first, as +cluster-admin, so nothing is deleted: + +```shell +kubectl annotate clusterrole "openshell-node-reader-" \ + helm.sh/resource-policy=keep --overwrite +kubectl annotate clusterrolebinding "openshell-node-reader-" \ + helm.sh/resource-policy=keep --overwrite +``` + +The objects then survive the upgrade that sets the flag, and the cluster-admin +owns them from that point on. Fresh installs need no such step. + +`rbac.clusterScoped.create` is independent of +`server.drivers.kubernetes.workspaceMode`. Managed and operator modes change +what the `ClusterRole` contains, but they never force the namespaced release to +apply it. Re-run the cluster-admin step after changing values that affect the +`ClusterRole` rules. + ## Prerequisites > **Required:** Your cluster CNI MUST enforce Kubernetes `NetworkPolicy` for @@ -260,6 +368,10 @@ discovery endpoint or its TLS CA. | probes.startup.failureThreshold | int | `30` | Startup probe failure threshold before the container is killed. | | probes.startup.periodSeconds | int | `2` | Startup probe period, in seconds. | | probes.startup.timeoutSeconds | int | `1` | Startup probe timeout, in seconds. | +| rbac.clusterScoped.clusterRoleBindingName | string | `""` | Name for the ClusterRoleBinding. Empty uses the `-node-reader-` default. | +| rbac.clusterScoped.clusterRoleName | string | `""` | Name for the ClusterRole. Empty uses the `-node-reader-` default. | +| rbac.clusterScoped.create | bool | `true` | Create the cluster-scoped ClusterRole and ClusterRoleBinding. Disable for a namespace-admin install where a cluster-admin applies them separately; the gateway ServiceAccount name and namespace are unchanged, so a pre-created ClusterRoleBinding still matches. | +| rbac.create | bool | `true` | Create the RBAC objects that grant the gateway ServiceAccount access. Disable to supply the namespaced sandbox and peer Role/RoleBinding and the cluster-scoped ClusterRole/ClusterRoleBinding out of band. The certgen hook and credential driver RBAC keep their own flags. | | replicaCount | int | `1` | Number of OpenShell gateway replicas. Values greater than 1 require server.externalDbSecret because the default SQLite backend is per pod. | | resources | object | `{}` | Gateway pod resource requests and limits. | | sandbox.image.digest | string | `""` | Sandbox image digest. When set, this takes precedence over tag. | diff --git a/deploy/helm/openshell/README.md.gotmpl b/deploy/helm/openshell/README.md.gotmpl index 6e49260c20..1e86e0cbcf 100644 --- a/deploy/helm/openshell/README.md.gotmpl +++ b/deploy/helm/openshell/README.md.gotmpl @@ -17,6 +17,115 @@ namespace. The gateway and workspace releases can then be upgraded and removed independently. Use Kubernetes `operator` workspace mode when one gateway serves multiple pre-provisioned workspace namespaces. +## Cluster-scoped vs namespaced objects + +Most objects in this chart are namespaced and land in the release namespace. +Only two are cluster-scoped: + +| Object | Default name | +| --- | --- | +| `ClusterRole` | `-node-reader-` | +| `ClusterRoleBinding` | `-node-reader-` | + +By default the release creates both, so an install by a cluster-admin is +unchanged. On clusters where cluster-scoped RBAC is owned by a different team, +split the install in two. + +A cluster-admin applies the cluster-scoped objects once per gateway +ServiceAccount, rendered from the same values the release uses: + +```shell +helm template openshell oci://ghcr.io/nvidia/openshell/helm-chart --version \ + --namespace openshell -f my-values.yaml \ + --set rbac.create=true \ + --set rbac.clusterScoped.create=true \ + --set agentSandbox.preflight.enabled=false \ + --show-only templates/clusterrole.yaml \ + --show-only templates/clusterrolebinding.yaml | kubectl apply -f - +``` + +A namespace-admin then installs and upgrades the release with cluster-scoped +objects omitted, using [`ci/values-namespace-admin.yaml`](ci/values-namespace-admin.yaml) +or the equivalent `--set`: + +```shell +helm upgrade --install openshell oci://ghcr.io/nvidia/openshell/helm-chart --version \ + --namespace openshell -f my-values.yaml \ + --set rbac.clusterScoped.create=false +``` + +The gateway ServiceAccount name and namespace do not change, so the +pre-created `ClusterRoleBinding` keeps matching the release. This works with +`serviceAccount.create=false` too: the `ClusterRoleBinding` subject follows +`serviceAccount.name`, so render the admin step with the same values. + +### Which flag the installer needs + +In the default `shared` workspace mode the release also creates a namespaced +sandbox `Role` granting Agent Sandbox (`agents.x-k8s.io`) permissions. +Kubernetes forbids granting permissions you do not hold, and the built-in +`admin` ClusterRole does not cover that CRD, so an installer holding only +`admin` cannot create it. `rbac.clusterScoped.create=false` alone is then not +enough and the install fails with `attempting to grant RBAC permissions not +currently held`. + +| Workspace mode | Installer holds | Use | +| --- | --- | --- | +| `shared` | built-in `admin` only | `rbac.create=false`, cluster-admin pre-creates all gateway RBAC | +| `shared` | `admin` plus the sandbox permissions in the namespace | `rbac.clusterScoped.create=false` | +| `managed`, `operator` | built-in `admin` only | `rbac.clusterScoped.create=false` | + +`managed` and `operator` render no namespaced sandbox `Role`, so no extra grant +is needed there. + +With `rbac.create=false` the cluster-admin applies the namespaced RBAC too, +adding it to the same render: + +```shell +helm template openshell oci://ghcr.io/nvidia/openshell/helm-chart --version \ + --namespace openshell -f my-values.yaml \ + --set rbac.create=true \ + --set rbac.clusterScoped.create=true \ + --set agentSandbox.preflight.enabled=false \ + --show-only templates/clusterrole.yaml \ + --show-only templates/clusterrolebinding.yaml \ + --show-only templates/role.yaml \ + --show-only templates/rolebinding.yaml \ + --show-only templates/peer-role.yaml | kubectl apply -f - +``` + +To grant the installer the sandbox permissions instead, bind it to a Role +carrying the same rules as the chart's `openshell-sandbox` Role. + +The certgen hook and credential driver RBAC keep their own flags +(`pkiInitJob.enabled` and +`server.credentialDrivers.kubernetesSecrets.rbac.create`). + +### Migrating an existing release + +Helm deletes objects that leave a release manifest, so setting +`rbac.clusterScoped.create=false` on a release that already owns the +`ClusterRole` and `ClusterRoleBinding` deletes them. The gateway then loses +TokenReview until a cluster-admin re-applies them. Hand ownership over first, as +cluster-admin, so nothing is deleted: + +```shell +kubectl annotate clusterrole "openshell-node-reader-" \ + helm.sh/resource-policy=keep --overwrite +kubectl annotate clusterrolebinding "openshell-node-reader-" \ + helm.sh/resource-policy=keep --overwrite +``` + +The objects then survive the upgrade that sets the flag, and the cluster-admin +owns them from that point on. Fresh installs need no such step. + +`rbac.clusterScoped.create` is independent of +`server.drivers.kubernetes.workspaceMode`. Managed and operator modes change +what the `ClusterRole` contains, but they never force the namespaced release to +apply it. Re-run the cluster-admin step after changing values that affect the +`ClusterRole` rules. + + ## Prerequisites > **Required:** Your cluster CNI MUST enforce Kubernetes `NetworkPolicy` for diff --git a/deploy/helm/openshell/ci/values-namespace-admin.yaml b/deploy/helm/openshell/ci/values-namespace-admin.yaml new file mode 100644 index 0000000000..7326be7328 --- /dev/null +++ b/deploy/helm/openshell/ci/values-namespace-admin.yaml @@ -0,0 +1,25 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +# Namespace-admin overlay — renders only namespaced objects. +# +# Use this when a cluster-admin applies the gateway ClusterRole and +# ClusterRoleBinding once, out of band, and the OpenShell release is installed +# and upgraded by an installer that holds no cluster-scoped permissions. +# +# Generate the cluster-scoped objects for the cluster-admin step from the same +# release values, then apply them as cluster-admin: +# helm template openshell oci://ghcr.io/nvidia/openshell/helm-chart \ +# --version --namespace openshell -f my-values.yaml \ +# --set rbac.create=true \ +# --set rbac.clusterScoped.create=true \ +# --set agentSandbox.preflight.enabled=false \ +# --show-only templates/clusterrole.yaml \ +# --show-only templates/clusterrolebinding.yaml | kubectl apply -f - +# +# The gateway ServiceAccount name and release namespace are unchanged, so the +# pre-created ClusterRoleBinding still matches this release. + +rbac: + clusterScoped: + create: false diff --git a/deploy/helm/openshell/templates/_helpers.tpl b/deploy/helm/openshell/templates/_helpers.tpl index 32e6b7cb63..ab42458759 100644 --- a/deploy/helm/openshell/templates/_helpers.tpl +++ b/deploy/helm/openshell/templates/_helpers.tpl @@ -86,6 +86,66 @@ default to enabled so upgrades with --reuse-values preserve the old topology. {{- if $enabled -}}true{{- end -}} {{- end }} +{{/* +Whether this chart owns gateway RBAC objects. Missing legacy values default to +enabled so upgrades with --reuse-values preserve the old topology. +*/}} +{{- define "openshell.rbacCreate" -}} +{{- $rbac := .Values.rbac | default dict -}} +{{- $create := true -}} +{{- if hasKey $rbac "create" -}} +{{- $create = get $rbac "create" -}} +{{- end -}} +{{- if $create -}}true{{- end -}} +{{- end }} + +{{/* +The rbac.clusterScoped values map, tolerating missing legacy values. +*/}} +{{- define "openshell.clusterScopedRbacValues" -}} +{{- $rbac := .Values.rbac | default dict -}} +{{- $clusterScoped := dict -}} +{{- if hasKey $rbac "clusterScoped" -}} +{{- $clusterScoped = get $rbac "clusterScoped" | default dict -}} +{{- end -}} +{{- toYaml $clusterScoped -}} +{{- end }} + +{{/* +Whether this chart owns the cluster-scoped ClusterRole and ClusterRoleBinding. +Disable for a namespace-admin install where a cluster-admin applies them +separately. Missing legacy values default to enabled. +*/}} +{{- define "openshell.clusterRbacCreate" -}} +{{- if include "openshell.rbacCreate" . -}} +{{- $clusterScoped := include "openshell.clusterScopedRbacValues" . | fromYaml -}} +{{- $create := true -}} +{{- if hasKey $clusterScoped "create" -}} +{{- $create = get $clusterScoped "create" -}} +{{- end -}} +{{- if $create -}}true{{- end -}} +{{- end -}} +{{- end }} + +{{/* +Name of the gateway ClusterRole. The release namespace is part of the default +name so multiple releases on one cluster do not collide. +*/}} +{{- define "openshell.clusterRoleName" -}} +{{- $clusterScoped := include "openshell.clusterScopedRbacValues" . | fromYaml -}} +{{- $default := printf "%s-node-reader-%s" (include "openshell.fullname" .) .Release.Namespace -}} +{{- default $default (get $clusterScoped "clusterRoleName") -}} +{{- end }} + +{{/* +Name of the gateway ClusterRoleBinding. +*/}} +{{- define "openshell.clusterRoleBindingName" -}} +{{- $clusterScoped := include "openshell.clusterScopedRbacValues" . | fromYaml -}} +{{- $default := printf "%s-node-reader-%s" (include "openshell.fullname" .) .Release.Namespace -}} +{{- default $default (get $clusterScoped "clusterRoleBindingName") -}} +{{- end }} + {{/* Gateway image reference. A digest takes precedence over a tag. */}} {{- define "openshell.image" -}} {{- $image := .Values.gateway.image -}} diff --git a/deploy/helm/openshell/templates/clusterrole.yaml b/deploy/helm/openshell/templates/clusterrole.yaml index 17c483deb9..85d10c6b60 100644 --- a/deploy/helm/openshell/templates/clusterrole.yaml +++ b/deploy/helm/openshell/templates/clusterrole.yaml @@ -1,3 +1,4 @@ +{{- if include "openshell.clusterRbacCreate" . }} # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 @@ -5,7 +6,7 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - name: {{ include "openshell.fullname" . }}-node-reader-{{ .Release.Namespace }} + name: {{ include "openshell.clusterRoleName" . }} labels: {{- include "openshell.labels" . | nindent 4 }} rules: @@ -128,3 +129,4 @@ rules: - update {{- end }} {{- end }} +{{- end }} diff --git a/deploy/helm/openshell/templates/clusterrolebinding.yaml b/deploy/helm/openshell/templates/clusterrolebinding.yaml index 9b3b254586..8de246730b 100644 --- a/deploy/helm/openshell/templates/clusterrolebinding.yaml +++ b/deploy/helm/openshell/templates/clusterrolebinding.yaml @@ -1,17 +1,19 @@ +{{- if include "openshell.clusterRbacCreate" . }} # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: - name: {{ include "openshell.fullname" . }}-node-reader-{{ .Release.Namespace }} + name: {{ include "openshell.clusterRoleBindingName" . }} labels: {{- include "openshell.labels" . | nindent 4 }} roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: {{ include "openshell.fullname" . }}-node-reader-{{ .Release.Namespace }} + name: {{ include "openshell.clusterRoleName" . }} subjects: - kind: ServiceAccount name: {{ include "openshell.serviceAccountName" . }} namespace: {{ .Release.Namespace }} +{{- end }} diff --git a/deploy/helm/openshell/templates/peer-role.yaml b/deploy/helm/openshell/templates/peer-role.yaml index 59ae7479ad..3aa2502f46 100644 --- a/deploy/helm/openshell/templates/peer-role.yaml +++ b/deploy/helm/openshell/templates/peer-role.yaml @@ -1,3 +1,4 @@ +{{- if include "openshell.rbacCreate" . }} # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 apiVersion: rbac.authorization.k8s.io/v1 @@ -33,3 +34,4 @@ subjects: - kind: ServiceAccount name: {{ include "openshell.serviceAccountName" . }} namespace: {{ .Release.Namespace }} +{{- end }} diff --git a/deploy/helm/openshell/templates/role.yaml b/deploy/helm/openshell/templates/role.yaml index b47efa090b..d9237e743d 100644 --- a/deploy/helm/openshell/templates/role.yaml +++ b/deploy/helm/openshell/templates/role.yaml @@ -1,5 +1,5 @@ {{- $workspaceMode := .Values.server.drivers.kubernetes.workspaceMode | default "shared" -}} -{{- if and (eq $workspaceMode "shared") (include "openshell.workspaceResourcesEnabled" .) }} +{{- if and (eq $workspaceMode "shared") (include "openshell.workspaceResourcesEnabled" .) (include "openshell.rbacCreate" .) }} # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 apiVersion: rbac.authorization.k8s.io/v1 diff --git a/deploy/helm/openshell/templates/rolebinding.yaml b/deploy/helm/openshell/templates/rolebinding.yaml index 32f11644bf..49cc6f7fba 100644 --- a/deploy/helm/openshell/templates/rolebinding.yaml +++ b/deploy/helm/openshell/templates/rolebinding.yaml @@ -1,5 +1,5 @@ {{- $workspaceMode := .Values.server.drivers.kubernetes.workspaceMode | default "shared" -}} -{{- if and (eq $workspaceMode "shared") (include "openshell.workspaceResourcesEnabled" .) }} +{{- if and (eq $workspaceMode "shared") (include "openshell.workspaceResourcesEnabled" .) (include "openshell.rbacCreate" .) }} # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 apiVersion: rbac.authorization.k8s.io/v1 diff --git a/deploy/helm/openshell/tests/clusterrole_test.yaml b/deploy/helm/openshell/tests/clusterrole_test.yaml index 326bc2cb60..08907c614a 100644 --- a/deploy/helm/openshell/tests/clusterrole_test.yaml +++ b/deploy/helm/openshell/tests/clusterrole_test.yaml @@ -163,3 +163,42 @@ tests: apiGroups: [""] resources: ["services"] verbs: ["create", "get"] + - it: omits the ClusterRole when cluster-scoped RBAC is disabled + set: + rbac.clusterScoped.create: false + asserts: + - hasDocuments: + count: 0 + + - it: omits the ClusterRole when all chart-managed RBAC is disabled + set: + rbac.create: false + asserts: + - hasDocuments: + count: 0 + + - it: omits the ClusterRole independently of the workspace mode + set: + rbac.clusterScoped.create: false + server.drivers.kubernetes.workspaceMode: managed + asserts: + - hasDocuments: + count: 0 + + - it: uses the configured ClusterRole name + set: + rbac.clusterScoped.clusterRoleName: platform-openshell-node-reader + asserts: + - equal: + path: metadata.name + value: platform-openshell-node-reader + + - it: creates the ClusterRole when legacy values omit the rbac block + set: + rbac: null + asserts: + - hasDocuments: + count: 1 + - equal: + path: metadata.name + value: openshell-node-reader-my-namespace diff --git a/deploy/helm/openshell/tests/clusterrolebinding_test.yaml b/deploy/helm/openshell/tests/clusterrolebinding_test.yaml index 5b9a83c04c..4cbf6d7d91 100644 --- a/deploy/helm/openshell/tests/clusterrolebinding_test.yaml +++ b/deploy/helm/openshell/tests/clusterrolebinding_test.yaml @@ -29,3 +29,48 @@ tests: kind: ServiceAccount name: openshell namespace: my-namespace + + - it: omits the ClusterRoleBinding when cluster-scoped RBAC is disabled + set: + rbac.clusterScoped.create: false + asserts: + - hasDocuments: + count: 0 + + - it: omits the ClusterRoleBinding when all chart-managed RBAC is disabled + set: + rbac.create: false + asserts: + - hasDocuments: + count: 0 + + - it: uses the configured ClusterRoleBinding and ClusterRole names + set: + rbac.clusterScoped.clusterRoleName: platform-openshell-node-reader + rbac.clusterScoped.clusterRoleBindingName: platform-openshell-node-reader-binding + asserts: + - equal: + path: metadata.name + value: platform-openshell-node-reader-binding + - equal: + path: roleRef.name + value: platform-openshell-node-reader + + - it: binds the custom gateway service account created by the namespaced release + set: + serviceAccount.create: false + serviceAccount.name: my-existing-sa + asserts: + - contains: + path: subjects + content: + kind: ServiceAccount + name: my-existing-sa + namespace: my-namespace + + - it: creates the ClusterRoleBinding when legacy values omit the rbac block + set: + rbac: null + asserts: + - hasDocuments: + count: 1 diff --git a/deploy/helm/openshell/tests/rbac_test.yaml b/deploy/helm/openshell/tests/rbac_test.yaml new file mode 100644 index 0000000000..3698465e27 --- /dev/null +++ b/deploy/helm/openshell/tests/rbac_test.yaml @@ -0,0 +1,95 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +suite: Gateway RBAC creation +templates: + - templates/role.yaml + - templates/rolebinding.yaml + - templates/peer-role.yaml +release: + name: openshell + namespace: my-namespace + +tests: + - it: creates the namespaced sandbox Role by default + template: templates/role.yaml + asserts: + - hasDocuments: + count: 1 + - equal: + path: metadata.name + value: openshell-sandbox + + - it: creates the namespaced sandbox RoleBinding by default + template: templates/rolebinding.yaml + asserts: + - hasDocuments: + count: 1 + - equal: + path: metadata.name + value: openshell-sandbox + + - it: creates the gateway peer Role and RoleBinding by default + template: templates/peer-role.yaml + asserts: + - hasDocuments: + count: 2 + - equal: + path: metadata.name + value: openshell-peer + + - it: keeps the namespaced RBAC when only cluster-scoped RBAC is disabled + set: + rbac.clusterScoped.create: false + asserts: + - hasDocuments: + count: 1 + template: templates/role.yaml + + - it: keeps the peer RBAC when only cluster-scoped RBAC is disabled + template: templates/peer-role.yaml + set: + rbac.clusterScoped.create: false + asserts: + - hasDocuments: + count: 2 + + - it: omits the namespaced sandbox Role when chart-managed RBAC is disabled + template: templates/role.yaml + set: + rbac.create: false + asserts: + - hasDocuments: + count: 0 + + - it: omits the namespaced sandbox RoleBinding when chart-managed RBAC is disabled + template: templates/rolebinding.yaml + set: + rbac.create: false + asserts: + - hasDocuments: + count: 0 + + - it: omits the gateway peer Role and RoleBinding when chart-managed RBAC is disabled + template: templates/peer-role.yaml + set: + rbac.create: false + asserts: + - hasDocuments: + count: 0 + + - it: creates the namespaced sandbox Role when legacy values omit the rbac block + template: templates/role.yaml + set: + rbac: null + asserts: + - hasDocuments: + count: 1 + + - it: creates the peer Role and RoleBinding when legacy values omit the rbac block + template: templates/peer-role.yaml + set: + rbac: null + asserts: + - hasDocuments: + count: 2 diff --git a/deploy/helm/openshell/values.yaml b/deploy/helm/openshell/values.yaml index 426f3ae424..4e7e11142d 100644 --- a/deploy/helm/openshell/values.yaml +++ b/deploy/helm/openshell/values.yaml @@ -141,6 +141,26 @@ sandboxServiceAccount: # -- Existing service account name for sandbox pods when sandboxServiceAccount.create is false. name: "" +# RBAC objects created for the gateway ServiceAccount. Cluster-scoped objects +# can be omitted so a cluster-admin applies them once and a namespace-admin +# installs and upgrades the gateway release without cluster-scoped permissions. +rbac: + # -- Create the RBAC objects that grant the gateway ServiceAccount access. + # Disable to supply the namespaced sandbox and peer Role/RoleBinding and the + # cluster-scoped ClusterRole/ClusterRoleBinding out of band. The certgen hook + # and credential driver RBAC keep their own flags. + create: true + clusterScoped: + # -- Create the cluster-scoped ClusterRole and ClusterRoleBinding. Disable + # for a namespace-admin install where a cluster-admin applies them + # separately; the gateway ServiceAccount name and namespace are unchanged, + # so a pre-created ClusterRoleBinding still matches. + create: true + # -- Name for the ClusterRole. Empty uses the `-node-reader-` default. + clusterRoleName: "" + # -- Name for the ClusterRoleBinding. Empty uses the `-node-reader-` default. + clusterRoleBindingName: "" + # Namespace-scoped resources needed to run sandboxes. Disable this when the # gateway and workspace prerequisites are managed as separate Helm releases # using the openshell-workspace chart. diff --git a/deploy/helm/test-split-ownership.sh b/deploy/helm/test-split-ownership.sh index 30fd7360d4..7285c61d6e 100755 --- a/deploy/helm/test-split-ownership.sh +++ b/deploy/helm/test-split-ownership.sh @@ -21,6 +21,32 @@ if yq ea -e \ exit 1 fi +helm template openshell "${repo_root}/deploy/helm/openshell" \ + --namespace openshell \ + --set agentSandbox.preflight.enabled=false \ + --set supervisor.sandboxRuntime.networkPolicyEnforced=true \ + --set rbac.clusterScoped.create=false \ + >"${work_dir}/namespace-admin.yaml" + +cluster_scoped_kinds="$( + yq ea -N -r \ + 'select(.kind == "ClusterRole" or .kind == "ClusterRoleBinding") | + [.kind, .metadata.name] | join(" ")' \ + "${work_dir}/namespace-admin.yaml" +)" +if [[ -n "${cluster_scoped_kinds}" ]]; then + echo "gateway chart rendered cluster-scoped objects despite rbac.clusterScoped.create=false:" >&2 + echo "${cluster_scoped_kinds}" >&2 + exit 1 +fi + +if ! yq ea -e \ + 'select(.kind == "ServiceAccount" and .metadata.name == "openshell")' \ + "${work_dir}/namespace-admin.yaml" >/dev/null 2>&1; then + echo "gateway chart dropped the gateway ServiceAccount with rbac.clusterScoped.create=false" >&2 + exit 1 +fi + helm template openshell-workspace "${repo_root}/deploy/helm/openshell-workspace" \ --namespace app-a \ --set gateway.serviceAccount.name=openshell \ diff --git a/docs/CONTRIBUTING.mdx b/docs/CONTRIBUTING.mdx index 383473918e..dec4f21e95 100644 --- a/docs/CONTRIBUTING.mdx +++ b/docs/CONTRIBUTING.mdx @@ -89,6 +89,8 @@ keywords: "Generative AI, Cybersecurity, AI Agents, Sandboxing" For explicit entries in `docs/index.yml`, keep `page:`. Fern still requires it. If the page defines `sidebar-title`, set `page:` to that value. Otherwise set `page:` to the frontmatter `title`. +Keep every page URL equal to its file path under `docs/`, without `.mdx`. Fern builds URLs from nav labels and slugs, not from file paths, so rename the file when you rename a page, and add a redirect in `fern/docs.yml` for the old URL. When a nav label does not produce the file name, for example `TypeScript`, which Fern turns into `type-script`, set a relative `slug:` on the entry in `docs/index.yml`. `mise run docs` runs `mise run docs:nav`, which fails when a page URL differs from its file path, when a nav label differs from the page's sidebar name, when a page is missing from the navigation, or when a redirect points to a page that does not exist. + ### Page Structure 1. Frontmatter `title` and `description`, plus any relevant page metadata. diff --git a/docs/about/run-an-agent.mdx b/docs/about/run-your-first-agent.mdx similarity index 100% rename from docs/about/run-an-agent.mdx rename to docs/about/run-your-first-agent.mdx diff --git a/docs/how-it-works/gateways/authentication.mdx b/docs/how-it-works/gateways/authentication.mdx index 7abb989276..3dbf6e524f 100644 --- a/docs/how-it-works/gateways/authentication.mdx +++ b/docs/how-it-works/gateways/authentication.mdx @@ -2,6 +2,7 @@ # SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 title: "Gateway Authentication" +sidebar-title: "Authentication" description: "Gateway resolution, authentication modes, connection flow, OIDC support, and credential file layout." keywords: "Generative AI, Cybersecurity, Gateway, Authentication, mTLS, OIDC, OpenID Connect, Edge Authentication, Reference" position: 1 diff --git a/docs/how-it-works/sandboxes/runtimes.mdx b/docs/how-it-works/sandboxes/runtimes.mdx index b36128aca5..cd3d576b52 100644 --- a/docs/how-it-works/sandboxes/runtimes.mdx +++ b/docs/how-it-works/sandboxes/runtimes.mdx @@ -176,6 +176,8 @@ The driver looks up sandbox images in local Docker or Podman before pulling from VM sandboxes have no network interface. All traffic flows through the OpenShell supervisor on the host. For networks that require a corporate proxy, the VM driver accepts the same `https_proxy` and `proxy_*` options as Podman. To reach a proxy on the gateway host, use `http://host.openshell.internal:`. +If the gateway is briefly unavailable while a VM starts, its supervisor retries the session connection. The sandbox stays in provisioning until the gateway accepts the session. + ## Kubernetes Driver The Kubernetes driver runs sandboxes as pods in a sandbox namespace. It requires the [Agent Sandbox](https://github.com/kubernetes-sigs/agent-sandbox) controller. Install the gateway with the Helm chart; see [Kubernetes setup](/kubernetes/setup). diff --git a/docs/index.yml b/docs/index.yml index defa2106e6..596ac87172 100644 --- a/docs/index.yml +++ b/docs/index.yml @@ -9,14 +9,14 @@ navigation: - section: "About NVIDIA OpenShell" slug: about contents: - - page: "Why OpenShell" + - page: "Overview" path: about/overview.mdx - page: "Architecture" path: about/architecture.mdx - page: "Installation" path: about/installation.mdx - page: "Run Your First Agent" - path: about/run-an-agent.mdx + path: about/run-your-first-agent.mdx - page: "Support Matrix" path: about/support-matrix.mdx - section: "How It Works" @@ -102,6 +102,7 @@ navigation: title: "Kubernetes" - section: "Tutorials" slug: tutorials + path: tutorials/index.mdx contents: - page: "Run Pi with OpenRouter" path: tutorials/run-pi-with-openrouter.mdx @@ -109,7 +110,7 @@ navigation: - page: "First Network Policy" path: tutorials/first-network-policy.mdx - page: "GitHub Push Access" - path: tutorials/github-sandbox.mdx + path: tutorials/github-push-access.mdx - page: "Microsoft Graph Provider Refresh" path: tutorials/microsoft-graph-provider-refresh.mdx - section: "SDK Reference" @@ -123,6 +124,7 @@ navigation: path: sdk/python.mdx - page: "TypeScript" path: sdk/typescript.mdx + slug: typescript - page: "API Errors" path: sdk/api-errors.mdx - page: "Protobuf Time Types" diff --git a/docs/kubernetes/setup.mdx b/docs/kubernetes/setup.mdx index b43ac2b073..b16065aa61 100644 --- a/docs/kubernetes/setup.mdx +++ b/docs/kubernetes/setup.mdx @@ -320,7 +320,10 @@ The chart creates the following RBAC resources in the release namespace: | ServiceAccount | Namespace | `openshell` | | ServiceAccount | Namespace | `openshell-sandbox` (for sandbox pods) | | Role + RoleBinding | Namespace | `openshell-sandbox` | -| ClusterRole + ClusterRoleBinding | Cluster | `openshell-node-reader` | +| ClusterRole + ClusterRoleBinding | Cluster | `openshell-node-reader-` | + +Every other object the chart creates is namespaced. The `ClusterRole` and +`ClusterRoleBinding` in the last row are the only cluster-scoped objects. When the Kubernetes Secrets credential driver is enabled, the chart also creates an `openshell-credential-secrets` Role and RoleBinding in the credential @@ -363,6 +366,118 @@ helm upgrade --install openshell \ The ServiceAccount must already have the Role and ClusterRole bindings described above. +### Installing without cluster-admin + +By default the release creates the `ClusterRole` and `ClusterRoleBinding`, so +the installer needs cluster-scoped permissions. When cluster-scoped RBAC is +owned by a different team, or the installer is a namespace-admin GitOps +controller, split the install into a cluster-admin step and a namespace-admin +step. + +A cluster-admin applies the cluster-scoped objects once per gateway +ServiceAccount. Render them from the same values the release uses, so the rules +match the configured workspace mode and credential driver: + +```shell +helm template openshell \ + oci://ghcr.io/nvidia/openshell/helm-chart \ + --version \ + --namespace openshell \ + -f my-values.yaml \ + --set rbac.create=true \ + --set rbac.clusterScoped.create=true \ + --set agentSandbox.preflight.enabled=false \ + --show-only templates/clusterrole.yaml \ + --show-only templates/clusterrolebinding.yaml | kubectl apply -f - +``` + +A namespace-admin then installs and upgrades the chart with cluster-scoped +objects omitted. The release renders only namespaced objects and needs no +permission on `clusterroles` or `clusterrolebindings`: + +```shell +helm upgrade --install openshell \ + oci://ghcr.io/nvidia/openshell/helm-chart \ + --version \ + --namespace openshell \ + -f my-values.yaml \ + --set rbac.clusterScoped.create=false +``` + +The gateway ServiceAccount name and namespace are unchanged by this flag, so +the pre-created `ClusterRoleBinding` still binds the ServiceAccount the +namespaced release creates. The same holds with `serviceAccount.create=false`: +the binding subject follows `serviceAccount.name`, so pass the same values to +both steps. + +#### Migrating an existing release + +Helm deletes objects that leave a release manifest, so setting +`rbac.clusterScoped.create=false` on a release that already owns the +`ClusterRole` and `ClusterRoleBinding` deletes them. The gateway then loses +TokenReview until a cluster-admin re-applies them. Hand ownership over first, as +cluster-admin, so nothing is deleted: + +```shell +kubectl annotate clusterrole "openshell-node-reader-" \ + helm.sh/resource-policy=keep --overwrite +kubectl annotate clusterrolebinding "openshell-node-reader-" \ + helm.sh/resource-policy=keep --overwrite +``` + +The objects then survive the upgrade that sets the flag, and the cluster-admin +owns them from that point on. Fresh installs need no such step. + +`rbac.clusterScoped.create` is independent of the workspace mode. `managed` and +`operator` modes change what the `ClusterRole` grants, but they never require a +namespace-admin to apply it. Re-run the cluster-admin step after changing any +value that affects the `ClusterRole` rules. + +Set `rbac.clusterScoped.clusterRoleName` and +`rbac.clusterScoped.clusterRoleBindingName` when the cluster-admin owns the +naming. + +#### Which flag the installer needs + +In the default `shared` workspace mode the release also creates a namespaced +sandbox `Role` granting Agent Sandbox (`agents.x-k8s.io`) permissions. +Kubernetes forbids granting permissions you do not hold, and the built-in +`admin` ClusterRole does not cover that CRD, so an installer holding only +`admin` cannot create it. `rbac.clusterScoped.create=false` alone is then not +enough, and the install fails with `attempting to grant RBAC permissions not +currently held`. + +| Workspace mode | Installer holds | Use | +|---|---|---| +| `shared` | built-in `admin` only | `rbac.create=false`, cluster-admin pre-creates all gateway RBAC | +| `shared` | `admin` plus the sandbox permissions in the namespace | `rbac.clusterScoped.create=false` | +| `managed`, `operator` | built-in `admin` only | `rbac.clusterScoped.create=false` | + +`managed` and `operator` render no namespaced sandbox `Role`, so no extra grant +is needed there. + +With `rbac.create=false`, the cluster-admin applies the namespaced RBAC in the +same step by adding it to the render: + +```shell +helm template openshell \ + oci://ghcr.io/nvidia/openshell/helm-chart \ + --version \ + --namespace openshell \ + -f my-values.yaml \ + --set rbac.create=true \ + --set rbac.clusterScoped.create=true \ + --set agentSandbox.preflight.enabled=false \ + --show-only templates/clusterrole.yaml \ + --show-only templates/clusterrolebinding.yaml \ + --show-only templates/role.yaml \ + --show-only templates/rolebinding.yaml \ + --show-only templates/peer-role.yaml | kubectl apply -f - +``` + +To grant the installer the sandbox permissions instead, bind it to a Role +carrying the same rules as the chart's `openshell-sandbox` Role. + ## Probes The gateway exposes `/healthz` for process liveness and `/readyz` for dependency-aware readiness on the health port. The Helm chart wires both into Kubernetes probes: diff --git a/docs/sdk/protobuf-time-types.mdx b/docs/sdk/protobuf-time-types.mdx index f658abf776..72de767ddd 100644 --- a/docs/sdk/protobuf-time-types.mdx +++ b/docs/sdk/protobuf-time-types.mdx @@ -1,5 +1,6 @@ --- title: Protobuf time types +sidebar-title: "Protobuf Time Types" description: Timestamp and duration representation in the OpenShell API --- diff --git a/docs/security/verifying-images.mdx b/docs/security/verify-image-contents.mdx similarity index 100% rename from docs/security/verifying-images.mdx rename to docs/security/verify-image-contents.mdx diff --git a/docs/tutorials/github-sandbox.mdx b/docs/tutorials/github-push-access.mdx similarity index 100% rename from docs/tutorials/github-sandbox.mdx rename to docs/tutorials/github-push-access.mdx diff --git a/e2e/rust/tests/vm_corporate_proxy.rs b/e2e/rust/tests/vm_corporate_proxy.rs index 1e34e71d0d..4b6efae5f7 100644 --- a/e2e/rust/tests/vm_corporate_proxy.rs +++ b/e2e/rust/tests/vm_corporate_proxy.rs @@ -30,13 +30,13 @@ use std::fmt::Write as _; use std::io::Write as _; +use std::net::{Ipv4Addr, SocketAddrV4, TcpListener}; use std::path::PathBuf; use std::time::Duration; use openshell_e2e::harness::cli::wait_for_healthy; use openshell_e2e::harness::gateway::ManagedGateway; use openshell_e2e::harness::host_process::HostPythonFixture; -use openshell_e2e::harness::port::find_free_port; use openshell_e2e::harness::sandbox::SandboxGuard; use serial_test::serial; use tempfile::NamedTempFile; @@ -63,11 +63,29 @@ struct FixturePorts { impl FixturePorts { fn pick() -> Self { + // Keep all four listeners bound while allocating ports. Calling the + // single-port helper repeatedly can return the same released port, + // making an allowed CONNECT look like one to the denied fixture. + let listeners: Vec<_> = (0..4) + .map(|_| { + TcpListener::bind(SocketAddrV4::new(Ipv4Addr::LOCALHOST, 0)) + .expect("reserve corporate proxy fixture port") + }) + .collect(); + let ports: Vec<_> = listeners + .iter() + .map(|listener| { + listener + .local_addr() + .expect("fixture listener address") + .port() + }) + .collect(); Self { - proxy: find_free_port(), - allowed: find_free_port(), - denied: find_free_port(), - bypass: find_free_port(), + proxy: ports[0], + allowed: ports[1], + denied: ports[2], + bypass: ports[3], } } } diff --git a/fern/docs.yml b/fern/docs.yml index 9066e2cc47..a36a9ac1e3 100644 --- a/fern/docs.yml +++ b/fern/docs.yml @@ -80,15 +80,28 @@ redirects: - source: "/openshell/dev/providers/google-vertex-ai" destination: "/openshell/dev/how-it-works/providers/google#vertex-ai" - source: "/openshell/latest/about/supported-agents" - destination: "/openshell/latest/about/run-an-agent" + destination: "/openshell/latest/about/run-your-first-agent" - source: "/openshell/dev/about/supported-agents" - destination: "/openshell/dev/about/run-an-agent" + destination: "/openshell/dev/about/run-your-first-agent" - source: "/openshell/latest/get-started/quickstart" - destination: "/openshell/latest/about/run-an-agent" + destination: "/openshell/latest/about/run-your-first-agent" - source: "/openshell/dev/get-started/quickstart" - destination: "/openshell/dev/about/run-an-agent" + destination: "/openshell/dev/about/run-your-first-agent" - source: "/openshell/latest/sandboxes/providers-v2" destination: "/openshell/latest/how-it-works/providers/profiles" + # Preserve published URLs when page names and slugs change. + - source: "/openshell/about/why-open-shell" + destination: "/openshell/latest/about/overview" + - source: "/openshell/sdk/type-script" + destination: "/openshell/latest/sdk/typescript" + - source: "/openshell/latest/about/why-open-shell" + destination: "/openshell/latest/about/overview" + - source: "/openshell/dev/about/why-open-shell" + destination: "/openshell/dev/about/overview" + - source: "/openshell/latest/sdk/type-script" + destination: "/openshell/latest/sdk/typescript" + - source: "/openshell/dev/sdk/type-script" + destination: "/openshell/dev/sdk/typescript" # Paths are relative to the site root; subpath prefix matches instances + custom-domain. # Legacy HTML URLs used .../path/to/page/index.html; Fern canonical URLs omit index.html. # List explicit /index.html routes before :path*/index.html so empty path segments do not diff --git a/tasks/docs.toml b/tasks/docs.toml index bc9534fa0d..b1cea2ee2a 100644 --- a/tasks/docs.toml +++ b/tasks/docs.toml @@ -5,7 +5,7 @@ ["docs"] description = "Validate Fern documentation" -depends = ["docs:build:strict"] +depends = ["docs:build:strict", "docs:nav"] ["docs:deps"] description = "Resolve Fern CLI for docs tasks" @@ -23,6 +23,10 @@ cd fern npx --yes "fern-api@${FERN_VERSION}" check """ +["docs:nav"] +description = "Check that docs page URLs match their file paths" +run = "uv run tasks/scripts/check_docs_nav.py" + ["docs:serve"] description = "Serve Fern docs locally" depends = ["docs:deps"] diff --git a/tasks/scripts/check_docs_nav.py b/tasks/scripts/check_docs_nav.py new file mode 100644 index 0000000000..f92dbcadd5 --- /dev/null +++ b/tasks/scripts/check_docs_nav.py @@ -0,0 +1,225 @@ +#!/usr/bin/env python3 +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +# /// script +# requires-python = ">=3.9" +# dependencies = [ +# "PyYAML==6.0.2", +# ] +# /// + +"""Check that the published docs navigation mirrors the docs folder structure. + +Fern does not build page URLs from file paths. It uses, in order, the page's +frontmatter `slug`, the nav entry's `slug`, or the nav label, and it splits +camel case in labels, so "TypeScript" becomes `type-script`. Pages in a +`folder:` section use their file names. A URL can therefore drift away from +where its file lives without any link breaking. + +This check fails when: + +- a page's URL differs from its file path under docs/, without `.mdx`, and + without `/index` for a section's own page; +- a nav page's sidebar name, its `sidebar-title` or else its `title`, differs + from its nav label. Fern shows the sidebar name but builds the URL from the + label, so the two must agree; +- a docs page is not reachable from the navigation; or +- a redirect for this docs version points to a URL that does not exist, or its + source is still a live page. +""" + +from __future__ import annotations + +import argparse +import re +import sys +from dataclasses import dataclass, field +from pathlib import Path + +import yaml + +# Files under docs/ that are intentionally not published as pages. +UNPUBLISHED = {"CONTRIBUTING.mdx"} + + +def fern_slug(label: str) -> str: + """Slugify a nav label the way Fern does, including camel-case splits.""" + split = re.sub(r"([a-z0-9])([A-Z])", r"\1-\2", label) + return re.sub(r"[^a-z0-9]+", "-", split.lower()).strip("-") + + +def path_url(rel_path: str) -> str: + """Return the URL that a docs file path implies.""" + url = "/" + re.sub(r"\.mdx$", "", rel_path) + return re.sub(r"/index$", "", url) + + +def frontmatter(path: Path) -> dict: + match = re.match(r"---\n(.*?)\n---", path.read_text(encoding="utf-8"), re.S) + if not match: + return {} + return yaml.safe_load(match.group(1)) or {} + + +@dataclass +class NavCheck: + docs_root: Path + urls: dict[str, str] = field(default_factory=dict) # URL -> docs-relative path + reachable: set[str] = field(default_factory=set) + issues: list[str] = field(default_factory=list) + + def page_url(self, rel_path: str, default_url: str) -> str: + slug = frontmatter(self.docs_root / rel_path).get("slug") + return "/" + str(slug).strip("/") if slug else default_url + + def record(self, rel_path: str, url: str, how: str) -> None: + self.reachable.add(rel_path) + if url in self.urls and self.urls[url] != rel_path: + self.issues.append( + f"{rel_path}: URL {url} is also used by {self.urls[url]}" + ) + self.urls[url] = rel_path + expected = path_url(rel_path) + if url != expected: + self.issues.append( + f"{rel_path}: URL {url} ({how}) does not match its file path " + f"{expected}. Rename the file, change the nav label, or set a " + "relative slug in docs/index.yml. For folder-discovered pages, " + f'use frontmatter slug: "{expected.lstrip("/")}".' + ) + + def walk(self, items: list, prefix: str) -> None: + for item in items or []: + if "section" in item: + url = prefix + "/" + item.get("slug", fern_slug(item["section"])) + if item.get("skip-slug"): + url = prefix + if "path" in item: + rel = item["path"] + self.record(rel, self.page_url(rel, url), "section page") + self.walk(item.get("contents", []), url) + elif "page" in item: + rel, label = item["path"], item["page"] + if not (self.docs_root / rel).exists(): + self.issues.append( + f"docs/index.yml: nav page {label!r} points to missing {rel}" + ) + continue + if item.get("skip-slug"): + default, how = prefix, "skip-slug" + elif "slug" in item: + default, how = prefix + "/" + item["slug"], "nav slug" + else: + default, how = ( + prefix + "/" + fern_slug(label), + f"nav label {label!r}", + ) + meta = frontmatter(self.docs_root / rel) + how = "frontmatter slug" if meta.get("slug") else how + self.record(rel, self.page_url(rel, default), how) + shown_key = "sidebar-title" if "sidebar-title" in meta else "title" + shown = meta.get(shown_key) + if shown != label: + self.issues.append( + f"{rel}: {shown_key} {shown!r} does not match nav label {label!r}" + ) + elif "folder" in item: + folder = item["folder"].strip("./").rstrip("/") + url = ( + prefix + + "/" + + item.get("slug", fern_slug(item.get("title", folder))) + ) + if item.get("skip-slug"): + url = prefix + for page in sorted((self.docs_root / folder).rglob("*.mdx")): + rel = page.relative_to(self.docs_root).as_posix() + stem = ( + page.relative_to(self.docs_root / folder) + .with_suffix("") + .as_posix() + ) + default = re.sub(r"/index$", "", url + "/" + stem) + how = ( + "frontmatter slug" + if frontmatter(page).get("slug") + else "folder file name" + ) + self.record(rel, self.page_url(rel, default), how) + + def check_orphans(self) -> None: + for page in sorted(self.docs_root.rglob("*.mdx")): + rel = page.relative_to(self.docs_root).as_posix() + if rel in UNPUBLISHED or rel in self.reachable or rel.startswith("_"): + continue + self.issues.append(f"{rel}: not reachable from docs/index.yml") + + def check_redirects(self, redirects: list, prefix: str) -> None: + live = set(self.urls) | {""} + for redirect in redirects or []: + source, dest = redirect.get("source", ""), redirect.get("destination", "") + if dest.startswith(prefix + "/") or dest == prefix: + target = dest[len(prefix) :].split("#", 1)[0].rstrip("/") + if ":" not in target and target not in live: + self.issues.append( + f"fern/docs.yml: redirect {source} -> {dest}: destination does not exist" + ) + if source.startswith(prefix + "/") and ":" not in source: + origin = source[len(prefix) :].rstrip("/") + if origin in live: + self.issues.append( + f"fern/docs.yml: redirect source {source} is still a live page" + ) + + +def redirect_prefix(fern_config: dict, nav_file: Path, config_dir: Path) -> str | None: + """Return the URL prefix, such as /openshell/dev, for the checked docs version.""" + instances = fern_config.get("instances") or [] + url = instances[0].get("url", "") if instances else "" + base = "/" + url.split("/", 1)[1].strip("/") if "/" in url else "" + for version in fern_config.get("versions") or []: + if (config_dir / version.get("path", "")).resolve() == nav_file.resolve(): + return f"{base}/{version['slug']}".replace("//", "/") + return None + + +def run(repo_root: Path) -> list[str]: + docs_root = repo_root / "docs" + nav_file = docs_root / "index.yml" + nav = yaml.safe_load(nav_file.read_text(encoding="utf-8")) + check = NavCheck(docs_root) + landing = (nav.get("landing-page") or {}).get("path") + if landing: + check.reachable.add(landing) + check.walk(nav.get("navigation", []), "") + check.check_orphans() + fern_file = repo_root / "fern" / "docs.yml" + if fern_file.exists(): + fern_config = yaml.safe_load(fern_file.read_text(encoding="utf-8")) + prefix = redirect_prefix(fern_config, nav_file, fern_file.parent) + if prefix: + check.check_redirects(fern_config.get("redirects"), prefix) + return check.issues + + +def main() -> int: + parser = argparse.ArgumentParser( + description="Check that the published docs navigation mirrors the docs folder structure." + ) + parser.add_argument( + "--repo-root", type=Path, default=Path(__file__).resolve().parents[2] + ) + args = parser.parse_args() + issues = run(args.repo_root) + for issue in issues: + print(f"docs nav: {issue}", file=sys.stderr) + if issues: + print(f"docs nav: {len(issues)} problem(s) found", file=sys.stderr) + return 1 + print("docs nav: every page URL matches its file path") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tasks/scripts/check_docs_nav_test.py b/tasks/scripts/check_docs_nav_test.py new file mode 100644 index 0000000000..80cd28b2fd --- /dev/null +++ b/tasks/scripts/check_docs_nav_test.py @@ -0,0 +1,242 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +from __future__ import annotations + +import textwrap +from typing import TYPE_CHECKING + +import check_docs_nav +import pytest + +if TYPE_CHECKING: + from pathlib import Path + +FERN_CONFIG = """\ +instances: + - url: example.docs.buildwithfern.com/openshell +versions: + - display-name: Dev + path: ../docs/index.yml + slug: dev +""" + + +def page(title: str, **extra: str) -> str: + lines = ( + ["---", f'title: "{title}"'] + + [f'{k}: "{v}"' for k, v in extra.items()] + + ["---", ""] + ) + return "\n".join(lines) + + +def build(tmp_path: Path, nav: str, pages: dict[str, str], redirects: str = "") -> Path: + docs = tmp_path / "docs" + for rel, content in pages.items(): + (docs / rel).parent.mkdir(parents=True, exist_ok=True) + (docs / rel).write_text(content) + (docs / "index.yml").write_text(textwrap.dedent(nav)) + (tmp_path / "fern").mkdir() + (tmp_path / "fern" / "docs.yml").write_text( + FERN_CONFIG + textwrap.dedent(redirects) + ) + return tmp_path + + +def test_fern_slug_splits_camel_case() -> None: + assert check_docs_nav.fern_slug("TypeScript") == "type-script" + assert check_docs_nav.fern_slug("Why OpenShell") == "why-open-shell" + assert check_docs_nav.fern_slug("API Errors") == "api-errors" + assert check_docs_nav.fern_slug("0.1.0") == "0-1-0" + + +def test_matching_nav_passes(tmp_path: Path) -> None: + root = build( + tmp_path, + """ + navigation: + - section: "Guides" + slug: guides + path: guides/index.mdx + contents: + - page: "Setup" + path: guides/setup.mdx + - page: "Policy Prover" + path: guides/prover.mdx + slug: prover + """, + { + "guides/index.mdx": page("Guides"), + "guides/setup.mdx": page("Set Up", **{"sidebar-title": "Setup"}), + "guides/prover.mdx": page("Policy Prover"), + }, + ) + assert check_docs_nav.run(root) == [] + + +def test_label_url_that_differs_from_file_path_fails(tmp_path: Path) -> None: + root = build( + tmp_path, + """ + navigation: + - section: "SDK" + slug: sdk + contents: + - page: "TypeScript" + path: sdk/typescript.mdx + """, + {"sdk/typescript.mdx": page("TypeScript")}, + ) + issues = check_docs_nav.run(root) + assert len(issues) == 1 + assert "URL /sdk/type-script" in issues[0] + assert "relative slug in docs/index.yml" in issues[0] + + +def test_frontmatter_slug_can_pin_the_url_to_the_file_path(tmp_path: Path) -> None: + root = build( + tmp_path, + """ + navigation: + - section: "SDK" + slug: sdk + contents: + - page: "TypeScript" + path: sdk/typescript.mdx + """, + {"sdk/typescript.mdx": page("TypeScript", slug="sdk/typescript")}, + ) + assert check_docs_nav.run(root) == [] + + +def test_sidebar_name_must_match_nav_label(tmp_path: Path) -> None: + root = build( + tmp_path, + """ + navigation: + - section: "About" + slug: about + contents: + - page: "Overview" + path: about/overview.mdx + """, + {"about/overview.mdx": page("Overview of the Product")}, + ) + issues = check_docs_nav.run(root) + assert issues == [ + "about/overview.mdx: title 'Overview of the Product' does not match nav label 'Overview'" + ] + + +@pytest.mark.parametrize( + ("nav", "rel"), + [ + ( + """ + navigation: + - section: Guides + slug: ignored + skip-slug: true + contents: + - page: Setup + path: setup.mdx + """, + "setup.mdx", + ), + ( + """ + navigation: + - section: Guides + contents: + - page: Setup + slug: ignored + skip-slug: true + path: guides/index.mdx + """, + "guides/index.mdx", + ), + ( + """ + navigation: + - section: Guides + contents: + - folder: guides + slug: ignored + skip-slug: true + """, + "guides/setup.mdx", + ), + ], +) +def test_skip_slug_omits_navigation_level(tmp_path: Path, nav: str, rel: str) -> None: + root = build(tmp_path, nav, {rel: page("Setup")}) + assert check_docs_nav.run(root) == [] + + +def test_skip_slug_detects_url_that_differs_from_file_path(tmp_path: Path) -> None: + root = build( + tmp_path, + """ + navigation: + - section: Guides + skip-slug: true + contents: + - page: Setup + path: guides/setup.mdx + """, + {"guides/setup.mdx": page("Setup")}, + ) + issues = check_docs_nav.run(root) + assert len(issues) == 1 + assert "URL /setup" in issues[0] + assert "does not match its file path /guides/setup" in issues[0] + + +def test_folder_pages_use_file_names_and_orphans_fail(tmp_path: Path) -> None: + root = build( + tmp_path, + """ + navigation: + - folder: kubernetes + title: "Kubernetes" + """, + { + "kubernetes/openshift.mdx": page("OpenShift"), + "stray.mdx": page("Stray"), + }, + ) + assert check_docs_nav.run(root) == ["stray.mdx: not reachable from docs/index.yml"] + + +@pytest.mark.parametrize( + ("redirect", "expected"), + [ + ( + ' - source: "/openshell/dev/old"\n destination: "/openshell/dev/missing"\n', + "destination does not exist", + ), + ( + ' - source: "/openshell/dev/guides/setup"\n destination: "/openshell/dev/guides/setup"\n', + "is still a live page", + ), + ], +) +def test_dev_redirects_must_point_to_live_pages( + tmp_path: Path, redirect: str, expected: str +) -> None: + root = build( + tmp_path, + """ + navigation: + - section: "Guides" + slug: guides + contents: + - page: "Setup" + path: guides/setup.mdx + """, + {"guides/setup.mdx": page("Setup")}, + "redirects:\n" + redirect, + ) + issues = check_docs_nav.run(root) + assert len(issues) == 1 and expected in issues[0] diff --git a/tasks/test.toml b/tasks/test.toml index 39dd070204..c45826f1db 100644 --- a/tasks/test.toml +++ b/tasks/test.toml @@ -20,6 +20,7 @@ depends = [ "test:qualification-summary", "test:codex-security-release-range", "test:docs-website", + "test:docs-nav", ] ["test:docs-website"] @@ -28,6 +29,10 @@ description = "Test the docs-website sync script" # dependencies and the script's runtime dependency, which live outside the project env. run = "uv run --no-project --with pytest --with pytest-asyncio --with pyyaml pytest tasks/scripts/sync_docs_website_test.py" +["test:docs-nav"] +description = "Test the docs navigation check" +run = "uv run --no-project --with pytest --with pytest-asyncio --with pyyaml pytest tasks/scripts/check_docs_nav_test.py" + ["test:sbom"] description = "Run SBOM tooling tests" run = "uv run --no-project --with pytest pytest -o \"python_files=*_test.py\" deploy/sbom/"