m/n/core/curator: add thin etcd storage abstraction
This implements etcdPrefix, a more formalized way to represent objects
stored within etcd under some unique ID key.
This ensures any time objects are retrieved by key they are not
accidentally traversing /-delimited 'path' elements, and implements the
mildly complex range start/end computation operation for when all
objects from within a prefix must retrieved.
Change-Id: Ib095f466faaf453b5f61a35642df6b0c1076ae05
Reviewed-on: https://review.monogon.dev/c/monogon/+/322
Reviewed-by: Lorenz Brun <lorenz@monogon.tech>
diff --git a/metropolis/node/core/curator/BUILD.bazel b/metropolis/node/core/curator/BUILD.bazel
index 25be134..40f7740 100644
--- a/metropolis/node/core/curator/BUILD.bazel
+++ b/metropolis/node/core/curator/BUILD.bazel
@@ -11,6 +11,7 @@
"impl_leader_curator.go",
"impl_leader_management.go",
"listener.go",
+ "state.go",
"state_node.go",
"state_pki.go",
],
@@ -49,6 +50,7 @@
"curator_test.go",
"impl_leader_test.go",
"listener_test.go",
+ "state_test.go",
],
embed = [":go_default_library"],
deps = [
diff --git a/metropolis/node/core/curator/bootstrap.go b/metropolis/node/core/curator/bootstrap.go
index c50cbbd..98d176a 100644
--- a/metropolis/node/core/curator/bootstrap.go
+++ b/metropolis/node/core/curator/bootstrap.go
@@ -62,7 +62,10 @@
// once. It's guaranteed to either succeed fully or fail fully, without leaving
// the cluster in an inconsistent state.
func BootstrapFinish(ctx context.Context, etcd client.Namespaced, initialNode *Node, pubkey []byte) error {
- nodeKey := initialNode.etcdPath()
+ nodeKey, err := initialNode.etcdPath()
+ if err != nil {
+ return fmt.Errorf("failed to get node key: %w", err)
+ }
nodeRaw, err := proto.Marshal(initialNode.proto())
if err != nil {
return fmt.Errorf("failed to marshal node: %w", err)
diff --git a/metropolis/node/core/curator/impl_leader_curator.go b/metropolis/node/core/curator/impl_leader_curator.go
index 34dd62b..8d14582 100644
--- a/metropolis/node/core/curator/impl_leader_curator.go
+++ b/metropolis/node/core/curator/impl_leader_curator.go
@@ -30,7 +30,10 @@
// there's also no need to filter anything.
// TODO(q3k): formalize and strongly type etcd paths for cluster state?
// Probably worth waiting for type parameters before attempting to do that.
- nodePath := nodeEtcdPath(nodeID)
+ nodePath, err := nodeEtcdPrefix.Key(nodeID)
+ if err != nil {
+ return status.Errorf(codes.InvalidArgument, "invalid node name: %v", err)
+ }
value := etcd.NewValue(l.etcd, nodePath, func(data []byte) (interface{}, error) {
return nodeUnmarshal(data)
diff --git a/metropolis/node/core/curator/state.go b/metropolis/node/core/curator/state.go
new file mode 100644
index 0000000..d460e4f
--- /dev/null
+++ b/metropolis/node/core/curator/state.go
@@ -0,0 +1,85 @@
+package curator
+
+import (
+ "fmt"
+ "strings"
+
+ "go.etcd.io/etcd/clientv3"
+)
+
+// etcdPrefix is the location of some data in etcd, with each data element keyed
+// by some unique string key.
+//
+// Prefixes are /-delimited filesystem-like paths, eg. prefix `/foo/bar/` could
+// contain an item with ID `one` at etcd key `/foo/bar/one`, and an item with ID
+// `two` at etcd key `/foo/bar/two`.
+//
+// Given etcd's lexicographic range operation, this allows for retrieval of all
+// items under a given prefix.
+//
+// An etcdPrefix should be built using the newEtcdPrefix function.
+type etcdPrefix struct {
+ // parts are the parts of an etcd prefix path, eg. for prefix '/foo/bar/' parts
+ // would be ["foo", "bar"].
+ parts []string
+}
+
+// newEtcdPrefix returns an etcdPrefix for the given string representation of a
+// prefix. The string representation must start and end with a / character, and
+// must contain at least one path component (ie. one /-delimited component) with
+// no empty components.
+func newEtcdPrefix(p string) (*etcdPrefix, error) {
+ parts := strings.Split(p, "/")
+ if len(parts) < 3 {
+ return nil, fmt.Errorf("invalid path: must contain at least one component")
+ }
+ // Expect ["", "foo", "bar", ""] for /foo/bar/. Ie., everything but the first
+ // and part path part must be non-empty, and the first and last path parts must
+ // be empty.
+ for i, p := range parts {
+ if i == 0 || i == len(parts)-1 {
+ if p != "" {
+ return nil, fmt.Errorf("invalid path: must start and end with a /")
+ }
+ } else {
+ if p == "" {
+ return nil, fmt.Errorf("invalid path: must not contain repeated / characters")
+ }
+ }
+ }
+
+ // Omit the leading and trailing parts, ie. keep ["foo", "bar"] for /foo/bar/.
+ parts = parts[1 : len(parts)-1]
+ return &etcdPrefix{parts}, nil
+}
+
+// mustNewEtcdPrefix returns an etcdPrefix given the string representation of a
+// path, or panics if the string representation is invalid (see newEtcdPrefix
+// for more information).
+func mustNewEtcdPrefix(p string) etcdPrefix {
+ res, err := newEtcdPrefix(p)
+ if err != nil {
+ panic(err)
+ }
+ return *res
+}
+
+// Key returns the key for an item within an etcdPrefix, or an error if the
+// given ID is invalid (ie. contains a / character).
+func (p etcdPrefix) Key(id string) (string, error) {
+ if strings.Contains(id, "/") {
+ return "", fmt.Errorf("invalid id: cannot contain / character")
+ }
+ path := append(p.parts, id)
+ return "/" + strings.Join(path, "/"), nil
+}
+
+// Range returns an etcd clientv3.Op that represents a Range Get request over
+// all the keys within this etcdPrefix.
+func (p etcdPrefix) Range() clientv3.Op {
+ // Range from /foo/bar/ ... to /foo/bar0 ('0' is one ASCII codepoint after '/').
+ start := "/" + strings.Join(p.parts, "/") + "/"
+ end := "/" + strings.Join(p.parts, "/") + "0"
+
+ return clientv3.OpGet(start, clientv3.WithRange(end))
+}
diff --git a/metropolis/node/core/curator/state_node.go b/metropolis/node/core/curator/state_node.go
index 6668b19..4e2535f 100644
--- a/metropolis/node/core/curator/state_node.go
+++ b/metropolis/node/core/curator/state_node.go
@@ -126,18 +126,14 @@
return &kw
}
-const (
- nodeEtcdPrefix = "/nodes/"
+var (
+ nodeEtcdPrefix = mustNewEtcdPrefix("/nodes/")
)
-func nodeEtcdPath(id string) string {
- return fmt.Sprintf("%s%s", nodeEtcdPrefix, id)
-}
-
// etcdPath builds the etcd path in which this node's protobuf-serialized state
// is stored in etcd.
-func (n *Node) etcdPath() string {
- return nodeEtcdPath(n.ID())
+func (n *Node) etcdPath() (string, error) {
+ return nodeEtcdPrefix.Key(n.ID())
}
// proto serializes the Node object into protobuf, to be used for saving to
diff --git a/metropolis/node/core/curator/state_test.go b/metropolis/node/core/curator/state_test.go
new file mode 100644
index 0000000..8b8152a
--- /dev/null
+++ b/metropolis/node/core/curator/state_test.go
@@ -0,0 +1,59 @@
+package curator
+
+import "testing"
+
+func TestEtcdPrefixParse(t *testing.T) {
+ for i, te := range []struct {
+ p string
+ ok bool
+ }{
+ {"/foo/", true},
+ {"/foo/bar/", true},
+
+ {"/foo//", false},
+ {"/foo//bar/", false},
+ {"/foo/bar", false},
+ {"foo", false},
+ {"foo/", false},
+ {"foo/bar", false},
+ } {
+ _, err := newEtcdPrefix(te.p)
+ if te.ok {
+ if err != nil {
+ t.Errorf("Case %d: wanted nil, got err %v", i, err)
+ }
+ } else {
+ if err == nil {
+ t.Errorf("Case %d: wanted err, got nil", i)
+ }
+ }
+ }
+}
+
+func TestEtcdPrefixKeyRange(t *testing.T) {
+ p := mustNewEtcdPrefix("/foo/")
+
+ // Test Key() functionality.
+ key, err := p.Key("bar")
+ if err != nil {
+ t.Fatalf("Key(bar): %v", err)
+ }
+ if want, got := "/foo/bar", key; want != got {
+ t.Errorf("Wrong key, wanted %q, got %q", want, got)
+ }
+
+ // Test Key() with invalid ID.
+ _, err = p.Key("bar/baz")
+ if err == nil {
+ t.Error("Key(bar/baz) returned nil, wanted error")
+ }
+
+ // Test Range() functionality.
+ op := p.Range()
+ if want, got := "/foo/", string(op.KeyBytes()); want != got {
+ t.Errorf("Wrong start key, wanted %q, got %q", want, got)
+ }
+ if want, got := "/foo0", string(op.RangeBytes()); want != got {
+ t.Errorf("Wrong end key, wanted %q, got %q", want, got)
+ }
+}