Skip to content

Prestart hook not appended into rspec.Hooks struct #2540

@sameersaeed

Description

@sameersaeed

While writing a hook injector unit test for NRI I noticed that the OCI Prestart hook isn't getting 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 on this repo 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

cc @giuseppe @mikebrow @haircommander - 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:

common/pkg/hooks/hooks.go

Lines 125 to 126 in dc0f263

case "createRuntime", "prestart":
config.Hooks.CreateRuntime = append(config.Hooks.CreateRuntime, namedHook.hook.Hook)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions