Merge pull request konflux-ci#799 from hongweiliu17/STONEINTG-966

feat(STONEINTG-966): ensure override snapshot is valid to be released

docs/snapshot-controller.md

@@ -98,6 +98,22 @@ flowchart TD
   rerun_static_env                ---->    remove_rerun_label
 
 
+  %%%%%%%%%%%%%%%%%%%%%%% Drawing EnsureOverrideSnapshotValid() function
+
+  %% Node definitions
+  ensure4(Process further if: Snapshot has override type label)
+  validate_override_valid{Is the override snapshot <br>defined with valid snapshotComponents, <br>image digest, and git source?}
+  mark_snapshot_invalid(<b>Mark</b> override snapshot as invalid)
+  continue_processing4(Controller continues processing...)
+
+  %% Node connections
+  predicate                       ---->    |"EnsureOverrideSnapshotValid()"|ensure4
+  ensure4                         -->      validate_override_valid
+  validate_override_valid         --Yes--> continue_processing4
+  validate_override_valid         --No-->  mark_snapshot_invalid
+  mark_snapshot_invalid           -->      continue_processing4
+
+
   %% Assigning styles to nodes
   class predicate Amber;
   class encountered_error1,encountered_error31,encountered_error32,encountered_error5 Red;

gitops/snapshot.go

@@ -476,6 +476,13 @@ func ValidateImageDigest(imageUrl string) error {
 	return err
 }
 
+// HaveGitSource checks if snapshotComponent contains non-empty source.git field
+// and have both url and revision fields defined
+func HaveGitSource(snapshotComponent applicationapiv1alpha1.SnapshotComponent) bool {
+	return reflect.ValueOf(snapshotComponent.Source).IsValid() && snapshotComponent.Source.GitSource != nil &&
+		snapshotComponent.Source.GitSource.Revision != "" && snapshotComponent.Source.GitSource.URL != ""
+}
+
 // HaveAppStudioTestsFinished checks if the AppStudio tests have finished by checking if the AppStudio Test Succeeded condition is set.
 func HaveAppStudioTestsFinished(snapshot *applicationapiv1alpha1.Snapshot) bool {
 	statusCondition := meta.FindStatusCondition(snapshot.Status.Conditions, AppStudioTestSucceededCondition)
@@ -865,3 +872,16 @@ func IsComponentSnapshot(snapshot *applicationapiv1alpha1.Snapshot) bool {
 func IsComponentSnapshotCreatedByPACPushEvent(snapshot *applicationapiv1alpha1.Snapshot) bool {
 	return IsComponentSnapshot(snapshot) && IsSnapshotCreatedByPACPushEvent(snapshot)
 }
+
+func SetOwnerReference(ctx context.Context, adapterClient client.Client, snapshot *applicationapiv1alpha1.Snapshot, owner *applicationapiv1alpha1.Application) (*applicationapiv1alpha1.Snapshot, error) {
+	patch := client.MergeFrom(snapshot.DeepCopy())
+	err := ctrl.SetControllerReference(owner, snapshot, adapterClient.Scheme())
+	if err != nil {
+		return snapshot, err
+	}
+	err = adapterClient.Patch(ctx, snapshot, patch)
+	if err != nil {
+		return snapshot, err
+	}
+	return snapshot, nil
+}

gitops/snapshot_test.go

@@ -30,6 +30,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -608,12 +609,19 @@ var _ = Describe("Gitops functions for managing Snapshots", Ordered, func() {
 			BeforeEach(func() {
 				overrideSnapshot = hasSnapshot.DeepCopy()
 				overrideSnapshot.Labels[gitops.SnapshotTypeLabel] = gitops.SnapshotOverrideType
+				Expect(controllerutil.HasControllerReference(overrideSnapshot)).To(BeFalse())
 			})
 
 			It("make sure correct label is returned in overrideSnapshot", func() {
 				isOverrideSnapshot := gitops.IsOverrideSnapshot(overrideSnapshot)
 				Expect(isOverrideSnapshot).To(BeTrue())
 			})
+
+			It("Can set owner reference for override snapshot", func() {
+				overrideSnapshot, err := gitops.SetOwnerReference(ctx, k8sClient, overrideSnapshot, hasApp)
+				Expect(controllerutil.HasControllerReference(overrideSnapshot)).To(BeTrue())
+				Expect(err).ToNot(HaveOccurred())
+			})
 		})
 
 	})

