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
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions