From df52317bfc45d9231c9bdb9cf4386bb31c1efa6a Mon Sep 17 00:00:00 2001 From: priyaselvaganesan Date: Tue, 4 Aug 2026 10:01:57 -0700 Subject: [PATCH 1/2] feat(helm-reval): serve /info on the api port with consistent version and commit --- .../helm-reval/cmd/reval-service/BUILD.bazel | 1 + .../helm-reval/cmd/reval-service/main.go | 5 ++-- .../helm-reval/cmd/reval/cli/server.go | 10 +++++-- .../cmd/reval/cli/server_internal_test.go | 29 +++++++------------ 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/control-plane-services/helm-reval/cmd/reval-service/BUILD.bazel b/src/control-plane-services/helm-reval/cmd/reval-service/BUILD.bazel index 557a16034..6734a9eec 100644 --- a/src/control-plane-services/helm-reval/cmd/reval-service/BUILD.bazel +++ b/src/control-plane-services/helm-reval/cmd/reval-service/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//src/control-plane-services/helm-reval/pkg/authorizers", "//src/control-plane-services/helm-reval/pkg/reval/config", "//src/control-plane-services/helm-reval/pkg/telemetry/logging", + "@com_github_nvidia_nvcf_src_libraries_go_lib//pkg/version", "@com_github_spf13_viper//:viper", "@org_uber_go_zap//:zap", ], diff --git a/src/control-plane-services/helm-reval/cmd/reval-service/main.go b/src/control-plane-services/helm-reval/cmd/reval-service/main.go index 9675d1152..f8927a860 100644 --- a/src/control-plane-services/helm-reval/cmd/reval-service/main.go +++ b/src/control-plane-services/helm-reval/cmd/reval-service/main.go @@ -25,11 +25,12 @@ import ( "github.com/NVIDIA/nvcf/src/control-plane-services/helm-reval/pkg/authorizers" "github.com/NVIDIA/nvcf/src/control-plane-services/helm-reval/pkg/reval/config" "github.com/NVIDIA/nvcf/src/control-plane-services/helm-reval/pkg/telemetry/logging" + golibversion "github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/version" ) var ( - Version = "dev" - GitCommit = "dev" + Version = golibversion.Version + GitCommit = golibversion.GitHash ) func main() { diff --git a/src/control-plane-services/helm-reval/cmd/reval/cli/server.go b/src/control-plane-services/helm-reval/cmd/reval/cli/server.go index 86734c136..251e763d8 100644 --- a/src/control-plane-services/helm-reval/cmd/reval/cli/server.go +++ b/src/control-plane-services/helm-reval/cmd/reval/cli/server.go @@ -36,7 +36,7 @@ import ( chiMiddleware "github.com/go-chi/chi/v5/middleware" - nvcfversion "github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/version" + golibversion "github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/version" "github.com/NVIDIA/nvcf/src/control-plane-services/helm-reval/pkg/authorizers" "github.com/NVIDIA/nvcf/src/control-plane-services/helm-reval/pkg/httpapi" @@ -119,6 +119,7 @@ func runServer(cfg *config.RevalConfig, v *viper.Viper, factory AuthorizerFactor router := chi.NewRouter() router.NotFound(httpapi.ServeNotFound) + serveInfo(router) oldGrpcMetricsMiddleware := metrics.CreateOldGrpcMetricsMiddleWare(logger, meter) @@ -159,6 +160,11 @@ func runServer(cfg *config.RevalConfig, v *viper.Viper, factory AuthorizerFactor return nil } +// serveInfo mounts the unauthenticated GET /info on the API router so it is reachable externally through the ingress. +func serveInfo(router chi.Router) { + router.Get("/info", golibversion.Handler().ServeHTTP) +} + func serveManagementRoutes(logger *zap.Logger, loggerAtomicLevel *zap.AtomicLevel, cfg config.HTTPConfig) *http.Server { router := chi.NewRouter() router.Get("/healthz", func(w http.ResponseWriter, r *http.Request) { @@ -169,8 +175,6 @@ func serveManagementRoutes(logger *zap.Logger, loggerAtomicLevel *zap.AtomicLeve } }) - router.Get("/info", nvcfversion.Handler().ServeHTTP) - router.Get("/log_level", loggerAtomicLevel.ServeHTTP) httpServer := &http.Server{ diff --git a/src/control-plane-services/helm-reval/cmd/reval/cli/server_internal_test.go b/src/control-plane-services/helm-reval/cmd/reval/cli/server_internal_test.go index 008be671d..dd5db2f31 100644 --- a/src/control-plane-services/helm-reval/cmd/reval/cli/server_internal_test.go +++ b/src/control-plane-services/helm-reval/cmd/reval/cli/server_internal_test.go @@ -26,6 +26,7 @@ import ( "testing" "time" + "github.com/go-chi/chi/v5" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -110,17 +111,15 @@ func TestServeManagementRoutes_UnknownRoute(t *testing.T) { assert.Equal(t, http.StatusNotFound, w.Code) } -func TestServeManagementRoutes_Info(t *testing.T) { - logger := zap.NewNop() - atomicLevel := zap.NewAtomicLevel() - cfg := config.HTTPConfig{ManagementPort: 0, Local: false} +// ── serveInfo ───────────────────────────────────────────────────────────────── - server := serveManagementRoutes(logger, &atomicLevel, cfg) - require.NotNil(t, server) +func TestServeInfo(t *testing.T) { + router := chi.NewRouter() + serveInfo(router) w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/info", nil) - server.Handler.ServeHTTP(w, r) + router.ServeHTTP(w, r) assert.Equal(t, http.StatusOK, w.Code) assert.Equal(t, "application/json", w.Header().Get("Content-Type")) @@ -129,27 +128,21 @@ func TestServeManagementRoutes_Info(t *testing.T) { // for any empty field, so all three values are guaranteed non-empty. var info map[string]string require.NoError(t, json.Unmarshal(w.Body.Bytes(), &info)) - assert.Contains(t, info, "service") - assert.Contains(t, info, "version") - assert.Contains(t, info, "commit") for _, field := range []string{"service", "version", "commit"} { + assert.Contains(t, info, field) assert.NotEmpty(t, info[field], field+" must be populated") } } -func TestServeManagementRoutes_Info_RejectsNonGET(t *testing.T) { - logger := zap.NewNop() - atomicLevel := zap.NewAtomicLevel() - cfg := config.HTTPConfig{ManagementPort: 0, Local: false} - - server := serveManagementRoutes(logger, &atomicLevel, cfg) - require.NotNil(t, server) +func TestServeInfo_RejectsNonGET(t *testing.T) { + router := chi.NewRouter() + serveInfo(router) for _, method := range []string{http.MethodPost, http.MethodPut, http.MethodDelete} { t.Run(method, func(t *testing.T) { w := httptest.NewRecorder() r := httptest.NewRequest(method, "/info", nil) - server.Handler.ServeHTTP(w, r) + router.ServeHTTP(w, r) assert.Equal(t, http.StatusMethodNotAllowed, w.Code) assert.Equal(t, http.MethodGet, w.Header().Get("Allow")) From 7dbde9772db9729ac777e66bb166fa59fa0d972d Mon Sep 17 00:00:00 2001 From: priyaselvaganesan Date: Tue, 4 Aug 2026 10:39:45 -0700 Subject: [PATCH 2/2] fix(helm-reval): observe /info and align unstamped version fallback --- .../helm-reval/cmd/reval-service/main.go | 6 +++-- .../helm-reval/cmd/reval/cli/server.go | 24 ++++++++++++++----- .../cmd/reval/cli/server_internal_test.go | 20 ++++++++++++++-- 3 files changed, 40 insertions(+), 10 deletions(-) diff --git a/src/control-plane-services/helm-reval/cmd/reval-service/main.go b/src/control-plane-services/helm-reval/cmd/reval-service/main.go index f8927a860..4c957ac1e 100644 --- a/src/control-plane-services/helm-reval/cmd/reval-service/main.go +++ b/src/control-plane-services/helm-reval/cmd/reval-service/main.go @@ -16,6 +16,7 @@ package main import ( + "cmp" "context" "github.com/spf13/viper" @@ -28,9 +29,10 @@ import ( golibversion "github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/version" ) +// Fall back to "unknown" for unstamped builds, matching the go-lib /info handler. var ( - Version = golibversion.Version - GitCommit = golibversion.GitHash + Version = cmp.Or(golibversion.Version, "unknown") + GitCommit = cmp.Or(golibversion.GitHash, "unknown") ) func main() { diff --git a/src/control-plane-services/helm-reval/cmd/reval/cli/server.go b/src/control-plane-services/helm-reval/cmd/reval/cli/server.go index 251e763d8..31424d87b 100644 --- a/src/control-plane-services/helm-reval/cmd/reval/cli/server.go +++ b/src/control-plane-services/helm-reval/cmd/reval/cli/server.go @@ -119,14 +119,26 @@ func runServer(cfg *config.RevalConfig, v *viper.Viper, factory AuthorizerFactor router := chi.NewRouter() router.NotFound(httpapi.ServeNotFound) - serveInfo(router) oldGrpcMetricsMiddleware := metrics.CreateOldGrpcMetricsMiddleWare(logger, meter) + httpMetrics := metrics.CreateHttpMetricsMiddleWare(logger, meter) + otelTrace := tracing.NewOtelTraceMiddleware() + zapLogger := logging.NewZapLoggerMiddleware(logger) + + publicMiddlewares := chi.Chain( + httpMetrics, + otelTrace, + zapLogger, + render.SetContentType(render.ContentTypeJSON), + chiMiddleware.Recoverer, + ) + serveInfo(router, publicMiddlewares) + middlewares := chi.Chain( - metrics.CreateHttpMetricsMiddleWare(logger, meter), - tracing.NewOtelTraceMiddleware(), - logging.NewZapLoggerMiddleware(logger), + httpMetrics, + otelTrace, + zapLogger, authzMiddleware, render.SetContentType(render.ContentTypeJSON), // This middleware is the last one in order to recover after panic @@ -161,8 +173,8 @@ func runServer(cfg *config.RevalConfig, v *viper.Viper, factory AuthorizerFactor } // serveInfo mounts the unauthenticated GET /info on the API router so it is reachable externally through the ingress. -func serveInfo(router chi.Router) { - router.Get("/info", golibversion.Handler().ServeHTTP) +func serveInfo(router chi.Router, middlewares chi.Middlewares) { + router.With(middlewares...).Get("/info", golibversion.Handler().ServeHTTP) } func serveManagementRoutes(logger *zap.Logger, loggerAtomicLevel *zap.AtomicLevel, cfg config.HTTPConfig) *http.Server { diff --git a/src/control-plane-services/helm-reval/cmd/reval/cli/server_internal_test.go b/src/control-plane-services/helm-reval/cmd/reval/cli/server_internal_test.go index dd5db2f31..4a73bdfea 100644 --- a/src/control-plane-services/helm-reval/cmd/reval/cli/server_internal_test.go +++ b/src/control-plane-services/helm-reval/cmd/reval/cli/server_internal_test.go @@ -111,11 +111,27 @@ func TestServeManagementRoutes_UnknownRoute(t *testing.T) { assert.Equal(t, http.StatusNotFound, w.Code) } +// TestServeManagementRoutes_Info_NotFound locks in that /info is served on the API +// router, not the management router. +func TestServeManagementRoutes_Info_NotFound(t *testing.T) { + logger := zap.NewNop() + atomicLevel := zap.NewAtomicLevel() + cfg := config.HTTPConfig{ManagementPort: 0, Local: false} + + server := serveManagementRoutes(logger, &atomicLevel, cfg) + require.NotNil(t, server) + + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/info", nil) + server.Handler.ServeHTTP(w, r) + assert.Equal(t, http.StatusNotFound, w.Code) +} + // ── serveInfo ───────────────────────────────────────────────────────────────── func TestServeInfo(t *testing.T) { router := chi.NewRouter() - serveInfo(router) + serveInfo(router, chi.Chain()) w := httptest.NewRecorder() r := httptest.NewRequest(http.MethodGet, "/info", nil) @@ -136,7 +152,7 @@ func TestServeInfo(t *testing.T) { func TestServeInfo_RejectsNonGET(t *testing.T) { router := chi.NewRouter() - serveInfo(router) + serveInfo(router, chi.Chain()) for _, method := range []string{http.MethodPost, http.MethodPut, http.MethodDelete} { t.Run(method, func(t *testing.T) {