From d425c90e0f5acc6947c3d3e32a3e54942d1cd2bf Mon Sep 17 00:00:00 2001
From: Luiz Aoqui <luiz@hashicorp.com>
Date: Thu, 5 Oct 2023 11:41:44 -0400
Subject: [PATCH] client: remove null dynamic metadata keys (#18664)

Setting a null value to a node metadata is expected to remove it from
subsequent reads. This is true both for static node metadata (defined in
the agent configuration file) as well as for dynamic node metadata
(defined via the Nomad API).

Null values for static metadata must be persisted to indicate that the
value has been removed, but strictly dynamic metadata null values can be
removed from state and client memory.
---
 .changelog/18664.txt         |  3 ++
 client/meta_endpoint.go      | 10 +++++
 client/meta_endpoint_test.go | 85 ++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)
 create mode 100644 .changelog/18664.txt

diff --git a/.changelog/18664.txt b/.changelog/18664.txt
new file mode 100644
index 000000000..8359cb6fe
--- /dev/null
+++ b/.changelog/18664.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+client: ensure null dynamic node metadata values are removed from memory
+```
diff --git a/client/meta_endpoint.go b/client/meta_endpoint.go
index 7336747e5..099bcd20f 100644
--- a/client/meta_endpoint.go
+++ b/client/meta_endpoint.go
@@ -45,6 +45,16 @@ func (n *NodeMeta) Apply(args *structs.NodeMetaApplyRequest, reply *structs.Node
 		dyn = maps.Clone(n.c.metaDynamic)
 		maps.Copy(dyn, args.Meta)
 
+		// Delete null values from the dynamic metadata if they are also not
+		// static. Static null values must be kept so their removal is
+		// persisted in client state.
+		for k, v := range args.Meta {
+			_, static := n.c.metaStatic[k]
+			if v == nil && !static {
+				delete(dyn, k)
+			}
+		}
+
 		if stateErr = n.c.stateDB.PutNodeMeta(dyn); stateErr != nil {
 			return
 		}
diff --git a/client/meta_endpoint_test.go b/client/meta_endpoint_test.go
index 6b2778be7..ac93aa48b 100644
--- a/client/meta_endpoint_test.go
+++ b/client/meta_endpoint_test.go
@@ -96,3 +96,88 @@ func TestNodeMeta_Validation(t *testing.T) {
 	err = c1.ClientRPC("NodeMeta.Apply", applyReq, &resp)
 	must.ErrorContains(t, err, "*")
 }
+
+func TestNodeMeta_unset(t *testing.T) {
+	ci.Parallel(t)
+
+	s, cleanupS := nomad.TestServer(t, nil)
+	defer cleanupS()
+	testutil.WaitForLeader(t, s.RPC)
+
+	c1, cleanup := TestClient(t, func(c *config.Config) {
+		c.Servers = []string{s.GetConfig().RPCAddr.String()}
+		c.Node.Meta["static_meta"] = "true"
+	})
+	defer cleanup()
+
+	// Set dynamic node metadata.
+	applyReq := &structs.NodeMetaApplyRequest{
+		NodeID: c1.NodeID(),
+		Meta: map[string]*string{
+			"dynamic_meta": pointer.Of("true"),
+		},
+	}
+	var resp structs.NodeMetaResponse
+	err := c1.ClientRPC("NodeMeta.Apply", applyReq, &resp)
+	must.NoError(t, err)
+
+	// Check static_meta:
+	//   1. must be present in Static.
+	//   2. must be present in Meta
+	must.Eq(t, resp.Static["static_meta"], "true")
+	must.Eq(t, resp.Meta["static_meta"], "true")
+
+	// Check dynamic_meta:
+	//   1. must be present in Dynamic.
+	//   2. must be present in Meta
+	must.Eq(t, *resp.Dynamic["dynamic_meta"], "true")
+	must.Eq(t, resp.Meta["dynamic_meta"], "true")
+
+	// Unset static node metada.
+	applyReq = &structs.NodeMetaApplyRequest{
+		NodeID: c1.NodeID(),
+		Meta: map[string]*string{
+			"static_meta": nil,
+		},
+	}
+	err = c1.ClientRPC("NodeMeta.Apply", applyReq, &resp)
+	must.NoError(t, err)
+
+	// Check static_meta:
+	//   1. must be present in Static.
+	//   2. must not be present in Meta
+	//   3. must be nil in Dynamic to persist its removal even on restarts.
+	must.Eq(t, resp.Static["static_meta"], "true")
+	must.MapNotContainsKey(t, resp.Meta, "static_meta")
+	must.Nil(t, resp.Dynamic["static_meta"])
+
+	// Check dynamic_meta:
+	//   1. must be present in Dynamic.
+	//   2. must be present in Meta
+	must.Eq(t, *resp.Dynamic["dynamic_meta"], "true")
+	must.Eq(t, resp.Meta["dynamic_meta"], "true")
+
+	// Unset dynamic node metada.
+	applyReq = &structs.NodeMetaApplyRequest{
+		NodeID: c1.NodeID(),
+		Meta: map[string]*string{
+			"dynamic_meta": nil,
+		},
+	}
+	err = c1.ClientRPC("NodeMeta.Apply", applyReq, &resp)
+	must.NoError(t, err)
+
+	// Check static_meta:
+	//   1. must be present in Static.
+	//   2. must not be present in Meta
+	//   3. must be nil in Dynamic to persist its removal even on restarts.
+	must.Eq(t, resp.Static["static_meta"], "true")
+	must.MapNotContainsKey(t, resp.Meta, "static_meta")
+	must.Nil(t, resp.Dynamic["static_meta"])
+
+	// Check dynamic_meta:
+	//   1. must not be present in Dynamic.
+	//   2. must not be present in Meta
+	must.MapNotContainsKey(t, resp.Dynamic, "dynamic_meta")
+	must.MapNotContainsKey(t, resp.Meta, "dynamic_meta")
+}