docs: correct package path, endpoint count, and authentication scope - #519
docs: correct package path, endpoint count, and authentication scope#519ArangoGutierrez wants to merge 4 commits into
Conversation
The quickstart told readers that `make deb` and `make rpm` write to `dist/`
and to install `dist/topograph_*.deb`. Neither is true: scripts/build-deb.sh
and scripts/build-rpm.sh both default their output directory to
`${REPO_HOME}/bin`, and build-deb.sh names the artifact
`topograph-<version>.<arch>.deb` with a hyphen, not an underscore. A reader
following the page verbatim hit a glob that matched nothing, twice over.
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
The API section claimed Topograph exposes three endpoints and described only `/healthz`, `/v1/generate` and `/v1/topology`. initHttpServer in pkg/server/http_server.go registers five handlers: the two remaining ones are `/v1/lookup`, which returns a stored result keyed by the hash of the provider and engine sections of the payload, and `/metrics`, served by the Prometheus handler on the same port. Readers had no way to learn either endpoint existed from the API reference. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
The default-routing note listed the paths the Topograph Service serves behind a catch-all Gateway API rule and omitted /v1/lookup, which initHttpServer in pkg/server/http_server.go registers alongside the other four. An operator narrowing gatewayAPI.rules from that list would have dropped a route the binary actually serves. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
The authentication note named only /v1/generate. The API server applies no authentication to any handler: initHttpServer serves LoggingMiddleware over a private mux whose five registrations are the whole route set, the middleware only logs and records metrics, and no handler inspects credentials. The configured ssl.ca_cert is validated for existence and never used to verify a client certificate, so TLS here authenticates the server and not the caller. An operator reading the old sentence literally could enforce Gateway-layer authentication on /v1/generate alone and leave /v1/lookup, which returns stored topology output, open to unauthenticated reads. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 6 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (8)
🧰 Additional context used📓 Path-based instructions (2)New / changed API endpoint, request parameter, or response field | `docs/api.md` New / changed config schema (`topograph-config.yaml` fields, defaults, validation) | `docs/api.md`📄 CodeRabbit inference engine (AGENTS.md) Files:
Every `.md` file added to `docs/` (outside `docs/design/`) must also be added to `docs/index.yml`📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (5)
📝 SummarySummary by CodeRabbit
WalkthroughThe documentation adds ChangesAPI documentation
Slurm packaging documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This documentation-only change corrects API, Gateway, and Slurm package guidance to match current behavior, with no remaining merge-readiness risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis documentation-only PR corrects package installation paths, completes the API endpoint inventory, documents lookup and metrics behavior, and broadens the Kubernetes authentication warning to cover every endpoint.
Confidence Score: 5/5The PR appears safe to merge because the corrected documentation agrees with the current server, chart, and package-building implementations. No actionable discrepancies were found in the endpoint behavior, authentication scope, metrics inventory, or package paths documented by the PR. Important Files Changed
Reviews (1): Last reviewed commit: "docs(engines/k8s): state that no endpoin..." | Re-trigger Greptile |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-519.docs.buildwithfern.com/topograph |
1 participant
Description
Corrects three factual errors in the documentation. No scorecard signal is involved; these are
simply wrong.
1. The Slurm quickstart points at a directory that does not exist
docs/get-started/quickstart-slurm.mdtold readers the packages land indist/and to rundpkg -i dist/topograph_*.deb. Both halves are wrong:scripts/build-deb.sh:57andscripts/build-rpm.sh:37default to${REPO_HOME}/binscripts/build-deb.sh:58names the filetopograph-<ver>.<arch>.deb, with a hyphengrep -rn 'dist/' Makefile scripts/returns nothing. The documented glob therefore failedtwice over. Corrected to
bin/topograph-*.debandbin/topograph-*.rpm.2.
docs/api.mdundercounted the endpointsIt said topograph exposes three endpoints.
initHttpServerregisters five:/v1/generate,/v1/topology,/v1/lookup,/healthzand/metrics. The count is corrected and sectionsfor
/v1/lookupand/metricsare added in the file's existing style, leaving the existingsections numbered as they were.
Worth knowing, and now documented:
Request.Hashhashes only provider name and params plusengine name and params, so the
/v1/lookupkey is not the whole payload. Credentials and thenode list are excluded, which makes hits and misses surprising if you assume full-payload
matching.
3. The authentication note named only one endpoint
docs/engines/k8s.mdsaid the binary has no built-in authentication on/v1/generate. True,but incomplete in a way that matters now that
/v1/lookupis documented as public: no handlerauthenticates anything, so
/v1/lookupis equally open and returns stored topology. Anoperator could have read the old sentence literally and protected only
/v1/generate.The claim was verified before broadening it: the mux is a local variable with exactly five
registrations, nothing registers on
http.DefaultServeMuxrepo-wide,readRequestandvalidateinspect no credential,LoggingMiddlewarenever returns early, and there is nomTLS anywhere. Each of those searches was positive-controlled so an empty result means real
absence.
The surrounding Gateway-layer guidance is unchanged; it already carries the remedy.
Documentation only.
Part of #513.
Checklist
git commit -s).