Skip to content

refactor(proto)!: use well-known time types #3113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
derekwaynecarr wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(proto)!: use well-known time types #3113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions architecture/gateway.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,14 @@ This keeps the gateway data model portable across storage backends and leaves
room for future stores that can provide the same object, label, version, and
scope semantics.

Public protobuf APIs represent absolute times with `google.protobuf.Timestamp`
and elapsed time with `google.protobuf.Duration`. The integer
`created_at_ms` and `updated_at_ms` database columns are intentionally internal
bookkeeping values, not part of that public convention. On startup, both
storage backends transactionally rewrite legacy scalar time fields inside
protobuf payloads before serving requests. A malformed affected payload aborts
and rolls back startup migration rather than silently dropping a value.

The SQLite adapter tightens the on-disk database file to mode `0o600` on every
connect so that provider API keys, SSH session tokens, and sandbox metadata are
not readable by other local users on shared hosts. The same restriction is
Expand Down
259 changes: 169 additions & 90 deletions crates/openshell-cli/src/run.rs
View file
Open in desktop

Large diffs are not rendered by default.

36 changes: 16 additions & 20 deletions crates/openshell-cli/tests/ensure_providers_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ impl TestOpenShell {
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
id: format!("id-{name}"),
name: name.to_string(),
created_at_ms: 0,
created_time: None,
labels: HashMap::new(),
resource_version: 0,
annotations: HashMap::new(),
workspace: String::new(),
deletion_timestamp_ms: 0,
deletion_time: None,
}),
r#type: provider_type.to_string(),
credentials: HashMap::new(),
config: HashMap::new(),
credential_expires_at_ms: HashMap::new(),
credential_expiration_times: HashMap::new(),
profile_workspace: "default".to_string(),
credential_handles: HashMap::new(),
},
Expand Down Expand Up @@ -382,38 +382,34 @@ impl OpenShell for TestOpenShell {
}
base
};
let merge_expiry = |mut base: HashMap<String, i64>, incoming: HashMap<String, i64>| {
if incoming.is_empty() {
return base;
}
for (k, v) in incoming {
if v <= 0 {
base.remove(&k);
} else {
base.insert(k, v);
let merge_expiry =
|mut base: HashMap<String, prost_types::Timestamp>,
incoming: HashMap<String, prost_types::Timestamp>| {
if incoming.is_empty() {
return base;
}
}
base
};
base.extend(incoming);
base
};
let existing_metadata = existing.metadata.clone().unwrap_or_default();
let provider_metadata = provider.metadata.clone().unwrap_or_default();
let updated = Provider {
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
id: existing_metadata.id,
name: provider_metadata.name,
created_at_ms: existing_metadata.created_at_ms,
created_time: existing_metadata.created_time,
labels: existing_metadata.labels,
resource_version: 0,
annotations: HashMap::new(),
workspace: String::new(),
deletion_timestamp_ms: 0,
deletion_time: None,
}),
r#type: existing.r#type,
credentials: merge(existing.credentials, provider.credentials),
config: merge(existing.config, provider.config),
credential_expires_at_ms: merge_expiry(
existing.credential_expires_at_ms,
provider.credential_expires_at_ms,
credential_expiration_times: merge_expiry(
existing.credential_expiration_times,
provider.credential_expiration_times,
),
profile_workspace: existing.profile_workspace,
credential_handles: if provider.credential_handles.is_empty() {
Expand Down
62 changes: 31 additions & 31 deletions crates/openshell-cli/tests/provider_commands_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,12 @@ impl OpenShell for TestOpenShell {
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
id: format!("sb-{name}"),
name,
created_at_ms: 0,
created_time: None,
labels: HashMap::new(),
resource_version: 1,
annotations: HashMap::new(),
workspace: String::new(),
deletion_timestamp_ms: 0,
deletion_time: None,
}),
spec: None,
status: None,
Expand Down Expand Up @@ -651,38 +651,34 @@ impl OpenShell for TestOpenShell {
}
base
};
let merge_expiry = |mut base: HashMap<String, i64>, incoming: HashMap<String, i64>| {
if incoming.is_empty() {
return base;
}
for (k, v) in incoming {
if v <= 0 {
base.remove(&k);
} else {
base.insert(k, v);
let merge_expiry =
|mut base: HashMap<String, prost_types::Timestamp>,
incoming: HashMap<String, prost_types::Timestamp>| {
if incoming.is_empty() {
return base;
}
}
base
};
base.extend(incoming);
base
};
let existing_metadata = existing.metadata.clone().unwrap_or_default();
let provider_metadata = provider.metadata.clone().unwrap_or_default();
let updated = Provider {
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
id: existing_metadata.id,
name: provider_metadata.name,
created_at_ms: existing_metadata.created_at_ms,
created_time: existing_metadata.created_time,
labels: existing_metadata.labels,
resource_version: 0,
annotations: HashMap::new(),
workspace: String::new(),
deletion_timestamp_ms: 0,
deletion_time: None,
}),
r#type: existing.r#type,
credentials: merge(existing.credentials, provider.credentials),
config: merge(existing.config, provider.config),
credential_expires_at_ms: merge_expiry(
existing.credential_expires_at_ms,
provider.credential_expires_at_ms,
credential_expiration_times: merge_expiry(
existing.credential_expiration_times,
provider.credential_expiration_times,
),
profile_workspace: existing.profile_workspace,
credential_handles: if provider.credential_handles.is_empty() {
Expand Down Expand Up @@ -743,7 +739,10 @@ impl OpenShell for TestOpenShell {
credential_key: request.credential_key.clone(),
material: request.material.clone(),
secret_material_keys: request.secret_material_keys.clone(),
expires_at_ms: request.expires_at_ms,
expires_at_ms: request
.expiration_time
.as_ref()
.and_then(|value| openshell_core::time::timestamp_to_millis(value).ok()),
});
let configure_failure = self
.state
Expand All @@ -764,14 +763,14 @@ impl OpenShell for TestOpenShell {
credential_key: request.credential_key.clone(),
strategy: request.strategy,
status: "configured".to_string(),
expires_at_ms: request.expires_at_ms.unwrap_or_default(),
next_refresh_at_ms: 0,
last_refresh_at_ms: 0,
expiration_time: request.expiration_time,
next_refresh_time: None,
last_refresh_time: None,
last_error: String::new(),
recovery_action: 0,
failure_code: String::new(),
provider_error_subtype: String::new(),
last_error_at_ms: 0,
last_error_time: None,
};
drop(providers);
self.state
Expand Down Expand Up @@ -810,9 +809,9 @@ impl OpenShell for TestOpenShell {
.get_mut(&(provider_name.clone(), credential_key.clone()))
.ok_or_else(|| Status::not_found("provider refresh state not found"))?;
status.status = "refreshed".to_string();
status.last_refresh_at_ms = 1;
status.next_refresh_at_ms = 3_600_000;
status.expires_at_ms = 3_600_000;
status.last_refresh_time = openshell_core::time::timestamp_from_millis(1).ok();
status.next_refresh_time = openshell_core::time::timestamp_from_millis(3_600_000).ok();
status.expiration_time = openshell_core::time::timestamp_from_millis(3_600_000).ok();
let status = status.clone();
drop(refresh_statuses);
let mut providers = self.state.providers.lock().await;
Expand All @@ -822,9 +821,10 @@ impl OpenShell for TestOpenShell {
provider
.credentials
.insert(credential_key.clone(), format!("minted-{credential_key}"));
provider
.credential_expires_at_ms
.insert(credential_key, 3_600_000);
provider.credential_expiration_times.insert(
credential_key,
openshell_core::time::timestamp_from_millis(3_600_000).unwrap(),
);
Ok(Response::new(RotateProviderCredentialResponse {
status: Some(status),
}))
Expand Down Expand Up @@ -2132,7 +2132,7 @@ async fn provider_update_from_existing_uses_profile_discovery_when_v2_enabled()
r#type: "custom-update-discovery".to_string(),
credentials: HashMap::new(),
config: HashMap::new(),
credential_expires_at_ms: HashMap::new(),
credential_expiration_times: HashMap::new(),
profile_workspace: "default".to_string(),
credential_handles: HashMap::new(),
},
Expand Down
16 changes: 8 additions & 8 deletions crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,12 @@ impl OpenShell for TestOpenShell {
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
id: format!("id-{sandbox_name}"),
name: sandbox_name,
created_at_ms: 0,
created_time: None,
labels: HashMap::new(),
resource_version: 0,
annotations: HashMap::new(),
workspace: String::new(),
deletion_timestamp_ms: 0,
deletion_time: None,
}),
..Sandbox::default()
};
Expand Down Expand Up @@ -158,12 +158,12 @@ impl OpenShell for TestOpenShell {
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
id: format!("id-{name}"),
name,
created_at_ms: 0,
created_time: None,
labels: HashMap::new(),
resource_version: 0,
annotations: HashMap::new(),
workspace: String::new(),
deletion_timestamp_ms: 0,
deletion_time: None,
}),
..Sandbox::default()
};
Expand Down Expand Up @@ -449,12 +449,12 @@ impl OpenShell for TestOpenShell {
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
id: sandbox_id.clone(),
name: sandbox_id.trim_start_matches("id-").to_string(),
created_at_ms: 0,
created_time: None,
labels: HashMap::new(),
resource_version: 0,
annotations: HashMap::new(),
workspace: String::new(),
deletion_timestamp_ms: 0,
deletion_time: None,
}),
..Sandbox::default()
};
Expand All @@ -467,7 +467,7 @@ impl OpenShell for TestOpenShell {
status: "False".to_string(),
reason: "ProcessExited".to_string(),
message: "VM process exited with status 0".to_string(),
last_transition_time: String::new(),
transition_time: None,
}],
..Default::default()
}),
Expand Down Expand Up @@ -517,7 +517,7 @@ impl OpenShell for TestOpenShell {
.send(Ok(SandboxStreamEvent {
payload: Some(sandbox_stream_event::Payload::Log(SandboxLogLine {
sandbox_id: sandbox_id.clone(),
timestamp_ms: 0,
event_time: None,
level: "INFO".to_string(),
target: "test".to_string(),
message: message.to_string(),
Expand Down
8 changes: 4 additions & 4 deletions crates/openshell-cli/tests/sandbox_name_fallback_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,12 @@ impl OpenShell for TestOpenShell {
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
id: "test-id".to_string(),
name,
created_at_ms: 0,
created_time: None,
labels: std::collections::HashMap::new(),
resource_version: 0,
annotations: std::collections::HashMap::new(),
workspace: String::new(),
deletion_timestamp_ms: 0,
deletion_time: None,
}),
..Default::default()
}),
Expand Down Expand Up @@ -473,8 +473,8 @@ impl OpenShell for TestOpenShell {
version: 7,
policy_hash: "sha256:test-policy".to_string(),
status: PolicyStatus::Loaded.into(),
created_at_ms: 1_700_000_000_000,
loaded_at_ms: 1_700_000_000_500,
created_time: openshell_core::time::timestamp_from_millis(1_700_000_000_000).ok(),
loaded_time: openshell_core::time::timestamp_from_millis(1_700_000_000_500).ok(),
policy: Some(policy),
..Default::default()
}),
Expand Down
2 changes: 1 addition & 1 deletion crates/openshell-core/src/forward.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1093,7 +1093,7 @@ mod tests {
gateway_host: "gateway.example.com".to_string(),
gateway_port: 443,
host_key_fingerprint: String::new(),
expires_at_ms: 0,
expiration_time: None,
}
}

Expand Down
32 changes: 26 additions & 6 deletions crates/openshell-core/src/grpc_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use crate::proto::{
UpdateConfigRequest, inference_client::InferenceClient, open_shell_client::OpenShellClient,
};
use crate::sandbox_env;
use crate::time::{duration_to_std, timestamp_to_millis};
use miette::{IntoDiagnostic, Result, WrapErr};
use openshell_extension_core::{BearerTokenSlot, ExtensionCredentialStore};
use tonic::Status;
Expand Down Expand Up @@ -483,10 +484,11 @@ async fn refresh_extension_credentials_with_client(
"gateway returned an unexpected or duplicate extension credential"
));
}
validated.insert(
credential.service_name,
(credential.token, credential.expires_at_ms),
);
let expiration_time = credential.expiration_time.as_ref().ok_or_else(|| {
miette::miette!("gateway returned an extension credential without an expiration time")
})?;
let expires_at_ms = timestamp_to_millis(expiration_time).into_diagnostic()?;
validated.insert(credential.service_name, (credential.token, expires_at_ms));
}
if validated.len() != expected.len() {
return Err(miette::miette!(
Expand Down Expand Up @@ -850,10 +852,19 @@ pub async fn fetch_provider_environment(
.into_diagnostic()?;

let inner = response.into_inner();
let credential_expires_at_ms = inner
.credential_expiration_times
.iter()
.map(|(name, expiration_time)| {
timestamp_to_millis(expiration_time)
.map(|value| (name.clone(), value))
.into_diagnostic()
})
.collect::<Result<HashMap<_, _>>>()?;
Ok(ProviderEnvironmentResult {
environment: inner.environment,
provider_env_revision: inner.provider_env_revision,
credential_expires_at_ms: inner.credential_expires_at_ms,
credential_expires_at_ms,
dynamic_credentials: inner.dynamic_credentials,
static_credential_bindings: inner.static_credential_bindings,
non_secret_environment_keys: inner.non_secret_environment_keys,
Expand Down Expand Up @@ -886,9 +897,18 @@ pub async fn exchange_provider_subject_token(
.await
.map_err(provider_subject_token_exchange_status)?;
let inner = response.into_inner();
let expires_in = inner
.expires_after
.as_ref()
.map(duration_to_std)
.transpose()
.into_diagnostic()?
.map_or(0, |value| {
i64::try_from(value.as_secs()).unwrap_or(i64::MAX)
});
Ok(ProviderSubjectTokenExchangeResult {
access_token: inner.access_token,
expires_in: inner.expires_in,
expires_in,
token_type: inner.token_type,
})
}
Expand Down
Loading
Loading