Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add mandatory-module label to mandatory ModuleTemplate #2033

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 59 additions & 2 deletions internal/controller/kyma/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, fmt.Errorf("KymaController: %w", err)
}

if err := r.UpdateModuleTemplatesIfNeeded(ctx); err != nil {
return ctrl.Result{}, fmt.Errorf("KymaController: %w", err)
}

status.InitConditions(kyma, r.SyncKymaEnabled(kyma), r.WatcherEnabled(kyma))

if kyma.SkipReconciliation() {
Expand Down Expand Up @@ -342,6 +346,7 @@ func (r *Reconciler) handleProcessingState(ctx context.Context, kyma *v1beta2.Ky
}
return nil
})

if r.SyncKymaEnabled(kyma) {
errGroup.Go(func() error {
if err := r.syncModuleCatalog(ctx, kyma); err != nil {
Expand Down Expand Up @@ -512,7 +517,8 @@ func (r *Reconciler) reconcileManifests(ctx context.Context, kyma *v1beta2.Kyma)
return nil
}

func (r *Reconciler) syncModuleCatalog(ctx context.Context, kyma *v1beta2.Kyma) error {
func (r *Reconciler) syncModuleCatalog(ctx context.Context, kyma *v1beta2.Kyma,
) error {
moduleTemplateList := &v1beta2.ModuleTemplateList{}
if err := r.List(ctx, moduleTemplateList, &client.ListOptions{}); err != nil {
return fmt.Errorf("could not aggregate module templates for module catalog sync: %w", err)
Expand All @@ -531,7 +537,8 @@ func (r *Reconciler) syncModuleCatalog(ctx context.Context, kyma *v1beta2.Kyma)
}

remoteCatalog := remote.NewRemoteCatalogFromKyma(r.Client, r.SkrContextFactory, r.RemoteSyncNamespace)
if err := remoteCatalog.Sync(ctx, kyma.GetNamespacedName(), modulesToSync, moduleReleaseMetaList.Items); err != nil {
if err := remoteCatalog.Sync(ctx, kyma.GetNamespacedName(), modulesToSync,
moduleReleaseMetaList.Items); err != nil {
return err
}

Expand Down Expand Up @@ -616,3 +623,53 @@ func (r *Reconciler) SyncKymaEnabled(kyma *v1beta2.Kyma) bool {
func (r *Reconciler) IsKymaManaged() bool {
return r.IsManagedKyma
}

func (r *Reconciler) GetModuleTemplateList(ctx context.Context) (*v1beta2.ModuleTemplateList, error) {
moduleTemplateList := &v1beta2.ModuleTemplateList{}
if err := r.List(ctx, moduleTemplateList, &client.ListOptions{}); err != nil {
return nil, fmt.Errorf("could not aggregate module templates for module catalog sync: %w", err)
}

return moduleTemplateList, nil
}

func (r *Reconciler) UpdateModuleTemplatesIfNeeded(ctx context.Context) error {
moduleTemplateList, err := r.GetModuleTemplateList(ctx)
if err != nil {
return err
}

for _, mt := range moduleTemplateList.Items {
if needUpdateForMandatoryModuleLabel(mt) {
if err = r.Update(ctx, &mt); err != nil {
return fmt.Errorf("failed to update ModuleTemplate, %w", err)
}
}
}

return nil
}

func needUpdateForMandatoryModuleLabel(moduleTemplate v1beta2.ModuleTemplate) bool {
if moduleTemplate.Labels == nil {
moduleTemplate.Labels = make(map[string]string)
}

if moduleTemplate.Spec.Mandatory {
if moduleTemplate.Labels[shared.IsMandatoryModule] == shared.EnableLabelValue {
return false
}

moduleTemplate.Labels[shared.IsMandatoryModule] = shared.EnableLabelValue
return true
}

if !moduleTemplate.Spec.Mandatory {
if moduleTemplate.Labels[shared.IsMandatoryModule] == shared.EnableLabelValue {
delete(moduleTemplate.Labels, shared.IsMandatoryModule)
return true
}
}

return false
}
14 changes: 9 additions & 5 deletions pkg/templatelookup/mandatory.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,27 @@ import (
"context"
"fmt"

k8slabels "k8s.io/apimachinery/pkg/labels"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/kyma-project/lifecycle-manager/api/shared"
"github.com/kyma-project/lifecycle-manager/api/v1beta2"
)

// GetMandatory returns ModuleTemplates TOs (Transfer Objects) which are marked are mandatory modules.
func GetMandatory(ctx context.Context, kymaClient client.Reader) (ModuleTemplatesByModuleName,
error,
) {
moduleTemplateList := &v1beta2.ModuleTemplateList{}
if err := kymaClient.List(ctx, moduleTemplateList); err != nil {
return nil, fmt.Errorf("could not list ModuleTemplates: %w", err)
mandatoryModuleTemplateList := &v1beta2.ModuleTemplateList{}
labelSelector := k8slabels.SelectorFromSet(k8slabels.Set{shared.IsMandatoryModule: shared.EnableLabelValue})
if err := kymaClient.List(ctx, mandatoryModuleTemplateList,
&client.ListOptions{LabelSelector: labelSelector}); err != nil {
return nil, fmt.Errorf("could not list mandatory ModuleTemplates: %w", err)
Comment on lines +18 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could also add a field selector client.MatchingFields{"metadata.deletionTimestamp": ""}. Then we could get rid of filtering in the for loop bellow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I am unable to use this, because if I use the fieldselector I will have to provide the field name and value

}

mandatoryModules := make(map[string]*ModuleTemplateInfo)
for _, moduleTemplate := range moduleTemplateList.Items {
if moduleTemplate.Spec.Mandatory && moduleTemplate.DeletionTimestamp.IsZero() {
for _, moduleTemplate := range mandatoryModuleTemplateList.Items {
if moduleTemplate.DeletionTimestamp.IsZero() {
mandatoryModules[moduleTemplate.Name] = &ModuleTemplateInfo{
ModuleTemplate: &moduleTemplate,
Err: nil,
Expand Down
3 changes: 3 additions & 0 deletions pkg/testutils/builder/moduletemplate.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ func NewModuleTemplateBuilder() ModuleTemplateBuilder {
ObjectMeta: apimetav1.ObjectMeta{
Name: random.Name(),
Namespace: apimetav1.NamespaceDefault,
Labels: map[string]string{
shared.IsMandatoryModule: shared.EnableLabelValue,
},
Comment on lines +37 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused by this change. Isn't this applying the mandatory module label to all tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but that's for the integration tests in which some don't have the kyma controller initialized. also, there is another followup issue to this to introduce the label using modulectl create command, so anyways the label should be there before the kyma controller

},
Spec: v1beta2.ModuleTemplateSpec{
Data: data,
Expand Down
26 changes: 26 additions & 0 deletions pkg/testutils/moduletemplate.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"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/internal/descriptor/provider"
"github.com/kyma-project/lifecycle-manager/pkg/templatelookup"
Expand Down Expand Up @@ -79,6 +80,31 @@ func UpdateModuleTemplateSpec(ctx context.Context,
return nil
}

func MandatoryModuleTemplateHasExpectedLabel(ctx context.Context, clnt client.Client, moduleName, key, value string,
) error {
mandatoryModuleTemplates, err := templatelookup.GetMandatory(ctx, clnt)
if err != nil {
return err
}

var moduleTemplate *v1beta2.ModuleTemplate
for _, moduleTemplateInfo := range mandatoryModuleTemplates {
if moduleTemplateInfo.ModuleTemplate.Labels[shared.ModuleName] == moduleName {
moduleTemplate = moduleTemplateInfo.ModuleTemplate
break
}
}

if moduleTemplate == nil {
return fmt.Errorf("module template not found, %s", moduleName)
}

if moduleTemplate.Labels[key] != value {
return fmt.Errorf("label %s:%s not found", key, value)
}
return nil
}

Comment on lines +83 to +107
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with this as is, but one idea to simplify may be to use the lookup by label already. I.e., get all ModuleTemplates with operator.kyma-project.io/mandatory-module: true, assert that the len of returned ModuleTemplates is 1 and the entry has the expected module name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the GetMandatory already uses the label for the lookup

func DeleteModuleTemplate(ctx context.Context,
clnt client.Client, module v1beta2.Module, kymaChannel string, namespace string,
) error {
Expand Down
8 changes: 8 additions & 0 deletions tests/e2e/mandatory_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ var _ = Describe("Mandatory Module Installation and Deletion", Ordered, func() {
WithArguments(kyma.GetName(), kyma.GetNamespace(), kcpClient, shared.StateReady).
Should(Succeed())
})
By("And the Mandatory ModuleTemplate has the correct mandatory-module label", func() {
Eventually(MandatoryModuleTemplateHasExpectedLabel).
WithContext(ctx).
WithArguments(kcpClient, "template-operator", shared.IsMandatoryModule,
shared.EnableLabelValue).
Should(Succeed())
})

By("And the mandatory ModuleTemplate is not synchronised to the SKR cluster", func() {
Consistently(CheckIfExists).
WithContext(ctx).
Expand Down
2 changes: 1 addition & 1 deletion unit-test-coverage.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@ packages:
internal/pkg/resources: 91.7
internal/remote: 13.2
internal/util/collections: 86
pkg/templatelookup: 77.7
pkg/templatelookup: 77.3
Loading