feat: Health check endpoint for satellite#266
Open
intojhanurag wants to merge 8 commits intocontainer-registry:mainfrom
Open
feat: Health check endpoint for satellite#266intojhanurag wants to merge 8 commits intocontainer-registry:mainfrom
intojhanurag wants to merge 8 commits intocontainer-registry:mainfrom
Conversation
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Contributor
📝 WalkthroughWalkthroughAdds a configurable HTTP health/metrics/debug server started during bootstrap; introduces Changes
Sequence DiagramsequenceDiagram
participant Main as rgba(52,152,219,0.5) Main
participant Router as rgba(46,204,113,0.5) HTTP_Router
participant App as rgba(155,89,182,0.5) App_Server
participant Health as rgba(241,196,15,0.5) HealthRegistrar
participant Registry as rgba(231,76,60,0.5) Registry
participant GC as rgba(52,73,94,0.5) GroundControl
participant Client as rgba(22,160,133,0.5) Client
Main->>Router: create router
Main->>Health: NewHealthRegistrar(config, logger)
Main->>App: NewApp(router, ctx, logger, port, Health)
Main->>App: SetupRoutes()
App->>Health: RegisterRoutes(router)
Main->>App: SetupServer(wg) -> start HTTP server
Main->>Satellite: satellite.Run(ctx)
Client->>Router: GET /ready
Router->>Health: readyHandler()
Health->>Registry: checkRegistry()
Registry-->>Health: registry status
Health->>GC: checkGroundControl()
GC-->>Health: gc status
Health->>Health: aggregate results
Health-->>Router: JSON response (200/503)
Router-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codacy's Analysis Summary0 new issue (≤ 0 issue) Review Pull Request in Codacy →
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/config/validate.go`:
- Around line 109-112: The health_server_port value is defaulted but not
validated, which can produce invalid listen addresses; after the defaulting of
config.AppConfig.HealthServerPort to DefaultHealthServerPort, parse the port
string (strip any leading ':') as an integer and verify it is numeric and in the
1–65535 range, appending a validation error (or returning it) if parsing fails
or the number is out of range; update the same function that contains the
existing defaulting logic (referencing config.AppConfig.HealthServerPort,
DefaultHealthServerPort and the warnings slice) to perform this check before
returning so ListenAndServe won't receive invalid Addr values.
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/server/health.go`:
- Line 111: The hardcoded "http" scheme used when building registryURL
(registryURL := fmt.Sprintf("http://%s:%s/v2/", address, port)) will fail for
TLS-enabled registries; update the code to choose the scheme dynamically (e.g.,
scheme := "https" when TLS is enabled, otherwise "http") by reading the Zot
registry TLS setting from the configuration or by adding a configurable scheme
parameter, then build registryURL using fmt.Sprintf("%s://%s:%s/v2/", scheme,
address, port); ensure the chosen source of truth for TLS (config field or
function parameter) is validated and used wherever registryURL is constructed.
- Line 88: Fix the typo in the log message inside the health handler: change the
Msg string used on h.logger.Error().Err(err).Msg(...) from "failed to write
rediness response" to "failed to write readiness response" so logs read
correctly; update the message only (leave the Err(err) chaining and logger call
intact).
🧹 Nitpick comments (3)
internal/server/health.go (3)
23-28: Consider adding nil checks for constructor parameters.If
cmorloggeris nil, the code will panic with nil pointer dereferences when the handlers are invoked. Adding defensive checks would make the API more robust.🛡️ Proposed defensive check
func NewHealthRegistrar(cm *config.ConfigManager, logger *zerolog.Logger) *HealthRegistrar { + if cm == nil || logger == nil { + panic("NewHealthRegistrar: cm and logger must not be nil") + } return &HealthRegistrar{ cm: cm, logger: logger, } }
137-137: Potential double slash in URL ifgcURLends with/.If
ResolveGroundControlURL()returns a URL ending with/, the concatenation produceshttp://example.com//ping. Consider usingstrings.TrimSuffixorurl.JoinPathfor safer URL construction.♻️ Proposed fix using strings.TrimSuffix
+import "strings" + func (h *HealthRegistrar) checkGroundControl() error { - gcURL := h.cm.ResolveGroundControlURL() + gcURL := strings.TrimSuffix(h.cm.ResolveGroundControlURL(), "/") // Simple ping to GC
125-127: Inconsistent HTTP status code validation between registry and ground control checks.
checkRegistryaccepts any 2xx status (200-299), whilecheckGroundControlaccepts only exactly 200. Consider aligning these for consistency unless there's a specific reason for the difference.Also applies to: 147-149
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/server/health.go`:
- Around line 99-113: The readiness check in health.go currently assumes
http.port is a string and fails for numeric ports; update the port extraction
after httpData lookup in the function that parses h.cm.GetRawZotConfig() so it
accepts both string and numeric values (JSON numeric becomes float64), e.g.,
check if httpData["port"] is a string else if it's a float64 convert it to an
integer string (or use fmt.Sprintf("%v", v)) and assign to port; keep the
existing address lookup logic (httpData["address"]) and then use the normalized
port string when building the bind address.
In `@pkg/config/validate.go`:
- Around line 122-132: Rename the function isvalidPort to isValidPort to follow
Go naming conventions and update every call site to the new name (e.g., the code
that currently calls isvalidPort at its validation location); ensure the
function signature remains the same and import usages or tests referencing
isvalidPort are updated to isValidPort so linters no longer flag the naming
inconsistency.
🧹 Nitpick comments (1)
internal/server/health.go (1)
96-140: Reduce cyclomatic complexity incheckRegistry.This function exceeds the configured complexity limit. Consider extracting config parsing and URL construction into helpers to keep each function under the threshold.
As per coding guidelines:
**/*.go: Use strict golangci-lint with 50+ linters for Go code quality, enforcing rules such as: no global variables (gochecknoglobals), no init functions (gochecknoinits), cyclomatic complexity limits, function length limits (100 lines, 50 statements)
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
intojhanurag
force-pushed
the
feat/health-check
branch
from
d5fbf5b to
968c5f5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/server/health.go`:
- Around line 108-133: The readiness probe currently builds registryURL directly
from address and port (using variables address, port, and scheme), which fails
when the service is bound to "0.0.0.0" or "::"; update the code to detect
bind-all addresses (0.0.0.0 and ::) and replace them with the appropriate
loopback host ("127.0.0.1" for IPv4, "[::1]" or use net.ParseIP to choose)
before composing the URL, and use net.JoinHostPort(address, port) to safely
handle IPv6 when creating registryURL (instead of manual "%s:%s"). Ensure the
TLS check (scheme) and existing port conversion logic remain unchanged while
only normalizing the bind-all address and using net.JoinHostPort to build the
host:port portion.
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/server/health.go`:
- Around line 98-158: The checkRegistry method's cyclomatic complexity is too
high; extract the config parsing/normalization and URL building into helpers to
reduce branches. Specifically, move the JSON unmarshal and validation of
http.address and http.port (including trimming, defaulting "0.0.0.0"/"::" to
"localhost", and converting port types) into a function like parseZotHTTPConfig
that returns (host string, port string, scheme string, error), then create a
buildRegistryURL(host, port, scheme) helper to produce the registryURL, and a
pingRegistry(url string) helper to perform the HTTP GET and status check (used
instead of inlining client creation and response handling) so checkRegistry only
calls these helpers (parseZotHTTPConfig, buildRegistryURL, pingRegistry) and
returns their errors.
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/server/health.go`:
- Around line 182-187: The detectZotScheme function currently treats any
presence of the "tls" key as enabled; update detectZotScheme to type-assert and
validate the value (e.g., check if httpData["tls"] is a bool and true, or handle
string equivalents like "true"/"false" if needed) and only return "https" when
the asserted value indicates TLS is enabled; otherwise return "http". Locate
detectZotScheme in internal/server/health.go and mirror the defensive checks
used in parseZotAddress/parseZotPort to avoid treating false/null/empty values
as enabled.
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/server/health.go`:
- Around line 98-121: In HealthRegistrar.checkRegistry, allow HTTP 401 from the
registry's /v2/ endpoint to be treated as healthy: after performing
client.Get(registryURL) and obtaining resp, change the status check so that a
2xx status OR resp.StatusCode == 401 returns nil (healthy); only return an error
for other non-2xx/non-401 codes. Keep the existing response body close handling
and error wrapping behavior around client.Get and fmt.Errorf.
Signed-off-by: intojhanurag <aojharaj2004@gmail.com>
Contributor
Author
|
Hey @bupd , Review Appreciated :) |
Contributor
Author
|
Need to resolve conflicts ! I will do it soon :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Satellite needs standard health check endpoints for Kubernetes liveness/readiness probes and monitoring systems.
closed:
#240
Summary by cubic
Adds a lightweight health server with /health (liveness) and /ready (readiness) endpoints for Kubernetes and monitoring. Readiness checks the local registry, Ground Control, and initial state sync, and the port is configurable.
New Features
Migration
Written for commit 0466f85. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.