From 149bd2508ca9e8b2323ba870ebb9d333b136794d Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Wed, 10 Jul 2024 12:54:22 +0200 Subject: [PATCH 01/10] extract default finalizer from opts --- internal/declarative/v2/options.go | 10 ---------- internal/declarative/v2/reconciler.go | 11 ++++++----- internal/manifest/custom_resource.go | 10 +++++----- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/internal/declarative/v2/options.go b/internal/declarative/v2/options.go index 59f8fe1fef..a4eed7ef09 100644 --- a/internal/declarative/v2/options.go +++ b/internal/declarative/v2/options.go @@ -20,7 +20,6 @@ import ( ) const ( - FinalizerDefault = "declarative.kyma-project.io/finalizer" FieldOwnerDefault = "declarative.kyma-project.io/applier" EventRecorderDefault = "declarative.kyma-project.io/events" DefaultInMemoryParseTTL = 24 * time.Hour @@ -29,7 +28,6 @@ const ( func DefaultOptions() *Options { return (&Options{}).Apply( WithNamespace(apimetav1.NamespaceDefault, false), - WithFinalizer(FinalizerDefault), WithFieldOwner(FieldOwnerDefault), WithPostRenderTransform( ManagedByDeclarativeV2, @@ -61,8 +59,6 @@ type Options struct { Namespace string CreateNamespace bool - Finalizer string - ServerSideApply bool FieldOwner client.FieldOwner @@ -112,12 +108,6 @@ func (o WithFieldOwner) Apply(options *Options) { options.FieldOwner = client.FieldOwner(o) } -type WithFinalizer string - -func (o WithFinalizer) Apply(options *Options) { - options.Finalizer = string(o) -} - type WithManagerOption struct { manager.Manager } diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index c7cf54ce30..fce0d331dc 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -37,9 +37,10 @@ var ( ) const ( - namespaceNotBeRemoved = "kyma-system" - CustomResourceManager = "resource.kyma-project.io/finalizer" - SyncedOCIRefAnnotation = "sync-oci-ref" + namespaceNotBeRemoved = "kyma-system" + CustomResourceManagerFinalizer = "resource.kyma-project.io/finalizer" + SyncedOCIRefAnnotation = "sync-oci-ref" + defaultFinalizer = "declarative.kyma-project.io/finalizer" ) func NewFromManager(mgr manager.Manager, prototype Object, requeueIntervals queue.RequeueIntervals, @@ -132,7 +133,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if obj.GetDeletionTimestamp().IsZero() { objMeta := r.partialObjectMetadata(obj) - if controllerutil.AddFinalizer(objMeta, r.Finalizer) { + if controllerutil.AddFinalizer(objMeta, defaultFinalizer) { return r.ssaSpec(ctx, objMeta, metrics.ManifestAddFinalizer) } } @@ -227,7 +228,7 @@ func (r *Reconciler) cleanupManifest(ctx context.Context, req ctrl.Request, obj } func (r *Reconciler) finalizerToRemove(originalErr error, obj Object) []string { - finalizersToRemove := []string{r.Finalizer} + finalizersToRemove := []string{defaultFinalizer} if errors.Is(originalErr, ErrAccessSecretNotFound) { finalizersToRemove = obj.GetFinalizers() } diff --git a/internal/manifest/custom_resource.go b/internal/manifest/custom_resource.go index 6f9513b6ce..b97f9f2323 100644 --- a/internal/manifest/custom_resource.go +++ b/internal/manifest/custom_resource.go @@ -31,7 +31,7 @@ func PostRunCreateCR( } resource := manifest.Spec.Resource.DeepCopy() - err := skr.Create(ctx, resource, client.FieldOwner(declarativev2.CustomResourceManager)) + err := skr.Create(ctx, resource, client.FieldOwner(declarativev2.CustomResourceManagerFinalizer)) if err != nil && !apierrors.IsAlreadyExists(err) { return fmt.Errorf("failed to create resource: %w", err) } @@ -41,9 +41,9 @@ func PostRunCreateCR( oMeta.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind()) oMeta.SetNamespace(obj.GetNamespace()) oMeta.SetFinalizers(obj.GetFinalizers()) - if added := controllerutil.AddFinalizer(oMeta, declarativev2.CustomResourceManager); added { + if added := controllerutil.AddFinalizer(oMeta, declarativev2.CustomResourceManagerFinalizer); added { if err := kcp.Patch( - ctx, oMeta, client.Apply, client.ForceOwnership, client.FieldOwner(declarativev2.CustomResourceManager), + ctx, oMeta, client.Apply, client.ForceOwnership, client.FieldOwner(declarativev2.CustomResourceManagerFinalizer), ); err != nil { return fmt.Errorf("failed to patch resource: %w", err) } @@ -83,9 +83,9 @@ func PreDeleteDeleteCR( if err != nil { return fmt.Errorf("failed to fetch resource: %w", err) } - if removed := controllerutil.RemoveFinalizer(onCluster, declarativev2.CustomResourceManager); removed { + if removed := controllerutil.RemoveFinalizer(onCluster, declarativev2.CustomResourceManagerFinalizer); removed { if err := kcp.Update( - ctx, onCluster, client.FieldOwner(declarativev2.CustomResourceManager), + ctx, onCluster, client.FieldOwner(declarativev2.CustomResourceManagerFinalizer), ); err != nil { return fmt.Errorf("failed to update resource: %w", err) } From 2c7ac4eea89f31c620a00bfa0bc57325d7af8c88 Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 08:03:39 +0200 Subject: [PATCH 02/10] extract field owner and namespace --- internal/declarative/v2/options.go | 31 --------------------------- internal/declarative/v2/reconciler.go | 23 ++++++++++---------- 2 files changed, 12 insertions(+), 42 deletions(-) diff --git a/internal/declarative/v2/options.go b/internal/declarative/v2/options.go index a4eed7ef09..bf7bb1de5c 100644 --- a/internal/declarative/v2/options.go +++ b/internal/declarative/v2/options.go @@ -6,7 +6,6 @@ import ( "strconv" "time" - apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" k8slabels "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/rest" @@ -20,15 +19,12 @@ import ( ) const ( - FieldOwnerDefault = "declarative.kyma-project.io/applier" EventRecorderDefault = "declarative.kyma-project.io/events" DefaultInMemoryParseTTL = 24 * time.Hour ) func DefaultOptions() *Options { return (&Options{}).Apply( - WithNamespace(apimetav1.NamespaceDefault, false), - WithFieldOwner(FieldOwnerDefault), WithPostRenderTransform( ManagedByDeclarativeV2, watchedByOwnedBy, @@ -56,11 +52,7 @@ type Options struct { ManifestCache CustomReadyCheck ReadyCheck - Namespace string - CreateNamespace bool - ServerSideApply bool - FieldOwner client.FieldOwner PostRenderTransforms []ObjectTransform @@ -85,29 +77,6 @@ func (o *Options) Apply(options ...Option) *Options { return o } -type WithNamespaceOption struct { - name string - createIfMissing bool -} - -func WithNamespace(name string, createIfMissing bool) WithNamespaceOption { - return WithNamespaceOption{ - name: name, - createIfMissing: createIfMissing, - } -} - -func (o WithNamespaceOption) Apply(options *Options) { - options.Namespace = o.name - options.CreateNamespace = o.createIfMissing -} - -type WithFieldOwner client.FieldOwner - -func (o WithFieldOwner) Apply(options *Options) { - options.FieldOwner = client.FieldOwner(o) -} - type WithManagerOption struct { manager.Manager } diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index fce0d331dc..8c34a6f930 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -37,10 +37,11 @@ var ( ) const ( - namespaceNotBeRemoved = "kyma-system" - CustomResourceManagerFinalizer = "resource.kyma-project.io/finalizer" - SyncedOCIRefAnnotation = "sync-oci-ref" - defaultFinalizer = "declarative.kyma-project.io/finalizer" + namespaceNotBeRemoved = "kyma-system" + CustomResourceManagerFinalizer = "resource.kyma-project.io/finalizer" + SyncedOCIRefAnnotation = "sync-oci-ref" + defaultFinalizer = "declarative.kyma-project.io/finalizer" + defaultFieldOwner client.FieldOwner = "declarative.kyma-project.io/applier" ) func NewFromManager(mgr manager.Manager, prototype Object, requeueIntervals queue.RequeueIntervals, @@ -312,7 +313,7 @@ func (r *Reconciler) renderResources( var err error var target, current ResourceList - converter := NewResourceToInfoConverter(ResourceInfoConverter(clnt), r.Namespace) + converter := NewResourceToInfoConverter(ResourceInfoConverter(clnt), apimetav1.NamespaceDefault) if target, err = r.renderTargetResources(ctx, clnt, converter, obj, spec); err != nil { obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) @@ -339,7 +340,7 @@ func (r *Reconciler) syncResources(ctx context.Context, clnt Client, obj Object, ) error { status := obj.GetStatus() - if err := ConcurrentSSA(clnt, r.FieldOwner).Run(ctx, target); err != nil { + if err := ConcurrentSSA(clnt, defaultFieldOwner).Run(ctx, target); err != nil { obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) return err } @@ -595,12 +596,12 @@ func (r *Reconciler) getTargetClient(ctx context.Context, obj Object) (Client, e r.AddClient(clientsCacheKey, clnt) } - if r.Namespace != apimetav1.NamespaceNone && r.Namespace != apimetav1.NamespaceDefault { + if apimetav1.NamespaceDefault != apimetav1.NamespaceNone && apimetav1.NamespaceDefault != apimetav1.NamespaceDefault { err := clnt.Patch( ctx, &apicorev1.Namespace{ TypeMeta: apimetav1.TypeMeta{APIVersion: "v1", Kind: "Namespace"}, - ObjectMeta: apimetav1.ObjectMeta{Name: r.Namespace}, - }, client.Apply, client.ForceOwnership, r.FieldOwner, + ObjectMeta: apimetav1.ObjectMeta{Name: apimetav1.NamespaceDefault}, + }, client.Apply, client.ForceOwnership, defaultFieldOwner, ) if err != nil { return nil, fmt.Errorf("failed to patch namespace: %w", err) @@ -651,7 +652,7 @@ func (r *Reconciler) finishReconcile(ctx context.Context, obj Object, func (r *Reconciler) patchStatusIfDiffExist(ctx context.Context, obj Object, previousStatus shared.Status) error { if hasStatusDiff(obj.GetStatus(), previousStatus) { resetNonPatchableField(obj) - if err := r.Status().Patch(ctx, obj, client.Apply, client.ForceOwnership, r.FieldOwner); err != nil { + if err := r.Status().Patch(ctx, obj, client.Apply, client.ForceOwnership, defaultFieldOwner); err != nil { return fmt.Errorf("failed to patch status: %w", err) } } @@ -668,7 +669,7 @@ func (r *Reconciler) ssaSpec(ctx context.Context, obj client.Object, ) (ctrl.Result, error) { resetNonPatchableField(obj) r.ManifestMetrics.RecordRequeueReason(requeueReason, queue.IntendedRequeue) - if err := r.Patch(ctx, obj, client.Apply, client.ForceOwnership, r.FieldOwner); err != nil { + if err := r.Patch(ctx, obj, client.Apply, client.ForceOwnership, defaultFieldOwner); err != nil { r.Event(obj, "Warning", "PatchObject", err.Error()) return ctrl.Result{}, fmt.Errorf("failed to patch object: %w", err) } From 1e7010210b3d0b3ba6ddfb53248fd38ef7acce93 Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 08:23:30 +0200 Subject: [PATCH 03/10] extract skip label funx --- internal/declarative/v2/options.go | 33 --------------------------- internal/declarative/v2/reconciler.go | 13 ++++++++++- 2 files changed, 12 insertions(+), 34 deletions(-) diff --git a/internal/declarative/v2/options.go b/internal/declarative/v2/options.go index bf7bb1de5c..fd6e83c81d 100644 --- a/internal/declarative/v2/options.go +++ b/internal/declarative/v2/options.go @@ -3,7 +3,6 @@ package v2 import ( "context" "os" - "strconv" "time" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -11,11 +10,7 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" - - "github.com/kyma-project/lifecycle-manager/api/shared" - "github.com/kyma-project/lifecycle-manager/internal" ) const ( @@ -33,7 +28,6 @@ func DefaultOptions() *Options { ), WithSingletonClientCache(NewMemoryClientCache()), WithManifestCache(os.TempDir()), - WithSkipReconcileOn(SkipReconcileOnDefaultLabelPresentAndTrue), WithManifestParser(NewInMemoryCachedManifestParser(DefaultInMemoryParseTTL)), WithModuleCRDeletionCheck(NewDefaultDeletionCheck()), ) @@ -62,8 +56,6 @@ type Options struct { DeletionCheck ModuleCRDeletionCheck DeletePrerequisites bool - - ShouldSkip SkipReconcile } type Option interface { @@ -236,31 +228,6 @@ func (o WithRemoteTargetClusterOption) Apply(options *Options) { options.TargetCluster = o.ClusterFn } -func WithSkipReconcileOn(skipReconcile SkipReconcile) WithSkipReconcileOnOption { - return WithSkipReconcileOnOption{skipReconcile: skipReconcile} -} - -type SkipReconcile func(context.Context, Object) (skip bool) - -// SkipReconcileOnDefaultLabelPresentAndTrue determines SkipReconcile by checking if DefaultSkipReconcileLabel is true. -func SkipReconcileOnDefaultLabelPresentAndTrue(ctx context.Context, object Object) bool { - if object.GetLabels() != nil && object.GetLabels()[shared.SkipReconcileLabel] == strconv.FormatBool(true) { - logf.FromContext(ctx, "skip-label", shared.SkipReconcileLabel). - V(internal.DebugLogLevel).Info("resource gets skipped because of label") - return true - } - - return false -} - -type WithSkipReconcileOnOption struct { - skipReconcile SkipReconcile -} - -func (o WithSkipReconcileOnOption) Apply(options *Options) { - options.ShouldSkip = o.skipReconcile -} - type ClientCacheKeyFn func(ctx context.Context, obj Object) (string, bool) type WithClientCacheKeyOption struct { diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 8c34a6f930..ad4cd04d76 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "github.com/kyma-project/lifecycle-manager/internal" "strconv" "time" @@ -98,6 +99,16 @@ func newResourcesCondition(obj Object) apimetav1.Condition { } } +func hasSkipReconcileLabel(ctx context.Context, object Object) bool { + if object.GetLabels() != nil && object.GetLabels()[shared.SkipReconcileLabel] == strconv.FormatBool(true) { + logf.FromContext(ctx, "skip-label", shared.SkipReconcileLabel). + V(internal.DebugLogLevel).Info("resource gets skipped because of label") + return true + } + + return false +} + //nolint:funlen,cyclop,gocognit // Declarative pkg will be removed soon func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { startTime := time.Now() @@ -117,7 +128,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } currentObjStatus := obj.GetStatus() - if r.ShouldSkip(ctx, obj) { + if hasSkipReconcileLabel(ctx, obj) { return ctrl.Result{RequeueAfter: r.Success}, nil } From 0b083f9cb403e88491d63c5d7ccfca6f706b2f8c Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 08:46:43 +0200 Subject: [PATCH 04/10] replace obj with manifest for manifest reconcile loop --- internal/declarative/v2/options.go | 2 - internal/declarative/v2/reconciler.go | 291 +++++++++++++------------- 2 files changed, 141 insertions(+), 152 deletions(-) diff --git a/internal/declarative/v2/options.go b/internal/declarative/v2/options.go index fd6e83c81d..57e647ae9c 100644 --- a/internal/declarative/v2/options.go +++ b/internal/declarative/v2/options.go @@ -46,8 +46,6 @@ type Options struct { ManifestCache CustomReadyCheck ReadyCheck - ServerSideApply bool - PostRenderTransforms []ObjectTransform PostRuns []PostRun diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index ad4cd04d76..45b0c24eed 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -79,28 +79,28 @@ const ( ConditionReasonReady ConditionReason = "Ready" ) -func newInstallationCondition(obj Object) apimetav1.Condition { +func newInstallationCondition(manifest *v1beta2.Manifest) apimetav1.Condition { return apimetav1.Condition{ Type: string(ConditionTypeInstallation), Reason: string(ConditionReasonReady), Status: apimetav1.ConditionFalse, Message: "installation is ready and resources can be used", - ObservedGeneration: obj.GetGeneration(), + ObservedGeneration: manifest.GetGeneration(), } } -func newResourcesCondition(obj Object) apimetav1.Condition { +func newResourcesCondition(manifest *v1beta2.Manifest) apimetav1.Condition { return apimetav1.Condition{ Type: string(ConditionTypeResources), Reason: string(ConditionReasonResourcesAreAvailable), Status: apimetav1.ConditionFalse, Message: "resources are parsed and ready for use", - ObservedGeneration: obj.GetGeneration(), + ObservedGeneration: manifest.GetGeneration(), } } -func hasSkipReconcileLabel(ctx context.Context, object Object) bool { - if object.GetLabels() != nil && object.GetLabels()[shared.SkipReconcileLabel] == strconv.FormatBool(true) { +func hasSkipReconcileLabel(ctx context.Context, manifest *v1beta2.Manifest) bool { + if manifest.GetLabels() != nil && manifest.GetLabels()[shared.SkipReconcileLabel] == strconv.FormatBool(true) { logf.FromContext(ctx, "skip-label", shared.SkipReconcileLabel). V(internal.DebugLogLevel).Info("resource gets skipped because of label") return true @@ -113,12 +113,9 @@ func hasSkipReconcileLabel(ctx context.Context, object Object) bool { func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { startTime := time.Now() defer r.recordReconciliationDuration(startTime, req.Name) - obj, ok := r.prototype.DeepCopyObject().(Object) - if !ok { - r.ManifestMetrics.RecordRequeueReason(metrics.ManifestTypeCast, queue.UnexpectedRequeue) - return ctrl.Result{}, common.ErrTypeAssert - } - if err := r.Get(ctx, req.NamespacedName, obj); err != nil { + + manifest := &v1beta2.Manifest{} + if err := r.Get(ctx, req.NamespacedName, manifest); err != nil { if util.IsNotFound(err) { logf.FromContext(ctx).Info(req.NamespacedName.String() + " got deleted!") return ctrl.Result{}, nil @@ -126,130 +123,130 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu r.ManifestMetrics.RecordRequeueReason(metrics.ManifestRetrieval, queue.UnexpectedRequeue) return ctrl.Result{}, fmt.Errorf("manifestController: %w", err) } - currentObjStatus := obj.GetStatus() + manifestStatus := manifest.GetStatus() - if hasSkipReconcileLabel(ctx, obj) { + if hasSkipReconcileLabel(ctx, manifest) { return ctrl.Result{RequeueAfter: r.Success}, nil } - if err := r.initialize(obj); err != nil { - return r.finishReconcile(ctx, obj, metrics.ManifestInit, currentObjStatus, err) + if err := r.initialize(manifest); err != nil { + return r.finishReconcile(ctx, manifest, metrics.ManifestInit, manifestStatus, err) } - if obj.GetLabels() != nil && obj.GetLabels()[shared.IsMandatoryModule] == strconv.FormatBool(true) { - state := obj.GetStatus().State - kymaName := obj.GetLabels()[shared.KymaName] - moduleName := obj.GetLabels()[shared.ModuleName] + if manifest.GetLabels() != nil && manifest.GetLabels()[shared.IsMandatoryModule] == strconv.FormatBool(true) { + state := manifest.GetStatus().State + kymaName := manifest.GetLabels()[shared.KymaName] + moduleName := manifest.GetLabels()[shared.ModuleName] r.MandatoryModuleMetrics.RecordMandatoryModuleState(kymaName, moduleName, state) } - if obj.GetDeletionTimestamp().IsZero() { - objMeta := r.partialObjectMetadata(obj) - if controllerutil.AddFinalizer(objMeta, defaultFinalizer) { - return r.ssaSpec(ctx, objMeta, metrics.ManifestAddFinalizer) + if manifest.GetDeletionTimestamp().IsZero() { + partialMeta := r.partialObjectMetadata(manifest) + if controllerutil.AddFinalizer(partialMeta, defaultFinalizer) { + return r.ssaSpec(ctx, partialMeta, metrics.ManifestAddFinalizer) } } - spec, err := r.Spec(ctx, obj) + spec, err := r.Spec(ctx, manifest) if err != nil { - if !obj.GetDeletionTimestamp().IsZero() { - return r.cleanupManifest(ctx, req, obj, currentObjStatus, metrics.ManifestParseSpec, err) + if !manifest.GetDeletionTimestamp().IsZero() { + return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestParseSpec, err) } - return r.finishReconcile(ctx, obj, metrics.ManifestParseSpec, currentObjStatus, err) + return r.finishReconcile(ctx, manifest, metrics.ManifestParseSpec, manifestStatus, err) } - if notContainsSyncedOCIRefAnnotation(obj) { - updateSyncedOCIRefAnnotation(obj, spec.OCIRef) - return r.updateObject(ctx, obj, metrics.ManifestInitSyncedOCIRef) + if notContainsSyncedOCIRefAnnotation(manifest) { + updateSyncedOCIRefAnnotation(manifest, spec.OCIRef) + return r.updateObject(ctx, manifest, metrics.ManifestInitSyncedOCIRef) } - clnt, err := r.getTargetClient(ctx, obj) + clnt, err := r.getTargetClient(ctx, manifest) if err != nil { - if !obj.GetDeletionTimestamp().IsZero() && errors.Is(err, ErrAccessSecretNotFound) { - return r.cleanupManifest(ctx, req, obj, currentObjStatus, metrics.ManifestClientInit, + if !manifest.GetDeletionTimestamp().IsZero() && errors.Is(err, ErrAccessSecretNotFound) { + return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestClientInit, err) } - obj.SetStatus(obj.GetStatus().WithState(shared.StateError).WithErr(err)) - return r.finishReconcile(ctx, obj, metrics.ManifestClientInit, currentObjStatus, err) + manifest.SetStatus(manifest.GetStatus().WithState(shared.StateError).WithErr(err)) + return r.finishReconcile(ctx, manifest, metrics.ManifestClientInit, manifestStatus, err) } - target, current, err := r.renderResources(ctx, clnt, obj, spec) + target, current, err := r.renderResources(ctx, clnt, manifest, spec) if err != nil { if util.IsConnectionRelatedError(err) { - r.invalidateClientCache(ctx, obj) - return r.finishReconcile(ctx, obj, metrics.ManifestUnauthorized, currentObjStatus, err) + r.invalidateClientCache(ctx, manifest) + return r.finishReconcile(ctx, manifest, metrics.ManifestUnauthorized, manifestStatus, err) } - return r.finishReconcile(ctx, obj, metrics.ManifestRenderResources, currentObjStatus, err) + return r.finishReconcile(ctx, manifest, metrics.ManifestRenderResources, manifestStatus, err) } - if err := r.pruneDiff(ctx, clnt, obj, current, target, spec); errors.Is(err, resources.ErrDeletionNotFinished) { + if err := r.pruneDiff(ctx, clnt, manifest, current, target, spec); errors.Is(err, resources.ErrDeletionNotFinished) { r.ManifestMetrics.RecordRequeueReason(metrics.ManifestPruneDiffNotFinished, queue.IntendedRequeue) return ctrl.Result{Requeue: true}, nil } else if err != nil { - return r.finishReconcile(ctx, obj, metrics.ManifestPruneDiff, currentObjStatus, err) + return r.finishReconcile(ctx, manifest, metrics.ManifestPruneDiff, manifestStatus, err) } - if err := r.removeModuleCR(ctx, clnt, obj); err != nil { + if err := r.removeModuleCR(ctx, clnt, manifest); err != nil { if errors.Is(err, ErrRequeueRequired) { r.ManifestMetrics.RecordRequeueReason(metrics.ManifestPreDeleteEnqueueRequired, queue.IntendedRequeue) return ctrl.Result{Requeue: true}, nil } - return r.finishReconcile(ctx, obj, metrics.ManifestPreDelete, currentObjStatus, err) + return r.finishReconcile(ctx, manifest, metrics.ManifestPreDelete, manifestStatus, err) } - if err = r.syncResources(ctx, clnt, obj, target); err != nil { + if err = r.syncResources(ctx, clnt, manifest, target); err != nil { if errors.Is(err, ErrRequeueRequired) { r.ManifestMetrics.RecordRequeueReason(metrics.ManifestSyncResourcesEnqueueRequired, queue.IntendedRequeue) return ctrl.Result{Requeue: true}, nil } if errors.Is(err, ErrClientUnauthorized) { - r.invalidateClientCache(ctx, obj) + r.invalidateClientCache(ctx, manifest) } - return r.finishReconcile(ctx, obj, metrics.ManifestSyncResources, currentObjStatus, err) + return r.finishReconcile(ctx, manifest, metrics.ManifestSyncResources, manifestStatus, err) } // This situation happens when manifest get new installation layer to update resources, // we need to make sure all updates successfully before we can update synced oci ref - if requireUpdateSyncedOCIRefAnnotation(obj, spec.OCIRef) { - updateSyncedOCIRefAnnotation(obj, spec.OCIRef) - return r.updateObject(ctx, obj, metrics.ManifestUpdateSyncedOCIRef) + if requireUpdateSyncedOCIRefAnnotation(manifest, spec.OCIRef) { + updateSyncedOCIRefAnnotation(manifest, spec.OCIRef) + return r.updateObject(ctx, manifest, metrics.ManifestUpdateSyncedOCIRef) } - if !obj.GetDeletionTimestamp().IsZero() { - return r.cleanupManifest(ctx, req, obj, currentObjStatus, metrics.ManifestReconcileFinished, nil) + if !manifest.GetDeletionTimestamp().IsZero() { + return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestReconcileFinished, nil) } - return r.finishReconcile(ctx, obj, metrics.ManifestReconcileFinished, currentObjStatus, nil) + return r.finishReconcile(ctx, manifest, metrics.ManifestReconcileFinished, manifestStatus, nil) } -func (r *Reconciler) cleanupManifest(ctx context.Context, req ctrl.Request, obj Object, currentObjStatus shared.Status, +func (r *Reconciler) cleanupManifest(ctx context.Context, req ctrl.Request, manifest *v1beta2.Manifest, manifestStatus shared.Status, requeueReason metrics.ManifestRequeueReason, originalErr error, ) (ctrl.Result, error) { r.ManifestMetrics.RemoveManifestDuration(req.Name) - r.cleanUpMandatoryModuleMetrics(obj) - if removeFinalizers(obj, r.finalizerToRemove(originalErr, obj)) { - return r.updateObject(ctx, obj, requeueReason) + r.cleanUpMandatoryModuleMetrics(manifest) + if removeFinalizers(manifest, r.finalizerToRemove(originalErr, manifest)) { + return r.updateObject(ctx, manifest, requeueReason) } - if obj.GetStatus().State != shared.StateWarning { - obj.SetStatus(obj.GetStatus().WithState(shared.StateDeleting). - WithOperation(fmt.Sprintf("waiting as other finalizers are present: %s", obj.GetFinalizers()))) + if manifest.GetStatus().State != shared.StateWarning { + manifest.SetStatus(manifest.GetStatus().WithState(shared.StateDeleting). + WithOperation(fmt.Sprintf("waiting as other finalizers are present: %s", manifest.GetFinalizers()))) } - return r.finishReconcile(ctx, obj, requeueReason, currentObjStatus, originalErr) + return r.finishReconcile(ctx, manifest, requeueReason, manifestStatus, originalErr) } -func (r *Reconciler) finalizerToRemove(originalErr error, obj Object) []string { +func (r *Reconciler) finalizerToRemove(originalErr error, manifest *v1beta2.Manifest) []string { finalizersToRemove := []string{defaultFinalizer} if errors.Is(originalErr, ErrAccessSecretNotFound) { - finalizersToRemove = obj.GetFinalizers() + finalizersToRemove = manifest.GetFinalizers() } return finalizersToRemove } -func (r *Reconciler) invalidateClientCache(ctx context.Context, obj Object) { +func (r *Reconciler) invalidateClientCache(ctx context.Context, manifest *v1beta2.Manifest) { if r.ClientCacheKeyFn != nil { - clientsCacheKey, ok := r.ClientCacheKeyFn(ctx, obj) + clientsCacheKey, ok := r.ClientCacheKeyFn(ctx, manifest) if ok { logf.FromContext(ctx).Info("Invalidating manifest-controller client cache entry for key: " + fmt.Sprintf("%#v", clientsCacheKey)) @@ -258,10 +255,10 @@ func (r *Reconciler) invalidateClientCache(ctx context.Context, obj Object) { } } -func removeFinalizers(obj Object, finalizersToRemove []string) bool { +func removeFinalizers(manifest *v1beta2.Manifest, finalizersToRemove []string) bool { finalizerRemoved := false for _, f := range finalizersToRemove { - if controllerutil.RemoveFinalizer(obj, f) { + if controllerutil.RemoveFinalizer(manifest, f) { finalizerRemoved = true } } @@ -269,21 +266,21 @@ func removeFinalizers(obj Object, finalizersToRemove []string) bool { return finalizerRemoved } -func (r *Reconciler) partialObjectMetadata(obj Object) *apimetav1.PartialObjectMetadata { +func (r *Reconciler) partialObjectMetadata(manifest *v1beta2.Manifest) *apimetav1.PartialObjectMetadata { objMeta := &apimetav1.PartialObjectMetadata{} - objMeta.SetName(obj.GetName()) - objMeta.SetNamespace(obj.GetNamespace()) - objMeta.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind()) - objMeta.SetFinalizers(obj.GetFinalizers()) + objMeta.SetName(manifest.GetName()) + objMeta.SetNamespace(manifest.GetNamespace()) + objMeta.SetGroupVersionKind(manifest.GetObjectKind().GroupVersionKind()) + objMeta.SetFinalizers(manifest.GetFinalizers()) return objMeta } -func (r *Reconciler) initialize(obj Object) error { - status := obj.GetStatus() +func (r *Reconciler) initialize(manifest *v1beta2.Manifest) error { + status := manifest.GetStatus() for _, condition := range []apimetav1.Condition{ - newResourcesCondition(obj), - newInstallationCondition(obj), + newResourcesCondition(manifest), + newInstallationCondition(manifest), } { if meta.FindStatusCondition(status.Conditions, condition.Type) == nil { meta.SetStatusCondition(&status.Conditions, condition) @@ -295,19 +292,19 @@ func (r *Reconciler) initialize(obj Object) error { } if status.State == "" { - obj.SetStatus(status.WithState(shared.StateProcessing).WithErr(ErrObjectHasEmptyState)) + manifest.SetStatus(status.WithState(shared.StateProcessing).WithErr(ErrObjectHasEmptyState)) return ErrObjectHasEmptyState } - obj.SetStatus(status) + manifest.SetStatus(status) return nil } -func (r *Reconciler) Spec(ctx context.Context, obj Object) (*Spec, error) { - spec, err := r.SpecResolver.Spec(ctx, obj) +func (r *Reconciler) Spec(ctx context.Context, manifest *v1beta2.Manifest) (*Spec, error) { + spec, err := r.SpecResolver.Spec(ctx, manifest) if err != nil { - obj.SetStatus(obj.GetStatus().WithState(shared.StateError).WithErr(err)) + manifest.SetStatus(manifest.GetStatus().WithState(shared.StateError).WithErr(err)) } return spec, err } @@ -315,44 +312,44 @@ func (r *Reconciler) Spec(ctx context.Context, obj Object) (*Spec, error) { func (r *Reconciler) renderResources( ctx context.Context, clnt Client, - obj Object, + manifest *v1beta2.Manifest, spec *Spec, ) ([]*resource.Info, []*resource.Info, error) { - resourceCondition := newResourcesCondition(obj) - status := obj.GetStatus() + resourceCondition := newResourcesCondition(manifest) + status := manifest.GetStatus() var err error var target, current ResourceList converter := NewResourceToInfoConverter(ResourceInfoConverter(clnt), apimetav1.NamespaceDefault) - if target, err = r.renderTargetResources(ctx, clnt, converter, obj, spec); err != nil { - obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) + if target, err = r.renderTargetResources(ctx, clnt, converter, manifest, spec); err != nil { + manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return nil, nil, err } current, err = converter.ResourcesToInfos(status.Synced) if err != nil { - obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) + manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return nil, nil, err } if !meta.IsStatusConditionTrue(status.Conditions, resourceCondition.Type) { resourceCondition.Status = apimetav1.ConditionTrue meta.SetStatusCondition(&status.Conditions, resourceCondition) - obj.SetStatus(status.WithOperation(resourceCondition.Message)) + manifest.SetStatus(status.WithOperation(resourceCondition.Message)) } return target, current, nil } -func (r *Reconciler) syncResources(ctx context.Context, clnt Client, obj Object, +func (r *Reconciler) syncResources(ctx context.Context, clnt Client, manifest *v1beta2.Manifest, target []*resource.Info, ) error { - status := obj.GetStatus() + status := manifest.GetStatus() if err := ConcurrentSSA(clnt, defaultFieldOwner).Run(ctx, target); err != nil { - obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) + manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return err } @@ -361,27 +358,27 @@ func (r *Reconciler) syncResources(ctx context.Context, clnt Client, obj Object, status.Synced = newSynced if hasDiff(oldSynced, newSynced) { - if obj.GetDeletionTimestamp().IsZero() { - obj.SetStatus(status.WithState(shared.StateProcessing).WithOperation(ErrWarningResourceSyncStateDiff.Error())) + if manifest.GetDeletionTimestamp().IsZero() { + manifest.SetStatus(status.WithState(shared.StateProcessing).WithOperation(ErrWarningResourceSyncStateDiff.Error())) } else if status.State != shared.StateWarning { - obj.SetStatus(status.WithState(shared.StateDeleting).WithOperation(ErrWarningResourceSyncStateDiff.Error())) + manifest.SetStatus(status.WithState(shared.StateDeleting).WithOperation(ErrWarningResourceSyncStateDiff.Error())) } return ErrWarningResourceSyncStateDiff } for i := range r.PostRuns { - if err := r.PostRuns[i](ctx, clnt, r.Client, obj); err != nil { - obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) + if err := r.PostRuns[i](ctx, clnt, r.Client, manifest); err != nil { + manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return err } } deploymentState, err := r.checkDeploymentState(ctx, clnt, target) if err != nil { - obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) + manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return err } - return r.setManifestState(obj, deploymentState) + return r.setManifestState(manifest, deploymentState) } func hasDiff(oldResources []shared.Resource, newResources []shared.Resource) bool { @@ -422,7 +419,7 @@ func (r *Reconciler) checkDeploymentState( return deploymentState, nil } -func (r *Reconciler) setManifestState(manifest Object, state shared.State) error { +func (r *Reconciler) setManifestState(manifest *v1beta2.Manifest, state shared.State) error { status := manifest.GetStatus() if state == shared.StateProcessing { @@ -448,12 +445,12 @@ func (r *Reconciler) setManifestState(manifest Object, state shared.State) error return nil } -func (r *Reconciler) removeModuleCR(ctx context.Context, clnt Client, obj Object) error { - if !obj.GetDeletionTimestamp().IsZero() { +func (r *Reconciler) removeModuleCR(ctx context.Context, clnt Client, manifest *v1beta2.Manifest) error { + if !manifest.GetDeletionTimestamp().IsZero() { for _, preDelete := range r.PreDeletes { - if err := preDelete(ctx, clnt, r.Client, obj); err != nil { + if err := preDelete(ctx, clnt, r.Client, manifest); err != nil { // we do not set a status here since it will be deleting if timestamp is set. - obj.SetStatus(obj.GetStatus().WithErr(err)) + manifest.SetStatus(manifest.GetStatus().WithErr(err)) return err } } @@ -465,11 +462,11 @@ func (r *Reconciler) renderTargetResources( ctx context.Context, clnt client.Client, converter ResourceToInfoConverter, - obj Object, + manifest *v1beta2.Manifest, spec *Spec, ) ([]*resource.Info, error) { - if !obj.GetDeletionTimestamp().IsZero() { - deleted, err := r.DeletionCheck.Run(ctx, clnt, obj) + if !manifest.GetDeletionTimestamp().IsZero() { + deleted, err := r.DeletionCheck.Run(ctx, clnt, manifest) if err != nil { return nil, err } @@ -478,24 +475,24 @@ func (r *Reconciler) renderTargetResources( } } - status := obj.GetStatus() + status := manifest.GetStatus() targetResources, err := r.ManifestParser.Parse(spec) if err != nil { - obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) + manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return nil, err } for _, transform := range r.PostRenderTransforms { - if err := transform(ctx, obj, targetResources.Items); err != nil { - obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) + if err := transform(ctx, manifest, targetResources.Items); err != nil { + manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return nil, err } } target, err := converter.UnstructuredToInfos(targetResources.Items) if err != nil { - obj.SetStatus(status.WithState(shared.StateError).WithErr(err)) + manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return nil, err } @@ -505,72 +502,66 @@ func (r *Reconciler) renderTargetResources( func (r *Reconciler) pruneDiff( ctx context.Context, clnt Client, - obj Object, + manifest *v1beta2.Manifest, current, target []*resource.Info, spec *Spec, ) error { diff, err := pruneResource(ResourceList(current).Difference(target), "Namespace", namespaceNotBeRemoved) if err != nil { - obj.SetStatus(obj.GetStatus().WithErr(err)) + manifest.SetStatus(manifest.GetStatus().WithErr(err)) return err } if len(diff) == 0 { return nil } - if manifestNotInDeletingAndOciRefNotChangedButDiffDetected(diff, obj, spec) { + if manifestNotInDeletingAndOciRefNotChangedButDiffDetected(diff, manifest, spec) { // This case should not happen normally, but if happens, it means the resources read from cache is incomplete, // and we should prevent diff resources to be deleted. // Meanwhile, evict cache to hope newly created resources back to normal. - obj.SetStatus(obj.GetStatus().WithState(shared.StateWarning).WithOperation(ErrResourceSyncDiffInSameOCILayer.Error())) + manifest.SetStatus(manifest.GetStatus().WithState(shared.StateWarning).WithOperation(ErrResourceSyncDiffInSameOCILayer.Error())) r.ManifestParser.EvictCache(spec) return ErrResourceSyncDiffInSameOCILayer } - // Remove this type casting while in progress this issue: https://github.com/kyma-project/lifecycle-manager/issues/1006 - manifest, ok := obj.(*v1beta2.Manifest) - if !ok { - obj.SetStatus(obj.GetStatus().WithErr(v1beta2.ErrTypeAssertManifest)) - return v1beta2.ErrTypeAssertManifest - } err = resources.NewConcurrentCleanup(clnt, manifest).DeleteDiffResources(ctx, diff) if err != nil { - obj.SetStatus(obj.GetStatus().WithErr(err)) + manifest.SetStatus(manifest.GetStatus().WithErr(err)) } return err } -func manifestNotInDeletingAndOciRefNotChangedButDiffDetected(diff []*resource.Info, obj Object, +func manifestNotInDeletingAndOciRefNotChangedButDiffDetected(diff []*resource.Info, manifest *v1beta2.Manifest, spec *Spec, ) bool { - return len(diff) > 0 && ociRefNotChanged(obj, spec.OCIRef) && obj.GetDeletionTimestamp().IsZero() + return len(diff) > 0 && ociRefNotChanged(manifest, spec.OCIRef) && manifest.GetDeletionTimestamp().IsZero() } -func ociRefNotChanged(obj Object, ref string) bool { - syncedOCIRef, found := obj.GetAnnotations()[SyncedOCIRefAnnotation] +func ociRefNotChanged(manifest *v1beta2.Manifest, ref string) bool { + syncedOCIRef, found := manifest.GetAnnotations()[SyncedOCIRefAnnotation] return found && syncedOCIRef == ref } -func requireUpdateSyncedOCIRefAnnotation(obj Object, ref string) bool { - syncedOCIRef, found := obj.GetAnnotations()[SyncedOCIRefAnnotation] +func requireUpdateSyncedOCIRefAnnotation(manifest *v1beta2.Manifest, ref string) bool { + syncedOCIRef, found := manifest.GetAnnotations()[SyncedOCIRefAnnotation] if found && syncedOCIRef != ref { return true } return false } -func notContainsSyncedOCIRefAnnotation(obj Object) bool { - _, found := obj.GetAnnotations()[SyncedOCIRefAnnotation] +func notContainsSyncedOCIRefAnnotation(manifest *v1beta2.Manifest) bool { + _, found := manifest.GetAnnotations()[SyncedOCIRefAnnotation] return !found } -func updateSyncedOCIRefAnnotation(obj Object, ref string) { - annotations := obj.GetAnnotations() +func updateSyncedOCIRefAnnotation(manifest *v1beta2.Manifest, ref string) { + annotations := manifest.GetAnnotations() if annotations == nil { annotations = make(map[string]string) } annotations[SyncedOCIRefAnnotation] = ref - obj.SetAnnotations(annotations) + manifest.SetAnnotations(annotations) } func pruneResource(diff []*resource.Info, resourceType string, resourceName string) ([]*resource.Info, error) { @@ -587,20 +578,20 @@ func pruneResource(diff []*resource.Info, resourceType string, resourceName stri return diff, nil } -func (r *Reconciler) getTargetClient(ctx context.Context, obj Object) (Client, error) { +func (r *Reconciler) getTargetClient(ctx context.Context, manifest *v1beta2.Manifest) (Client, error) { var err error var clnt Client if r.ClientCacheKeyFn == nil { - return r.configClient(ctx, obj) + return r.configClient(ctx, manifest) } - clientsCacheKey, found := r.ClientCacheKeyFn(ctx, obj) + clientsCacheKey, found := r.ClientCacheKeyFn(ctx, manifest) if found { clnt = r.GetClient(clientsCacheKey) } if clnt == nil { - clnt, err = r.configClient(ctx, obj) + clnt, err = r.configClient(ctx, manifest) if err != nil { return nil, err } @@ -622,7 +613,7 @@ func (r *Reconciler) getTargetClient(ctx context.Context, obj Object) (Client, e return clnt, nil } -func (r *Reconciler) configClient(ctx context.Context, obj Object) (Client, error) { +func (r *Reconciler) configClient(ctx context.Context, manifest *v1beta2.Manifest) (Client, error) { var err error cluster := &ClusterInfo{ @@ -631,7 +622,7 @@ func (r *Reconciler) configClient(ctx context.Context, obj Object) (Client, erro } if r.TargetCluster != nil { - cluster, err = r.TargetCluster(ctx, obj) + cluster, err = r.TargetCluster(ctx, manifest) if err != nil { return nil, err } @@ -645,11 +636,11 @@ func (r *Reconciler) configClient(ctx context.Context, obj Object) (Client, erro return clnt, nil } -func (r *Reconciler) finishReconcile(ctx context.Context, obj Object, +func (r *Reconciler) finishReconcile(ctx context.Context, manifest *v1beta2.Manifest, requeueReason metrics.ManifestRequeueReason, previousStatus shared.Status, originalErr error, ) (ctrl.Result, error) { - if err := r.patchStatusIfDiffExist(ctx, obj, previousStatus); err != nil { - r.Event(obj, "Warning", "PatchStatus", err.Error()) + if err := r.patchStatusIfDiffExist(ctx, manifest, previousStatus); err != nil { + r.Event(manifest, "Warning", "PatchStatus", err.Error()) return ctrl.Result{}, fmt.Errorf("failed to patch status: %w", err) } if originalErr != nil { @@ -660,10 +651,10 @@ func (r *Reconciler) finishReconcile(ctx context.Context, obj Object, return ctrl.Result{RequeueAfter: r.Success}, nil } -func (r *Reconciler) patchStatusIfDiffExist(ctx context.Context, obj Object, previousStatus shared.Status) error { - if hasStatusDiff(obj.GetStatus(), previousStatus) { - resetNonPatchableField(obj) - if err := r.Status().Patch(ctx, obj, client.Apply, client.ForceOwnership, defaultFieldOwner); err != nil { +func (r *Reconciler) patchStatusIfDiffExist(ctx context.Context, manifest *v1beta2.Manifest, previousStatus shared.Status) error { + if hasStatusDiff(manifest.GetStatus(), previousStatus) { + resetNonPatchableField(manifest) + if err := r.Status().Patch(ctx, manifest, client.Apply, client.ForceOwnership, defaultFieldOwner); err != nil { return fmt.Errorf("failed to patch status: %w", err) } } @@ -713,10 +704,10 @@ func (r *Reconciler) recordReconciliationDuration(startTime time.Time, name stri } } -func (r *Reconciler) cleanUpMandatoryModuleMetrics(obj Object) { - if obj.GetLabels()[shared.IsMandatoryModule] == strconv.FormatBool(true) { - kymaName := obj.GetLabels()[shared.KymaName] - moduleName := obj.GetLabels()[shared.ModuleName] +func (r *Reconciler) cleanUpMandatoryModuleMetrics(manifest *v1beta2.Manifest) { + if manifest.GetLabels()[shared.IsMandatoryModule] == strconv.FormatBool(true) { + kymaName := manifest.GetLabels()[shared.KymaName] + moduleName := manifest.GetLabels()[shared.ModuleName] r.MandatoryModuleMetrics.CleanupMetrics(kymaName, moduleName) } } From f726166b3d41bec69e26f4bb3d65f0ff9352565b Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 09:14:15 +0200 Subject: [PATCH 05/10] add skip reconcile check as method. internalize cr deletion check --- api/v1beta2/manifest_types.go | 5 ++ internal/controller/manifest/controller.go | 4 +- .../declarative/v2/moduleCR_deletion_check.go | 22 ------- internal/declarative/v2/options.go | 17 ----- internal/declarative/v2/reconciler.go | 66 ++++++++++++------- internal/manifest/cr_deletion_check.go | 55 ---------------- .../custom_resource_check/suite_test.go | 2 +- .../controller/manifest/suite_test.go | 2 +- 8 files changed, 50 insertions(+), 123 deletions(-) delete mode 100644 internal/declarative/v2/moduleCR_deletion_check.go delete mode 100644 internal/manifest/cr_deletion_check.go diff --git a/api/v1beta2/manifest_types.go b/api/v1beta2/manifest_types.go index ac52970a71..e3c902971a 100644 --- a/api/v1beta2/manifest_types.go +++ b/api/v1beta2/manifest_types.go @@ -20,6 +20,7 @@ import ( apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" machineryruntime "k8s.io/apimachinery/pkg/runtime" + "strconv" "github.com/kyma-project/lifecycle-manager/api/shared" ) @@ -130,3 +131,7 @@ type ManifestList struct { func init() { SchemeBuilder.Register(&Manifest{}, &ManifestList{}) } + +func (manifest *Manifest) SkipReconciliation() bool { + return manifest.GetLabels() != nil && manifest.GetLabels()[shared.SkipReconcileLabel] == strconv.FormatBool(true) +} diff --git a/internal/controller/manifest/controller.go b/internal/controller/manifest/controller.go index be6af3b9be..1b4993baf5 100644 --- a/internal/controller/manifest/controller.go +++ b/internal/controller/manifest/controller.go @@ -3,7 +3,6 @@ package manifest import ( "sigs.k8s.io/controller-runtime/pkg/manager" - "github.com/kyma-project/lifecycle-manager/api/v1beta2" declarativev2 "github.com/kyma-project/lifecycle-manager/internal/declarative/v2" "github.com/kyma-project/lifecycle-manager/internal/manifest" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" @@ -22,7 +21,7 @@ func NewReconciler(mgr manager.Manager, extractor := manifest.NewPathExtractor(nil) lookup := &manifest.RemoteClusterLookup{KCP: kcp} return declarativev2.NewFromManager( - mgr, &v1beta2.Manifest{}, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, + mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, declarativev2.WithSpecResolver( manifest.NewSpecResolver(kcp, extractor), ), @@ -31,6 +30,5 @@ func NewReconciler(mgr manager.Manager, manifest.WithClientCacheKey(), declarativev2.WithPostRun{manifest.PostRunCreateCR}, declarativev2.WithPreDelete{manifest.PreDeleteDeleteCR}, - declarativev2.WithModuleCRDeletionCheck(manifest.NewModuleCRDeletionCheck()), ) } diff --git a/internal/declarative/v2/moduleCR_deletion_check.go b/internal/declarative/v2/moduleCR_deletion_check.go deleted file mode 100644 index e5fdc63d96..0000000000 --- a/internal/declarative/v2/moduleCR_deletion_check.go +++ /dev/null @@ -1,22 +0,0 @@ -package v2 - -import ( - "context" - - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type ModuleCRDeletionCheck interface { - Run(ctx context.Context, clnt client.Client, obj Object) (bool, error) -} - -// NewDefaultDeletionCheck creates a check that verifies that the Resource CR in the remote cluster is deleted. -func NewDefaultDeletionCheck() *DefaultDeletionCheck { - return &DefaultDeletionCheck{} -} - -type DefaultDeletionCheck struct{} - -func (c *DefaultDeletionCheck) Run(ctx context.Context, clnt client.Client, obj Object) (bool, error) { - return true, nil -} diff --git a/internal/declarative/v2/options.go b/internal/declarative/v2/options.go index 57e647ae9c..2553d60dc1 100644 --- a/internal/declarative/v2/options.go +++ b/internal/declarative/v2/options.go @@ -29,7 +29,6 @@ func DefaultOptions() *Options { WithSingletonClientCache(NewMemoryClientCache()), WithManifestCache(os.TempDir()), WithManifestParser(NewInMemoryCachedManifestParser(DefaultInMemoryParseTTL)), - WithModuleCRDeletionCheck(NewDefaultDeletionCheck()), ) } @@ -50,10 +49,6 @@ type Options struct { PostRuns []PostRun PreDeletes []PreDelete - - DeletionCheck ModuleCRDeletionCheck - - DeletePrerequisites bool } type Option interface { @@ -156,18 +151,6 @@ func (o WithPreDelete) Apply(options *Options) { options.PreDeletes = append(options.PreDeletes, o...) } -func WithModuleCRDeletionCheck(deletionCheckFn ModuleCRDeletionCheck) WithModuleCRDeletionCheckOption { - return WithModuleCRDeletionCheckOption{ModuleCRDeletionCheck: deletionCheckFn} -} - -type WithModuleCRDeletionCheckOption struct { - ModuleCRDeletionCheck -} - -func (o WithModuleCRDeletionCheckOption) Apply(options *Options) { - options.DeletionCheck = o -} - type WithSingletonClientCacheOption struct { ClientCache } diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 45b0c24eed..4ac2bc06c4 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "github.com/kyma-project/lifecycle-manager/internal" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "strconv" "time" @@ -45,11 +47,10 @@ const ( defaultFieldOwner client.FieldOwner = "declarative.kyma-project.io/applier" ) -func NewFromManager(mgr manager.Manager, prototype Object, requeueIntervals queue.RequeueIntervals, +func NewFromManager(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, metrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, options ...Option, ) *Reconciler { reconciler := &Reconciler{} - reconciler.prototype = prototype reconciler.ManifestMetrics = metrics reconciler.MandatoryModuleMetrics = mandatoryModulesMetrics reconciler.RequeueIntervals = requeueIntervals @@ -58,7 +59,6 @@ func NewFromManager(mgr manager.Manager, prototype Object, requeueIntervals queu } type Reconciler struct { - prototype Object queue.RequeueIntervals *Options ManifestMetrics *metrics.ManifestMetrics @@ -99,16 +99,6 @@ func newResourcesCondition(manifest *v1beta2.Manifest) apimetav1.Condition { } } -func hasSkipReconcileLabel(ctx context.Context, manifest *v1beta2.Manifest) bool { - if manifest.GetLabels() != nil && manifest.GetLabels()[shared.SkipReconcileLabel] == strconv.FormatBool(true) { - logf.FromContext(ctx, "skip-label", shared.SkipReconcileLabel). - V(internal.DebugLogLevel).Info("resource gets skipped because of label") - return true - } - - return false -} - //nolint:funlen,cyclop,gocognit // Declarative pkg will be removed soon func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { startTime := time.Now() @@ -125,7 +115,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } manifestStatus := manifest.GetStatus() - if hasSkipReconcileLabel(ctx, manifest) { + if manifest.SkipReconciliation() { + logf.FromContext(ctx, "skip-label", shared.SkipReconcileLabel). + V(internal.DebugLogLevel).Info("resource gets skipped because of label") return ctrl.Result{RequeueAfter: r.Success}, nil } @@ -160,7 +152,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.updateObject(ctx, manifest, metrics.ManifestInitSyncedOCIRef) } - clnt, err := r.getTargetClient(ctx, manifest) + skrClient, err := r.getTargetClient(ctx, manifest) if err != nil { if !manifest.GetDeletionTimestamp().IsZero() && errors.Is(err, ErrAccessSecretNotFound) { return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestClientInit, @@ -171,7 +163,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.finishReconcile(ctx, manifest, metrics.ManifestClientInit, manifestStatus, err) } - target, current, err := r.renderResources(ctx, clnt, manifest, spec) + target, current, err := r.renderResources(ctx, skrClient, manifest, spec) if err != nil { if util.IsConnectionRelatedError(err) { r.invalidateClientCache(ctx, manifest) @@ -181,14 +173,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.finishReconcile(ctx, manifest, metrics.ManifestRenderResources, manifestStatus, err) } - if err := r.pruneDiff(ctx, clnt, manifest, current, target, spec); errors.Is(err, resources.ErrDeletionNotFinished) { + if err := r.pruneDiff(ctx, skrClient, manifest, current, target, spec); errors.Is(err, resources.ErrDeletionNotFinished) { r.ManifestMetrics.RecordRequeueReason(metrics.ManifestPruneDiffNotFinished, queue.IntendedRequeue) return ctrl.Result{Requeue: true}, nil } else if err != nil { return r.finishReconcile(ctx, manifest, metrics.ManifestPruneDiff, manifestStatus, err) } - if err := r.removeModuleCR(ctx, clnt, manifest); err != nil { + if err := r.removeModuleCR(ctx, skrClient, manifest); err != nil { if errors.Is(err, ErrRequeueRequired) { r.ManifestMetrics.RecordRequeueReason(metrics.ManifestPreDeleteEnqueueRequired, queue.IntendedRequeue) return ctrl.Result{Requeue: true}, nil @@ -196,7 +188,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.finishReconcile(ctx, manifest, metrics.ManifestPreDelete, manifestStatus, err) } - if err = r.syncResources(ctx, clnt, manifest, target); err != nil { + if err = r.syncResources(ctx, skrClient, manifest, target); err != nil { if errors.Is(err, ErrRequeueRequired) { r.ManifestMetrics.RecordRequeueReason(metrics.ManifestSyncResourcesEnqueueRequired, queue.IntendedRequeue) return ctrl.Result{Requeue: true}, nil @@ -311,7 +303,7 @@ func (r *Reconciler) Spec(ctx context.Context, manifest *v1beta2.Manifest) (*Spe func (r *Reconciler) renderResources( ctx context.Context, - clnt Client, + skrClient Client, manifest *v1beta2.Manifest, spec *Spec, ) ([]*resource.Info, []*resource.Info, error) { @@ -321,9 +313,9 @@ func (r *Reconciler) renderResources( var err error var target, current ResourceList - converter := NewResourceToInfoConverter(ResourceInfoConverter(clnt), apimetav1.NamespaceDefault) + converter := NewResourceToInfoConverter(ResourceInfoConverter(skrClient), apimetav1.NamespaceDefault) - if target, err = r.renderTargetResources(ctx, clnt, converter, manifest, spec); err != nil { + if target, err = r.renderTargetResources(ctx, skrClient, converter, manifest, spec); err != nil { manifest.SetStatus(status.WithState(shared.StateError).WithErr(err)) return nil, nil, err } @@ -460,13 +452,13 @@ func (r *Reconciler) removeModuleCR(ctx context.Context, clnt Client, manifest * func (r *Reconciler) renderTargetResources( ctx context.Context, - clnt client.Client, + skrClient client.Client, converter ResourceToInfoConverter, manifest *v1beta2.Manifest, spec *Spec, ) ([]*resource.Info, error) { if !manifest.GetDeletionTimestamp().IsZero() { - deleted, err := r.DeletionCheck.Run(ctx, clnt, manifest) + deleted, err := checkCRDeletion(ctx, skrClient, manifest) if err != nil { return nil, err } @@ -499,6 +491,32 @@ func (r *Reconciler) renderTargetResources( return target, nil } +func checkCRDeletion(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (bool, error) { + if manifest.Spec.Resource == nil { + return true, nil + } + + name := manifest.Spec.Resource.GetName() + namespace := manifest.Spec.Resource.GetNamespace() + gvk := manifest.Spec.Resource.GroupVersionKind() + + resourceCR := &unstructured.Unstructured{} + resourceCR.SetGroupVersionKind(schema.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + }) + + if err := skrClient.Get(ctx, + client.ObjectKey{Name: name, Namespace: namespace}, resourceCR); err != nil { + if util.IsNotFound(err) { + return true, nil + } + return false, fmt.Errorf("%w: failed to fetch default resource CR", err) + } + return false, nil +} + func (r *Reconciler) pruneDiff( ctx context.Context, clnt Client, diff --git a/internal/manifest/cr_deletion_check.go b/internal/manifest/cr_deletion_check.go deleted file mode 100644 index d694dcd83a..0000000000 --- a/internal/manifest/cr_deletion_check.go +++ /dev/null @@ -1,55 +0,0 @@ -package manifest - -import ( - "context" - "fmt" - - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" - "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/kyma-project/lifecycle-manager/api/v1beta2" - declarativev2 "github.com/kyma-project/lifecycle-manager/internal/declarative/v2" - "github.com/kyma-project/lifecycle-manager/pkg/util" -) - -// NewModuleCRDeletionCheck creates a check that verifies that the Resource CR in the remote cluster is deleted. -func NewModuleCRDeletionCheck() *ModuleCRDeletionCheck { - return &ModuleCRDeletionCheck{} -} - -type ModuleCRDeletionCheck struct{} - -func (c *ModuleCRDeletionCheck) Run( - ctx context.Context, - clnt client.Client, - obj declarativev2.Object, -) (bool, error) { - manifest, ok := obj.(*v1beta2.Manifest) - if !ok { - return false, v1beta2.ErrTypeAssertManifest - } - if manifest.Spec.Resource == nil { - return true, nil - } - - name := manifest.Spec.Resource.GetName() - namespace := manifest.Spec.Resource.GetNamespace() - gvk := manifest.Spec.Resource.GroupVersionKind() - - resourceCR := &unstructured.Unstructured{} - resourceCR.SetGroupVersionKind(schema.GroupVersionKind{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - }) - - if err := clnt.Get(ctx, - client.ObjectKey{Name: name, Namespace: namespace}, resourceCR); err != nil { - if util.IsNotFound(err) { - return true, nil - } - return false, fmt.Errorf("%w: failed to fetch default resource CR", err) - } - return false, nil -} diff --git a/tests/integration/controller/manifest/custom_resource_check/suite_test.go b/tests/integration/controller/manifest/custom_resource_check/suite_test.go index 2d4ee5533f..7e7e4eac07 100644 --- a/tests/integration/controller/manifest/custom_resource_check/suite_test.go +++ b/tests/integration/controller/manifest/custom_resource_check/suite_test.go @@ -136,7 +136,7 @@ var _ = BeforeSuite(func() { kcp := &declarativev2.ClusterInfo{Config: cfg, Client: kcpClient} extractor := manifest.NewPathExtractor(nil) - reconciler = declarativev2.NewFromManager(mgr, &v1beta2.Manifest{}, queue.RequeueIntervals{ + reconciler = declarativev2.NewFromManager(mgr, queue.RequeueIntervals{ Success: 1 * time.Second, Error: 1 * time.Second, }, diff --git a/tests/integration/controller/manifest/suite_test.go b/tests/integration/controller/manifest/suite_test.go index 4990d4b878..a813d1694e 100644 --- a/tests/integration/controller/manifest/suite_test.go +++ b/tests/integration/controller/manifest/suite_test.go @@ -133,7 +133,7 @@ var _ = BeforeSuite(func() { kcp := &declarativev2.ClusterInfo{Config: cfg, Client: kcpClient} extractor := manifest.NewPathExtractor(nil) - reconciler = declarativev2.NewFromManager(mgr, &v1beta2.Manifest{}, queue.RequeueIntervals{ + reconciler = declarativev2.NewFromManager(mgr, queue.RequeueIntervals{ Success: 1 * time.Second, Busy: 1 * time.Second, }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), From 13d15ba4692fce49dc2d7d4804809ec34b12b248 Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 10:58:50 +0200 Subject: [PATCH 06/10] simplify SpecResolver --- internal/controller/manifest/controller.go | 4 +- internal/declarative/v2/inmemory_rendered.go | 3 +- internal/declarative/v2/options.go | 13 ---- internal/declarative/v2/reconciler.go | 21 +++-- internal/declarative/v2/spec.go | 36 +-------- internal/manifest/spec_resolver.go | 82 +++++--------------- 6 files changed, 32 insertions(+), 127 deletions(-) diff --git a/internal/controller/manifest/controller.go b/internal/controller/manifest/controller.go index 1b4993baf5..3fe47c12ae 100644 --- a/internal/controller/manifest/controller.go +++ b/internal/controller/manifest/controller.go @@ -22,9 +22,7 @@ func NewReconciler(mgr manager.Manager, lookup := &manifest.RemoteClusterLookup{KCP: kcp} return declarativev2.NewFromManager( mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, - declarativev2.WithSpecResolver( - manifest.NewSpecResolver(kcp, extractor), - ), + manifest.NewSpecResolver(kcp.Client, extractor), declarativev2.WithCustomReadyCheck(manifest.NewDeploymentReadyCheck()), declarativev2.WithRemoteTargetCluster(lookup.ConfigResolver), manifest.WithClientCacheKey(), diff --git a/internal/declarative/v2/inmemory_rendered.go b/internal/declarative/v2/inmemory_rendered.go index 07fe19708a..e2249be8f8 100644 --- a/internal/declarative/v2/inmemory_rendered.go +++ b/internal/declarative/v2/inmemory_rendered.go @@ -62,6 +62,5 @@ func (c *InMemoryManifestCache) Parse(spec *Spec, } func generateCacheKey(spec *Spec) string { - file := filepath.Join(ManifestFilePrefix, spec.Path, spec.ManifestName) - return fmt.Sprintf("%s-%s", file, spec.Mode) + return filepath.Join(ManifestFilePrefix, spec.Path, spec.ManifestName) + "-raw" } diff --git a/internal/declarative/v2/options.go b/internal/declarative/v2/options.go index 2553d60dc1..ef3a160b8d 100644 --- a/internal/declarative/v2/options.go +++ b/internal/declarative/v2/options.go @@ -38,7 +38,6 @@ type Options struct { client.Client TargetCluster ClusterFn - SpecResolver ClientCache ClientCacheKeyFn ManifestParser @@ -95,18 +94,6 @@ func (o WithCustomResourceLabels) Apply(options *Options) { options.PostRenderTransforms = append(options.PostRenderTransforms, labelTransform) } -func WithSpecResolver(resolver SpecResolver) SpecResolverOption { - return SpecResolverOption{resolver} -} - -type SpecResolverOption struct { - SpecResolver -} - -func (o SpecResolverOption) Apply(options *Options) { - options.SpecResolver = o -} - type ObjectTransform = func(context.Context, Object, []*unstructured.Unstructured) error func WithPostRenderTransform(transforms ...ObjectTransform) PostRenderTransformOption { diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 4ac2bc06c4..1bb840e3f6 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -47,13 +47,18 @@ const ( defaultFieldOwner client.FieldOwner = "declarative.kyma-project.io/applier" ) -func NewFromManager(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, - metrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, options ...Option, +func NewFromManager(mgr manager.Manager, + requeueIntervals queue.RequeueIntervals, + metrics *metrics.ManifestMetrics, + mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, + specResolver SpecResolver, + options ...Option, ) *Reconciler { reconciler := &Reconciler{} reconciler.ManifestMetrics = metrics reconciler.MandatoryModuleMetrics = mandatoryModulesMetrics reconciler.RequeueIntervals = requeueIntervals + reconciler.specResolver = specResolver reconciler.Options = DefaultOptions().Apply(WithManager(mgr)).Apply(options...) return reconciler } @@ -63,6 +68,7 @@ type Reconciler struct { *Options ManifestMetrics *metrics.ManifestMetrics MandatoryModuleMetrics *metrics.MandatoryModulesMetrics + specResolver SpecResolver } type ConditionType string @@ -139,8 +145,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } } - spec, err := r.Spec(ctx, manifest) + spec, err := r.specResolver.GetSpec(ctx, manifest) if err != nil { + manifest.SetStatus(manifest.GetStatus().WithState(shared.StateError).WithErr(err)) if !manifest.GetDeletionTimestamp().IsZero() { return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestParseSpec, err) } @@ -293,14 +300,6 @@ func (r *Reconciler) initialize(manifest *v1beta2.Manifest) error { return nil } -func (r *Reconciler) Spec(ctx context.Context, manifest *v1beta2.Manifest) (*Spec, error) { - spec, err := r.SpecResolver.Spec(ctx, manifest) - if err != nil { - manifest.SetStatus(manifest.GetStatus().WithState(shared.StateError).WithErr(err)) - } - return spec, err -} - func (r *Reconciler) renderResources( ctx context.Context, skrClient Client, diff --git a/internal/declarative/v2/spec.go b/internal/declarative/v2/spec.go index e012e5f05b..99e297bd15 100644 --- a/internal/declarative/v2/spec.go +++ b/internal/declarative/v2/spec.go @@ -2,47 +2,15 @@ package v2 import ( "context" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" ) type SpecResolver interface { - Spec(ctx context.Context, object Object) (*Spec, error) + GetSpec(ctx context.Context, manifest *v1beta2.Manifest) (*Spec, error) } -type RenderMode string - -const ( - RenderModeRaw RenderMode = "raw" -) - type Spec struct { ManifestName string Path string OCIRef string - Mode RenderMode -} - -func DefaultSpec(path, ociref string, mode RenderMode) *CustomSpecFns { - return &CustomSpecFns{ - ManifestNameFn: func(_ context.Context, obj Object) string { return obj.GetName() }, - PathFn: func(_ context.Context, _ Object) string { return path }, - OCIRefFn: func(_ context.Context, _ Object) string { return ociref }, - ModeFn: func(_ context.Context, _ Object) RenderMode { return mode }, - } -} - -// CustomSpecFns is a simple static resolver that always uses the same chart and values. -type CustomSpecFns struct { - ManifestNameFn func(ctx context.Context, obj Object) string - PathFn func(ctx context.Context, obj Object) string - OCIRefFn func(ctx context.Context, obj Object) string - ModeFn func(ctx context.Context, obj Object) RenderMode -} - -func (s *CustomSpecFns) Spec(ctx context.Context, obj Object) (*Spec, error) { - return &Spec{ - ManifestName: s.ManifestNameFn(ctx, obj), - Path: s.PathFn(ctx, obj), - OCIRef: s.OCIRefFn(ctx, obj), - Mode: s.ModeFn(ctx, obj), - }, nil } diff --git a/internal/manifest/spec_resolver.go b/internal/manifest/spec_resolver.go index 1066e3b9d4..7b09d5578e 100644 --- a/internal/manifest/spec_resolver.go +++ b/internal/manifest/spec_resolver.go @@ -4,9 +4,6 @@ import ( "context" "errors" "fmt" - "os" - "reflect" - "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/v1/google" "sigs.k8s.io/controller-runtime/pkg/client" @@ -17,74 +14,32 @@ import ( "github.com/kyma-project/lifecycle-manager/pkg/ocmextensions" ) -// RawManifestInfo defines raw manifest information. -type RawManifestInfo struct { - Path string - OCIRef string -} - type SpecResolver struct { - KCP *declarativev2.ClusterInfo + kcpClient client.Client manifestPathExtractor *PathExtractor - ChartCache string - cachedCharts map[string]string } -func NewSpecResolver(kcp *declarativev2.ClusterInfo, extractor *PathExtractor) *SpecResolver { +func NewSpecResolver(kcpClient client.Client, extractor *PathExtractor) *SpecResolver { return &SpecResolver{ - KCP: kcp, + kcpClient: kcpClient, manifestPathExtractor: extractor, - ChartCache: os.TempDir(), - cachedCharts: make(map[string]string), } } -var ( - ErrRenderModeInvalid = errors.New("render mode is invalid") - ErrInvalidObjectPassedToSpecResolution = errors.New("invalid object passed to spec resolution") -) - -func (s *SpecResolver) Spec(ctx context.Context, obj declarativev2.Object) (*declarativev2.Spec, error) { - manifest, ok := obj.(*v1beta2.Manifest) - if !ok { - return nil, fmt.Errorf( - "invalid type %s: %w", reflect.TypeOf(obj), - ErrInvalidObjectPassedToSpecResolution, - ) - } +var errRenderModeInvalid = errors.New("render mode is invalid") +func (s *SpecResolver) GetSpec(ctx context.Context, manifest *v1beta2.Manifest) (*declarativev2.Spec, error) { var imageSpec v1beta2.ImageSpec if err := yaml.Unmarshal(manifest.Spec.Install.Source.Raw, &imageSpec); err != nil { return nil, fmt.Errorf("failed to unmarshal data: %w", err) } - var mode declarativev2.RenderMode - switch imageSpec.Type { - case v1beta2.OciRefType: - mode = declarativev2.RenderModeRaw - default: + if imageSpec.Type != v1beta2.OciRefType { return nil, fmt.Errorf("could not determine render mode for %s: %w", - client.ObjectKeyFromObject(manifest), ErrRenderModeInvalid) - } - - rawManifestInfo, err := s.getRawManifestForInstall(ctx, imageSpec, s.KCP.Client) - if err != nil { - return nil, err + client.ObjectKeyFromObject(manifest), errRenderModeInvalid) } - return &declarativev2.Spec{ - ManifestName: manifest.Spec.Install.Name, - Path: rawManifestInfo.Path, - OCIRef: rawManifestInfo.OCIRef, - Mode: mode, - }, nil -} - -func (s *SpecResolver) getRawManifestForInstall(ctx context.Context, - imageSpec v1beta2.ImageSpec, - targetClient client.Client, -) (*RawManifestInfo, error) { - keyChain, err := s.lookupKeyChain(ctx, imageSpec, targetClient) + keyChain, err := s.lookupKeyChain(ctx, imageSpec) if err != nil { return nil, fmt.Errorf("failed to fetch keyChain: %w", err) } @@ -93,23 +48,22 @@ func (s *SpecResolver) getRawManifestForInstall(ctx context.Context, if err != nil { return nil, fmt.Errorf("failed to extract raw manifest from layer digest: %w", err) } - return &RawManifestInfo{ - Path: rawManifestPath, - OCIRef: imageSpec.Ref, + + return &declarativev2.Spec{ + ManifestName: manifest.Spec.Install.Name, + Path: rawManifestPath, + OCIRef: imageSpec.Ref, }, nil } -func (s *SpecResolver) lookupKeyChain( - ctx context.Context, imageSpec v1beta2.ImageSpec, targetClient client.Client, -) (authn.Keychain, error) { +func (s *SpecResolver) lookupKeyChain(ctx context.Context, imageSpec v1beta2.ImageSpec) (authn.Keychain, error) { var keyChain authn.Keychain var err error - if imageSpec.CredSecretSelector != nil { - if keyChain, err = ocmextensions.GetAuthnKeychain(ctx, imageSpec.CredSecretSelector, targetClient); err != nil { - return nil, err - } - } else { + if imageSpec.CredSecretSelector == nil { keyChain = authn.DefaultKeychain + } else if keyChain, err = ocmextensions.GetAuthnKeychain(ctx, imageSpec.CredSecretSelector, s.kcpClient); err != nil { + return nil, err } + return authn.NewMultiKeychain(google.Keychain, keyChain), nil } From 22ac10577b0eef6df1bcfcf9b27b89f106927aac Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 11:08:21 +0200 Subject: [PATCH 07/10] fix integration test setup --- .../controller/manifest/custom_resource_check/suite_test.go | 5 ++--- tests/integration/controller/manifest/suite_test.go | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/integration/controller/manifest/custom_resource_check/suite_test.go b/tests/integration/controller/manifest/custom_resource_check/suite_test.go index 7e7e4eac07..39ae7f4576 100644 --- a/tests/integration/controller/manifest/custom_resource_check/suite_test.go +++ b/tests/integration/controller/manifest/custom_resource_check/suite_test.go @@ -141,9 +141,8 @@ var _ = BeforeSuite(func() { Error: 1 * time.Second, }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), - declarativev2.WithSpecResolver( - manifest.NewSpecResolver(kcp, extractor), - ), declarativev2.WithRemoteTargetCluster( + manifest.NewSpecResolver(kcp.Client, extractor), + declarativev2.WithRemoteTargetCluster( func(_ context.Context, _ declarativev2.Object) (*declarativev2.ClusterInfo, error) { return &declarativev2.ClusterInfo{Config: authUser.Config()}, nil }, diff --git a/tests/integration/controller/manifest/suite_test.go b/tests/integration/controller/manifest/suite_test.go index a813d1694e..f8ce48125e 100644 --- a/tests/integration/controller/manifest/suite_test.go +++ b/tests/integration/controller/manifest/suite_test.go @@ -137,9 +137,8 @@ var _ = BeforeSuite(func() { Success: 1 * time.Second, Busy: 1 * time.Second, }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), - declarativev2.WithSpecResolver( - manifest.NewSpecResolver(kcp, extractor), - ), declarativev2.WithRemoteTargetCluster( + manifest.NewSpecResolver(kcp.Client, extractor), + declarativev2.WithRemoteTargetCluster( func(_ context.Context, _ declarativev2.Object) (*declarativev2.ClusterInfo, error) { return &declarativev2.ClusterInfo{Config: authUser.Config()}, nil }, From 1dc9ad2fd509d04c56c1c4dd85cc2771c2ccda18 Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 11:14:48 +0200 Subject: [PATCH 08/10] linting --- api/v1beta2/manifest_types.go | 3 ++- internal/declarative/v2/reconciler.go | 6 +++--- internal/declarative/v2/spec.go | 1 + internal/manifest/spec_resolver.go | 1 + 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/api/v1beta2/manifest_types.go b/api/v1beta2/manifest_types.go index e3c902971a..1c34d53bb8 100644 --- a/api/v1beta2/manifest_types.go +++ b/api/v1beta2/manifest_types.go @@ -17,10 +17,11 @@ limitations under the License. package v1beta2 import ( + "strconv" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" machineryruntime "k8s.io/apimachinery/pkg/runtime" - "strconv" "github.com/kyma-project/lifecycle-manager/api/shared" ) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 1bb840e3f6..bc29bdeee6 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -4,15 +4,14 @@ import ( "context" "errors" "fmt" - "github.com/kyma-project/lifecycle-manager/internal" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" "strconv" "time" apicorev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/cli-runtime/pkg/resource" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -22,6 +21,7 @@ import ( "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/internal/pkg/resources" "github.com/kyma-project/lifecycle-manager/pkg/common" diff --git a/internal/declarative/v2/spec.go b/internal/declarative/v2/spec.go index 99e297bd15..6e3044bf8e 100644 --- a/internal/declarative/v2/spec.go +++ b/internal/declarative/v2/spec.go @@ -2,6 +2,7 @@ package v2 import ( "context" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" ) diff --git a/internal/manifest/spec_resolver.go b/internal/manifest/spec_resolver.go index 7b09d5578e..c13362c1aa 100644 --- a/internal/manifest/spec_resolver.go +++ b/internal/manifest/spec_resolver.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/v1/google" "sigs.k8s.io/controller-runtime/pkg/client" From 6570e720cabb5de85ca03ba608eecc664baec95c Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 11:26:56 +0200 Subject: [PATCH 09/10] linting --- internal/declarative/v2/reconciler.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index bc29bdeee6..91de58d41b 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -7,7 +7,6 @@ import ( "strconv" "time" - apicorev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -615,18 +614,6 @@ func (r *Reconciler) getTargetClient(ctx context.Context, manifest *v1beta2.Mani r.AddClient(clientsCacheKey, clnt) } - if apimetav1.NamespaceDefault != apimetav1.NamespaceNone && apimetav1.NamespaceDefault != apimetav1.NamespaceDefault { - err := clnt.Patch( - ctx, &apicorev1.Namespace{ - TypeMeta: apimetav1.TypeMeta{APIVersion: "v1", Kind: "Namespace"}, - ObjectMeta: apimetav1.ObjectMeta{Name: apimetav1.NamespaceDefault}, - }, client.Apply, client.ForceOwnership, defaultFieldOwner, - ) - if err != nil { - return nil, fmt.Errorf("failed to patch namespace: %w", err) - } - } - return clnt, nil } From 1e41a0da1ce320e45d6aad80e59ab95836b91660 Mon Sep 17 00:00:00 2001 From: Benjamin Lindner Date: Thu, 11 Jul 2024 14:00:15 +0200 Subject: [PATCH 10/10] remove generace cache key --- internal/declarative/v2/inmemory_rendered.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/declarative/v2/inmemory_rendered.go b/internal/declarative/v2/inmemory_rendered.go index e2249be8f8..1abcd8af98 100644 --- a/internal/declarative/v2/inmemory_rendered.go +++ b/internal/declarative/v2/inmemory_rendered.go @@ -62,5 +62,5 @@ func (c *InMemoryManifestCache) Parse(spec *Spec, } func generateCacheKey(spec *Spec) string { - return filepath.Join(ManifestFilePrefix, spec.Path, spec.ManifestName) + "-raw" + return filepath.Join(ManifestFilePrefix, spec.Path, spec.ManifestName) }