Add enhancement for NRI plugin mutation policy - #1971
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
9270ab5 to
84d10a6
Compare
amritansh1502
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
305d44f to
e2ef038
Compare
e2ef038 to
c7421ef
Compare
| # NRI plugin mutation policy | ||
|
|
||
| ## Summary | ||
|
|
There was a problem hiding this comment.
I think a small overview of what NRI is may be helpful for more context
|
|
||
| CRI-O merges NRI plugin adjustments and validates the combined result in a | ||
| single step. This enhancement proposes a small, standalone NRI policy plugin | ||
| not policy logic embedded in the CRI-O daemon that decides whether a |
There was a problem hiding this comment.
or if not policy logic embedded in the CRI-O daemon is a qualifier on the fact the policy plugin is standalone then you need commas surrounding it
| enable NRI and load the plugin. How that configuration is delivered and updated | ||
| on OpenShift (for example node provisioning / Machine Config versus cluster API | ||
| / Operator / CRD) is left to the enhancement design and alternatives. |
There was a problem hiding this comment.
I don't think we neeed this last sentence
There was a problem hiding this comment.
done, removed that sentence
| and manage its rules through the OpenShift-aligned path we agree in this | ||
| enhancement (node-level config and/or API-driven rollout), and to run |
There was a problem hiding this comment.
I would drop and manage its rules through the OpenShift-aligned path we agree in this enhancement (node-level config and/or API-driven rollout)
generally, I recommend treating this document as the authoritative source. We can debate what the details are, but whatever details are in this proposal are those being proposed (with alternatives listed). right now, the proposal is very weak: "we'll do this, if we decide to in the enhancement" . Make it stronger: "we'll do this"
There was a problem hiding this comment.
Dropped that language, user story now states the behavior directly without qualifying it on future decisions
| - As a cluster administrator, I want to deploy a standalone NRI policy plugin | ||
| and manage its rules through the OpenShift-aligned path we agree in this | ||
| enhancement (node-level config and/or API-driven rollout), and to run | ||
| permissive mode while tuning and strict mode when enforcing, so that I can |
There was a problem hiding this comment.
avoid first person. "a cluster admin can..."
There was a problem hiding this comment.
rewritten in third person: "A cluster administrator can.."
| None for Proposal A. Proposal B introduces a cluster-scoped CRD (exact | ||
| name, group, and version TBD pending API review) whose spec mirrors the | ||
| on-disk policy schema described below. |
There was a problem hiding this comment.
we'll be implementing both A and B, so instead of calling it A and B, call it Dev Preview and TP/GA
| the hosted control plane. Delivery via MachineConfig (Proposal A) or | ||
| DaemonSet (Proposal B) applies to the node pool as usual. |
There was a problem hiding this comment.
technically hypershift workers don't use machine configs but another method (config maps)
There was a problem hiding this comment.
Fixed , changed to "config maps" for HyperShift delivery path.
| On SNO the plugin runs on the single node. A MachineConfig rollout reboots | ||
| that node, causing a temporary cluster outage operators should schedule | ||
| policy changes during a maintenance window. MicroShift support is TBD. |
There was a problem hiding this comment.
we could consider shipping the validation plugin as a systemd service on microshift, but I am open to excluding these from this proposal
There was a problem hiding this comment.
Agreed , MicroShift is now marked out of scope, with a note that a future iteration could ship it as a systemd service.
| - Config is read once at startup; a plugin restart (node reboot for | ||
| Proposal A, DaemonSet pod restart for Proposal B) is required to pick up | ||
| changes. | ||
| - The `ALLOW_MUTATIONS_CONFIG` environment variable overrides the canonical |
There was a problem hiding this comment.
I recommend calling this OPENSHIFT_UNSUPPORTED_ALLOW_MUTATIONS_CONFIG or similar
|
|
||
| ### Risks and Mitigations | ||
|
|
||
| TBD |
There was a problem hiding this comment.
at minimum a risk is we're introducing a new moving part to an already complicated startup process
| | Condition | permissive mode | strict mode | | ||
| |---|---|---| | ||
| | Namespace matches no policy | warn, allow | reject | | ||
| | Namespace matches policy, `allowedMutations` empty | allow all | allow all | |
There was a problem hiding this comment.
it's confusing the empty list is treated like a disallowed list (if empty, none are disallowed) but if it has content it's an allowed list. I would recommend having empty mean none, and maybe a special keyword "all" or something to allow all.
There was a problem hiding this comment.
thanks, changed semantics so empty means "none allowed" and added "all" keyword to explicitly permit everything. Consistent allowlist behavior now.
| cluster. | ||
| - Unit test suite passing with no data races. | ||
| - Enhancement doc reviewed and merged. | ||
|
|
There was a problem hiding this comment.
DP-> TP involves creating the API, but the API is gated by the tech preview no upgrade feature set
There was a problem hiding this comment.
Added , CRD is introduced at TP and gated by TechPreviewNoUpgrade feature set
c7421ef to
956388e
Compare
|
|
||
| ### Goals | ||
|
|
||
| - Standalone NRI plugin that enforces namespace policy against **merged** container adjustments at the validation stage. |
There was a problem hiding this comment.
NRI does not only modify the OCI spec, but also could modify other files or settings.
Do we have any guardrail for those plugins?
There was a problem hiding this comment.
I suppose when we go the operator installation route the operator could block daemonsets from accessing hostpaths, but that's less on a per-container basis and more on a per-plugin
There was a problem hiding this comment.
That makes sense.
Anyway we should narrow down the scope like support NRI plugins that works without hostpaths (or read only access could be allowed?).
NRI plugin is too capable, and allowing all plugins is dangerous and not achievable, I guess.
There was a problem hiding this comment.
@amritansh1502
Can you clarify about the scope, specifically what NRI plugins are the targets?
At least ValidateContainerAdjustment can block the container spec change, but do we allow all other NRI hooks (e.g. RunPodSandbox)?
956388e to
a2ad84c
Compare
|
/assign |
haircommander
left a comment
haircommander
left a comment
There was a problem hiding this comment.
moving in the right direction!
| ### Risks and Mitigations | ||
|
|
||
| - The NRI policy plugin adds a new component to the container startup path. A bug or misconfiguration in the plugin could block container creation on affected nodes. | ||
| - Mitigation: the plugin never rejects containers outright. It strips disallowed mutations and logs what was stripped, so operators can observe the effect before tightening the allowed list. |
There was a problem hiding this comment.
I think you added policies to branch here, mitigation may not apply depending on configuration
There was a problem hiding this comment.
updated, the risk now explicitly says "a misconfigured policy could strip mutations that a workload depends on" rather than implying every configuration is risky. The mitigation clarifies the plugin only strips, and logs are available to diagnose misconfigured policies.
|
|
||
| For Tech Preview the file-based MachineConfig delivery path is replaced by the **NRI Plugins Operator** ([`nri-plugins-operator`](https://github.com/amritansh1502/nri-plugins-operator)). The operator is the single supported entry point for NRI plugin deployment on OpenShift. An administrator creates an `NriPlugin` CR to deploy a plugin and optionally creates an `NriMutationPolicy` CR to define namespace-scoped mutation policy. | ||
|
|
||
| Out of the box a cluster has no NRI plugins installed. When the first `NriPlugin` CR is created, the operator enables NRI in CRI-O, waits for the MachineConfig rollout, deploys the requested plugin as a DaemonSet, and automatically deploys the allow-mutations validation plugin alongside it. Mutation policy is defined via `NriMutationPolicy` CRs; namespaces with no matching policy pass through untouched. |
There was a problem hiding this comment.
I do think we as openshift devs may want a path to install nri plugins without going through the CR. NRI operator should handle this situation gracefully I think
There was a problem hiding this comment.
Added, the enhancement now states: "The operator does not assume exclusive ownership of NRI plugins on the node. Plugins deployed outside the operator (e.g. manually installed DaemonSets) are left untouched, the operator only manages plugins created through NriPlugin CRs."
|
|
||
| | Field | Type | Required | Description | | ||
| |---|---|---|---| | ||
| | `spec.pluginName` | string (enum) | yes | NRI plugin to deploy. One of: `topology-aware`, `balloons`, `memtierd`, `memory-qos`, `sgx-epc`. | |
There was a problem hiding this comment.
hm over time the enum will be hard to maintain, as we'll have to change the API each time we want to allow a new plugin. maybe this should just be a string
There was a problem hiding this comment.
Agreed, changed pluginName from an enum to a plain string
| | Field | Type | Required | Description | | ||
| |---|---|---|---| | ||
| | `spec.pluginName` | string (enum) | yes | NRI plugin to deploy. One of: `topology-aware`, `balloons`, `memtierd`, `memory-qos`, `sgx-epc`. | | ||
| | `spec.image` | string | yes | Full container image reference for the plugin (registry, repo, tag). | |
There was a problem hiding this comment.
what does the operator do with an image? create a daemonset for it? should we expose additional ds configurations?
There was a problem hiding this comment.
Updated the field description to clarify, "Container image for the plugin. The operator deploys a DaemonSet using this image on targeted worker nodes.
whether we want the full DaemonSet customization exposed now?
|
|
||
| | Field | Type | Description | | ||
| |---|---|---| | ||
| | `status.phase` | string | Current lifecycle phase: `Pending`, `EnablingNRI`, `WaitingForMCO`, `DeployingPlugin`, `Running`, or `Degraded`. | |
There was a problem hiding this comment.
this is an orthoganal thing: I would kinda love it if we didn't make it fully openshift specific so other entities may want to use it, but I do recognize there's tension between that desire and actually getting it to run on openshift. not a -1 on having a MCO specific phase, but something to think about in design: could we have it run on vanilla k8s and openshift?
There was a problem hiding this comment.
that is so great! , the allow-mutations plugin itself is already runtime-agnostic . The only OpenShift-specific part is the MachineConfig delivery to enable NRI on nodes. One approach to support vanilla k8s: make the MachineConfig step conditional , the operator checks if the MCO CRD exists on the cluster. If yes (OpenShift), create MachineConfig as usual. If no (vanilla k8s), skip it and assume the admin has already enabled NRI in their runtime config, then go straight to deploying the DaemonSet. Does that approach sound reasonable to you?
There was a problem hiding this comment.
the other option is the operator could not be responsible for turning on NRI and it could be turned on by default, then it needs nothing to do with mco
| 2. Waits for the MachineConfig Operator to finish rebooting worker nodes (polls MachineConfigPool `worker` status every 30 seconds). | ||
| 3. Creates the necessary RBAC and SecurityContextConstraints for the plugin pod. | ||
| 4. For plugins with `pluginConfigCR`: creates the referenced config CR with the provided spec. | ||
| 5. Deploys the requested plugin as a DaemonSet (`nri-<pluginName>`) on all targeted worker nodes. Plugin pods run privileged with host networking and mount the NRI socket. |
There was a problem hiding this comment.
how do we make it secure that a user will be able to specify an image and have it deployed with privilege?
There was a problem hiding this comment.
NriPlugin is a cluster-scoped resource, so only users with cluster-admin level can create one , regular users cannot specify an image and have it deployed. The operator also creates a dedicated ServiceAccount and SecurityContextConstraints per plugin, scoped to the privileges the plugin actually needs.
do you think we need more stronger controls ?
| 2. Waits for the MachineConfig Operator to finish rebooting worker nodes (polls MachineConfigPool `worker` status every 30 seconds). | ||
| 3. Creates the necessary RBAC and SecurityContextConstraints for the plugin pod. | ||
| 4. For plugins with `pluginConfigCR`: creates the referenced config CR with the provided spec. | ||
| 5. Deploys the requested plugin as a DaemonSet (`nri-<pluginName>`) on all targeted worker nodes. Plugin pods run privileged with host networking and mount the NRI socket. |
There was a problem hiding this comment.
there is a little bit of a tricky timing problem here: a user who requires a nri plugin to be running would need to make sure other pods are not scheduled until that plugin is installed. we don't really provide a mechanism to do that transparantly. that's a problem the node-readiness-controller is handling, but we don't package it in openshift today.
There was a problem hiding this comment.
this is a real gap. If a workload is scheduled on a node before the NRI plugin DaemonSet is ready, the container starts without the expected adjustments. for now, the operator reports readyNodes vs desiredNodes in the CR status so an admin can verify plugin readiness before deploying workloads.
do you think we need to address this problem within this proposal?
There was a problem hiding this comment.
It should be noted somewhere in this enhancement (probably risks and mitigations?)
a2ad84c to
7024a00
Compare
| ### Implementation Details/Notes/Constraints | ||
|
|
||
| - The plugin binary is statically compiled and has no runtime dependencies beyond the NRI socket provided by CRI-O. | ||
| - Config is read once at startup; a plugin restart (node reboot for Dev Preview, DaemonSet pod restart for TP/GA) is required to pick up changes. |
There was a problem hiding this comment.
I think this is justified because the NRI operator rollout the new daemonset, correct?
Can you add the explanation here.
|
|
||
| ### Goals | ||
|
|
||
| - Standalone NRI plugin that enforces namespace policy against **merged** container adjustments at the validation stage. |
There was a problem hiding this comment.
@amritansh1502
Can you clarify about the scope, specifically what NRI plugins are the targets?
At least ValidateContainerAdjustment can block the container spec change, but do we allow all other NRI hooks (e.g. RunPodSandbox)?
|
|
||
| ### Drawbacks | ||
|
|
||
|
|
There was a problem hiding this comment.
Is it empty or a parent of ### Enforcement point?
|
|
||
| ### Enforcement point | ||
|
|
||
| The plugin hooks into `ValidateContainerAdjustment`, which runs after CRI-O merges adjustments from all NRI plugins. At this stage the full combined adjustment is visible, unlike `CreateContainer` where each plugin only sees its own changes. The plugin does not register `CreateContainer` at all—CRI-O's NRI stub discovers the registered methods automatically. |
There was a problem hiding this comment.
The plugin does not register
CreateContainerat all—CRI-O's NRI stub discovers the registered methods automatically.
I'm a bit confused about why CreateContainer is mentioned here.
Does this paragraph essentially want to say "We hook ValidateContainerAdjustment (not CreateContainer) because that's the only point where the full merged adjustment is visible." Correct?
There was a problem hiding this comment.
exactly right, simplified the section. It now just says we hook ValidateContainerAdjustment because that's where the full merged adjustment is visible. Removed the CreateContainer mention since it was adding confusion without adding value.
| 2. the policy file at `/etc/crio/nri_plugins/AllowMutations/config.yaml`; | ||
| 3. a CRI-O drop-in under `/etc/crio/crio.conf.d/` that enables NRI and registers the plugin. | ||
|
|
||
| - Policy changes require a new MachineConfig rollout—MCO drains and reboots nodes one at a time. This is the accepted cost of Day Zero delivery. The plugin strips disallowed mutations and logs what was stripped, so operators can tune the `allowedMutations` list and observe the effect before tightening policy. |
There was a problem hiding this comment.
Is the operation really limited to Day Zero?
After operating the cluster some time, they may start wanting to add a new policy.
There was a problem hiding this comment.
fair point, updated. The doc now says the MachineConfig rollout cost applies to both initial setup and any subsequent policy updates, not just Day Zero. This is one reason TP/GA moves to operator-managed delivery where policy changes only need a DaemonSet restart, no node reboot.
| 2. Waits for the MachineConfig Operator to finish rebooting worker nodes (polls MachineConfigPool `worker` status every 30 seconds). | ||
| 3. Creates the necessary RBAC and SecurityContextConstraints for the plugin pod. | ||
| 4. For plugins with `pluginConfigCR`: creates the referenced config CR with the provided spec. | ||
| 5. Deploys the requested plugin as a DaemonSet (`nri-<pluginName>`) on all targeted worker nodes. Plugin pods run privileged with host networking and mount the NRI socket. |
There was a problem hiding this comment.
It should be noted somewhere in this enhancement (probably risks and mitigations?)
| | Field | Type | Required | Description | | ||
| |---|---|---|---| | ||
| | `spec.pluginName` | string | yes | Name of the NRI plugin to deploy. | | ||
| | `spec.image` | string | yes | Container image for the plugin. The operator deploys a DaemonSet using this image on targeted worker nodes. | | ||
| | `spec.nodeSelector` | map[string]string | no | Targets which nodes get this plugin. Tech Preview supports worker nodes only. | | ||
| | `spec.pluginConfigCR` | object | no | For plugins that read config from a Kubernetes CR (topology-aware, balloons, etc.). The operator creates the referenced CR with the provided spec passed through as-is. | |
There was a problem hiding this comment.
Users may want to configure some envvars, volumes, etc.
It should be as expressive as daemonset spec.
I'm wondering if we can stop deploying daemonsets from this operator, and just reference the existing plugin daemonset.
There was a problem hiding this comment.
i am looking at this options
- expand the CRD to expose commonly needed pod spec fields (env, resources, tolerations, volumes)
why it si risky? @haircommander @bitoku
There was a problem hiding this comment.
Any reason we want to restrict the fields? I'm wondering if we have the full pod spec in the CRD.
Why did we want to deploy the daemonset from our operator in the first place?
There was a problem hiding this comment.
the original motivation was convenience, a single CR to deploy a plugin without writing DaemonSet YAML, RBAC, SCC manually.
But you're right, maintaining a shadow DaemonSet API isn't sustainable. For TP/GA we can choose this path : - the operator only handles NRI enablement (MachineConfig) and the allow-mutations policy plugin
There was a problem hiding this comment.
Yeah I think we can think about it during TP.
| When an `NriPlugin` CR is created the operator reconciles in this order: | ||
|
|
||
| 1. Creates a shared MachineConfig (`99-nri-enable`) that drops a CRI-O config file on every worker node to enable NRI and set the NRI socket path. | ||
| 2. Waits for the MachineConfig Operator to finish rebooting worker nodes (polls MachineConfigPool `worker` status every 30 seconds). |
There was a problem hiding this comment.
Have you ever explored the possibility where we don't have to reboot the node?
There was a problem hiding this comment.
yeah a reload should be possible here
| 3. Creates the necessary RBAC and SecurityContextConstraints for the plugin pod. | ||
| 4. For plugins with `pluginConfigCR`: creates the referenced config CR with the provided spec. | ||
| 5. Deploys the requested plugin as a DaemonSet (`nri-<pluginName>`) on all targeted worker nodes. Plugin pods run privileged with host networking and mount the NRI socket. | ||
| 6. Deploys the allow-mutations validation plugin as a DaemonSet (`nri-allow-mutations`) if not already running, configured by `NriMutationPolicy` CRs. |
There was a problem hiding this comment.
Does it mean only nodes with NRI plugins configured have nri-allow-mutations plugin?
There was a problem hiding this comment.
yes, nri-allow-mutations is deployed only on nodes targeted by NriPlugin CRs (same nodeSelector).
7024a00 to
a8cc695
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughChangesThe enhancement proposal defines a standalone NRI mutation-policy plugin. It filters merged container adjustments by ordered namespace policies and specifies MachineConfig and Operator-managed delivery, lifecycle behavior, APIs, testing, upgrades, and graduation criteria. NRI mutation-policy plugin
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CRI-O
participant NRI
participant MutationPolicyPlugin
NRI->>MutationPolicyPlugin: ValidateContainerAdjustment
MutationPolicyPlugin->>MutationPolicyPlugin: Match namespace policy
MutationPolicyPlugin->>MutationPolicyPlugin: Strip disallowed mutation categories
MutationPolicyPlugin-->>NRI: Return adjusted result
NRI-->>CRI-O: Apply merged container adjustment
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
enhancements/machine-config/nri-plugin-mutation-policy.md (1)
327-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisambiguate the repeated
ProsandConsheadings.Use section-specific headings such as
Tech Preview ProsandTech Preview Cons. This avoids duplicate Markdown headings and improves document navigation.🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 327 - 334, Rename the repeated Pros and Cons headings in the NRI plugin mutation policy document to section-specific headings, such as Tech Preview Pros and Tech Preview Cons, while preserving the listed content.Source: Linters/SAST tools
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md`:
- Around line 58-60: Update the delivery-stage references in the enhancement
document, including the Hypershift / Hosted Control Planes section and the Tech
Preview → GA criteria, to classify DaemonSet/operator delivery as Tech Preview
only. Remove GA wording for this path and keep the GA ContainerRuntimeConfig/MCO
approach outside the committed scope.
- Around line 192-199: Address the scheduling gap in the machine-config mutation
policy by adding a node-readiness gate or taint-based rollout barrier that
prevents workloads from starting on targeted nodes until the plugin is ready. If
the implementation remains observational via status.readyNodes, explicitly
document the limitation and add tests covering workload scheduling before plugin
readiness.
- Around line 104-105: Resolve the allowedMutations contract consistently across
the policy schema, evaluation table, implementation behavior, and related tests:
choose one meaning for an empty or omitted list, ensure ["all"] follows the
documented explicit allow-all behavior, and update
TestEmptyAllowedMutationsStripsAll plus the referenced policy documentation and
evaluation logic to match. Keep namespace matching and other mutation-category
behavior unchanged.
- Line 179: Update the NRI ownership and cleanup design in the machine-config
mutation policy so deleting the last managed NriPlugin does not remove shared
NRI enablement when pre-existing configuration or externally deployed plugins
exist. Define how the operator records ownership of NRI-related MachineConfig
resources and only removes resources it created, preserving externally owned
configuration; align the referenced mixed-CPU node plugin proposal with this
ownership rule.
- Around line 201-212: Update the NriMutationPolicy lifecycle and status
behavior so a policy cannot remain Active when no allow-mutations validator
DaemonSet exists after the last NriPlugin is deleted. Ensure enforcement remains
available, reject the unsupported policy state, or set phase to Degraded, and
apply the same consistency to the status behavior described in the later
policy-status section.
- Around line 98-105: Define deterministic handling for multiple
NriMutationPolicy objects in the documented operator behavior: either require a
singleton resource or specify stable object ordering, duplicate namespace
precedence, and deletion semantics. Align the merge behavior with the ordered
first-match-wins rules for policies[] and add tests covering multiple objects,
duplicates, and deletion.
- Around line 420-436: Replace the “Version Skew Strategy” TBD section with an
explicit Tech Preview version-skew contract covering supported CRI-O, NRI,
plugin, and operator version combinations, policy-schema compatibility across
upgrades and downgrades, and the expected failure behavior during mixed-version
rollouts. Align the contract with the upgrade, downgrade, and rollout behavior
described in the surrounding “Upgrade / Downgrade Strategy” section.
- Around line 146-152: Align the Tech Preview deployment and test configuration
with the plugin’s canonical `/etc/crio/nri_plugins/AllowMutations/config.yaml`
path by mounting the operator policy ConfigMap there and updating all related
assertions. Do not rely on the testing-only
`OPENSHIFT_UNSUPPORTED_ALLOW_MUTATIONS_CONFIG` override for production; document
or configure an explicit supported production override only if the operator path
must remain `/config/config.yaml`.
- Around line 192-212: Update the API type block to define the missing top-level
NriPlugin resource and add a Status field with an NriMutationPolicyStatus type
to NriMutationPolicy, including phase, policyCount, and conditions. Ensure the
documented types match all referenced CRD status fields; alternatively,
explicitly label the entire type block as illustrative.
- Line 79: Update the unmatched-namespace descriptions at the references to
lines 140 and 373 to state that ValidateContainerAdjustment runs as a
pass-through/no-op and returns the adjustment unchanged. Remove wording that
says there is no hook or that the plugin did not hook.
---
Nitpick comments:
In `@enhancements/machine-config/nri-plugin-mutation-policy.md`:
- Around line 327-334: Rename the repeated Pros and Cons headings in the NRI
plugin mutation policy document to section-specific headings, such as Tech
Preview Pros and Tech Preview Cons, while preserving the listed content.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7690c2a-00bc-4107-bc7b-e8b4cee75b9c
📒 Files selected for processing (1)
enhancements/machine-config/nri-plugin-mutation-policy.md
| #### Hypershift / Hosted Control Planes | ||
|
|
||
| The plugin runs on data-plane worker nodes only and does not interact with the hosted control plane. Delivery via config maps (Dev Preview) or DaemonSet (TP/GA) applies to the node pool as usual. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep the GA delivery path out of this enhancement's committed scope.
The PR objective says the GA ContainerRuntimeConfig and MCO approach is out of scope. This document presents DaemonSet/operator delivery as TP/GA and uses it in the Tech Preview -> GA criteria. Mark the operator path as Tech Preview only, or update the PR objective.
Also applies to: 405-411
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 58 -
60, Update the delivery-stage references in the enhancement document, including
the Hypershift / Hosted Control Planes section and the Tech Preview → GA
criteria, to classify DaemonSet/operator delivery as Tech Preview only. Remove
GA wording for this path and keep the GA ContainerRuntimeConfig/MCO approach
outside the committed scope.
| - The plugin binary is statically compiled and has no runtime dependencies beyond the NRI socket provided by CRI-O. | ||
| - Config is read once at startup; a node reboot (Dev Preview) or a DaemonSet pod restart (TP/GA, triggered automatically by the operator when an NriMutationPolicy CR is updated) is required to pick up changes. | ||
| - The `OPENSHIFT_UNSUPPORTED_ALLOW_MUTATIONS_CONFIG` environment variable overrides the canonical config path for unit-test and development use only. | ||
| - The plugin registers only `ValidateContainerAdjustment`; CRI-O's NRI stub discovers this automatically from the exported methods. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)nri-plugin-mutation-policy\.md$' || true
echo "== relevant snippets =="
if [ -f enhancements/machine-config/nri-plugin-mutation-policy.md ]; then
wc -l enhancements/machine-config/nri-plugin-mutation-policy.md
sed -n '60,90p' enhancements/machine-config/nri-plugin-mutation-policy.md
sed -n '125,155p' enhancements/machine-config/nri-plugin-mutation-policy.md
sed -n '360,380p' enhancements/machine-config/nri-plugin-mutation-policy.md
echo "== occurrences =="
rg -n 'No hook|did not hook|hook|Unmatched|matched|namespace|ValidateContainerAdjustment' enhancements/machine-config/nri-plugin-mutation-policy.md
fiRepository: openshift/enhancements
Length of output: 12402
🌐 Web query:
CRI-O NRI plugin ValidateContainerAdjustment returns adjustments unchanged unmatched namespace hook is invoked
💡 Result:
The scenario where a Node Resource Interface (NRI) ValidateContainerAdjustment hook reports "adjustments unchanged" alongside an "unmatched namespace" warning typically relates to how NRI validating plugins inspect container change requests [1][2]. In the NRI lifecycle, validating plugins are invoked after all mutating plugins have proposed adjustments [1][2]. These validating plugins receive the ValidateContainerAdjustmentRequest containing the pending adjustments [3]. The validator then reviews these changes—such as modifications to Linux namespaces, seccomp policies, or OCI hooks—against defined security policies [1][2]. An "adjustments unchanged" status in this context often indicates that the validating plugin examined the proposed changes and determined they did not require modification or rejection, or that no relevant changes were found in the scope being validated [1][2]. If a warning or error regarding an "unmatched namespace" appears simultaneously, it generally points to a discrepancy between the container's actual runtime namespace configuration and the configuration changes requested or expected by the NRI plugin [1][2]. To resolve or investigate this behavior: 1. Check the Default Validator Configuration: If you are using the CRI-O built-in default validator, ensure it is not misconfigured. You can check settings such as --nri-validator-reject-namespace-adjustment in your CRI-O configuration [4][5]. 2. Audit Plugin Logic: If you are using a custom NRI plugin, inspect its ValidateContainerAdjustment implementation [3][6]. The "unmatched namespace" message likely stems from the plugin's internal logic comparing the LinuxContainerAdjustment struct in the request against the pod/container state [3]. 3. Verify Namespace Policies: Ensure that any namespace adjustments requested by mutating plugins comply with the runtime security policies enforced by your validating plugins [1][2]. For detailed configuration parameters, refer to the CRI-O documentation for NRI flags [4][5] and the containerd NRI documentation for validator plugin behavior [7][1].
Citations:
- 1: https://github.com/containerd/nri
- 2: https://github.com/containerd/nri/blob/main/README.md
- 3: https://github.com/containerd/nri/blob/main/pkg/api/api.proto
- 4: https://github.com/cri-o/cri-o/blob/main/docs/crio.8.md
- 5: https://github.com/cri-o/cri-o/blob/v1.35.2/docs/crio.8.md
- 6: https://github.com/containerd/nri/blob/main/plugins/logger/nri-logger.go
- 7: https://containerd.io/docs/main/nri/
🌐 Web query:
CRI-O source ValidateContainerAdjustment NRI plugin if adapter has no plugins return request Adjustments unchanged
💡 Result:
In the context of CRI-O and the Node Resource Interface (NRI), the ValidateContainerAdjustment mechanism is designed to allow validating plugins to approve or reject proposed container changes [1][2]. When no NRI plugins are registered, or if the plugins do not request any modifications, the runtime implementation aims to "short-circuit" this process to avoid unnecessary overhead [3][4]. Specifically, the system has been optimized to handle cases where there are no effective adjustments. By returning a "nil" or empty adjustment response when no plugins are present—or when none of the present plugins touch a container—the runtime can skip the processing of adjustments [3][4]. This prevents the runtime from going through the motions of applying empty or null configurations, which was previously a source of performance and logic issues [3][4][5]. When this short-circuiting logic is active, the container request remains unchanged because the runtime treats the absence of an adjustment as a "no-op" [3]. This ensures that container creation proceeds with its original, pristine specification rather than being inadvertently modified by empty data structures [3][5]. This behavior is part of the broader effort within CRI-O and containerd to ensure that NRI processing is efficient and that only actual, requested changes are applied to containers [3][4][6][7].
Citations:
- 1: https://github.com/containerd/nri/blob/main/README.md
- 2: https://github.com/containerd/nri/blob/main/pkg/api/api.proto
- 3: adaptation: return nil as no-op adjustment. containerd/nri#118
- 4: cri/nri: short-circuit nil adjustment. containerd/containerd#12574
- 5: fixes: fix resource adjustment to properly ignore unset/unadjusted fields. containerd/nri#28
- 6: nri: update NRI to v0.10.0. cri-o/cri-o#9415
- 7: server, nri: bump NRI dependencies to v0.11.0. cri-o/cri-o#9673
🌐 Web query:
site:github.com/cri-o/cri-o ValidateContainerAdjustment NRI plugin list
💡 Result:
There is no single plugin named "ValidateContainerAdjustment" in the Node Resource Interface (NRI). Instead, the term refers to the validator component integrated into container runtimes like CRI-O, which manages and enforces constraints on how NRI plugins can adjust container configurations [1][2]. In CRI-O, this mechanism is controlled via a built-in default validator plugin [3]. Administrators can configure this validator to reject specific types of container adjustments made by external NRI plugins using the following command-line flags or corresponding configuration file settings [1][2]: --nri-validator-reject-custom-seccomp-adjustment: Rejects adjustments to custom seccomp policies [1][2]. --nri-validator-reject-namespace-adjustment: Rejects adjustments to Linux namespaces [1][2]. --nri-validator-reject-oci-hook-adjustment: Rejects adjustments to OCI hooks [1][2]. --nri-validator-reject-runtime-default-seccomp-adjustment: Rejects adjustments to runtime default seccomp policies [1][2]. --nri-validator-reject-unconfined-seccomp-adjustment: Rejects adjustments to unconfined seccomp policies [1][2]. Additionally, the validator allows you to define a list of mandatory plugins using --nri-validator-required-plugins, and you can specify an annotation to tolerate missing required plugins via --nri-validator-tolerate-missing-plugins-annotation [1][2]. This functionality is part of the NRI integration in CRI-O to ensure secure and controlled container resource management [4]. You can enable the default validator by setting --nri-enable-default-validator [1][2].
Citations:
- 1: https://github.com/cri-o/cri-o/blob/main/docs/crio.8.md
- 2: https://github.com/cri-o/cri-o/blob/v1.35.2/docs/crio.8.md
- 3: nri: update NRI to v0.10.0. cri-o/cri-o#9415
- 4: Add support for latest NRI with extended scope. cri-o/cri-o#5318
🌐 Web query:
github.com/cri-o/cri-o internal nri hook validatecontaineradjustment loop plugins
💡 Result:
In the context of CRI-O and the Node Resource Interface (NRI), ValidateContainerAdjustment is an API event defined in the NRI specification (starting from NRI v0.10.0) used to enforce cluster-wide or node-wide boundary conditions on container configuration [1][2][3]. It is not a "loop" in the sense of a recurring execution cycle, but rather a transactional validation step invoked during container creation [4]. Key technical aspects include: 1. Purpose and Semantics: When NRI-enabled plugins propose adjustments to a container during creation, the ValidateContainerAdjustment interface allows other plugins—specifically "validating" plugins—to approve or deny these changes [4]. If any validating plugin rejects the adjustment, the container creation process fails, and all associated changes are rolled back [4]. 2. Integration in CRI-O: CRI-O integrated support for NRI v0.10.0, which includes the ValidateContainerAdjustment event [5]. The integration allows for the use of a built-in "default validator" plugin that can be configured to enforce policies such as rejecting unauthorized OCI hook injections or enforcing mandatory plugin requirements [4][6][5]. 3. Implementation Context: While CRI-O manages the NRI lifecycle, the validation logic is separate from the container creation hot path [7]. Recent updates to CRI-O have focused on ensuring that plugin synchronization and event processing do not introduce deadlocks or livelocks [8]. For example, CRI-O uses mechanisms like BlockPluginSync to control plugin interactions during sensitive lifecycle transitions, ensuring that container state remains consistent during adjustments [8]. In summary, the functionality is a gatekeeping mechanism in the NRI plugin architecture, not a recursive loop. The integration within CRI-O ensures that any validation failure prevents the container from being successfully created, maintaining the desired cluster state [4].
Citations:
- 1: https://github.com/containerd/nri/blob/main/pkg/api/api.proto
- 2: https://dependabot.ecosyste.ms/hosts/GitHub/repositories/cri-o%2Fcri-o/issues/9417
- 3: containerd/nri@v0.8.0...v0.10.0
- 4: https://github.com/containerd/nri
- 5: nri: update NRI to v0.10.0. cri-o/cri-o#9415
- 6: https://containerd.io/docs/main/nri/
- 7: https://github.com/cri-o/cri-o/blob/master/server/container_create.go
- 8: nri: fix a deadlock and a livelock, block plugin sync during other event processing cri-o/cri-o#9835
🌐 Web query:
github.com/containerd/nri ValidateContainerAdjustment plugin invoke return adjustments unchanged
💡 Result:
In the Node Resource Interface (NRI) for containerd, the ValidateContainerAdjustment interface is designed to allow validating plugins to approve or reject proposed adjustments to a container [1][2]. When a plugin implements ValidateContainerAdjustment, it is expected to return a response indicating whether it accepts or rejects the adjustments [3]. If a plugin is satisfied with the proposed adjustments and does not wish to reject them, it should return a response that does not set the reject field to true (typically an empty ValidateContainerAdjustmentResponse or simply nil in some implementations, depending on the language bindings/stub) [3][4][5]. Returning a response that does not signal a rejection effectively leaves the proposed container adjustments unchanged by that specific plugin [1]. Key points regarding this process include: 1. Transactional Semantics: Validation is transactional; if any validating plugin rejects an adjustment, the container creation fails, and no changes from any other plugin are applied [1][2]. 2. Purpose: The ValidateContainerAdjustment call is used to enforce node- or cluster-wide constraints. For instance, the default built-in NRI validator uses this interface to conditionally block adjustments like OCI hook injection or specific seccomp policy changes [1][6]. 3. No-op Handling: If no adjustments are proposed, or if a plugin has no modifications to make, it may return a nil adjustment or a neutral response, which acts as a no-op [7][8]. The container runtime has been optimized to short-circuit and skip processing if the returned adjustment is nil, effectively leaving the container state as is [7][8]. In summary, a plugin returns an "unchanged" status for adjustments by not setting the rejection flag in its ValidateContainerAdjustment response, thereby allowing the requested changes (or the base container configuration) to proceed [3][1].
Citations:
- 1: https://github.com/containerd/nri/blob/main/README.md
- 2: https://github.com/containerd/nri
- 3: https://github.com/containerd/nri/blob/main/pkg/api/api.proto
- 4: https://github.com/containerd/nri/blob/main/plugins/wasm/plugin.go
- 5: https://github.com/containerd/nri/blob/main/plugins/logger/nri-logger.go
- 6: https://containerd.io/docs/main/nri/
- 7: adaptation: return nil as no-op adjustment. containerd/nri#118
- 8: containerd/containerd@3a717c1
🌐 Web query:
CRI-O v1.35.2 pkg/nri hooks ValidateContainerAdjustment implementations.go listHooks
💡 Result:
In CRI-O, the Node Resource Interface (NRI) is used to manage container adjustments, with the ValidateContainerAdjustment event serving as a key mechanism for plugins to validate or reject requested container changes [1][2][3]. The specific function ValidateContainerAdjustment exists within the NRI DefaultValidator implementation, which is responsible for enforcing security restrictions on container adjustments such as OCI hook injection, seccomp policy changes, Linux namespace modifications, and sysctl adjustments [1][2][4]. The term listHooks as requested does not appear to be a standard or core component of the CRI-O NRI implementation; it is commonly associated with repository hook management APIs in other software projects (e.g., GitHub, Gitea) [5][6][7][8] or unrelated JavaScript hook management libraries [9][10]. In CRI-O, OCI hooks are configured via directory paths (defaulting to /usr/share/containers/oci/hooks.d) and are managed separately from the NRI subsystem [11][12]. For authoritative technical details regarding the NRI validation logic, you should refer to the containerd/nri repository for the interface definitions and the cri-o/cri-o repository for its server-side integration [2][13][14]. The ValidateContainerAdjustment method typically handles requests containing information about container changes, ownership, and involved plugins to ensure compliance with configured security policies [1][4].
Citations:
- 1: https://github.com/containerd/nri/blob/main/README.md
- 2: https://github.com/containerd/nri
- 3: https://newreleases.io/project/github/containerd/nri/release/v0.10.0
- 4: thaJeztah/nri@96f0f20
- 5: https://github.com/google/go-github/blob/master/github/repos_hooks.go
- 6: https://github.com/google/go-github/blob/v17.0.0/github/repos_hooks.go
- 7: https://github.com/go-gitea/gitea/blob/v1.8.3/routers/api/v1/repo/hook.go
- 8: https://github.com/mrexodia/git/blob/master/repo_hook.go
- 9: https://github.com/seanchatmangpt/unrdf/blob/main/packages/hooks/src/index.mjs
- 10: https://github.com/seanchatmangpt/unrdf/blob/main/packages/hooks/src/hooks/hook-management.mjs
- 11: https://github.com/cri-o/cri-o/blob/v1.35.2/docs/crio.8.md
- 12: https://github.com/cri-o/cri-o/blob/main/docs/crio.8.md
- 13: https://github.com/cri-o/cri-o/blob/master/server/server.go
- 14: server, nri: bump NRI dependencies to v0.11.0. cri-o/cri-o#9673
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate vendored/external NRI or CRI-O nri references =="
rg -n 'ValidateContainerAdjustment|nri|NRI|Validate()' go.mod go.sum 2>/dev/null || true
echo "== inspect external references if present =="
for d in . .github; do
[ -d "$d" ] && find "$d" -maxdepth 3 -type f \( -name 'go.mod' -o -name 'go.sum' -o -name 'vendor.mod' \) -print
doneRepository: openshift/enhancements
Length of output: 294
🌐 Web query:
github.com/containerd/nri ValidateContainerAdjustmentRequest Adjustments unchanged returned when plugin does not reject
💡 Result:
In the containerd Node Resource Interface (NRI), ValidateContainerAdjustmentRequest is a gRPC call used during the container creation process to allow validating plugins to approve or reject proposed container adjustments [1][2][3]. When a container is created, NRI operates in two phases: 1. Mutating: Plugins propose adjustments (e.g., modifying annotations, mounts, environment variables, or resource limits) [2][4]. 2. Validating: Plugins receive all requested changes and can either reject the adjustment or allow it to proceed [2][3][4]. The ValidateContainerAdjustmentRequest includes the container's state, pending adjustments, and information about the plugins involved [1]. A validating plugin responds with a ValidateContainerAdjustmentResponse, which contains a boolean reject field and a string reason [1]. If a plugin does not reject the adjustments (i.e., it returns a response with reject set to false), the adjustments are considered approved by that plugin. The NRI framework has transactional semantics; if any single validating plugin rejects an adjustment, the container creation fails and none of the proposed adjustments are applied [2][3][4]. If no plugins reject the adjustment, the process continues [2]. While the NRI API does not explicitly return a message specifically stating "Adjustments unchanged," a successful validation where no rejections occur simply allows the container creation to proceed with the proposed adjustments [2].
Citations:
- 1: https://github.com/containerd/nri/blob/main/pkg/api/api.proto
- 2: https://github.com/containerd/nri/blob/main/README.md
- 3: https://github.com/containerd/nri
- 4: https://pkg.go.dev/github.com/containerd/nri
🌐 Web query:
"unmatched namespace" "No hook" "allowedMutations" "NriMutationPolicy"
💡 Result:
The error "unmatched namespace" associated with "No hook allowed" and "NriMutationPolicy" refers to a restriction mechanism within the Node Resource Interface (NRI) in containerd [1][2]. NRI allows administrators to exercise fine-grained control over changes made to containers by plugins [1]. Within this framework, specific plugins can be configured to restrict or allow certain operations, such as OCI hook injection, seccomp policy adjustments, or namespace modifications [1][2]. If an NRI plugin or policy attempts to modify a container configuration—such as injecting an OCI hook—that the active NRI policy (e.g., NriMutationPolicy) has restricted for a given context or namespace, the operation will be rejected, resulting in an "unmatched namespace" or "No hook allowed" error [1][3][2]. For example, when using the NVIDIA Container Toolkit or similar tools that utilize NRI, checks are often implemented to ensure that CDI (Container Device Interface) devices or other resources are only injected into pods within authorized namespaces [3]. If a mutation request is made for a pod in a namespace not included in the allowed list defined by the NRI policy, the runtime will reject the request because the namespace is "unmatched" for that specific operation [3]. If you are encountering this error, verify your NRI plugin configuration to ensure that the target namespace is permitted for the specific mutation being attempted, such as OCI hook injection or device assignment [1][3].
Citations:
- 1: https://github.com/containerd/nri/
- 2: https://github.com/containerd/nri/blob/main/README.md
- 3: [nri] only restrict management CDI devices to the toolkit namespace NVIDIA/nvidia-container-toolkit#1854
Describe unmatched namespaces as a no-op, not as “no hook.”
The plugin registers ValidateContainerAdjustment, so unmatched namespaces still invoke the hook and return the adjustment unchanged. Replace “No hook”/“did not hook” at lines 140 and 373 with pass-through/no-op wording.
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` at line 79, Update
the unmatched-namespace descriptions at the references to lines 140 and 373 to
state that ValidateContainerAdjustment runs as a pass-through/no-op and returns
the adjustment unchanged. Remove wording that says there is no hook or that the
plugin did not hook.
| The on-disk file and any future CR **spec** share the same shape so plugin behavior is identical regardless of delivery path (file-based or Operator-managed). `policies[]` is evaluated in order—first matching entry wins. | ||
|
|
||
| #### Field reference | ||
|
|
||
| | Field | Type | Required | Description | | ||
| |---|---|---|---| | ||
| | `spec.policies[].namespaceSelector.matchNames` | []string | yes | Exact namespace names this policy entry covers. | | ||
| | `spec.policies[].allowedMutations` | []string | no | Mutation categories permitted for matching namespaces. If empty or omitted, all mutation types are allowed. Use `["all"]` to explicitly permit all categories. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define deterministic merge semantics for multiple NriMutationPolicy objects.
The plugin contract uses ordered policies[] entries with first-match-wins behavior. The operator instead says that it combines multiple NriMutationPolicy objects, but it does not define object ordering, duplicate namespace handling, or deletion behavior. Use a singleton policy resource, or specify deterministic merge rules and tests.
Also applies to: 201-212
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 98 -
105, Define deterministic handling for multiple NriMutationPolicy objects in the
documented operator behavior: either require a singleton resource or specify
stable object ordering, duplicate namespace precedence, and deletion semantics.
Align the merge behavior with the ordered first-match-wins rules for policies[]
and add tests covering multiple objects, duplicates, and deletion.
| | `spec.policies[].namespaceSelector.matchNames` | []string | yes | Exact namespace names this policy entry covers. | | ||
| | `spec.policies[].allowedMutations` | []string | no | Mutation categories permitted for matching namespaces. If empty or omitted, all mutation types are allowed. Use `["all"]` to explicitly permit all categories. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the contradictory empty-list semantics.
The schema and evaluation table say an empty allowedMutations list allows every mutation. TestEmptyAllowedMutationsStripsAll says the same input strips every mutation. A previous review also defined empty as “none allowed” and ["all"] as the explicit allow-all value. Align the schema, evaluation table, implementation contract, and tests.
Also applies to: 136-144, 352-359
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 104 -
105, Resolve the allowedMutations contract consistently across the policy
schema, evaluation table, implementation behavior, and related tests: choose one
meaning for an empty or omitted list, ensure ["all"] follows the documented
explicit allow-all behavior, and update TestEmptyAllowedMutationsStripsAll plus
the referenced policy documentation and evaluation logic to match. Keep
namespace matching and other mutation-category behavior unchanged.
| ### Dev Preview: file-based delivery | ||
|
|
||
| - Ship the standalone NRI policy plugin and its policy file as node configuration. CRI-O enables NRI and loads the plugin through the normal NRI integration path. | ||
|
|
||
| - At validation time, the plugin evaluates the workload namespace against the configured rules (see **Policy schema (v1)**). The plugin reads config once at startup from a fixed canonical path. An `OPENSHIFT_UNSUPPORTED_ALLOW_MUTATIONS_CONFIG` environment variable overrides the path for testing only. | ||
|
|
||
| - Policy is read from the single canonical path: `/etc/crio/nri_plugins/AllowMutations/config.yaml`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate file"
fd -a 'nri-plugin-mutation-policy\.md$' . || true
echo "## Relevant document sections"
FILE="$(fd 'nri-plugin-mutation-policy\.md$' . | head -n1)"
if [ -n "${FILE:-}" ]; then
wc -l "$FILE"
sed -n '130,165p' "$FILE"
echo "----"
sed -n '190,215p' "$FILE"
echo "----"
sed -n '375,392p' "$FILE"
fi
echo "## Search ConfigMap mount/config paths"
rg -n "config.yaml|/config|/etc/crio/nri_plugins|OPENSHIFT_UNSUPPORTED_ALLOW_MUTATIONS_CONFIG|configMap|AllowMutations" -S . || trueRepository: openshift/enhancements
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="enhancements/machine-config/nri-plugin-mutation-policy.md"
echo "## Relevant section outline around unit tests/config path"
sed -n '340,390p' "$FILE"
echo "----"
sed -n '280,330p' "$FILE"
echo "## All exact path references in this file"
python3 - <<'PY'
from pathlib import Path
text = Path("enhancements/machine-config/nri-plugin-mutation-policy.md").read_text()
for path in ["/etc/crio/nri_plugins/AllowMutations/config.yaml", "/config/config.yaml"]:
print(path, text.count(path))
for i, line in enumerate(text.splitlines(), 1):
if path in line:
print(f"{i}: {line}")
PYRepository: openshift/enhancements
Length of output: 6323
Mount the operator policy ConfigMap where the plugin reads it.
The plugin reads /etc/crio/nri_plugins/AllowMutations/config.yaml, but the Tech Preview test requires the ConfigMap to be mounted at /config/config.yaml. Update the Tech Preview path and test to use the plugin’s canonical config path, or document a supported production override for the operator path.
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 146 -
152, Align the Tech Preview deployment and test configuration with the plugin’s
canonical `/etc/crio/nri_plugins/AllowMutations/config.yaml` path by mounting
the operator policy ConfigMap there and updating all related assertions. Do not
rely on the testing-only `OPENSHIFT_UNSUPPORTED_ALLOW_MUTATIONS_CONFIG` override
for production; document or configure an explicit supported production override
only if the operator path must remain `/config/config.yaml`.
|
|
||
| Out of the box a cluster has no NRI plugins installed. When the first `NriPlugin` CR is created, the operator enables NRI in CRI-O, waits for the MachineConfig rollout, deploys the requested plugin as a DaemonSet, and automatically deploys the allow-mutations validation plugin alongside it. Mutation policy is defined via `NriMutationPolicy` CRs; namespaces with no matching policy pass through untouched. | ||
|
|
||
| The operator does not assume exclusive ownership of NRI plugins on the node. Plugins deployed outside the operator (e.g. manually installed DaemonSets) are left untouched, the operator only manages plugins created through `NriPlugin` CRs. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not remove shared NRI enablement based only on managed CR count.
The document says externally deployed plugins remain untouched, but deleting the last managed NriPlugin removes the shared MachineConfig and disables NRI cluster-wide. The related enhancements/node-tuning/mixed-cpu-node-plugin.md proposal also uses MachineConfig to enable NRI. Track ownership of pre-existing NRI configuration and remove only resources owned by this operator.
Also applies to: 285-300
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` at line 179,
Update the NRI ownership and cleanup design in the machine-config mutation
policy so deleting the last managed NriPlugin does not remove shared NRI
enablement when pre-existing configuration or externally deployed plugins exist.
Define how the operator records ownership of NRI-related MachineConfig resources
and only removes resources it created, preserving externally owned
configuration; align the referenced mixed-CPU node plugin proposal with this
ownership rule.
| Status fields: | ||
|
|
||
| | Field | Type | Description | | ||
| |---|---|---| | ||
| | `status.phase` | string | Current lifecycle phase: `Pending`, `EnablingNRI`, `WaitingForMCO`, `DeployingPlugin`, `Running`, or `Degraded`. | | ||
| | `status.readyNodes` | int32 | Number of nodes where the plugin pod is running. | | ||
| | `status.desiredNodes` | int32 | Total number of targeted nodes. | | ||
| | `status.conditions` | []Condition | Standard Kubernetes conditions. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add a scheduling barrier for plugin readiness.
MCO completion and DaemonSet creation do not ensure that the plugin pod is ready. A workload can start on a targeted node before the plugin is running and bypass the expected adjustment. readyNodes is observational only. Add a node-readiness gate or taint-based rollout barrier, or document this as an explicit limitation and test it.
Also applies to: 285-295
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 192 -
199, Address the scheduling gap in the machine-config mutation policy by adding
a node-readiness gate or taint-based rollout barrier that prevents workloads
from starting on targeted nodes until the plugin is ready. If the implementation
remains observational via status.readyNodes, explicitly document the limitation
and add tests covering workload scheduling before plugin readiness.
| Status fields: | ||
|
|
||
| | Field | Type | Description | | ||
| |---|---|---| | ||
| | `status.phase` | string | Current lifecycle phase: `Pending`, `EnablingNRI`, `WaitingForMCO`, `DeployingPlugin`, `Running`, or `Degraded`. | | ||
| | `status.readyNodes` | int32 | Number of nodes where the plugin pod is running. | | ||
| | `status.desiredNodes` | int32 | Total number of targeted nodes. | | ||
| | `status.conditions` | []Condition | Standard Kubernetes conditions. | | ||
|
|
||
| #### NriMutationPolicy CRD | ||
|
|
||
| The operator introduces a second cluster-scoped CRD (`nri.openshift.io/v1alpha1`, kind `NriMutationPolicy`) for namespace-scoped mutation policy. Mutation policy is managed separately from plugin deployment, so administrators can change policy without touching plugin CRs. | ||
|
|
||
| | Field | Type | Required | Description | | ||
| |---|---|---|---| | ||
| | `spec.policies[].namespaceSelector.matchNames` | []string | yes | Exact namespace names this policy entry covers. | | ||
| | `spec.policies[].allowedMutations` | []string | no | Mutation categories permitted for matching namespaces. Same values as Policy schema (v1). If empty or omitted, all mutation types are allowed. Use `["all"]` to explicitly permit all categories. | | ||
|
|
||
| The operator watches `NriMutationPolicy` CRs and marshals the combined policy into a ConfigMap mounted into the auto-deployed allow-mutations plugin. Policy changes trigger a rolling restart of the allow-mutations DaemonSet — no node reboot required. | ||
|
|
||
| Status reports `phase` (`Active` or `Degraded`), `policyCount` (number of policy entries), and standard Kubernetes `conditions`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="enhancements/machine-config/nri-plugin-mutation-policy.md"
if [ -f "$file" ]; then
echo "== lines 160-300 =="
sed -n '160,300p' "$file" | nl -ba -v160
else
echo "File not found: $file"
fi
echo
echo "== search for top-level NriPlugin/NriMutationPolicy type blocks/status =="
rg -n "### .*NriPlugin|### .*NriMutationPolicy|NriPluginSpec|NriPluginStatus|NriMutationPolicyStatus|NriMutationPolicy" "$file" || true
echo
echo "== structural extraction of type blocks =="
python3 - <<'PY'
from pathlib import Path
import re
p=Path("enhancements/machine-config/nri-plugin-mutation-policy.md")
lines=p.read_text().splitlines()
for i,l in enumerate(lines,1):
if re.match(r'#{3,}\s+.*Nri(Policy|Plugin|MutationPolicy)($|\s)', l, re.I) or re.match(r'#{3,}\s+.*Status($|\s)', l):
print(f"{i}: {l}")
PYRepository: openshift/enhancements
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="enhancements/machine-config/nri-plugin-mutation-policy.md"
if [ -f "$file" ]; then
echo "== lines 160-300 =="
sed -n '160,300p' "$file"
else
echo "File not found: $file"
fi
echo
echo "== search for top-level NriPlugin/NriMutationPolicy type blocks/status =="
rg -n "### .*NriPlugin|### .*NriMutationPolicy|NriPluginSpec|NriPluginStatus|NriMutationPolicyStatus|NriMutationPolicy|status:" "$file" || true
echo
echo "== structural extraction of Nri/status headings =="
python3 - <<'PY'
from pathlib import Path
import re
p=Path("enhancements/machine-config/nri-plugin-mutation-policy.md")
lines=p.read_text().splitlines()
for i,l in enumerate(lines,1):
if re.search(r'#{3,}\s+.*(Nri\.?Policy|NriPlugin|NriMutationPolicy|Status)(\s|$)', l):
print(f"{i}: {l}")
PYRepository: openshift/enhancements
Length of output: 11307
Add the missing CRD status types/status field or mark the type block illustrative.
The API type block defines NriPluginSpec, PluginConfigCR, NriPluginStatus, NriMutationPolicySpec, PolicyEntry, NamespaceSelector, and NriMutationPolicy, but it does not define a top-level NriPlugin type. It also documents NriMutationPolicy status fields without adding Status or NriMutationPolicyStatus. Add those types/field so the referenced CRD contract matches, or state that the API type block is illustrative only.
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 192 -
212, Update the API type block to define the missing top-level NriPlugin
resource and add a Status field with an NriMutationPolicyStatus type to
NriMutationPolicy, including phase, policyCount, and conditions. Ensure the
documented types match all referenced CRD status fields; alternatively,
explicitly label the entire type block as illustrative.
| #### NriMutationPolicy CRD | ||
|
|
||
| The operator introduces a second cluster-scoped CRD (`nri.openshift.io/v1alpha1`, kind `NriMutationPolicy`) for namespace-scoped mutation policy. Mutation policy is managed separately from plugin deployment, so administrators can change policy without touching plugin CRs. | ||
|
|
||
| | Field | Type | Required | Description | | ||
| |---|---|---|---| | ||
| | `spec.policies[].namespaceSelector.matchNames` | []string | yes | Exact namespace names this policy entry covers. | | ||
| | `spec.policies[].allowedMutations` | []string | no | Mutation categories permitted for matching namespaces. Same values as Policy schema (v1). If empty or omitted, all mutation types are allowed. Use `["all"]` to explicitly permit all categories. | | ||
|
|
||
| The operator watches `NriMutationPolicy` CRs and marshals the combined policy into a ConfigMap mounted into the auto-deployed allow-mutations plugin. Policy changes trigger a rolling restart of the allow-mutations DaemonSet — no node reboot required. | ||
|
|
||
| Status reports `phase` (`Active` or `Degraded`), `policyCount` (number of policy entries), and standard Kubernetes `conditions`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep policy status aligned with enforcement state.
NriMutationPolicy is described as independently managed, but deleting the last NriPlugin removes the allow-mutations DaemonSet. A surviving policy can therefore report Active while no validator enforces it. Keep the validator alive, reject this state, or mark the policy Degraded.
Also applies to: 297-300
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 201 -
212, Update the NriMutationPolicy lifecycle and status behavior so a policy
cannot remain Active when no allow-mutations validator DaemonSet exists after
the last NriPlugin is deleted. Ensure enforcement remains available, reject the
unsupported policy state, or set phase to Degraded, and apply the same
consistency to the status behavior described in the later policy-status section.
| ## Upgrade / Downgrade Strategy | ||
|
|
||
| **Dev Preview** | ||
|
|
||
| Plugin binary and config are delivered via MachineConfig. Upgrading means applying a new MachineConfig with the updated binary and config file, which triggers an MCO rollout (drain and reboot per node). Downgrading means reverting or removing the MachineConfig, which also triggers a rollout. | ||
|
|
||
| **Tech Preview** | ||
|
|
||
| - *Plugin Upgrade*: update the `image` field in the `NriPlugin` CR. The operator updates the DaemonSet pod template and performs a rolling restart no node reboot required. | ||
| - *Policy change*: update the `NriMutationPolicy` CR spec. The operator updates the ConfigMap and triggers a rolling restart of the allow-mutations DaemonSet. | ||
| - *Operator upgrade*: standard Deployment rollout. The new controller picks up existing `NriPlugin` CRs and reconciles them. The shared MachineConfig (`99-nri-enable`) persists across operator upgrades, it only enables NRI in CRI-O and does not change between versions. | ||
| - *Downgrade*: revert the operator Deployment image. The controller reconciles existing CRs. If the CRD schema changed between versions, the administrator must ensure CR compatibility before downgrading. | ||
| - Deleting all `NriPlugin` CRs removes the shared MachineConfig and disables NRI cluster-wide. Re-creating a CR re-enables NRI and triggers another MachineConfig rollout. | ||
|
|
||
| ## Version Skew Strategy | ||
|
|
||
| TBD |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate target file and relevant version skew content"
if git ls-files | grep -Fxq "enhancements/machine-config/nri-plugin-mutation-policy.md"; then
echo "Target exists"
wc -l enhancements/machine-config/nri-plugin-mutation-policy.md
sed -n '400,450p' enhancements/machine-config/nri-plugin-mutation-policy.md
echo "--- version skew occurrences ---"
rg -n -i "version skew|skew|compatibility|CRI-O|NRI|downgrade|migration|schema|compat|upgrade|reboot|drain" enhancements/machine-config/nri-plugin-mutation-policy.md
else
echo "Target file not found"
fiRepository: openshift/enhancements
Length of output: 17583
Define the version-skew contract before Tech Preview.
The Version Skew Strategy remains TBD, while the document defines plugin, policy, operator, and downgrade paths. Specify supported CRI-O/NRI/plugin/operator versions, policy-schema compatibility, and failure behavior for mixed-version rollouts.
🤖 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 `@enhancements/machine-config/nri-plugin-mutation-policy.md` around lines 420 -
436, Replace the “Version Skew Strategy” TBD section with an explicit Tech
Preview version-skew contract covering supported CRI-O, NRI, plugin, and
operator version combinations, policy-schema compatibility across upgrades and
downgrades, and the expected failure behavior during mixed-version rollouts.
Align the contract with the upgrade, downgrade, and rollout behavior described
in the surrounding “Upgrade / Downgrade Strategy” section.
|
@amritansh1502: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Description:
Problem
OpenShift nodes with NRI can apply merged container adjustments from more than one plugin, so administrators need an explicit rule for which namespaces may receive that combined result and whether enforcement is strict or observe-only. Policy cannot be defined reliably in a single plugin's CreateContainer path because that view does not represent the fully merged adjustment.
Proposal:
This enhancement proposes a standalone NRI policy plugin evaluated at CRI-O's ValidateContainerAdjustment hook, with namespace-scoped rules in v1.
Approach: A Dev Preview (MachineConfig delivery, in scope for this enhancement)
Ship the standalone policy plugin and its policy YAML on worker nodes via MachineConfig/Ignition, and enable NRI through a CRI-O drop-in that loads the plugin. The plugin reads the canonical on-disk policy at /etc/crio/nri_plugins/AllowMutations/config.yaml and enforces it at ValidateContainerAdjustment against the merged adjustment. This approach allows early feedback to be gathered on the policy API shape without committing to a final delivery mechanism.
Approach B: (GA (out of scope, tracked separately):
Based on reviewer feedback, the agreed GA path is to extend the existing ContainerRuntimeConfig (ctrcfg) API with an NRI mutation policy field, delivered via the MCO's existing reconciliation pipeline. The plugin binary is unchanged — only delivery moves from raw Ignition to the ctrcfg API. This will be designed and tracked in a follow-on enhancement once Approach A is proven in Dev Preview.
cc: @rphillips @haircommander @saschagrunert @ngopalak-redhat
Summary by CodeRabbit
NriPluginandNriMutationPolicyresources.