From 7dbfffc375b007d3640d45b75b364cef1dfd9658 Mon Sep 17 00:00:00 2001 From: Abrar Shivani Date: Mon, 27 Jul 2026 14:45:08 -0700 Subject: [PATCH] Reject incomplete image specs in ImagePath When a CR component sets repository or version but leaves repository, image or version empty, ImagePath/imagePath previously concatenated the parts into a structurally invalid reference such as "/image:tag", "repo/:tag" or "repo/image:", which only failed much later at image pull with an opaque error. Validate the branch that builds "repo/image:tag"/"@digest" and return a clear error when repository, image or version is empty, in both image.ImagePath (internal/image) and the duplicated api/nvidia/v1 imagePath used by v1.ImagePath. The error uses %s (not %q) so the field values stay readable in the operator's JSON logs. The kbld/carvel passthrough (repository and version both empty) and the env-var fallback are unchanged. Add tests covering the valid tag/digest, kbld passthrough, env fallback, all-empty error, incomplete-spec and unsupported-type paths. Signed-off-by: Abrar Shivani --- api/nvidia/v1/clusterpolicy_types.go | 3 + api/nvidia/v1/clusterpolicy_types_test.go | 74 ++++++++++++ internal/image/image.go | 3 + internal/image/imagepath_validation_test.go | 127 ++++++++++++++++++++ 4 files changed, 207 insertions(+) create mode 100644 api/nvidia/v1/clusterpolicy_types_test.go create mode 100644 internal/image/imagepath_validation_test.go diff --git a/api/nvidia/v1/clusterpolicy_types.go b/api/nvidia/v1/clusterpolicy_types.go index 61f97b92ca..234b5eba36 100644 --- a/api/nvidia/v1/clusterpolicy_types.go +++ b/api/nvidia/v1/clusterpolicy_types.go @@ -2061,6 +2061,9 @@ func imagePath(repository string, image string, version string, imagePathEnvName crdImagePath = image } } else { + if repository == "" || image == "" || version == "" { + return "", fmt.Errorf("invalid image specification: repository, image and version must all be set (repository=%s, image=%s, version=%s)", repository, image, version) + } // use @ if image digest is specified instead of tag if strings.HasPrefix(version, "sha256:") { crdImagePath = repository + "/" + image + "@" + version diff --git a/api/nvidia/v1/clusterpolicy_types_test.go b/api/nvidia/v1/clusterpolicy_types_test.go new file mode 100644 index 0000000000..dbbedfa8b0 --- /dev/null +++ b/api/nvidia/v1/clusterpolicy_types_test.go @@ -0,0 +1,74 @@ +/** +# Copyright (c) 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 v1 + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestImagePath(t *testing.T) { + t.Run("valid spec builds repo/image:tag", func(t *testing.T) { + path, err := ImagePath(&DriverSpec{Repository: "nvcr.io/nvidia", Image: "driver", Version: "535"}) + require.NoError(t, err) + assert.Equal(t, "nvcr.io/nvidia/driver:535", path) + }) + + t.Run("sha256 version builds a digest reference", func(t *testing.T) { + path, err := ImagePath(&DriverSpec{Repository: "nvcr.io/nvidia", Image: "driver", Version: "sha256:abc"}) + require.NoError(t, err) + assert.Equal(t, "nvcr.io/nvidia/driver@sha256:abc", path) + }) + + t.Run("kbld passthrough when repository and version are empty", func(t *testing.T) { + path, err := ImagePath(&DriverSpec{Image: "nvcr.io/nvidia/driver@sha256:abc"}) + require.NoError(t, err) + assert.Equal(t, "nvcr.io/nvidia/driver@sha256:abc", path) + }) + + t.Run("incomplete spec (empty repository) is rejected", func(t *testing.T) { + // A partial CR spec must fail fast; an env value must not rescue it. + t.Setenv("DRIVER_IMAGE", "from-env/driver:v9") + path, err := ImagePath(&DriverSpec{Image: "driver", Version: "535"}) + require.Error(t, err) + assert.Empty(t, path) + assert.ErrorContains(t, err, "invalid image specification") + }) + + t.Run("incomplete spec (empty image) is rejected", func(t *testing.T) { + path, err := ImagePath(&DriverSpec{Repository: "nvcr.io/nvidia", Version: "535"}) + require.Error(t, err) + assert.Empty(t, path) + assert.ErrorContains(t, err, "invalid image specification") + }) + + t.Run("incomplete spec (empty version) is rejected", func(t *testing.T) { + path, err := ImagePath(&DriverSpec{Repository: "nvcr.io/nvidia", Image: "driver"}) + require.Error(t, err) + assert.Empty(t, path) + assert.ErrorContains(t, err, "invalid image specification") + }) + + t.Run("unsupported spec type errors", func(t *testing.T) { + path, err := ImagePath("not-a-spec") + require.Error(t, err) + assert.Empty(t, path) + assert.ErrorContains(t, err, "invalid type to construct image path") + }) +} diff --git a/internal/image/image.go b/internal/image/image.go index 25804242ac..9712f36894 100644 --- a/internal/image/image.go +++ b/internal/image/image.go @@ -32,6 +32,9 @@ func ImagePath(repository string, image string, version string, imagePathEnvName crdImagePath = image } } else { + if repository == "" || image == "" || version == "" { + return "", fmt.Errorf("invalid image specification: repository, image and version must all be set (repository=%s, image=%s, version=%s)", repository, image, version) + } // use @ if image digest is specified instead of tag if strings.HasPrefix(version, "sha256:") { crdImagePath = repository + "/" + image + "@" + version diff --git a/internal/image/imagepath_validation_test.go b/internal/image/imagepath_validation_test.go new file mode 100644 index 0000000000..544748ed06 --- /dev/null +++ b/internal/image/imagepath_validation_test.go @@ -0,0 +1,127 @@ +/** +# Copyright (c) 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 image + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestImagePath_ValidSpec(t *testing.T) { + const envName = "TEST_IMAGE_PATH_VALID" + + t.Run("repository, image and tag version resolve to repo/image:tag", func(t *testing.T) { + path, err := ImagePath("nvcr.io/nvidia", "gpu-operator", "v1.0.0", envName) + require.NoError(t, err) + assert.Equal(t, "nvcr.io/nvidia/gpu-operator:v1.0.0", path) + }) + + t.Run("sha256 version resolves to a digest reference", func(t *testing.T) { + path, err := ImagePath("nvcr.io/nvidia", "gpu-operator", "sha256:cafe", envName) + require.NoError(t, err) + assert.Equal(t, "nvcr.io/nvidia/gpu-operator@sha256:cafe", path) + }) +} + +func TestImagePath_InvalidSpec(t *testing.T) { + const envName = "TEST_IMAGE_PATH_ENV_INVALID" + + testCases := []struct { + description string + repository string + image string + version string + }{ + { + description: "empty repository with tag version", + repository: "", + image: "gpu-operator", + version: "v1.0.0", + }, + { + description: "empty repository with digest version", + repository: "", + image: "gpu-operator", + version: "sha256:cafe", + }, + { + description: "empty image with repository and version set", + repository: "nvcr.io/nvidia", + image: "", + version: "v1.0.0", + }, + { + description: "empty repository and image with tag version", + repository: "", + image: "", + version: "v1.0.0", + }, + { + description: "empty repository and image with digest version", + repository: "", + image: "", + version: "sha256:abc", + }, + { + description: "empty version with repository and image set", + repository: "nvcr.io/nvidia", + image: "gpu-operator", + version: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + // An env value must not rescue an incomplete spec. + t.Setenv(envName, "from-env/op:v9") + + path, err := ImagePath(tc.repository, tc.image, tc.version, envName) + + require.Error(t, err) + assert.Empty(t, path) + assert.ErrorContains(t, err, "invalid image specification") + }) + } +} + +func TestImagePath_KbldPassthrough(t *testing.T) { + const envName = "TEST_IMAGE_PATH_KBLD" + + path, err := ImagePath("", "nvcr.io/nvidia/driver@sha256:cafe", "", envName) + require.NoError(t, err) + assert.Equal(t, "nvcr.io/nvidia/driver@sha256:cafe", path) +} + +func TestImagePath_EnvFallback(t *testing.T) { + const envName = "TEST_IMAGE_PATH_ENV_FALLBACK" + t.Setenv(envName, "from-env/op:v9") + + path, err := ImagePath("", "", "", envName) + require.NoError(t, err) + assert.Equal(t, "from-env/op:v9", path) +} + +func TestImagePath_EmptyError(t *testing.T) { + const envName = "TEST_IMAGE_PATH_EMPTY" + + path, err := ImagePath("", "", "", envName) + require.Error(t, err) + assert.Empty(t, path) + assert.ErrorContains(t, err, "empty image path provided through both CR and ENV "+envName) +}