Separate PullOption for build process and image verification (#32)

yevgeny-shnaidman · web-flow · commit 1b7f0b3e3594 · 2022-09-05T01:54:36.000-07:00
Currently we have pull option in the Build section of the Module API. It is used both
for configuring the pull option passed to the build configuration (kaniko)
and also to the function that verifies images existence (pulling its manifest).
We should separate them into 2 different entities:
1) present in ModuleLoader spec - used for image contents inspection and existence verification
2) present in the Build - used for specifying pull options of the base images of the build process
diff --git a/api/v1beta1/module_types.go b/api/v1beta1/module_types.go
@@ -63,7 +63,7 @@ type Build struct {
 	Dockerfile string `json:"dockerfile"`
 
 	// +optional
-	// Pull contains settings determining how to check if the DriverContainer image already exists.
+	// Pull contains settings determining how to pull the base images of the build process.
 	Pull PullOptions `json:"pull"`
 
 	// +optional
@@ -96,6 +96,11 @@ type KernelMapping struct {
 	// Literal defines a literal target kernel version to be matched exactly against node kernels.
 	Literal string `json:"literal"`
 
+	// +optional
+	// Pull contains settings determining how to check if the ModuleLoader image already exists
+	// and allows overriding of the ModuleLoader's pull options
+	Pull *PullOptions `json:"pull"`
+
 	// +optional
 	// Regexp is a regular expression to be match against node kernels.
 	Regexp string `json:"regexp"`
@@ -162,6 +167,10 @@ type ModuleLoaderContainerSpec struct {
 
 	// Modprobe is a set of properties to customize which module modprobe loads and with which properties.
 	Modprobe ModprobeSpec `json:"modprobe"`
+
+	// +optional
+	// Pull contains settings determining how to check if the ModuleLoader image already exists.
+	Pull *PullOptions `json:"pull"`
 }
 
 type ModuleLoaderSpec struct {
diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/ci/module-kmm-ci-build.template.yaml b/ci/module-kmm-ci-build.template.yaml
@@ -11,6 +11,8 @@ spec:
       kernelMappings:
         - literal: KVER_CHANGEME
           containerImage: registry.minikube/kmm-kmod:local
+          pull:
+            insecure: true
           build:
             buildArgs:
               - name: CI_BUILD_ARG
diff --git a/config/crd/bases/kmm.sigs.k8s.io_modules.yaml b/config/crd/bases/kmm.sigs.k8s.io_modules.yaml
@@ -1880,7 +1880,7 @@ spec:
                             type: object
                           pull:
                             description: Pull contains settings determining how to
-                              check if the DriverContainer image already exists.
+                              pull the base images of the build process.
                             properties:
                               insecure:
                                 description: If Insecure is true, images can be pulled
@@ -1977,8 +1977,7 @@ spec:
                                   type: object
                                 pull:
                                   description: Pull contains settings determining
-                                    how to check if the DriverContainer image already
-                                    exists.
+                                    how to pull the base images of the build process.
                                   properties:
                                     insecure:
                                       description: If Insecure is true, images can
@@ -2035,6 +2034,20 @@ spec:
                               description: Literal defines a literal target kernel
                                 version to be matched exactly against node kernels.
                               type: string
+                            pull:
+                              description: Pull contains settings determining how
+                                to check if the ModuleLoader image already exists
+                                and allows overriding of the ModuleLoader's pull options
+                              properties:
+                                insecure:
+                                  description: If Insecure is true, images can be
+                                    pulled from an insecure (plain HTTP) registry.
+                                  type: boolean
+                                insecureSkipTLSVerify:
+                                  description: If InsecureSkipTLSVerify, the operator
+                                    will accept any certificate provided by the registry.
+                                  type: boolean
+                              type: object
                             regexp:
                               description: Regexp is a regular expression to be match
                                 against node kernels.
@@ -2111,6 +2124,19 @@ spec:
                         required:
                         - moduleName
                         type: object
+                      pull:
+                        description: Pull contains settings determining how to check
+                          if the ModuleLoader image already exists.
+                        properties:
+                          insecure:
+                            description: If Insecure is true, images can be pulled
+                              from an insecure (plain HTTP) registry.
+                            type: boolean
+                          insecureSkipTLSVerify:
+                            description: If InsecureSkipTLSVerify, the operator will
+                              accept any certificate provided by the registry.
+                            type: boolean
+                        type: object
                     required:
                     - kernelMappings
                     - modprobe
diff --git a/internal/build/helper.go b/internal/build/helper.go
@@ -12,6 +12,7 @@ import (
 type Helper interface {
 	ApplyBuildArgOverrides(args []v1beta1.BuildArg, overrides ...v1beta1.BuildArg) []v1beta1.BuildArg
 	GetRelevantBuild(mod kmmv1beta1.Module, km kmmv1beta1.KernelMapping) *kmmv1beta1.Build
+	GetRelevantPullOptions(mod kmmv1beta1.Module, km kmmv1beta1.KernelMapping) *kmmv1beta1.PullOptions
 }
 
 type helper struct{}
@@ -67,3 +68,10 @@ func (m *helper) GetRelevantBuild(mod kmmv1beta1.Module, km kmmv1beta1.KernelMap
 	buildConfig.Secrets = append(buildConfig.Secrets, km.Build.Secrets...)
 	return buildConfig
 }
+
+func (m *helper) GetRelevantPullOptions(mod kmmv1beta1.Module, km kmmv1beta1.KernelMapping) *kmmv1beta1.PullOptions {
+	if km.Pull != nil {
+		return km.Pull
+	}
+	return mod.Spec.ModuleLoader.Container.Pull
+}
diff --git a/internal/build/job/manager.go b/internal/build/job/manager.go
@@ -66,6 +66,7 @@ func (jbm *jobManager) Sync(ctx context.Context, mod kmmv1beta1.Module, m kmmv1b
 	logger := log.FromContext(ctx)
 
 	buildConfig := jbm.helper.GetRelevantBuild(mod, m)
+	imagePullOptions := jbm.helper.GetRelevantPullOptions(mod, m)
 
 	var registryAuthGetter auth.RegistryAuthGetter
 
@@ -76,7 +77,7 @@ func (jbm *jobManager) Sync(ctx context.Context, mod kmmv1beta1.Module, m kmmv1b
 		}
 		registryAuthGetter = auth.NewRegistryAuthGetter(jbm.client, namespacedName)
 	}
-	imageAvailable, err := jbm.registry.ImageExists(ctx, m.ContainerImage, buildConfig.Pull, registryAuthGetter)
+	imageAvailable, err := jbm.registry.ImageExists(ctx, m.ContainerImage, imagePullOptions, registryAuthGetter)
 	if err != nil {
 		return build.Result{}, fmt.Errorf("could not check if the image is available: %v", err)
 	}
diff --git a/internal/build/job/manager_test.go b/internal/build/job/manager_test.go
@@ -60,17 +60,18 @@ var _ = Describe("JobManager", func() {
 			helper = build.NewMockHelper(ctrl)
 		})
 
-		po := kmmv1beta1.PullOptions{}
+		po := &kmmv1beta1.PullOptions{}
 
 		km := kmmv1beta1.KernelMapping{
-			Build:          &kmmv1beta1.Build{Pull: po},
+			Build:          &kmmv1beta1.Build{Pull: *po},
 			ContainerImage: imageName,
 		}
 
 		It("should return an error if there was an error getting the image", func() {
 			ctx := context.Background()
 			gomock.InOrder(
 				helper.EXPECT().GetRelevantBuild(gomock.Any(), km).Return(km.Build),
+				helper.EXPECT().GetRelevantPullOptions(gomock.Any(), km).Return(po),
 				registry.EXPECT().ImageExists(ctx, imageName, po, gomock.Any()).Return(false, errors.New("random error")),
 			)
 			mgr := NewBuildManager(nil, registry, maker, helper)
@@ -84,6 +85,7 @@ var _ = Describe("JobManager", func() {
 
 			gomock.InOrder(
 				helper.EXPECT().GetRelevantBuild(gomock.Any(), km).Return(km.Build),
+				helper.EXPECT().GetRelevantPullOptions(gomock.Any(), km).Return(po),
 				registry.EXPECT().ImageExists(ctx, imageName, po, gomock.Any()).Return(true, nil),
 			)
 
@@ -125,6 +127,7 @@ var _ = Describe("JobManager", func() {
 
 				gomock.InOrder(
 					helper.EXPECT().GetRelevantBuild(mod, km).Return(km.Build),
+					helper.EXPECT().GetRelevantPullOptions(gomock.Any(), km).Return(po),
 					registry.EXPECT().ImageExists(ctx, imageName, po, gomock.Any()).Return(false, nil),
 				)
 
@@ -149,6 +152,7 @@ var _ = Describe("JobManager", func() {
 
 			gomock.InOrder(
 				helper.EXPECT().GetRelevantBuild(mod, km).Return(km.Build),
+				helper.EXPECT().GetRelevantPullOptions(gomock.Any(), km).Return(po),
 				registry.EXPECT().ImageExists(ctx, imageName, po, gomock.Any()),
 				maker.EXPECT().MakeJob(mod, km.Build, kernelVersion, km.ContainerImage, true).Return(nil, errors.New("random error")),
 			)
@@ -179,6 +183,7 @@ var _ = Describe("JobManager", func() {
 
 			gomock.InOrder(
 				helper.EXPECT().GetRelevantBuild(mod, km).Return(km.Build),
+				helper.EXPECT().GetRelevantPullOptions(gomock.Any(), km).Return(po),
 				registry.EXPECT().ImageExists(ctx, imageName, po, gomock.Any()),
 				maker.EXPECT().MakeJob(mod, km.Build, kernelVersion, km.ContainerImage, true).Return(&j, nil),
 			)
@@ -216,6 +221,7 @@ var _ = Describe("JobManager", func() {
 
 			gomock.InOrder(
 				helper.EXPECT().GetRelevantBuild(mod, km).Return(km.Build),
+				helper.EXPECT().GetRelevantPullOptions(gomock.Any(), km).Return(po),
 				registry.EXPECT().ImageExists(ctx, imageName, po, gomock.Any()).DoAndReturn(
 					func(_ interface{}, _ interface{}, _ interface{}, registryAuthGetter auth.RegistryAuthGetter) (bool, error) {
 						Expect(registryAuthGetter).ToNot(BeNil())
diff --git a/internal/build/mock_helper.go b/internal/build/mock_helper.go
diff --git a/internal/registry/mock_registry_api.go b/internal/registry/mock_registry_api.go
diff --git a/internal/registry/registry.go b/internal/registry/registry.go
@@ -40,7 +40,7 @@ type RepoPullConfig struct {
 //go:generate mockgen -source=registry.go -package=registry -destination=mock_registry_api.go
 
 type Registry interface {
-	ImageExists(ctx context.Context, image string, po kmmv1beta1.PullOptions, registryAuthGetter auth.RegistryAuthGetter) (bool, error)
+	ImageExists(ctx context.Context, image string, po *kmmv1beta1.PullOptions, registryAuthGetter auth.RegistryAuthGetter) (bool, error)
 	VerifyModuleExists(layer v1.Layer, pathPrefix, kernelVersion, moduleFileName string) bool
 	GetLayersDigests(ctx context.Context, image string, registryAuthGetter auth.RegistryAuthGetter) ([]string, *RepoPullConfig, error)
 	GetLayerByDigest(digest string, pullConfig *RepoPullConfig) (v1.Layer, error)
@@ -52,8 +52,8 @@ func NewRegistry() Registry {
 	return &registry{}
 }
 
-func (r *registry) ImageExists(ctx context.Context, image string, po kmmv1beta1.PullOptions, registryAuthGetter auth.RegistryAuthGetter) (bool, error) {
-	pullConfig, err := r.getPullOptions(ctx, image, &po, registryAuthGetter)
+func (r *registry) ImageExists(ctx context.Context, image string, po *kmmv1beta1.PullOptions, registryAuthGetter auth.RegistryAuthGetter) (bool, error) {
+	pullConfig, err := r.getPullOptions(ctx, image, po, registryAuthGetter)
 	if err != nil {
 		return false, fmt.Errorf("failed to get pull options for image %s: %w", image, err)
 	}
diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go
@@ -58,7 +58,7 @@ var _ = Describe("ImageExists", func() {
 
 		It("should fail if the image name isn't valid", func() {
 
-			_, err = reg.ImageExists(ctx, invalidImage, kmmv1beta1.PullOptions{}, nil)
+			_, err = reg.ImageExists(ctx, invalidImage, &kmmv1beta1.PullOptions{}, nil)
 
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("does not contain hash or tag"))
@@ -68,7 +68,7 @@ var _ = Describe("ImageExists", func() {
 
 			mockRegistryAuthGetter.EXPECT().GetKeyChain(ctx).Return(nil, errors.New("some error"))
 
-			_, err = reg.ImageExists(ctx, validImage, kmmv1beta1.PullOptions{}, mockRegistryAuthGetter)
+			_, err = reg.ImageExists(ctx, validImage, &kmmv1beta1.PullOptions{}, mockRegistryAuthGetter)
 
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("cannot get keychain from the registry auth getter"))
@@ -93,7 +93,7 @@ var _ = Describe("ImageExists", func() {
 			u := mustParseURL(server.URL)
 
 			image := fmt.Sprintf("%s/%s/%s:%s", u.Host, validImageOrg, validImageName, validImageTag)
-			_, err = reg.ImageExists(ctx, image, kmmv1beta1.PullOptions{}, nil)
+			_, err = reg.ImageExists(ctx, image, &kmmv1beta1.PullOptions{}, nil)
 
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("failed to get crane manifest from image"))
@@ -108,7 +108,7 @@ var _ = Describe("ImageExists", func() {
 			u := mustParseURL(server.URL)
 
 			image := fmt.Sprintf("%s/%s/%s:%s", u.Host, validImageOrg, validImageName, validImageTag)
-			_, err = reg.ImageExists(ctx, image, kmmv1beta1.PullOptions{}, nil)
+			_, err = reg.ImageExists(ctx, image, &kmmv1beta1.PullOptions{}, nil)
 
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("failed to unmarshal crane manifest"))
@@ -134,7 +134,7 @@ var _ = Describe("ImageExists", func() {
 			u := mustParseURL(server.URL)
 
 			image := fmt.Sprintf("%s/%s/%s:%s", u.Host, validImageOrg, validImageName, validImageTag)
-			_, err = reg.ImageExists(ctx, image, kmmv1beta1.PullOptions{}, nil)
+			_, err = reg.ImageExists(ctx, image, &kmmv1beta1.PullOptions{}, nil)
 
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("mediaType is missing from the image"))
@@ -150,7 +150,7 @@ var _ = Describe("ImageExists", func() {
 		u := mustParseURL(server.URL)
 
 		image := fmt.Sprintf("%s/%s/%s:%s", u.Host, validImageOrg, validImageName, validImageTag)
-		_, err := reg.ImageExists(ctx, image, kmmv1beta1.PullOptions{}, nil)
+		_, err := reg.ImageExists(ctx, image, &kmmv1beta1.PullOptions{}, nil)
 
 		Expect(err).ToNot(HaveOccurred())
 	})
@@ -173,9 +173,9 @@ var _ = Describe("ImageExists", func() {
 		var err error
 		image := fmt.Sprintf("%s/%s/%s:%s", u.Host, validImageOrg, validImageName, validImageTag)
 		if withRegistryAuthGetter {
-			_, err = reg.ImageExists(ctx, image, kmmv1beta1.PullOptions{}, mockRegistryAuthGetter)
+			_, err = reg.ImageExists(ctx, image, &kmmv1beta1.PullOptions{}, mockRegistryAuthGetter)
 		} else {
-			_, err = reg.ImageExists(ctx, image, kmmv1beta1.PullOptions{}, nil)
+			_, err = reg.ImageExists(ctx, image, &kmmv1beta1.PullOptions{}, nil)
 		}
 		Expect(err).ToNot(HaveOccurred())
 	},
@@ -223,7 +223,7 @@ var _ = Describe("GetLayersDigests", func() {
 
 		It("should fail if the image name isn't valid", func() {
 
-			_, err = reg.ImageExists(ctx, invalidImage, kmmv1beta1.PullOptions{}, nil)
+			_, err = reg.ImageExists(ctx, invalidImage, &kmmv1beta1.PullOptions{}, nil)
 
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("does not contain hash or tag"))
@@ -233,7 +233,7 @@ var _ = Describe("GetLayersDigests", func() {
 
 			mockRegistryAuthGetter.EXPECT().GetKeyChain(ctx).Return(nil, errors.New("some error"))
 
-			_, err = reg.ImageExists(ctx, validImage, kmmv1beta1.PullOptions{}, mockRegistryAuthGetter)
+			_, err = reg.ImageExists(ctx, validImage, &kmmv1beta1.PullOptions{}, mockRegistryAuthGetter)
 
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("cannot get keychain from the registry auth getter"))

Original file line number	Diff line number	Diff line change
`@@ -66,6 +66,7 @@ func (jbm *jobManager) Sync(ctx context.Context, mod kmmv1beta1.Module, m kmmv1b`
`66`	`66`	`logger := log.FromContext(ctx)`
`67`	`67`
`68`	`68`	`buildConfig := jbm.helper.GetRelevantBuild(mod, m)`
	`69`	`+ imagePullOptions := jbm.helper.GetRelevantPullOptions(mod, m)`
`69`	`70`
`70`	`71`	`var registryAuthGetter auth.RegistryAuthGetter`
`71`	`72`
`@@ -76,7 +77,7 @@ func (jbm *jobManager) Sync(ctx context.Context, mod kmmv1beta1.Module, m kmmv1b`
`76`	`77`	`}`
`77`	`78`	`registryAuthGetter = auth.NewRegistryAuthGetter(jbm.client, namespacedName)`
`78`	`79`	`}`
`79`		`- imageAvailable, err := jbm.registry.ImageExists(ctx, m.ContainerImage, buildConfig.Pull, registryAuthGetter)`
	`80`	`+ imageAvailable, err := jbm.registry.ImageExists(ctx, m.ContainerImage, imagePullOptions, registryAuthGetter)`
`80`	`81`	`if err != nil {`
`81`	`82`	`return build.Result{}, fmt.Errorf("could not check if the image is available: %v", err)`
`82`	`83`	`}`