Repository navigation
test(conformance): port the credential gating e2e tests - #5318
matthewgrossman wants to merge 17 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Label |
|
🌿 Preview your docs: https://nvidia-preview-pr-5318.docs.buildwithfern.com/openshell |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Port the six credential gating scenarios from e2e/rust/tests/credential_gating.rs into a conformance suite. Each test imports its own provider profile and creates a provider under a run-unique name, creates sandboxes from the Python test image through the CLI, and reaches probe servers on the host through host.openshell.internal. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
The conformance suite now creates sandboxes from testkit::python_image(). Docker and Podman runners build openshell/e2e-python:dev also in e2e-conformance-only mode, and the Kubernetes and VM runners default OPENSHELL_TEST_PYTHON_IMAGE to the pullable uv Python image. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Add testkit::policy (a base policy from base-policy.yaml, allow(), and Endpoint settings), install_provider(), YamlFile, ExecOutput::success(), and PYTHON_BINARIES, so suites build policies and providers the same way. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Add host-server-support with HttpRecorder and WebSocketRecorder, servers on the test host that sandboxes reach through host.openshell.internal. Move the host-gateway feature test onto them and onto testkit's base policy. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Use testkit's policy helpers and install_provider and host-server-support's recorders, and assert on the requests that reached the host server instead of verdicts the sandbox clients print. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
8e5c8d7 to
566b39d
Compare
|
/ok to test 566b39d |
The external-driver lane renders server.hostGatewayIP, but only the in-process driver reads it from the gateway TOML. The standalone driver takes OPENSHELL_HOST_GATEWAY_IP, which the post-renderer never set, so sandbox policy DNS refused host.openshell.internal on that lane. Copy the chart's rendered value into the driver container when present. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
/ok to test 99bbed4 |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. cli — No explicit risk acceptance foundOPENSHELL_E2E_KUBERNETES_FEATURES=e2e,e2e-kubernetes mise run e2e:kubernetes previously excluded host-dependent credential tests but now runs them unconditionally through openshell-conformance.
Review details
Advisory review; publication gates are unchanged. |
Conformance has to run against any gateway, and the unpublished openshell/e2e-python:dev image only exists where a lane built or loaded it. Default to the pinned uv Python image, which any gateway can pull, and keep OPENSHELL_TEST_PYTHON_IMAGE as the override. Host lanes no longer build the workload image for conformance. The base test policy names no workload user, so drivers use a default non-root user on images without one, and host_gateway runs python3 from PATH. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
/ok to test 61504a3 |
- The WebSocket recorder reads frames until the connection ends, and the test waits for that before checking that no binary frame arrived. - The HTTP recorder keeps the request head and body apart. The REST tests compare the body exactly and check the secret appears nowhere, and failure messages redact it. - testkit errors keep the gateway's gRPC status code, and the admission test checks for FailedPrecondition. - The policy create request destructures the SDK spec, so a new field fails to compile instead of being dropped. - host_gateway no longer retries its connection. - Docs state that conformance requires sandboxes to reach the test machine as host.openshell.internal. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
/ok to test 3a31220 |
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Thanks for the port. The recorder-based assertions are a clear improvement on the old verdict strings, and keeping all six scenarios while moving them onto shared helpers is the right shape for the pilot.
I'd like two things changed before this merges, plus some smaller suggestions inline:
- Drop
61504a338(the default Python image change) and handle "conformance runs on any gateway" in a follow-up. - Give the WebSocket test a positive control, so the denial assertion can fail.
Remote-cluster behaviour (inline comment on TESTING.md) is fine as a tracked follow-up.
| /// The image for tests that run `python3` in a sandbox. It is public and | ||
| /// pinned by digest so any gateway can pull it, and has the Python standard | ||
| /// library only: no curl, git or openssl. | ||
| const DEFAULT_PYTHON_IMAGE: &str = "ghcr.io/astral-sh/uv:0.12.17-python3.12-trixie-slim@sha256:9a59bb7206905ccaae4f7dab222fbac47c125a21e5fc16f43f427cd6c940ade3"; |
There was a problem hiding this comment.
Requested change (introduced in 61504a338)
Could we drop this commit and do the "runs on any gateway" goal as a follow-up?
Before it, python_image() defaults to openshell/e2e-python:dev. The tmachine Docker, Podman and k3s installers load it, and the Docker and Podman runners build it, so no registry pull happens. After it, the default is the uv image from ghcr.io. Nothing pre-pulls it, so each host or node pulls it on its first sandbox, inside that test's timeout. Every lane then needs outbound registry access, and the default loses curl, git and openssl.
15fd553c8 limits the damage: Docker and Podman build the local image, but the Kubernetes and VM runners point the tests at the uv image, so those lanes still pull it. With this commit dropped, the tmachine lanes keep using the loaded image.
The "any gateway" goal is a good one, and the follow-up can pick the mechanism deliberately: pre-load the pinned image in every installer, publish our own test image, or add a capability check. Dropping the commit also restores run_as_user: sandbox in base-policy.yaml. Please check that nothing else depends on the Podman workaround, and keep the OPENSHELL_TEST_PYTHON_IMAGE override.
There was a problem hiding this comment.
ya this was one I went back and forth on, I think it's something worth striving for but I think it's fair if it doesn't work quite yet. I think a followup ticket to make sure that we can eventually do this would be nice, which I'll do
There was a problem hiding this comment.
Follow-up for the "runs on any gateway" goal is tracked in #5352, under #5281. It records why the public-image default was reverted here and lists the open choices: a fuller public image or our own published one, pre-loading it so tests don't pull inside their timeouts, and an up-front pull check.
This PR is now back to openshell/e2e-python:dev as the default, with the Kubernetes and VM runners passing the pinned uv image through OPENSHELL_TEST_PYTHON_IMAGE, and run_as_user: sandbox restored in the base policy.
| async fn profile_websocket_endpoint_denies_binary_frames(cx: &mut Context) -> testkit::Result { | ||
| let server = HttpRecorder::start("ok").await?; | ||
| let websocket = WebSocketRecorder::start().await?; | ||
| let provider = install_gating_provider(cx, Some((server.port, websocket.port))).await?; | ||
|
|
||
| let mut policy = policy::base(); | ||
| allow(&mut policy, websocket.port).websocket(Access::ReadWrite); | ||
| let sandbox = create_sandbox(cx, "ws", policy, &provider).await?; | ||
| run_python(&sandbox, WEBSOCKET_CLIENT, websocket.port).await?; | ||
| websocket | ||
| .wait_for_handshake() | ||
| .await | ||
| .context("upstream should receive the WebSocket handshake")?; | ||
| let binary_seen = websocket.wait_until_finished().await?; | ||
| testkit::ensure!( | ||
| !binary_seen, | ||
| "credentialed WebSocket binary frame reached upstream" |
There was a problem hiding this comment.
Requested change (introduced in 566b39d81)
This test passes whenever no binary frame reaches upstream, whatever the reason. The supervisor forwards binary frames by default and denies them only when deny_uninspected_credentials is set (l7/websocket.rs:866). A supervisor that dropped every frame, or a close before the frame was read, would pass here.
Could you add a control: the same client and endpoint in a sandbox without the provider attached, expecting wait_until_finished() to return true? That shows the recorder can see a binary frame and makes the denial meaningful. A small unit test in host-server that sends a binary frame directly to WebSocketRecorder would also guard read_frames.
There was a problem hiding this comment.
Added both. The test now first runs the same client and endpoint in a sandbox without the provider and requires wait_until_finished() to return true, then runs the credentialed case and requires false. host-server has unit tests that send frames straight to WebSocketRecorder: a binary frame after a 300-byte text frame (the 126 extended-length path) is seen, and text frames alone are not. They also check the accept key against the RFC 6455 sample.
| @@ -656,6 +657,8 @@ explains the isolation backend separation behind the capability-free runtime. | |||
| On a **remote** cluster, drop the `e2e-host-gateway` feature. Those tests rely | |||
There was a problem hiding this comment.
Non-blocking, fine as a follow-up
Before this PR, e2e:kubernetes with e2e,e2e-kubernetes excluded the host-dependent credential tests. With this PR, openshell-conformance runs them unconditionally, so a remote Kubernetes or OpenShift gateway fails with connection errors even when it meets the contract. The compatibility bot flagged the same thing.
This is acceptable for the pilot if it is tracked. Please link a follow-up issue and say here that these tests need the gateway to reach host.openshell.internal. The options for the follow-up are to preserve the host-gateway opt-in, move these tests to a feature suite, or add an up-front reachability probe that skips with a clear message instead of a 10s poll timeout.
There was a problem hiding this comment.
Tracked in #5409, under #5281, with the three options you listed (keep an opt-in, move the tests to a feature suite, or check reachability up front and skip with a clear message). TESTING.md and the conformance README now say these tests need sandboxes to reach the test machine as host.openshell.internal and link the issue.
| }; | ||
| // The supervisor checks Sec-WebSocket-Accept, so compute it properly. | ||
| // SHA-1 is what RFC 6455 specifies for it; it protects nothing here. | ||
| let mut hasher = Sha1::new(); |
There was a problem hiding this comment.
Suggestion (introduced in f0d528712; also applies to Cargo.toml:17)
sha1 and base64 are added only for the accept key. tungstenite::handshake::derive_accept_key does the same computation. It's public in 0.26.2, the version already in all the tests lockfiles, and it passes the RFC 6455 sample. Using it would drop both dependencies and avoid a new sha1 user that CRYPTO.md:116 doesn't list (only openshell-supervisor-network is). The tests workspaces aren't scanned by cargo deny, so CI won't flag it either way.
Separately, at about 300 lines with two independent servers, splitting into http.rs and websocket.rs would keep the handshake code in one file. Optional.
There was a problem hiding this comment.
Done: the accept key now comes from tungstenite::handshake::derive_accept_key, and sha1 and base64 are gone from host-server. tungstenite was already in the test workspaces' lockfiles. host-server is split into http.rs and websocket.rs, with the shared bind and header helpers in lib.rs. The recorder unit tests still check the accept value against the RFC 6455 sample.
| # server.hostGatewayIP renders the gateway pod's only hostAliases entry | ||
| # (deploy/helm/openshell/templates/_gateway-workload.tpl), which the | ||
| # host-gateway component copies from hostAliases.0.ip. | ||
| if grep -q '^ *hostAliases:' "${work_dir}/rendered.yaml"; then |
There was a problem hiding this comment.
Suggestion (introduced in 99bbed46c)
The hostAliases: grep matches any workload, and when it misses, sandboxes fail later on name resolution (the symptom this fixes). Two small changes: match the - host.openshell.internal line, and fail when the lane set an IP but no alias rendered. The second needs the lane to export HOST_GATEWAY_IP, which is a plain shell variable in with-kube-gateway.sh today. I checked both against helm template output with and without the IP, and with an unrelated workload that has hostAliases.
There was a problem hiding this comment.
Done: the post-renderer now matches the - host.openshell.internal line instead of any hostAliases:. The kube gateway script exports OPENSHELL_E2E_EXPECTED_HOST_GATEWAY_IP for the external-driver lane, and the renderer fails with a clear error if that is set but no alias rendered. Checked with helm template: IP set copies it into the driver env, no IP adds nothing, and a chart with the alias renamed fails.
| crate::ensure!( | ||
| profile | ||
| .lines() | ||
| .any(|line| line.trim_end() == format!("id: {name}")), |
There was a problem hiding this comment.
Suggestion (introduced in 7a70c1283)
The id: {name} check is a line match, so id: "x" or a different layout fails or passes wrongly. Parsing the profile would be sturdier. Also, tying the provider name to the profile id rules out several providers from one profile and the built-in provider types. Splitting this into import_profile and create_provider, with install_provider built from them, would cover both.
There was a problem hiding this comment.
Done: testkit::import_profile(cx, profile) parses the YAML for id (with noyalib, the workspace's YAML crate, already in testkit's lockfile) and returns it, and testkit::create_provider(cx, name, type, credentials) takes the name and type separately. Each registers its own cleanup. install_provider is now built from the two, so the provider name no longer has to match the profile id.
| /// Let `binaries` connect to `host:port` under the network policy `name`, | ||
| /// adding the policy if it doesn't exist. Returns the new endpoint, which | ||
| /// [`Endpoint`] configures further. | ||
| pub fn allow<'a>( |
There was a problem hiding this comment.
Suggestion, follow-up (introduced in 7a70c1283; also applies to the Endpoint trait at line 75)
Two notes for the next policy tests. allow() merges binaries across a rule name and doesn't dedupe endpoints, so a second call widens the first endpoint's binaries. Worth documenting or scoping. Endpoint also lacks enforcement, L7 rules and allowed_ips; keycloak-provider-refresh already uses enforcement: enforce and still hand-rolls its tempfile instead of YamlFile. The trait shape is fine for adding these.
There was a problem hiding this comment.
Documented on policy::allow() and in the testkit README: binaries apply to every endpoint under a rule name, so use a different name to keep them apart. I left Endpoint as is and will add fields when a ported test needs them.
| /// `..`, so a field added to the SDK's spec fails to compile here instead of | ||
| /// being dropped. [`SandboxBuilder::create`] rejects a GPU or service | ||
| /// exposures before calling this. | ||
| fn policy_create_request( |
There was a problem hiding this comment.
Note, follow-up (introduced in 3a3122064)
policy_create_request rebuilds the SDK's create request because the SDK spec has no policy field. The exhaustive destructure is a good guard. The better fix is a policy field on the SDK builder, which would also lift the GPU and service-exposure restriction.
| /// The paths `python3` runs from in [`python_image`] images, as network | ||
| /// policy binaries: the system Python, the pullable uv image's Python, and the | ||
| /// workload image's uv-installed Python behind its virtualenv. | ||
| pub const PYTHON_BINARIES: &[&str] = &[ |
There was a problem hiding this comment.
Note (introduced in 7a70c1283)
PYTHON_BINARIES is valid for the known images but separate from python_image(). With OPENSHELL_TEST_PYTHON_IMAGE set to an image with a different layout, policy rules silently don't match. Either return the binaries with the image or document the override contract.
There was a problem hiding this comment.
Documented on OPENSHELL_TEST_PYTHON_IMAGE in image.rs and in the testkit README: an override image must run python3 from one of PYTHON_BINARIES, or policies won't match it. The PYTHON_BINARIES comment says to add a path for an image that installs Python elsewhere.
…mage The tmachine installers load it and the Docker and Podman lanes build it, so those lanes don't pull an image inside a test's timeout or lose curl, git and openssl. The Kubernetes and VM lanes keep using the pinned public image through OPENSHELL_TEST_PYTHON_IMAGE, and the base test policy names the sandbox user again. Running conformance against any gateway without an override is left to a follow-up. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
/ok to test 9ab1be1 |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. cli — Risk explicitly accepted
Review details
Advisory review; publication gates are unchanged. |
The test passed whenever no binary frame reached upstream, whatever the reason. It now first sends the same frame from a sandbox without the provider and requires that it arrives, so the denial check can fail. Unit tests check that WebSocketRecorder sees a binary frame after a long text frame, and none among text frames. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
/ok to test 4ff621b |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
/ok to test e5e72bf |
Use tungstenite's derive_accept_key instead of hashing with sha1 and base64 directly, and split host-server into http.rs and websocket.rs. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
… is missing The post-renderer copied the host gateway IP whenever the chart rendered any hostAliases. It now matches the host.openshell.internal entry, and fails if the lane set server.hostGatewayIP but no such entry rendered. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
…r create import_profile reads the profile's id from its YAML and returns it, and create_provider takes the provider name and type separately, so the provider name no longer has to match the profile id. install_provider does both. Document that policy::allow merges binaries under one name and that an override Python image must use a PYTHON_BINARIES path. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
/ok to test e22087f |
…gating-pilot/matthewgrossman Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
/ok to test aeec397 |
Summary
Port the six credential gating e2e tests from #5264 into the conformance family, and remove the old
e2e/rusttest. This is the pilot port for #5296. The pieces the port needed that other tests will need too are shared: policy helpers and provider install in testkit, and a newtests/support/host-servercrate for servers on the test host. The existinghost-gatewayfeature test now uses them as well.Related Issue
Closes #5317
Part of #5281. Pilot for #5296, which stays open. Ports the tests from #5264.
What the tests check, and what they needed
The #5264 tests check what the sandbox supervisor does with a provider credential when the sandbox sends traffic out:
tls: skip), unless the endpoint opts in.request_body_credential_rewriteis on.policy setadds its environment variable to a running sandbox, and unbinding removes it.Porting them needed four things. Each was added as a generic helper, and
host-gateway, an existing feature test that had its own copies, now uses the shared ones.tests/support/host-server(host-server-support):HttpRecorder::start(body)answers every request with200andbody.wait_for_requests(n)returns the requests, head and body apart, oncenhave arrived. It readsContent-Lengthbodies, not chunked ones.WebSocketRecorder::start()completes the WebSocket handshake. It then reads frames until a close frame, the end of the connection, or a 10s timeout.wait_for_handshake()waits for the handshake, andwait_until_finished()waits for the connection to end and reports whether a binary frame arrived.host.openshell.internal, and stop when dropped. It lives intests/support/, alongside the Keycloak fixture, because it's a service outside OpenShell that any suite can use.host-gatewayreplaces its hand-written server withHttpRecorder.protocol: restorwebsocket,tls: skip, body rewriting,credential_binding,allow_uninspected_credentials). Before this PR, tests wrote out a whole policy as a YAML string. New testkit moduletestkit::policy:base()readsbase-policy.yaml: the filesystem and process settings every sandbox needs, and no network access.allow(&mut policy, name, host, port, binaries)adds an endpoint. ItsEndpointsetters are named after the YAML fields, e.g..rest(Access::Full).credential_binding(&provider).to_yaml()renders the policy for CLI commands such aspolicy set.PYTHON_BINARIESnames the pathspython3runs from intestkit::python_image()images, which differ between the workload image and the pullable uv image.cx.sandbox(..).policy(..)rejected both, and these tests need all three.testkit::import_profile(cx, profile_yaml)imports a profile and returns itsid, read from the YAML, andtestkit::create_provider(cx, name, type, credentials)creates a provider. Each registers its cleanup before creating.testkit::install_provider(cx, name, profile_yaml, credentials)does both.testkit::YamlFilewrites YAML to a temporary file for CLI commands that read one (profile import,policy set);host-gatewayreplaces itsPolicyFilecopy with it.ExecOutput::success()fails a test on a non-zeroexecexit, likeCliOutput::success().Helpers for OpenShell itself (sandboxes, policies, providers, the CLI) go in testkit. Fixtures for services outside OpenShell go in
tests/support/.Changes
credential_gating, one file with the same six scenarios and unique names as test(e2e): split credential_gating into independent parallel tests #5264, built on the helpers above.create()error: the gRPC code must beFailedPrecondition, and the message keeps test(e2e): split credential_gating into independent parallel tests #5264's wording checks. testkit'sErrornow keeps the gateway's status code (grpc_code(),testkit::GrpcCode). The CLI is used forprofile import,provider createandpolicy set --wait.testkit::python_image()still defaults toopenshell/e2e-python:dev. The Docker and Podman runners now build it for conformance, as the tmachine installers already load it. The Kubernetes and VM runners setOPENSHELL_TEST_PYTHON_IMAGEto a pinned public uv Python image unless it is already set. A gateway outside these lanes has to setOPENSHELL_TEST_PYTHON_IMAGE; running conformance against any gateway without an override is a follow-up.host_gatewayrunspython3fromPATHand allowsPYTHON_BINARIES, so it works with either image, and no longer retries its connection for 20s: the credential tests never did, and the server listens before the sandbox is created.server.hostGatewayIPinto the standalone driver'sOPENSHELL_HOST_GATEWAY_IP. Before, only the in-process driver got it, so sandboxes on that lane couldn't resolvehost.openshell.internal. The lane hadn't run a host-reachability test before. The renderer fails if the lane set the IP but the chart rendered nohost.openshell.internalalias.e2e/rust/tests/credential_gating.rs, its[[test]]entry, and itsPODMAN_CI_TESTSentry.TESTING.mdlist the suite and state that conformance requires the machine running it to be reachable from sandboxes ashost.openshell.internal.Gaps found by the pilot (for #5296)
cx.sandbox(suffix). testkit doesn't check this before sending the request.e2e/rust/e2e-*.shscripts, not tmachine.Testing
cargo fmtandcargo clippy --locked --all-targets -D warningson testkit, support, features and conformance.testkit unit tests: 43 passed.
check_cargo_lockfile_alignment.pyandcheck-conformance-program.sh(22 tests) pass.Local Podman (macOS,
e2e/with-podman-gateway.sh):openshell-conformance run credential_gatingpassed 6/6 in 33.7s.host_gatewaypassed.policy_binding_requires_inspection_or_opt_inpolicy_binding_rest_body_keeps_placeholder_without_rewritepolicy_binding_rest_body_rewrite_sends_secret_upstreampolicy_binding_updates_provider_environment_liveprofile_rest_endpoint_keeps_body_placeholderprofile_websocket_endpoint_denies_binary_framesNot run locally: Docker (Docker Desktop requires sign-in), k3s, k3s-ha. CI covers these rows.
CI: copy-pr-bot doesn't sync draft PRs, so E2E runs only after a maintainer comments
/ok to test <sha>. In #5264, the WebSocket test took 60–120s on Kubernetes. If k3s or k3s-ha hit the per-test timeout, I'll raise it here rather than change.config/nextest.toml(see #5295).Checklist