Remove intermediate image when we have both build and sign.

When signing is added to a Module CR after an initial unsigned build, the
operator entered an infinite rebuild loop because the sign pod expected an
intermediate image tag (containerImage:ns_name_kmm_unsigned) that was never
populated by the original build.

Changes:
- Remove intermediate image concept (internal/buildsign/resource/common.go)
  - makeBuildTemplate: always pushes directly to mld.ContainerImage; no
    longer redirects to an intermediate tag when sign is defined
  - makeSignTemplate: when build is also present, uses mld.ContainerImage
    as the unsigned source - sign overwrites the final image in place
- Remove IntermediateImageName (internal/module/helper.go) - function is
  now unused

Flow before vs. after:
  Build (with sign): pushed to image:ns_name_kmm_unsigned -> pushes to image
  Sign (with build): read from image:ns_name_kmm_unsigned -> reads from image,
                     overwrites in place

With this change, adding signing to an existing built module works without a
rebuild: the sign pod finds the already-pushed image at the correct tag and
signs it directly.

The intermediate image was initially required because we didn't have the
MIC and MBSC objects to sync the build/sign jobs and the existence of
the image in the image registry was used as a proof that the sign is
done. This is not required anymore since MIC is syncing the sign job
only once the build job has succeed.

Signed-off-by: Yoni Bettan <yonibettan@gmail.com>

Author: ybettan

docs/mkdocs/documentation/secure_boot.md

@@ -163,19 +163,11 @@ spec:
 The YAML below should build a new container image using the
 [source code from the repo](https://github.com/kubernetes-sigs/kernel-module-management/tree/main/ci/kmm-kmod) (this
 kernel module does nothing useful but provides a good example).
-The image produced is saved back in the registry with a temporary name, and this temporary image is then signed using
-the parameters in the `sign` section.
 
-The temporary image name is based on the final image name and is set to be
-`<containerImage>:<tag>-<namespace>_<module name>_kmm_unsigned`.
-
-For example, given the YAML below KMM would build an image named
-`quay.io/chrisp262/minimal-driver:final-default_example-module_kmm_unsigned` containing the build but unsigned kmods,
-and push it to the registry.
-Then it would create a second image, `quay.io/chrisp262/minimal-driver:final` containing the signed kmods.
-It is this second image that will be loaded by the DaemonSet and will deploy the kmods to the cluster nodes.
-
-Once it is signed the temporary image can be safely deleted from the registry (it will be rebuilt if needed).
+The build pod pushes the unsigned image directly to `containerImage`.
+The sign pod then reads from that same image, signs the kernel modules in place, and overwrites `containerImage` with
+the signed result.
+The image loaded by the DaemonSet onto cluster nodes is the final signed image at `containerImage`.
 
 
 ## Example

internal/buildsign/resource/common.go

