Move cmd/nvidia-container-runtime/modifier package to internal/modifier

Signed-off-by: Evan Lezar <elezar@nvidia.com>

cmd/nvidia-container-runtime/main_test.go

@@ -10,7 +10,7 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/NVIDIA/nvidia-container-toolkit/cmd/nvidia-container-runtime/modifier"
+	"github.com/NVIDIA/nvidia-container-toolkit/internal/modifier"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/test"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/require"

cmd/nvidia-container-runtime/runtime_factory.go

@@ -19,9 +19,9 @@ package main
 import (
 	"fmt"
 
-	"github.com/NVIDIA/nvidia-container-toolkit/cmd/nvidia-container-runtime/modifier"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/config"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/info"
+	"github.com/NVIDIA/nvidia-container-toolkit/internal/modifier"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/oci"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/runtime"
 	"github.com/sirupsen/logrus"

internal/modifier/csv.go

@@ -24,7 +24,6 @@ import (
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/cuda"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/discover"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/discover/csv"
-	"github.com/NVIDIA/nvidia-container-toolkit/internal/modifier"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/oci"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/requirements"
 	"github.com/sirupsen/logrus"
@@ -51,8 +50,7 @@ func NewCSVModifier(logger *logrus.Logger, cfg *config.Config, ociSpec oci.Spec)
 		return nil, fmt.Errorf("failed to load OCI spec: %v", err)
 	}
 
-	// In experimental mode, we check whether a modification is required at all and return the lowlevelRuntime directly
-	// if no modification is required.
+	// We check whether a modification is required and return a nil modifier if this is not the case.
 	visibleDevices, exists := ociSpec.LookupEnv(visibleDevicesEnvvar)
 	if !exists || visibleDevices == "" || visibleDevices == visibleDevicesVoid {
 		logger.Infof("No modification required: %v=%v (exists=%v)", visibleDevicesEnvvar, visibleDevices, exists)
@@ -103,17 +101,12 @@ func NewCSVModifier(logger *logrus.Logger, cfg *config.Config, ociSpec oci.Spec)
 
 	d := discover.NewList(csvDiscoverer, ldcacheUpdateHook, createSymlinksHook)
 
-	return newCSVModifierFromDiscoverer(logger, d)
-}
-
-// newCSVModifierFromDiscoverer is used to test with dependency injection
-func newCSVModifierFromDiscoverer(logger *logrus.Logger, d discover.Discover) (oci.SpecModifier, error) {
-	discoverModifier, err := modifier.NewModifierFromDiscoverer(logger, d)
+	discoverModifier, err := NewModifierFromDiscoverer(logger, d)
 	if err != nil {
 		return nil, fmt.Errorf("failed to construct modifier: %v", err)
 	}
 
-	modifiers := modifier.Merge(
+	modifiers := Merge(
 		nvidiaContainerRuntimeHookRemover{logger},
 		discoverModifier,
 	)

internal/modifier/csv_test.go

@@ -21,7 +21,6 @@ import (
 	"testing"
 
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/config"
-	"github.com/NVIDIA/nvidia-container-toolkit/internal/discover"
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/oci"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	testlog "github.com/sirupsen/logrus/hooks/test"
@@ -95,105 +94,15 @@ func TestNewCSVModifier(t *testing.T) {
 	}
 }
 
-func TestExperimentalModifier(t *testing.T) {
+func TestCSVModifierRemovesHook(t *testing.T) {
 	logger, _ := testlog.NewNullLogger()
 
 	testCases := []struct {
 		description   string
-		discover      *discover.DiscoverMock
 		spec          *specs.Spec
 		expectedError error
 		expectedSpec  *specs.Spec
 	}{
-		{
-			description: "empty discoverer does not modify spec",
-			discover:    &discover.DiscoverMock{},
-		},
-		{
-			description: "failed hooks discoverer returns error",
-			discover: &discover.DiscoverMock{
-				HooksFunc: func() ([]discover.Hook, error) {
-					return nil, fmt.Errorf("discover.Hooks error")
-				},
-			},
-			expectedError: fmt.Errorf("discover.Hooks error"),
-		},
-		{
-			description: "discovered hooks are injected into spec",
-			spec:        &specs.Spec{},
-			discover: &discover.DiscoverMock{
-				HooksFunc: func() ([]discover.Hook, error) {
-					hooks := []discover.Hook{
-						{
-							Lifecycle: "prestart",
-							Path:      "/hook/a",
-							Args:      []string{"/hook/a", "arga"},
-						},
-						{
-							Lifecycle: "createContainer",
-							Path:      "/hook/b",
-							Args:      []string{"/hook/b", "argb"},
-						},
-					}
-					return hooks, nil
-				},
-			},
-			expectedSpec: &specs.Spec{
-				Hooks: &specs.Hooks{
-					Prestart: []specs.Hook{
-						{
-							Path: "/hook/a",
-							Args: []string{"/hook/a", "arga"},
-						},
-					},
-					CreateContainer: []specs.Hook{
-						{
-							Path: "/hook/b",
-							Args: []string{"/hook/b", "argb"},
-						},
-					},
-				},
-			},
-		},
-		{
-			description: "existing hooks are maintained",
-			spec: &specs.Spec{
-				Hooks: &specs.Hooks{
-					Prestart: []specs.Hook{
-						{
-							Path: "/hook/a",
-							Args: []string{"/hook/a", "arga"},
-						},
-					},
-				},
-			},
-			discover: &discover.DiscoverMock{
-				HooksFunc: func() ([]discover.Hook, error) {
-					hooks := []discover.Hook{
-						{
-							Lifecycle: "prestart",
-							Path:      "/hook/b",
-							Args:      []string{"/hook/b", "argb"},
-						},
-					}
-					return hooks, nil
-				},
-			},
-			expectedSpec: &specs.Spec{
-				Hooks: &specs.Hooks{
-					Prestart: []specs.Hook{
-						{
-							Path: "/hook/a",
-							Args: []string{"/hook/a", "arga"},
-						},
-						{
-							Path: "/hook/b",
-							Args: []string{"/hook/b", "argb"},
-						},
-					},
-				},
-			},
-		},
 		{
 			description: "modification removes existing nvidia-container-runtime-hook",
 			spec: &specs.Spec{
@@ -206,26 +115,9 @@ func TestExperimentalModifier(t *testing.T) {
 					},
 				},
 			},
-			discover: &discover.DiscoverMock{
-				HooksFunc: func() ([]discover.Hook, error) {
-					hooks := []discover.Hook{
-						{
-							Lifecycle: "prestart",
-							Path:      "/hook/b",
-							Args:      []string{"/hook/b", "argb"},
-						},
-					}
-					return hooks, nil
-				},
-			},
 			expectedSpec: &specs.Spec{
 				Hooks: &specs.Hooks{
-					Prestart: []specs.Hook{
-						{
-							Path: "/hook/b",
-							Args: []string{"/hook/b", "argb"},
-						},
-					},
+					Prestart: []specs.Hook{},
 				},
 			},
 		},
@@ -241,26 +133,9 @@ func TestExperimentalModifier(t *testing.T) {
 					},
 				},
 			},
-			discover: &discover.DiscoverMock{
-				HooksFunc: func() ([]discover.Hook, error) {
-					hooks := []discover.Hook{
-						{
-							Lifecycle: "prestart",
-							Path:      "/hook/b",
-							Args:      []string{"/hook/b", "argb"},
-						},
-					}
-					return hooks, nil
-				},
-			},
 			expectedSpec: &specs.Spec{
 				Hooks: &specs.Hooks{
-					Prestart: []specs.Hook{
-						{
-							Path: "/hook/b",
-							Args: []string{"/hook/b", "argb"},
-						},
-					},
+					Prestart: []specs.Hook{},
 				},
 			},
 		},
@@ -268,17 +143,16 @@ func TestExperimentalModifier(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			m, err := newCSVModifierFromDiscoverer(logger, tc.discover)
-			require.NoError(t, err)
+			m := nvidiaContainerRuntimeHookRemover{logger: logger}
 
-			err = m.Modify(tc.spec)
+			err := m.Modify(tc.spec)
 			if tc.expectedError != nil {
 				require.Error(t, err)
 			} else {
 				require.NoError(t, err)
 			}
 
-			require.EqualValues(t, tc.expectedSpec, tc.spec)
+			require.Empty(t, tc.spec.Hooks.Prestart)
 		})
 	}
 }

