[NET11250]Added fix for health check update for consul-dataplane

CHANGELOG.md

@@ -1,4 +1,6 @@
 ## Unreleased
+BUG FIXES
+* Fix the issue where the service was accepting traffic even though it wasn't healthy. This fix updates the health check status for `consul-dataplane` container and takes into account the health of the service container as well.
 
 IMPROVEMENTS
 * Bump Go version to `1.22.7`

subcommand/health-sync/checks.go

@@ -108,7 +108,6 @@ func (c *Command) syncChecks(consulClient *api.Client,
 	}
 
 	serviceName := c.constructServiceName(taskMeta.Family)
-
 	containersToSync, missingContainers := findContainersToSync(parsedContainerNames, taskMeta)
 
 	// Mark the Consul health status as critical for missing containers
@@ -131,20 +130,20 @@ func (c *Command) syncChecks(consulClient *api.Client,
 		}
 	}
 
+	parsedContainers := make(map[string]string)
+	// iterate over parse
 	for _, container := range containersToSync {
 		c.log.Debug("updating Consul check from ECS container health",
 			"name", container.Name,
 			"status", container.Health.Status,
 			"statusSince", container.Health.StatusSince,
 			"exitCode", container.Health.ExitCode,
 		)
-
+		parsedContainers[container.Name] = container.Health.Status
 		previousStatus := currentStatuses[container.Name]
 		if container.Health.Status != previousStatus {
 			var err error
-			if container.Name == config.ConsulDataplaneContainerName {
-				err = c.handleHealthForDataplaneContainer(consulClient, taskMeta.TaskID(), serviceName, clusterARN, container.Name, container.Health.Status)
-			} else {
+			if container.Name != config.ConsulDataplaneContainerName {
 				checkID := constructCheckID(makeServiceID(serviceName, taskMeta.TaskID()), container.Name)
 				err = c.updateConsulHealthStatus(consulClient, checkID, clusterARN, container.Health.Status)
 			}
@@ -161,8 +160,33 @@ func (c *Command) syncChecks(consulClient *api.Client,
 				currentStatuses[container.Name] = container.Health.Status
 			}
 		}
+
+	}
+	overallDataplaneHealthStatus, ok := parsedContainers[config.ConsulDataplaneContainerName]
+	// if dataplane container exist and healthy then proceed to checking the other containers health
+	if ok && overallDataplaneHealthStatus == ecs.HealthStatusHealthy {
+		//
+		for _, healthStatus := range parsedContainers {
+			// as soon as we find any unhealthy container, we can set the dataplane health to unhealthy
+			if healthStatus != ecs.HealthStatusHealthy {
+				overallDataplaneHealthStatus = ecs.HealthStatusUnhealthy
+				break
+			}
+		}
+	} else {
+		// If no dataplane container or dataplane container is not healthy set overall health to unhealthy
+		overallDataplaneHealthStatus = ecs.HealthStatusUnhealthy
 	}
 
+	err = c.handleHealthForDataplaneContainer(consulClient, taskMeta.TaskID(), serviceName, clusterARN, config.ConsulDataplaneContainerName, overallDataplaneHealthStatus)
+	if err != nil {
+		c.log.Warn("failed to update Consul health status", "err", err)
+	} else {
+		c.log.Info("container health check updated in Consul",
+			"name", config.ConsulDataplaneContainerName,
+			"status", overallDataplaneHealthStatus,
+		)
+	}
 	return currentStatuses
 }
 

subcommand/health-sync/command_test.go

@@ -103,6 +103,7 @@ func TestRun(t *testing.T) {
 		healthSyncContainers            map[string]healthSyncContainerMetaData
 		missingDataplaneContainer       bool
 		shouldMissingContainersReappear bool
+		expectedDataplaneHealthStatus   string
 	}{
 		"no additional health sync containers": {},
 		"one healthy health sync container": {
@@ -132,6 +133,7 @@ func TestRun(t *testing.T) {
 					status: ecs.HealthStatusUnhealthy,
 				},
 			},
+			expectedDataplaneHealthStatus: api.HealthCritical,
 		},
 		"one healthy and one missing health sync containers": {
 			healthSyncContainers: map[string]healthSyncContainerMetaData{
@@ -143,7 +145,8 @@ func TestRun(t *testing.T) {
 					status:  ecs.HealthStatusUnhealthy,
 				},
 			},
-			consulLogin: consulLoginCfg,
+			expectedDataplaneHealthStatus: api.HealthPassing,
+			consulLogin:                   consulLoginCfg,
 		},
 		"two unhealthy health sync containers": {
 			healthSyncContainers: map[string]healthSyncContainerMetaData{
@@ -154,6 +157,7 @@ func TestRun(t *testing.T) {
 					status: ecs.HealthStatusUnhealthy,
 				},
 			},
+			expectedDataplaneHealthStatus: api.HealthCritical,
 		},
 		"missing dataplane container": {
 			missingDataplaneContainer: true,
@@ -200,6 +204,7 @@ func TestRun(t *testing.T) {
 					status:  ecs.HealthStatusUnhealthy,
 				},
 			},
+			expectedDataplaneHealthStatus:   api.HealthPassing,
 			shouldMissingContainersReappear: true,
 			consulLogin:                     consulLoginCfg,
 		},
@@ -368,6 +373,7 @@ func TestRun(t *testing.T) {
 
 			// Align the expectations for checks according to the
 			// state of health sync containers
+			markDataplaneContainerUnhealthy := false
 			for _, expCheck := range expectedSvcChecks {
 				found := false
 				for name, hsc := range c.healthSyncContainers {
@@ -377,21 +383,41 @@ func TestRun(t *testing.T) {
 							expCheck.Status = api.HealthCritical
 						} else {
 							expCheck.Status = ecsHealthToConsulHealth(hsc.status)
+							// If there are multiple health sync containers and one of them is unhealthy
+							// then the service check should be critical.
+							for containerName := range c.healthSyncContainers {
+								if c.healthSyncContainers[containerName].status == ecs.HealthStatusUnhealthy &&
+									c.healthSyncContainers[containerName].missing == false {
+									expCheck.Status = api.HealthCritical
+									markDataplaneContainerUnhealthy = true
+									break
+								}
+							}
+
 						}
 						found = true
 						break
 					}
 				}
 
 				if !found {
-					if c.missingDataplaneContainer {
-						expCheck.Status = api.HealthCritical
+					if c.expectedDataplaneHealthStatus != "" {
+						expCheck.Status = c.expectedDataplaneHealthStatus
 					} else {
-						expCheck.Status = api.HealthPassing
+						if c.missingDataplaneContainer || markDataplaneContainerUnhealthy {
+							expCheck.Status = api.HealthCritical
+						} else if len(c.healthSyncContainers) == 0 || !markDataplaneContainerUnhealthy {
+							expCheck.Status = api.HealthPassing
+						}
 					}
 				}
 			}
-			expectedProxyCheck.Status = api.HealthPassing
+			if markDataplaneContainerUnhealthy {
+				expectedProxyCheck.Status = api.HealthCritical
+			} else {
+				expectedProxyCheck.Status = api.HealthPassing
+			}
+
 			if c.missingDataplaneContainer {
 				expectedProxyCheck.Status = api.HealthCritical
 			}