Separate ValidateConfiguration into the static and dynamic parts.

src/docker.cc

@@ -58,7 +58,7 @@ class DockerReader::NonRetriableError : public DockerReader::QueryException {
 DockerReader::DockerReader(const Configuration& config)
     : config_(config), environment_(config) {}
 
-void DockerReader::ValidateConfiguration() const
+void DockerReader::ValidateDynamicConfiguration() const
     throw(MetadataUpdater::ConfigurationValidationError) {
   const std::string container_filter(
       config_.DockerContainerFilter().empty()
@@ -230,10 +230,10 @@ json::value DockerReader::QueryDocker(const std::string& path) const
   }
 }
 
-void DockerUpdater::ValidateConfiguration() const
+void DockerUpdater::ValidateDynamicConfiguration() const
     throw(ConfigurationValidationError) {
-  PollingMetadataUpdater::ValidateConfiguration();
-  reader_.ValidateConfiguration();
+  PollingMetadataUpdater::ValidateDynamicConfiguration();
+  reader_.ValidateDynamicConfiguration();
 }
 
 }

src/docker.h

@@ -35,8 +35,8 @@ class DockerReader {
   // A Docker metadata query function.
   std::vector<MetadataUpdater::ResourceMetadata> MetadataQuery() const;
 
-  // Validates the relevant configuration and throws if it's incorrect.
-  void ValidateConfiguration() const
+  // Validates the relevant dynamic configuration and throws if it's incorrect.
+  void ValidateDynamicConfiguration() const
       throw(MetadataUpdater::ConfigurationValidationError);
 
  private:
@@ -73,7 +73,7 @@ class DockerUpdater : public PollingMetadataUpdater {
           [=]() { return reader_.MetadataQuery(); }) { }
 
  protected:
-  void ValidateConfiguration() const throw(ConfigurationValidationError);
+  void ValidateDynamicConfiguration() const throw(ConfigurationValidationError);
 
  private:
   DockerReader reader_;

src/kubernetes.cc

@@ -1072,7 +1072,7 @@ void KubernetesReader::UpdateServiceToPodsCache(
   }
 }
 
