upstream: fix bugs of healthStatus() method (envoyproxy#28081)

* upstream: fix bugs of healthStatus() method

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* add change log

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

* update member name

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

---------

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>

changelogs/current.yaml

@@ -210,6 +210,14 @@ bug_fixes:
     Fixes a bug where route properties such as key_formatter,
     prefix and remove_prefix do not take effect when configured for :ref:`catch_all_route
     <envoy_v3_api_field_extensions.filters.network.redis_proxy.v3.RedisProxy.PrefixRoutes.catch_all_route>`.
+- area: upstream
+  change: |
+    Fixes a bug where the healthStatus() method of host return incorrect health status
+    when the host status is updated by the EDS.
+- area: upstream
+  change: |
+    Fixes a bug where the healthStatus() method of host return unmatched health status
+    with the coarseHealth() method.
 - area: original_dst
   change: |
     Fixes an issue with the ORIGINAL_DST cluster cleanup timer lifetime, which

envoy/upstream/upstream.h

@@ -219,6 +219,16 @@ class Host : virtual public HostDescription {
    */
   virtual HealthStatus healthStatus() const PURE;
 
+  /**
+   * Set the EDS health status of the host. This is used when the host status is updated via EDS.
+   */
+  virtual void setEdsHealthStatus(HealthStatus health_status) PURE;
+
+  /**
+   * @return the EDS health status of the host.
+   */
+  virtual HealthStatus edsHealthStatus() const PURE;
+
   /**
    * Set the host's health checker monitor. Monitors are assumed to be thread safe, however
    * a new monitor must be installed before the host is used across threads. Thus,

source/common/upstream/upstream_impl.cc

@@ -127,28 +127,23 @@ absl::flat_hash_map<std::string, ProtocolOptionsConfigConstSharedPtr> parseExten
   return options;
 }
 
-// Updates the health flags for an existing host to match the new host.
+// Updates the EDS health flags for an existing host to match the new host.
 // @param updated_host the new host to read health flag values from.
 // @param existing_host the host to update.
-// @param flag the health flag to update.
 // @return bool whether the flag update caused the host health to change.
-bool updateHealthFlag(const Host& updated_host, Host& existing_host, Host::HealthFlag flag) {
-  // Check if the health flag has changed.
-  if (existing_host.healthFlagGet(flag) != updated_host.healthFlagGet(flag)) {
-    // Keep track of the previous health value of the host.
-    const auto previous_health = existing_host.coarseHealth();
-
-    if (updated_host.healthFlagGet(flag)) {
-      existing_host.healthFlagSet(flag);
-    } else {
-      existing_host.healthFlagClear(flag);
-    }
+bool updateEdsHealthFlag(const Host& updated_host, Host& existing_host) {
+  auto updated_eds_health_status = updated_host.edsHealthStatus();
 
-    // Rebuild if changing the flag affected the host health.
-    return previous_health != existing_host.coarseHealth();
+  // Check if the health flag has changed.
+  if (updated_eds_health_status == existing_host.edsHealthStatus()) {
+    return false;
   }
 
-  return false;
+  const auto previous_health = existing_host.coarseHealth();
+  existing_host.setEdsHealthStatus(updated_eds_health_status);
+
+  // Rebuild if changing the flag affected the host health.
+  return previous_health != existing_host.coarseHealth();
 }
 
 // Converts a set of hosts into a HostVector, excluding certain hosts.
@@ -468,6 +463,11 @@ Host::CreateConnectionData HostImpl::createConnection(
 }
 
 void HostImpl::setEdsHealthFlag(envoy::config::core::v3::HealthStatus health_status) {
+  // Clear all old EDS health flags first.
+  healthFlagClear(Host::HealthFlag::FAILED_EDS_HEALTH);
+  healthFlagClear(Host::HealthFlag::DEGRADED_EDS_HEALTH);
+
+  // Set the appropriate EDS health flag.
   switch (health_status) {
   case envoy::config::core::v3::UNHEALTHY:
     FALLTHRU;
@@ -2080,10 +2080,7 @@ bool BaseDynamicClusterImpl::updateDynamicHostList(
         hosts_changed = true;
       }
 
-      hosts_changed |=
-          updateHealthFlag(*host, *existing_host->second, Host::HealthFlag::FAILED_EDS_HEALTH);
-      hosts_changed |=
-          updateHealthFlag(*host, *existing_host->second, Host::HealthFlag::DEGRADED_EDS_HEALTH);
+      hosts_changed |= updateEdsHealthFlag(*host, *existing_host->second);
 
       // Did metadata change?
       bool metadata_changed = true;

source/common/upstream/upstream_impl.h

@@ -346,11 +346,10 @@ class HostImpl : public HostDescriptionImpl,
            TimeSource& time_source)
       : HostDescriptionImpl(cluster, hostname, address, metadata, locality, health_check_config,
                             priority, time_source),
-        disable_active_health_check_(health_check_config.disable_active_health_check()),
-        health_status_(health_status) {
+        disable_active_health_check_(health_check_config.disable_active_health_check()) {
     // This EDS flags setting is still necessary for stats, configuration dump, canonical
     // coarseHealth() etc.
-    setEdsHealthFlag(health_status);
+    setEdsHealthStatus(health_status);
     HostImpl::weight(initial_weight);
   }
 
@@ -397,30 +396,38 @@ class HostImpl : public HostDescriptionImpl,
     // Evaluate active health status first.
 
     // Active unhealthy.
-    if (healthFlagGet(HealthFlag::FAILED_ACTIVE_HC) ||
-        healthFlagGet(HealthFlag::FAILED_OUTLIER_CHECK)) {
+    if (healthFlagsGet(enumToInt(HealthFlag::FAILED_ACTIVE_HC) |
+                       enumToInt(HealthFlag::FAILED_OUTLIER_CHECK))) {
       return HealthStatus::UNHEALTHY;
     }
+
+    // Eds unhealthy.
+    if (eds_health_status_ == envoy::config::core::v3::UNHEALTHY ||
+        eds_health_status_ == envoy::config::core::v3::DRAINING ||
+        eds_health_status_ == envoy::config::core::v3::TIMEOUT) {
+      return eds_health_status_;
+    }
+
     // Active degraded.
     if (healthFlagGet(HealthFlag::DEGRADED_ACTIVE_HC)) {
       return HealthStatus::DEGRADED;
     }
 
-    // Use EDS status if no any active status.
-    return health_status_;
+    // Eds degraded or healthy.
+    return eds_health_status_;
   }
 
   Host::Health coarseHealth() const override {
     // If any of the unhealthy flags are set, host is unhealthy.
-    if (healthFlagGet(HealthFlag::FAILED_ACTIVE_HC) ||
-        healthFlagGet(HealthFlag::FAILED_OUTLIER_CHECK) ||
-        healthFlagGet(HealthFlag::FAILED_EDS_HEALTH)) {
+    if (healthFlagsGet(enumToInt(HealthFlag::FAILED_ACTIVE_HC) |
+                       enumToInt(HealthFlag::FAILED_OUTLIER_CHECK) |
+                       enumToInt(HealthFlag::FAILED_EDS_HEALTH))) {
       return Host::Health::Unhealthy;
     }
 
     // If any of the degraded flags are set, host is degraded.
-    if (healthFlagGet(HealthFlag::DEGRADED_ACTIVE_HC) ||
-        healthFlagGet(HealthFlag::DEGRADED_EDS_HEALTH)) {
+    if (healthFlagsGet(enumToInt(HealthFlag::DEGRADED_ACTIVE_HC) |
+                       enumToInt(HealthFlag::DEGRADED_EDS_HEALTH))) {
       return Host::Health::Degraded;
     }
 
@@ -429,6 +436,14 @@ class HostImpl : public HostDescriptionImpl,
     return Host::Health::Healthy;
   }
 
+  void setEdsHealthStatus(envoy::config::core::v3::HealthStatus eds_health_status) override {
+    eds_health_status_ = eds_health_status;
+    setEdsHealthFlag(eds_health_status);
+  }
+  Host::HealthStatus edsHealthStatus() const override {
+    return Host::HealthStatus(eds_health_status_.load());
+  }
+
   uint32_t weight() const override { return weight_; }
   void weight(uint32_t new_weight) override;
   bool used() const override { return handle_count_ > 0; }
@@ -447,6 +462,9 @@ class HostImpl : public HostDescriptionImpl,
                    HostDescriptionConstSharedPtr host);
 
 private:
+  // Helper function to check multiple health flags at once.
+  bool healthFlagsGet(uint32_t flags) const { return health_flags_ & flags; }
+
   void setEdsHealthFlag(envoy::config::core::v3::HealthStatus health_status);
 
   std::atomic<uint32_t> health_flags_{};
@@ -455,7 +473,7 @@ class HostImpl : public HostDescriptionImpl,
   // TODO(wbpcode): should we store the EDS health status to health_flags_ to get unified status or
   // flag access? May be we could refactor HealthFlag to contain all these statuses and flags in the
   // future.
-  HealthStatus health_status_{};
+  std::atomic<Host::HealthStatus> eds_health_status_{};
 
   struct HostHandleImpl : HostHandle {
     HostHandleImpl(const std::shared_ptr<const HostImpl>& parent) : parent_(parent) {

test/common/upstream/upstream_impl_test.cc

@@ -1528,25 +1528,66 @@ TEST_F(HostImplTest, HealthFlags) {
 TEST_F(HostImplTest, HealthStatus) {
   MockClusterMockPrioritySet cluster;
   HostSharedPtr host = makeTestHost(cluster.info_, "tcp://10.0.0.1:1234", simTime(), 1, 0,
-                                    Host::HealthStatus::DRAINING);
+                                    Host::HealthStatus::DEGRADED);
 
-  // To begin with, no flags are set so EDS status is used.
+  // To begin with, no active flags are set so EDS status is used.
+  EXPECT_EQ(Host::HealthStatus::DEGRADED, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::DEGRADED, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Degraded, host->coarseHealth());
+  EXPECT_TRUE(host->healthFlagGet(Host::HealthFlag::DEGRADED_EDS_HEALTH));
+
+  // Update EDS status to healthy, host should be healthy.
+  host->setEdsHealthStatus(Host::HealthStatus::HEALTHY);
+  EXPECT_EQ(Host::HealthStatus::HEALTHY, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::HEALTHY, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Healthy, host->coarseHealth());
+  // Old EDS flags should be cleared.
+  EXPECT_FALSE(host->healthFlagGet(Host::HealthFlag::DEGRADED_EDS_HEALTH));
+
+  // Update EDS status to draining.
+  host->setEdsHealthStatus(Host::HealthStatus::DRAINING);
   EXPECT_EQ(Host::HealthStatus::DRAINING, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::DRAINING, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Unhealthy, host->coarseHealth());
+  // Old EDS flags should be cleared and new flag will be set.
+  EXPECT_TRUE(host->healthFlagGet(Host::HealthFlag::FAILED_EDS_HEALTH));
 
   // Setting an active unhealthy flag make the host unhealthy.
   host->healthFlagSet(Host::HealthFlag::FAILED_ACTIVE_HC);
   EXPECT_EQ(Host::HealthStatus::UNHEALTHY, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::DRAINING, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Unhealthy, host->coarseHealth());
   host->healthFlagClear(Host::HealthFlag::FAILED_ACTIVE_HC);
+
+  // Setting another active unhealthy flag make the host unhealthy.
   host->healthFlagSet(Host::HealthFlag::FAILED_OUTLIER_CHECK);
   EXPECT_EQ(Host::HealthStatus::UNHEALTHY, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::DRAINING, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Unhealthy, host->coarseHealth());
 
-  // Setting a degraded flag on an unhealthy host has no effect.
+  // Setting a active degraded flag on an unhealthy host has no effect.
   host->healthFlagSet(Host::HealthFlag::DEGRADED_ACTIVE_HC);
   EXPECT_EQ(Host::HealthStatus::UNHEALTHY, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::DRAINING, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Unhealthy, host->coarseHealth());
 
-  // If the degraded flag is the only thing set, host is degraded.
+  // If the active degraded flag is the only thing set, the unhealthy EDS status is used.
   host->healthFlagClear(Host::HealthFlag::FAILED_OUTLIER_CHECK);
+  EXPECT_EQ(Host::HealthStatus::DRAINING, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::DRAINING, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Unhealthy, host->coarseHealth());
+
+  // If the unhealthy EDS is removed, the active degraded flag is used.
+  host->setEdsHealthStatus(Host::HealthStatus::HEALTHY);
   EXPECT_EQ(Host::HealthStatus::DEGRADED, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::HEALTHY, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Degraded, host->coarseHealth());
+
+  // Remove the active degraded flag, host should be healthy.
+  host->healthFlagClear(Host::HealthFlag::DEGRADED_ACTIVE_HC);
+  EXPECT_EQ(Host::HealthStatus::HEALTHY, host->healthStatus());
+  EXPECT_EQ(Host::HealthStatus::HEALTHY, host->edsHealthStatus());
+  EXPECT_EQ(Host::Health::Healthy, host->coarseHealth());
 }
 
 TEST_F(HostImplTest, SkipActiveHealthCheckFlag) {

test/extensions/clusters/eds/eds_test.cc

@@ -591,6 +591,7 @@ TEST_F(EdsTest, EndpointHealthStatus) {
 
     for (uint32_t i = 0; i < hosts.size(); ++i) {
       EXPECT_EQ(health_status_expected[i].second, hosts[i]->coarseHealth());
+      EXPECT_EQ(health_status_expected[i].first, hosts[i]->edsHealthStatus());
     }
   }
 
@@ -602,9 +603,11 @@ TEST_F(EdsTest, EndpointHealthStatus) {
     auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts();
     EXPECT_EQ(hosts.size(), health_status_expected.size());
     EXPECT_EQ(Host::Health::Unhealthy, hosts[0]->coarseHealth());
+    EXPECT_EQ(envoy::config::core::v3::UNHEALTHY, hosts[0]->edsHealthStatus());
 
     for (uint32_t i = 1; i < hosts.size(); ++i) {
       EXPECT_EQ(health_status_expected[i].second, hosts[i]->coarseHealth());
+      EXPECT_EQ(health_status_expected[i].first, hosts[i]->edsHealthStatus());
     }
   }
 
@@ -617,9 +620,11 @@ TEST_F(EdsTest, EndpointHealthStatus) {
     auto& hosts = cluster_->prioritySet().hostSetsPerPriority()[0]->hosts();
     EXPECT_EQ(hosts.size(), health_status_expected.size());
     EXPECT_EQ(Host::Health::Healthy, hosts[hosts.size() - 1]->coarseHealth());
+    EXPECT_EQ(envoy::config::core::v3::HEALTHY, hosts[hosts.size() - 1]->edsHealthStatus());
 
     for (uint32_t i = 1; i < hosts.size() - 1; ++i) {
       EXPECT_EQ(health_status_expected[i].second, hosts[i]->coarseHealth());
+      EXPECT_EQ(health_status_expected[i].first, hosts[i]->edsHealthStatus());
     }
   }
 

test/mocks/upstream/host.h

@@ -198,6 +198,9 @@ class MockHost : public Host {
   MOCK_METHOD(void, healthFlagsSetAll, (uint32_t));
   MOCK_METHOD(Host::Health, coarseHealth, (), (const));
   MOCK_METHOD(Host::HealthStatus, healthStatus, (), (const));
+  MOCK_METHOD(Host::HealthStatus, edsHealthStatus, (), (const));
+  MOCK_METHOD(void, setEdsHealthStatus, (Host::HealthStatus), ());
+
   MOCK_METHOD(const std::string&, hostnameForHealthChecks, (), (const));
   MOCK_METHOD(const std::string&, hostname, (), (const));
   MOCK_METHOD(Network::UpstreamTransportSocketFactory&, transportSocketFactory, (), (const));