Skip to content

Provision default component policy for SimEnv - #4841

Merged
pcnudde merged 4 commits into
NVIDIA:mainfrom
pcnudde:feat/simenv-component-policy
Jun 30, 2026
Merged

Provision default component policy for SimEnv#4841
pcnudde merged 4 commits into
NVIDIA:mainfrom
pcnudde:feat/simenv-component-policy

Conversation

@pcnudde

@pcnudde pcnudde commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • centralize the standard exact-path class_allow_list and render it into server and client provisioning resources
  • provision the same policy into a new SimEnv job workspace when the effective resources file does not already define one
  • preserve explicit workspace policies and retain fail-closed handling for invalid policies
  • remove the integration-test-only resources.json prewrite
  • clarify in the security guide that provisioning supplies the policy while the authorizer itself has no fallback

Root cause

Non-BYOC jobs are checked by ComponentPathAuthorizer, but SimEnv did not provision the standard component policy used by PoC and production workspaces. The standalone NumPy cross-site evaluation example therefore failed in simulation unless a test or user manually created local/resources.json.

Impact

The documented standalone pretrained NumPy cross-site evaluation command now succeeds unchanged in SimEnv. Custom/BYOC handling is unchanged, no broad package prefix is introduced, and existing user policies remain authoritative.

@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a gap where SimEnv did not provision the standard class_allow_list component policy that PoC and production workspaces receive via provisioning, causing non-BYOC jobs (e.g. the NumPy cross-site evaluation example) to fail in simulation unless a resources.json was created manually. It also centralizes the previously duplicated allow-list into a single DEFAULT_CLASS_ALLOW_LIST constant.

  • nvflare/app_common/default_component_policy.py (new): single source of truth for the curated allow list; removes ~80-line duplication from the YAML template and tests.
  • nvflare/recipe/sim_env.py: adds _ensure_default_component_policy, which reads/creates local/resources.json.default before the simulator starts, preserving any user-supplied policy.
  • nvflare/lighter/impl/static_file.py + master_template.yml: replaces the two hardcoded inline lists in the provisioning template with a {~~class_allow_list~~} placeholder rendered from the shared constant.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to provisioning a curated allow list that already existed in the YAML template; no new classes are permitted beyond what production workspaces already allow.

The fix is straightforward: a duplicated hardcoded list is centralized, and the SimEnv code path that was missing the provisioning step now mirrors the behavior of PoC and production provisioners. All four branching cases in _ensure_default_component_policy are covered by unit tests, the integration test validates end-to-end execution, and the provisioning template test confirms the rendered output matches the constant.

No files require special attention.

Important Files Changed

Filename Overview
nvflare/app_common/default_component_policy.py New file: defines DEFAULT_CLASS_ALLOW_LIST as a single, canonical tuple of 81 allowed component paths — the single source of truth replacing two hardcoded copies.
nvflare/recipe/sim_env.py Adds _ensure_default_component_policy before simulator_run; correctly handles the four cases (no file, resources.json.default, resources.json with no policy, resources.json with explicit policy). Logic is clean and well-covered by the new unit tests.
nvflare/lighter/impl/static_file.py Passes class_allow_list as a template replacement for both server and client resources; json module already imported. Change is minimal and correctly wired.
nvflare/lighter/templates/master_template.yml Replaces 82-line hardcoded lists in local_client_resources and local_server_resources with the {class_allow_list} placeholder; existing provisioning tests verify the rendered output equals DEFAULT_CLASS_ALLOW_LIST.
tests/unit_test/recipe/sim_env_test.py New comprehensive test suite: bootstraps policy on fresh workspace, merges policy into existing resources (both file variants), preserves explicit policies (empty and non-empty), validates fail-closed for invalid resources, and confirms BYOC jobs are unaffected.
tests/integration_test/slow/recipe_system_test.py Removes the manual _write_simulator_resources helper and the tight allow-list export comparison; replaces with a post-run assertion that resources.json.default contains DEFAULT_CLASS_ALLOW_LIST, mirroring the new automatic provisioning.
tests/unit_test/lighter/static_file_builder_test.py Updates _extract_class_allow_list to handle the {class_allow_list} placeholder before parsing; adds test_build_renders_shared_default_component_policy to verify end-to-end provisioning output equals DEFAULT_CLASS_ALLOW_LIST.
docs/user_guide/admin_guide/security/unsafe_component_detection.rst Documentation updated to clarify that the authorizer has no implicit fallback (provisioning supplies the list) and that SimEnv now installs the curated list for new simulation workspaces.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[SimEnv.deploy job] --> B[_ensure_default_component_policy]
    B --> C{resources.json\nexists?}
    C -- Yes --> D[target = resources.json]
    C -- No --> E[target = resources.json.default]
    D --> F{target file\nexists?}
    E --> F
    F -- Yes --> G[json.load target_file]
    G --> H{valid dict?}
    H -- No --> I[raise ValueError\nfail-closed]
    H -- Yes --> J{CLASS_ALLOW_LIST\nalready present?}
    F -- No --> K[start with empty\nformat_version 2 dict]
    K --> J
    J -- Yes --> L[return\npreserve user policy]
    J -- No --> M[inject DEFAULT_CLASS_ALLOW_LIST]
    M --> N[write to target_file]
    N --> O[job.simulator_run]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[SimEnv.deploy job] --> B[_ensure_default_component_policy]
    B --> C{resources.json\nexists?}
    C -- Yes --> D[target = resources.json]
    C -- No --> E[target = resources.json.default]
    D --> F{target file\nexists?}
    E --> F
    F -- Yes --> G[json.load target_file]
    G --> H{valid dict?}
    H -- No --> I[raise ValueError\nfail-closed]
    H -- Yes --> J{CLASS_ALLOW_LIST\nalready present?}
    F -- No --> K[start with empty\nformat_version 2 dict]
    K --> J
    J -- Yes --> L[return\npreserve user policy]
    J -- No --> M[inject DEFAULT_CLASS_ALLOW_LIST]
    M --> N[write to target_file]
    N --> O[job.simulator_run]
