From c0d1e9b06c77781ae61befc0f2b6b69b7fc23319 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 10 Sep 2026 22:27:15 +0100 Subject: [PATCH 1/2] fix: just in case patch the Pod failed then the instance ID will be leaked Signed-off-by: kerthcet --- pkg/vnode/handler.go | 31 ++++++++++++--- pkg/vnode/handler_test.go | 79 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 5 deletions(-) diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index 196bf45..253a8cd 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -445,8 +445,11 @@ func (h *Handler) UpdatePod(_ context.Context, pod *corev1.Pod) error { h.mu.Lock() defer h.mu.Unlock() if tp, ok := h.tracked[key(pod.Namespace, pod.Name)]; ok { - // Copy the status and endpoint just in case the update failed, - // so we do not lose them in the tracked copy. + // VK's Pod comes from the API server, so it may predate our own metadata patch. The + // status and endpoint are carried across or the tracked copy — what the poll loop + // re-emits, and so the only retry — loses them for good; the endpoint has no other + // in-memory home. The instance id deliberately has no line here: persistMetadata falls + // back to trackedPod.instance, which this never touches. status := tp.pod.Status endpoint := tp.pod.Annotations[nebulav1alpha1.EndpointAnnotation] tp.pod = pod.DeepCopy() @@ -758,9 +761,14 @@ func setEndpoint(pod *corev1.Pod, endpoint string) { setAnnotation(pod, nebulav1alpha1.EndpointAnnotation, endpoint) } -// setInstanceID stamps the provider's instance id on the Pod. One writer only — -// CreatePod, the moment Provision returns it — because that is the only place it is -// learned; the poll loop matches instances by CLAIM NAME and never re-derives the id. +// setInstanceID stamps the provider's instance id on the Pod. One stamping site — +// CreatePod, the moment Provision returns it — and, unlike the endpoint, no read-path +// site that re-applies it every tick. +// +// So this is not the only way the id reaches etcd, and must not become it: a re-adopted +// pod (see GetPod) learns its id from the provider's List without any Pod to stamp, which +// is why persistMetadata falls back to trackedPod.instance. Anything that drops the +// annotation from a tracked copy depends on that fallback to recover. // // It rides the Pod so the NodeClaim controller can record it from an object it already // has, instead of asking the provider for a full instance list on every reconcile (see @@ -875,6 +883,19 @@ func (h *Handler) persistMetadata(ctx context.Context, pod *corev1.Pod) { // An untracked pod has nothing to compare against, so everything it carries is // patched: these annotations are the only place those values reach a reader. if tp, tracked := h.tracked[key(pod.Namespace, pod.Name)]; tracked { + // The ANNOTATION wins: it is the value already offered to readers, so etcd stays the + // source of truth. trackedPod.instance is the fallback for the paths that hold an id + // without one ever being stamped on the Pod — a re-adopted pod (see GetPod), and one + // whose copy UpdatePod replaced from the API server before the patch landed. They are + // the same value wherever both exist. + // + // Load-bearing, not belt-and-braces: nothing re-derives this annotation later (see + // setInstanceID), so an id missing here is missing for the instance's whole life, and + // with it status.InstanceID — leaving teardown to search the provider by claim name + // and logship to skip the Pod entirely. + if want.instanceID == "" { + want.instanceID = tp.instance + } want = want.minus(tp.patchedMeta) } h.mu.Unlock() diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index 89581a5..5de0050 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -950,6 +950,85 @@ func TestCreatePod_PersistsInstanceIDAlongsideEndpoint(t *testing.T) { } } +// A pod re-adopted after a VK restart is the case the create-path stamp cannot cover: GetPod +// builds a fresh Pod from the provider's List, so the id exists only in trackedPod.instance and +// there is no annotation to re-offer. Nothing re-derives it later either (see setInstanceID), so +// without persistMetadata's fallback status.InstanceID stays empty for the instance's whole life — +// leaving teardown to search the provider by claim name and logship to skip the Pod. +func TestReadoptedPod_PersistsInstanceIDFromTracking(t *testing.T) { + pod := testPod("default", "p1") + client := fake.NewSimpleClientset(pod) + + var patches int + client.PrependReactor("patch", "pods", func(k8stesting.Action) (bool, runtime.Object, error) { + patches++ + return false, nil, nil // fall through to the tracker so the object updates + }) + + fp := &fakeProvider{list: []provider.Instance{ + {ID: "inst-1", ClaimName: "default-p1", State: provider.InstanceRunning}, + }} + h := NewHandler(fp, client, nil, openCluster()) + h.NotifyPods(context.Background(), func(*corev1.Pod) {}) + + // Cold tracking map, live instance: VK's existence check re-adopts it. No emit here, so + // nothing is written yet — the id is in tracking alone. + if _, err := h.GetPod(context.Background(), "default", "p1"); err != nil { + t.Fatalf("GetPod: %v", err) + } + h.reconcileOnce(context.Background()) + + live, err := client.CoreV1().Pods("default").Get(context.Background(), "p1", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get patched pod: %v", err) + } + if got := live.Annotations[nebulav1alpha1.InstanceIDAnnotation]; got != "inst-1" { + t.Fatalf("instance id annotation = %q, want inst-1 — a re-adopted pod's id never reaches "+ + "a reader otherwise", got) + } + + // The fallback feeds the same dedup as a stamped value, or every tick re-patches the fleet. + h.reconcileOnce(context.Background()) + if patches != 1 { + t.Fatalf("an unchanged instance id must not re-patch; got %d patches", patches) + } +} + +// The dedup is only safe because the write is a MERGE patch: a body carrying one annotation +// leaves every other one alone. So once the endpoint has been patched, minus drops it from every +// later body and the id travels by itself without erasing the address. Sending "" for the omitted +// field instead — the obvious-looking alternative to podMeta.annotations omitting it — would +// overwrite a working address with an empty string. +func TestPersistMetadata_OneFieldPatchLeavesTheOtherAlone(t *testing.T) { + const dns = "ec2-1-2-3-4.compute.amazonaws.com" + pod := testPod("default", "p1") + pod.Annotations = map[string]string{nebulav1alpha1.EndpointAnnotation: dns} + client := fake.NewSimpleClientset(pod) + + h := NewHandler(&fakeProvider{}, client, nil, openCluster()) + // The endpoint is already durable, the id is not — the state after any tick that patched + // the address before the id was known. + h.tracked[key("default", "p1")] = &trackedPod{ + pod: pod.DeepCopy(), + claimName: "default-p1", + instance: "inst-1", + patchedMeta: podMeta{endpoint: dns}, + } + + h.persistMetadata(context.Background(), pod.DeepCopy()) + + live, err := client.CoreV1().Pods("default").Get(context.Background(), "p1", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get patched pod: %v", err) + } + if got := live.Annotations[nebulav1alpha1.InstanceIDAnnotation]; got != "inst-1" { + t.Fatalf("instance id annotation = %q, want inst-1", got) + } + if got := live.Annotations[nebulav1alpha1.EndpointAnnotation]; got != dns { + t.Fatalf("endpoint annotation = %q, want %q — a patch that omits a field must not clear it", got, dns) + } +} + // connectSecret fetches the connect Secret for a pod, or nil when absent. func connectSecret(t *testing.T, client *fake.Clientset, ns, podName string) *corev1.Secret { t.Helper() From 840972b3bc1176a8110357917f1e9173be1500df Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 10 Sep 2026 22:50:19 +0100 Subject: [PATCH 2/2] update comment Signed-off-by: kerthcet --- pkg/vnode/handler.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index 253a8cd..7ebf1dc 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -445,11 +445,8 @@ func (h *Handler) UpdatePod(_ context.Context, pod *corev1.Pod) error { h.mu.Lock() defer h.mu.Unlock() if tp, ok := h.tracked[key(pod.Namespace, pod.Name)]; ok { - // VK's Pod comes from the API server, so it may predate our own metadata patch. The - // status and endpoint are carried across or the tracked copy — what the poll loop - // re-emits, and so the only retry — loses them for good; the endpoint has no other - // in-memory home. The instance id deliberately has no line here: persistMetadata falls - // back to trackedPod.instance, which this never touches. + // Preserve status and the endpoint if this API-server copy predates our writes. + // persistMetadata recovers the instance ID from tp.instance instead. status := tp.pod.Status endpoint := tp.pod.Annotations[nebulav1alpha1.EndpointAnnotation] tp.pod = pod.DeepCopy()