Skip to content

Commit

Permalink
Refusing Modules with container images without a sha or a tag. (#1154)
Browse files Browse the repository at this point in the history 
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 enforce the
customer to explicitely set a sha or a tag in his container image.

Signed-off-by: Yoni Bettan <[email protected]>
  • Loading branch information
@ybettan
ybettan authored Jul 4, 2024
1 parent 46737ba commit b6ee602
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 36 deletions.
4 changes: 0 additions & 4 deletions internal/registry/registry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,6 @@ func (r *registry) getPullOptions(ctx context.Context, image string, tlsOptions
repo = tag[0]
}

if repo == "" {
return nil, fmt.Errorf("image url %s is not valid, does not contain hash or tag", image)
}

options := []crane.Option{
crane.WithContext(ctx),
}
Expand Down
23 changes: 0 additions & 23 deletions internal/registry/registry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,6 @@ var _ = Describe("ImageExists", func() {
Expect(err.Error()).To(ContainSubstring("failed to get pull options for image"))
})

It("should fail if the image name isn't valid", func() {

_, err = reg.ImageExists(ctx, invalidImage, &kmmv1beta1.TLSOptions{}, nil)

Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("does not contain hash or tag"))
})

It("should fail if it cannot get key chain from secret", func() {

mockRegistryAuthGetter.EXPECT().GetKeyChain(ctx).Return(nil, errors.New("some error"))
Expand Down Expand Up @@ -222,14 +214,6 @@ var _ = Describe("GetLayersDigests", func() {
Expect(err.Error()).To(ContainSubstring("failed to get pull options for image"))
})

It("should fail if the image name isn't valid", func() {

_, err = reg.ImageExists(ctx, invalidImage, &kmmv1beta1.TLSOptions{}, nil)

Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("does not contain hash or tag"))
})

It("should fail if it cannot get key chain from secret", func() {

mockRegistryAuthGetter.EXPECT().GetKeyChain(ctx).Return(nil, errors.New("some error"))
Expand Down Expand Up @@ -393,13 +377,6 @@ var _ = Describe("GetDigest", func() {
Expect(err.Error()).To(ContainSubstring("failed to get pull options for image"))
})

It("should fail if the image name isn't valid", func() {
_, err = reg.GetDigest(ctx, invalidImage, &kmmv1beta1.TLSOptions{}, nil)

Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("does not contain hash or tag"))
})

It("should fail if it cannot get key chain from secret", func() {
mockRegistryAuthGetter.EXPECT().GetKeyChain(ctx).Return(nil, errors.New("some error"))

Expand Down
25 changes: 23 additions & 2 deletions internal/webhook/module.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
"regexp"
"strings"

"github.com/go-logr/logr"
kmmv1beta1 "github.com/rh-ecosystem-edge/kernel-module-management/api/v1beta1"
Expand Down Expand Up @@ -111,11 +112,27 @@ func validateModule(mod *kmmv1beta1.Module) (admission.Warnings, error) {
return nil, validateModprobe(mod.Spec.ModuleLoader.Container.Modprobe)
}

func validateImageFormat(img string) error {

if !strings.Contains(img, ":") && !strings.Contains(img, "@") {
return fmt.Errorf("container image must explicitely set a tag or digest; got: %s", img)
}

return nil
}

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")
}

if contImg := container.ContainerImage; contImg != "" {
if err := validateImageFormat(contImg); err != nil {
return fmt.Errorf("failed to validate image format: %v", err)
}
}

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)
Expand All @@ -129,8 +146,12 @@ func validateModuleLoaderContainerSpec(container kmmv1beta1.ModuleLoaderContaine
return fmt.Errorf("invalid regexp at index %d: %v", idx, err)
}

if container.ContainerImage == "" && km.ContainerImage == "" {
return fmt.Errorf("missing spec.moduleLoader.container.kernelMappings[%d].containerImage", idx)
if kmImg := km.ContainerImage; kmImg == "" {
if container.ContainerImage == "" {
return fmt.Errorf("missing spec.moduleLoader.container.kernelMappings[%d].containerImage", idx)
}
} else if err := validateImageFormat(kmImg); err != nil {
return fmt.Errorf("failed to validate image format: %v", err)
}

if km.InTreeModulesToRemove != nil && km.InTreeModuleToRemove != "" { //nolint:staticcheck
Expand Down
69 changes: 62 additions & 7 deletions internal/webhook/module_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ var (
ModuleName: "mod-name",
},
KernelMappings: []kmmv1beta1.KernelMapping{
{Regexp: "valid-regexp", ContainerImage: "image-url"},
{Regexp: "valid-regexp", ContainerImage: "image-url:mytag"},
},
},
},
Expand Down Expand Up @@ -75,6 +75,27 @@ var _ = Describe("maxCombinedLength", func() {
})
})

