From d2b4f3f13a249718ec68ab177a161fec9304730e Mon Sep 17 00:00:00 2001 From: Xin Ruan Date: Mon, 29 Jul 2024 07:40:06 +0200 Subject: [PATCH] feat: Add Version to Kyma.Spec.Modules (#1694) * add version to kyma.spec.modules list * add missing manifest update base on moduletemplate generation change. * remove parameter for NewCachedDescriptorProvider * fix dead link * adjust unit test coverage * fix flaky test * chore: Refactor NewCachedDescriptorProvider (#1695) * remove parameter for NewCachedDescriptorProvider * fix dead link * adjust unit test coverage * fix flaky test * refactor FilterTemplate * add integration test * Update tests/integration/controller/kyma/kyma_module_channel_test.go Co-authored-by: Tomasz Smelcerz * Update tests/integration/controller/kyma/kyma_module_version_test.go Co-authored-by: Tomasz Smelcerz * fix existing test --------- Co-authored-by: Tomasz Smelcerz --- api/shared/channel.go | 8 + api/v1beta2/kyma_types.go | 45 ++++-- api/v1beta2/kyma_types_test.go | 152 ++++++++++++++++++ api/v1beta2/moduletemplate_types.go | 19 +++ .../bases/operator.kyma-project.io_kymas.yaml | 18 +++ ...rator.kyma-project.io_moduletemplates.yaml | 4 +- internal/controller/kyma/controller.go | 31 +--- internal/descriptor/cache/key.go | 14 +- internal/descriptor/provider/provider_test.go | 6 +- pkg/module/parse/template_to_module.go | 48 +++--- pkg/module/sync/errors.go | 5 - pkg/templatelookup/regular.go | 6 + pkg/templatelookup/regular_test.go | 50 ++++++ pkg/testutils/utils.go | 11 +- tests/e2e/module_status_decoupling_test.go | 2 +- .../controller/kyma/helper_test.go | 4 +- .../kyma/kyma_module_channel_test.go | 2 +- .../kyma/kyma_module_enable_test.go | 44 +++++ .../kyma/kyma_module_version_test.go | 39 +++++ .../controller/kyma/manifest_test.go | 12 +- unit-test-coverage.yaml | 14 +- 21 files changed, 434 insertions(+), 100 deletions(-) create mode 100644 api/shared/channel.go create mode 100644 api/v1beta2/kyma_types_test.go delete mode 100644 pkg/module/sync/errors.go create mode 100644 tests/integration/controller/kyma/kyma_module_enable_test.go create mode 100644 tests/integration/controller/kyma/kyma_module_version_test.go diff --git a/api/shared/channel.go b/api/shared/channel.go new file mode 100644 index 0000000000..b041a78fc6 --- /dev/null +++ b/api/shared/channel.go @@ -0,0 +1,8 @@ +package shared + +type Channel string + +const ( + // NoneChannel when this value is defined for the ModuleTemplate, it means that the ModuleTemplate is not assigned to any channel. + NoneChannel Channel = "none" +) diff --git a/api/v1beta2/kyma_types.go b/api/v1beta2/kyma_types.go index dff905ad65..93094398b3 100644 --- a/api/v1beta2/kyma_types.go +++ b/api/v1beta2/kyma_types.go @@ -71,11 +71,19 @@ type Module struct { // Channel is the desired channel of the Module. If this changes or is set, it will be used to resolve a new // ModuleTemplate based on the new resolved resources. + // The Version and Channel are mutually exclusive options. // +kubebuilder:validation:Pattern:=^[a-z]+$ // +kubebuilder:validation:MaxLength:=32 // +kubebuilder:validation:MinLength:=3 Channel string `json:"channel,omitempty"` + // Version is the desired version of the Module. If this changes or is set, it will be used to resolve a new + // ModuleTemplate based on this specific version. + // The Version and Channel are mutually exclusive options. + // The regular expression come from here: https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string + // +kubebuilder:validation:Pattern:=`^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$` + Version string `json:"version,omitempty"` + // RemoteModuleTemplateRef is deprecated and will no longer have any functionality. // It will be removed in the upcoming API version. RemoteModuleTemplateRef string `json:"remoteModuleTemplateRef,omitempty"` @@ -208,14 +216,6 @@ type PartialMeta struct { const DefaultChannel = "regular" -func PartialMetaFromObject(object apimetav1.Object) PartialMeta { - return PartialMeta{ - Name: object.GetName(), - Namespace: object.GetNamespace(), - Generation: object.GetGeneration(), - } -} - func (m PartialMeta) GetName() string { return m.Name } @@ -388,6 +388,7 @@ func (kyma *Kyma) IsBeta() bool { type AvailableModule struct { Module Enabled bool + Valid bool } func (kyma *Kyma) GetAvailableModules() []AvailableModule { @@ -395,25 +396,43 @@ func (kyma *Kyma) GetAvailableModules() []AvailableModule { modules := make([]AvailableModule, 0) for _, module := range kyma.Spec.Modules { moduleMap[module.Name] = true - modules = append(modules, AvailableModule{Module: module, Enabled: true}) + if strings.ToLower(module.Channel) == string(shared.NoneChannel) { + modules = append(modules, AvailableModule{Module: module, Enabled: true, Valid: false}) + continue + } + if module.Version != "" && module.Channel != "" { + modules = append(modules, AvailableModule{Module: module, Enabled: true, Valid: false}) + continue + } + modules = append(modules, AvailableModule{Module: module, Enabled: true, Valid: true}) } - for _, module := range kyma.Status.Modules { - _, exist := moduleMap[module.Name] + for _, moduleInStatus := range kyma.Status.Modules { + _, exist := moduleMap[moduleInStatus.Name] if exist { continue } + modules = append(modules, AvailableModule{ Module: Module{ - Name: module.Name, - Channel: module.Channel, + Name: moduleInStatus.Name, + Channel: moduleInStatus.Channel, + Version: moduleInStatus.Version, }, Enabled: false, + Valid: determineModuleValidity(moduleInStatus), }) } return modules } +func determineModuleValidity(moduleStatus ModuleStatus) bool { + if moduleStatus.Template == nil { + return false + } + return true +} + func (kyma *Kyma) EnsureLabelsAndFinalizers() bool { if controllerutil.ContainsFinalizer(kyma, "foregroundDeletion") { return false diff --git a/api/v1beta2/kyma_types_test.go b/api/v1beta2/kyma_types_test.go new file mode 100644 index 0000000000..525de6954d --- /dev/null +++ b/api/v1beta2/kyma_types_test.go @@ -0,0 +1,152 @@ +package v1beta2 + +import ( + "reflect" + "testing" + + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func Test_GetAvailableModules_When_ModuleInSpecOnly(t *testing.T) { + tests := []struct { + name string + KymaSpec KymaSpec + want []AvailableModule + }{ + { + name: "When Channel and Version are both set, then the module is invalid", + KymaSpec: KymaSpec{ + Modules: []Module{ + {Name: "Module1", Channel: "regular", Version: "v1.0"}, + }, + }, + want: []AvailableModule{ + {Module: Module{Name: "Module1", Channel: "regular", Version: "v1.0"}, Enabled: true, Valid: false}, + }, + }, + { + name: "When Channel is set, then the module is valid", + KymaSpec: KymaSpec{ + Modules: []Module{ + {Name: "Module1", Channel: "regular"}, + }, + }, + want: []AvailableModule{ + {Module: Module{Name: "Module1", Channel: "regular"}, Enabled: true, Valid: true}, + }, + }, + { + name: "When Version is set, then the module is valid", + KymaSpec: KymaSpec{ + Modules: []Module{ + {Name: "Module1", Version: "v1.0"}, + }, + }, + want: []AvailableModule{ + {Module: Module{Name: "Module1", Version: "v1.0"}, Enabled: true, Valid: true}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + kyma := &Kyma{ + Spec: tt.KymaSpec, + } + if got := kyma.GetAvailableModules(); !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetAvailableModules() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_GetAvailableModules_When_ModuleInStatusOnly(t *testing.T) { + tests := []struct { + name string + KymaStatus KymaStatus + want []AvailableModule + }{ + { + name: "When Template exists, then the module is valid", + KymaStatus: KymaStatus{ + Modules: []ModuleStatus{ + { + Name: "Module1", + Channel: "regular", + Version: "v1.0", + Template: &TrackingObject{TypeMeta: apimetav1.TypeMeta{Kind: "ModuleTemplate"}}, + }, + }, + }, + want: []AvailableModule{ + {Module: Module{Name: "Module1", Channel: "regular", Version: "v1.0"}, Enabled: false, Valid: true}, + }, + }, + { + name: "When Template not exists,then the module is invalid", + KymaStatus: KymaStatus{ + Modules: []ModuleStatus{ + { + Name: "Module1", + Channel: "regular", + Version: "v1.0", + Template: nil, + }, + }, + }, + want: []AvailableModule{ + {Module: Module{Name: "Module1", Channel: "regular", Version: "v1.0"}, Enabled: false, Valid: false}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + kyma := &Kyma{ + Status: tt.KymaStatus, + } + if got := kyma.GetAvailableModules(); !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetAvailableModules() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_GetAvailableModules_When_ModuleExistsInSpecAndStatus(t *testing.T) { + tests := []struct { + name string + KymaSpec KymaSpec + KymaStatus KymaStatus + want []AvailableModule + }{ + { + name: "When Module have different version between Spec and Status, the output should be based on Spec", + KymaSpec: KymaSpec{ + Modules: []Module{ + {Name: "Module1", Version: "v1.1"}, + }, + }, + KymaStatus: KymaStatus{ + Modules: []ModuleStatus{ + { + Name: "Module1", + Version: "v1.0", + }, + }, + }, + want: []AvailableModule{ + {Module: Module{Name: "Module1", Version: "v1.1"}, Enabled: true, Valid: true}, + }, + }, + } + for _, tt := range tests { + test := tt + t.Run(test.name, func(t *testing.T) { + kyma := &Kyma{ + Spec: test.KymaSpec, + Status: test.KymaStatus, + } + if got := kyma.GetAvailableModules(); !reflect.DeepEqual(got, test.want) { + t.Errorf("GetAvailableModules() = %v, want %v", got, test.want) + } + }) + } +} diff --git a/api/v1beta2/moduletemplate_types.go b/api/v1beta2/moduletemplate_types.go index 11fa3e8585..90fde2d0af 100644 --- a/api/v1beta2/moduletemplate_types.go +++ b/api/v1beta2/moduletemplate_types.go @@ -17,8 +17,11 @@ limitations under the License. package v1beta2 import ( + "errors" + "fmt" "strings" + "github.com/Masterminds/semver/v3" "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -183,6 +186,22 @@ func (m *ModuleTemplate) IsInternal() bool { return false } +var ErrInvalidVersion = errors.New("can't find valid semantic version") + +func (m *ModuleTemplate) GetVersion() (*semver.Version, error) { + if m.Annotations != nil { + moduleVersion, found := m.Annotations[shared.ModuleVersionAnnotation] + if found { + version, err := semver.NewVersion(moduleVersion) + if err != nil { + return nil, fmt.Errorf("%w: %s", ErrInvalidVersion, err.Error()) + } + return version, nil + } + } + return nil, ErrInvalidVersion +} + func (m *ModuleTemplate) IsBeta() bool { if isBeta, found := m.Labels[shared.BetaLabel]; found { return strings.ToLower(isBeta) == shared.EnableLabelValue diff --git a/config/crd/bases/operator.kyma-project.io_kymas.yaml b/config/crd/bases/operator.kyma-project.io_kymas.yaml index 304b0cc8cd..9ebcd9386a 100644 --- a/config/crd/bases/operator.kyma-project.io_kymas.yaml +++ b/config/crd/bases/operator.kyma-project.io_kymas.yaml @@ -64,6 +64,7 @@ spec: description: |- Channel is the desired channel of the Module. If this changes or is set, it will be used to resolve a new ModuleTemplate based on the new resolved resources. + The Version and Channel are mutually exclusive options. maxLength: 32 minLength: 3 pattern: ^[a-z]+$ @@ -97,6 +98,14 @@ spec: RemoteModuleTemplateRef is deprecated and will no longer have any functionality. It will be removed in the upcoming API version. type: string + version: + description: |- + Version is the desired version of the Module. If this changes or is set, it will be used to resolve a new + ModuleTemplate based on this specific version. + The Version and Channel are mutually exclusive options. + The regular expression come from here: https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string + pattern: ^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$ + type: string required: - name type: object @@ -497,6 +506,7 @@ spec: description: |- Channel is the desired channel of the Module. If this changes or is set, it will be used to resolve a new ModuleTemplate based on the new resolved resources. + The Version and Channel are mutually exclusive options. maxLength: 32 minLength: 3 pattern: ^[a-z]+$ @@ -530,6 +540,14 @@ spec: RemoteModuleTemplateRef is deprecated and will no longer have any functionality. It will be removed in the upcoming API version. type: string + version: + description: |- + Version is the desired version of the Module. If this changes or is set, it will be used to resolve a new + ModuleTemplate based on this specific version. + The Version and Channel are mutually exclusive options. + The regular expression come from here: https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string + pattern: ^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$ + type: string required: - name type: object diff --git a/config/crd/bases/operator.kyma-project.io_moduletemplates.yaml b/config/crd/bases/operator.kyma-project.io_moduletemplates.yaml index a41f184b1a..a91710f4c8 100644 --- a/config/crd/bases/operator.kyma-project.io_moduletemplates.yaml +++ b/config/crd/bases/operator.kyma-project.io_moduletemplates.yaml @@ -228,8 +228,8 @@ spec: pattern: ^([a-z]{3,}(-[a-z]{3,})*)?$ type: string version: - description: Version identifies the version of the Module. It can - be empty, or a semantic version. + description: Version identifies the version of the Module. Can be + empty, or a semantic version. maxLength: 32 pattern: ^((0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(-[a-zA-Z-][0-9a-zA-Z-]*)?)?$ type: string diff --git a/internal/controller/kyma/controller.go b/internal/controller/kyma/controller.go index f8a8eea7b9..28fe16995f 100644 --- a/internal/controller/kyma/controller.go +++ b/internal/controller/kyma/controller.go @@ -37,7 +37,6 @@ import ( "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/internal/remote" "github.com/kyma-project/lifecycle-manager/pkg/log" - "github.com/kyma-project/lifecycle-manager/pkg/module/common" "github.com/kyma-project/lifecycle-manager/pkg/module/parse" "github.com/kyma-project/lifecycle-manager/pkg/module/sync" "github.com/kyma-project/lifecycle-manager/pkg/queue" @@ -50,11 +49,10 @@ import ( var ErrManifestsStillExist = errors.New("manifests still exist") const ( - moduleReconciliationError event.Reason = "ModuleReconciliationError" - metricsError event.Reason = "MetricsError" - updateSpecError event.Reason = "UpdateSpecError" - updateStatusError event.Reason = "UpdateStatusError" - patchStatusError event.Reason = "PatchStatus" + metricsError event.Reason = "MetricsError" + updateSpecError event.Reason = "UpdateSpecError" + updateStatusError event.Reason = "UpdateStatusError" + patchStatusError event.Reason = "PatchStatus" ) type Reconciler struct { @@ -472,17 +470,14 @@ func (r *Reconciler) updateKyma(ctx context.Context, kyma *v1beta2.Kyma) error { } func (r *Reconciler) reconcileManifests(ctx context.Context, kyma *v1beta2.Kyma) error { - modules, err := r.GenerateModulesFromTemplate(ctx, kyma) - if err != nil { - return fmt.Errorf("error while fetching modules during processing: %w", err) - } + templates := templatelookup.NewTemplateLookup(client.Reader(r), r.DescriptorProvider).GetRegularTemplates(ctx, kyma) + parser := parse.NewParser(r.Client, r.DescriptorProvider, r.InKCPMode, r.RemoteSyncNamespace) + modules := parser.GenerateModulesFromTemplates(kyma, templates) runner := sync.New(r) - if err := runner.ReconcileManifests(ctx, kyma, modules); err != nil { return fmt.Errorf("sync failed: %w", err) } - runner.SyncModuleStatus(ctx, kyma, modules, r.Metrics) // If module get removed from kyma, the module deletion happens here. if err := r.DeleteNoLongerExistingModules(ctx, kyma); err != nil { @@ -529,18 +524,6 @@ func (r *Reconciler) updateStatusWithError(ctx context.Context, kyma *v1beta2.Ky return nil } -func (r *Reconciler) GenerateModulesFromTemplate(ctx context.Context, kyma *v1beta2.Kyma) (common.Modules, error) { - lookup := templatelookup.NewTemplateLookup(client.Reader(r), r.DescriptorProvider) - templates := lookup.GetRegularTemplates(ctx, kyma) - for _, template := range templates { - if template.Err != nil { - r.Event.Warning(kyma, moduleReconciliationError, template.Err) - } - } - parser := parse.NewParser(r.Client, r.DescriptorProvider, r.InKCPMode, r.RemoteSyncNamespace) - return parser.GenerateModulesFromTemplates(kyma, templates), nil -} - func (r *Reconciler) DeleteNoLongerExistingModules(ctx context.Context, kyma *v1beta2.Kyma) error { moduleStatus := kyma.GetNoLongerExistingModuleStatus() var err error diff --git a/internal/descriptor/cache/key.go b/internal/descriptor/cache/key.go index 643dee5742..f5d7cf90e2 100644 --- a/internal/descriptor/cache/key.go +++ b/internal/descriptor/cache/key.go @@ -3,22 +3,16 @@ package cache import ( "fmt" - "github.com/Masterminds/semver/v3" - - "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" ) type DescriptorKey string func GenerateDescriptorKey(template *v1beta2.ModuleTemplate) DescriptorKey { - if template.Annotations != nil { - moduleVersion := template.Annotations[shared.ModuleVersionAnnotation] - _, err := semver.NewVersion(moduleVersion) - if moduleVersion != "" && err == nil { - return DescriptorKey(fmt.Sprintf("%s:%s:%d:%s", template.Name, template.Spec.Channel, template.Generation, - moduleVersion)) - } + version, err := template.GetVersion() + if err == nil { + return DescriptorKey(fmt.Sprintf("%s:%s:%d:%s", template.Name, template.Spec.Channel, template.Generation, + version)) } return DescriptorKey(fmt.Sprintf("%s:%s:%d", template.Name, template.Spec.Channel, template.Generation)) diff --git a/internal/descriptor/provider/provider_test.go b/internal/descriptor/provider/provider_test.go index b526e35bf0..22fe5d97ec 100644 --- a/internal/descriptor/provider/provider_test.go +++ b/internal/descriptor/provider/provider_test.go @@ -14,7 +14,7 @@ import ( ) func TestGetDescriptor_OnEmptySpec_ReturnsErrDecode(t *testing.T) { - descriptorProvider := provider.NewCachedDescriptorProvider() + descriptorProvider := provider.NewCachedDescriptorProvider() // assuming it handles nil cache internally template := &v1beta2.ModuleTemplate{} _, err := descriptorProvider.GetDescriptor(template) @@ -81,7 +81,9 @@ func TestAdd_OnDescriptorTypeButNull_ReturnsNoError(t *testing.T) { func TestGetDescriptor_OnEmptyCache_AddsDescriptorFromTemplate(t *testing.T) { descriptorCache := cache.NewDescriptorCache() - descriptorProvider := provider.CachedDescriptorProvider{DescriptorCache: descriptorCache} + descriptorProvider := &provider.CachedDescriptorProvider{ + DescriptorCache: descriptorCache, + } expected := &v1beta2.Descriptor{ ComponentDescriptor: &compdesc.ComponentDescriptor{ diff --git a/pkg/module/parse/template_to_module.go b/pkg/module/parse/template_to_module.go index 7b136212db..466321564f 100644 --- a/pkg/module/parse/template_to_module.go +++ b/pkg/module/parse/template_to_module.go @@ -48,8 +48,10 @@ func (p *Parser) GenerateModulesFromTemplates(kyma *v1beta2.Kyma, templates temp modules := make(common.Modules, 0) for _, module := range kyma.GetAvailableModules() { - template := templates[module.Name] - modules = p.appendModuleWithInformation(module, kyma, template, modules) + template, found := templates[module.Name] + if found { + modules = p.appendModuleWithInformation(module, kyma, template, modules) + } } return modules } @@ -81,51 +83,47 @@ func (p *Parser) GenerateMandatoryModulesFromTemplates(ctx context.Context, return modules } -func (p *Parser) appendModuleWithInformation(module v1beta2.AvailableModule, kyma *v1beta2.Kyma, +func (p *Parser) appendModuleWithInformation(availableModule v1beta2.AvailableModule, kyma *v1beta2.Kyma, template *templatelookup.ModuleTemplateInfo, modules common.Modules, ) common.Modules { if template.Err != nil && !errors.Is(template.Err, templatelookup.ErrTemplateNotAllowed) { - modules = append(modules, &common.Module{ - ModuleName: module.Name, - Template: template, - Enabled: module.Enabled, - }) - return modules + return appendInvalidModule(availableModule, modules, template) } descriptor, err := p.descriptorProvider.GetDescriptor(template.ModuleTemplate) if err != nil { template.Err = err - modules = append(modules, &common.Module{ - ModuleName: module.Name, - Template: template, - Enabled: module.Enabled, - }) - return modules + return appendInvalidModule(availableModule, modules, template) } fqdn := descriptor.GetName() - name := common.CreateModuleName(fqdn, kyma.Name, module.Name) + name := common.CreateModuleName(fqdn, kyma.Name, availableModule.Name) setNameAndNamespaceIfEmpty(template, name, p.remoteSyncNamespace) var manifest *v1beta2.Manifest - if manifest, err = p.newManifestFromTemplate(module.Module, + if manifest, err = p.newManifestFromTemplate(availableModule.Module, template.ModuleTemplate); err != nil { template.Err = err - modules = append(modules, &common.Module{ - ModuleName: module.Name, - Template: template, - Enabled: module.Enabled, - }) - return modules + return appendInvalidModule(availableModule, modules, template) } // we name the manifest after the module name manifest.SetName(name) // to have correct owner references, the manifest must always have the same namespace as kyma manifest.SetNamespace(kyma.GetNamespace()) modules = append(modules, &common.Module{ - ModuleName: module.Name, + ModuleName: availableModule.Name, FQDN: fqdn, Template: template, Manifest: manifest, - Enabled: module.Enabled, + Enabled: availableModule.Enabled, + }) + return modules +} + +func appendInvalidModule(availableModule v1beta2.AvailableModule, modules common.Modules, + template *templatelookup.ModuleTemplateInfo, +) common.Modules { + modules = append(modules, &common.Module{ + ModuleName: availableModule.Name, + Template: template, + Enabled: availableModule.Enabled, }) return modules } diff --git a/pkg/module/sync/errors.go b/pkg/module/sync/errors.go deleted file mode 100644 index 623885d2fb..0000000000 --- a/pkg/module/sync/errors.go +++ /dev/null @@ -1,5 +0,0 @@ -package sync - -import "errors" - -var ErrManifestConversion = errors.New("manifest casting error") diff --git a/pkg/templatelookup/regular.go b/pkg/templatelookup/regular.go index 53a5c99610..0c8d6c7b9a 100644 --- a/pkg/templatelookup/regular.go +++ b/pkg/templatelookup/regular.go @@ -22,6 +22,7 @@ var ( ErrTemplateMarkedAsMandatory = errors.New("template marked as mandatory") ErrTemplateNotAllowed = errors.New("module template not allowed") ErrTemplateUpdateNotAllowed = errors.New("module template update not allowed") + ErrTemplateNotValid = errors.New("given module template is not valid") ) type ModuleTemplateInfo struct { @@ -51,6 +52,11 @@ func (t *TemplateLookup) GetRegularTemplates(ctx context.Context, kyma *v1beta2. if found { continue } + if !module.Valid { + templates[module.Name] = &ModuleTemplateInfo{Err: fmt.Errorf("%w: invalid module", ErrTemplateNotValid)} + continue + } + templateInfo := t.GetAndValidate(ctx, module.Name, module.Channel, kyma.Spec.Channel) templateInfo = ValidateTemplateMode(templateInfo, kyma) if templateInfo.Err != nil { diff --git a/pkg/templatelookup/regular_test.go b/pkg/templatelookup/regular_test.go index 3d67f2f343..7a5116e8d2 100644 --- a/pkg/templatelookup/regular_test.go +++ b/pkg/templatelookup/regular_test.go @@ -92,6 +92,56 @@ func TestValidateTemplateMode(t *testing.T) { } } +func Test_GetRegularTemplates_WhenInvalidModuleProvided(t *testing.T) { + tests := []struct { + name string + KymaSpec v1beta2.KymaSpec + KymaStatus v1beta2.KymaStatus + wantErr error + }{ + { + name: "When Module in Spec contains both Channel and Version, Then result contains error", + KymaSpec: v1beta2.KymaSpec{ + Modules: []v1beta2.Module{ + {Name: "Module1", Channel: "regular", Version: "v1.0"}, + }, + }, + wantErr: templatelookup.ErrTemplateNotValid, + }, + { + name: "When Template not exists in Status, Then result contains error", + KymaStatus: v1beta2.KymaStatus{ + Modules: []v1beta2.ModuleStatus{ + { + Name: "Module1", + Channel: "regular", + Version: "v1.0", + Template: nil, + }, + }, + }, + wantErr: templatelookup.ErrTemplateNotValid, + }, + } + + for _, tt := range tests { + test := tt + t.Run(tt.name, func(t *testing.T) { + lookup := templatelookup.NewTemplateLookup(nil, provider.NewCachedDescriptorProvider()) + kyma := &v1beta2.Kyma{ + Spec: test.KymaSpec, + Status: test.KymaStatus, + } + got := lookup.GetRegularTemplates(context.TODO(), kyma) + for _, err := range got { + if !errors.Is(err.Err, test.wantErr) { + t.Errorf("GetRegularTemplates() = %v, want %v", got, test.wantErr) + } + } + }) + } +} + func TestTemplateLookup_GetRegularTemplates_WhenSwitchModuleChannel(t *testing.T) { testModule := testutils.NewTestModule("module1", "new_channel") diff --git a/pkg/testutils/utils.go b/pkg/testutils/utils.go index 32b715449d..eb281cdf26 100644 --- a/pkg/testutils/utils.go +++ b/pkg/testutils/utils.go @@ -42,17 +42,22 @@ var ( ) func NewTestModule(name, channel string) v1beta2.Module { - return NewTestModuleWithFixName(fmt.Sprintf("%s-%s", name, random.Name()), channel) + return NewTestModuleWithFixName(fmt.Sprintf("%s-%s", name, random.Name()), channel, "") +} + +func NewTestModuleWithChannelVersion(name, channel, version string) v1beta2.Module { + return NewTestModuleWithFixName(fmt.Sprintf("%s-%s", name, random.Name()), channel, version) } func NewTemplateOperator(channel string) v1beta2.Module { - return NewTestModuleWithFixName("template-operator", channel) + return NewTestModuleWithFixName("template-operator", channel, "") } -func NewTestModuleWithFixName(name, channel string) v1beta2.Module { +func NewTestModuleWithFixName(name, channel, version string) v1beta2.Module { return v1beta2.Module{ Name: name, Channel: channel, + Version: version, } } diff --git a/tests/e2e/module_status_decoupling_test.go b/tests/e2e/module_status_decoupling_test.go index b6890935fb..6a9bcb6336 100644 --- a/tests/e2e/module_status_decoupling_test.go +++ b/tests/e2e/module_status_decoupling_test.go @@ -18,7 +18,7 @@ import ( var _ = Describe("Module Status Decoupling", Ordered, func() { kyma := NewKymaWithSyncLabel("kyma-sample", ControlPlaneNamespace, v1beta2.DefaultChannel) module := NewTemplateOperator(v1beta2.DefaultChannel) - moduleWrongConfig := NewTestModuleWithFixName("template-operator-misconfigured", "regular") + moduleWrongConfig := NewTestModuleWithFixName("template-operator-misconfigured", "regular", "") moduleCR := NewTestModuleCR(RemoteNamespace) InitEmptyKymaBeforeAll(kyma) CleanupKymaAfterAll(kyma) diff --git a/tests/integration/controller/kyma/helper_test.go b/tests/integration/controller/kyma/helper_test.go index 47beda0df4..4a1dd4ee10 100644 --- a/tests/integration/controller/kyma/helper_test.go +++ b/tests/integration/controller/kyma/helper_test.go @@ -79,8 +79,8 @@ func DeployModuleTemplates(ctx context.Context, kcpClient client.Client, kyma *v WithLabelModuleName(module.Name). WithChannel(module.Channel). WithOCM(compdescv2.SchemaVersion).Build() - Eventually(kcpClient.Create, Timeout, Interval).WithContext(ctx). - WithArguments(template). + Eventually(CreateCR, Timeout, Interval).WithContext(ctx). + WithArguments(kcpClient, template). Should(Succeed()) } } diff --git a/tests/integration/controller/kyma/kyma_module_channel_test.go b/tests/integration/controller/kyma/kyma_module_channel_test.go index ef8b65c96e..c794de206a 100644 --- a/tests/integration/controller/kyma/kyma_module_channel_test.go +++ b/tests/integration/controller/kyma/kyma_module_channel_test.go @@ -76,7 +76,7 @@ var _ = Describe("module channel different from the global channel", func() { }) }) -var _ = Describe("Given invalid channel", func() { +var _ = Describe("Given invalid channel which is rejected by CRD validation rules", func() { DescribeTable( "Test kyma CR, module template creation", func(givenCondition func() error) { Eventually(givenCondition, Timeout, Interval).Should(Succeed()) diff --git a/tests/integration/controller/kyma/kyma_module_enable_test.go b/tests/integration/controller/kyma/kyma_module_enable_test.go new file mode 100644 index 0000000000..de83028fc0 --- /dev/null +++ b/tests/integration/controller/kyma/kyma_module_enable_test.go @@ -0,0 +1,44 @@ +package kyma_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + . "github.com/kyma-project/lifecycle-manager/pkg/testutils" +) + +var _ = Describe("Given kyma CR with invalid module enabled", Ordered, func() { + kyma := NewTestKyma("kyma") + BeforeAll(func() { + Eventually(CreateCR). + WithContext(ctx). + WithArguments(kcpClient, kyma).Should(Succeed()) + }) + + It("When enable module with channel and version, expect module status in Error state", func() { + module := NewTestModuleWithChannelVersion("test", v1beta2.DefaultChannel, "1.0.0") + Eventually(givenKymaWithModule, Timeout, Interval). + WithArguments(kyma, module).Should(Succeed()) + Eventually(expectKymaStatusModules(ctx, kyma, module.Name, shared.StateError), Timeout, + Interval).Should(Succeed()) + }) + It("When enable module with none channel, expect module status become error", func() { + module := NewTestModuleWithChannelVersion("test", string(shared.NoneChannel), "") + Eventually(givenKymaWithModule, Timeout, Interval). + WithArguments(kyma, module).Should(Succeed()) + Eventually(expectKymaStatusModules(ctx, kyma, module.Name, shared.StateError), Timeout, + Interval).Should(Succeed()) + }) +}) + +func givenKymaWithModule(kyma *v1beta2.Kyma, module v1beta2.Module) error { + if err := EnableModule(ctx, kcpClient, kyma.Name, kyma.Namespace, module); err != nil { + return err + } + Eventually(SyncKyma, Timeout, Interval). + WithContext(ctx).WithArguments(kcpClient, kyma).Should(Succeed()) + DeployModuleTemplates(ctx, kcpClient, kyma) + return nil +} diff --git a/tests/integration/controller/kyma/kyma_module_version_test.go b/tests/integration/controller/kyma/kyma_module_version_test.go new file mode 100644 index 0000000000..ccdd899dd9 --- /dev/null +++ b/tests/integration/controller/kyma/kyma_module_version_test.go @@ -0,0 +1,39 @@ +package kyma_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + + . "github.com/kyma-project/lifecycle-manager/pkg/testutils" +) + +var _ = Describe("Given invalid module version which is rejected by CRD validation rules", func() { + DescribeTable( + "Test enable module", func(givenCondition func() error) { + Eventually(givenCondition, Timeout, Interval).Should(Succeed()) + }, + + Entry( + "invalid semantic version", + givenKymaWithInvalidModuleVersion("20240101"), + ), + Entry( + "invalid semantic version", + givenKymaWithInvalidModuleVersion("1.0.0.abc"), + ), + ) +}) + +func givenKymaWithInvalidModuleVersion(version string) func() error { + return func() error { + kyma := NewTestKyma("kyma") + kyma.Spec.Channel = v1beta2.DefaultChannel + module := NewTestModuleWithChannelVersion("test", v1beta2.DefaultChannel, version) + kyma.Spec.Modules = append( + kyma.Spec.Modules, module) + err := kcpClient.Create(ctx, kyma) + return ignoreInvalidError(err) + } +} diff --git a/tests/integration/controller/kyma/manifest_test.go b/tests/integration/controller/kyma/manifest_test.go index 1c6bc56ffa..11f339d8e5 100644 --- a/tests/integration/controller/kyma/manifest_test.go +++ b/tests/integration/controller/kyma/manifest_test.go @@ -24,9 +24,10 @@ import ( "github.com/kyma-project/lifecycle-manager/pkg/templatelookup" "github.com/kyma-project/lifecycle-manager/pkg/testutils/builder" - . "github.com/kyma-project/lifecycle-manager/pkg/testutils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + + . "github.com/kyma-project/lifecycle-manager/pkg/testutils" ) const ( @@ -95,7 +96,8 @@ var _ = Describe("Update Manifest CR", Ordered, func() { By("Update Module Template spec") var moduleTemplateFromFile v1beta2.ModuleTemplate - builder.ReadComponentDescriptorFromFile("operator_v1beta2_moduletemplate_kcp-module_updated.yaml", &moduleTemplateFromFile) + builder.ReadComponentDescriptorFromFile("operator_v1beta2_moduletemplate_kcp-module_updated.yaml", + &moduleTemplateFromFile) moduleTemplateInCluster := &v1beta2.ModuleTemplate{} err := kcpClient.Get(ctx, client.ObjectKey{ @@ -332,10 +334,10 @@ var _ = Describe("Test Reconciliation Skip label for Manifest", Ordered, func() var _ = Describe("Modules can only be referenced via module name", Ordered, func() { kyma := NewTestKyma("random-kyma") - moduleReferencedWithLabel := NewTestModuleWithFixName("random-module", v1beta2.DefaultChannel) - moduleReferencedWithNamespacedName := NewTestModuleWithFixName( + moduleReferencedWithLabel := NewTestModule("random-module", v1beta2.DefaultChannel) + moduleReferencedWithNamespacedName := NewTestModule( v1beta2.DefaultChannel+shared.Separator+"random-module", v1beta2.DefaultChannel) - moduleReferencedWithFQDN := NewTestModuleWithFixName("kyma-project.io/module/"+"random-module", v1beta2.DefaultChannel) + moduleReferencedWithFQDN := NewTestModule("kyma-project.io/module/"+"random-module", v1beta2.DefaultChannel) kyma.Spec.Modules = append(kyma.Spec.Modules, moduleReferencedWithLabel) RegisterDefaultLifecycleForKyma(kyma) diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index f57b0b809c..7835469546 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -1,10 +1,10 @@ packages: - internal/crd: 92 - internal/descriptor/cache: 93 - internal/descriptor/provider: 66 + internal/crd: 92.1 + internal/descriptor/cache: 92.3 + internal/descriptor/provider: 66.7 internal/event: 100 internal/manifest/filemutex: 100 - internal/istio: 63 - internal/pkg/resources: 85 - internal/remote: 5 - pkg/templatelookup: 63 + internal/istio: 63.5 + internal/pkg/resources: 91.8 + internal/remote: 6.6 + pkg/templatelookup: 63 \ No newline at end of file