Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package main

import (
"cmp"
"context"

"github.com/spf13/viper"
Expand All @@ -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() {
Expand Down
28 changes: 22 additions & 6 deletions src/control-plane-services/helm-reval/cmd/reval/cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Comment thread
priyaselvaganesan marked this conversation as resolved.

"github.com/NVIDIA/nvcf/src/control-plane-services/helm-reval/pkg/authorizers"
"github.com/NVIDIA/nvcf/src/control-plane-services/helm-reval/pkg/httpapi"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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}
Expand All @@ -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)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

assert.Equal(t, http.StatusOK, w.Code)
assert.Equal(t, "application/json", w.Header().Get("Content-Type"))
Expand All @@ -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"))
Expand Down
Loading