Defaulting to the latest tag if no tag was specified.

In the current code, we handle differently build/sign and deploying a
pre-built kmod.

This is the current behavior:
* If a `build`/`sign` section is set in the module, and the image tag
  isn't specified, then we will return an error.
* If there are no `build`/`sign` section, and the image tag isn't
  specified, then we will default to the `latest` tag.

This change is adjusting the behavior of both workflow to always default
to the `latest` image tag if it is not specified in the `Module`.

This will also add a webhook warning in case we are defaulting to
`latest` for visibility.

Signed-off-by: Yoni Bettan <[email protected]>

Author: ybettan

go.mod

@@ -12,6 +12,7 @@ require (
 	github.com/google/go-cmp v0.6.0
 	github.com/google/go-containerregistry v0.19.1
 	github.com/google/go-containerregistry/pkg/authn/kubernetes v0.0.0-20230523181351-c3f8a49229d3
+	github.com/hashicorp/go-multierror v1.1.1
 	github.com/mitchellh/hashstructure/v2 v2.0.2
 	github.com/moby/moby v26.1.3+incompatible
 	github.com/onsi/ginkgo/v2 v2.19.0
@@ -66,6 +67,7 @@ require (
 	github.com/google/pprof v0.0.0-20240424215950-a892ee059fd6 // indirect
 	github.com/google/uuid v1.3.1 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0 // indirect
+	github.com/hashicorp/errwrap v1.1.0 // indirect
 	github.com/imdario/mergo v0.3.13 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/josharian/intern v1.0.0 // indirect

go.sum

@@ -96,6 +96,11 @@ github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4=
 github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0 h1:YBftPWNWd4WwGqtY2yeZL2ef8rHAxPBD8KFhJpmcqms=
 github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0/go.mod h1:YN5jB8ie0yfIUg6VvR9Kz84aCaG7AsGZnLjhHbUqwPg=
+github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
+github.com/hashicorp/errwrap v1.1.0 h1:OxrOeh75EUXMY8TBjag2fzXGZ40LB6IKw45YeGUDY2I=
+github.com/hashicorp/errwrap v1.1.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
+github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+lD48awMYo=
+github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM=
 github.com/imdario/mergo v0.3.13 h1:lFzP57bqS/wsqKssCGmtLAb8A0wKjLGrve2q3PPVcBk=
 github.com/imdario/mergo v0.3.13/go.mod h1:4lJ1jqUDcsbIECGy0RUJAXNIhg+6ocWgb1ALK2O4oXg=
 github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=

internal/module/kernelmapper.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"regexp"
+	"strings"
 
 	kmmv1beta1 "github.com/kubernetes-sigs/kernel-module-management/api/v1beta1"
 	"github.com/kubernetes-sigs/kernel-module-management/internal/api"
@@ -114,6 +115,9 @@ func (kh *kernelMapperHelper) prepareModuleLoaderData(mapping *kmmv1beta1.Kernel
 	if mapping.ContainerImage == "" {
 		mld.ContainerImage = mod.Spec.ModuleLoader.Container.ContainerImage
 	}
+	if !strings.Contains(mld.ContainerImage, "@") && !strings.Contains(mld.ContainerImage, ":") {
+		mld.ContainerImage = fmt.Sprintf("%s:latest", mld.ContainerImage)
+	}
 
 	mld.InTreeModulesToRemove = mod.Spec.ModuleLoader.Container.InTreeModulesToRemove
 	if mapping.InTreeModulesToRemove != nil {

internal/module/kernelmapper_test.go

@@ -3,6 +3,7 @@ package module
 import (
 	"errors"
 	"fmt"
+	"strings"
 
 	kmmv1beta1 "github.com/kubernetes-sigs/kernel-module-management/api/v1beta1"
 	"github.com/kubernetes-sigs/kernel-module-management/internal/api"
@@ -170,7 +171,7 @@ var _ = Describe("prepareModuleLoaderData", func() {
 		signHelper = sign.NewMockHelper(ctrl)
 		kh = newKernelMapperHelper(buildHelper, signHelper)
 		mod = kmmv1beta1.Module{}
-		mod.Spec.ModuleLoader.Container.ContainerImage = "spec container image"
+		mod.Spec.ModuleLoader.Container.ContainerImage = "registry.io/org/image:spec"
 		mod.Spec.ModuleLoader.Container.ImagePullPolicy = "Always"
 		mapping = kmmv1beta1.KernelMapping{}
 	})
@@ -180,7 +181,7 @@ var _ = Describe("prepareModuleLoaderData", func() {
 	})
 
 	DescribeTable("prepare mapping", func(buildExistsInMapping, buildExistsInModuleSpec, signExistsInMapping, SignExistsInModuleSpec,
-		registryTLSExistsInMapping, containerImageExistsInMapping, inTreeModulesToRemoveExistsInMapping bool) {
+		registryTLSExistsInMapping, containerImageExistsInMapping, inTreeModulesToRemoveExistsInMapping, containerImageNotTagged bool) {
 		build := &kmmv1beta1.Build{
 			DockerfileConfigMap: &v1.LocalObjectReference{
 				Name: "some name",
@@ -224,9 +225,14 @@ var _ = Describe("prepareModuleLoaderData", func() {
 		}
 		mld.ContainerImage = mod.Spec.ModuleLoader.Container.ContainerImage
 		if containerImageExistsInMapping {
-			mapping.ContainerImage = "mapping container image"
+			mapping.ContainerImage = "registry.io/org/image:mapping"
 			mld.ContainerImage = mapping.ContainerImage
 		}
+		if containerImageNotTagged {
+			mod.Spec.ModuleLoader.Container.ContainerImage = strings.Split(mod.Spec.ModuleLoader.Container.ContainerImage, ":")[0]
+			mapping.ContainerImage = strings.Split(mapping.ContainerImage, ":")[0]
+			mld.ContainerImage = "registry.io/org/image:latest"
+		}
 
 		if buildExistsInMapping || buildExistsInModuleSpec {
 			mld.Build = build
@@ -245,13 +251,14 @@ var _ = Describe("prepareModuleLoaderData", func() {
 		Expect(err).NotTo(HaveOccurred())
 		Expect(*res).To(Equal(mld))
 	},
-		Entry("build in mapping only", true, false, false, false, false, false, false),
-		Entry("build in spec only", false, true, false, false, false, false, false),
-		Entry("sign in mapping only", false, false, true, false, false, false, false),
-		Entry("sign in spec only", false, false, false, true, false, false, false),
-		Entry("registryTLS in mapping", false, false, false, false, true, false, false),
-		Entry("containerImage in mapping", false, false, false, false, false, true, false),
-		Entry("inTreeModulesToRemove in mapping", false, false, false, false, false, false, true),
+		Entry("build in mapping only", true, false, false, false, false, false, false, false),
+		Entry("build in spec only", false, true, false, false, false, false, false, false),
+		Entry("sign in mapping only", false, false, true, false, false, false, false, false),
+		Entry("sign in spec only", false, false, false, true, false, false, false, false),
+		Entry("registryTLS in mapping", false, false, false, false, true, false, false, false),
+		Entry("containerImage in mapping", false, false, false, false, false, true, false, false),
+		Entry("containerImage with no tag or hash set", false, false, false, false, false, true, false, true),
+		Entry("inTreeModulesToRemove in mapping", false, false, false, false, false, false, true, false),
 	)
 
 	// [TODO] remove this unit test once InTreeModuleToRemove depricated field is removed from CRD

internal/registry/registry.go

@@ -23,7 +23,8 @@ import (
 )
 
 const (
-	modulesLocationPath = "lib/modules"
+	modulesLocationPath   = "lib/modules"
+	latestTagNotSetErrMsg = "`latest` tag should have been set to the image if not specified by the user"
 )
 
 type DriverToolkitEntry struct {
@@ -106,15 +107,14 @@ func (r *registry) GetDigest(ctx context.Context, image string, tlsOptions *kmmv
 }
 
 func (r *registry) getPullOptions(ctx context.Context, image string, tlsOptions *kmmv1beta1.TLSOptions, registryAuthGetter auth.RegistryAuthGetter) (*RepoPullConfig, error) {
+
 	var repo string
 	if hash := strings.Split(image, "@"); len(hash) > 1 {
 		repo = hash[0]
 	} else if tag := strings.Split(image, ":"); len(tag) > 1 {
 		repo = tag[0]
-	}
-
-	if repo == "" {
-		return nil, fmt.Errorf("image url %s is not valid, does not contain hash or tag", image)
+	} else {
+		return nil, errors.New(latestTagNotSetErrMsg)
 	}
 
 	options := []crane.Option{

internal/registry/registry_test.go

@@ -62,7 +62,7 @@ var _ = Describe("ImageExists", func() {
 			_, err = reg.ImageExists(ctx, invalidImage, &kmmv1beta1.TLSOptions{}, nil)
 
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("does not contain hash or tag"))
+			Expect(err.Error()).To(ContainSubstring(latestTagNotSetErrMsg))
 		})
 
 		It("should fail if it cannot get key chain from secret", func() {
@@ -227,7 +227,7 @@ var _ = Describe("GetLayersDigests", func() {
 			_, err = reg.ImageExists(ctx, invalidImage, &kmmv1beta1.TLSOptions{}, nil)
 
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("does not contain hash or tag"))
+			Expect(err.Error()).To(ContainSubstring(latestTagNotSetErrMsg))
 		})
 
 		It("should fail if it cannot get key chain from secret", func() {
@@ -397,7 +397,7 @@ var _ = Describe("GetDigest", func() {
 			_, err = reg.GetDigest(ctx, invalidImage, &kmmv1beta1.TLSOptions{}, nil)
 
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("does not contain hash or tag"))
+			Expect(err.Error()).To(ContainSubstring(latestTagNotSetErrMsg))
 		})
 
 		It("should fail if it cannot get key chain from secret", func() {

internal/webhook/errors.go

@@ -3,3 +3,4 @@ package webhook
 import "errors"
 
 var NotImplemented = errors.New("not implemented")
+var ImageHashOrTagNotSet = errors.New("the container image does not contain hash or tag")

internal/webhook/module.go

@@ -21,8 +21,10 @@ import (
 	"errors"
 	"fmt"
 	"regexp"
+	"strings"
 
 	"github.com/go-logr/logr"
+	multierror "github.com/hashicorp/go-multierror"
 	kmmv1beta1 "github.com/kubernetes-sigs/kernel-module-management/api/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -104,18 +106,24 @@ func validateModule(mod *kmmv1beta1.Module) (admission.Warnings, error) {
 		)
 	}
 
+	var warnings admission.Warnings
 	if err := validateModuleLoaderContainerSpec(mod.Spec.ModuleLoader.Container); err != nil {
-		return nil, fmt.Errorf("failed to validate kernel mappings: %v", err)
+		if !errors.Is(err, ImageHashOrTagNotSet) {
+			return nil, fmt.Errorf("failed to validate kernel mappings: %v", err)
+		}
+		warn := fmt.Sprintf("%s - defaulting to `latest` tag", err.Error())
+		warnings = append(warnings, warn)
 	}
 
-	return nil, validateModprobe(mod.Spec.ModuleLoader.Container.Modprobe)
+	return warnings, validateModprobe(mod.Spec.ModuleLoader.Container.Modprobe)
 }
 
 func validateModuleLoaderContainerSpec(container kmmv1beta1.ModuleLoaderContainerSpec) error {
 	if container.InTreeModulesToRemove != nil && container.InTreeModuleToRemove != "" { //nolint:staticcheck
 		return fmt.Errorf("only one of the Container's fields: InTreeModulesToRemove or InTreeModuleToRemove can be defined")
 	}
 
+	var imagesErrors error
 	for idx, km := range container.KernelMappings {
 		if km.Regexp != "" && km.Literal != "" {
 			return fmt.Errorf("regexp and literal are mutually exclusive properties at kernelMappings[%d]", idx)
@@ -141,6 +149,19 @@ func validateModuleLoaderContainerSpec(container kmmv1beta1.ModuleLoaderContaine
 			(km.InTreeModuleToRemove != "" && container.InTreeModulesToRemove != nil) { //nolint:staticcheck
 			return fmt.Errorf("only one type if field (InTreeModuleToRemove or InTreeModulesToRemove) can be defined in KenrelMapping or Container")
 		}
+
+		var imgToCheck string
+		imgToCheck = container.ContainerImage
+		if km.ContainerImage != "" {
+			imgToCheck = km.ContainerImage
+		}
+		if !strings.ContainsRune(imgToCheck, '@') && !strings.ContainsRune(imgToCheck, ':') {
+			imagesErrors = multierror.Append(imagesErrors, fmt.Errorf("image %s is malformed", imgToCheck))
+		}
+	}
+
+	if imagesErrors != nil {
+		return multierror.Append(imagesErrors, ImageHashOrTagNotSet)
 	}
 
 	return nil

internal/webhook/module_test.go

@@ -76,6 +76,8 @@ var _ = Describe("maxCombinedLength", func() {
 })
 
 var _ = Describe("validateModuleLoaderContainerSpec", func() {
+
+	const containerImage = "registry.io/org/image:tag"
 	It("should pass when there are not kernel mappings", func() {
 		Expect(
 			validateModuleLoaderContainerSpec(kmmv1beta1.ModuleLoaderContainerSpec{}),
@@ -87,15 +89,15 @@ var _ = Describe("validateModuleLoaderContainerSpec", func() {
 	It("should pass when regexp,literal and containerImage are valid", func() {
 		containerSpec1 := kmmv1beta1.ModuleLoaderContainerSpec{
 			KernelMappings: []kmmv1beta1.KernelMapping{
-				{Regexp: "valid-regexp", ContainerImage: "image-url"},
-				{Regexp: "^.*$", ContainerImage: "image-url"},
+				{Regexp: "valid-regexp", ContainerImage: containerImage},
+				{Regexp: "^.*$", ContainerImage: containerImage},
 			},
 		}
 
 		Expect(validateModuleLoaderContainerSpec(containerSpec1)).ToNot(HaveOccurred())
 
 		containerSpec2 := kmmv1beta1.ModuleLoaderContainerSpec{
-			ContainerImage: "image-url",
+			ContainerImage: containerImage,
 			KernelMappings: []kmmv1beta1.KernelMapping{
 				{Regexp: "valid-regexp"},
 			},
@@ -104,7 +106,7 @@ var _ = Describe("validateModuleLoaderContainerSpec", func() {
 		Expect(validateModuleLoaderContainerSpec(containerSpec2)).ToNot(HaveOccurred())
 
 		containerSpec3 := kmmv1beta1.ModuleLoaderContainerSpec{
-			ContainerImage: "image-url",
+			ContainerImage: containerImage,
 			KernelMappings: []kmmv1beta1.KernelMapping{
 				{Literal: "any-value"},
 			},
@@ -176,6 +178,25 @@ var _ = Describe("validateModuleLoaderContainerSpec", func() {
 		)
 	})
 
+	It("should fail when a kernel-mapping has a container image with no sha or tag", func() {
+		containerSpec := kmmv1beta1.ModuleLoaderContainerSpec{
+			KernelMappings: []kmmv1beta1.KernelMapping{
+				{
+					Regexp:         "valid-regexp",
+					ContainerImage: "registry.io/org/image-with-no-tag",
+				},
+			},
+		}
+
+		Expect(
+			validateModuleLoaderContainerSpec(containerSpec),
+		).To(
+			MatchError(
+				ContainSubstring(ImageHashOrTagNotSet.Error()),
+			),
+		)
+	})
+
 	DescribeTable("should fail when InTreeModulesToRemove and InTreeModuleToRemove both defined",
 		func(inTreeModulesInContainer, inTreeModuleInContainer, inTreeModulesInKM, inTreeModuleInKM bool) {
 			containerSpec := kmmv1beta1.ModuleLoaderContainerSpec{}
@@ -349,7 +370,7 @@ var _ = Describe("validateModule", func() {
 	chars21 := strings.Repeat("a", 21)
 
 	DescribeTable(
-		"should work as expected",
+		"module naming",
 		func(name, ns string, errExpected bool) {
 			mod := validModule
 			mod.Name = name
@@ -367,6 +388,31 @@ var _ = Describe("validateModule", func() {
 		Entry("not too long", "name", "ns", false),
 		Entry("too long", chars21, chars21, true),
 	)
+
+	DescribeTable(
+		"container image with/without tag or has",
+		func(containerImg string, expectedWarnings bool) {
+			mod := validModule
+			mod.Spec.ModuleLoader.Container.ContainerImage = containerImg
+			mod.Spec.ModuleLoader.Container.KernelMappings = []kmmv1beta1.KernelMapping{
+				{
+					Regexp:         "valid-regexp",
+					ContainerImage: containerImg,
+				},
+			}
+
+			warnings, err := validateModule(&mod)
+			Expect(err).NotTo(HaveOccurred())
+			if expectedWarnings {
+				Expect(warnings).NotTo(BeEmpty())
+			} else {
+				Expect(warnings).To(BeEmpty())
+			}
+		},
+		Entry("image has tag", "registry.io/org/image:some-tag", false),
+		Entry("image has hash", "registry.io/org/image@99999", false),
+		Entry("image has no tag nor hash", "registry.io/org/image", true),
+	)
 })
 
 var _ = Describe("ValidateCreate", func() {