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..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" @@ -25,11 +26,13 @@ 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" ) +// Fall back to "unknown" for unstamped builds, matching the go-lib /info handler. var ( - Version = "dev" - GitCommit = "dev" + 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 86734c136..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 @@ -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" @@ -122,10 +122,23 @@ func runServer(cfg *config.RevalConfig, v *viper.Viper, factory AuthorizerFactor 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 @@ -159,6 +172,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, middlewares chi.Middlewares) { + router.With(middlewares...).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 +187,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..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 @@ -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,7 +111,9 @@ func TestServeManagementRoutes_UnknownRoute(t *testing.T) { assert.Equal(t, http.StatusNotFound, w.Code) } -func TestServeManagementRoutes_Info(t *testing.T) { +// 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} @@ -121,6 +124,18 @@ func TestServeManagementRoutes_Info(t *testing.T) { 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, chi.Chain()) + + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/info", nil) + router.ServeHTTP(w, r) assert.Equal(t, http.StatusOK, w.Code) assert.Equal(t, "application/json", w.Header().Get("Content-Type")) @@ -129,27 +144,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, chi.Chain()) 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"))