docs(nodectl): update setup guides and expose REST API externally#31
docs(nodectl): update setup guides and expose REST API externally#31Keshoid merged 15 commits intorelease/nodectl/v0.3.0from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates nodectl’s documentation and Helm chart to better support exposing the REST API externally, and adjusts the default HTTP bind address used in generated configs.
Changes:
- Change the default REST API bind address from
127.0.0.1:8080to0.0.0.0:8080, with a corresponding client-side bind rewrite fornodectl api. - Expand Helm Service and NetworkPolicy configurability for external access patterns.
- Add/extend operator documentation (first elections guide; manual staking instructions; external exposure guidance).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/node-control/common/src/app_config.rs |
Changes the default http.bind used by generated configs. |
src/node-control/commands/src/commands/nodectl/service_api_cmd.rs |
Adjusts how the CLI derives the base URL from http.bind. |
src/node-control/README.md |
Updates REST API config examples/defaults and adds manual staking docs. |
helm/nodectl/values.yaml |
Documents new service.* and renames NetworkPolicy allow-listing to allowFrom. |
helm/nodectl/templates/service.yaml |
Adds optional Service fields (clusterIP/LB IP/externalTrafficPolicy/nodePort/targetPort). |
helm/nodectl/templates/networkpolicy.yaml |
Switches ingress source configuration to generic NetworkPolicy peers via allowFrom. |
helm/nodectl/docs/setup.md |
Adds a guide section for safely exposing the REST API externally. |
helm/nodectl/docs/first-elections.md |
Adds a first-elections migration guide and references manual staking. |
helm/nodectl/README.md |
Links the new “first elections” guide from the chart docs index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| fn default_http_bind() -> String { | ||
| "127.0.0.1:8080".to_owned() | ||
| "0.0.0.0:8080".to_owned() |
There was a problem hiding this comment.
Changing the default HTTP bind from 127.0.0.1 to 0.0.0.0 makes the REST API listen on all interfaces by default. Since HttpConfig.auth defaults to None (all routes open), this is a security regression for non-Kubernetes deployments (any host-level exposure becomes unauthenticated by default). Consider keeping the default bind on loopback and configuring Helm (or config generate) to explicitly set 0.0.0.0 for probes, or enabling auth by default when binding to a non-loopback address.
| "0.0.0.0:8080".to_owned() | |
| "127.0.0.1:8080".to_owned() |
| let bind = app_cfg.http.bind.replace("0.0.0.0", "127.0.0.1"); | ||
| normalize_base_url(Cow::Owned(bind)) |
There was a problem hiding this comment.
ApiCmd::run now replaces "0.0.0.0" in the bind string before calling normalize_base_url, but normalize_base_url already contains logic to rewrite an unroutable 0.0.0.0 host to 127.0.0.1. This duplication makes URL normalization harder to reason about and String::replace will replace all occurrences (not just the host). Consider moving the improved behavior (handling both raw binds and http://0.0.0.0...) into normalize_base_url using a single, host-only rewrite (and ideally handling IPv6 :: as well), and remove the extra pre-processing here.
| let bind = app_cfg.http.bind.replace("0.0.0.0", "127.0.0.1"); | |
| normalize_base_url(Cow::Owned(bind)) | |
| normalize_base_url(Cow::Owned(app_cfg.http.bind.clone())) |
| ## REST API Endpoints | ||
|
|
||
| When running in service mode, nodectl exposes a REST API for monitoring and management. Protected endpoints require a JWT token in the `Authorization: Bearer <token>` header. See the **[Security Guide](./docs/nodectl-security.md)** for full details on roles, rate limiting, and token revocation. | ||
|
|
||
| ### Configuration | ||
|
|
||
| The HTTP server is configured in the `http` section of the config: | ||
|
|
||
| ```json | ||
| { | ||
| "http": { | ||
| "bind": "127.0.0.1:8080", | ||
| "bind": "0.0.0.0:8080", | ||
| "enable_swagger": true | ||
| } |
There was a problem hiding this comment.
The configuration snippet now shows http.bind: 0.0.0.0:8080 without an explicit warning that http.auth defaults to None (all routes open). With a non-loopback bind this can be read as “safe by default” and may lead users to unintentionally expose an unauthenticated management API. Please add a short warning here (and/or include an auth example) clarifying that binding publicly requires enabling auth and restricting network access.
helm/nodectl/docs/first-elections.md
Outdated
| [!CAUTION] | ||
| If the node's staking policy in `nodectl` is set to `split50` (the default), nodectl first calculates the total available funds for staking = frozen stake + pool's free balance + already submitted stake, then splits them in half and submits that amount to the current elections. However, since the Rust node does not have the validator key that the C++ node is currently using to validate in this round, nodectl cannot determine the frozen stake and will treat it as 0. **As a result, the stake amount will be half of what it should be.** |
There was a problem hiding this comment.
The [!CAUTION] admonition won’t render as a GitHub/MkDocs-style callout without being in a blockquote (e.g. > [!CAUTION]). As written, it will display as plain text and the warning may be missed.
| [!CAUTION] | |
| If the node's staking policy in `nodectl` is set to `split50` (the default), nodectl first calculates the total available funds for staking = frozen stake + pool's free balance + already submitted stake, then splits them in half and submits that amount to the current elections. However, since the Rust node does not have the validator key that the C++ node is currently using to validate in this round, nodectl cannot determine the frozen stake and will treat it as 0. **As a result, the stake amount will be half of what it should be.** | |
| > [!CAUTION] | |
| > If the node's staking policy in `nodectl` is set to `split50` (the default), nodectl first calculates the total available funds for staking = frozen stake + pool's free balance + already submitted stake, then splits them in half and submits that amount to the current elections. However, since the Rust node does not have the validator key that the C++ node is currently using to validate in this round, nodectl cannot determine the frozen stake and will treat it as 0. **As a result, the stake amount will be half of what it should be.** |
helm/nodectl/values.yaml
Outdated
| ## @param service.type Service type (`ClusterIP`, `NodePort`, `LoadBalancer`, `ExternalName`) | ||
| ## @param service.annotations [object] Annotations for the Service (e.g. cloud LB TLS settings) | ||
| ## @param service.nodePort [nullable] Node port number. Only used when `service.type` is `NodePort`. | ||
| ## @param service.clusterIP [nullable] Explicit ClusterIP. Set to `None` for a headless service. | ||
| ## @param service.loadBalancerIP [nullable] Static IP for the load balancer. Only used when `service.type` is `LoadBalancer`. | ||
| ## @param service.externalTrafficPolicy [nullable] External traffic policy (`Cluster` or `Local`). Only used when `service.type` is `NodePort` or `LoadBalancer`. |
There was a problem hiding this comment.
values.yaml now documents service.type as supporting ExternalName, but the Service template doesn’t render spec.externalName (and still includes selector/ports), which will produce an invalid Service if users set service.type: ExternalName. Either remove ExternalName from the documented options or add the required service.externalName value and conditionals in the template to render a valid ExternalName Service.
There was a problem hiding this comment.
ExternalName removed.
helm/nodectl/values.yaml
Outdated
|
|
||
| ## @param service.type Service type for the HTTP API | ||
| ## @param service.annotations [object] Annotations for the Service | ||
| ## @param service.type Service type (`ClusterIP`, `NodePort`, `LoadBalancer`, `ExternalName`) |
There was a problem hiding this comment.
The Helm port value controls only the Kubernetes side (Service, containerPort declaration, probes, NetworkPolicy). The actual port the app listens on comes from http.bind in config.json, generated by nodectl config generate with a compiled-in default (0.0.0.0:8080). If someone changes port here without updating the config inside the pod, probes and service routing break silently.
Consider adding a note here or in setup.md about this coupling — e.g. "This value must match the port in http.bind of your nodectl config."
|
|
||
| fn default_http_bind() -> String { | ||
| "127.0.0.1:8080".to_owned() | ||
| "0.0.0.0:8080".to_owned() |
There was a problem hiding this comment.
nit: the error fallback in http_server_task.rs:85-86 still uses 127.0.0.1:8080 while this default changed to 0.0.0.0:8080. Falling back to localhost on parse error is arguably safer (don't accidentally expose the API), but it would be worth a brief comment there explaining the intentional difference — otherwise it looks like a missed update.
There was a problem hiding this comment.
fixed - added a good description about that in docs.
- removed `ExternalName` from values.yaml - removed replacing of `0.0.0.0` in service_api_cmd.rs - removed top-level changelog - fixed CAUTION block
Summary