NRI: allow plugins to see/modify env vars by robmry · Pull Request #51674 · moby/moby

robmry · 2025-12-09T09:35:05Z

- What I did

related to NRI: initial implementation #51511

- How I did it

Call NRI's ContainerCreate hook.
Tell NRI plugins about env-vars, and allow plugins to modify them.
Add an integration test

- How to verify it

- Human readable description for the release notes

Add experimental NRI support

akerouanton · 2025-12-09T11:43:03Z

daemon/internal/nri/nri.go

+		return nil
+	}
+	ctr.State.Lock()
+	defer ctr.State.Unlock()


AFAIK the container hasn't been made visible to other goroutines when this method is called, so there should be no reasons to lock its state. Thus, I think this is not needed.

Yes, that's true at the moment ... but locking the lock makes sure, it's super-cheap if there really is no contention, and particularly as there's a package boundary - just acquiring the lock anyway seemed best to me.

(If the implementation carries on past a minimal initial version, there are more hooks / lifecycle events to add. They'll definitely need to lock the container. If they copy from CreateContainer, they won't forget!)

So I can remove it, but would prefer not to - do you (or others) feel more strongly that it should go?

IMO, it's better to remove it, but leave a comment explaining why we don't need a lock here (because it's the initial creation and container hasn't been registered yet).

Ok - it's gone!

akerouanton · 2025-12-09T11:47:12Z

daemon/internal/nri/nri.go

 }
