Refactor Capability(Set) and add more testing

Hide all nitty-gritty details of how capabilities map to internal
bit set positions by making more of Capability private and only
allowing CapabilitySet to see how the sausages are made. Move all
bit set functionality to CapabilitySet, where it really belongs.

8 files changed, 127 insertions, 75 deletions

diff --git a/fnet/src/vespa/fnet/frt/CMakeLists.txt b/fnet/src/vespa/fnet/frt/CMakeLists.txt
index fa9623b950a..329c6c8fa57 100644
--- a/fnet/src/vespa/fnet/frt/CMakeLists.txt
+++ b/fnet/src/vespa/fnet/frt/CMakeLists.txt
@@ -5,7 +5,7 @@ vespa_add_library(fnet_frt OBJECT
     invoker.cpp
     packets.cpp
     reflection.cpp
-    require_capability.cpp
+    require_capabilities.cpp
     rpcrequest.cpp
     supervisor.cpp
     target.cpp
diff --git a/fnet/src/vespa/fnet/frt/require_capability.cpp b/fnet/src/vespa/fnet/frt/require_capabilities.cpp
index 5c64c2bb123..c74e9ad648a 100644
--- a/fnet/src/vespa/fnet/frt/require_capability.cpp
+++ b/fnet/src/vespa/fnet/frt/require_capabilities.cpp
@@ -1,12 +1,12 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
-#include "require_capability.h"
+#include "require_capabilities.h"
 #include "rpcrequest.h"
 #include <vespa/fnet/connection.h>
 #include <vespa/vespalib/net/connection_auth_context.h>
 
 bool
-FRT_RequireCapability::allow(FRT_RPCRequest& req) const noexcept
+FRT_RequireCapabilities::allow(FRT_RPCRequest& req) const noexcept
 {
     const auto& auth_ctx = req.GetConnection()->auth_context();
     return auth_ctx.capabilities().contains_all(_required_capabilities);
diff --git a/fnet/src/vespa/fnet/frt/require_capability.h b/fnet/src/vespa/fnet/frt/require_capabilities.h
index c9eaf4937a8..7c80484783d 100644
--- a/fnet/src/vespa/fnet/frt/require_capability.h
+++ b/fnet/src/vespa/fnet/frt/require_capabilities.h
@@ -9,10 +9,10 @@
  * context that contains, at minimum, a given set of capabilities. If one or more
  * required capabilities are missing, the request is denied.
  */
-class FRT_RequireCapability final : public FRT_RequestAccessFilter {
+class FRT_RequireCapabilities final : public FRT_RequestAccessFilter {
     vespalib::net::tls::CapabilitySet _required_capabilities;
 public:
-    explicit constexpr FRT_RequireCapability(vespalib::net::tls::CapabilitySet required_capabilities) noexcept
+    explicit constexpr FRT_RequireCapabilities(vespalib::net::tls::CapabilitySet required_capabilities) noexcept
         : _required_capabilities(required_capabilities)
     {
     }
diff --git a/vespalib/src/tests/net/tls/capabilities/capabilities_test.cpp b/vespalib/src/tests/net/tls/capabilities/capabilities_test.cpp
index 5f74bdceff4..4a20200c631 100644
--- a/vespalib/src/tests/net/tls/capabilities/capabilities_test.cpp
+++ b/vespalib/src/tests/net/tls/capabilities/capabilities_test.cpp
@@ -7,12 +7,6 @@ using namespace vespalib;
 using namespace vespalib::net::tls;
 using namespace std::string_view_literals;
 
-TEST("Capability bit positions are stable across calls") {
-    auto cap1 = Capability::content_storage_api();
-    auto cap2 = Capability::content_storage_api();
-    EXPECT_EQUAL(cap1.id_bit_pos(), cap2.id_bit_pos());
-}
-
 TEST("Capability instances are equality comparable") {
     auto cap1 = Capability::content_document_api();
     auto cap2 = Capability::content_document_api();
@@ -22,6 +16,27 @@ TEST("Capability instances are equality comparable") {
     EXPECT_NOT_EQUAL(cap1, cap3);
 }
 
+TEST("CapabilitySet instances are equality comparable") {
+    const auto cap1 = Capability::content_document_api();
+    const auto cap2 = Capability::content_search_api();
+
+    const auto all_caps = CapabilitySet::make_with_all_capabilities();
+    const auto set_12_a = CapabilitySet::of({cap1, cap2});
+    const auto set_12_b = CapabilitySet::of({cap1, cap2});
+    const auto set_1  = CapabilitySet::of({cap1});
+    const auto empty  = CapabilitySet::make_empty();
+
+    EXPECT_EQUAL(all_caps, all_caps);
+    EXPECT_EQUAL(empty, empty);
+    EXPECT_EQUAL(set_12_a, set_12_b);
+    EXPECT_EQUAL(set_12_b, set_12_a);
+
+    EXPECT_NOT_EQUAL(all_caps, empty);
+    EXPECT_NOT_EQUAL(set_12_a, set_1);
+    EXPECT_NOT_EQUAL(set_12_a, all_caps);
+    EXPECT_NOT_EQUAL(set_1, empty);
+}
+
 TEST("Can get underlying name of all Capability instances") {
     EXPECT_EQUAL(Capability::content_storage_api().name(),  "vespa.content.storage_api"sv);
     EXPECT_EQUAL(Capability::content_document_api().name(), "vespa.content.document_api"sv);
@@ -37,14 +52,31 @@ TEST("Capability instances can be stringified") {
     EXPECT_EQUAL(Capability::content_storage_api().to_string(), "Capability(vespa.content.storage_api)");
 }
 
+namespace {
+
+void check_capability_mapping(const std::string& name, Capability expected) {
+    auto cap = Capability::find_capability(name);
+    ASSERT_TRUE(cap.has_value());
+    EXPECT_EQUAL(*cap, expected);
+}
+
+void check_capability_set_mapping(const std::string& name, CapabilitySet expected) {
+    auto caps = CapabilitySet::find_capability_set(name);
+    ASSERT_TRUE(caps.has_value());
+    EXPECT_EQUAL(*caps, expected);
+}
+
+}
+
 TEST("All known capabilities can be looked up by name") {
-    EXPECT_TRUE(Capability::find_capability("vespa.content.storage_api").has_value());
-    EXPECT_TRUE(Capability::find_capability("vespa.content.document_api").has_value());
-    EXPECT_TRUE(Capability::find_capability("vespa.content.search_api").has_value());
-    EXPECT_TRUE(Capability::find_capability("vespa.content.cluster_controller.internal_state_api").has_value());
-    EXPECT_TRUE(Capability::find_capability("vespa.slobrok.api").has_value());
-    EXPECT_TRUE(Capability::find_capability("vespa.content.status_pages").has_value());
-    EXPECT_TRUE(Capability::find_capability("vespa.content.metrics_api").has_value());
+    check_capability_mapping("vespa.content.storage_api",  Capability::content_storage_api());
+    check_capability_mapping("vespa.content.document_api", Capability::content_document_api());
+    check_capability_mapping("vespa.content.search_api",   Capability::content_search_api());
+    check_capability_mapping("vespa.slobrok.api",          Capability::slobrok_api());
+    check_capability_mapping("vespa.content.status_pages", Capability::content_status_pages());
+    check_capability_mapping("vespa.content.metrics_api",  Capability::content_metrics_api());
+    check_capability_mapping("vespa.content.cluster_controller.internal_state_api",
+                             Capability::content_cluster_controller_internal_state_api());
 }
 
 TEST("Unknown capability name returns nullopt") {
@@ -57,11 +89,11 @@ TEST("CapabilitySet instances can be stringified") {
 }
 
 TEST("All known capability sets can be looked up by name") {
-    EXPECT_TRUE(CapabilitySet::find_capability_set("vespa.content_node").has_value());
-    EXPECT_TRUE(CapabilitySet::find_capability_set("vespa.container_node").has_value());
-    EXPECT_TRUE(CapabilitySet::find_capability_set("vespa.telemetry").has_value());
-    EXPECT_TRUE(CapabilitySet::find_capability_set("vespa.cluster_controller_node").has_value());
-    EXPECT_TRUE(CapabilitySet::find_capability_set("vespa.config_server").has_value());
+    check_capability_set_mapping("vespa.content_node",            CapabilitySet::content_node());
+    check_capability_set_mapping("vespa.container_node",          CapabilitySet::container_node());
+    check_capability_set_mapping("vespa.telemetry",               CapabilitySet::telemetry());
+    check_capability_set_mapping("vespa.cluster_controller_node", CapabilitySet::cluster_controller_node());
+    check_capability_set_mapping("vespa.config_server",           CapabilitySet::config_server());
 }
 
 TEST("Unknown capability set name returns nullopt") {
@@ -96,6 +128,18 @@ TEST("Resolving an unknown capability set returns false and does not add anythin
     EXPECT_TRUE(caps.empty());
 }
 
+TEST("Resolving multiple capabilities/sets adds union of capabilities") {
+    CapabilitySet caps;
+    EXPECT_TRUE(caps.resolve_and_add("vespa.content_node"));   // CapabilitySet
+    EXPECT_TRUE(caps.resolve_and_add("vespa.container_node")); // ditto
+    EXPECT_EQUAL(caps, CapabilitySet::of({Capability::content_storage_api(), Capability::content_document_api(),
+                                          Capability::slobrok_api(), Capability::content_search_api()}));
+    EXPECT_TRUE(caps.resolve_and_add("vespa.content.metrics_api")); // Capability (single)
+    EXPECT_EQUAL(caps, CapabilitySet::of({Capability::content_storage_api(), Capability::content_document_api(),
+                                          Capability::slobrok_api(), Capability::content_search_api(),
+                                          Capability::content_metrics_api()}));
+}
+
 TEST("Default-constructed CapabilitySet has no capabilities") {
     CapabilitySet caps;
     EXPECT_EQUAL(caps.count(), 0u);
@@ -105,7 +149,7 @@ TEST("Default-constructed CapabilitySet has no capabilities") {
 
 TEST("CapabilitySet can be created with all capabilities") {
     auto caps = CapabilitySet::make_with_all_capabilities();
-    EXPECT_EQUAL(caps.count(), max_capability_bit_count());
+    EXPECT_EQUAL(caps.count(), CapabilitySet::max_count());
     EXPECT_TRUE(caps.contains(Capability::content_storage_api()));
     EXPECT_TRUE(caps.contains(Capability::content_metrics_api()));
     // ... we just assume the rest are present as well.
diff --git a/vespalib/src/vespa/vespalib/net/tls/capability.cpp b/vespalib/src/vespa/vespalib/net/tls/capability.cpp
index e114ea174b9..64de250e60d 100644
--- a/vespalib/src/vespa/vespalib/net/tls/capability.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/capability.cpp
@@ -12,7 +12,7 @@ namespace {
 using namespace std::string_view_literals;
 
 // Important: must match 1-1 with CapabilityId values!
-constexpr std::array<std::string_view, max_capability_bit_count()> capability_names = {
+constexpr std::array<std::string_view, Capability::max_value_count()> capability_names = {
     "vespa.content.storage_api"sv,
     "vespa.content.document_api"sv,
     "vespa.content.search_api"sv,
@@ -25,7 +25,7 @@ constexpr std::array<std::string_view, max_capability_bit_count()> capability_na
 } // anon ns
 
 std::string_view Capability::name() const noexcept {
-    return capability_names[id_bit_pos()];
+    return capability_names[id_as_idx()];
 }
 
 string Capability::to_string() const {
diff --git a/vespalib/src/vespa/vespalib/net/tls/capability.h b/vespalib/src/vespa/vespalib/net/tls/capability.h
index 67d8067977b..842e3f3a363 100644
--- a/vespalib/src/vespa/vespalib/net/tls/capability.h
+++ b/vespalib/src/vespa/vespalib/net/tls/capability.h
@@ -12,27 +12,6 @@ namespace vespalib { class asciistream; }
 
 namespace vespalib::net::tls {
 
-// Each ID value corresponds to a unique single-bit position.
-// These values shall never be exposed outside the running process, i.e. they
-// must be possible to change arbitrarily internally across versions.
-enum class CapabilityId : uint32_t {
-    ContentStorageApi = 0, // Must start at zero
-    ContentDocumentApi,
-    ContentSearchApi,
-    ContentClusterControllerInternalStateApi,
-    SlobrokApi,
-    ContentStatusPages,
-    ContentMetricsApi,
-    // When adding a capability ID to the end, max_capability_bit_count() MUST be updated
-};
-
-constexpr size_t max_capability_bit_count() noexcept {
-    // This must refer to the highest possible CapabilityId enum value.
-    return static_cast<size_t>(CapabilityId::ContentMetricsApi) + 1;
-}
-
-using CapabilityBitSet = std::bitset<max_capability_bit_count()>;
-
 /**
  * A capability represents the ability to access a distinct service or API
  * plane in Vespa (such as the Document API).
@@ -41,21 +20,41 @@ using CapabilityBitSet = std::bitset<max_capability_bit_count()>;
  */
 class Capability {
 private:
-    CapabilityId _cap_id;
-
-    constexpr explicit Capability(CapabilityId cap_id) noexcept : _cap_id(cap_id) {}
+    // Each ID value corresponds to a unique single-bit position.
+    // These values shall never be exposed outside the running process, i.e. they
+    // must be possible to change arbitrarily internally across versions.
+    enum class Id : uint32_t {
+        ContentStorageApi = 0, // Must start at zero
+        ContentDocumentApi,
+        ContentSearchApi,
+        ContentClusterControllerInternalStateApi,
+        SlobrokApi,
+        ContentStatusPages,
+        ContentMetricsApi,
+        // When adding a capability ID to the end, max_value_count() MUST be updated
+    };
 public:
-    Capability() = delete; // Only valid capabilities can be created.
+    constexpr static size_t max_value_count() noexcept {
+        // This must refer to the highest possible CapabilityId enum value.
+        return static_cast<size_t>(Id::ContentMetricsApi) + 1;
+    }
+private:
+    Id _cap_id;
 
-    constexpr CapabilityId id() const noexcept { return _cap_id; }
+    friend class CapabilitySet; // CapabilitySet needs to know the raw IDs for bit set bookkeeping
 
-    constexpr uint32_t id_bit_pos() const noexcept { return static_cast<uint32_t>(_cap_id); }
+    constexpr Id id() const noexcept { return _cap_id; }
+    constexpr uint32_t id_as_idx() const noexcept { return static_cast<uint32_t>(_cap_id); }
 
-    constexpr CapabilityBitSet id_as_bit_set() const noexcept {
-        static_assert(max_capability_bit_count() <= 32); // Must fit into uint32_t bitmask
-        return {uint32_t(1) << id_bit_pos()};
+    constexpr explicit Capability(Id cap_id) noexcept : _cap_id(cap_id) {}
+
+    constexpr static Capability of(Id id) noexcept {
+        return Capability(id);
     }
 
+public:
+    Capability() = delete; // Only valid capabilities can be created.
+
     constexpr bool operator==(const Capability& rhs) const noexcept {
         return (_cap_id == rhs._cap_id);
     }
@@ -67,38 +66,34 @@ public:
     std::string_view name() const noexcept;
     string to_string() const;
 
-    constexpr static Capability of(CapabilityId id) noexcept {
-        return Capability(id);
-    }
-
     static std::optional<Capability> find_capability(const string& cap_name) noexcept;
 
     constexpr static Capability content_storage_api() noexcept {
-        return Capability(CapabilityId::ContentStorageApi);
+        return Capability(Id::ContentStorageApi);
     }
 
     constexpr static Capability content_document_api() noexcept {
-        return Capability(CapabilityId::ContentDocumentApi);
+        return Capability(Id::ContentDocumentApi);
     }
 
     constexpr static Capability content_search_api() noexcept {
-        return Capability(CapabilityId::ContentSearchApi);
+        return Capability(Id::ContentSearchApi);
     }
 
     constexpr static Capability content_cluster_controller_internal_state_api() noexcept {
-        return Capability(CapabilityId::ContentClusterControllerInternalStateApi);
+        return Capability(Id::ContentClusterControllerInternalStateApi);
     }
 
     constexpr static Capability slobrok_api() noexcept {
-        return Capability(CapabilityId::SlobrokApi);
+        return Capability(Id::SlobrokApi);
     }
 
     constexpr static Capability content_status_pages() noexcept {
-        return Capability(CapabilityId::ContentStatusPages);
+        return Capability(Id::ContentStatusPages);
     }
 
     constexpr static Capability content_metrics_api() noexcept {
-        return Capability(CapabilityId::ContentMetricsApi);
+        return Capability(Id::ContentMetricsApi);
     }
 
 };
diff --git a/vespalib/src/vespa/vespalib/net/tls/capability_set.cpp b/vespalib/src/vespa/vespalib/net/tls/capability_set.cpp
index d0d25924960..3663694e31a 100644
--- a/vespalib/src/vespa/vespalib/net/tls/capability_set.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/capability_set.cpp
@@ -41,7 +41,7 @@ bool CapabilitySet::resolve_and_add(const string& set_or_cap_name) noexcept {
         _capability_mask |= cap_set->_capability_mask;
         return true;
     } else if (auto cap = Capability::find_capability(set_or_cap_name)) {
-        _capability_mask |= cap->id_as_bit_set();
+        _capability_mask |= cap_as_bit_set(*cap);
         return true;
     }
     return false;
@@ -77,7 +77,7 @@ CapabilitySet CapabilitySet::config_server() noexcept {
 }
 
 CapabilitySet CapabilitySet::make_with_all_capabilities() noexcept {
-    CapabilityBitSet bit_set;
+    BitSet bit_set;
     bit_set.flip(); // All cap bits set
     return CapabilitySet(bit_set);
 }
diff --git a/vespalib/src/vespa/vespalib/net/tls/capability_set.h b/vespalib/src/vespa/vespalib/net/tls/capability_set.h
index 99c78105924..f86b043ee7b 100644
--- a/vespalib/src/vespa/vespalib/net/tls/capability_set.h
+++ b/vespalib/src/vespa/vespalib/net/tls/capability_set.h
@@ -23,9 +23,19 @@ namespace vespalib::net::tls {
  * CapabilitySet instances are intended to be very cheap to pass and store by value.
  */
 class CapabilitySet {
-    CapabilityBitSet _capability_mask;
+    using BitSet = std::bitset<Capability::max_value_count()>;
+    BitSet _capability_mask;
 
-    explicit constexpr CapabilitySet(CapabilityBitSet capabilities) noexcept
+    constexpr static uint32_t cap_as_bit_pos(const Capability& cap) noexcept {
+        return cap.id_as_idx();
+    }
+
+    constexpr static BitSet cap_as_bit_set(const Capability& cap) noexcept {
+        static_assert(Capability::max_value_count() <= 32); // Must fit into uint32_t bitmask
+        return {uint32_t(1) << cap_as_bit_pos(cap)};
+    }
+
+    explicit constexpr CapabilitySet(BitSet capabilities) noexcept
         : _capability_mask(capabilities)
     {}
 public:
@@ -44,16 +54,19 @@ public:
     size_t count() const noexcept {
         return _capability_mask.count();
     }
+    constexpr static size_t max_count() noexcept {
+        return Capability::max_value_count();
+    }
 
     [[nodiscard]] constexpr bool contains(Capability cap) const noexcept {
-        return _capability_mask[cap.id_bit_pos()];
+        return _capability_mask[cap_as_bit_pos(cap)];
     }
     [[nodiscard]] bool contains_all(CapabilitySet caps) const noexcept {
         return ((_capability_mask & caps._capability_mask) == caps._capability_mask);
     }
 
     void add(const Capability& cap) noexcept {
-        _capability_mask |= cap.id_as_bit_set();
+        _capability_mask |= cap_as_bit_set(cap);
     }
     void add_all(const CapabilitySet& cap_set) noexcept {
         _capability_mask |= cap_set._capability_mask;
@@ -63,7 +76,7 @@ public:
     void for_each_capability(Func f) const noexcept(noexcept(f(Capability::content_storage_api()))) {
         for (size_t i = 0; i < _capability_mask.size(); ++i) {
             if (_capability_mask[i]) {
-                f(Capability::of(static_cast<CapabilityId>(i)));
+                f(Capability::of(static_cast<Capability::Id>(i)));
             }
         }
     }
@@ -83,7 +96,7 @@ public:
     static CapabilitySet of(std::initializer_list<Capability> caps) noexcept {
         CapabilitySet set;
         for (const auto& cap : caps) {
-            set._capability_mask |= cap.id_as_bit_set();
+            set._capability_mask |= cap_as_bit_set(cap);
         }
         return set;
     }