From 07c08db85476cc7a91fa13d74d151408dbf9f72c Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Mon, 20 Jul 2026 19:22:44 +0300 Subject: [PATCH] fix(core): enforce chart uniqueness via lease claim The validating webhook could not reliably prevent two HelmClusterAddon objects from using the same repository/chart pair: admission requests are served concurrently and before the object is persisted, and the check read from the controller cache, so bursts of creates raced past it. Move the guarantee into the reconciler and delegate it to the API server's only atomic cross-object primitive, object-name uniqueness: each repo/chart pair maps to a single Lease name, claimed via ChartClaimService before any downstream resource is created. The first creator wins; a duplicate that loses surfaces a ChartClaimConflict condition and requeues, recovering once the owner releases the pair. Stale claims (deleted or repointed owner) are taken over. The claim is acquired before the finalizer so a loser accrues no finalizer, and released only after all downstream resources are gone. Drop uniqueness validation from the webhook's UPDATE path (and UPDATE from the manifest): with duplicates transiently present, validating updates would reject the very finalizer the reconciler needs to resolve the conflict. The webhook now only fast-rejects the obvious duplicate on CREATE. Signed-off-by: Ilya Drey --- api/v1alpha1/conditions.go | 1 + .../controller/helmclusteraddon/controller.go | 1 + .../reconcile/helmclusteraddon/reconciler.go | 44 +++ .../internal/services/chart_claim_service.go | 260 ++++++++++++++++++ .../internal/utils/name.go | 12 + .../webhook/helmclusteraddon/webhook.go | 10 +- .../validation-webhook.yaml | 2 +- tests/e2e/helmclusteraddon/chartclaim.go | 228 +++++++++++++++ 8 files changed, 555 insertions(+), 3 deletions(-) create mode 100644 images/operator-helm-controller/internal/services/chart_claim_service.go create mode 100644 tests/e2e/helmclusteraddon/chartclaim.go diff --git a/api/v1alpha1/conditions.go b/api/v1alpha1/conditions.go index ed5d37d..fd34bd3 100644 --- a/api/v1alpha1/conditions.go +++ b/api/v1alpha1/conditions.go @@ -34,6 +34,7 @@ const ( ReasonSuccess = "Success" ReasonFailed = "Failed" ReasonUninstallFailed = "UninstallFailed" + ReasonChartClaimConflict = "ChartClaimConflict" // HelmRelease error reasons ReasonReleaseFailed = "ReleaseFailed" diff --git a/images/operator-helm-controller/internal/controller/helmclusteraddon/controller.go b/images/operator-helm-controller/internal/controller/helmclusteraddon/controller.go index eb728f3..bec1053 100644 --- a/images/operator-helm-controller/internal/controller/helmclusteraddon/controller.go +++ b/images/operator-helm-controller/internal/controller/helmclusteraddon/controller.go @@ -45,6 +45,7 @@ func SetupWithManager(mgr ctrl.Manager) error { services.NewOCIRepoService(client, mgr.GetScheme(), helmv1alpha1.TargetNamespace), services.NewReleaseService(client, mgr.GetScheme(), helmv1alpha1.TargetNamespace), services.NewMaintenanceService(client, mgr.GetScheme(), helmv1alpha1.TargetNamespace), + services.NewClaimService(client, mgr.GetAPIReader(), helmv1alpha1.TargetNamespace), status.NewManager(client), ) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go index 8c5d10a..f2ecb26 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go @@ -46,12 +46,19 @@ import ( // whose deletion is stuck and stops emitting events. const internalResourceDeletionRequeueInterval = 30 * time.Second +// chartClaimConflictRequeueInterval bounds how often an addon that lost the claim +// on its repository/chart pair re-checks whether the owner has released it. There +// is no watch on the claim Lease, so this periodic requeue is what lets a duplicate +// recover once the conflicting addon is deleted or repointed at another chart. +const chartClaimConflictRequeueInterval = 30 * time.Second + func New( client client.Client, chartService *services.ChartService, ociRepositoryService *services.OCIRepoService, releaseService *services.ReleaseService, maintenanceService *services.MaintenanceService, + claimService *services.ClaimService, statusManager *status.Manager, ) *Reconciler { return &Reconciler{ @@ -60,6 +67,7 @@ func New( ociRepositoryService: ociRepositoryService, releaseService: releaseService, maintenanceService: maintenanceService, + claimService: claimService, statusManager: statusManager, } } @@ -71,6 +79,7 @@ type Reconciler struct { ociRepositoryService *services.OCIRepoService releaseService *services.ReleaseService maintenanceService *services.MaintenanceService + claimService *services.ClaimService statusManager *status.Manager } @@ -90,6 +99,29 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco return r.reconcileDelete(ctx, addon) } + // Claim the repository/chart pair before anything else, including adding the + // finalizer. The claim is the authoritative, race-free guard on uniqueness (the + // webhook only fast-rejects the obvious duplicate on CREATE and cannot stop + // concurrent creates from racing past it). It must run before the finalizer + // because a duplicate that loses the race must not accrue a finalizer it would + // otherwise have to clean up: it simply surfaces the conflict on its status and + // requeues, recovering on its own once the owner releases the pair. + acquired, holder, err := r.claimService.Acquire(ctx, addon) + if err != nil { + return reconcile.Result{}, fmt.Errorf("acquiring chart claim: %w", err) + } + if !acquired { + return reconcile.Result{RequeueAfter: chartClaimConflictRequeueInterval}, r.statusManager.Update( + ctx, addon, status.NoopStatusMutator, status.NoopStatusMapper, + services.ReleaseResult{Status: status.Failed( + addon, + helmv1alpha1.ReasonChartClaimConflict, + fmt.Sprintf("chart %q is already used by helmclusteraddon/%s", addon.Spec.Chart.HelmClusterAddonChartName, holder), + nil, + )}, + ) + } + if !controllerutil.ContainsFinalizer(addon, helmv1alpha1.FinalizerName) { controllerutil.AddFinalizer(addon, helmv1alpha1.FinalizerName) if err := r.Update(ctx, addon); err != nil { @@ -101,6 +133,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // would not trigger a follow-up reconcile. } + if err := r.claimService.ReleaseStale(ctx, addon); err != nil { + return reconcile.Result{}, fmt.Errorf("releasing stale chart claims: %w", err) + } + if r.maintenanceService.IsMaintenanceModeChangeRequired(addon) { maintenanceRes := r.maintenanceService.EnsureMaintenanceMode(ctx, addon) return reconcile.Result{}, r.statusManager.Update(ctx, addon, status.NoopStatusMutator, status.NoopStatusMapper, maintenanceRes, status.AsCondition(maintenanceRes, "Ready")) @@ -274,6 +310,14 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, addon *helmv1alpha1.He return r.awaitInternalResourceDeletion(ctx, addon, "internal repository", ociRepo) } + // Release the claim only once every downstream resource is gone: releasing it + // earlier would let another addon start reconciling the same chart while this + // one's release is still being uninstalled — exactly the collision the claim + // prevents. + if err := r.claimService.Release(ctx, addon); err != nil { + return reconcile.Result{}, fmt.Errorf("releasing chart claim: %w", err) + } + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { latestAddon := &helmv1alpha1.HelmClusterAddon{} if err := r.Get(ctx, client.ObjectKeyFromObject(addon), latestAddon); err != nil { diff --git a/images/operator-helm-controller/internal/services/chart_claim_service.go b/images/operator-helm-controller/internal/services/chart_claim_service.go new file mode 100644 index 0000000..aabe7ca --- /dev/null +++ b/images/operator-helm-controller/internal/services/chart_claim_service.go @@ -0,0 +1,260 @@ +/* +Copyright 2026 Flant JSC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package services + +import ( + "context" + "fmt" + + coordinationv1 "k8s.io/api/coordination/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/utils" +) + +// ClaimService enforces that at most one HelmClusterAddon uses a given +// repository/chart pair. A validating webhook cannot guarantee this: admission +// requests are served concurrently and before the object is persisted, so two +// simultaneous creates can both pass. Uniqueness is instead delegated to the API +// server's only atomic cross-object primitive — the uniqueness of an object name. +// Each repository/chart pair maps to a single Lease name; whoever creates that +// Lease first owns the pair, and every other addon reconciles into a conflict. +type ClaimService struct { + // reader reads from the API server directly (mgr.GetAPIReader()), bypassing the + // controller cache: an acquisition decision must never be made against stale data. + reader client.Reader + client client.Client + namespace string +} + +func NewClaimService(c client.Client, reader client.Reader, namespace string) *ClaimService { + return &ClaimService{ + reader: reader, + client: c, + namespace: namespace, + } +} + +// Acquire ensures the addon owns the claim for its repository/chart pair. It +// reports whether the claim is held by this addon; when it is not, holder names +// the addon that currently owns it so the caller can surface a conflict. +func (s *ClaimService) Acquire(ctx context.Context, addon *helmv1alpha1.HelmClusterAddon) (acquired bool, holder string, err error) { + nn := s.leaseKey(addon) + + lease := &coordinationv1.Lease{} + getErr := s.reader.Get(ctx, nn, lease) + switch { + case apierrors.IsNotFound(getErr): + created, createErr := s.create(ctx, nn, addon) + if createErr != nil { + return false, "", createErr + } + if created { + return true, addon.Name, nil + } + // Lost the create race against a concurrent reconcile; re-read the winning + // Lease authoritatively and fall through to the ownership check. + if err := s.reader.Get(ctx, nn, lease); err != nil { + return false, "", fmt.Errorf("getting chart claim lease: %w", err) + } + case getErr != nil: + return false, "", fmt.Errorf("getting chart claim lease: %w", getErr) + } + + holder = leaseHolder(lease) + if holder == addon.Name { + return true, holder, nil + } + + stale, err := s.isHolderStale(ctx, holder, nn.Name) + if err != nil { + return false, "", err + } + if !stale { + return false, holder, nil + } + + // The recorded holder is gone or no longer uses this chart; take the Lease over. + // The update is optimistic: if another addon takes it over first, our write is + // rejected with a conflict and we report the pair as claimed and requeue. + if err := s.takeOver(ctx, lease, addon); err != nil { + if apierrors.IsConflict(err) { + return false, holder, nil + } + return false, "", err + } + + return true, addon.Name, nil +} + +// Release deletes the claim Lease, but only if this addon still owns it, so a +// duplicate addon that never acquired the pair cannot free the real owner's claim. +func (s *ClaimService) Release(ctx context.Context, addon *helmv1alpha1.HelmClusterAddon) error { + nn := s.leaseKey(addon) + + lease := &coordinationv1.Lease{} + if err := s.reader.Get(ctx, nn, lease); err != nil { + return client.IgnoreNotFound(err) + } + + if leaseHolder(lease) != addon.Name { + return nil + } + + // Delete only this exact revision: if the Lease was taken over between the read + // and the delete, the precondition fails and we leave the new owner's claim intact. + err := s.client.Delete(ctx, lease, client.Preconditions{ResourceVersion: &lease.ResourceVersion}) + if apierrors.IsConflict(err) { + return nil + } + if client.IgnoreNotFound(err) != nil { + return fmt.Errorf("releasing chart claim lease: %w", err) + } + + return nil +} + +// ReleaseStale deletes claim Leases this addon still owns for repository/chart +// pairs it no longer references — e.g. after the chart ref changed. The Lease name +// is derived from the current spec, so a chart change makes Acquire claim a new +// Lease and orphan the old one; the previous pair is not stored anywhere. The +// orphans are found instead by the source-name label every claim already carries. +// Only the addon's own claims (by HolderIdentity) other than the current one are +// removed, so a pair taken over by another addon is left intact. +func (s *ClaimService) ReleaseStale(ctx context.Context, addon *helmv1alpha1.HelmClusterAddon) error { + current := s.leaseKey(addon).Name + + // Direct reader, not the cached client: claim Leases are not watched, so the + // informer cache has no data for them (same reason Acquire/Release use reader). + leases := &coordinationv1.LeaseList{} + if err := s.reader.List(ctx, leases, + client.InNamespace(s.namespace), + client.MatchingLabels{ + helmv1alpha1.LabelManagedBy: helmv1alpha1.LabelManagedByValue, + helmv1alpha1.HelmClusterAddonLabelSourceName: addon.Name, + }, + ); err != nil { + return fmt.Errorf("listing chart claim leases: %w", err) + } + + for i := range leases.Items { + lease := &leases.Items[i] + if lease.Name == current || leaseHolder(lease) != addon.Name { + continue + } + + // Delete only this exact revision: if the Lease was taken over between the + // list and the delete, the precondition fails and we leave it intact. + err := s.client.Delete(ctx, lease, client.Preconditions{ResourceVersion: &lease.ResourceVersion}) + if apierrors.IsConflict(err) { + continue + } + if client.IgnoreNotFound(err) != nil { + return fmt.Errorf("releasing stale chart claim lease %q: %w", lease.Name, err) + } + } + + return nil +} + +func (s *ClaimService) create(ctx context.Context, nn types.NamespacedName, addon *helmv1alpha1.HelmClusterAddon) (created bool, err error) { + holder := addon.Name + lease := &coordinationv1.Lease{ + ObjectMeta: metav1.ObjectMeta{ + Name: nn.Name, + Namespace: nn.Namespace, + Labels: claimLabels(addon), + }, + Spec: coordinationv1.LeaseSpec{ + HolderIdentity: &holder, + }, + } + + err = s.client.Create(ctx, lease) + if apierrors.IsAlreadyExists(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("creating chart claim lease: %w", err) + } + + return true, nil +} + +func (s *ClaimService) takeOver(ctx context.Context, lease *coordinationv1.Lease, addon *helmv1alpha1.HelmClusterAddon) error { + holder := addon.Name + lease.Spec.HolderIdentity = &holder + + if lease.Labels == nil { + lease.Labels = map[string]string{} + } + for k, v := range claimLabels(addon) { + lease.Labels[k] = v + } + + return s.client.Update(ctx, lease) +} + +// isHolderStale reports whether the recorded holder no longer keeps this claim: it +// either no longer exists or its current spec points at a different chart, in which +// case the Lease is an orphan left behind by a deletion or a chart change. +func (s *ClaimService) isHolderStale(ctx context.Context, holder, leaseName string) (bool, error) { + if holder == "" { + return true, nil + } + + other := &helmv1alpha1.HelmClusterAddon{} + err := s.reader.Get(ctx, types.NamespacedName{Name: holder}, other) + if apierrors.IsNotFound(err) { + return true, nil + } + if err != nil { + return false, fmt.Errorf("getting chart claim holder: %w", err) + } + + holderLease := utils.GetChartClaimLeaseName( + other.Spec.Chart.HelmClusterAddonRepository, other.Spec.Chart.HelmClusterAddonChartName, + ) + + return holderLease != leaseName, nil +} + +func (s *ClaimService) leaseKey(addon *helmv1alpha1.HelmClusterAddon) types.NamespacedName { + return types.NamespacedName{ + Name: utils.GetChartClaimLeaseName(addon.Spec.Chart.HelmClusterAddonRepository, addon.Spec.Chart.HelmClusterAddonChartName), + Namespace: s.namespace, + } +} + +func leaseHolder(lease *coordinationv1.Lease) string { + if lease.Spec.HolderIdentity == nil { + return "" + } + + return *lease.Spec.HolderIdentity +} + +func claimLabels(addon *helmv1alpha1.HelmClusterAddon) map[string]string { + return map[string]string{ + helmv1alpha1.LabelManagedBy: helmv1alpha1.LabelManagedByValue, + helmv1alpha1.HelmClusterAddonLabelSourceName: addon.Name, + } +} diff --git a/images/operator-helm-controller/internal/utils/name.go b/images/operator-helm-controller/internal/utils/name.go index ae40712..29b4e13 100644 --- a/images/operator-helm-controller/internal/utils/name.go +++ b/images/operator-helm-controller/internal/utils/name.go @@ -127,6 +127,18 @@ func GetInternalOCIRepositoryName(addonName string) string { return strings.TrimRight(result, "-") + postfix } +// GetChartClaimLeaseName derives the name of the Lease that guards uniqueness of a +// repository/chart pair across HelmClusterAddon objects. The name is a pure hash of +// the pair: repository and chart names may contain characters that are invalid in an +// object name (uppercase, dots for OCI references), and the hash is always a valid +// DNS subdomain. +func GetChartClaimLeaseName(repoName, chartName string) string { + prefix := "hca-claim" + hash := GetHash(fmt.Sprintf("%s-%s-%s", prefix, repoName, chartName)) + + return prefix + "-" + hash +} + func GetInternalHelmRepositoryName(addonRepositoryName string) string { prefix := "hcar" hash := GetHash(fmt.Sprintf("%s-%s", prefix, addonRepositoryName)) diff --git a/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go b/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go index 02b18ba..c32356c 100644 --- a/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go +++ b/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go @@ -71,8 +71,14 @@ func (v *UniqRepositoryAndChartNameWebhookValidator) ValidateCreate(ctx context. return nil, v.checkUniqueness(ctx, addon) } -func (v *UniqRepositoryAndChartNameWebhookValidator) ValidateUpdate(ctx context.Context, _, newObj *helmv1alpha1.HelmClusterAddon) (admission.Warnings, error) { - return nil, v.checkUniqueness(ctx, newObj) +// ValidateUpdate intentionally does not check chart/repo uniqueness. Uniqueness is +// enforced authoritatively by the chart claim in the reconciler; the webhook only +// fast-rejects the obvious duplicate on CREATE. Validating it on UPDATE would be +// actively harmful: once a race has let several addons with the same chart/repo +// exist, every one of them lists the others and would reject any update — including +// the finalizer the reconciler must add before it can ever resolve the conflict. +func (v *UniqRepositoryAndChartNameWebhookValidator) ValidateUpdate(_ context.Context, _, _ *helmv1alpha1.HelmClusterAddon) (admission.Warnings, error) { + return nil, nil } func (v *UniqRepositoryAndChartNameWebhookValidator) ValidateDelete(_ context.Context, _ *helmv1alpha1.HelmClusterAddon) (admission.Warnings, error) { diff --git a/templates/operator-helm-controller/validation-webhook.yaml b/templates/operator-helm-controller/validation-webhook.yaml index 460f37f..37adca4 100644 --- a/templates/operator-helm-controller/validation-webhook.yaml +++ b/templates/operator-helm-controller/validation-webhook.yaml @@ -11,7 +11,7 @@ webhooks: rules: - apiGroups: ["helm.deckhouse.io"] apiVersions: ["v1alpha1"] - operations: ["CREATE", "UPDATE", "DELETE"] + operations: ["CREATE", "DELETE"] resources: ["helmclusteraddons"] scope: "Cluster" clientConfig: diff --git a/tests/e2e/helmclusteraddon/chartclaim.go b/tests/e2e/helmclusteraddon/chartclaim.go new file mode 100644 index 0000000..ce5150b --- /dev/null +++ b/tests/e2e/helmclusteraddon/chartclaim.go @@ -0,0 +1,228 @@ +/* +Copyright 2026 Flant JSC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helmclusteraddon + +import ( + "context" + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + apiv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/tests/e2e/internal/controller" + "github.com/deckhouse/operator-helm/tests/e2e/internal/framework" + "github.com/deckhouse/operator-helm/tests/e2e/internal/util" +) + +// claimLeaseNames returns the names of the claim Leases the controller currently +// records for an addon. Each claim Lease carries the source-name label, so the +// leases an addon owns can be listed without recomputing the hashed Lease name. +func claimLeaseNames(addonName string) []string { + GinkgoHelper() + + selector := fmt.Sprintf("%s=%s,%s=%s", + apiv1alpha1.LabelManagedBy, apiv1alpha1.LabelManagedByValue, + apiv1alpha1.HelmClusterAddonLabelSourceName, addonName, + ) + + list, err := framework.GetClients().KubeClient().CoordinationV1(). + Leases(apiv1alpha1.TargetNamespace). + List(context.Background(), metav1.ListOptions{LabelSelector: selector}) + Expect(err).NotTo(HaveOccurred()) + + names := make([]string, 0, len(list.Items)) + for _, lease := range list.Items { + names = append(names, lease.Name) + } + return names +} + +var _ = Describe("HelmClusterAddon chart claim", Ordered, func() { + f := framework.NewFramework("addon-chart-claim") + + const ( + repoURL = "https://stefanprodan.github.io/podinfo" + chartName = "podinfo" + version = "6.10.2" + ) + + repoAName := "e2e-claim-repo-a" + repoBName := "e2e-claim-repo-b" + addonAName := "e2e-claim-addon-a" + addonBName := "e2e-claim-addon-b" + + // A dedicated namespace for addon B: it installs the same chart as addon A, so + // it must land in a separate namespace to avoid colliding on release resources. + var addonBNamespace string + + // The name of the claim Lease addon A holds while it points at repoA; captured + // before the chart reference change so its release can be asserted afterwards. + var repoAClaimLease string + + BeforeAll(func() { + DeferCleanup(f.After) + f.Before() + + nsB := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: fmt.Sprintf("%s-addon-chart-claim-b-", framework.NamespacePrefix), + Labels: map[string]string{framework.E2ELabel: "true"}, + }, + } + Expect(f.GenericClient().Create(context.Background(), nsB)).To(Succeed()) + f.DeferDelete(nsB) + addonBNamespace = nsB.Name + }) + + AfterEach(func() { + By("Verifying no errors in operator-helm-controller logs") + controller.AssertNoErrorsFor("operator-helm-controller") + }) + + It("should create two repositories pointing at the same source", func() { + for _, name := range []string{repoAName, repoBName} { + repo := &apiv1alpha1.HelmClusterAddonRepository{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: apiv1alpha1.HelmClusterAddonRepositorySpec{ + URL: repoURL, + InsecureSkipVerify: false, + }, + } + + created, err := f.OperatorClient().HelmV1alpha1(). + HelmClusterAddonRepositories(). + Create(context.Background(), repo, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + f.DeferDelete(created) + + By(fmt.Sprintf("Waiting for repository %q to become Ready and Synced", name)) + util.UntilConditionTrue(apiv1alpha1.ConditionTypeReady, framework.LongTimeout, created) + util.UntilConditionTrue(apiv1alpha1.ConditionTypeSynced, framework.LongTimeout, created) + } + }) + + It("should create addon A and claim the repository/chart pair", func() { + addon := &apiv1alpha1.HelmClusterAddon{ + ObjectMeta: metav1.ObjectMeta{Name: addonAName}, + Spec: apiv1alpha1.HelmClusterAddonSpec{ + Chart: apiv1alpha1.HelmClusterAddonChartRef{ + HelmClusterAddonChartName: chartName, + HelmClusterAddonRepository: repoAName, + Version: version, + }, + Namespace: f.NamespaceName(), + }, + } + + created, err := f.OperatorClient().HelmV1alpha1(). + HelmClusterAddons(). + Create(context.Background(), addon, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + f.DeferDelete(created) + + By("Waiting for addon A to become Ready") + util.UntilConditionTrue(apiv1alpha1.ConditionTypeReady, framework.LongTimeout, created) + + By("Verifying addon A holds exactly one claim Lease") + Eventually(func(g Gomega) { + leases := claimLeaseNames(addonAName) + g.Expect(leases).To(HaveLen(1)) + repoAClaimLease = leases[0] + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + }) + + It("should reject a second addon that reuses the same repository and chart", func() { + duplicate := &apiv1alpha1.HelmClusterAddon{ + ObjectMeta: metav1.ObjectMeta{Name: addonBName}, + Spec: apiv1alpha1.HelmClusterAddonSpec{ + Chart: apiv1alpha1.HelmClusterAddonChartRef{ + HelmClusterAddonChartName: chartName, + HelmClusterAddonRepository: repoAName, + Version: version, + }, + Namespace: addonBNamespace, + }, + } + + By("Attempting to create a duplicate addon") + _, err := f.OperatorClient().HelmV1alpha1(). + HelmClusterAddons(). + Create(context.Background(), duplicate, metav1.CreateOptions{}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("already used by helmclusteraddon/" + addonAName)) + + By("Verifying addon A still holds a single claim Lease") + Expect(claimLeaseNames(addonAName)).To(ConsistOf(repoAClaimLease)) + }) + + It("should release the stale claim when addon A changes its repository", func() { + By("Repointing addon A to the second repository") + updated := util.UpdateHelmClusterAddon(addonAName, func(addon *apiv1alpha1.HelmClusterAddon) { + addon.Spec.Chart.HelmClusterAddonRepository = repoBName + }) + + By("Waiting for addon A to reconcile against the new repository") + util.UntilConditionTrue(apiv1alpha1.ConditionTypeReady, framework.LongTimeout, updated) + + By("Verifying the old claim Lease is released and a single new one is held") + Eventually(func(g Gomega) { + leases := claimLeaseNames(addonAName) + g.Expect(leases).To(HaveLen(1)) + g.Expect(leases).NotTo(ContainElement(repoAClaimLease)) + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + + By("Verifying the old Lease object no longer exists") + Eventually(func(g Gomega) { + _, err := f.KubeClient().CoordinationV1(). + Leases(apiv1alpha1.TargetNamespace). + Get(context.Background(), repoAClaimLease, metav1.GetOptions{}) + g.Expect(err).To(HaveOccurred()) + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + }) + + It("should let a new addon claim the released repository/chart pair", func() { + reuse := &apiv1alpha1.HelmClusterAddon{ + ObjectMeta: metav1.ObjectMeta{Name: addonBName}, + Spec: apiv1alpha1.HelmClusterAddonSpec{ + Chart: apiv1alpha1.HelmClusterAddonChartRef{ + HelmClusterAddonChartName: chartName, + HelmClusterAddonRepository: repoAName, + Version: version, + }, + Namespace: addonBNamespace, + }, + } + + By("Creating addon B on the freed repository/chart pair") + created, err := f.OperatorClient().HelmV1alpha1(). + HelmClusterAddons(). + Create(context.Background(), reuse, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + f.DeferDelete(created) + + By("Waiting for addon B to become Ready") + util.UntilConditionTrue(apiv1alpha1.ConditionTypeReady, framework.LongTimeout, created) + + By("Verifying addon B did not surface a chart claim conflict") + claimLease := claimLeaseNames(addonBName) + Expect(claimLease).To(HaveLen(1)) + Expect(claimLease).To(ConsistOf(repoAClaimLease)) + }) +})