var _ = Describe("validateImageFormat", func() {

It("should fail if no tag or digest were specified", func() {

Expect(
validateImageFormat("image-ur"),
).To(HaveOccurred())
})

It("should work if a tag or digest were specified", func() {

Expect(
validateImageFormat("image-url:tag"),
).NotTo(HaveOccurred())

Expect(
validateImageFormat("image-url@sha256:xxxxxxxxxxxx"),
).NotTo(HaveOccurred())
})
})

var _ = Describe("validateModuleLoaderContainerSpec", func() {
It("should pass when there are not kernel mappings", func() {
Expect(
Expand All @@ -84,18 +105,36 @@ var _ = Describe("validateModuleLoaderContainerSpec", func() {
)
})

It("should fail if the containerImage doesn't contain a sha or tag", func() {

containerSpec := kmmv1beta1.ModuleLoaderContainerSpec{
ContainerImage: "image-url",
KernelMappings: []kmmv1beta1.KernelMapping{
{Regexp: "valid-regexp"},
},
}

Expect(
validateModuleLoaderContainerSpec(containerSpec),
).To(
MatchError(
ContainSubstring("failed to validate image format"),
),
)
})

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: "image-url:mytag"},
{Regexp: "^.*$", ContainerImage: "image-url:mytag"},
},
}

Expect(validateModuleLoaderContainerSpec(containerSpec1)).ToNot(HaveOccurred())

containerSpec2 := kmmv1beta1.ModuleLoaderContainerSpec{
ContainerImage: "image-url",
ContainerImage: "image-url:mytag",
KernelMappings: []kmmv1beta1.KernelMapping{
{Regexp: "valid-regexp"},
},
Expand All @@ -104,7 +143,7 @@ var _ = Describe("validateModuleLoaderContainerSpec", func() {
Expect(validateModuleLoaderContainerSpec(containerSpec2)).ToNot(HaveOccurred())

containerSpec3 := kmmv1beta1.ModuleLoaderContainerSpec{
ContainerImage: "image-url",
ContainerImage: "image-url:mytag",
KernelMappings: []kmmv1beta1.KernelMapping{
{Literal: "any-value"},
},
Expand Down Expand Up @@ -176,6 +215,22 @@ var _ = Describe("validateModuleLoaderContainerSpec", func() {
)
})

It("should fail when a kernel-mapping has invalid containerName", func() {
containerSpec := kmmv1beta1.ModuleLoaderContainerSpec{
KernelMappings: []kmmv1beta1.KernelMapping{
{Regexp: "valid-regexp", ContainerImage: "image-url"},
},
}

Expect(
validateModuleLoaderContainerSpec(containerSpec),
).To(
MatchError(
ContainSubstring("failed to validate image format"),
),
)
})

DescribeTable("should fail when InTreeModulesToRemove and InTreeModuleToRemove both defined",
func(inTreeModulesInContainer, inTreeModuleInContainer, inTreeModulesInKM, inTreeModuleInKM bool) {
containerSpec := kmmv1beta1.ModuleLoaderContainerSpec{}
Expand Down Expand Up @@ -412,7 +467,7 @@ var _ = Describe("ValidateUpdate", func() {
Modprobe: kmmv1beta1.ModprobeSpec{
ModuleName: "module-name",
},
ContainerImage: "image-url",
ContainerImage: "image-url:mytag",
KernelMappings: []kmmv1beta1.KernelMapping{
{Regexp: "valid-regexp"},
},
Expand All @@ -430,7 +485,7 @@ var _ = Describe("ValidateUpdate", func() {
Load: []string{"arg-1"}, Unload: []string{"arg-1"},
},
},
ContainerImage: "image-url",
ContainerImage: "image-url:mytag",
KernelMappings: []kmmv1beta1.KernelMapping{
{Regexp: "valid-regexp"},
},
Expand Down

0 comments on commit b6ee602

Please sign in to comment.