Skip to content

Commit 0a567a1

Browse files
committed
refactor: deduplicate repeated patterns across crates
Remove ~280 lines of duplicated code across 30 files in 5 areas: - centered_rect: consolidate 5 identical TUI layout helpers into a single pub fn in openshell-tui/src/ui/mod.rs - server test helpers: replace ~100 inline Store::connect() calls with local test_store() helpers; deduplicate test_server_state() in grpc/service.rs to use the shared test_support version - rogue PKI: extract 20-line rogue CA+client cert generation block (duplicated in two integration tests) into generate_rogue_pki() in tests/common/mod.rs - provider tests: replace 8 identical 28-line test modules with a single macro_rules! test_discovers_env_credential! invocation - label constants: centralize openshell.ai/ container label keys in openshell-core::driver_utils; update Docker and Kubernetes drivers to import from there instead of redefining them locally
1 parent e3f009f commit 0a567a1

30 files changed

Lines changed: 360 additions & 642 deletions

File tree

crates/openshell-core/src/driver_utils.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,28 @@
55
66
use crate::proto::compute::v1::DriverSandbox;
77

8+
// ---------------------------------------------------------------------------
9+
// Sandbox container/pod label keys (openshell.ai/ namespace)
10+
// ---------------------------------------------------------------------------
11+
12+
/// Container/pod label that identifies this resource as managed by `OpenShell`.
13+
/// Value should be `"openshell"`.
14+
pub const LABEL_MANAGED_BY: &str = "openshell.ai/managed-by";
15+
16+
/// Expected value for [`LABEL_MANAGED_BY`].
17+
pub const LABEL_MANAGED_BY_VALUE: &str = "openshell";
18+
19+
/// Container/pod label carrying the sandbox ID.
20+
pub const LABEL_SANDBOX_ID: &str = "openshell.ai/sandbox-id";
21+
22+
/// Container/pod label carrying the sandbox name.
23+
pub const LABEL_SANDBOX_NAME: &str = "openshell.ai/sandbox-name";
24+
25+
/// Container/pod label carrying the sandbox namespace.
26+
pub const LABEL_SANDBOX_NAMESPACE: &str = "openshell.ai/sandbox-namespace";
27+
28+
// ---------------------------------------------------------------------------
29+
830
/// Path to the sandbox supervisor binary inside the container image.
931
///
1032
/// All compute drivers must launch this binary as the container entrypoint to

crates/openshell-driver-docker/src/lib.rs

