feat: rename forge.proto → core.proto + dual-listen on /forge.Forge and /core.Core#1770
feat: rename forge.proto → core.proto + dual-listen on /forge.Forge and /core.Core#1770shayan1995 wants to merge 2 commits into
Conversation
89da6f6 to
9e14774
Compare
|
/ok to test 533e60d |
989a934 to
6577ead
Compare
|
/ok to test 54e9462 |
6577ead to
bd61730
Compare
|
💥 |
|
The AI summary doesn't really match the PR. Example,
I don't see this code being changed in the PR. Anyway, this basically creates a parallel gRPC service named core instead of Forge, cool. I assume site-agent and the old admin-cli needs to get recompiled with the new protos which will happen after this lands? I don't see in this PR where the proto maps to a rust Impl for it, but maybe I missed it. And while this is happening, we have to update both old and new proto services. Is there no simpler way to have an "alias" for a service or something? |
bd61730 to
ac4bd2a
Compare
|
Thanks @ajf good catches on both fronts. On the AI summary being off: you're right, the description didn't match the actual PR. The mock-api-server change and the cross-reference proto edits were in an earlier revision and got reverted (they broke compilation and had inaccurate comments). The PR is actually just four files now: On client recompile: in-tree On the Rust impl: intentionally not here. On the alias question: and yes, you're right that maintaining both is a footgun. I did try the cleaner design: rewrite what we need to be worried about(catch) : it breaks ~4000 Rust references in this repo that import |
154a4f0 to
7904258
Compare
|
@ajf I went over the code with @shayan1995 and I think the best course of action would be to:
No breaking attribute renaming will take place in this PR, we can generate another smaller PR for that. |
7904258 to
c545379
Compare
… and /core.Core
Per review feedback, take the smallest possible step toward NICo:
* Rename forge.proto → core.proto
* Inside core.proto: package forge → package core
* Inside core.proto: service Forge → service Core
* In api/src/listener.rs, register an additional alias route so the
server accepts both /forge.Forge/* and /core.Core/* wire paths.
The alias rewrites /forge.Forge/<method> → /core.Core/<method>
before dispatch; payload bytes are untouched.
No message types, field names, enum values, or comments are renamed in
this PR — all those breaking attribute renames are deferred to a
smaller follow-up so reviewers can evaluate them independently.
Necessary scaffolding for the rename:
* crates/rpc/build.rs: update proto file path (proto/forge.proto →
proto/core.proto), reflection bin name (forge.bin → core.bin), and
type_attribute prefixes (forge.X → core.X) so the build script can
locate and codegen against the renamed proto.
* crates/rpc/src/protos/mod.rs: declare pub mod core (the generated
module from core.proto). Add a backward-compat alias module
pub mod forge { pub use super::core::*; ... } that re-exports
every item from core under the old forge name (including the
forge_server::Forge and forge_client::ForgeClient type aliases for
the renamed Core trait and CoreClient struct). This lets the
~4000 existing crate::protos::forge::* callsites in the workspace
keep compiling without modification; they get migrated to
crate::protos::core::* in a follow-up alongside the type-name
renames.
Wire compatibility: existing /forge.Forge/* clients (site-agent,
admin-cli, anything not yet rebuilt) continue to work unchanged —
the alias route rewrites their path before the generated CoreServer's
internal dispatch matches. New clients can target /core.Core/*
directly. Both paths are served by the same handler.
Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
The nico-api binary requires a Vault Kubernetes auth role at
auth/kubernetes/role/nico-api to authenticate via its ServiceAccount
JWT and read BMC credentials from the KV mount (used by SiteExplorer
for per-machine credential lookups).
Until now the role was opt-in (vault.nicoApiK8sAuth.enabled defaulted
to false), so every fresh setup.sh deploy hit a tight loop of:
level=ERROR msg="Detected errors in API response: [
\"invalid role name \\\"nico-api\\\"\",
]"
level=ERROR msg="SiteExplorer run failed due to:
Internal { message: \"Missing credential machines/bmc/site/root\" }"
every 30 seconds, with SiteExplorer unable to make progress on
endpoint exploration. Operators discovered this only after digging
into the binary's behavior — the chart silently shipped a role the
binary requires as disabled-by-default.
Flip the default to enabled: true. The role is bound to the
nico-api ServiceAccount in the nico-system namespace and granted
the existing nico-vault-policy (read access to {{ kvMount }}/data/*),
which matches what the binary expects.
Operators who explicitly want to disable can still override via
--set vault.nicoApiK8sAuth.enabled=false or values overrides.
BMC root credentials still need to be uploaded to
secrets/data/machines/bmc/site/root after the role is created; that
is a separate operational step not addressed here.
Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>
c545379 to
2899d46
Compare
feat: rename forge.proto → core.proto + dual-listen on /forge.Forge and /core.Core
Description
Smallest possible step toward NICo, per review feedback:
crates/rpc/proto/forge.proto→crates/rpc/proto/core.protopackage forge;→package core;service Forge {→service Core {crates/api/src/listener.rs, register an additional alias route so the server accepts both/forge.Forge/*and/core.Core/*wire paths. The alias rewrites/forge.Forge/<method>→/core.Core/<method>before dispatch; payload bytes are untouched.No message types, field names, enum values, or comments are renamed in this PR. All those breaking attribute renames are deferred to a smaller follow-up PR so reviewers can evaluate them independently.
Also flips
vault.nicoApiK8sAuth.enabled: false → trueinhelm-prereqs/values.yaml— fixes a tight-loop "invalid role name" error in freshsetup.shdeploys (separate commit on this branch, unrelated to the proto rename).Files changed
crates/rpc/proto/{forge.proto → core.proto}crates/rpc/build.rstype_attributeprefixes fromforge.Xtocore.Xcrates/rpc/src/protos/mod.rspub mod core;(the renamed generated module). Add a backward-compat aliaspub mod forge { pub use super::core::*; ... }so existingcrate::protos::forge::*callsites in the workspace keep compiling unchangedcrates/api/src/listener.rs/forge.Forge/*with atower::ServiceBuilder::map_requestpath-rewrite to/core.Core/*helm-prereqs/values.yamlvault.nicoApiK8sAuth.enabled: false → true(separate commit)Type of Change
/core.Core/*wire path, generatedcore::*Rust module)nicoApiK8sAuth.enabled)Related Issues (Optional)
Follow-up PRs in the rename rollout:
Forge*message types toCore*(ForgeAgentControl→CoreAgentControl, etc.)forge_*field names tocore_*(forge_dpu_agent→core_dpu_agent, etc.)crate::protos::forge::*Rust callsites tocrate::protos::core::*and drop the backward-compat alias moduleBreaking Changes
Wire-level:
/forge.Forge/*continues to work through the alias route. Existing clients (site-agent, admin-cli, anything not yet rebuilt) are unaffected.Source-level:
crate::protos::forge::*continues to resolve through the alias module inmod.rs. No existing Rust callsite needs to change.Testing
Manual validation:
protoccompile ofcore.proto: clean (only pre-existing unused-import warnings inherited from upstreamforge.proto).cargo check -p carbide-rpc -p carbide-tls -p carbide-api-db -p carbide-api-model -p carbide-rpc-utils -p carbide-measured-boot -p carbide-secrets: clean — the alias module inmod.rskeeps the downstreamcrate::protos::forge::*consumers compiling without modification.helm lint helm-prereqs/: clean.