Skip to content
Open
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
2 changes: 1 addition & 1 deletion controller/Containerfile
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ RUN --mount=type=cache,target=/opt/app-root/src/go/pkg/mod,sharing=locked,uid=1
CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} \
go build -a \
-ldflags "-X main.version=${GIT_VERSION} -X main.gitCommit=${GIT_COMMIT} -X main.buildDate=${BUILD_DATE}" \
-o router cmd/router/main.go
-o router ./cmd/router

FROM registry.access.redhat.com/ubi9/ubi-micro:9.8-1782840931@sha256:35de56a9413112f1474e392ebc35e0cf6f0fb484c8e8877bbae59b513694b41f
WORKDIR /
Expand Down
4 changes: 2 additions & 2 deletions controller/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ build-operator:
.PHONY: build
build: manifests generate fmt vet ## Build manager binary.
go build -ldflags "$(LDFLAGS)" -o bin/manager cmd/main.go
go build -ldflags "$(LDFLAGS)" -o bin/router cmd/router/main.go
go build -ldflags "$(LDFLAGS)" -o bin/router ./cmd/router
go build -ldflags "$(LDFLAGS)" -o bin/exporter-set-controller cmd/exporter-set-controller/main.go

.PHONY: run
Expand All @@ -122,7 +122,7 @@ run: manifests generate fmt vet ## Run a controller from your host.

.PHONY: run-router
run-router: manifests generate fmt vet ## Run a router from your host.
go run ./cmd/router/main.go
go run ./cmd/router

# If you wish to build the manager image targeting other platforms you can use the --platform flag.
# (i.e. docker build --platform linux/arm64). However, you must enable docker buildKit for it.
Expand Down
11 changes: 11 additions & 0 deletions controller/cmd/router/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ func main() {
opts := zap.Options{}
opts.BindFlags(flag.CommandLine)

var metricsAddr string
flag.StringVar(&metricsAddr, "metrics-bind-address", "0",
"The address the metric endpoint binds to. Use :8080 to enable. Set to 0 to disable.")

flag.Parse()

ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)).WithValues("component", "router"))
Expand All @@ -58,6 +62,13 @@ func main() {
"buildDate", buildDate,
)

if listenAddr, err := startMetricsServer(metricsAddr); err != nil {
logger.Error(err, "failed to start metrics server", "bindAddress", metricsAddr)
os.Exit(1)
} else if listenAddr != "" {
logger.Info("Serving metrics server", "bindAddress", listenAddr)
}

