feat: Avoid Redundant SSA for Manifest Patching

docs/technical-reference/api/manifest-cr.md

@@ -2,6 +2,16 @@
 
 The [Manifest custom resource (CR)](../../../api/v1beta2/manifest_types.go) is our internal representation of what results from the resolution of a ModuleTemplate CR in the context of a single cluster represented by a Kyma CR. Thus, a lot of configuration elements are similar or entirely equivalent to the data layer found in a ModuleTemplate CR.
 
+## Patching
+
+The [Runner](../../../pkg/module/sync/runner.go) is responsible for creating and updating manifests via Server Side Apply (SSA). An update is only performed when one of the following conditions is met:
+
+1. The version of the manifest object is different from the version in the Kyma module status.
+2. The channel of the manifest object is different from the channel in the Kyma module status.
+3. The state of the manifest object is different from the state in the Kyma module status.
+
+*NOTE:* The module status is not present in Kyma for mandatory modules, hence their manifest is updated via SSA in every reconcile loop.
+
 ## Configuration
 
 ### **.spec.remote**

internal/descriptor/provider/provider.go

@@ -12,6 +12,7 @@ import (
 var (
 	ErrTypeAssert    = errors.New("failed to convert to v1beta2.Descriptor")
 	ErrDecode        = errors.New("failed to decode to descriptor target")
+	ErrEncode        = errors.New("failed to encode the descriptor")
 	ErrTemplateNil   = errors.New("module template is nil")
 	ErrDescriptorNil = errors.New("module template contains nil descriptor")
 )
@@ -70,6 +71,20 @@ func (c *CachedDescriptorProvider) GetDescriptor(template *v1beta2.ModuleTemplat
 	return descriptor, nil
 }
 
+func (c *CachedDescriptorProvider) SetDescriptor(template *v1beta2.ModuleTemplate, descriptor *v1beta2.Descriptor,
+) error {
+	if template == nil {
+		return ErrTemplateNil
+	}
+	template.Spec.Descriptor.Object = descriptor
+	encoded, err := compdesc.Encode(descriptor.ComponentDescriptor, compdesc.DefaultJSONCodec)
+	template.Spec.Descriptor.Raw = encoded
+	if err != nil {
+		return errors.Join(ErrEncode, err)
+	}
+	return nil
+}
+
 func (c *CachedDescriptorProvider) Add(template *v1beta2.ModuleTemplate) error {
 	if template == nil {
 		return ErrTemplateNil

internal/descriptor/provider/provider_test.go

@@ -106,3 +106,28 @@ func TestGetDescriptor_OnEmptyCache_AddsDescriptorFromTemplate(t *testing.T) {
 	assert.NotNil(t, entry)
 	assert.Equal(t, expected.Name, entry.Name)
 }
+
+func TestSetDescriptor_OnEmptyTemplate_ReturnsErrTemplateNil(t *testing.T) {
+	descriptorProvider := provider.NewCachedDescriptorProvider(nil)
+
+	err := descriptorProvider.SetDescriptor(nil, nil)
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, provider.ErrTemplateNil)
+}
+
+func TestSetDescriptor_OnProvidedDescriptor_ReturnsNoError(t *testing.T) {
+	descriptorProvider := provider.NewCachedDescriptorProvider(nil)
+	descriptorBefore := &v1beta2.Descriptor{
+		ComponentDescriptor: &compdesc.ComponentDescriptor{Metadata: compdesc.Metadata{ConfiguredVersion: "v1"}},
+	}
+	template := builder.NewModuleTemplateBuilder().WithDescriptor(descriptorBefore).Build()
+
+	descriptorAfter := &v1beta2.Descriptor{
+		ComponentDescriptor: &compdesc.ComponentDescriptor{Metadata: compdesc.Metadata{ConfiguredVersion: "v2"}},
+	}
+	err := descriptorProvider.SetDescriptor(template, descriptorAfter)
+
+	require.NoError(t, err)
+	assert.Equal(t, descriptorAfter, template.Spec.Descriptor.Object)
+}

pkg/module/sync/runner.go

@@ -64,7 +64,7 @@ func (r *Runner) ReconcileManifests(ctx context.Context, kyma *v1beta2.Kyma,
 				results <- nil
 				return
 			}
-			if err := r.updateManifests(ctx, kyma, module); err != nil {
+			if err := r.updateManifest(ctx, kyma, module); err != nil {
 				results <- fmt.Errorf("could not update module %s: %w", module.GetName(), err)
 				return
 			}
@@ -95,7 +95,7 @@ func (r *Runner) getModule(ctx context.Context, module client.Object) error {
 	return nil
 }
 
-func (r *Runner) updateManifests(ctx context.Context, kyma *v1beta2.Kyma,
+func (r *Runner) updateManifest(ctx context.Context, kyma *v1beta2.Kyma,
 	module *common.Module,
 ) error {
 	if err := r.setupModule(module, kyma); err != nil {
@@ -110,17 +110,34 @@ func (r *Runner) updateManifests(ctx context.Context, kyma *v1beta2.Kyma,
 		return commonerrs.ErrTypeAssert
 	}
 
+	moduleStatus := kyma.GetModuleStatusMap()[module.ModuleName]
 	if err := r.doUpdateWithStrategy(ctx, kyma.Labels[shared.ManagedBy], module.Enabled,
-		manifestObj); err != nil {
+		manifestObj, moduleStatus); err != nil {
 		return err
 	}
 	module.Manifest = manifestObj
 	return nil
 }
 
 func (r *Runner) doUpdateWithStrategy(ctx context.Context, owner string, isEnabledModule bool,
-	manifestObj *v1beta2.Manifest,
+	manifestObj *v1beta2.Manifest, kymaModuleStatus *v1beta2.ModuleStatus,
 ) error {
+	manifestInCluster := &v1beta2.Manifest{}
+	if err := r.Get(ctx, client.ObjectKey{
+		Namespace: manifestObj.GetNamespace(),
+		Name:      manifestObj.GetName(),
+	}, manifestInCluster); err != nil {
+		if !util.IsNotFound(err) {
+			return fmt.Errorf("error get manifest %s: %w", client.ObjectKeyFromObject(manifestObj), err)
+		}
+		manifestInCluster = nil
+	}
+
+	if !NeedToUpdate(manifestInCluster, manifestObj, kymaModuleStatus) {
+		// Point to the current state from the cluster for the outside sync of the manifest
+		*manifestObj = *manifestInCluster
+		return nil
+	}
 	if isEnabledModule {
 		return r.patchManifest(ctx, owner, manifestObj)
 	}
@@ -144,23 +161,27 @@ func (r *Runner) patchManifest(ctx context.Context, owner string, manifestObj *v
 
 func (r *Runner) updateAvailableManifestSpec(ctx context.Context, manifestObj *v1beta2.Manifest) error {
 	manifestInCluster := &v1beta2.Manifest{}
-
-	if err := r.Get(ctx, client.ObjectKey{Namespace: manifestObj.GetNamespace(), Name: manifestObj.GetName()},
-		manifestInCluster); err != nil {
+	if err := r.Get(ctx, client.ObjectKey{
+		Namespace: manifestObj.GetNamespace(),
+		Name:      manifestObj.GetName(),
+	}, manifestInCluster); err != nil {
 		return fmt.Errorf("error get manifest %s: %w", client.ObjectKeyFromObject(manifestObj), err)
 	}
-	if !needToUpdate(manifestInCluster, manifestObj) {
-		return nil
-	}
 	manifestInCluster.Spec = manifestObj.Spec
 	if err := r.Update(ctx, manifestInCluster); err != nil {
 		return fmt.Errorf("error update manifest %s: %w", client.ObjectKeyFromObject(manifestObj), err)
 	}
 	return nil
 }
 
-func needToUpdate(manifestInCluster, manifestObj *v1beta2.Manifest) bool {
-	return manifestInCluster.Spec.Version != manifestObj.Spec.Version
+func NeedToUpdate(manifestInCluster, manifestObj *v1beta2.Manifest, moduleStatus *v1beta2.ModuleStatus) bool {
+	if manifestInCluster == nil || moduleStatus == nil { // moduleStatus is nil in case of mandatory module
+		return true
+	}
+
+	return manifestObj.Spec.Version != moduleStatus.Version ||
+		manifestObj.Labels[shared.ChannelLabel] != moduleStatus.Channel ||
+		moduleStatus.State != manifestInCluster.Status.State
 }
 
 func (r *Runner) deleteManifest(ctx context.Context, module *common.Module) error {

pkg/module/sync/runner_test.go

@@ -9,9 +9,11 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/kyma-project/lifecycle-manager/api/shared"
 	"github.com/kyma-project/lifecycle-manager/api/v1beta2"
 	"github.com/kyma-project/lifecycle-manager/pkg/module/sync"
 	"github.com/kyma-project/lifecycle-manager/pkg/testutils"
@@ -186,3 +188,83 @@ func configureModuleInKyma(
 		kyma.Status.Modules = append(kyma.Status.Modules, module)
 	}
 }
+
+func TestNeedToUpdate(t *testing.T) {
+	type args struct {
+		manifestInCluster *v1beta2.Manifest
+		manifestObj       *v1beta2.Manifest
+		moduleStatus      *v1beta2.ModuleStatus
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			"When manifest in cluster is nil, expect need to update",
+			args{nil, &v1beta2.Manifest{}, &v1beta2.ModuleStatus{}},
+			true,
+		},
+		{
+			"When new module version available, expect need to update",
+			args{
+				&v1beta2.Manifest{},
+				&v1beta2.Manifest{
+					ObjectMeta: apimetav1.ObjectMeta{
+						Labels: map[string]string{shared.ChannelLabel: "regular"},
+					},
+					Spec: v1beta2.ManifestSpec{Version: "0.2"},
+				}, &v1beta2.ModuleStatus{Version: "0.1", Channel: "regular"},
+			},
+			true,
+		},
+		{
+			"When channel switch, expect need to update",
+			args{
+				&v1beta2.Manifest{},
+				&v1beta2.Manifest{
+					ObjectMeta: apimetav1.ObjectMeta{
+						Labels: map[string]string{shared.ChannelLabel: "fast"},
+					},
+					Spec: v1beta2.ManifestSpec{Version: "0.1"},
+				}, &v1beta2.ModuleStatus{Version: "0.1", Channel: "regular"},
+			},
+			true,
+		},
+		{
+			"When cluster Manifest in divergent state, expect need to update",
+			args{
+				&v1beta2.Manifest{Status: shared.Status{
+					State: "Warning",
+				}},
+				&v1beta2.Manifest{},
+				&v1beta2.ModuleStatus{State: "Ready"},
+			},
+			true,
+		},
+		{
+			"When no update required, expect no update",
+			args{
+				&v1beta2.Manifest{
+					Status: shared.Status{
+						State: "Ready",
+					},
+					Spec: v1beta2.ManifestSpec{Version: "0.1"},
+				},
+				&v1beta2.Manifest{
+					ObjectMeta: apimetav1.ObjectMeta{
+						Labels: map[string]string{shared.ChannelLabel: "regular"},
+					},
+					Spec: v1beta2.ManifestSpec{Version: "0.1"},
+				},
+				&v1beta2.ModuleStatus{State: "Ready", Version: "0.1", Channel: "regular"},
+			},
+			false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, sync.NeedToUpdate(tt.args.manifestInCluster, tt.args.manifestObj, tt.args.moduleStatus), "needToUpdate(%v, %v, %v)", tt.args.manifestInCluster, tt.args.manifestObj, tt.args.moduleStatus)
+		})
+	}
+}

pkg/testutils/moduletemplate.go

@@ -27,6 +27,16 @@ func GetModuleTemplate(ctx context.Context,
 	return templateInfo.ModuleTemplate, nil
 }
 
+func UpdateModuleTemplate(ctx context.Context,
+	clnt client.Client,
+	moduleTemplate *v1beta2.ModuleTemplate,
+) error {
+	if err := clnt.Update(ctx, moduleTemplate); err != nil {
+		return fmt.Errorf("update module tempate: %w", err)
+	}
+	return nil
+}
+
 func ModuleTemplateExists(ctx context.Context,
 	clnt client.Client,
 	module v1beta2.Module,
@@ -65,8 +75,8 @@ func UpdateModuleTemplateSpec(ctx context.Context,
 		return ErrManifestResourceIsNil
 	}
 	moduleTemplate.Spec.Data.Object["spec"] = map[string]any{key: newValue}
-	if err := clnt.Update(ctx, moduleTemplate); err != nil {
-		return fmt.Errorf("update module tempate: %w", err)
+	if err := UpdateModuleTemplate(ctx, clnt, moduleTemplate); err != nil {
+		return err
 	}
 	return nil
 }
@@ -102,3 +112,18 @@ func ReadModuleVersionFromModuleTemplate(ctx context.Context, clnt client.Client
 
 	return ocmDesc.Version, nil
 }
+
+func ModifyModuleTemplateVersion(moduleTemplate *v1beta2.ModuleTemplate, newVersion string) error {
+	descriptorProvider := provider.NewCachedDescriptorProvider(nil)
+	ocmDesc, err := descriptorProvider.GetDescriptor(moduleTemplate)
+	if err != nil {
+		return fmt.Errorf("failed to get descriptor: %w", err)
+	}
+	ocmDesc.Version = newVersion
+	err = descriptorProvider.SetDescriptor(moduleTemplate, ocmDesc)
+	if err != nil {
+		return fmt.Errorf("failed to set descriptor: %w", err)
+	}
+
+	return nil
+}

tests/integration/controller/kyma/kyma_test.go

@@ -514,9 +514,33 @@ func updateKCPModuleTemplateSpecData(kymaName, valueUpdated string) func() error
 			return err
 		}
 		for _, activeModule := range createdKyma.Spec.Modules {
-			return UpdateModuleTemplateSpec(ctx, kcpClient,
+			err := UpdateModuleTemplateSpec(ctx, kcpClient,
 				activeModule, InitSpecKey, valueUpdated, createdKyma.Spec.Channel)
+			if err != nil {
+				return err
+			}
+			err = changeKCPModuleTemplateVersion(ctx, activeModule, createdKyma.Spec.Channel, "v1.7.2") // required to allow SSA for manifest
+			if err != nil {
+				return err
+			}
 		}
 		return nil
 	}
 }
+
+func changeKCPModuleTemplateVersion(ctx context.Context,
+	module v1beta2.Module, channel, version string,
+) error {
+	moduleTemplate, err := GetModuleTemplate(ctx, kcpClient, module, channel)
+	if err != nil {
+		return err
+	}
+	err = ModifyModuleTemplateVersion(moduleTemplate, version)
+	if err != nil {
+		return err
+	}
+	if err := UpdateModuleTemplate(ctx, kcpClient, moduleTemplate); err != nil {
+		return err
+	}
+	return nil
+}

tests/integration/controller/kyma/manifest_test.go

@@ -112,6 +112,7 @@ var _ = Describe("Update Manifest CR", Ordered, func() {
 					Fail("Can't find \"baseUrl\" property in ModuleTemplate spec")
 				}
 				repositoryContext["baseUrl"] = updateRepositoryURL
+				descriptor.Version = "v1.7.3" // required to allow for SSA of manifest
 
 				newDescriptorRaw, err := compdesc.Encode(descriptor.ComponentDescriptor, compdesc.DefaultJSONCodec)
 				Expect(err).ToNot(HaveOccurred())
@@ -209,6 +210,7 @@ var _ = Describe("Manifest.Spec is reset after manual update", Ordered, func() {
 
 		manifestImageSpec := extractInstallImageSpec(manifest.Spec.Install)
 		manifestImageSpec.Repo = updateRepositoryURL
+		manifest.Spec.Version = "v1.7.0" // required to allow for SSA of manifest
 
 		// is there a simpler way to update manifest.Spec.Install?
 		updatedBytes, err := json.Marshal(manifestImageSpec)