Skip to content

fix(core): remove identity from /meta-data index#3905

Open
pbreton wants to merge 1 commit into
NVIDIA:mainfrom
pbreton:fix/NV6483573
Open

fix(core): remove identity from /meta-data index#3905
pbreton wants to merge 1 commit into
NVIDIA:mainfrom
pbreton:fix/NV6483573

Conversation

@pbreton

@pbreton pbreton commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Scope (not universal): only images whose cloud-init uses the EC2/IMDS datasource that crawls /meta-data/.

Root cause: get_metadata_params (crates/fmds/src/rest_server.rs:179-191) advertises identity, but serve_meta_data_identity (crates/dpu-fmds-shared/src/machine_identity.rs:525-531) returns 400 without a Metadata: true header. cloud-init crawls with plain GETs; a 400 on any advertised key is fatal (404 is tolerated) → empty metadata → KeyError: 'instance-id' (DataSourceEc2.py:242) → datasource rejected → DataSourceNone → no phone-home.

Fix: remove identity from the /meta-data/ index (keep the route for Metadata: true clients). Also add a regression test that the plain /meta-data/ index excludes identity.

Type of Change

  • Fix - Bug fixes

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated

Signed-off-by: Patrice Breton <pbreton@nvidia.com>
@pbreton
pbreton requested a review from a team as a code owner July 23, 2026 02:08
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • Bug Fixes
    • Updated the metadata listing to expose hostname, sitename, machine ID, instance ID, and ASN.
    • Removed the machine identity category from metadata index responses.
    • Ensured consistent results for both /meta-data and /meta-data/ endpoints.

Walkthrough

The /meta-data listings in the agent and FMDS services now expose hostname, sitename, machine-id, instance-id, and asn, while omitting the identity category. Related tests verify the updated ordering and absence of the identity entry.

Changes

Metadata listing contract

Layer / File(s) Summary
Update metadata listings and validation
crates/agent/src/instance_metadata_endpoint.rs, crates/fmds/src/rest_server.rs
Both handlers return the reduced category set, and tests verify the response contents for trailing-slash and non-trailing-slash requests.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: removing identity from the /meta-data index.
Description check ✅ Passed The description clearly matches the implemented fix and regression test for the /meta-data index behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/fmds/src/rest_server.rs (1)

535-567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a table-driven check for both metadata index variants.

The /meta-data and /meta-data/ assertions duplicate the same logic. Iterate over both paths so future contract changes cannot update one case while leaving the other stale.

Suggested refactor
-        let (status, body) = get_request(port, "meta-data").await;
-        assert_eq!(status, StatusCode::OK);
-        assert_eq!(body, expected);
-        assert!(
-            !body
-                .lines()
-                .any(|line| line == machine_identity::META_DATA_IDENTITY_CATEGORY),
-            "identity must not be listed in /meta-data/ (cloud-init EC2 crawl)"
-        );
-
-        // Also check with trailing slash (cloud-init compat).
-        let (status, body) = get_request(port, "meta-data/").await;
-        assert_eq!(status, StatusCode::OK);
-        assert_eq!(body, expected);
-        assert!(
-            !body
-                .lines()
-                .any(|line| line == machine_identity::META_DATA_IDENTITY_CATEGORY),
-            "identity must not be listed in /meta-data/ (cloud-init EC2 crawl)"
-        );
+        for path in ["meta-data", "meta-data/"] {
+            let (status, body) = get_request(port, path).await;
+            assert_eq!(status, StatusCode::OK);
+            assert_eq!(body, expected);
+            assert!(
+                !body
+                    .lines()
+                    .any(|line| line == machine_identity::META_DATA_IDENTITY_CATEGORY),
+                "identity must not be listed in {path} (cloud-init EC2 crawl)"
+            );
+        }
🤖 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 `@crates/fmds/src/rest_server.rs` around lines 535 - 567, Refactor
test_get_metadata_listing to iterate over both "meta-data" and "meta-data/"
paths in a table-driven loop. Keep the shared status, body, and
identity-exclusion assertions inside the loop, preserving the existing expected
response and cloud-init compatibility checks.

Source: Coding guidelines

🤖 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 `@crates/fmds/src/rest_server.rs`:
- Around line 535-567: Refactor test_get_metadata_listing to iterate over both
"meta-data" and "meta-data/" paths in a table-driven loop. Keep the shared
status, body, and identity-exclusion assertions inside the loop, preserving the
existing expected response and cloud-init compatibility checks.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0b8c5668-dbab-4fbf-8ae6-120a3fac5a7c

📥 Commits

Reviewing files that changed from the base of the PR and between 862b66a and 2255a45.

📒 Files selected for processing (2)
  • crates/agent/src/instance_metadata_endpoint.rs
  • crates/fmds/src/rest_server.rs
💤 Files with no reviewable changes (1)
  • crates/agent/src/instance_metadata_endpoint.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant