From d2cc75a27251dc1ab2d580c4543ca95b730a54fc Mon Sep 17 00:00:00 2001 From: bdchatham Date: Fri, 29 May 2026 16:53:41 -0700 Subject: [PATCH] fix(snd): propagate Spec.Peers from SND template to existing child SeiNodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensureSeiNode previously only deep-copied Spec.Peers on the create-path (via generateSeiNode). The update-path used an allow-list of fields to diff and re-apply — Labels, Annotations, Spec.Image, Spec.Sidecar, Spec.PodLabels, Spec.ExternalAddress — and Peers was silently missing. This contradicted the templateHash godoc in labels.go, which explicitly states peers "propagate in-place via ensureSeiNode" and so are excluded from the hash. Adding a peer source to a live SND (e.g. platform#759 adding a LabelPeerSource to existing Harbor syncers) appeared to work at the SND CR level but never reached the child SeiNodes — their spec.peers stayed frozen at child-creation time, the resolver kept re-emitting the stale list, and Status.ResolvedPeers never picked up new sources. Fix: add a Spec.Peers diff/update branch between PodLabels and ExternalAddress, using equality.Semantic.DeepEqual so etcd nil/empty round-trips don't churn the diff. Coral cross-review (kubernetes-specialist + platform-engineer) confirmed the fix doesn't open a deployment-trigger feedback loop: - detectDeploymentNeeded gates on templateHash, which excludes peers. - childPhaseChangedPredicate filters out status-only updates, so live Status.ResolvedPeers churn doesn't enqueue the SND. - The Update() only fires when SND.spec.peers actually differs from the child's spec.peers — a human/Flux edit, not a resolver fluctuation. Tests: - Unit (nodes_test.go): PropagatesPeersOnUpdate, PropagatesPeerRemoval, PeersNoOpWhenUnchanged. - Envtest (peers_propagation_test.go): AddSourceOnLiveSND, RemoveSourceOnLiveSND. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../nodedeployment/envtest/fixtures/snd.go | 7 ++ .../envtest/peers_propagation_test.go | 107 ++++++++++++++++++ internal/controller/nodedeployment/nodes.go | 9 ++ .../controller/nodedeployment/nodes_test.go | 97 ++++++++++++++++ 4 files changed, 220 insertions(+) create mode 100644 internal/controller/nodedeployment/envtest/peers_propagation_test.go diff --git a/internal/controller/nodedeployment/envtest/fixtures/snd.go b/internal/controller/nodedeployment/envtest/fixtures/snd.go index 1ba5051..983df1e 100644 --- a/internal/controller/nodedeployment/envtest/fixtures/snd.go +++ b/internal/controller/nodedeployment/envtest/fixtures/snd.go @@ -82,3 +82,10 @@ func WithValidator() Option { snd.Spec.Template.Spec.Validator = &seiv1alpha1.ValidatorSpec{} } } + +// WithPeers sets spec.template.spec.peers. +func WithPeers(peers ...seiv1alpha1.PeerSource) Option { + return func(snd *seiv1alpha1.SeiNodeDeployment) { + snd.Spec.Template.Spec.Peers = peers + } +} diff --git a/internal/controller/nodedeployment/envtest/peers_propagation_test.go b/internal/controller/nodedeployment/envtest/peers_propagation_test.go new file mode 100644 index 0000000..75314c0 --- /dev/null +++ b/internal/controller/nodedeployment/envtest/peers_propagation_test.go @@ -0,0 +1,107 @@ +//go:build envtest + +package envtest_test + +import ( + "testing" + "time" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + seiv1alpha1 "github.com/sei-protocol/sei-k8s-controller/api/v1alpha1" + "github.com/sei-protocol/sei-k8s-controller/internal/controller/nodedeployment/envtest/fixtures" +) + +// Live edit to SND.spec.template.spec.peers must reach the already-created +// child. Regression for the silent-allow-list gap in ensureSeiNode. +func TestPeersPropagation_AddSourceOnLiveSND(t *testing.T) { + g := NewWithT(t) + ns := makeNamespace(t) + + initial := []seiv1alpha1.PeerSource{ + {EC2Tags: &seiv1alpha1.EC2TagsPeerSource{ + Region: "eu-central-1", + Tags: map[string]string{"ChainIdentifier": "pacific-1", "Component": "state-syncer"}, + }}, + } + snd := fixtures.NewSND(ns, "peers-add", + fixtures.WithReplicas(1), + fixtures.WithPeers(initial...), + ) + g.Expect(testCli.Create(testCtx, snd)).To(Succeed()) + + childKey := types.NamespacedName{Name: snd.Name + "-0", Namespace: ns} + waitFor(t, func() bool { + child := &seiv1alpha1.SeiNode{} + if err := testCli.Get(testCtx, childKey, child); err != nil { + return false + } + return len(child.Spec.Peers) == 1 + }, "child converged with initial peer set") + + // Live edit: add a LabelPeerSource (the platform#759 shape). + g.Eventually(func() error { + latest := getSND(t, client.ObjectKeyFromObject(snd)) + patch := client.MergeFrom(latest.DeepCopy()) + latest.Spec.Template.Spec.Peers = append(latest.Spec.Template.Spec.Peers, + seiv1alpha1.PeerSource{Label: &seiv1alpha1.LabelPeerSource{ + Namespace: ns, + Selector: map[string]string{"sei.io/chain": "pacific-1"}, + }}) + return testCli.Patch(testCtx, latest, patch) + }, 5*time.Second, 200*time.Millisecond).Should(Succeed()) + + waitFor(t, func() bool { + child := &seiv1alpha1.SeiNode{} + if err := testCli.Get(testCtx, childKey, child); err != nil { + return false + } + if len(child.Spec.Peers) != 2 { + return false + } + return child.Spec.Peers[1].Label != nil && + child.Spec.Peers[1].Label.Selector["sei.io/chain"] == "pacific-1" + }, "child Spec.Peers reflected the live SND peer-list addition") +} + +// Removing a peer source on a live SND must propagate (shrink case). +func TestPeersPropagation_RemoveSourceOnLiveSND(t *testing.T) { + g := NewWithT(t) + ns := makeNamespace(t) + + initial := []seiv1alpha1.PeerSource{ + {EC2Tags: &seiv1alpha1.EC2TagsPeerSource{Region: "eu-central-1", Tags: map[string]string{"k": "v"}}}, + {Label: &seiv1alpha1.LabelPeerSource{Namespace: ns, Selector: map[string]string{"k": "v"}}}, + } + snd := fixtures.NewSND(ns, "peers-remove", + fixtures.WithReplicas(1), + fixtures.WithPeers(initial...), + ) + g.Expect(testCli.Create(testCtx, snd)).To(Succeed()) + + childKey := types.NamespacedName{Name: snd.Name + "-0", Namespace: ns} + waitFor(t, func() bool { + child := &seiv1alpha1.SeiNode{} + if err := testCli.Get(testCtx, childKey, child); err != nil { + return false + } + return len(child.Spec.Peers) == 2 + }, "child converged with initial 2-source peer set") + + g.Eventually(func() error { + latest := getSND(t, client.ObjectKeyFromObject(snd)) + patch := client.MergeFrom(latest.DeepCopy()) + latest.Spec.Template.Spec.Peers = latest.Spec.Template.Spec.Peers[:1] + return testCli.Patch(testCtx, latest, patch) + }, 5*time.Second, 200*time.Millisecond).Should(Succeed()) + + waitFor(t, func() bool { + child := &seiv1alpha1.SeiNode{} + if err := testCli.Get(testCtx, childKey, child); err != nil { + return false + } + return len(child.Spec.Peers) == 1 && child.Spec.Peers[0].EC2Tags != nil + }, "child Spec.Peers shrunk to match the live SND") +} diff --git a/internal/controller/nodedeployment/nodes.go b/internal/controller/nodedeployment/nodes.go index 1abcd4b..ecd2e9d 100644 --- a/internal/controller/nodedeployment/nodes.go +++ b/internal/controller/nodedeployment/nodes.go @@ -6,6 +6,7 @@ import ( "maps" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -219,6 +220,14 @@ func (r *SeiNodeDeploymentReconciler) ensureSeiNode(ctx context.Context, group * existing.Spec.PodLabels = desired.Spec.PodLabels updated = true } + // Peers are intentionally in-place propagatable (templateHash godoc + // in labels.go). Semantic.DeepEqual treats nil and empty maps/slices + // as equal — etcd round-trips normalize empties to nil, so reflect + // would spuriously diff after the first reconcile post-restart. + if !equality.Semantic.DeepEqual(existing.Spec.Peers, desired.Spec.Peers) { + existing.Spec.Peers = desired.Spec.Peers + updated = true + } if existing.Spec.ExternalAddress != desired.Spec.ExternalAddress { existing.Spec.ExternalAddress = desired.Spec.ExternalAddress updated = true diff --git a/internal/controller/nodedeployment/nodes_test.go b/internal/controller/nodedeployment/nodes_test.go index 29b1212..38183c7 100644 --- a/internal/controller/nodedeployment/nodes_test.go +++ b/internal/controller/nodedeployment/nodes_test.go @@ -1,16 +1,20 @@ package nodedeployment import ( + "context" "testing" . "github.com/onsi/gomega" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" seiv1alpha1 "github.com/sei-protocol/sei-k8s-controller/api/v1alpha1" ) +const testSyncerOrd0 = "syncer-0" + func newTestGroup(name, namespace string) *seiv1alpha1.SeiNodeDeployment { return &seiv1alpha1.SeiNodeDeployment{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, @@ -398,3 +402,96 @@ func TestSetGenesisCeremonyCondition(t *testing.T) { }) } } + +// Adding a peer source to an existing SND must reach the child. +func TestEnsureSeiNode_PropagatesPeersOnUpdate(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + group := newTestGroup("syncer", "pacific-1") + group.Spec.Template.Spec.Peers = []seiv1alpha1.PeerSource{ + {EC2Tags: &seiv1alpha1.EC2TagsPeerSource{ + Region: "eu-central-1", + Tags: map[string]string{"ChainIdentifier": "pacific-1", "Component": "state-syncer"}, + }}, + } + + r := newPlanTestReconciler(t, group) + + // First reconcile: child created with the EC2Tags-only spec. + g.Expect(r.ensureSeiNode(ctx, group, 0)).To(Succeed()) + + child := &seiv1alpha1.SeiNode{} + childKey := types.NamespacedName{Name: testSyncerOrd0, Namespace: "pacific-1"} + g.Expect(r.Get(ctx, childKey, child)).To(Succeed()) + g.Expect(child.Spec.Peers).To(HaveLen(1)) + + // SND template gains a LabelPeerSource (the platform#759 shape). + group.Spec.Template.Spec.Peers = append(group.Spec.Template.Spec.Peers, + seiv1alpha1.PeerSource{Label: &seiv1alpha1.LabelPeerSource{ + Namespace: "pacific-1", + Selector: map[string]string{"sei.io/chain": "pacific-1"}, + }}) + + // Second reconcile: the new peer source must propagate to the + // already-existing child. + g.Expect(r.ensureSeiNode(ctx, group, 0)).To(Succeed()) + + g.Expect(r.Get(ctx, childKey, child)).To(Succeed()) + g.Expect(child.Spec.Peers).To(HaveLen(2), + "existing child Spec.Peers must reflect SND template additions") + g.Expect(child.Spec.Peers[1].Label).NotTo(BeNil(), + "second entry must be the newly-added LabelPeerSource") + g.Expect(child.Spec.Peers[1].Label.Selector).To(Equal(map[string]string{"sei.io/chain": "pacific-1"})) +} + +// Removing a peer source from the SND must also propagate (proves the +// diff captures shrink, not just grow). +func TestEnsureSeiNode_PropagatesPeerRemoval(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + group := newTestGroup("syncer", "pacific-1") + group.Spec.Template.Spec.Peers = []seiv1alpha1.PeerSource{ + {EC2Tags: &seiv1alpha1.EC2TagsPeerSource{Region: "eu-central-1", Tags: map[string]string{"k": "v"}}}, + {Label: &seiv1alpha1.LabelPeerSource{Namespace: "pacific-1", Selector: map[string]string{"k": "v"}}}, + } + + r := newPlanTestReconciler(t, group) + g.Expect(r.ensureSeiNode(ctx, group, 0)).To(Succeed()) + + // Drop the label entry from the SND template. + group.Spec.Template.Spec.Peers = group.Spec.Template.Spec.Peers[:1] + g.Expect(r.ensureSeiNode(ctx, group, 0)).To(Succeed()) + + child := &seiv1alpha1.SeiNode{} + g.Expect(r.Get(ctx, types.NamespacedName{Name: testSyncerOrd0, Namespace: "pacific-1"}, child)).To(Succeed()) + g.Expect(child.Spec.Peers).To(HaveLen(1)) + g.Expect(child.Spec.Peers[0].EC2Tags).NotTo(BeNil()) + g.Expect(child.Spec.Peers[0].Label).To(BeNil()) +} + +// No-op reconcile path: identical SND template across two reconciles +// should not trigger a child Update (no resourceVersion bump). +func TestEnsureSeiNode_PeersNoOpWhenUnchanged(t *testing.T) { + g := NewWithT(t) + ctx := context.Background() + + group := newTestGroup("syncer", "pacific-1") + group.Spec.Template.Spec.Peers = []seiv1alpha1.PeerSource{ + {Label: &seiv1alpha1.LabelPeerSource{Namespace: "pacific-1", Selector: map[string]string{"k": "v"}}}, + } + + r := newPlanTestReconciler(t, group) + g.Expect(r.ensureSeiNode(ctx, group, 0)).To(Succeed()) + + child := &seiv1alpha1.SeiNode{} + childKey := types.NamespacedName{Name: testSyncerOrd0, Namespace: "pacific-1"} + g.Expect(r.Get(ctx, childKey, child)).To(Succeed()) + rvBefore := child.ResourceVersion + + g.Expect(r.ensureSeiNode(ctx, group, 0)).To(Succeed()) + g.Expect(r.Get(ctx, childKey, child)).To(Succeed()) + g.Expect(child.ResourceVersion).To(Equal(rvBefore), + "no-op reconcile must not bump resourceVersion") +}