[TT-15507] Reverting /hello endpoint to always return 200 status code

[mativm02]

User description

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

• I ensured that the documentation is up to date
• I explained why this PR updates go.mod in detail with reasoning why it's required
• I would like a code coverage CI quality gate exception and have explained why

---

PR Type

Bug fix

---

Description

• Reverts /hello (liveness) endpoint to always return HTTP 200

• Removes logic for dynamic HTTP status based on health checks

• Ensures JSON encoding errors are logged in readiness handler

• Simplifies health status calculation for liveness endpoint

---

Diagram Walkthrough

flowchart LR
  oldLogic["Dynamic HTTP status for /hello"] -- "removed" --> static200["Always return HTTP 200"]
  healthChecks["Health check evaluation"] -- "simplified" --> statusCalc["Simplified status calculation"]
  readinessHandler["Readiness handler"] -- "add error logging" --> errorLog["Logs JSON encoding errors"]

Loading

File Walkthrough

	Relevant files
Bug fix	health_check.go Revert liveness endpoint to static 200 and improve error logging gateway/health_check.go /hello endpoint now always returns HTTP 200 status Removed dynamic status code logic based on health checks Simplified health status calculation for liveness checks Added error logging for JSON encoding in readiness handler +24/-4

---

[buger]

💔 The detected issue is not in one of the allowed statuses 💔

Detected Status	Open	❌
Allowed Statuses	In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review	✔️

Please ensure your jira story is in one of the allowed statuses

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logic Change The /hello endpoint now always returns HTTP 200, regardless of health check results. Reviewers should validate that this change aligns with the intended liveness semantics and does not negatively impact monitoring or alerting systems that rely on non-200 responses for failures. w.WriteHeader(http.StatusOK) err := json.NewEncoder(w).Encode(res) Error Logging Consistency The readiness handler now logs JSON encoding errors, matching the liveness handler. Confirm that this logging is sufficient and does not expose sensitive information in logs. err := json.NewEncoder(w).Encode(res) if err != nil { mainLog.Warning(fmt.Sprintf("[Readiness] Could not encode response, error: %s", err.Error())) }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle empty health checks correctly The use of case len(checks): in the switch statement may never match, since failCount can only reach len(checks) if all checks fail. However, if checks is empty, this logic will incorrectly set the status to Fail. Add an explicit check for zero checks to avoid misclassifying the health status when there are no checks. gateway/health_check.go [192-209] for _, v := range checks { if v.Status == Fail { failCount++ } } var status HealthCheckStatus -switch failCount { -case 0: - status = Pass - -case len(checks): - status = Fail - -default: +if len(checks) == 0 { status = Warn +} else { + switch failCount { + case 0: + status = Pass + case len(checks): + status = Fail + default: + status = Warn + } } Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a subtle bug: if checks is empty, the current logic would incorrectly set status to Fail. The improved code ensures that an empty set of checks results in a Warn status, which is more appropriate and prevents misleading health reporting. This is a significant correctness fix.	Medium

[github-actions]

API Changes

no api changes detected

[github-actions]

Code Review: Reverting /hello endpoint to always return 200 status code

Summary

This PR reverts the /hello (liveness) endpoint to always return HTTP 200 status code, regardless of the health check results. The changes are focused on the gateway/health_check.go file and align with the PR description.

Key Changes

1. Always returning HTTP 200 for liveness endpoint

   • Changed from dynamic status code to always returning http.StatusOK
   • Health check results are still included in the response body

2. Simplified health status calculation

   • Removed calls to evaluateHealthChecks and determineHealthStatus functions
   • Added direct calculation of failCount by iterating through checks
   • Added a simple switch statement to determine the status based on failCount

3. Added error logging for readiness handler

   • Added JSON encoding error logging in the readiness handler to match the liveness handler

Potential Issues

1. Empty health checks edge case

switch failCount {
case 0:
    status = Pass
case len(checks):
    status = Fail
default:
    status = Warn
}

If checks is empty (len(checks) == 0), then failCount would be 0, and the status would be set to Pass. However, if checks is empty, the case len(checks): would also match, potentially leading to incorrect status determination. Consider adding an explicit check for empty health checks:

if len(checks) == 0 {
    status = Warn
} else {
    switch failCount {
    case 0:
        status = Pass
    case len(checks):
        status = Fail
    default:
        status = Warn
    }
}

2. Unused functions

The functions determineHealthStatus, evaluateHealthChecks, and isCriticalFailure are still present in the code but are no longer used by the liveCheckHandler. If they're not used elsewhere, consider removing them to clean up the code.

Impact Analysis

1. Monitoring systems: Systems monitoring the HTTP status code of the /hello endpoint will need to be updated to look at the response body content instead of relying on non-200 status codes for failure detection.

2. Kubernetes liveness probes: If Kubernetes liveness probes are configured to check the /hello endpoint, they will now always succeed regardless of the actual health status. This could lead to unhealthy pods remaining in service.

Conclusion

The changes align with the PR description and successfully revert the /hello endpoint to always return HTTP 200. The code is generally well-structured, but there's a potential edge case with empty health checks that should be addressed. Additionally, consider removing unused functions if they're not needed elsewhere.

---

Tip: Mention me again using /probe <request>.
Powered by Probe AI