From 5f25d33f48383bac039bbe50f528f93687b6f375 Mon Sep 17 00:00:00 2001 From: Igor Peshansky Date: Tue, 1 May 2018 14:27:30 -0400 Subject: [PATCH] Separate ValidateConfiguration into the static and dynamic parts. --- src/docker.cc | 8 +-- src/docker.h | 6 +- src/kubernetes.cc | 8 +-- src/kubernetes.h | 6 +- src/updater.cc | 5 +- src/updater.h | 14 +++- test/updater_unittest.cc | 135 ++++++++++++++++++++++++++++++++++++--- 7 files changed, 153 insertions(+), 29 deletions(-) diff --git a/src/docker.cc b/src/docker.cc index 1850e699..3040fbfb 100644 --- a/src/docker.cc +++ b/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(); } } diff --git a/src/docker.h b/src/docker.h index 1725118b..0177ed0e 100644 --- a/src/docker.h +++ b/src/docker.h @@ -35,8 +35,8 @@ class DockerReader { // A Docker metadata query function. std::vector 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_; diff --git a/src/kubernetes.cc b/src/kubernetes.cc index c1f2695a..8594b4aa 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -1072,7 +1072,7 @@ void KubernetesReader::UpdateServiceToPodsCache( } } -void KubernetesReader::ValidateConfiguration() const +void KubernetesReader::ValidateDynamicConfiguration() const throw(MetadataUpdater::ConfigurationValidationError) { try { util::Retry( @@ -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 { diff --git a/src/kubernetes.h b/src/kubernetes.h index d5d7d9dc..757d49ff 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -43,8 +43,8 @@ class KubernetesReader { // A Kubernetes metadata query function. std::vector 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(); diff --git a/src/updater.cc b/src/updater.cc index 293d4b57..4286df9c 100644 --- a/src/updater.cc +++ b/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( diff --git a/src/updater.h b/src/updater.h index e26276be..4e47e37d 100644 --- a/src/updater.h +++ b/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(); diff --git a/test/updater_unittest.cc b/test/updater_unittest.cc index 030e1dca..be37d904 100644 --- a/test/updater_unittest.cc +++ b/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& 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 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({ + "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({ + "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({ + "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({ + "ValidateStaticConfiguration", + "ShouldStartUpdater", + "ValidateDynamicConfiguration", + "StartUpdater", + }), + updater.call_sequence()); +} + + TEST_F(UpdaterTest, UpdateMetadataCallback) { MetadataStore::Metadata m( "test-version",