internal/controller/snapshot/snapshot_adapter.go

@@ -28,6 +28,7 @@ import (
 	clienterrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/konflux-ci/integration-service/api/v1beta2"
 	"github.com/konflux-ci/integration-service/gitops"
@@ -387,6 +388,7 @@ func (a *Adapter) EnsureAllReleasesExist() (controller.OperationResult, error) {
 			"reasons", strings.Join(reasons, ","))
 		return controller.ContinueProcessing()
 	}
+
 	if gitops.IsSnapshotMarkedAsAutoReleased(a.snapshot) {
 		a.logger.Info("The Snapshot was previously auto-released, skipping auto-release.")
 		return controller.ContinueProcessing()
@@ -439,6 +441,70 @@ func (a *Adapter) EnsureAllReleasesExist() (controller.OperationResult, error) {
 	return controller.ContinueProcessing()
 }
 
+// EnsureOverrideSnapshotValid is an operation that ensure the manually created override snapshot have valid
+// digest and git source in snapshotComponents, mark it as invalid otherwise
+func (a *Adapter) EnsureOverrideSnapshotValid() (controller.OperationResult, error) {
+	if !gitops.IsOverrideSnapshot(a.snapshot) {
+		a.logger.Info("The snapshot was not override snapshot, skipping")
+		return controller.ContinueProcessing()
+	}
+
+	if gitops.IsSnapshotMarkedAsInvalid(a.snapshot) {
+		a.logger.Info("The override snapshot has been marked as invalid, skipping")
+		return controller.ContinueProcessing()
+	}
+
+	var err error
+	if !controllerutil.HasControllerReference(a.snapshot) {
+		a.snapshot, err = gitops.SetOwnerReference(a.context, a.client, a.snapshot, a.application)
+		if err != nil {
+			a.logger.Error(err, fmt.Sprintf("Failed to set owner reference for snapshot %s/%s", a.snapshot.Namespace, a.snapshot.Name))
+			return controller.RequeueWithError(err)
+		}
+		a.logger.Info("Owner reference has been set to snapshot")
+	}
+
+	// validate all snapshotComponents' containerImages/source in snapshot, make all errors joined
+	var errsForSnapshot error
+	for _, snapshotComponent := range a.snapshot.Spec.Components {
+		snapshotComponent := snapshotComponent //G601
+		_, err := a.loader.GetComponent(a.context, a.client, snapshotComponent.Name, a.snapshot.Namespace)
+		if err != nil {
+			a.logger.Error(err, "Failed to get component from applicaion", "application.Name", a.application.Name, "component.Name", snapshotComponent.Name)
+			_, loaderError := h.HandleLoaderError(a.logger, err, snapshotComponent.Name, a.application.Name)
+			if loaderError != nil {
+				return controller.RequeueWithError(loaderError)
+			} else {
+				errsForSnapshot = errors.Join(errsForSnapshot, fmt.Errorf("snapshotComponent %s defined in snapshot %s doesn't exist under application %s/%s", snapshotComponent.Name, a.snapshot.Name, a.application.Namespace, a.application.Name))
+			}
+		}
+
+		err = gitops.ValidateImageDigest(snapshotComponent.ContainerImage)
+		if err != nil {
+			a.logger.Error(err, "containerImage in snapshotComponent has invalid digest", "snapshotComponent.Name", snapshotComponent.Name, "snapshotComponent.ContainerImage", snapshotComponent.ContainerImage)
+			errsForSnapshot = errors.Join(errsForSnapshot, err)
+		}
+
+		if !gitops.HaveGitSource(snapshotComponent) {
+			a.logger.Error(err, "snapshotComponent has no git url/revision fields defined", "snapshotComponent.Name", snapshotComponent.Name, "snapshotComponent.ContainerImage", snapshotComponent.ContainerImage)
+			errsForSnapshot = errors.Join(errsForSnapshot, err)
+		}
+	}
+
+	if errsForSnapshot != nil {
+		a.logger.Error(errsForSnapshot, "mark the override snapshot as invalid due to invalid snapshotComponent")
+		err = gitops.MarkSnapshotAsInvalid(a.context, a.client, a.snapshot, errsForSnapshot.Error())
+		if err != nil {
+			a.logger.Error(err, "Failed to update snapshot to Invalid",
+				"snapshot.Namespace", a.snapshot.Namespace, "snapshot.Name", a.snapshot.Name)
+			return controller.RequeueWithError(err)
+		}
+		a.logger.Info("Snapshot has been marked as invalid due to invalid snapshotComponent",
+			"snapshot.Namespace", a.snapshot.Namespace, "snapshot.Name", a.snapshot.Name)
+	}
+	return controller.ContinueProcessing()
+}
+
 // createMissingReleasesForReleasePlans checks if there's existing Releases for a given list of ReleasePlans and creates
 // new ones if they are missing. In case the Releases can't be created, an error will be returned.
 func (a *Adapter) createMissingReleasesForReleasePlans(application *applicationapiv1alpha1.Application, releasePlans *[]releasev1alpha1.ReleasePlan, snapshot *applicationapiv1alpha1.Snapshot) error {

internal/controller/snapshot/snapshot_adapter_test.go

@@ -24,6 +24,7 @@ import (
 	"time"
 
 	"github.com/tonglil/buflogr"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -63,6 +64,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() {
 		hasSnapshotPR                             *applicationapiv1alpha1.Snapshot
 		hasOverRideSnapshot                       *applicationapiv1alpha1.Snapshot
 		hasInvalidSnapshot                        *applicationapiv1alpha1.Snapshot
+		hasInvalidOverrideSnapshot                *applicationapiv1alpha1.Snapshot
 		integrationTestScenario                   *v1beta2.IntegrationTestScenario
 		integrationTestScenarioForInvalidSnapshot *v1beta2.IntegrationTestScenario
 		env                                       *applicationapiv1alpha1.Environment
@@ -284,6 +286,45 @@ var _ = Describe("Snapshot Adapter", Ordered, func() {
 		}
 		Expect(k8sClient.Create(ctx, hasOverRideSnapshot)).Should(Succeed())
 
+		hasInvalidOverrideSnapshot = &applicationapiv1alpha1.Snapshot{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "snapshotpr-sample-override-invalid",
+				Namespace: "default",
+				Labels: map[string]string{
+					gitops.SnapshotTypeLabel: gitops.SnapshotOverrideType,
+				},
+			},
+			Spec: applicationapiv1alpha1.SnapshotSpec{
+				Application: hasApp.Name,
+				Components: []applicationapiv1alpha1.SnapshotComponent{
+					{
+						Name:           hasComp.Name,
+						ContainerImage: sample_image + "@" + sampleDigest,
+						Source: applicationapiv1alpha1.ComponentSource{
+							ComponentSourceUnion: applicationapiv1alpha1.ComponentSourceUnion{
+								GitSource: &applicationapiv1alpha1.GitSource{
+									Revision: sample_revision,
+								},
+							},
+						},
+					},
+					{
+						Name:           hasCompMissingImageDigest.Name,
+						ContainerImage: sample_image,
+						Source: applicationapiv1alpha1.ComponentSource{
+							ComponentSourceUnion: applicationapiv1alpha1.ComponentSourceUnion{
+								GitSource: &applicationapiv1alpha1.GitSource{
+									URL:      SampleRepoLink,
+									Revision: sample_revision,
+								},
+							},
+						},
+					},
+				},
+			},
+		}
+		Expect(k8sClient.Create(ctx, hasInvalidOverrideSnapshot)).Should(Succeed())
+
 		Eventually(func() error {
 			err := k8sClient.Get(ctx, types.NamespacedName{
 				Name:      hasSnapshot.Name,
@@ -300,6 +341,8 @@ var _ = Describe("Snapshot Adapter", Ordered, func() {
 		Expect(err == nil || errors.IsNotFound(err)).To(BeTrue())
 		err = k8sClient.Delete(ctx, hasOverRideSnapshot)
 		Expect(err == nil || errors.IsNotFound(err)).To(BeTrue())
+		err = k8sClient.Delete(ctx, hasInvalidOverrideSnapshot)
+		Expect(err == nil || errors.IsNotFound(err)).To(BeTrue())
 	})
 
 	AfterAll(func() {
@@ -1210,6 +1253,47 @@ var _ = Describe("Snapshot Adapter", Ordered, func() {
 		})
 	})
 
+	When("Adapter is created for override snapshot", func() {
+		var buf bytes.Buffer
+
+		It("ensures override snapshot with invalid snapshotComponent is marked as invalid", func() {
+			log := helpers.IntegrationLogger{Logger: buflogr.NewWithBuffer(&buf)}
+			Expect(gitops.IsSnapshotValid(hasInvalidOverrideSnapshot)).To(BeTrue())
+			Expect(controllerutil.HasControllerReference(hasInvalidOverrideSnapshot)).To(BeFalse())
+			adapter = NewAdapter(ctx, hasInvalidOverrideSnapshot, hasApp, log, loader.NewMockLoader(), k8sClient)
+			adapter.context = toolkit.GetMockedContext(ctx, []toolkit.MockData{
+				{
+					ContextKey: loader.ApplicationContextKey,
+					Resource:   hasApp,
+				},
+				{
+					ContextKey: loader.ComponentContextKey,
+					Resource:   hasComp,
+				},
+				{
+					ContextKey: loader.SnapshotContextKey,
+					Resource:   hasInvalidOverrideSnapshot,
+				},
+				{
+					ContextKey: loader.SnapshotComponentsContextKey,
+					Resource:   []applicationapiv1alpha1.Component{*hasComp, *hasCompMissingImageDigest},
+				},
+			})
+
+			result, err := adapter.EnsureOverrideSnapshotValid()
+			Expect(result.CancelRequest).To(BeFalse())
+			Expect(result.RequeueRequest).To(BeFalse())
+			Expect(controllerutil.HasControllerReference(hasInvalidOverrideSnapshot)).To(BeTrue())
+			Expect(buf.String()).Should(ContainSubstring("Snapshot has been marked as invalid"))
+			Expect(err).ToNot(HaveOccurred())
+
+			result, err = adapter.EnsureOverrideSnapshotValid()
+			Expect(buf.String()).Should(ContainSubstring("The override snapshot has been marked as invalid, skipping"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(result.CancelRequest).To(BeFalse())
+			Expect(result.RequeueRequest).To(BeFalse())
+		})
+	})
 })
 
 func getAllIntegrationPipelineRunsForSnapshot(ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) ([]tektonv1.PipelineRun, error) {

internal/controller/snapshot/snapshot_controller.go

@@ -117,6 +117,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	adapter := NewAdapter(ctx, snapshot, application, logger, loader, r.Client)
 
 	return controller.ReconcileHandler([]controller.Operation{
+		adapter.EnsureOverrideSnapshotValid,
 		adapter.EnsureAllReleasesExist,
 		adapter.EnsureGlobalCandidateImageUpdated,
 		adapter.EnsureRerunPipelineRunsExist,
@@ -130,6 +131,7 @@ type AdapterInterface interface {
 	EnsureRerunPipelineRunsExist() (controller.OperationResult, error)
 	EnsureIntegrationPipelineRunsExist() (controller.OperationResult, error)
 	EnsureGlobalCandidateImageUpdated() (controller.OperationResult, error)
+	EnsureOverrideSnapshotValid() (controller.OperationResult, error)
 }
 
 // SetupController creates a new Integration controller and adds it to the Manager.