Loading

Reviews (3): Last reviewed commit: "Merge branch 'main' into feat/simenv-com..." | Re-trigger Greptile

Comment thread tests/unit_test/recipe/sim_env_test.py
@codecov-commenter

codecov-commenter commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.41%. Comparing base (2005421) to head (61222d6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4841      +/-   ##
==========================================
+ Coverage   56.39%   56.41%   +0.02%     
==========================================
  Files         968      969       +1     
  Lines       92166    92192      +26     
==========================================
+ Hits        51976    52014      +38     
+ Misses      40190    40178      -12     
Flag Coverage Δ
unit-tests 56.41% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pcnudde commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the two Greptile test observations in 5b4de91:

  • removed the redundant hardcoded 81 policy-length assertion
  • parameterized the missing-policy merge test to cover both resources.json.default and higher-precedence resources.json

Validation: tests/unit_test/recipe/sim_env_test.py passes (11 tests), and ./runtest.sh -s passes.

@pcnudde
pcnudde enabled auto-merge (squash) June 30, 2026 18:42
@pcnudde
pcnudde merged commit 36d0763 into NVIDIA:main Jun 30, 2026
17 checks passed
nvidianz added a commit that referenced this pull request Jul 15, 2026
### Description

Cherry-pick of #4888 (merged to `2.8` as c3daf63) onto `main`,
bringing the class allow-list migration controls forward:

- support `"*"` in `class_allow_list` to allow all component classes,
ignore remaining entries, and record an audit event
- add `class_list_enforcement_mode` with `enforce` (default) and `warn`
behavior
- include the enforcement mode in provisioned client and server resource
templates
- use the curated built-in default allow list (with an audit event) when
a site does not configure `class_allow_list`
- unit coverage for wildcard, warn/enforce modes, site configuration
filtering, and provisioning defaults

**Conflict resolutions vs `main`** (which had diverged via #4841):

- `master_template.yml`: kept main's `{~~class_allow_list~~}`
placeholder (filled from `DEFAULT_CLASS_ALLOW_LIST` by the static file
builder) and added only the new `class_list_enforcement_mode` line to
both client and server resource blocks
- `default_component_policy.py`: kept main's copy (identical list);
updated its docstring since the authorizer now does use it as the
implicit default
- `static_file_builder_test.py`: kept main's
`DEFAULT_CLASS_ALLOW_LIST`-based assertion instead of the PR's inline
list; the new enforcement-mode assertion is included
- `unsafe_component_detection.rst`: adopted the PR's default-with-audit
semantics, keeping main's provisioning and `SimEnv` sentences

Note: this intentionally changes main's #4841 behavior for unconfigured
sites from "fail with an explicit setup error" to "use the curated
built-in default and record an audit event", matching 2.8. Main's
no-fallback tests were superseded by the PR's
`test_uses_default_allow_list_*` tests.

### Types of changes

- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [x] New tests added to cover the changes.
- [x] Quick tests passed locally by running `./runtest.sh`.
- [x] In-line docstrings updated.
- [x] Documentation updated.

Validation: 155 passed, 1 skipped across all unit suites referencing the
policy/authorizer; flake8 and black clean on changed files.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Peter Cnudde <pcnudde@nvidia.com>
@pcnudde
pcnudde deleted the feat/simenv-component-policy branch July 29, 2026 20:33
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.

3 participants