-void KubernetesReader::ValidateConfiguration() const
+void KubernetesReader::ValidateDynamicConfiguration() const
     throw(MetadataUpdater::ConfigurationValidationError) {
   try {
     util::Retry<NonRetriableError, QueryException>(
@@ -1268,10 +1268,10 @@ KubernetesUpdater::KubernetesUpdater(const Configuration& config,
         config.KubernetesUpdaterIntervalSeconds(),
         [=]() { return reader_.MetadataQuery(); }) { }
 
-void KubernetesUpdater::ValidateConfiguration() const
+void KubernetesUpdater::ValidateDynamicConfiguration() const
     throw(ConfigurationValidationError) {
-  PollingMetadataUpdater::ValidateConfiguration();
-  reader_.ValidateConfiguration();
+  PollingMetadataUpdater::ValidateDynamicConfiguration();
+  reader_.ValidateDynamicConfiguration();
 }
 
 bool KubernetesUpdater::ShouldStartUpdater() const {

src/kubernetes.h

@@ -43,8 +43,8 @@ class KubernetesReader {
   // A Kubernetes metadata query function.
   std::vector<MetadataUpdater::ResourceMetadata> MetadataQuery() const;
 
-  // Validates the relevant configuration and throws if it's incorrect.
-  void ValidateConfiguration() const
+  // Validates the relevant dynamic configuration and throws if it's incorrect.
+  void ValidateDynamicConfiguration() const
       throw(MetadataUpdater::ConfigurationValidationError);
 
   // Node watcher.
@@ -227,7 +227,7 @@ class KubernetesUpdater : public PollingMetadataUpdater {
   }
 
  protected:
-  void ValidateConfiguration() const throw(ConfigurationValidationError);
+  void ValidateDynamicConfiguration() const throw(ConfigurationValidationError);
   bool ShouldStartUpdater() const;
 
   void StartUpdater();

src/updater.cc

@@ -31,9 +31,10 @@ MetadataUpdater::MetadataUpdater(const Configuration& config,
 MetadataUpdater::~MetadataUpdater() {}
 
 void MetadataUpdater::start() throw(ConfigurationValidationError) {
-  ValidateConfiguration();
+  ValidateStaticConfiguration();
 
   if (ShouldStartUpdater()) {
+    ValidateDynamicConfiguration();
     StartUpdater();
   } else {
     LOG(INFO) << "Not starting " << name_;
@@ -60,7 +61,7 @@ PollingMetadataUpdater::~PollingMetadataUpdater() {
   }
 }
 
-void PollingMetadataUpdater::ValidateConfiguration() const
+void PollingMetadataUpdater::ValidateStaticConfiguration() const
     throw(ConfigurationValidationError) {
   if (period_ < time::seconds::zero()) {
     throw ConfigurationValidationError(

src/updater.h

@@ -81,10 +81,10 @@ class MetadataUpdater {
  protected:
   friend class UpdaterTest;
 
-  // Validates the relevant configuration.
+  // Validates static properties of the relevant configuration.
   // If the configuration is invalid, throws a ConfigurationValidationError,
   // which is generally expected to pass through and terminate the program.
-  virtual void ValidateConfiguration() const
+  virtual void ValidateStaticConfiguration() const
       throw(ConfigurationValidationError) {}
 
   // Decides whether the updater logic should be started based on the current
@@ -93,6 +93,13 @@ class MetadataUpdater {
     return true;
   }
 
+  // Validates dynamic properties of the relevant configuration.
+  // If the configuration is invalid, throws a ConfigurationValidationError,
+  // which is generally expected to pass through and terminate the program.
+  // This should only run when we know the updater is about to be started.
+  virtual void ValidateDynamicConfiguration() const
+      throw(ConfigurationValidationError) {}
+
   // Internal method for starting the updater's logic.
   virtual void StartUpdater() = 0;
 
@@ -135,7 +142,8 @@ class PollingMetadataUpdater : public MetadataUpdater {
  protected:
   friend class UpdaterTest;
 
-  void ValidateConfiguration() const throw(ConfigurationValidationError);
+  void ValidateStaticConfiguration() const throw(ConfigurationValidationError);
+  using MetadataUpdater::ValidateDynamicConfiguration;
   bool ShouldStartUpdater() const;
   void StartUpdater();
   void StopUpdater();

test/updater_unittest.cc

@@ -14,14 +14,18 @@ class UpdaterTest : public ::testing::Test {
   // query_metadata function not needed to test callbacks.
   UpdaterTest() : config(), store(config) {}
 
-  static void ValidateConfiguration(MetadataUpdater* updater) {
-    updater->ValidateConfiguration();
+  static void ValidateStaticConfiguration(MetadataUpdater* updater) {
+    updater->ValidateStaticConfiguration();
   }
 
   static bool ShouldStartUpdater(MetadataUpdater* updater) {
     return updater->ShouldStartUpdater();
   }
 
+  static void ValidateDynamicConfiguration(MetadataUpdater* updater) {
+    updater->ValidateDynamicConfiguration();
+  }
+
   static void UpdateMetadataCallback(
       MetadataUpdater* updater,
       MetadataUpdater::ResourceMetadata&& result) {
@@ -40,33 +44,144 @@ class UpdaterTest : public ::testing::Test {
 
 namespace {
 
-TEST_F(UpdaterTest, ValidateConfiguration_OneMinutePollingIntervalIsValid) {
+class ValidateStaticConfigurationTest : public UpdaterTest {};
+
+TEST_F(ValidateStaticConfigurationTest, OneMinutePollingIntervalIsValid) {
   PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr);
-  EXPECT_NO_THROW(ValidateConfiguration(&updater));
+  EXPECT_NO_THROW(ValidateStaticConfiguration(&updater));
 }
 
-TEST_F(UpdaterTest, ValidateConfiguration_ZeroSecondPollingIntervalIsValid) {
+TEST_F(ValidateStaticConfigurationTest, ZeroSecondPollingIntervalIsValid) {
   PollingMetadataUpdater updater(config, &store, "Test", 0, nullptr);
-  EXPECT_NO_THROW(ValidateConfiguration(&updater));
+  EXPECT_NO_THROW(ValidateStaticConfiguration(&updater));
 }
 
-TEST_F(UpdaterTest, ValidateConfiguration_NegativePollingIntervalIsInvalid) {
+TEST_F(ValidateStaticConfigurationTest, NegativePollingIntervalIsInvalid) {
   PollingMetadataUpdater updater(config, &store, "BadUpdater", -1, nullptr);
   EXPECT_THROW(
-      ValidateConfiguration(&updater),
+      ValidateStaticConfiguration(&updater),
       MetadataUpdater::ConfigurationValidationError);
 }
 
-TEST_F(UpdaterTest, ShouldStart_OneMinutePollingIntervalEnablesUpdate) {
+class ShouldStartUpdaterTest : public UpdaterTest {};
+
+TEST_F(ShouldStartUpdaterTest, OneMinutePollingIntervalEnablesUpdate) {
   PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr);
   EXPECT_TRUE(ShouldStartUpdater(&updater));
 }
 
-TEST_F(UpdaterTest, ShouldStart_ZeroSecondPollingIntervalDisablesUpdate) {
+TEST_F(ShouldStartUpdaterTest, ZeroSecondPollingIntervalDisablesUpdate) {
   PollingMetadataUpdater updater(config, &store, "Test", 0, nullptr);
   EXPECT_FALSE(ShouldStartUpdater(&updater));
 }
 
+class ValidationOrderingTest : public UpdaterTest {};
+
+class MockMetadataUpdater : public MetadataUpdater {
+ public:
+  MockMetadataUpdater(const Configuration& config, bool fail_static, bool should_start, bool fail_dynamic)
+      : MetadataUpdater(config, nullptr, "MOCK"),
+        fail_static_validation(fail_static),
+        should_start_updater(should_start),
+        fail_dynamic_validation(fail_dynamic) {}
+
+  const std::vector<std::string>& call_sequence() { return call_sequence_; }
+
+ protected:
+  void ValidateStaticConfiguration() const
+      throw(ConfigurationValidationError) {
+    call_sequence_.push_back("ValidateStaticConfiguration");
+    if (fail_static_validation) {
+      throw ConfigurationValidationError("ValidateStaticConfiguration");
+    }
+  }
+  bool ShouldStartUpdater() const {
+    call_sequence_.push_back("ShouldStartUpdater");
+    return should_start_updater;
+  }
+  void ValidateDynamicConfiguration() const
+      throw(ConfigurationValidationError) {
+    call_sequence_.push_back("ValidateDynamicConfiguration");
+    if (fail_dynamic_validation) {
+      throw ConfigurationValidationError("ValidateDynamicConfiguration");
+    }
+  }
+  void StartUpdater() {
+    call_sequence_.push_back("StartUpdater");
+  }
+  void StopUpdater() {}
+
+  mutable std::vector<std::string> call_sequence_;
+
+ private:
+  bool fail_static_validation;
+  bool should_start_updater;
+  bool fail_dynamic_validation;
+};
+
+TEST_F(ValidationOrderingTest, FailedStaticCheckStopsOtherChecks) {
+  MockMetadataUpdater updater(
+      config,
+      /*fail_static=*/true,
+      /*should_start=*/true,
+      /*fail_dynamic=*/true);
+  EXPECT_THROW(updater.start(), MetadataUpdater::ConfigurationValidationError);
+  EXPECT_EQ(
+      std::vector<std::string>({
+          "ValidateStaticConfiguration",
+      }),
+      updater.call_sequence());
+}
+
+TEST_F(ValidationOrderingTest, FalseShouldStartUpdaterStopsDynamicChecks) {
+  MockMetadataUpdater updater(
+      config,
+      /*fail_static=*/false,
+      /*should_start=*/false,
+      /*fail_dynamic=*/false);
+  EXPECT_NO_THROW(updater.start());
+  EXPECT_EQ(
+      std::vector<std::string>({
+          "ValidateStaticConfiguration",
+          "ShouldStartUpdater",
+      }),
+      updater.call_sequence());
+}
+
+TEST_F(ValidationOrderingTest, FailedDynamicCheckStopsStartUpdater) {
+  MockMetadataUpdater updater(
+      config,
+      /*fail_static=*/false,
+      /*should_start=*/true,
+      /*fail_dynamic=*/true);
+  EXPECT_THROW(updater.start(), MetadataUpdater::ConfigurationValidationError);
+  EXPECT_EQ(
+      std::vector<std::string>({
+          "ValidateStaticConfiguration",
+          "ShouldStartUpdater",
+          "ValidateDynamicConfiguration",
+      }),
+      updater.call_sequence());
+}
+
+TEST_F(ValidationOrderingTest, AllChecksPassedInvokesStartUpdater) {
+  MockMetadataUpdater updater(
+      config,
+      /*fail_static=*/false,
+      /*should_start=*/true,
+      /*fail_dynamic=*/false);
+  EXPECT_NO_THROW(updater.start());
+  EXPECT_EQ(
+      std::vector<std::string>({
+          "ValidateStaticConfiguration",
+          "ShouldStartUpdater",
+          "ValidateDynamicConfiguration",
+          "StartUpdater",
+      }),
+      updater.call_sequence());
+}
+
+
 TEST_F(UpdaterTest, UpdateMetadataCallback) {
   MetadataStore::Metadata m(
       "test-version",