From f4d6a72024768bc05ee1ba8af93c4bea221fcc03 Mon Sep 17 00:00:00 2001 From: Daniel Grimm Date: Wed, 18 Jun 2025 09:42:50 +0200 Subject: [PATCH] fix IstioRevisionTag uninstall when revision namespace changes Fixes #917. This is quite a complex edge case, I'll do my best to explain. If you look at the integration test I wrote (part of this commit), you can follow along. When a tag was created that referenced a revision in ns1, then patched to reference a revision in ns2, we failed to clean up the old helm install in ns1. That almost never causes problems - except in the case where afterwards, you remove all tags, and then create a new one with the same name that is never reconciled. If that tag is deleted, the operator will attempt to uninstall that lingering helm release, which fails, leading the operator to deadlock, and stopping the user from deleting the tag. The only way to get the operator back into a working state would be to manually delete that lingering helm release. That is a pretty significant bug in my opinion. Signed-off-by: Daniel Grimm --- .../istiorevisiontag_controller.go | 7 ++ .../integration/api/istiorevisiontag_test.go | 117 ++++++++++++++++++ 2 files changed, 124 insertions(+) diff --git a/controllers/istiorevisiontag/istiorevisiontag_controller.go b/controllers/istiorevisiontag/istiorevisiontag_controller.go index 630c18aeef..175b2ee778 100644 --- a/controllers/istiorevisiontag/istiorevisiontag_controller.go +++ b/controllers/istiorevisiontag/istiorevisiontag_controller.go @@ -110,6 +110,13 @@ func (r *Reconciler) doReconcile(ctx context.Context, tag *v1.IstioRevisionTag) return nil, reconciler.NewValidationError("IstioRevisionTag cannot reference a remote IstioRevision") } + // if the IstioRevision's namespace changes, we need to completely reinstall the tag + if tag.Status.IstiodNamespace != "" && tag.Status.IstiodNamespace != rev.Spec.Namespace { + if err := r.uninstallHelmCharts(ctx, tag); err != nil { + return nil, err + } + } + log.Info("Installing Helm chart") return rev, r.installHelmCharts(ctx, tag, rev) } diff --git a/tests/integration/api/istiorevisiontag_test.go b/tests/integration/api/istiorevisiontag_test.go index 59724e5cf9..68c286dbaa 100644 --- a/tests/integration/api/istiorevisiontag_test.go +++ b/tests/integration/api/istiorevisiontag_test.go @@ -43,6 +43,7 @@ var _ = Describe("IstioRevisionTag resource", Ordered, func() { gracePeriod = 5 * time.Second ) istio := &v1.Istio{} + istio2 := &v1.Istio{} istioKey := client.ObjectKey{Name: istioName} defaultTagKey := client.ObjectKey{Name: defaultTagName} workloadNamespaceKey := client.ObjectKey{Name: workloadNamespace} @@ -360,6 +361,122 @@ var _ = Describe("IstioRevisionTag resource", Ordered, func() { }) }) }) + + When("Changing the targetRef of a tag to an IstioRevision in another namespace", func() { + BeforeAll(func() { + Step("Create primary Istio") + istio = &v1.Istio{ + ObjectMeta: metav1.ObjectMeta{ + Name: istioName, + }, + Spec: v1.IstioSpec{ + Version: istioversion.Base, + Namespace: istioRevisionTagNamespace, + UpdateStrategy: &v1.IstioUpdateStrategy{ + Type: v1.UpdateStrategyTypeInPlace, + InactiveRevisionDeletionGracePeriodSeconds: ptr.Of(int64(gracePeriod.Seconds())), + }, + }, + } + Expect(k8sClient.Create(ctx, istio)).To(Succeed()) + Step("Create secondary Istio") + istio2 = &v1.Istio{ + ObjectMeta: metav1.ObjectMeta{ + Name: istioName + "2", + }, + Spec: v1.IstioSpec{ + Version: istioversion.Base, + Namespace: workloadNamespace, + UpdateStrategy: &v1.IstioUpdateStrategy{ + Type: v1.UpdateStrategyTypeInPlace, + InactiveRevisionDeletionGracePeriodSeconds: ptr.Of(int64(gracePeriod.Seconds())), + }, + }, + } + Expect(k8sClient.Create(ctx, istio2)).To(Succeed()) + Step("Create IstioRevisionTag default") + tag = &v1.IstioRevisionTag{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaultTagName, + }, + Spec: v1.IstioRevisionTagSpec{ + TargetRef: v1.IstioRevisionTagTargetReference{ + Kind: "Istio", + Name: istioName, + }, + }, + } + Expect(k8sClient.Create(ctx, tag)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, defaultTagKey, tag)).To(Succeed()) + g.Expect(tag.Status.IstiodNamespace).To(Equal(istioRevisionTagNamespace)) + }).Should(Succeed()) + Step("Switch IstioRevisionTag to secondary istio") + tag.Spec.TargetRef.Name = istio2.Name + Expect(k8sClient.Update(ctx, tag)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, defaultTagKey, tag)).To(Succeed()) + g.Expect(tag.Status.IstiodNamespace).To(Equal(workloadNamespace)) + }).Should(Succeed()) + + deleteAllIstiosAndRevisions(ctx) + deleteAllIstioRevisionTags(ctx) + + Step("Create conflicting Istio and IstioRevisionTags") + istio = &v1.Istio{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaultTagName, + }, + Spec: v1.IstioSpec{ + Version: istioversion.Base, + Namespace: istioRevisionTagNamespace, + UpdateStrategy: &v1.IstioUpdateStrategy{ + Type: v1.UpdateStrategyTypeInPlace, + InactiveRevisionDeletionGracePeriodSeconds: ptr.Of(int64(gracePeriod.Seconds())), + }, + }, + } + Expect(k8sClient.Create(ctx, istio)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, defaultTagKey, istio)).To(Succeed()) + g.Expect(istio.Status.ObservedGeneration).To(Equal(istio.ObjectMeta.Generation)) + }).Should(Succeed()) + + tag = &v1.IstioRevisionTag{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaultTagName, + }, + Spec: v1.IstioRevisionTagSpec{ + TargetRef: v1.IstioRevisionTagTargetReference{ + Kind: "Istio", + Name: istioName, + }, + }, + } + Expect(k8sClient.Create(ctx, tag)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, defaultTagKey, tag)).To(Succeed()) + g.Expect(tag.Status.ObservedGeneration).To(Equal(tag.Generation)) + }).Should(Succeed()) + Consistently(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, defaultTagKey, tag)).To(Succeed()) + g.Expect(tag.Status.GetCondition(v1.IstioRevisionTagConditionReconciled).Status).To(Equal(metav1.ConditionFalse)) + g.Expect(tag.Status.GetCondition(v1.IstioRevisionTagConditionReconciled).Reason).To(Equal(v1.IstioRevisionTagReasonNameAlreadyExists)) + }).Should(Succeed()) + }) + + AfterAll(func() { + deleteAllIstiosAndRevisions(ctx) + deleteAllIstioRevisionTags(ctx) + }) + + It("can still delete the IstioRevisionTag", func() { + Eventually(k8sClient.Delete).WithArguments(ctx, tag).Should(Succeed()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, defaultTagKey, tag)).To(ReturnNotFoundError()) + }).Should(Succeed()) + }) + }) }) func deleteAllIstioRevisionTags(ctx context.Context) {