Improve parsing error messages for strategies.yaml.

mgmt/Makefile.am

@@ -34,6 +34,7 @@ AM_CPPFLAGS += \
 	-I$(abs_top_srcdir)/proxy \
 	-I$(abs_top_srcdir)/proxy/http \
 	-I$(abs_top_srcdir)/proxy/hdrs \
+	-I$(abs_top_srcdir)/lib/yamlcpp/include \
 	$(TS_INCLUDES)
 
 libmgmt_c_la_SOURCES = \
@@ -51,7 +52,9 @@ libmgmt_p_la_SOURCES = \
 	ProcessManager.cc \
 	ProcessManager.h \
 	ProxyConfig.cc \
-	ProxyConfig.h
+	ProxyConfig.h \
+	YamlCfg.cc \
+	YamlCfg.h
 
 libmgmt_lm_la_SOURCES = \
 	$(libmgmt_COMMON) \

mgmt/YamlCfg.cc

@@ -0,0 +1,83 @@
+/** @file
+
+  Implementation file for YamlCfg.h.
+
+  @section license License
+
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+ */
+
+#include "YamlCfg.h"
+
+#include <algorithm>
+#include <string>
+
+#include <tscore/ink_assert.h>
+
+namespace ts
+{
+namespace Yaml
+{
+  Map::Map(YAML::Node map) : _map{map}
+  {
+    if (!_map.IsMap()) {
+      throw YAML::ParserException(_map.Mark(), "map expected");
+    }
+  }
+
+  YAML::Node
+  Map::operator[](std::string_view key)
+  {
+    auto n = _map[std::string(key)];
+
+    if (n) {
+      // Add key to _used_key if not in it already.
+      //
+      if (std::find(_used_key.begin(), _used_key.end(), key) == _used_key.end()) {
+        _used_key.push_back(key);
+      }
+    }
+
+    return n;
+  }
+
+  void
+  Map::done()
+  {
+    if (!_bad && (_used_key.size() != _map.size())) {
+      ink_assert(_used_key.size() < _map.size());
+
+      std::string msg{(_map.size() - _used_key.size()) > 1 ? "keys " : "key "};
+      bool first{true};
+
+      for (auto const &kv : _map) {
+        auto key = kv.first.as<std::string>();
+
+        if (std::find(_used_key.begin(), _used_key.end(), key) == _used_key.end()) {
+          if (!first) {
+            msg += ", ";
+          }
+          first = false;
+          msg += key;
+        }
+      }
+      throw YAML::ParserException(_map.Mark(), msg + " invalid in this map");
+    }
+  }
+
+} // end namespace Yaml
+} // end namespace ts

mgmt/YamlCfg.h

@@ -0,0 +1,77 @@
+/** @file
+
+  Utilites to help with parsing YAML files with good error reporting.
+
+  @section license License
+
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+ */
+
+#pragma once
+
+#include <vector>
+#include <string_view>
+
+#include <yaml-cpp/yaml.h>
+
+namespace ts
+{
+namespace Yaml
+{
+  // A class that is a wrapper for a YAML::Node that corresponds to a map in a YAML input file.
+  // It's purpose is to make sure all keys in the map are processed.
+  //
+  class Map
+  {
+  public:
+    // A YAML::ParserException will be thrown if 'map' isn't actually a map.
+    //
+    explicit Map(YAML::Node map);
+
+    // Get the node for a key.  Throw a YAML::Exception if 'key' is not in the map.  The node for each key in the
+    // map must be gotten at least once.  The lifetime of the char array referenced by passed key must be as long
+    // as this instance.
+    //
+    YAML::Node operator[](std::string_view key);
+
+    // Call this after the last call to the [] operator.  Will throw a YAML::ParserException if instance not
+    // already marked bad, and all keys in the map were not accessed at least once with the [] operator.  The
+    // 'what' of the exception will list the keys that were not accessed as invalid for the map.
+    //
+    void done();
+
+    // Mark instance as bad.
+    //
+    void
+    bad()
+    {
+      _bad = true;
+    }
+
+    // No copy/move.
+    //
+    Map(Map const &) = delete;
+    Map &operator=(Map const &) = delete;
+
+  private:
+    YAML::Node _map;
+    std::vector<std::string_view> _used_key;
+    bool _bad{false};
+  };
+
+} // end namespace Yaml
+} // end namespace ts

