While writing a hook injector unit test for NRI I noticed that the OCI Prestart hook doesn't get appended as expected to &rspec.Hooks{}
The OCI runtime spec includes a Prestart hook that is deprecated, but is still currently expected to work
From discussing with @mikebrow it seems some changes were made in a previous commit so that the Prestart hook could be handled as CreateRuntime. But this removed the append() into &rspec.Hooks.Prestart which seems to have led to the mentioned issue in my test
For example, in the following, snippet, hooks ends up being populated with CreateRuntime instead of Prestart:
func testCreateContainerWithHooks(t *testing.T) {
t.Helper()
tempDir := t.TempDir()
hookJSON := []byte(`{
"version": "1.0.0",
"hook": {
"path": "/bin/echo",
"args": ["echo", "testing from hook"]
},
"when": {
"always": true
},
"stages": ["prestart"]
}`)
hookPath := filepath.Join(tempDir, "test-hook.json")
err := os.WriteFile(hookPath, hookJSON, 0644)
assert.NoError(t, err)
mgr, err := hooks.New(context.Background(), []string{tempDir}, []string{})
assert.NoError(t, err)
p := &plugin{mgr: mgr}
pod, container := createTestPodAndContainer()
adjust, updates, err := p.CreateContainer(context.Background(), pod, container)
assert.NoError(t, err)
assert.NotNil(t, adjust)
assert.Nil(t, updates)
hooks := adjust.Hooks
t.Log("Hooks:", hooks)
assert.NotNil(t, hooks.Hooks())
assert.NotEmpty(t, hooks.Prestart, "expected prestart hooks to be injected") // fails here because hooks has CreateRuntime instead of Prestart
[...]
% go test -v .
=== RUN TestHookInjector
[...]
=== RUN TestHookInjector/hooks_injected_correctly
time="2026-01-27T10:48:56-05:00" level=info msg="test-pod-hook-injector/test-container-hook-injector: OCI hooks injected"
hook-injector_test.go:40: Hooks: create_runtime:{path:"/bin/echo" args:"echo" args:"testing from hook"}
hook-injector_test.go:40:
Error Trace: [...]/nri/plugins/hook-injector/hook-injector_test.go:101
[...]/nri/plugins/hook-injector/hook-injector_test.go:40
Error: Should NOT be empty, but was []
Test: TestHookInjector/hooks_injected_correctly
Messages: expected prestart hooks to be injected
hook-injector_test.go:40:
Error Trace: [...]/nri/plugins/hook-injector/hook-injector_test.go:110
Since Prestart hasn't been removed from the spec yet, would it be possible to add an append() in the switch case statement so the Prestart hook gets added as well? i.e. add config.Hooks.Prestart = append(config.Hooks.Prestart, namedHook.hook.Hook) in here:
|
case "createRuntime", "prestart": |
|
config.Hooks.CreateRuntime = append(config.Hooks.CreateRuntime, namedHook.hook.Hook) |
cc @giuseppe @mikebrow @haircommander
While writing a hook injector unit test for NRI I noticed that the OCI Prestart hook doesn't get appended as expected to
&rspec.Hooks{}The OCI runtime spec includes a Prestart hook that is deprecated, but is still currently expected to work
From discussing with @mikebrow it seems some changes were made in a previous commit so that the
Prestarthook could be handled asCreateRuntime. But this removed theappend()into&rspec.Hooks.Prestartwhich seems to have led to the mentioned issue in my testFor example, in the following, snippet,
hooksends up being populated withCreateRuntimeinstead ofPrestart:Since
Prestarthasn't been removed from the spec yet, would it be possible to add anappend()in the switch case statement so thePrestarthook gets added as well? i.e. addconfig.Hooks.Prestart = append(config.Hooks.Prestart, namedHook.hook.Hook)in here:container-libs/common/pkg/hooks/hooks.go
Lines 125 to 126 in 1e46b07
cc @giuseppe @mikebrow @haircommander