migration: phase 2 — adapters can target local syva-core#49
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a shared Unix-socket gRPC client for the node-local syva.core.v1 API and updates multiple adapters (file/k8s/api) to optionally target a local syva-core via --core-socket while keeping existing CP (--cp-endpoint) mode.
Changes:
- Introduces new
syva-core-clientcrate to centralize tonic Unix-socket connection + retry logic forsyva.core.v1. - Adds dual-mode (CP vs local-core) configuration and execution paths to
syva-file,syva-k8s, andsyva-api. - Implements local-core translation/mapping and adds focused tests + a name-based desired/applied diff helper for file adapter core mode.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| syva-core-client/src/lib.rs | New Unix-socket tonic client and retry loop for local syva-core. |
| syva-core-client/Cargo.toml | New crate manifest with tonic/tokio/tower/hyper-util deps. |
| syva-adapter-k8s/src/watcher.rs | Adds CP vs local-core mode selection and local-core reconcile/apply/delete paths. |
| syva-adapter-k8s/src/mapper.rs | Adds CRD → syva.core.v1.RegisterZoneRequest translation + tests. |
| syva-adapter-k8s/src/main.rs | Adds --core-socket and makes CP args optional/validated. |
| syva-adapter-k8s/Cargo.toml | Adds dependency on syva-core-client. |
| syva-adapter-file/src/translate.rs | Adds file policy → local-core RegisterZoneRequest mapping + tests. |
| syva-adapter-file/src/run.rs | Adds CP vs local-core run modes; local-core reconcile loop and core RPC usage. |
| syva-adapter-file/src/main.rs | Adds --core-socket and makes CP args optional/validated. |
| syva-adapter-file/src/lib.rs | Exposes new diff module. |
| syva-adapter-file/src/diff.rs | Adds desired-vs-applied name diffing for local-core reconcile + tests. |
| syva-adapter-file/Cargo.toml | Adds dependency on syva-core-client. |
| syva-adapter-api/src/routes.rs | Adds CP vs local-core state/mode; routes can operate against core via Unix socket. |
| syva-adapter-api/src/main.rs | Adds --core-socket and makes CP args optional/validated. |
| syva-adapter-api/Cargo.toml | Adds dependency on syva-core-client. |
| Cargo.toml | Adds syva-core-client to workspace members. |
| Cargo.lock | Locks new crate and its dependency edges into the workspace. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let create = desired_names | ||
| .difference(&applied_names) | ||
| .map(|name| (*name).to_string()) | ||
| .collect(); | ||
| let update = desired_names | ||
| .intersection(&applied_names) | ||
| .map(|name| (*name).to_string()) | ||
| .collect(); |
There was a problem hiding this comment.
diff_against_core marks every zone present in both desired and applied as update, regardless of whether the policy actually changed. As a result, reconcile_once_core will re-register every existing zone on every tick, and re-run host-path + comm registration loops, which can cause unnecessary BPF map churn. If core doesn’t expose applied policy details, consider diffing against an in-memory last-applied snapshot (policy hash/JSON) and only updating when the desired policy changes, while still doing a full refresh on startup.
| core.register_zone(spec_to_core_register(&name, crd)) | ||
| .await?; | ||
| info!(zone = %name, "zone registered from CRD (initial)"); |
There was a problem hiding this comment.
Local-core mode registers zones via register_zone, but syva-core currently does not translate ZonePolicy.allowed_zones into ZONE_ALLOWED_COMMS entries (those are only set via the allow_comm RPC). That means CRD allowed_zones will have no effect in local-core mode. To match CP semantics, reconcile comm pairs explicitly (and deny pairs that are no longer mutually allowed).
| Ok(RegisterZoneRequest { | ||
| zone_name: name.to_string(), | ||
| policy: Some(ZonePolicy { | ||
| host_paths: policy.host_paths, | ||
| allowed_zones: policy.allowed_zones, | ||
| allow_ptrace: policy.allow_ptrace, | ||
| zone_type: match policy.zone_type { | ||
| CoreZoneType::Privileged => 1, | ||
| CoreZoneType::Standard | CoreZoneType::Isolated => 0, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
In local-core mode core_register_request accepts/forwards allowed_zones, but the handlers only call register_zone. In the current syva-core implementation, cross-zone comms are enforced via ZONE_ALLOWED_COMMS and are only updated through the allow_comm / deny_comm RPCs, not via register_zone. As a result, allowed_zones from the API won’t actually be applied in local-core mode. Consider either (a) translating allowed_zones into comm-pair reconciliation in the adapter, or (b) removing/ignoring allowed_zones from the accepted local-core schema until core enforces it via register_zone.
| if let Some(status) = query.status { | ||
| zones.retain(|zone| zone.state == status); | ||
| } | ||
| zones.truncate(query.limit.unwrap_or(100).max(0) as usize); |
There was a problem hiding this comment.
zones.truncate(query.limit.unwrap_or(100).max(0) as usize) can wrap on 32-bit targets if a client passes a large limit (because the as usize cast truncates). Prefer clamping with usize::try_from(limit).unwrap_or(usize::MAX) (after handling negatives) to avoid surprising behavior.
| zones.truncate(query.limit.unwrap_or(100).max(0) as usize); | |
| let limit = query.limit.unwrap_or(100).max(0); | |
| let limit = usize::try_from(limit).unwrap_or(usize::MAX); | |
| zones.truncate(limit); |
| for path in &policy.policy.filesystem.host_paths { | ||
| core.register_host_path(RegisterHostPathRequest { | ||
| zone_name: name.to_string(), | ||
| path: path.clone(), | ||
| recursive: true, | ||
| }) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
register_host_path only ever adds/populates inode entries; there is no corresponding removal here. If a host path is removed from the desired policy, the old inode mappings will remain in INODE_ZONE_MAP until the whole zone is removed/drained, meaning local-core updates can only widen filesystem access over time. If policy updates are expected to be authoritative, consider adding/using a core RPC to clear a zone’s inode mappings (or re-register by clearing inodes first) so removals take effect.
| for allowed_zone in &policy.policy.network.allowed_zones { | ||
| if let Err(error) = core | ||
| .allow_comm(AllowCommRequest { | ||
| zone_a: name.to_string(), | ||
| zone_b: allowed_zone.clone(), | ||
| }) | ||
| .await | ||
| { | ||
| warn!(zone = %name, peer = %allowed_zone, error = %error, "allow_comm failed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
In local-core mode this uses allow_comm for every allowed_zone listed in a single zone’s policy. In syva-core, allow_comm writes both directions (A→B and B→A) into ZONE_ALLOWED_COMMS (see syva-core/src/ebpf.rs), which means a one-sided allowed_zones entry grants bidirectional comms and weakens the intended “mutual allow” semantics used by CP reconcile. Also, this never issues deny_comm, so removing an entry from allowed_zones will not revoke previously-allowed comms. Consider reconciling comm pairs by: (1) deriving desired mutual pairs from all on-disk policies, (2) listing current pairs from core (ListComms), then (3) calling allow_comm/deny_comm to converge.
| for allowed_zone in &policy.policy.network.allowed_zones { | |
| if let Err(error) = core | |
| .allow_comm(AllowCommRequest { | |
| zone_a: name.to_string(), | |
| zone_b: allowed_zone.clone(), | |
| }) | |
| .await | |
| { | |
| warn!(zone = %name, peer = %allowed_zone, error = %error, "allow_comm failed"); | |
| } | |
| } | |
| // Do not call `allow_comm` from a single zone's `allowed_zones` entries. | |
| // In local-core mode `allow_comm` programs both directions, so applying it | |
| // here would turn a one-sided declaration into bidirectional communication. | |
| // Communication rules must instead be reconciled globally from the complete | |
| // policy set by deriving mutual pairs and converging core state with | |
| // allow/deny operations. |
|
Addressed the review comments in
Verification after the fixes: |
Summary
syva-core-client, a thin shared Unix-socket tonic client for the node-localsyva.core.v1API, so adapters do not duplicate connector code.syva-file,syva-k8s, andsyva-apibehind--core-socket, while keeping existing--cp-endpointCP mode intact.Design Notes
I used a new
syva-core-clientcrate rather than putting the connector insyva-protobecausesyva-protois currently just generated API types. Keeping transport/retry code in a sibling client crate mirrorssyva-cp-clientand avoids making the proto crate responsible for runtime client behavior.syva.core.v1.ZonePolicyhas typed fields only:host_paths,allowed_zones,allow_ptrace, andzone_type. Local-mode translations intentionally drop CP-onlydisplay_name,selector_json, and metadata. The API adapter also rejects unknown localpolicy_jsonfields so operators see the narrower local schema instead of assuming CP semantics still apply.Verification
Succeeded on this macOS host:
Attempted but blocked by host/toolchain, not by this patch:
Manual smoke test was not run because this machine is macOS and does not have Linux kernel BPF support.
Surprises
ZONE_TYPE_ISOLATED = 2;parse_proto_zone_typeaccepts only standard0and privileged1. Local adapter translations therefore mapisolatedto standard for now. This should be revisited when local core accepts the full proto enum.