[gmenher]

https://redhat.atlassian.net/browse/RHOAIENG-83116

Replaces #40 (RHOAIENG-83121) and #41 (RHOAIENG-83122). Phase 3 is RHOAIENG-83123. Phase 4 cleanup is a follow-up: RHOAIENG-83124.

Description

This PR moves the dashboard BFF off the hand-rolled gRPC wrappers (backend/internal/gateway/ + backend/gen/) and onto the official NVIDIA OpenShell Go SDK (github.com/NVIDIA/OpenShell/sdk/go). Every HTTP handler that talks to OpenShell now goes through app.sdk sub-clients. The frontend JSON contract is unchanged.

I originally split this into stacked PRs (sandbox → providers → the rest). After talking it through with Roland, mixed gRPC/SDK in the same binary is not worth the review cost: you still pay the proto registration conflict, you still ship both clients, and reviewers cannot tell if a bug is “SDK” or “the leftover wrapper”. So this is one PR covering what was Phase 1 + 2 + 3. Phase 4 (delete backend/gen/, stop dialing gateway.New(), drop GOLANG_PROTOBUF_REGISTRATION_CONFLICT) stays separate so we can land the handler cut without also deleting the generated stubs in the same diff.

The BFF was wrapping 57 RPCs through a private client and ~21K lines of generated stubs. The SDK (merged upstream in NVIDIA/OpenShell#2702) is the supported surface: typed sub-clients, domain types, and error helpers maintained by the OpenShell team. Roland’s old intermediate PR (review/sdk-migration / github.com/rhuss/openshell-sdk-go) is the design reference, not the dependency.

What changed

Foundation

• backend/go.mod: github.com/NVIDIA/OpenShell/sdk/go v0.0.0-20260814001211-c4b500a7de64 (upstream, post-#2702 — goroutine leak fixes in InteractiveSession).
• backend/internal/sdkclient/auth.go: ContextAuthProvider implements grpc.PerRPCCredentials by reading the bearer from request context on every RPC. RequireTLS follows the gateway URL scheme.
• backend/cmd/server/main.go: builds the SDK client next to the old gateway client. grpcs:// / https:// → TLS (optional GATEWAY_CA_CERT). Plaintext → TLSConfig{Insecure: true}. scripts/dev-env.sh now emits grpcs://localhost:${GATEWAY_GRPC_PORT}.
• backend/internal/api/respond.go: writeSDKError() maps SDK typed errors (IsNotFound, IsAlreadyExists, …) onto the existing HTTP codes, with a fallback to raw gRPC status for FailedPrecondition / ResourceExhausted. Message comes from StatusError.Message so we do not leak the error-chain prefix.
• backend/internal/api/app.go: App still has both gateway and sdk (hybrid until Phase 4). Handlers only call sdk.

Phase 1 — sandboxes

• sandboxes_handler.go: Create / Get / List / Delete via app.sdk.Sandboxes(). Annotations go through CreateOptions (they were silently dropped on the old path).

Phase 2 — providers + attach

• providers_handler.go: all 15 methods on Providers(), Providers().Profiles(), Providers().Refresh().
• logs_handler.go: ListSandboxProviders / Attach / Detach via Sandboxes().ListProviders/AttachProvider/DetachProvider. Logs themselves also moved in Phase 3.
• Deleted gateway/providers.go (209 lines) and the provider wrappers in gateway/logs.go.

Phase 3 — everything else

• Workspaces + members
• Policy (sandbox + global) and drafts (get / approve / reject / approve-all / edit / undo / clear / history)
• Settings, services, inference
• Gateway info, healthz, readyz, whoami
• Terminal (Exec().Interactive → WebSocket relay)
• Files (upload / download — see SDK gap below)

Converters + tests

• models/sdk_converters.go: SDK domain types → existing HTTP DTOs. We map into the JSON the frontend already speaks (camelCase, UPPER_SNAKE categories), not the SDK’s TitleCase.
• Handler tests rewritten against mockSDK (newTestAppWithSDK). writeSDKError has its own table tests.

Design decisions

1. One PR, not four stacked ones

Stacked PRs looked cleaner on paper and matched the Jira tickets. In practice the hybrid binary is the painful part, and splitting it across #40 / #41 / a Phase 3 PR means three reviews of the same wiring. Roland’s guidance was to do this in one or two shots, agent-reviewable. This PR is that shot. The Jira tickets still map 1:1 onto the commits if anyone wants to follow the original phases.

2. Hybrid until Phase 4 — do not delete backend/gen/ here

main.go still dials gateway.New() and assigns App.gateway. Nothing in the HTTP handlers reads it. I left it because several files under internal/models and internal/gateway still import the generated stubs, and deleting gen/ in the same PR would mix “did the SDK mapping work?” with “did we miss a proto import?”. CI needs GOLANG_PROTOBUF_REGISTRATION_CONFLICT=warn until Phase 4 because both backend/gen/ and the SDK register the same proto packages.

3. Upstream SDK, not Roland’s fork

github.com/NVIDIA/OpenShell/sdk/go, not github.com/rhuss/openshell-sdk-go. Upstream has the InteractiveSession close/leak fixes. Roland’s branch was useful as a map of which handler goes to which sub-client (app.client there → app.sdk here).

4. Preserve the HTTP JSON contract

The frontend is not part of this change. Converters (FromSDKSandbox, FromSDKProvider, FromSDKDraftPolicy, …) keep:

• camelCase field names
• profile categories as UPPER_SNAKE (INFERENCE, AGENT, …)
• credential names only, never values
• credentialNames as the union of Credentials and CredentialHandles (sorted). Handles-only providers were coming back with an empty list after the first SDK cut; that is fixed.

We do not invent RPCs. If the OpenShell API does not expose it, the BFF does not fake it. GetDraftSummary is still the pre-existing empty stub (no cross-workspace draft RPC).

5. Delete semantics follow the SDK, with two exceptions

Old wrappers returned (bool, error) for several deletes. The SDK returns error only. On success we now always return {"deleted": true} / {"removed": true}; a missing resource is 404. That is a small, intentional contract change.

DeleteProviderProfile and DeleteProviderRefresh still forward the SDK’s deleted bool, because those RPCs actually return it.

6. File transfer — SDK Files() is a stub in this build

defaultSSHTransport.available() is hardcoded false in the SDK we pin. Official docs: the standalone SDK does not currently ship an SSH file-transfer transport. Files().Upload / Download always return ErrTransportNotAvailable.

Upload therefore pipes bytes into Exec().Interactive with dd of=<path> bs=4096 (same idea as the old ExecSandbox path). Non-zero dd exit is HTTP 502 upload_failed (not 200 + success: false). Download is Exec().Run(["cat", path]); non-zero exit is 404. Path validation and a 64MiB MaxBytesReader are unchanged. This is a workaround we should drop when the SDK ships a real transport.

7. Policy history — Policy.List drops the sandbox name

The proto has ListSandboxPoliciesRequest.Name. The SDK Policy.List method does not send it, so a list call is not scoped to the sandbox. GetSandboxPolicy therefore:

• GetStatus for the latest revision
• older versions via GetStatus(WithVersion(v)) in parallel
• missing revisions are skipped with a slog.Warn (not a 500)

That is reconstructing history the SDK should have given us. Worth an upstream issue if it is still true on a newer SDK drop.

8. Error mapping lives in one place

Migrated handlers use writeSDKError. writeGrpcError stays until Phase 4 because the leftover gateway client / tests still use it. Status mapping:

SDK / gRPC	HTTP
NotFound	404
AlreadyExists	409
InvalidArgument / FailedPrecondition	400
PermissionDenied	403
Unauthenticated	401
Unavailable	502
ResourceExhausted	429
Conflict	409
unknown	500

9. Known JSON gaps vs the old proto converters (not regressions we paper over)

These are SDK type gaps, called out so reviewers do not think the converters “forgot” fields:

• Logs: FromSDKSandboxLogs does not set sandboxId (SDK LogLine has no id; JSON omitempty).
• WhoAmI: SDK CurrentUser has no email; we do not invent one.

10. Token relay is unchanged (ADR 0014)

The BFF still does not run OIDC. It forwards the bearer (or none, when AUTH_DISABLED=true). The SDK’s ContextAuthProvider is the same relay, just on the SDK dial instead of the hand-rolled client.

Dependencies

• SDK: NVIDIA/OpenShell#2702 (merged), foundation #2271
• Plan: GitHub Issue #35
• Epic: RHOAIENG-83116
• This PR replaces feat(bff): migrate sandbox handler to OpenShell Go SDK [RHOAIENG-83121] #40 and feat(bff): migrate provider handlers to OpenShell Go SDK [RHOAIENG-83122] #41 — please review here, not there

Out of scope (Phase 4)

• Delete backend/gen/ and backend/proto/
• Stop constructing gateway.New() / remove App.gateway
• Delete writeGrpcError and mock_gateway_test.go
• Remove proto converters that still import gen/
• Drop GOLANG_PROTOBUF_REGISTRATION_CONFLICT=warn

Go SDK e2e against NVIDIA/OpenShell (e2e/go/) is jpetrlik’s track, not this repo.

How Has This Been Tested?

1. Unit tests

cd backend
GOLANG_PROTOBUF_REGISTRATION_CONFLICT=warn go test ./internal/api ./internal/models

Coverage by surface:

• Sandboxes: list / create (validation + annotations) / get / delete
• Providers: CRUD, refresh configure/status/rotate/delete, profiles import/get/update/lint/delete, attach/detach with expectedResourceVersion
• Workspaces + members
• Policy: latest + gapped history (GetStatus(WithVersion) skips missing revisions)
• Drafts: get / approve / reject / approve-all / edit / undo / clear / history / empty summary stub
• Files: path validation, upload builds dd of=… bs=4096, failed dd → 502, Interactive error → 500, download 200 / 404
• Terminal: Interactive mock, stdout relay, stdin, resize 120×40
• Logs, settings, services, inference, gateway health/readyz/whoami
• writeSDKError table for the status mapping above
• Converters: sandbox spec (GPU/CPU/memory), provider credentialNames union, profile categories, nil sandbox

2. Live smoke (Phase 1 + 2, personal ROSA gateway)

Same setup as #40 / #41:

oc port-forward svc/openshell -n openshell 50051:8080
GOLANG_PROTOBUF_REGISTRATION_CONFLICT=warn AUTH_DISABLED=true OPENSHELL_GATEWAY_URL=localhost:50051 go run ./cmd/server

Sandboxes (UI, localhost:3000)

Operation	Result
List	200 — existing sandbox prueba
Create prueba-2	201
Delete prueba-2	200
Invalid names (space / underscore)	400 invalid_name

Providers (UI)

Operation	Result
List / create Codex / get / delete	200/201 as expected; credential values never in JSON
List profiles	builtin set with UPPER_SNAKE category badges
Attach/detach on prueba	exercised, then detached

AUTH_DISABLED=true still hits the real SDK dial, converters, and error mapping. The gateway validates the RPCs either way.

3. Phase 3 live (files / terminal / drafts)

Unit-tested as above. I started a local dev-env.sh gateway to go further and hit two environment issues that are not BFF bugs:

• Dialing grpcs://localhost:17670 lands on the compute-driver callback listener (127.0.0.1 is covered by the 0.0.0.0 primary bind and scoped to sandbox-callback RPCs only). The BFF has to dial a non-loopback address that is still the primary listener (LAN IP + cert SAN), or the SDK correctly gets PermissionDenied.
• A generic python:3.12-slim sandbox dies with OCI USER is required because run_as_user is omitted. Community image ghcr.io/nvidia/openshell-community/sandboxes/base:latest is the right target; I did not wait out a full pull + files/terminal pass for this PR.

So: handler + mock coverage for files/terminal/drafts is in the diff. End-to-end against a live sandbox for those three is still a manual pass (Dan’s review / a gateway that is already up). Happy to run it on the ROSA forward if we want that before merge.

4. Why this testing approach

Full browser OIDC would only test the proxy in front of the BFF. The migration risk is SDK mapping, error codes, and the two SDK gaps (files transport, policy list). Unit tests pin those. Phase 1/2 live smoke already proved TLS + token relay + CRUD against a real gateway. Phase 4 should not merge until gen/ is gone and the dual-registration env var can disappear.

[Gkrumbach07]

Testing

Rebased onto latest main (picks up the mTLS PR #42; conflict resolved by re-wiring client cert/key into the SDK + raw-exec TLS config). Verified the branch end-to-end:

Automated

• go build ./..., go test -race ./..., go vet ./..., golangci-lint run — all clean.
• CI green, including check-backend and e2e-integration now running without the GOLANG_PROTOBUF_REGISTRATION_CONFLICT=warn mask (removed in this branch) — so the startup-panic fix is genuinely exercised, not hidden.

Manual, live against a local gateway (v0.0.85, podman, plaintext dev)

• Startup panic: built the server binary and ran it with no special env — boots and serves /healthz (previously it panicked on proto: file "options.proto" is already registered because the old backend/gen stubs and the SDK both registered the same descriptors; a prod container had no env-var mask and would crash-loop).
• Policy fidelity: created a sandbox through the BFF with a policy carrying rules (L7 allow), denyRules (L7 deny), allowedIps, multi-port ports, and binaries, then GET it back through the real gateway — every field survived (the pre-fix hand-rolled converter dropped all of these on read and write). Confirms both ParseSDKPolicy (write) and marshalSDKPolicy (read).
• Binary upload: the upload path now issues the non-TTY ExecSandbox RPC (confirmed in gateway logs — zero ExecSandboxInteractive/PTY calls), with errors mapped cleanly (400, no panic/leak). Byte-fidelity (Tty=false + exact stdin bytes incl. control bytes) is covered by a bufconn unit test against a real gRPC server, since local sandboxes can't reach READY here due to a rootless-podman netns permission limitation (pre-existing, unrelated to this change).
• mTLS: --gateway-client-cert/--gateway-client-key are honored by both the SDK client and the upload client.

🤖 Generated with Claude Code