proxy/http/remap/NextHopConsistentHash.cc

@@ -26,6 +26,7 @@
 #include "tscore/HashSip.h"
 #include "HttpSM.h"
 #include "I_Machine.h"
+#include "YamlCfg.h"
 #include "NextHopConsistentHash.h"
 
 // hash_key strings.
@@ -63,7 +64,7 @@ NextHopConsistentHash::~NextHopConsistentHash()
 }
 
 bool
-NextHopConsistentHash::Init(const YAML::Node &n)
+NextHopConsistentHash::Init(ts::Yaml::Map &n)
 {
   ATSHash64Sip24 hash;
 

proxy/http/remap/NextHopConsistentHash.h

@@ -48,6 +48,6 @@ class NextHopConsistentHash : public NextHopSelectionStrategy
   NextHopConsistentHash() = delete;
   NextHopConsistentHash(const std::string_view name, const NHPolicyType &policy) : NextHopSelectionStrategy(name, policy) {}
   ~NextHopConsistentHash();
-  bool Init(const YAML::Node &n);
+  bool Init(ts::Yaml::Map &n);
   void findNextHop(TSHttpTxn txnp, void *ih = nullptr, time_t now = 0) override;
 };

proxy/http/remap/NextHopRoundRobin.h

@@ -36,7 +36,7 @@ class NextHopRoundRobin : public NextHopSelectionStrategy
   NextHopRoundRobin(const std::string_view &name, const NHPolicyType &policy) : NextHopSelectionStrategy(name, policy) {}
   ~NextHopRoundRobin();
   bool
-  Init(const YAML::Node &n)
+  Init(ts::Yaml::Map &n)
   {
     return NextHopSelectionStrategy::Init(n);
   }

proxy/http/remap/NextHopSelectionStrategy.cc

@@ -21,7 +21,10 @@
   limitations under the License.
  */
 
+#include <optional>
+
 #include <yaml-cpp/yaml.h>
+#include <YamlCfg.h>
 #include "I_Machine.h"
 #include "HttpSM.h"
 #include "NextHopSelectionStrategy.h"