@@ -261,14 +261,7 @@ func (rm *resourceManager) getResources(ctx context.Context, namespace string, l
 func (rm *resourceManager) makeBuildTemplate(ctx context.Context, mld *api.ModuleLoaderData, owner metav1.Object,
 	pushImage bool) (metav1.Object, error) {
 
-	// if build AND sign are specified, then we will build an intermediate image
-	// and let sign produce the one specified in its targetImage
-	containerImage := mld.ContainerImage
-	if module.ShouldBeSigned(mld) {
-		containerImage = module.IntermediateImageName(mld.Name, mld.Namespace, containerImage)
-	}
-
-	buildSpec := rm.buildSpec(mld, containerImage, pushImage)
+	buildSpec := rm.buildSpec(mld, mld.ContainerImage, pushImage)
 	buildSpecHash, err := rm.getBuildHashAnnotationValue(
 		ctx,
 		mld.Build.DockerfileConfigMap.Name,
@@ -310,13 +303,8 @@ func (rm *resourceManager) makeSignTemplate(ctx context.Context, mld *api.Module
 		DirName:     mld.Modprobe.DirName,
 	}
 
-	imageToSign := ""
 	if module.ShouldBeBuilt(mld) {
-		imageToSign = module.IntermediateImageName(mld.Name, mld.Namespace, mld.ContainerImage)
-	}
-
-	if imageToSign != "" {
-		td.UnsignedImage = imageToSign
+		td.UnsignedImage = mld.ContainerImage
 	} else if signConfig.UnsignedImage != "" {
 		td.UnsignedImage = signConfig.UnsignedImage
 	} else {

internal/buildsign/resource/common_test.go

@@ -411,7 +411,7 @@ var _ = Describe("makeBuildTemplate", func() {
 		Expect(actualPod.Spec.Containers[0].Image).To(Equal("some-build-image:" + customTag))
 	})
 
-	It("should add the kmm_unsigned suffix to the target image if sign is defined", func() {
+	It("should use the final container image as the build destination even when sign is defined", func() {
 		ctx := context.Background()
 
 		mld := api.ModuleLoaderData{
@@ -429,8 +429,6 @@ var _ = Describe("makeBuildTemplate", func() {
 			KernelNormalizedVersion: kernelNormalizedVersion,
 		}
 
-		expectedImageName := mld.ContainerImage + ":" + mld.Namespace + "_" + mld.Name + "_kmm_unsigned"
-
 		gomock.InOrder(
 			mbao.EXPECT().ApplyBuildArgOverrides(buildArgs, defaultBuildArgs),
 			clnt.EXPECT().Get(ctx, types.NamespacedName{Name: dockerfileConfigMap.Name, Namespace: mld.Namespace}, gomock.Any()).DoAndReturn(
@@ -447,7 +445,7 @@ var _ = Describe("makeBuildTemplate", func() {
 
 		Expect(err).NotTo(HaveOccurred())
 		Expect(actualPod.Spec.Containers[0].Args).To(ContainElement("--destination"))
-		Expect(actualPod.Spec.Containers[0].Args).To(ContainElement(expectedImageName))
+		Expect(actualPod.Spec.Containers[0].Args).To(ContainElement(image))
 	})
 })
 
@@ -896,6 +894,45 @@ COPY --from=signimage /opt/modules /modules
 			[]string{"source-extract"},
 		),
 	)
+
+	It("should use the container image as source when build is also defined", func() {
+		GinkgoT().Setenv("RELATED_IMAGE_BUILD", buildImage)
+		GinkgoT().Setenv("RELATED_IMAGE_SIGN", "some-sign-image:some-tag")
+
+		ctx := context.Background()
+		mld.Build = &kmmv1beta1.Build{}
+		mld.Sign = &kmmv1beta1.Sign{
+			KeySecret:   &v1.LocalObjectReference{Name: "securebootkey"},
+			CertSecret:  &v1.LocalObjectReference{Name: "securebootcert"},
+			FilesToSign: []string{"/modules/test.ko"},
+		}
+		mld.ContainerImage = unsignedImage
+		mld.RegistryTLS = &kmmv1beta1.TLSOptions{}
+
+		gomock.InOrder(
+			clnt.EXPECT().Get(ctx, types.NamespacedName{Name: mld.Sign.KeySecret.Name, Namespace: mld.Namespace}, gomock.Any()).DoAndReturn(
+				func(_ interface{}, _ interface{}, secret *v1.Secret, _ ...ctrlclient.GetOption) error {
+					secret.Data = privateSignData
+					return nil
+				},
+			),
+			clnt.EXPECT().Get(ctx, types.NamespacedName{Name: mld.Sign.CertSecret.Name, Namespace: mld.Namespace}, gomock.Any()).DoAndReturn(
+				func(_ interface{}, _ interface{}, secret *v1.Secret, _ ...ctrlclient.GetOption) error {
+					secret.Data = publicSignData
+					return nil
+				},
+			),
+		)
+
+		actual, err := rm.makeSignTemplate(ctx, &mld, mld.Owner, true)
+		Expect(err).NotTo(HaveOccurred())
+		actualPod, ok := actual.(*v1.Pod)
+		Expect(ok).To(BeTrue())
+
+		// The Dockerfile annotation should use the container image (not an intermediate image) as the
+		// unsigned source when build is also present.
+		Expect(actualPod.Annotations["dockerfile"]).To(ContainSubstring(unsignedImage))
+	})
 })
 var _ = Describe("resourceLabels", func() {
 

internal/module/helper.go

@@ -35,19 +35,8 @@ func AppendToTag(name string, tag string) string {
 	return name + separator + tag
 }
 
-// IntermediateImageName returns the image name of the pre-signed module image name
-func IntermediateImageName(name, namespace, targetImage string) string {
-	return AppendToTag(targetImage, namespace+"_"+name+"_kmm_unsigned")
-}
-
 // ShouldBeBuilt indicates whether the specified ModuleLoaderData of the
 // Module should be built or not.
 func ShouldBeBuilt(mld *api.ModuleLoaderData) bool {
 	return mld.Build != nil
 }
-
-// ShouldBeSigned indicates whether the specified ModuleLoaderData of the
-// Module should be signed or not.
-func ShouldBeSigned(mld *api.ModuleLoaderData) bool {
-	return mld.Sign != nil
-}

internal/module/helper_test.go

@@ -28,13 +28,3 @@ var _ = Describe("AppendToTag", func() {
 		)
 	})
 })
-
-var _ = Describe("IntermediateImageName", func() {
-	It("should add the kmm_unsigned suffix to the target image name", func() {
-		Expect(
-			IntermediateImageName("module-name", "test-namespace", "some-image-name"),
-		).To(
-			Equal("some-image-name:test-namespace_module-name_kmm_unsigned"),
-		)
-	})
-})