cfg := ctrl.GetConfigOrDie()
client, err := kclient.New(cfg, kclient.Options{})
if err != nil {
Expand Down
49 changes: 49 additions & 0 deletions controller/cmd/router/metrics.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
Copyright 2026. The Jumpstarter Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package main

import (
"net"
"net/http"

"github.com/prometheus/client_golang/prometheus/promhttp"
)

// startMetricsServer starts an HTTP server exposing GET /metrics.
// addr "0" or empty disables the server and returns ("", nil).
// addr ending with ":0" binds an ephemeral port; the returned listen address
// is host:port suitable for http.Get.
func startMetricsServer(addr string) (string, error) {
if addr == "" || addr == "0" {
return "", nil
}

ln, err := net.Listen("tcp", addr)
if err != nil {
return "", err
}

mux := http.NewServeMux()
mux.Handle("/metrics", promhttp.Handler())

srv := &http.Server{Handler: mux}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set at least ReadHeaderTimeout and IdleTimeout:

srv := &http.Server{
    Handler:           mux,
    ReadHeaderTimeout: 10 * time.Second,
    IdleTimeout:       60 * time.Second,
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idle timeout may need to be higher, i.e. it really depends on prometheus doing keepalive & the periodicity of checks, I'd give it 5 minutes perhaps.

go func() {
_ = srv.Serve(ln)
Comment on lines +40 to +45

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Files:"
git ls-files | rg '(^|/)metrics\.go$|Makefile$|.*router.*metrics.*' || true

echo
echo "Target file context:"
if [ -f controller/cmd/router/metrics.go ]; then
  nl -ba controller/cmd/router/metrics.go | sed -n '1,140p'
fi

echo
echo "References to metrics listener/port:"
rg -n 'metrics|:8080|http\.Serve|http\.NewServeMux|promhttp\.Handler|IdleTimeout|ReadHeaderTimeout|ReadTimeout|WriteTimeout' . --glob '!vendor/**' --glob '!node_modules/**' || true

Repository: jumpstarter-dev/jumpstarter

Length of output: 637


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Package build/test/lint references:"
if [ -f controller/Makefile ]; then
  cat controller/Makefile | sed -n '1,220p'
fi

echo
echo "Target file content:"
if [ -f controller/cmd/router/metrics.go ]; then
  awk '{printf "%5d\t%s\n", NR, $0}' controller/cmd/router/metrics.go
fi

echo
echo "Router metrics bind tests:"
if [ -f controller/deploy/operator/internal/controller/jumpstarter/router_metrics_bind_test.go ]; then
  awk '{printf "%5d\t%s\n", NR, $0}' controller/deploy/operator/internal/controller/jumpstarter/router_metrics_bind_test.go
fi

Repository: jumpstarter-dev/jumpstarter

Length of output: 13925


Bound request timeouts on the metrics server.

controller/cmd/router/metrics.go:43 creates a zero-valued http.Server, so slow headers, reads, writes, and idle connections can remain unbounded. Add ReadHeaderTimeout, ReadTimeout, WriteTimeout, and IdleTimeout, or implement a timeout wrapper before srv.Serve(ln).

🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 42-42: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{Handler: mux}
Note: [CWE-400] Uncontrolled Resource Consumption.

(http-server-missing-read-timeout-go)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controller/cmd/router/metrics.go` around lines 40 - 45, Configure explicit
ReadHeaderTimeout, ReadTimeout, WriteTimeout, and IdleTimeout values on the
http.Server created in the metrics server setup before calling srv.Serve(ln),
ensuring slow requests and idle connections are bounded.

Source: Linters/SAST tools

}()
Comment on lines +44 to +46

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go func() {
    if err := srv.Serve(ln); err != nil && err != http.ErrServerClosed {
        ctrl.Log.WithName("metrics").Error(err, "metrics server stopped unexpectedly")
    }
}()


return ln.Addr().String(), nil
}
Comment on lines +30 to +49

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startMetricsServer returns only (string, error) but creates an http.Server and launches srv.Serve(ln) in a detached goroutine. The caller has no way to call srv.Shutdown(ctx) on SIGINT/SIGTERM, so the server never gets a graceful shutdown.

77 changes: 77 additions & 0 deletions controller/cmd/router/metrics_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
Copyright 2026. The Jumpstarter Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package main

import (
"io"
"net/http"
"strings"
"testing"
"time"
)

func TestMetricsEndpointServesPrometheusText(t *testing.T) {
addr, err := startMetricsServer("127.0.0.1:0")
if err != nil {
t.Fatalf("startMetricsServer: %v", err)
}
if addr == "" {
t.Fatal("expected non-empty listen address")
}

client := &http.Client{Timeout: 2 * time.Second}
var resp *http.Response
var lastErr error
for i := 0; i < 20; i++ {
resp, lastErr = client.Get("http://" + addr + "/metrics")
if lastErr == nil {
break
}
time.Sleep(50 * time.Millisecond)
}
if lastErr != nil {
t.Fatalf("GET /metrics: %v", lastErr)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
t.Fatalf("status = %d, want 200", resp.StatusCode)
}
ct := resp.Header.Get("Content-Type")
if !strings.Contains(ct, "text/plain") && !strings.Contains(ct, "openmetrics") {
t.Fatalf("unexpected Content-Type %q", ct)
}
body, err := io.ReadAll(resp.Body)
if err != nil {
t.Fatalf("read body: %v", err)
}
text := string(body)
// Default Go process metrics should appear; do not require undocumented jumpstarter_* series.
if !strings.Contains(text, "go_") && !strings.Contains(text, "process_") && !strings.Contains(text, "promhttp_") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If go_* metrics were removed but promhttp_* remained, the test would still pass. Assert each required family independently.

t.Fatalf("expected Prometheus exposition with go_/process_ metrics, got:\n%s", text)
Comment on lines +58 to +65

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate file =="
fd -a 'metrics_test.go' . || true

echo "== file context =="
cat -n controller/cmd/router/metrics_test.go | sed -n '1,120p'

echo "== go.mod deps for prometheus parser =="
rg -n 'prometheus/client_golang|prometheus/common|expfmt|ParseMetricText' go.mod go.sum || true

echo "== related metrics handlers =="
rg -n 'prometheus|DefaultGatherers|http.Handler|MetricText|Metrics|promhttp' controller/cmd/router -S || true

Repository: jumpstarter-dev/jumpstarter

Length of output: 4295


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
samples = [
 ('contain_go_without_exp', 'go_abc process_def'),
 ('contain_process_without_exp', 'process_xyz'),
 ('contain_promhttp_without_exp', 'promhttp_request_duration_seconds'),
 ('valid_comment_like', '# HELP go_gc_duration_seconds summary\nhelp foo'),
 ('valid_comment_like_with_keyword', '# HELP go_gc_duration_seconds summary\n# PROCESS foo'),
 ('single_valid_metric_lines', 'go_info{version="1.0"} 1\nprocess_start_time_seconds 123\n'),
 ('single_metric_missing_help', 'go_info{version="1.0"} 1\n'),
 ('single_metric_with_extra_label_quote', 'go_info{version=1.0"} 1\n'),
 ('single_metric_with_extra_label_quote_backwards', 'go_info{version="1.0 1\n'),
]
# Python does not use Go substrings; emulate exact `strings.Contains(text, "go_")` logic separately.
import re
for name,s in samples:
    contains_go = 'go_' in s
    contains_process = 'process_' in s
    contains_promhttp = 'promhttp_' in s
    # crude prometheus comment/text parser based on common fields can reject malformed labels/types
    # Use a read-only behavioral probe simulating: substring check passes without parsing.
    print(f"{name}: contains={contains_go or contains_process or contains_promhttp} {s!r}")
PY

Repository: jumpstarter-dev/jumpstarter

Length of output: 889


Parse the metrics body with a Prometheus exposition parser.

The go_ / process_ / promhttp_ substring check can pass malformed text that is not valid Prometheus exposition, so it doesn’t validate the /metrics response contract. Parse the body and fail on parse errors.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controller/cmd/router/metrics_test.go` around lines 58 - 65, The metrics test
should validate the response as Prometheus exposition rather than checking for
metric-name substrings. Update the test around the body read and existing
strings.Contains assertion to parse text with the project’s Prometheus
exposition parser and fail the test when parsing returns an error.

}
}

func TestMetricsServerDisabledWhenAddrZero(t *testing.T) {
addr, err := startMetricsServer("0")
if err != nil {
t.Fatalf("startMetricsServer(0): %v", err)
}
if addr != "" {
t.Fatalf("expected empty addr when disabled, got %q", addr)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1030,8 +1030,11 @@ func (r *JumpstarterReconciler) createRouterDeployment(jumpstarter *operatorv1al
Image: jumpstarter.Spec.Routers.Image,
ImagePullPolicy: jumpstarter.Spec.Routers.ImagePullPolicy,
Command: []string{"/router"},
Env: envVars,
VolumeMounts: volumeMounts,
Args: []string{
"-metrics-bind-address=:8080",
},
Env: envVars,
VolumeMounts: volumeMounts,
Ports: []corev1.ContainerPort{
{
ContainerPort: 8083,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
Copyright 2026. The Jumpstarter Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package jumpstarter

import (
"testing"

operatorv1alpha1 "github.com/jumpstarter-dev/jumpstarter/controller/deploy/operator/api/v1alpha1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// Stdlib unit test (no envtest): asserts Router Deployment metrics bind for JEP-0013 Phase 2.
func TestRouterDeploymentMetricsBind(t *testing.T) {
r := &JumpstarterReconciler{}
js := &operatorv1alpha1.Jumpstarter{
ObjectMeta: metav1.ObjectMeta{
Name: "jumpstarter",
Namespace: "jumpstarter-lab",
},
Spec: operatorv1alpha1.JumpstarterSpec{
Routers: operatorv1alpha1.RoutersConfig{
Image: "example.com/router:test",
ImagePullPolicy: corev1.PullIfNotPresent,
Replicas: 1,
},
},
}

dep := r.createRouterDeployment(js, 0)
Comment on lines +27 to +44

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== candidate files =="
fd -a '_test\.go$' controller/deploy/operator/internal/controller 2>/dev/null | sed 's#^\./##' | head -200 || true

echo "== target file excerpt =="
target='controller/deploy/operator/internal/controller/jumpstarter/router_metrics_bind_test.go'
if [ -f "$target" ]; then
  wc -l "$target"
  sed -n '1,140p' "$target"
else
  echo "target missing"
fi

echo "== package declarations in target dir tests =="
rg -n "^(package|func Test|var Environment|NewTestEnvironment|Setup|Suite)" controller/deploy/operator/internal/controller -g '*_test.go' | head -200

echo "== helper/envtest related references =="
rg -n "envtest|NewTestEnvironment|Setup|fake|Fake|clientset|NewClient|JumpstarterReconciler|Reconcile\\(" controller/deploy/operator -g '*_test.go' | head -300

Repository: jumpstarter-dev/jumpstarter

Length of output: 14214


Use envtest for this operator test.

This file is under controller/deploy/operator/internal/controller/jumpstarter, and the test directly constructs JumpstarterReconciler objects and calls createRouterDeployment. Move the assertion into the existing envtest-based suite or exercise creation through the fake Kubernetes API instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@controller/deploy/operator/internal/controller/jumpstarter/router_metrics_bind_test.go`
around lines 27 - 44, The TestRouterDeploymentMetricsBind test should use the
operator’s existing envtest-based suite instead of directly invoking
JumpstarterReconciler.createRouterDeployment. Move the assertion there, or
create the Jumpstarter resource through the fake Kubernetes API and validate the
resulting Deployment through the client.

Source: Coding guidelines

if dep == nil {
t.Fatal("expected non-nil deployment")
}
if len(dep.Spec.Template.Spec.Containers) == 0 {
t.Fatal("expected at least one container")
}

c := dep.Spec.Template.Spec.Containers[0]
foundArg := false
for _, arg := range c.Args {
if arg == "-metrics-bind-address=:8080" || arg == "--metrics-bind-address=:8080" {
foundArg = true
break
}
}
if !foundArg {
t.Fatalf("expected metrics-bind-address=:8080 in router args, got %#v", c.Args)
}

foundPort := false
for _, p := range c.Ports {
if p.Name == "metrics" {
foundPort = true
if p.ContainerPort != 8080 {
t.Fatalf("metrics port = %d, want 8080", p.ContainerPort)
}
break
}
}
if !foundPort {
t.Fatal("expected container port named metrics")
}
}
Loading