@@ -57,7 +60,7 @@ NextHopSelectionStrategy::NextHopSelectionStrategy(const std::string_view &name,
 // parse out the data for this strategy.
 //
 bool
-NextHopSelectionStrategy::Init(const YAML::Node &n)
+NextHopSelectionStrategy::Init(ts::Yaml::Map &n)
 {
   NH_Debug(NH_DEBUG_TAG, "calling Init()");
 
@@ -95,9 +98,9 @@ NextHopSelectionStrategy::Init(const YAML::Node &n)
     }
 
     // failover node.
-    YAML::Node failover_node;
-    if (n["failover"]) {
-      failover_node = n["failover"];
+    YAML::Node failover_node_n = n["failover"];
+    if (failover_node_n) {
+      ts::Yaml::Map failover_node{failover_node_n};
       if (failover_node["ring_mode"]) {
         auto ring_mode_val = failover_node["ring_mode"].Scalar();
         if (ring_mode_val == alternate_rings) {
@@ -174,12 +177,12 @@ NextHopSelectionStrategy::Init(const YAML::Node &n)
           }
         }
       }
+      failover_node.done();
     }
 
     // parse and load the host data
-    YAML::Node groups_node;
-    if (n["groups"]) {
-      groups_node = n["groups"];
+    YAML::Node groups_node = n["groups"];
+    if (groups_node) {
       // a groups list is required.
       if (groups_node.Type() != YAML::NodeType::Sequence) {
         throw std::invalid_argument("Invalid groups definition, expected a sequence, '" + strategy_name + "' cannot be loaded.");
@@ -224,7 +227,7 @@ NextHopSelectionStrategy::Init(const YAML::Node &n)
       }
     }
   } catch (std::exception &ex) {
-    NH_Note("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(), ex.what());
+    NH_Error("Error parsing the strategy named '%s' due to '%s', this strategy will be ignored.", strategy_name.c_str(), ex.what());
     return false;
   }
 
@@ -296,26 +299,26 @@ template <> struct convert<HostRecord> {
   static bool
   decode(const Node &node, HostRecord &nh)
   {
-    YAML::Node nd;
-    bool merge_tag_used = false;
+    ts::Yaml::Map map{node};
+    ts::Yaml::Map *mmap{&map};
+    std::optional<ts::Yaml::Map> mergeable_map;
 
     // check for YAML merge tag.
-    if (node["<<"]) {
-      nd             = node["<<"];
-      merge_tag_used = true;
-    } else {
-      nd = node;
+    YAML::Node mergeable_map_n = map["<<"];
+    if (mergeable_map_n) {
+      mergeable_map.emplace(mergeable_map_n);
+      mmap = &mergeable_map.value();
     }
 
     // lookup the hostname
-    if (nd["host"]) {
-      nh.hostname = nd["host"].Scalar();
+    if ((*mmap)["host"]) {
+      nh.hostname = (*mmap)["host"].Scalar();
     } else {
       throw std::invalid_argument("Invalid host definition, missing host name.");
     }
 
     // lookup the port numbers supported by this host.
-    YAML::Node proto = nd["protocol"];
+    YAML::Node proto = (*mmap)["protocol"];
 
     if (proto.Type() != YAML::NodeType::Sequence) {
       throw std::invalid_argument("Invalid host protocol definition, expected a sequence.");
@@ -327,24 +330,34 @@ template <> struct convert<HostRecord> {
       }
     }
 
-    // get the host's weight
-    YAML::Node weight;
-    if (merge_tag_used) {
-      weight    = node["weight"];
-      nh.weight = weight.as<float>();
-    } else if ((weight = nd["weight"])) {
+    // get the host's weight, allowing override of weight in merged map
+    YAML::Node weight = map["weight"];
+    if (mmap != &map) {
+      // weight must always be looked up in the merged map, even if overridden, so it's presence will not
+      // cause an exception when mmap->done() is called
+      YAML::Node w = (*mmap)["weight"];
+      if (!weight) {
+        weight = w;
+      }
+    }
+    if (weight) {
       nh.weight = weight.as<float>();
     } else {
       NH_Note("No weight is defined for the host '%s', using default 1.0", nh.hostname.data());
       nh.weight = 1.0;
     }
 
     // get the host's optional hash_string
-    YAML::Node hash;
-    if ((hash = nd["hash_string"])) {
+    YAML::Node hash{(*mmap)["hash_string"]};
+    if (hash) {
       nh.hash_string = hash.Scalar();
     }
 
+    map.done();
+    if (mmap != &map) {
+      mmap->done();
+    }
+
     return true;
   }
 };
@@ -353,21 +366,27 @@ template <> struct convert<NHProtocol> {
   static bool
   decode(const Node &node, NHProtocol &nh)
   {
-    if (node["scheme"]) {
-      if (node["scheme"].Scalar() == "http") {
+    ts::Yaml::Map map{node};
+
+    if (map["scheme"]) {
+      if (map["scheme"].Scalar() == "http") {
         nh.scheme = NH_SCHEME_HTTP;
-      } else if (node["scheme"].Scalar() == "https") {
+      } else if (map["scheme"].Scalar() == "https") {
         nh.scheme = NH_SCHEME_HTTPS;
       } else {
         nh.scheme = NH_SCHEME_NONE;
       }
     }
-    if (node["port"]) {
-      nh.port = node["port"].as<int>();
+    if (map["port"]) {
+      nh.port = map["port"].as<int>();
+      if ((nh.port <= 0) || (nh.port > 65535)) {
+        throw YAML::ParserException(map["port"].Mark(), "port number must be in (inclusive) range 0 - 65,536");
+      }
     }
-    if (node["health_check_url"]) {
-      nh.health_check_url = node["health_check_url"].Scalar();
+    if (map["health_check_url"]) {
+      nh.health_check_url = map["health_check_url"].Scalar();
     }
+    map.done();
     return true;
   }
 };