From 03072e82b687e17467a3d04fab2c9ffe9a32f931 Mon Sep 17 00:00:00 2001 From: zerospiel Date: Mon, 28 Oct 2024 18:11:15 +0100 Subject: [PATCH] Validate mgmt providers removal * forbid Management .spec.providers items removal if a ManagedCluster utilizes one of the removed providers * amends to CRDs descriptions Closes #554 --- .golangci.yml | 2 +- api/v1alpha1/clustertemplate_types.go | 6 +- api/v1alpha1/management_types.go | 3 +- api/v1alpha1/providertemplate_types.go | 6 +- .../controller/managedcluster_controller.go | 17 +-- internal/controller/template_controller.go | 3 +- .../webhook/managedcluster_webhook_test.go | 36 ++--- internal/webhook/management_webhook.go | 125 ++++++++++++++--- internal/webhook/management_webhook_test.go | 132 ++++++++++++++++-- .../hmc.mirantis.com_clustertemplates.yaml | 7 +- .../crds/hmc.mirantis.com_managements.yaml | 4 +- .../hmc.mirantis.com_providertemplates.yaml | 7 +- test/objects/managedcluster/managedcluster.go | 2 +- test/objects/management/management.go | 6 +- test/objects/template/template.go | 22 +-- 15 files changed, 268 insertions(+), 110 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index ae4c816ea..3a8d88e12 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -18,7 +18,7 @@ issues: - text: "Unhandled error in call to function fmt.Print*" linters: - revive - - path: cmd/main.go + - path: (cmd/main\.go)|(_test\.go) linters: - maintidx - path: test/ diff --git a/api/v1alpha1/clustertemplate_types.go b/api/v1alpha1/clustertemplate_types.go index 9d7c16b0f..94af11e05 100644 --- a/api/v1alpha1/clustertemplate_types.go +++ b/api/v1alpha1/clustertemplate_types.go @@ -40,9 +40,8 @@ type ClusterTemplateSpec struct { ProviderContracts CompatibilityContracts `json:"providerContracts,omitempty"` // Kubernetes exact version in the SemVer format provided by this ClusterTemplate. KubernetesVersion string `json:"k8sVersion,omitempty"` - // Providers represent required CAPI providers with supported contract versions. + // Providers represent required CAPI providers. // Should be set if not present in the Helm chart metadata. - // Compatibility attributes are optional to be defined. Providers Providers `json:"providers,omitempty"` } @@ -57,8 +56,7 @@ type ClusterTemplateStatus struct { ProviderContracts CompatibilityContracts `json:"providerContracts,omitempty"` // Kubernetes exact version in the SemVer format provided by this ClusterTemplate. KubernetesVersion string `json:"k8sVersion,omitempty"` - // Providers represent required CAPI providers with supported contract versions - // if the latter has been given. + // Providers represent required CAPI providers. Providers Providers `json:"providers,omitempty"` TemplateStatusCommon `json:",inline"` diff --git a/api/v1alpha1/management_types.go b/api/v1alpha1/management_types.go index 90b801606..40844a3ed 100644 --- a/api/v1alpha1/management_types.go +++ b/api/v1alpha1/management_types.go @@ -98,8 +98,7 @@ type ManagementStatus struct { Components map[string]ComponentStatus `json:"components,omitempty"` // Release indicates the current Release object. Release string `json:"release,omitempty"` - // AvailableProviders holds all CAPI providers available along with - // their supported contract versions, if specified in ProviderTemplates, on the Management cluster. + // AvailableProviders holds all available CAPI providers. AvailableProviders Providers `json:"availableProviders,omitempty"` // ObservedGeneration is the last observed generation. ObservedGeneration int64 `json:"observedGeneration,omitempty"` diff --git a/api/v1alpha1/providertemplate_types.go b/api/v1alpha1/providertemplate_types.go index a9982f674..909a134ed 100644 --- a/api/v1alpha1/providertemplate_types.go +++ b/api/v1alpha1/providertemplate_types.go @@ -27,17 +27,15 @@ const ProviderTemplateKind = "ProviderTemplate" type ProviderTemplateSpec struct { Helm HelmSpec `json:"helm,omitempty"` CAPIContracts CompatibilityContracts `json:"capiContracts,omitempty"` - // Providers represent exposed CAPI providers with supported contract versions. + // Providers represent exposed CAPI providers. // Should be set if not present in the Helm chart metadata. - // Supported contract versions are optional to be defined. Providers Providers `json:"providers,omitempty"` } // ProviderTemplateStatus defines the observed state of ProviderTemplate type ProviderTemplateStatus struct { CAPIContracts CompatibilityContracts `json:"capiContracts,omitempty"` - // Providers represent exposed CAPI providers with supported contract versions - // if the latter has been given. + // Providers represent exposed CAPI providers. Providers Providers `json:"providers,omitempty"` TemplateStatusCommon `json:",inline"` diff --git a/internal/controller/managedcluster_controller.go b/internal/controller/managedcluster_controller.go index ec5fe2a4d..b55333c43 100644 --- a/internal/controller/managedcluster_controller.go +++ b/internal/controller/managedcluster_controller.go @@ -46,7 +46,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" - "sigs.k8s.io/controller-runtime/pkg/reconcile" hmc "github.com/Mirantis/hmc/api/v1alpha1" "github.com/Mirantis/hmc/internal/credspropagation" @@ -781,20 +780,12 @@ func (r *ManagedClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { For(&hmc.ManagedCluster{}). Watches(&hcv2.HelmRelease{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) []ctrl.Request { - managedCluster := hmc.ManagedCluster{} - managedClusterRef := client.ObjectKey{ - Namespace: o.GetNamespace(), - Name: o.GetName(), - } - err := r.Client.Get(ctx, managedClusterRef, &managedCluster) - if err != nil { + managedClusterRef := client.ObjectKeyFromObject(o) + if err := r.Client.Get(ctx, managedClusterRef, &hmc.ManagedCluster{}); err != nil { return []ctrl.Request{} } - return []reconcile.Request{ - { - NamespacedName: managedClusterRef, - }, - } + + return []ctrl.Request{{NamespacedName: managedClusterRef}} }), ). Watches(&hmc.ClusterTemplateChain{}, diff --git a/internal/controller/template_controller.go b/internal/controller/template_controller.go index ebe3d86be..803fafb12 100644 --- a/internal/controller/template_controller.go +++ b/internal/controller/template_controller.go @@ -342,8 +342,7 @@ func (r *ClusterTemplateReconciler) validateCompatibilityAttrs(ctx context.Conte exposedProviders, requiredProviders := management.Status.AvailableProviders, template.Status.Providers l := ctrl.LoggerFrom(ctx) - l.V(1).Info("providers to check", "exposed", exposedProviders, "required", requiredProviders, - "exposed_capi_contract_versions", management.Status.CAPIContracts, "required_provider_contract_versions", template.Status.ProviderContracts) + l.V(1).Info("providers to check", "exposed", exposedProviders, "required", requiredProviders) var ( merr error diff --git a/internal/webhook/managedcluster_webhook_test.go b/internal/webhook/managedcluster_webhook_test.go index 4d8cc84a5..27460336e 100644 --- a/internal/webhook/managedcluster_webhook_test.go +++ b/internal/webhook/managedcluster_webhook_test.go @@ -127,11 +127,11 @@ func TestManagedClusterValidateCreate(t *testing.T) { cred, template.NewClusterTemplate( template.WithName(testTemplateName), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-aws", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), }, @@ -170,11 +170,11 @@ func TestManagedClusterValidateCreate(t *testing.T) { mgmt, template.NewClusterTemplate( template.WithName(testTemplateName), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-aws", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), }, @@ -199,11 +199,11 @@ func TestManagedClusterValidateCreate(t *testing.T) { ), template.NewClusterTemplate( template.WithName(testTemplateName), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-aws", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), }, @@ -226,11 +226,11 @@ func TestManagedClusterValidateCreate(t *testing.T) { ), template.NewClusterTemplate( template.WithName(testTemplateName), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-azure", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), ), }, @@ -313,20 +313,20 @@ func TestManagedClusterValidateUpdate(t *testing.T) { template.NewClusterTemplate( template.WithName(testTemplateName), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-aws", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), ), template.NewClusterTemplate( template.WithName(upgradeTargetTemplateName), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-aws", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), ), }, warnings: admission.Warnings{fmt.Sprintf("Cluster can't be upgraded from %s to %s. This upgrade sequence is not allowed", testTemplateName, upgradeTargetTemplateName)}, @@ -348,20 +348,20 @@ func TestManagedClusterValidateUpdate(t *testing.T) { template.NewClusterTemplate( template.WithName(testTemplateName), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-aws", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), ), template.NewClusterTemplate( template.WithName(newTemplateName), template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-aws", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), ), }, }, @@ -386,11 +386,11 @@ func TestManagedClusterValidateUpdate(t *testing.T) { Valid: false, ValidationError: "validation error example", }), - template.WithProvidersStatus(v1alpha1.Providers{ + template.WithProvidersStatus( "infrastructure-aws", "control-plane-k0smotron", "bootstrap-k0smotron", - }), + ), ), }, }, diff --git a/internal/webhook/management_webhook.go b/internal/webhook/management_webhook.go index 78f349c82..216f5365e 100644 --- a/internal/webhook/management_webhook.go +++ b/internal/webhook/management_webhook.go @@ -18,9 +18,12 @@ import ( "context" "errors" "fmt" + "slices" + "strings" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -55,21 +58,109 @@ func (*ManagementValidator) ValidateCreate(_ context.Context, _ runtime.Object) } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (v *ManagementValidator) ValidateUpdate(ctx context.Context, _, newObj runtime.Object) (admission.Warnings, error) { +func (v *ManagementValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { const invalidMgmtMsg = "the Management is invalid" - mgmt, ok := newObj.(*hmcv1alpha1.Management) + newMgmt, ok := newObj.(*hmcv1alpha1.Management) if !ok { return nil, apierrors.NewBadRequest(fmt.Sprintf("expected Management but got a %T", newObj)) } + oldMgmt, ok := oldObj.(*hmcv1alpha1.Management) + if !ok { + return nil, apierrors.NewBadRequest(fmt.Sprintf("expected Management but got a %T", oldObj)) + } + + if err := checkComponentsRemoval(ctx, v.Client, oldMgmt, newMgmt); err != nil { + return admission.Warnings{"Some of the providers cannot be removed"}, + apierrors.NewInvalid(newMgmt.GroupVersionKind().GroupKind(), newMgmt.Name, field.ErrorList{ + field.Forbidden(field.NewPath("spec", "providers"), err.Error()), + }) + } + + incompatibleContracts, err := getIncompatibleContracts(ctx, v, newMgmt) + if err != nil { + return nil, fmt.Errorf("%s: %v", invalidMgmtMsg, err) + } + + if incompatibleContracts != "" { + return admission.Warnings{"The Management object has incompatible CAPI contract versions in ProviderTemplates"}, fmt.Errorf("%s: %s", invalidMgmtMsg, incompatibleContracts) + } + + return nil, nil +} + +func checkComponentsRemoval(ctx context.Context, cl client.Client, oldMgmt, newMgmt *hmcv1alpha1.Management) error { + removedComponentNames := []string{} + for _, oldComp := range oldMgmt.Spec.Providers { + if !slices.ContainsFunc(newMgmt.Spec.Providers, func(newComp hmcv1alpha1.Provider) bool { return oldComp.Name == newComp.Name }) { + removedComponentNames = append(removedComponentNames, oldComp.Name) + } + } + + if len(removedComponentNames) == 0 { + return nil + } + + removedProvidersSet := make(map[string]struct{}) + for _, m := range removedComponentNames { + c, ok := oldMgmt.Status.Components[m] + if !ok { + continue + } + + // it does not matter if component has been successfully installed + prTpl := new(hmcv1alpha1.ProviderTemplate) + if err := cl.Get(ctx, client.ObjectKey{Name: c.Template}, prTpl); err != nil { + return fmt.Errorf("failed to get ProviderTemplate %s: %w", c.Template, err) + } + + for _, pn := range prTpl.Status.Providers { + removedProvidersSet[pn] = struct{}{} + } + } + + if len(removedProvidersSet) == 0 { // sanity + return nil + } + + mcls := new(hmcv1alpha1.ManagedClusterList) + if err := cl.List(ctx, mcls); err != nil { + return fmt.Errorf("failed to list ManagedClusters: %w", err) + } + + if len(mcls.Items) == 0 { + return nil + } + + // if at least one required provider for an mcl is being held by + // a provider template referenced by removed component, then we cannot allow to remove the latter + for _, mcl := range mcls.Items { + clTpl := new(hmcv1alpha1.ClusterTemplate) + oref := client.ObjectKey{Namespace: mcl.Namespace, Name: mcl.Spec.Template} + if err := cl.Get(ctx, oref, clTpl); err != nil { + return fmt.Errorf("failed to get ClusterTemplate %s: %w", oref, err) + } + + for _, pn := range clTpl.Status.Providers { + if _, ok := removedProvidersSet[pn]; ok { + return fmt.Errorf("provider %s is required by the ManagedCluster %s and cannot be removed from the Management %s", pn, client.ObjectKeyFromObject(&mcl), newMgmt.Name) + } + } + } + + return nil +} + +func getIncompatibleContracts(ctx context.Context, cl client.Client, mgmt *hmcv1alpha1.Management) (string, error) { release := new(hmcv1alpha1.Release) - if err := v.Get(ctx, client.ObjectKey{Name: mgmt.Spec.Release}, release); err != nil { + if err := cl.Get(ctx, client.ObjectKey{Name: mgmt.Spec.Release}, release); err != nil { // TODO: probably we do not want this skip if extra checks will be introduced if apierrors.IsNotFound(err) && (mgmt.Spec.Core == nil || mgmt.Spec.Core.CAPI.Template == "") { - return nil, nil // nothing to do + return "", nil // nothing to do } - return nil, fmt.Errorf("failed to get Release %s: %w", mgmt.Spec.Release, err) + + return "", fmt.Errorf("failed to get Release %s: %w", mgmt.Spec.Release, err) } capiTplName := release.Spec.CAPI.Template @@ -78,19 +169,19 @@ func (v *ManagementValidator) ValidateUpdate(ctx context.Context, _, newObj runt } capiTpl := new(hmcv1alpha1.ProviderTemplate) - if err := v.Get(ctx, client.ObjectKey{Name: capiTplName}, capiTpl); err != nil { - return nil, fmt.Errorf("failed to get ProviderTemplate %s: %w", capiTplName, err) + if err := cl.Get(ctx, client.ObjectKey{Name: capiTplName}, capiTpl); err != nil { + return "", fmt.Errorf("failed to get ProviderTemplate %s: %w", capiTplName, err) } if len(capiTpl.Status.CAPIContracts) == 0 { - return nil, nil // nothing to validate against + return "", nil // nothing to validate against } if !capiTpl.Status.Valid { - return nil, fmt.Errorf("%s: not valid ProviderTemplate %s", invalidMgmtMsg, capiTpl.Name) + return "", fmt.Errorf("not valid ProviderTemplate %s", capiTpl.Name) } - var wrongVersions error + incompatibleContracts := strings.Builder{} for _, p := range mgmt.Spec.Providers { tplName := p.Template if tplName == "" { @@ -102,8 +193,8 @@ func (v *ManagementValidator) ValidateUpdate(ctx context.Context, _, newObj runt } pTpl := new(hmcv1alpha1.ProviderTemplate) - if err := v.Get(ctx, client.ObjectKey{Name: tplName}, pTpl); err != nil { - return nil, fmt.Errorf("failed to get ProviderTemplate %s: %w", tplName, err) + if err := cl.Get(ctx, client.ObjectKey{Name: tplName}, pTpl); err != nil { + return "", fmt.Errorf("failed to get ProviderTemplate %s: %w", tplName, err) } if len(pTpl.Status.CAPIContracts) == 0 { @@ -111,21 +202,17 @@ func (v *ManagementValidator) ValidateUpdate(ctx context.Context, _, newObj runt } if !pTpl.Status.Valid { - return nil, fmt.Errorf("%s: not valid ProviderTemplate %s", invalidMgmtMsg, tplName) + return "", fmt.Errorf("not valid ProviderTemplate %s", tplName) } for capi := range capiTpl.Status.CAPIContracts { if _, ok := pTpl.Status.CAPIContracts[capi]; !ok { - wrongVersions = errors.Join(wrongVersions, fmt.Errorf("core CAPI contract version %s does not support ProviderTemplate %s", capi, pTpl.Name)) + _, _ = incompatibleContracts.WriteString(fmt.Sprintf("core CAPI contract version %s does not support ProviderTemplate %s, ", capi, pTpl.Name)) } } } - if wrongVersions != nil { - return admission.Warnings{"The Management object has incompatible CAPI contract versions in ProviderTemplates"}, fmt.Errorf("%s: %s", invalidMgmtMsg, wrongVersions) - } - - return nil, nil + return strings.TrimSuffix(incompatibleContracts.String(), ", "), nil } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. diff --git a/internal/webhook/management_webhook_test.go b/internal/webhook/management_webhook_test.go index 98142ed96..58bc4ce3d 100644 --- a/internal/webhook/management_webhook_test.go +++ b/internal/webhook/management_webhook_test.go @@ -42,36 +42,132 @@ func TestManagementValidateUpdate(t *testing.T) { someContractVersion = "v1alpha4_v1beta1" capiVersion = "v1beta1" capiVersionOther = "v1alpha3" + + infraAWSProvider = "infrastructure-aws" + infraOtherProvider = "infrastructure-other-provider" ) validStatus := v1alpha1.TemplateValidationStatus{Valid: true} - providerAwsDefaultTpl := v1alpha1.Provider{ - Name: "infrastructure-aws", + componentAwsDefaultTpl := v1alpha1.Provider{ + Name: v1alpha1.ProviderCAPAName, Component: v1alpha1.Component{ Template: template.DefaultName, }, } + compStatusAws := map[string]v1alpha1.ComponentStatus{ + v1alpha1.ProviderCAPAName: { + Template: template.DefaultName, + Success: true, + }, + } + tests := []struct { name string + oldMgmt *v1alpha1.Management management *v1alpha1.Management existingObjects []runtime.Object err string warnings admission.Warnings }{ + { + name: "no providers removed, should succeed", + oldMgmt: management.NewManagement(management.WithProviders(componentAwsDefaultTpl)), + management: management.NewManagement(management.WithProviders(componentAwsDefaultTpl)), + }, + { + name: "removed provider does not have related providertemplate, should fail", + oldMgmt: management.NewManagement( + management.WithProviders(componentAwsDefaultTpl), + management.WithComponentsStatus(compStatusAws), + ), + management: management.NewManagement(management.WithProviders()), + warnings: admission.Warnings{"Some of the providers cannot be removed"}, + err: fmt.Sprintf(`Management "%s" is invalid: spec.providers: Forbidden: failed to get ProviderTemplate %s: providertemplates.hmc.mirantis.com "%s" not found`, management.DefaultName, template.DefaultName, template.DefaultName), + }, + { + name: "no managed clusters, should succeed", + oldMgmt: management.NewManagement( + management.WithProviders(componentAwsDefaultTpl), + management.WithComponentsStatus(compStatusAws), + ), + management: management.NewManagement(management.WithProviders()), + existingObjects: []runtime.Object{ + template.NewProviderTemplate(template.WithProvidersStatus(infraAWSProvider)), + }, + }, + { + name: "no clustertemplate present referenced in a managed cluster, should fail", + oldMgmt: management.NewManagement( + management.WithProviders(componentAwsDefaultTpl), + management.WithComponentsStatus(compStatusAws), + ), + management: management.NewManagement(management.WithProviders()), + existingObjects: []runtime.Object{ + template.NewProviderTemplate(template.WithProvidersStatus(infraAWSProvider)), + managedcluster.NewManagedCluster(managedcluster.WithClusterTemplate(template.DefaultName)), + }, + warnings: admission.Warnings{"Some of the providers cannot be removed"}, + err: fmt.Sprintf(`Management "%s" is invalid: spec.providers: Forbidden: failed to get ClusterTemplate %s/%s: clustertemplates.hmc.mirantis.com "%s" not found`, management.DefaultName, template.DefaultNamespace, template.DefaultName, template.DefaultName), + }, + { + name: "no clustertemplate present referenced in a managed cluster, should fail", + oldMgmt: management.NewManagement( + management.WithProviders(componentAwsDefaultTpl), + management.WithComponentsStatus(compStatusAws), + ), + management: management.NewManagement(management.WithProviders()), + existingObjects: []runtime.Object{ + template.NewProviderTemplate(template.WithProvidersStatus(infraAWSProvider)), + managedcluster.NewManagedCluster(managedcluster.WithClusterTemplate(template.DefaultName)), + }, + warnings: admission.Warnings{"Some of the providers cannot be removed"}, + err: fmt.Sprintf(`Management "%s" is invalid: spec.providers: Forbidden: failed to get ClusterTemplate %s/%s: clustertemplates.hmc.mirantis.com "%s" not found`, management.DefaultName, template.DefaultNamespace, template.DefaultName, template.DefaultName), + }, + { + name: "managed cluster uses the removed provider, should fail", + oldMgmt: management.NewManagement( + management.WithProviders(componentAwsDefaultTpl), + management.WithComponentsStatus(compStatusAws), + ), + management: management.NewManagement(management.WithProviders()), + existingObjects: []runtime.Object{ + template.NewProviderTemplate(template.WithProvidersStatus(infraAWSProvider)), + template.NewClusterTemplate(template.WithProvidersStatus(infraAWSProvider)), + managedcluster.NewManagedCluster(managedcluster.WithClusterTemplate(template.DefaultName)), + }, + warnings: admission.Warnings{"Some of the providers cannot be removed"}, + err: fmt.Sprintf(`Management "%s" is invalid: spec.providers: Forbidden: provider %s is required by the ManagedCluster %s/%s and cannot be removed from the Management %s`, management.DefaultName, infraAWSProvider, managedcluster.DefaultNamespace, managedcluster.DefaultName, management.DefaultName), + }, + { + name: "managed cluster does not use the removed provider, should succeed", + oldMgmt: management.NewManagement( + management.WithProviders(componentAwsDefaultTpl), + management.WithComponentsStatus(compStatusAws), + ), + management: management.NewManagement(management.WithProviders()), + existingObjects: []runtime.Object{ + template.NewProviderTemplate(template.WithProvidersStatus(infraAWSProvider)), + template.NewClusterTemplate(template.WithProvidersStatus(infraOtherProvider)), + managedcluster.NewManagedCluster(managedcluster.WithClusterTemplate(template.DefaultName)), + }, + }, { name: "no release and no core capi tpl set, should succeed", + oldMgmt: management.NewManagement(), management: management.NewManagement(), }, { name: "no capi providertemplate, should fail", + oldMgmt: management.NewManagement(), management: management.NewManagement(management.WithRelease(release.DefaultName)), existingObjects: []runtime.Object{release.New()}, - err: fmt.Sprintf(`failed to get ProviderTemplate %s: providertemplates.hmc.mirantis.com "%s" not found`, release.DefaultCAPITemplateName, release.DefaultCAPITemplateName), + err: fmt.Sprintf(`the Management is invalid: failed to get ProviderTemplate %s: providertemplates.hmc.mirantis.com "%s" not found`, release.DefaultCAPITemplateName, release.DefaultCAPITemplateName), }, { name: "capi providertemplate without capi version set, should succeed", + oldMgmt: management.NewManagement(), management: management.NewManagement(management.WithRelease(release.DefaultName)), existingObjects: []runtime.Object{ release.New(), @@ -80,6 +176,7 @@ func TestManagementValidateUpdate(t *testing.T) { }, { name: "capi providertemplate is not valid, should fail", + oldMgmt: management.NewManagement(), management: management.NewManagement(management.WithRelease(release.DefaultName)), existingObjects: []runtime.Object{ release.New(), @@ -91,10 +188,11 @@ func TestManagementValidateUpdate(t *testing.T) { err: "the Management is invalid: not valid ProviderTemplate " + release.DefaultCAPITemplateName, }, { - name: "no providertemplates that declared in mgmt spec.providers, should fail", + name: "no providertemplates that declared in mgmt spec.providers, should fail", + oldMgmt: management.NewManagement(), management: management.NewManagement( management.WithRelease(release.DefaultName), - management.WithProviders([]v1alpha1.Provider{providerAwsDefaultTpl}), + management.WithProviders(componentAwsDefaultTpl), ), existingObjects: []runtime.Object{ release.New(), @@ -104,13 +202,14 @@ func TestManagementValidateUpdate(t *testing.T) { template.WithValidationStatus(validStatus), ), }, - err: fmt.Sprintf(`failed to get ProviderTemplate %s: providertemplates.hmc.mirantis.com "%s" not found`, template.DefaultName, template.DefaultName), + err: fmt.Sprintf(`the Management is invalid: failed to get ProviderTemplate %s: providertemplates.hmc.mirantis.com "%s" not found`, template.DefaultName, template.DefaultName), }, { - name: "providertemplates without specified capi contracts, should succeed", + name: "providertemplates without specified capi contracts, should succeed", + oldMgmt: management.NewManagement(), management: management.NewManagement( management.WithRelease(release.DefaultName), - management.WithProviders([]v1alpha1.Provider{providerAwsDefaultTpl}), + management.WithProviders(componentAwsDefaultTpl), ), existingObjects: []runtime.Object{ release.New(), @@ -123,10 +222,11 @@ func TestManagementValidateUpdate(t *testing.T) { }, }, { - name: "providertemplates is not ready, should succeed", + name: "providertemplates is not ready, should succeed", + oldMgmt: management.NewManagement(), management: management.NewManagement( management.WithRelease(release.DefaultName), - management.WithProviders([]v1alpha1.Provider{providerAwsDefaultTpl}), + management.WithProviders(componentAwsDefaultTpl), ), existingObjects: []runtime.Object{ release.New(), @@ -142,10 +242,11 @@ func TestManagementValidateUpdate(t *testing.T) { err: "the Management is invalid: not valid ProviderTemplate " + template.DefaultName, }, { - name: "providertemplates do not match capi contracts, should fail", + name: "providertemplates do not match capi contracts, should fail", + oldMgmt: management.NewManagement(), management: management.NewManagement( management.WithRelease(release.DefaultName), - management.WithProviders([]v1alpha1.Provider{providerAwsDefaultTpl}), + management.WithProviders(componentAwsDefaultTpl), ), existingObjects: []runtime.Object{ release.New(), @@ -163,10 +264,11 @@ func TestManagementValidateUpdate(t *testing.T) { err: fmt.Sprintf("the Management is invalid: core CAPI contract version %s does not support ProviderTemplate %s", capiVersion, template.DefaultName), }, { - name: "providertemplates match capi contracts, should succeed", + name: "providertemplates match capi contracts, should succeed", + oldMgmt: management.NewManagement(), management: management.NewManagement( management.WithRelease(release.DefaultName), - management.WithProviders([]v1alpha1.Provider{providerAwsDefaultTpl}), + management.WithProviders(componentAwsDefaultTpl), ), existingObjects: []runtime.Object{ release.New(), @@ -188,7 +290,7 @@ func TestManagementValidateUpdate(t *testing.T) { c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build() validator := &ManagementValidator{Client: c} - warnings, err := validator.ValidateUpdate(ctx, nil, tt.management) + warnings, err := validator.ValidateUpdate(ctx, tt.oldMgmt, tt.management) if tt.err != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err).To(MatchError(tt.err)) diff --git a/templates/provider/hmc/templates/crds/hmc.mirantis.com_clustertemplates.yaml b/templates/provider/hmc/templates/crds/hmc.mirantis.com_clustertemplates.yaml index acb5ac0bf..94bfcbb20 100644 --- a/templates/provider/hmc/templates/crds/hmc.mirantis.com_clustertemplates.yaml +++ b/templates/provider/hmc/templates/crds/hmc.mirantis.com_clustertemplates.yaml @@ -120,9 +120,8 @@ spec: type: object providers: description: |- - Providers represent required CAPI providers with supported contract versions. + Providers represent required CAPI providers. Should be set if not present in the Helm chart metadata. - Compatibility attributes are optional to be defined. items: type: string type: array @@ -193,9 +192,7 @@ spec: [contract versions]: https://cluster-api.sigs.k8s.io/developer/providers/contracts type: object providers: - description: |- - Providers represent required CAPI providers with supported contract versions - if the latter has been given. + description: Providers represent required CAPI providers. items: type: string type: array diff --git a/templates/provider/hmc/templates/crds/hmc.mirantis.com_managements.yaml b/templates/provider/hmc/templates/crds/hmc.mirantis.com_managements.yaml index af53adbdc..1dc941cca 100644 --- a/templates/provider/hmc/templates/crds/hmc.mirantis.com_managements.yaml +++ b/templates/provider/hmc/templates/crds/hmc.mirantis.com_managements.yaml @@ -112,9 +112,7 @@ spec: description: ManagementStatus defines the observed state of Management properties: availableProviders: - description: |- - AvailableProviders holds all CAPI providers available along with - their supported contract versions, if specified in ProviderTemplates, on the Management cluster. + description: AvailableProviders holds all available CAPI providers. items: type: string type: array diff --git a/templates/provider/hmc/templates/crds/hmc.mirantis.com_providertemplates.yaml b/templates/provider/hmc/templates/crds/hmc.mirantis.com_providertemplates.yaml index b6c8751ad..3157fb79c 100644 --- a/templates/provider/hmc/templates/crds/hmc.mirantis.com_providertemplates.yaml +++ b/templates/provider/hmc/templates/crds/hmc.mirantis.com_providertemplates.yaml @@ -116,9 +116,8 @@ spec: && has(self.chartRef)) providers: description: |- - Providers represent exposed CAPI providers with supported contract versions. + Providers represent exposed CAPI providers. Should be set if not present in the Helm chart metadata. - Supported contract versions are optional to be defined. items: type: string type: array @@ -183,9 +182,7 @@ spec: format: int64 type: integer providers: - description: |- - Providers represent exposed CAPI providers with supported contract versions - if the latter has been given. + description: Providers represent exposed CAPI providers. items: type: string type: array diff --git a/test/objects/managedcluster/managedcluster.go b/test/objects/managedcluster/managedcluster.go index 4bc2d5db1..7310cb49e 100644 --- a/test/objects/managedcluster/managedcluster.go +++ b/test/objects/managedcluster/managedcluster.go @@ -23,7 +23,7 @@ import ( const ( DefaultName = "managedcluster" - DefaultNamespace = "default" + DefaultNamespace = metav1.NamespaceDefault ) type Opt func(managedCluster *v1alpha1.ManagedCluster) diff --git a/test/objects/management/management.go b/test/objects/management/management.go index b120c6be5..785faa643 100644 --- a/test/objects/management/management.go +++ b/test/objects/management/management.go @@ -28,6 +28,10 @@ type Opt func(management *v1alpha1.Management) func NewManagement(opts ...Opt) *v1alpha1.Management { p := &v1alpha1.Management{ + TypeMeta: metav1.TypeMeta{ + Kind: "Management", + APIVersion: v1alpha1.GroupVersion.Version, + }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultName, Finalizers: []string{v1alpha1.ManagementFinalizer}, @@ -58,7 +62,7 @@ func WithCoreComponents(core *v1alpha1.Core) Opt { } } -func WithProviders(providers []v1alpha1.Provider) Opt { +func WithProviders(providers ...v1alpha1.Provider) Opt { return func(p *v1alpha1.Management) { p.Spec.Providers = providers } diff --git a/test/objects/template/template.go b/test/objects/template/template.go index 3d2b0e550..8518c861d 100644 --- a/test/objects/template/template.go +++ b/test/objects/template/template.go @@ -26,7 +26,7 @@ import ( const ( DefaultName = "template" - DefaultNamespace = "default" + DefaultNamespace = metav1.NamespaceDefault ) type ( @@ -140,27 +140,15 @@ func WithValidationStatus(validationStatus v1alpha1.TemplateValidationStatus) Op } } -func WithProvidersStatus(providers v1alpha1.Providers) Opt { +func WithProvidersStatus(providers ...string) Opt { return func(t Template) { switch v := t.(type) { case *v1alpha1.ClusterTemplate: - var ok bool - v.Status.Providers, ok = any(providers).(v1alpha1.Providers) - if !ok { - panic(fmt.Sprintf("unexpected type %T", providers)) - } + v.Status.Providers = providers case *v1alpha1.ProviderTemplate: - var ok bool - v.Status.Providers, ok = any(providers).(v1alpha1.Providers) - if !ok { - panic(fmt.Sprintf("unexpected type %T", providers)) - } + v.Status.Providers = providers case *v1alpha1.ServiceTemplate: - var ok bool - v.Status.Providers, ok = any(providers).(v1alpha1.Providers) - if !ok { - panic(fmt.Sprintf("unexpected type %T", providers)) - } + v.Status.Providers = providers } } }