From 6a741953ff6dca8698704a5f5901414c8d13347e Mon Sep 17 00:00:00 2001 From: Hongwei Liu Date: Thu, 4 Jul 2024 19:19:39 +0800 Subject: [PATCH] feat(STONEINTG-966): ensure override snapshot is valid to be released * Validate Override snapshot to release it and mark it as invalid if it doesn't follow: the snapshotComponent should exist as component containerImage in snapshotComponent has valid digest snapshotComponent has git source defined Signed-off-by: Hongwei Liu --- docs/snapshot-controller.md | 32 ++++---- gitops/snapshot.go | 19 +++++ gitops/snapshot_test.go | 8 ++ .../controller/snapshot/snapshot_adapter.go | 69 ++++++++++++++++ .../snapshot/snapshot_adapter_test.go | 78 +++++++++++++++++++ .../snapshot/snapshot_controller.go | 2 + 6 files changed, 192 insertions(+), 16 deletions(-) diff --git a/docs/snapshot-controller.md b/docs/snapshot-controller.md index 55d577d8a..777bf6a04 100644 --- a/docs/snapshot-controller.md +++ b/docs/snapshot-controller.md @@ -80,25 +80,25 @@ flowchart TD encountered_error32 --Yes--> mark_snapshot_Invalid3 - %%%%%%%%%%%%%%%%%%%%%%% Drawing EnsureRerunPipelineRunsExist() function + %%%%%%%%%%%%%%%%%%%%%%% Drawing EnsureIntegrationPipelineRunsExist() function + + %% Assigning styles to nodes + class predicate Amber; + class encountered_error1,encountered_error31,encountered_error32,encountered_error5 Red; + + + %%%%%%%%%%%%%%%%%%%%%%% Drawing EnsureOverrideSnapshotValid() function %% Node definitions - ensure6(Process further if: Snapshot has re-run label added by a user) - if_scenario_exist{Does scenario requested by user exist?} - remove_rerun_label(Remove rerun label) - rerun_static_env(Rerun static env pipeline for scenario) + ensure4(Process further if: Snapshot has override type label) + validate_override_valid{Check if override snapshot has
valid image digest and git source} + mark_snapshot_invalid(Mark overide snapshot as invalid) continue_processing6(Controller continues processing...) %% Node connections - predicate ----> |"EnsureRerunPipelineRunsExist()"|ensure6 - ensure6 --> if_scenario_exist - if_scenario_exist --Yes--> rerun_static_env - if_scenario_exist --No--> remove_rerun_label - remove_rerun_label ----> continue_processing6 - rerun_static_env ----> remove_rerun_label - - - %% Assigning styles to nodes - class predicate Amber; - class encountered_error1,encountered_error31,encountered_error32,encountered_error5 Red; + predicate ----> |"EnsureOverrideSnapshotValid()"|ensure4 + ensure4 --> validate_override_valid + validate_override_valid --Yes--> continue_processing6 + validate_override_valid --No--> mark_snapshot_invalid + mark_snapshot_invalid --> continue_processing6 ``` diff --git a/gitops/snapshot.go b/gitops/snapshot.go index 20893e6de..cb4da5bf0 100644 --- a/gitops/snapshot.go +++ b/gitops/snapshot.go @@ -473,6 +473,12 @@ func ValidateImageDigest(imageUrl string) error { return err } +// HaveGitSource checks if snapshotComponent contains git source +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) @@ -864,3 +870,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 +} diff --git a/gitops/snapshot_test.go b/gitops/snapshot_test.go index 90b5229ab..e20253611 100644 --- a/gitops/snapshot_test.go +++ b/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()) + }) }) }) diff --git a/internal/controller/snapshot/snapshot_adapter.go b/internal/controller/snapshot/snapshot_adapter.go index 22bd84e46..9e64202d0 100644 --- a/internal/controller/snapshot/snapshot_adapter.go +++ b/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" @@ -385,6 +386,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() @@ -433,6 +435,73 @@ 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 if it is invalid +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)) + continue + } + } + + err = gitops.ValidateImageDigest(snapshotComponent.ContainerImage) + if err != nil { + a.logger.Error(err, "containerImage in snapshotComponent has no valid digest", "snapshotComponent.Name", snapshotComponent.Name, "snapshotComponent.ContainerImage", snapshotComponent.ContainerImage) + errsForSnapshot = errors.Join(errsForSnapshot, err) + continue + } + + if !gitops.HaveGitSource(snapshotComponent) { + a.logger.Error(err, "snapshotComponent has no git url/revision", "snapshotComponent.Name", snapshotComponent.Name, "snapshotComponent.ContainerImage", snapshotComponent.ContainerImage) + errsForSnapshot = errors.Join(errsForSnapshot, err) + continue + } + } + + if errsForSnapshot != nil { + a.logger.Error(errsForSnapshot, "mark the override snapshot as invalid due to invalid containerImage or git source") + 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 containerImage or git source", + "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 { diff --git a/internal/controller/snapshot/snapshot_adapter_test.go b/internal/controller/snapshot/snapshot_adapter_test.go index 7bfbedf00..92b2a16a4 100644 --- a/internal/controller/snapshot/snapshot_adapter_test.go +++ b/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" @@ -62,6 +63,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 @@ -281,6 +283,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, @@ -297,6 +338,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() { @@ -1141,6 +1184,41 @@ 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()) + }) + }) }) func getAllIntegrationPipelineRunsForSnapshot(ctx context.Context, snapshot *applicationapiv1alpha1.Snapshot) ([]tektonv1.PipelineRun, error) { diff --git a/internal/controller/snapshot/snapshot_controller.go b/internal/controller/snapshot/snapshot_controller.go index 99d87397f..b8bd8b88e 100644 --- a/internal/controller/snapshot/snapshot_controller.go +++ b/internal/controller/snapshot/snapshot_controller.go @@ -121,6 +121,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu adapter.EnsureGlobalCandidateImageUpdated, adapter.EnsureRerunPipelineRunsExist, adapter.EnsureIntegrationPipelineRunsExist, + 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.