internal/modifier/discover_test.go

@@ -0,0 +1,145 @@
+/**
+# Copyright (c) 2022, NVIDIA CORPORATION.  All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+**/
+
+package modifier
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/NVIDIA/nvidia-container-toolkit/internal/discover"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	testlog "github.com/sirupsen/logrus/hooks/test"
+	"github.com/stretchr/testify/require"
+)
+
+func TestDiscoverModifier(t *testing.T) {
+	logger, _ := testlog.NewNullLogger()
+
+	testCases := []struct {
+		description   string
+		discover      *discover.DiscoverMock
+		spec          *specs.Spec
+		expectedError error
+		expectedSpec  *specs.Spec
+	}{
+		{
+			description: "empty discoverer does not modify spec",
+			discover:    &discover.DiscoverMock{},
+		},
+		{
+			description: "failed hooks discoverer returns error",
+			discover: &discover.DiscoverMock{
+				HooksFunc: func() ([]discover.Hook, error) {
+					return nil, fmt.Errorf("discover.Hooks error")
+				},
+			},
+			expectedError: fmt.Errorf("discover.Hooks error"),
+		},
+		{
+			description: "discovered hooks are injected into spec",
+			spec:        &specs.Spec{},
+			discover: &discover.DiscoverMock{
+				HooksFunc: func() ([]discover.Hook, error) {
+					hooks := []discover.Hook{
+						{
+							Lifecycle: "prestart",
+							Path:      "/hook/a",
+							Args:      []string{"/hook/a", "arga"},
+						},
+						{
+							Lifecycle: "createContainer",
+							Path:      "/hook/b",
+							Args:      []string{"/hook/b", "argb"},
+						},
+					}
+					return hooks, nil
+				},
+			},
+			expectedSpec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: []specs.Hook{
+						{
+							Path: "/hook/a",
+							Args: []string{"/hook/a", "arga"},
+						},
+					},
+					CreateContainer: []specs.Hook{
+						{
+							Path: "/hook/b",
+							Args: []string{"/hook/b", "argb"},
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "existing hooks are maintained",
+			spec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: []specs.Hook{
+						{
+							Path: "/hook/a",
+							Args: []string{"/hook/a", "arga"},
+						},
+					},
+				},
+			},
+			discover: &discover.DiscoverMock{
+				HooksFunc: func() ([]discover.Hook, error) {
+					hooks := []discover.Hook{
+						{
+							Lifecycle: "prestart",
+							Path:      "/hook/b",
+							Args:      []string{"/hook/b", "argb"},
+						},
+					}
+					return hooks, nil
+				},
+			},
+			expectedSpec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: []specs.Hook{
+						{
+							Path: "/hook/a",
+							Args: []string{"/hook/a", "arga"},
+						},
+						{
+							Path: "/hook/b",
+							Args: []string{"/hook/b", "argb"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			m, err := NewModifierFromDiscoverer(logger, tc.discover)
+			require.NoError(t, err)
+
+			err = m.Modify(tc.spec)
+			if tc.expectedError != nil {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+
+			require.EqualValues(t, tc.expectedSpec, tc.spec)
+		})
+	}
+}

internal/modifier/hook_remover_test.go

@@ -0,0 +1,111 @@
+/**
+# Copyright (c) 2022, NVIDIA CORPORATION.  All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+**/
+
+package modifier
+
+import (
+	"testing"
+
+	"github.com/opencontainers/runtime-spec/specs-go"
+	testlog "github.com/sirupsen/logrus/hooks/test"
+	"github.com/stretchr/testify/require"
+)
+
+func TestHookRemover(t *testing.T) {
+	logger, _ := testlog.NewNullLogger()
+
+	testCases := []struct {
+		description   string
+		spec          *specs.Spec
+		expectedError error
+		expectedSpec  *specs.Spec
+	}{
+		{
+			description: "existing hooks are maintained",
+			spec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: []specs.Hook{
+						{
+							Path: "/hook/a",
+							Args: []string{"/hook/a", "arga"},
+						},
+					},
+				},
+			},
+			expectedSpec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: []specs.Hook{
+						{
+							Path: "/hook/a",
+							Args: []string{"/hook/a", "arga"},
+						},
+					},
+				},
+			},
+		},
+		{
+			description: "modification removes existing nvidia-container-runtime-hook",
+			spec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: []specs.Hook{
+						{
+							Path: "/path/to/nvidia-container-runtime-hook",
+							Args: []string{"/path/to/nvidia-container-runtime-hook", "prestart"},
+						},
+					},
+				},
+			},
+			expectedSpec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: nil,
+				},
+			},
+		},
+		{
+			description: "modification removes existing nvidia-container-toolkit",
+			spec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: []specs.Hook{
+						{
+							Path: "/path/to/nvidia-container-toolkit",
+							Args: []string{"/path/to/nvidia-container-toolkit", "prestart"},
+						},
+					},
+				},
+			},
+			expectedSpec: &specs.Spec{
+				Hooks: &specs.Hooks{
+					Prestart: nil,
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			m := nvidiaContainerRuntimeHookRemover{logger: logger}
+
+			err := m.Modify(tc.spec)
+			if tc.expectedError != nil {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+
+			require.EqualValues(t, tc.expectedSpec, tc.spec)
+		})
+	}
+}