docs: record the PXE and cloud-init client identity contract - #4744
Open
chet wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe pull request adds a design document that defines PXE and cloud-init client identity as the socket peer IP observed by ChangesPXE client identity
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕬᕬ ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4744.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/design/pxe_cloud_init_client_identity.md (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign primary sections with the architecture-document heading convention.
Use numbered primary headings such as
# 1. Repository-backed request path. Reserve H2 headings for subsections. Keep the document title at line 1.Based on learnings, architecture design documents under
docs/design/use a title followed by numbered primary sections.Also applies to: 72-72, 92-92, 135-135, 171-171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/pxe_cloud_init_client_identity.md` at line 16, Update the primary section headings in pxe_cloud_init_client_identity.md, including “Repository-backed request path” and the other referenced sections, to use numbered H1 headings in document order (for example, “# 1. …”). Keep the existing document title as the first line and convert subsection headings to H2 where needed.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/pxe_cloud_init_client_identity.md`:
- Around line 9-14: Update the identity investigation section to make issue
`#3902` an explicit final enablement gate: keep duplicate overlay addresses
disabled until the `#3887` ambiguity rejection is deployed, qualification evidence
passes, and issue `#3902` receives final sign-off.
- Around line 50-54: The documentation currently conflates absent schema fields
with fields that are present but unused. Update the discussion around
PxeInstructionRequest.interface_id to state that nico-pxe does not populate it,
and explicitly document that Core does not use it for client selection; preserve
the separate distinction for fields absent from CloudInitInstructionsRequest.
- Around line 145-156: Update the qualification checklist around the per-request
recording requirements to add correlation data for every request: use a unique
request marker where supported; otherwise record synchronized timestamps, the
complete connection tuple, translation state, node or pod identity, and response
status. Ensure these fields are captured consistently across all listed hops and
endpoints.
- Around line 88-90: Reconcile the identity contract documented by the protobuf
and Core handler comments with the canonical behavior described in this page:
state that the nico-pxe request extractors deliberately ignore forwarded
headers, or explicitly link those comments to this page as the authoritative
contract until updated. Ensure no source comment continues to claim that
forwarded headers are read when a proxy is present.
- Around line 106-129: Expand the documented response contract for each listed
PXE and cloud-init endpoint, covering no-match behavior, the ambiguity status
and message, HTTP status/body, cache policy, client-visible distinction from
no-match, and bounded operator diagnostics. Explicitly preserve vendor-data’s
lookup-free static response and generic templates for successful lookups with
missing payloads, and state that ambiguity selects no candidate and never falls
through to the other address domain.
---
Nitpick comments:
In `@docs/design/pxe_cloud_init_client_identity.md`:
- Line 16: Update the primary section headings in
pxe_cloud_init_client_identity.md, including “Repository-backed request path”
and the other referenced sections, to use numbered H1 headings in document order
(for example, “# 1. …”). Keep the existing document title as the first line and
convert subsection headings to H2 where needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 198bf154-f131-461d-b306-bde001b4023d
📒 Files selected for processing (1)
docs/design/pxe_cloud_init_client_identity.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/design/pxe_cloud_init_client_identity.md (1)
16-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s numbered H1 section structure.
The document uses H2 headings for primary sections. Change the primary sections to numbered H1 headings and reserve H2/H3 headings for subsections. Preserve the single unnumbered H1 title.
Based on learnings: architecture design documents under
docs/design/use one H1 title followed by numbered H1 primary sections and H2/H3 subsections.Also applies to: 72-73, 92-93, 135-136, 171-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/pxe_cloud_init_client_identity.md` around lines 16 - 17, Update the primary section headings in pxe_cloud_init_client_identity.md, including “Repository-backed request path” and the other referenced sections, to numbered H1 headings following the repository’s design-document structure. Preserve the existing single unnumbered H1 title and keep subsection headings at H2/H3 levels.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/design/pxe_cloud_init_client_identity.md`:
- Around line 16-17: Update the primary section headings in
pxe_cloud_init_client_identity.md, including “Repository-backed request path”
and the other referenced sections, to numbered H1 headings following the
repository’s design-document structure. Preserve the existing single unnumbered
H1 title and keep subsection headings at H2/H3 levels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9d86e249-4af5-4458-baef-9b268a074507
📒 Files selected for processing (1)
docs/design/pxe_cloud_init_client_identity.md
PXE and cloud-init resolve a client from the source IP observed by nico-pxe. That works while addresses are globally unique, but it cannot distinguish two clients using the same address in separate VPCs. Record the request path, the fail-closed Core contract, and the deployed network evidence required before duplicate addresses can be enabled. This keeps the implementation choice open until we know which trusted identity actually survives the DPU and gateway path. This supports NVIDIA#3888 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
gh-issue-3888-docs
branch
from
August 8, 2026 07:19
9ad8db0 to
932ef02
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PXE and cloud-init have gotten by with the source IP observed by
nico-pxeas the client identity because overlay addresses are globally unique. Once separate VPCs can reuse an address, that is no longer enough: repository code cannot tell us whether the deployed DPU and gateway path preserves a trustworthy per-VPC identity or returns the response through the correct VRF.So, this records what the code and manifests do prove, the generic fail-closed contract #3887 must provide, and the production-equivalent request/return captures required before duplicate addresses can be enabled. It deliberately leaves the protocol choice open until those captures show which component can provide and authenticate the smallest useful discriminator.
Related issues
This supports #3888
Part of #3883
Final enablement remains gated by #3902
Type of Change
Breaking Changes
Testing
Documentation validation:
rumdl check --config docs/.rumdl.toml AGENTS.md docs/design/pxe_cloud_init_client_identity.mdgit diff --checkAdditional Notes
The repository cannot provide the off-box DPU, gateway, anti-spoofing, or reply-VRF evidence, so the design note names the production-equivalent captures and owners needed to collect it. #3887 is still required as the fail-closed data-isolation backstop, but it is not enough to declare duplicate-address PXE/cloud-init usable by itself.
This stays under
docs/design/rather than the public documentation navigation because the deployment qualification is unresolved. I did not file the conditional runtime implementation Task yet: the capture is what tells us whether one is needed and which component it belongs to.