+
+func containerToNRI(ctr *container.Container) (*adaptation.PodSandbox, *adaptation.Container, error) {
+	nriPod := &adaptation.PodSandbox{


Probably a dumb question, is this mandatory? If so, is it required by github.com/containerd/nri or individual plugins?

I haven't actually tried passing in a nil PodSandbox, or looked at whether it's nil-checked in the NRI code... but, although we don't have pods, the PodSandbox is where the netns lives in the plugin interface. So, it seems likely individual plugins will want that at some point.

My thinking is that we'll tell plugins there's a pod-per-container, with our non-pods having the same lifecycle as our containers. (And we'll probably represent container-networking as containers sharing a pod.)

For now though, in this super-minimal implementation that only allows env-var and mount updates - it's not populated. Only because the implementation is incomplete.

I've added comments to that effect ... I hope the plan makes sense?

akerouanton · 2025-12-09T11:52:28Z

daemon/internal/nri/nri.go

+		Linux: &adaptation.LinuxContainer{
+			Namespaces:     nil,
+			Devices:        nil,
+			Resources:      nil,
+			OomScoreAdj:    nil,
+			CgroupsPath:    "",
+			IoPriority:     nil,
+			SeccompProfile: nil,
+			SeccompPolicy:  nil,
+		},
+		Mounts:        nil,
+		Pid:           uint32(ctr.Pid),
+		Rlimits:       nil,
+		CreatedAt:     0,
+		StartedAt:     0,
+		FinishedAt:    0,
+		ExitCode:      0,
+		StatusReason:  "",
+		StatusMessage: "",
+		CDIDevices:    nil,


Any particular reason to not fill these fields? Is this coming in a later PR?

For most fields ... potentially coming in a later PR (there's not currently any plan beyond an initial implementation that allows modification of env-vars and mounts).

To support that minimal case, a few more fields are coming soon - master...robmry:moby:nri

Most of the fields have a fairly obvious mapping from our internal data - but each thing that's populated here will need a test and, if it's adjustable by the plugin, tests for that too ... I've expanded the structs with zero values here to show what's missing.

The other big gap in the initial implementation is that it'll only support the "creation" lifecycle event / hook point ... not-implemented yet are updating and stopping (which allow adjustments), and informative events post-creation, starting, post-start, post-update and removal.

akerouanton · 2025-12-09T11:56:46Z

daemon/internal/nri/nri.go

+	return nil
+}
+
 func (n *NRI) syncFn(ctx context.Context, syncCB adaptation.SyncCB) error {


If I get it correctly, syncFn is called by the adaptation package when a NRI plugin request a 'container update' to update its internal state. Maybe worth documenting for laymen? (But that's not directly related to this PR, so feel free to address that whenever you want if that makes sense)

I think it's meant for newly registered plugins to learn about existing containers - I've added a comment.

Are those plugins able to modify state on those containers as part of their registration? i.e., can we expect them to modify them after-the-fact when registering?

(mostly curious how such plugins hook into the container lifecycle; I'd expect them to act when creating containers, so slightly wondering why they need to be aware of existing containers)

Also, like Albin's comment; non-blocking, but we can improve docs and/or look at the integration in follow-ups.

akerouanton · 2025-12-09T11:59:53Z

daemon/internal/nri/nri.go

+		return err
+	}
+
+	if resp.GetUpdate() != nil {


What's an update? Looking at NRI types, it seems the type returned by GetUpdate only holds resource limits. Any links to share, etc.? Maybe a few code comments could help understand what's supported or not, and where to learn more about NRI.

Should we check that GetEvict() == nil too, and log / return an error otherwise?

Updates won't be supported in the initial implementation - but they do map (ish) to what can be modified by our "docker update", so it should be possible to support them at some point.

I've added a missing package comment, with a link to https://github.com/containerd/nri - which describes the NRI lifecycle, what mods it can make, and when.

There's a commit to check for unsupported adjustments in the queue ("NRI: error on unsupported adjustment") - but I missed GetEvict, so will add a check for it there.

akerouanton · 2025-12-09T12:05:21Z

daemon/internal/nri/nri.go

+			return errors.New("empty environment variable key")
+		}
+		val := kv.Key + "=" + kv.Value
+		log.G(ctx).Debugf("Applying NRI env var adjustment: %s", val)


Debug logs aren't enabled by default, but this could leak secrets, etc. I think it'd be better to only log the list of updated keys.

Good point, done.

The modified env-var's value will always show up in container-inspect. So, storing secrets directly in env-vars set by NRI may not be the best idea anyway.

akerouanton · 2025-12-09T12:17:37Z

integration/daemon/nri/plugin.go

+	p.stub = stub
+	err = p.stub.Start(ctx)
+	assert.Assert(t, err)
+	t.Cleanup(p.stub.Stop)


It might be a matter of personal taste, but I feel uncomfortable when a stop is burried inside of a start func. From a caller's perspective, it seems the plugin is started but never stopped.

You could instead return that stop func and call it through a defer.

vvoland

LGTM; the mutex can be handled in follow up

Signed-off-by: Rob Murray <rob.murray@docker.com>

akerouanton

LGTM

thaJeztah

LGTM

thaJeztah · 2025-12-10T14:32:45Z

daemon/internal/nri/nri.go

+// the same level of trust as the daemon itself, because they can make arbitrary
+// modifications to container settings.
+//
+// The NRI framework is implemented by https://github.com/containerd/nri - see that


If you change this to a docs-link, then GoDoc automatically links it to the right place (on pkg.go.dev);

Suggested change

// The NRI framework is implemented by https://github.com/containerd/nri - see that

// The NRI framework is implemented by [github.com/containerd/nri] - see that

Before / After;

thaJeztah · 2025-12-10T14:36:06Z

daemon/internal/nri/nri.go

+func (n *NRI) CreateContainer(ctx context.Context, ctr *container.Container) error {
+	n.mu.RLock()
+	defer n.mu.RUnlock()
+	if n.nri == nil {


As a follow-up, we should look if we can come with more descriptive names for these; there's various places where we use nri (both in package names and field-names), and it's not always clear "NRI-what? is it a bird, a plane, a service?"; e.g. also this one, which is a service that's started stopped, I THINK ?

moby/daemon/daemon.go

Line 120 in 186a5ab

nri *nri.NRI

thaJeztah · 2025-12-10T14:38:15Z

daemon/internal/nri/nri.go

+	// ctr.State can safely be locked here, but there's no need because it's expected
+	// to be newly created and not yet accessible in any other thread.


Wondering if this would be something we should also mention in the CreateContainer GoDoc; i.e., that it's not designed for concurrent use (on the container) or something along those lines (because it doesn't lock the container's state).

(definitely non-blocking, just reading along and fine for a follow-up)

Yeah - just taking the lock would've made it impossible to get wrong ... but I was outvoted!

thaJeztah · 2025-12-10T14:42:29Z

daemon/internal/nri/nri.go

+	return nil
+}
+
 func (n *NRI) syncFn(ctx context.Context, syncCB adaptation.SyncCB) error {


Are those plugins able to modify state on those containers as part of their registration? i.e., can we expect them to modify them after-the-fact when registering?

(mostly curious how such plugins hook into the container lifecycle; I'd expect them to act when creating containers, so slightly wondering why they need to be aware of existing containers)

Also, like Albin's comment; non-blocking, but we can improve docs and/or look at the integration in follow-ups.

thaJeztah · 2025-12-10T14:48:03Z

integration/daemon/nri/nri_test.go

+
+func TestNRIContainerCreateEnvVarMod(t *testing.T) {
+	skip.If(t, testEnv.IsRemoteDaemon, "cannot run daemon when remote daemon")
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")


I'm guessing "non windows" is also because we can't start a custom daemon with these options, right? Perhaps we should add a comment to the skip (Looking at "patch env-vars", I could imagine that can be supported on Windows as well, just not easy to test it).

thaJeztah · 2025-12-10T14:49:23Z

integration/daemon/nri/nri_test.go

+
+	ctx := testutil.StartSpan(baseContext, t)
+
+	tmp := t.TempDir()


Really (really) nit; I think in most places we use tmpDir as var-name for these.

git grep -i 'tmp := t.TempDir()' | wc -l 2 git grep -i 'tmpDir := t.TempDir()' | wc -l 34 git grep -i 'tempDir := t.TempDir()' | wc -l 7

thaJeztah · 2025-12-10T14:51:12Z

integration/daemon/nri/nri_test.go

+	defer d.Stop(t)
+	c := d.NewClientT(t)
+
+	testcases := []struct {


Similar nit;

git grep -i ':= range testcases' | wc -l 109 git grep -i ':= range tests' | wc -l 177

thaJeztah · 2025-12-10T14:54:08Z

Ah, LOL, race;

robmry · 2025-12-10T15:14:30Z

Ah, LOL, race;

Raised #51678 to address the extra comments.

robmry added this to the 29.2.0 milestone Dec 9, 2025

robmry self-assigned this Dec 9, 2025

robmry added area/runtime Runtime kind/feature Functionality or other elements that the project doesn't currently have. Features are new and shiny area/daemon Core Engine labels Dec 9, 2025

github-actions bot added area/logging platform/windows area/docs area/security area/security/seccomp area/dependencies labels Dec 9, 2025

robmry mentioned this pull request Dec 9, 2025

NRI: allow plugins to add mounts #51675

Merged

robmry force-pushed the nri-env-vars branch 2 times, most recently from 73156ed to de3712e Compare December 9, 2025 11:03

robmry marked this pull request as ready for review December 9, 2025 11:08

akerouanton reviewed Dec 9, 2025

View reviewed changes

robmry force-pushed the nri-env-vars branch from de3712e to c04e0c0 Compare December 9, 2025 15:01

robmry requested a review from akerouanton December 9, 2025 17:33

vvoland approved these changes Dec 10, 2025

View reviewed changes

robmry added 2 commits December 10, 2025 10:30

NRI: add ContainerCreate hook, allow env-var adjustments

b67f0c0

Signed-off-by: Rob Murray <rob.murray@docker.com>

NRI: add TestNRIContainerCreateEnvVarMod

322dda3

Signed-off-by: Rob Murray <rob.murray@docker.com>

robmry force-pushed the nri-env-vars branch from c04e0c0 to 322dda3 Compare December 10, 2025 10:31

akerouanton approved these changes Dec 10, 2025

View reviewed changes

akerouanton merged commit 0702422 into moby:master Dec 10, 2025
185 of 186 checks passed

thaJeztah approved these changes Dec 10, 2025

View reviewed changes

robmry mentioned this pull request Dec 10, 2025

NRI: minor cleanups #51678

Merged

robmry added the impact/changelog label Dec 16, 2025

vvoland removed the area/logging label Dec 16, 2025

vvoland removed area/docs area/security area/security/seccomp area/dependencies labels Dec 16, 2025

	// The NRI framework is implemented by https://github.com/containerd/nri - see that
	// The NRI framework is implemented by [github.com/containerd/nri] - see that

		// ctr.State can safely be locked here, but there's no need because it's expected
		// to be newly created and not yet accessible in any other thread.

Conversation

robmry commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

akerouanton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 10, 2025

Uh oh!

robmry commented Dec 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robmry commented Dec 9, 2025 •

edited

Loading