Lines changed: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ use bollard::query_parameters::{
1919
use bytes::Bytes;
2020
use futures::{Stream, StreamExt};
2121
use openshell_core::config::{DEFAULT_DOCKER_NETWORK_NAME, DEFAULT_STOP_TIMEOUT_SECS};
22-
use openshell_core::driver_utils::SUPERVISOR_IMAGE_BINARY_PATH;
22+
use openshell_core::driver_utils::{
23+
LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME,
24+
LABEL_SANDBOX_NAMESPACE, SUPERVISOR_IMAGE_BINARY_PATH,
25+
};
2326
use openshell_core::gpu::cdi_gpu_device_ids;
2427
use openshell_core::proto::compute::v1::{
2528
CreateSandboxRequest, CreateSandboxResponse, DeleteSandboxRequest, DeleteSandboxResponse,
@@ -48,12 +51,6 @@ const WATCH_BUFFER: usize = 128;
4851
const WATCH_POLL_INTERVAL: Duration = Duration::from_secs(2);
4952
const WATCH_POLL_MAX_BACKOFF: Duration = Duration::from_secs(30);
5053

51-
const MANAGED_BY_LABEL_KEY: &str = "openshell.ai/managed-by";
52-
const MANAGED_BY_LABEL_VALUE: &str = "openshell";
53-
const SANDBOX_ID_LABEL_KEY: &str = "openshell.ai/sandbox-id";
54-
const SANDBOX_NAME_LABEL_KEY: &str = "openshell.ai/sandbox-name";
55-
const SANDBOX_NAMESPACE_LABEL_KEY: &str = "openshell.ai/sandbox-namespace";
56-
5754
const SUPERVISOR_MOUNT_PATH: &str = "/opt/openshell/bin/openshell-sandbox";
5855
const TLS_CA_MOUNT_PATH: &str = "/etc/openshell/tls/client/ca.crt";
5956
const TLS_CERT_MOUNT_PATH: &str = "/etc/openshell/tls/client/tls.crt";
@@ -683,9 +680,9 @@ impl DockerComputeDriver {
683680
) -> Result<Option<ContainerSummary>, Status> {
684681
let mut label_filter_values = Vec::new();
685682
if !sandbox_id.is_empty() {
686-
label_filter_values.push(format!("{SANDBOX_ID_LABEL_KEY}={sandbox_id}"));
683+
label_filter_values.push(format!("{LABEL_SANDBOX_ID}={sandbox_id}"));
687684
} else if !sandbox_name.is_empty() {
688-
label_filter_values.push(format!("{SANDBOX_NAME_LABEL_KEY}={sandbox_name}"));
685+
label_filter_values.push(format!("{LABEL_SANDBOX_NAME}={sandbox_name}"));
689686
}
690687

691688
let filters =
@@ -706,15 +703,15 @@ impl DockerComputeDriver {
706703
return false;
707704
};
708705
let namespace_matches = labels
709-
.get(SANDBOX_NAMESPACE_LABEL_KEY)
706+
.get(LABEL_SANDBOX_NAMESPACE)
710707
.is_some_and(|value| value == &self.config.sandbox_namespace);
711708
let id_matches = sandbox_id.is_empty()
712709
|| labels
713-
.get(SANDBOX_ID_LABEL_KEY)
710+
.get(LABEL_SANDBOX_ID)
714711
.is_some_and(|value| value == sandbox_id);
715712
let name_matches = sandbox_name.is_empty()
716713
|| labels
717-
.get(SANDBOX_NAME_LABEL_KEY)
714+
.get(LABEL_SANDBOX_NAME)
718715
.is_some_and(|value| value == sandbox_name);
719716
namespace_matches && id_matches && name_matches
720717
}))
@@ -1015,17 +1012,17 @@ fn build_container_create_body(
10151012
let resource_limits = docker_resource_limits(template)?;
10161013
let mut labels = template.labels.clone();
10171014
labels.insert(
1018-
MANAGED_BY_LABEL_KEY.to_string(),
1019-
MANAGED_BY_LABEL_VALUE.to_string(),
1015+
LABEL_MANAGED_BY.to_string(),
1016+
LABEL_MANAGED_BY_VALUE.to_string(),
10201017
);
1021-
labels.insert(SANDBOX_ID_LABEL_KEY.to_string(), sandbox.id.clone());
1022-
labels.insert(SANDBOX_NAME_LABEL_KEY.to_string(), sandbox.name.clone());
1018+
labels.insert(LABEL_SANDBOX_ID.to_string(), sandbox.id.clone());
1019+
labels.insert(LABEL_SANDBOX_NAME.to_string(), sandbox.name.clone());
10231020
// The list/get/find paths filter by `config.sandbox_namespace`, so use
10241021
// the same value here. `DriverSandbox.namespace` is unset on the request
10251022
// path (the gateway elides it), and using it would produce containers
10261023
// that the driver itself cannot find afterwards.
10271024
labels.insert(
1028-
SANDBOX_NAMESPACE_LABEL_KEY.to_string(),
1025+
LABEL_SANDBOX_NAMESPACE.to_string(),
10291026
config.sandbox_namespace.clone(),
10301027
);
10311028

@@ -1217,8 +1214,8 @@ async fn ensure_bridge_network(docker: &Docker, network_name: &str) -> CoreResul
12171214
driver: Some(DOCKER_NETWORK_DRIVER.to_string()),
12181215
attachable: Some(true),
12191216
labels: Some(HashMap::from([(
1220-
MANAGED_BY_LABEL_KEY.to_string(),
1221-
MANAGED_BY_LABEL_VALUE.to_string(),
1217+
LABEL_MANAGED_BY.to_string(),
1218+
LABEL_MANAGED_BY_VALUE.to_string(),
12221219
)])),
12231220
..Default::default()
12241221
})
@@ -1397,10 +1394,10 @@ fn sandbox_from_container_summary(
13971394
readiness: &dyn SupervisorReadiness,
13981395
) -> Option<DriverSandbox> {
13991396
let labels = summary.labels.as_ref()?;
1400-
let id = labels.get(SANDBOX_ID_LABEL_KEY)?.clone();
1401-
let name = labels.get(SANDBOX_NAME_LABEL_KEY)?.clone();
1397+
let id = labels.get(LABEL_SANDBOX_ID)?.clone();
1398+
let name = labels.get(LABEL_SANDBOX_NAME)?.clone();
14021399
let namespace = labels
1403-
.get(SANDBOX_NAMESPACE_LABEL_KEY)
1400+
.get(LABEL_SANDBOX_NAMESPACE)
14041401
.cloned()
14051402
.unwrap_or_default();
14061403

@@ -1573,8 +1570,8 @@ fn managed_container_label_filters(
15731570
extra_values: impl IntoIterator<Item = String>,
15741571
) -> HashMap<String, Vec<String>> {
15751572
let mut values = vec![
1576-
format!("{MANAGED_BY_LABEL_KEY}={MANAGED_BY_LABEL_VALUE}"),
1577-
format!("{SANDBOX_NAMESPACE_LABEL_KEY}={sandbox_namespace}"),
1573+
format!("{LABEL_MANAGED_BY}={LABEL_MANAGED_BY_VALUE}"),
1574+
format!("{LABEL_SANDBOX_NAMESPACE}={sandbox_namespace}"),
15781575
];
15791576
values.extend(extra_values);
15801577
label_filters(values)

crates/openshell-driver-docker/src/tests.rs

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@
33

44
use super::*;
55
use openshell_core::config::{CDI_GPU_DEVICE_ALL, DEFAULT_SERVER_PORT};
6+
use openshell_core::driver_utils::{
7+
LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME,
8+
LABEL_SANDBOX_NAMESPACE,
9+
};
610
use openshell_core::proto::compute::v1::{
711
DriverResourceRequirements, DriverSandboxSpec, DriverSandboxTemplate,
812
};
@@ -441,12 +445,12 @@ fn build_binds_uses_docker_tls_directory() {
441445
#[test]
442446
fn managed_container_label_filters_include_gateway_namespace() {
443447
let filters =
444-
managed_container_label_filters("tenant-a", [format!("{SANDBOX_ID_LABEL_KEY}=sbx-123")]);
448+
managed_container_label_filters("tenant-a", [format!("{LABEL_SANDBOX_ID}=sbx-123")]);
445449
let labels = filters.get("label").unwrap();
446450

447-
assert!(labels.contains(&format!("{MANAGED_BY_LABEL_KEY}={MANAGED_BY_LABEL_VALUE}")));
448-
assert!(labels.contains(&format!("{SANDBOX_NAMESPACE_LABEL_KEY}=tenant-a")));
449-
assert!(labels.contains(&format!("{SANDBOX_ID_LABEL_KEY}=sbx-123")));
451+
assert!(labels.contains(&format!("{LABEL_MANAGED_BY}={LABEL_MANAGED_BY_VALUE}")));
452+
assert!(labels.contains(&format!("{LABEL_SANDBOX_NAMESPACE}=tenant-a")));
453+
assert!(labels.contains(&format!("{LABEL_SANDBOX_ID}=sbx-123")));
450454
}
451455

452456
#[test]
@@ -462,7 +466,7 @@ fn build_container_create_body_clears_inherited_cmd() {
462466
create_body
463467
.labels
464468
.as_ref()
465-
.and_then(|labels| labels.get(SANDBOX_NAMESPACE_LABEL_KEY)),
469+
.and_then(|labels| labels.get(LABEL_SANDBOX_NAMESPACE)),
466470
Some(&"default".to_string())
467471
);
468472
let host_config = create_body.host_config.as_ref().unwrap();
@@ -606,7 +610,7 @@ fn build_container_create_body_uses_runtime_namespace_label() {
606610
let labels = create_body.labels.expect("labels are populated");
607611

608612
assert_eq!(
609-
labels.get(SANDBOX_NAMESPACE_LABEL_KEY),
613+
labels.get(LABEL_SANDBOX_NAMESPACE),
610614
Some(&"tenant-a".to_string()),
611615
"namespace label must reflect the driver's runtime config"
612616
);
@@ -618,12 +622,9 @@ fn driver_status_keeps_running_sandboxes_provisioning_with_stable_message() {
618622
id: Some("cid".to_string()),
619623
names: Some(vec!["/openshell-demo".to_string()]),
620624
labels: Some(HashMap::from([
621-
(SANDBOX_ID_LABEL_KEY.to_string(), "sbx-1".to_string()),
622-
(SANDBOX_NAME_LABEL_KEY.to_string(), "demo".to_string()),
623-
(
624-
SANDBOX_NAMESPACE_LABEL_KEY.to_string(),
625-
"default".to_string(),
626-
),
625+
(LABEL_SANDBOX_ID.to_string(), "sbx-1".to_string()),
626+
(LABEL_SANDBOX_NAME.to_string(), "demo".to_string()),
627+
(LABEL_SANDBOX_NAMESPACE.to_string(), "default".to_string()),
627628
])),
628629
state: Some(ContainerSummaryStateEnum::RUNNING),
629630
status: Some("Up 2 seconds".to_string()),
@@ -675,12 +676,9 @@ fn driver_status_marks_restarting_sandboxes_as_error() {
675676
id: Some("cid".to_string()),
676677
names: Some(vec!["/openshell-demo".to_string()]),
677678
labels: Some(HashMap::from([
678-
(SANDBOX_ID_LABEL_KEY.to_string(), "sbx-1".to_string()),
679-
(SANDBOX_NAME_LABEL_KEY.to_string(), "demo".to_string()),
680-
(
681-
SANDBOX_NAMESPACE_LABEL_KEY.to_string(),
682-
"default".to_string(),
683-
),
679+
(LABEL_SANDBOX_ID.to_string(), "sbx-1".to_string()),
680+
(LABEL_SANDBOX_NAME.to_string(), "demo".to_string()),
681+
(LABEL_SANDBOX_NAMESPACE.to_string(), "default".to_string()),
684682
])),
685683
state: Some(ContainerSummaryStateEnum::RESTARTING),
686684
status: Some("Restarting (1) 2 seconds ago".to_string()),

crates/openshell-driver-kubernetes/src/driver.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ use kube::core::gvk::GroupVersionKind;
1313
use kube::core::{DynamicObject, ObjectMeta};
1414
use kube::runtime::watcher::{self, Event};
1515
use kube::{Client, Error as KubeError};
16-
use openshell_core::driver_utils::SUPERVISOR_IMAGE_BINARY_PATH;
16+
use openshell_core::driver_utils::{
17+
LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, SUPERVISOR_IMAGE_BINARY_PATH,
18+
};
1719
use openshell_core::progress::{
1820
PROGRESS_STEP_PULLING_IMAGE, PROGRESS_STEP_REQUESTING_SANDBOX, PROGRESS_STEP_STARTING_SANDBOX,
1921
mark_progress_active, mark_progress_complete, mark_progress_detail,
@@ -72,9 +74,7 @@ const KUBE_API_TIMEOUT: Duration = Duration::from_secs(30);
7274
const SANDBOX_GROUP: &str = "agents.x-k8s.io";
7375
const SANDBOX_VERSION: &str = "v1alpha1";
7476
pub const SANDBOX_KIND: &str = "Sandbox";
75-
const SANDBOX_ID_LABEL: &str = "openshell.ai/sandbox-id";
76-
const SANDBOX_MANAGED_LABEL: &str = "openshell.ai/managed-by";
77-
const SANDBOX_MANAGED_VALUE: &str = "openshell";
77+
7878
const GPU_RESOURCE_NAME: &str = "nvidia.com/gpu";
7979
const GPU_RESOURCE_QUANTITY: &str = "1";
8080

@@ -552,17 +552,17 @@ impl KubernetesComputeDriver {
552552

553553
fn sandbox_labels(sandbox: &Sandbox) -> BTreeMap<String, String> {
554554
let mut labels = BTreeMap::new();
555-
labels.insert(SANDBOX_ID_LABEL.to_string(), sandbox.id.clone());
555+
labels.insert(LABEL_SANDBOX_ID.to_string(), sandbox.id.clone());
556556
labels.insert(
557-
SANDBOX_MANAGED_LABEL.to_string(),
558-
SANDBOX_MANAGED_VALUE.to_string(),
557+
LABEL_MANAGED_BY.to_string(),
558+
LABEL_MANAGED_BY_VALUE.to_string(),
559559
);
560560
labels
561561
}
562562

563563
fn sandbox_id_from_object(obj: &DynamicObject) -> Result<String, String> {
564564
if let Some(labels) = obj.metadata.labels.as_ref()
565-
&& let Some(id) = labels.get(SANDBOX_ID_LABEL)
565+
&& let Some(id) = labels.get(LABEL_SANDBOX_ID)
566566
{
567567
return Ok(id.clone());
568568
}

crates/openshell-providers/src/providers/anthropic.rs

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,8 @@ pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec {
88
credential_env_vars: &["ANTHROPIC_API_KEY"],
99
};
1010

11-
#[cfg(test)]
12-
mod tests {
13-
use super::SPEC;
14-
use crate::discover_with_spec;
15-
use crate::test_helpers::MockDiscoveryContext;
16-
17-
#[test]
18-
fn discovers_anthropic_env_credentials() {
19-
let ctx = MockDiscoveryContext::new().with_env("ANTHROPIC_API_KEY", "sk-ant-test");
20-
let discovered = discover_with_spec(&SPEC, &ctx)
21-
.expect("discovery")
22-
.expect("provider");
23-
assert_eq!(
24-
discovered.credentials.get("ANTHROPIC_API_KEY"),
25-
Some(&"sk-ant-test".to_string())
26-
);
27-
}
28-
}
11+
test_discovers_env_credential!(
12+
discovers_anthropic_env_credentials,
13+
"ANTHROPIC_API_KEY",
14+
"sk-ant-test"
15+
);

crates/openshell-providers/src/providers/claude.rs

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,8 @@ pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec {
88
credential_env_vars: &["ANTHROPIC_API_KEY", "CLAUDE_API_KEY"],
99
};
1010

11-
#[cfg(test)]
12-
mod tests {
13-
use super::SPEC;
14-
use crate::discover_with_spec;
15-
use crate::test_helpers::MockDiscoveryContext;
16-
17-
#[test]
18-
fn discovers_claude_env_credentials() {
19-
let ctx = MockDiscoveryContext::new().with_env("ANTHROPIC_API_KEY", "test-key");
20-
let discovered = discover_with_spec(&SPEC, &ctx)
21-
.expect("discovery")
22-
.expect("provider");
23-
assert_eq!(
24-
discovered.credentials.get("ANTHROPIC_API_KEY"),
25-
Some(&"test-key".to_string())
26-
);
27-
}
28-
}
11+
test_discovers_env_credential!(
12+
discovers_claude_env_credentials,
13+
"ANTHROPIC_API_KEY",
14+
"test-key"
15+
);

crates/openshell-providers/src/providers/codex.rs

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,8 @@ pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec {
88
credential_env_vars: &["OPENAI_API_KEY"],
99
};
1010

11-
#[cfg(test)]
12-
mod tests {
13-
use super::SPEC;
14-
use crate::discover_with_spec;
15-
use crate::test_helpers::MockDiscoveryContext;
16-
17-
#[test]
18-
fn discovers_codex_env_credentials() {
19-
let ctx = MockDiscoveryContext::new().with_env("OPENAI_API_KEY", "openai-key");
20-
let discovered = discover_with_spec(&SPEC, &ctx)
21-
.expect("discovery")
22-
.expect("provider");
23-
assert_eq!(
24-
discovered.credentials.get("OPENAI_API_KEY"),
25-
Some(&"openai-key".to_string())
26-
);
27-
}
28-
}
11+
test_discovers_env_credential!(
12+
discovers_codex_env_credentials,
13+
"OPENAI_API_KEY",
14+
"openai-key"
15+
);

crates/openshell-providers/src/providers/copilot.rs

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,8 @@ pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec {
88
credential_env_vars: &["COPILOT_GITHUB_TOKEN", "GH_TOKEN", "GITHUB_TOKEN"],
99
};
1010

11-
#[cfg(test)]
12-
mod tests {
13-
use super::SPEC;
14-
use crate::discover_with_spec;
15-
use crate::test_helpers::MockDiscoveryContext;
16-
17-
#[test]
18-
fn discovers_copilot_env_credentials() {
19-
let ctx = MockDiscoveryContext::new().with_env("COPILOT_GITHUB_TOKEN", "ghp-copilot-token");
20-
let discovered = discover_with_spec(&SPEC, &ctx)
21-
.expect("discovery")
22-
.expect("provider");
23-
assert_eq!(
24-
discovered.credentials.get("COPILOT_GITHUB_TOKEN"),
25-
Some(&"ghp-copilot-token".to_string())
26-
);
27-
}
28-
}
11+
test_discovers_env_credential!(
12+
discovers_copilot_env_credentials,
13+
"COPILOT_GITHUB_TOKEN",
14+
"ghp-copilot-token"
15+
);

crates/openshell-providers/src/providers/github.rs

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,4 @@ pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec {
88
credential_env_vars: &["GITHUB_TOKEN", "GH_TOKEN"],
99
};
1010

11-
#[cfg(test)]
12-
mod tests {
13-
use super::SPEC;
14-
use crate::discover_with_spec;
15-
use crate::test_helpers::MockDiscoveryContext;
16-
17-
#[test]
18-
fn discovers_github_env_credentials() {
19-
let ctx = MockDiscoveryContext::new().with_env("GH_TOKEN", "gh-token");
20-
let discovered = discover_with_spec(&SPEC, &ctx)
21-
.expect("discovery")
22-
.expect("provider");
23-
assert_eq!(
24-
discovered.credentials.get("GH_TOKEN"),
25-
Some(&"gh-token".to_string())
26-
);
27-
}
28-
}
11+
test_discovers_env_credential!(discovers_github_env_credentials, "GH_TOKEN", "gh-token");

0 commit comments

Comments
 (0)