From a4573b178f6abd0c85ff0c9da5b365865de31495 Mon Sep 17 00:00:00 2001 From: hermanngeorge15 Date: Thu, 12 Mar 2026 11:46:07 +0100 Subject: [PATCH] fix: handle swallowed errors, add metrics and discovery tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Check LastInsertId/RowsAffected errors in tenant apikey store - Check RowsAffected error in tenant Delete - Check io.ReadAll error in security osv QueryByGitCommit - Add metrics middleware tests (normalizePath, Flush, status recording) - Add discovery package tests (image inference, annotation parsing) - Middleware coverage: 45.7% → 100% - Discovery coverage: 0% → 55.6% Co-Authored-By: Claude Opus 4.6 --- internal/discovery/discovery_test.go | 156 +++++++++++++++++++++++++++ internal/middleware/metrics_test.go | 113 +++++++++++++++++++ internal/security/osv.go | 5 +- internal/tenant/apikey.go | 10 +- internal/tenant/tenant.go | 5 +- 5 files changed, 285 insertions(+), 4 deletions(-) create mode 100644 internal/discovery/discovery_test.go create mode 100644 internal/middleware/metrics_test.go diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go new file mode 100644 index 0000000..67c2d5f --- /dev/null +++ b/internal/discovery/discovery_test.go @@ -0,0 +1,156 @@ +package discovery + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" +) + +func TestInferRepoFromImage_GHCR(t *testing.T) { + got := inferRepoFromImage("ghcr.io/org/app:v1.0") + if got != "github.com/org/app" { + t.Errorf("expected github.com/org/app, got %q", got) + } +} + +func TestInferRepoFromImage_GitLab(t *testing.T) { + got := inferRepoFromImage("registry.gitlab.com/org/service:latest") + if got != "gitlab.com/org/service" { + t.Errorf("expected gitlab.com/org/service, got %q", got) + } +} + +func TestInferRepoFromImage_DockerHub(t *testing.T) { + got := inferRepoFromImage("docker.io/org/app:v2.0") + if got != "github.com/org/app" { + t.Errorf("expected github.com/org/app, got %q", got) + } +} + +func TestInferRepoFromImage_UnknownRegistry(t *testing.T) { + got := inferRepoFromImage("custom.registry.io/org/app:v1") + if got != "" { + t.Errorf("expected empty for unknown registry, got %q", got) + } +} + +func TestInferRepoFromImage_TooFewParts(t *testing.T) { + got := inferRepoFromImage("nginx:latest") + if got != "" { + t.Errorf("expected empty for short image ref, got %q", got) + } +} + +func TestInferRepoFromImage_Digest(t *testing.T) { + got := inferRepoFromImage("ghcr.io/org/app@sha256:abc123") + if got != "github.com/org/app" { + t.Errorf("expected github.com/org/app, got %q", got) + } +} + +func TestImageWithoutTag(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"ghcr.io/org/app:v1.0", "ghcr.io/org/app"}, + {"ghcr.io/org/app@sha256:abc", "ghcr.io/org/app"}, + {"ghcr.io/org/app", "ghcr.io/org/app"}, + {"localhost:5000/app:latest", "localhost:5000/app"}, + } + for _, tt := range tests { + got := imageWithoutTag(tt.input) + if got != tt.want { + t.Errorf("imageWithoutTag(%q) = %q, want %q", tt.input, got, tt.want) + } + } +} + +func TestDiscoverFromWorkload_WithAnnotation(t *testing.T) { + annotations := map[string]string{ + AnnotationRepo: "github.com/my-org/my-repo", + AnnotationName: "custom-name", + } + containers := []corev1.Container{ + {Image: "ghcr.io/other/image:v1"}, + } + + svcs := discoverFromWorkload("deploy-name", annotations, containers) + + if len(svcs) != 1 { + t.Fatalf("expected 1 service, got %d", len(svcs)) + } + if svcs[0].Name != "custom-name" { + t.Errorf("expected name 'custom-name', got %q", svcs[0].Name) + } + if svcs[0].Repo != "github.com/my-org/my-repo" { + t.Errorf("expected repo from annotation, got %q", svcs[0].Repo) + } +} + +func TestDiscoverFromWorkload_AnnotationRepoOnly(t *testing.T) { + annotations := map[string]string{ + AnnotationRepo: "github.com/org/repo", + } + + svcs := discoverFromWorkload("my-deploy", annotations, nil) + + if len(svcs) != 1 { + t.Fatalf("expected 1 service, got %d", len(svcs)) + } + // Falls back to workload name when AnnotationName is absent. + if svcs[0].Name != "my-deploy" { + t.Errorf("expected name 'my-deploy', got %q", svcs[0].Name) + } +} + +func TestDiscoverFromWorkload_InferredFromImage(t *testing.T) { + annotations := map[string]string{} + containers := []corev1.Container{ + {Image: "ghcr.io/org/service:v1.2.3"}, + } + + svcs := discoverFromWorkload("my-deploy", annotations, containers) + + if len(svcs) != 1 { + t.Fatalf("expected 1 service, got %d", len(svcs)) + } + if svcs[0].Repo != "github.com/org/service" { + t.Errorf("expected inferred repo, got %q", svcs[0].Repo) + } + if svcs[0].Registry != "ghcr.io/org/service" { + t.Errorf("expected registry 'ghcr.io/org/service', got %q", svcs[0].Registry) + } +} + +func TestDiscoverFromWorkload_NoAnnotationUnknownImage(t *testing.T) { + annotations := map[string]string{} + containers := []corev1.Container{ + {Image: "nginx:latest"}, + } + + svcs := discoverFromWorkload("web", annotations, containers) + + if len(svcs) != 0 { + t.Errorf("expected 0 services for unknown image, got %d", len(svcs)) + } +} + +func TestDiscoverFromWorkload_EmptyAnnotationRepo(t *testing.T) { + // Empty AnnotationRepo should fall through to image inference. + annotations := map[string]string{ + AnnotationRepo: "", + } + containers := []corev1.Container{ + {Image: "ghcr.io/org/app:v1"}, + } + + svcs := discoverFromWorkload("deploy", annotations, containers) + + if len(svcs) != 1 { + t.Fatalf("expected 1 service from image inference, got %d", len(svcs)) + } + if svcs[0].Repo != "github.com/org/app" { + t.Errorf("expected inferred repo, got %q", svcs[0].Repo) + } +} diff --git a/internal/middleware/metrics_test.go b/internal/middleware/metrics_test.go new file mode 100644 index 0000000..c283164 --- /dev/null +++ b/internal/middleware/metrics_test.go @@ -0,0 +1,113 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func TestNormalizePath_API(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"/api/v1/services/my-svc/releases", "/api/v1/services/{name}/releases"}, + {"/api/v1/services/other", "/api/v1/services/{name}"}, + {"/api/v1/timeline", "/api/v1/timeline"}, + {"/api/health", "/api/health"}, + } + for _, tt := range tests { + got := normalizePath(tt.input) + if got != tt.want { + t.Errorf("normalizePath(%q) = %q, want %q", tt.input, got, tt.want) + } + } +} + +func TestNormalizePath_SSE(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"/sse", "/sse"}, + {"/sse/some-session-id", "/sse"}, + } + for _, tt := range tests { + got := normalizePath(tt.input) + if got != tt.want { + t.Errorf("normalizePath(%q) = %q, want %q", tt.input, got, tt.want) + } + } +} + +func TestNormalizePath_Message(t *testing.T) { + got := normalizePath("/message/abc123") + if got != "/message" { + t.Errorf("normalizePath(/message/abc123) = %q, want /message", got) + } +} + +func TestNormalizePath_Dashboard(t *testing.T) { + got := normalizePath("/dashboard/settings") + if got != "/dashboard" { + t.Errorf("normalizePath(/dashboard/settings) = %q, want /dashboard", got) + } +} + +func TestNormalizePath_UnknownCapped(t *testing.T) { + // Unknown paths with many segments get capped at 3. + got := normalizePath("/foo/bar/baz/qux/extra") + if got != "/foo/bar/baz" { + t.Errorf("normalizePath(/foo/bar/baz/qux/extra) = %q, want /foo/bar/baz", got) + } +} + +func TestNormalizePath_ShortPathPassThrough(t *testing.T) { + tests := []string{"/", "/health", "/metrics", "/foo/bar"} + for _, p := range tests { + got := normalizePath(p) + if got != p { + t.Errorf("normalizePath(%q) = %q, want passthrough", p, got) + } + } +} + +func TestMetrics_RecordsStatus(t *testing.T) { + handler := Metrics(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTeapot) + })) + + req := httptest.NewRequest(http.MethodGet, "/health", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusTeapot { + t.Errorf("expected status 418, got %d", rec.Code) + } +} + +func TestMetrics_DefaultStatus(t *testing.T) { + handler := Metrics(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("ok")) + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Errorf("expected default status 200, got %d", rec.Code) + } +} + +func TestStatusRecorder_Flush(t *testing.T) { + // httptest.ResponseRecorder implements http.Flusher. + rec := httptest.NewRecorder() + sr := &statusRecorder{ResponseWriter: rec, status: 200} + // Should not panic. + sr.Flush() + + if !rec.Flushed { + t.Error("expected Flush to delegate to underlying writer") + } +} diff --git a/internal/security/osv.go b/internal/security/osv.go index 7bdabcb..0a393e8 100644 --- a/internal/security/osv.go +++ b/internal/security/osv.go @@ -198,7 +198,10 @@ func (c *Client) QueryByGitCommit(ctx context.Context, repoURL, commitHash strin return nil, fmt.Errorf("osv commit query: HTTP %d", resp.StatusCode) } - respBody, _ := io.ReadAll(resp.Body) + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("read commit response: %w", err) + } var osvResp osvQueryResponse if err := json.Unmarshal(respBody, &osvResp); err != nil { return nil, err diff --git a/internal/tenant/apikey.go b/internal/tenant/apikey.go index bb693ba..3ba229d 100644 --- a/internal/tenant/apikey.go +++ b/internal/tenant/apikey.go @@ -67,7 +67,10 @@ func (ks *KeyStore) Generate(tenantID int64) (rawKey string, key *APIKey, err er return "", nil, fmt.Errorf("store key: %w", err) } - id, _ := result.LastInsertId() + id, err := result.LastInsertId() + if err != nil { + return "", nil, fmt.Errorf("get key id: %w", err) + } return raw, &APIKey{ ID: id, TenantID: tenantID, @@ -121,7 +124,10 @@ func (ks *KeyStore) Revoke(keyID int64) error { if err != nil { return err } - rows, _ := result.RowsAffected() + rows, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("check rows affected: %w", err) + } if rows == 0 { return fmt.Errorf("key not found") } diff --git a/internal/tenant/tenant.go b/internal/tenant/tenant.go index fdc44c8..da60eed 100644 --- a/internal/tenant/tenant.go +++ b/internal/tenant/tenant.go @@ -123,7 +123,10 @@ func (s *Store) Delete(name string) error { if err != nil { return err } - rows, _ := result.RowsAffected() + rows, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("check rows affected: %w", err) + } if rows == 0 { return fmt.Errorf("tenant %q not found", name) }