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
51 changes: 47 additions & 4 deletions cmd/campaign-service/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,15 @@ package main

import (
"context"
"fmt"
"log/slog"
"net/http"
"os"
"sync"

connsvcsvr "github.com/linuxfoundation/lfx-v2-campaign-service/gen/http/lfx_v2_campaign_service_connections/server"
svcsvr "github.com/linuxfoundation/lfx-v2-campaign-service/gen/http/lfx_v2_campaign_service_svc/server"
connsvc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_connections"
svc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_svc"
"github.com/linuxfoundation/lfx-v2-campaign-service/internal/container"
"github.com/linuxfoundation/lfx-v2-campaign-service/internal/infrastructure/config"
Expand All @@ -34,15 +37,34 @@ func StartServer(ctx context.Context, cfg *config.Config) error {
return err
}

// NOTE: debug.LogPayloads() is intentionally NOT applied. Every authenticated
// payload carries a BearerToken (a valid JWT), and the connection service's
// create/set-credential payloads carry plaintext provider credentials, so
// enabling it would leak those secrets into logs. debug.HTTP() IS still
// applied below, but in clue v1.2.1 it does not log headers or statuses — it
// only propagates the runtime /debug toggle into each request's context (so
// debug-level logs elsewhere activate); it decodes no payload.
endpoints := svc.NewEndpoints(cont.Service)
if cfg.Debug {
endpoints.Use(debug.LogPayloads())

// The container always initializes Connections (NewContainer wires it in both
// the DB and no-DB paths), so construct the endpoints unconditionally. Fail
// loudly if it's unexpectedly nil rather than silently skipping the mount —
// a mis-wired container should crash at startup, not serve 404s on the
// connection routes.
if cont.Connections == nil {
return fmt.Errorf("container misconfigured: Connections service is nil")
}
connEndpoints := connsvc.NewEndpoints(cont.Connections)

return handleHTTPServer(ctx, cfg, endpoints, cont)
return handleHTTPServer(ctx, cfg, endpoints, connEndpoints, cont)
}

func handleHTTPServer(ctx context.Context, cfg *config.Config, endpoints *svc.Endpoints, cont *container.Container) error {
// buildMux constructs the Goa muxer and mounts BOTH the campaign service and the
// connection service onto it. It is a seam so tests can assert that the
// connection routes are actually reachable (the bug this fixes — routes that
// compile but are never mounted return 404) without standing up a full server.
// It returns an error only for a programmer-level mis-wiring (nil connEndpoints).
func buildMux(ctx context.Context, cfg *config.Config, endpoints *svc.Endpoints, connEndpoints *connsvc.Endpoints) (goahttp.Muxer, error) {
mux := goahttp.NewMuxer()
if cfg.Debug {
debug.MountPprofHandlers(debug.Adapt(mux))
Expand Down Expand Up @@ -70,6 +92,27 @@ func handleHTTPServer(ctx context.Context, cfg *config.Config, endpoints *svc.En
)
svcsvr.Mount(mux, server)

// Mount the connection routes unconditionally. connEndpoints is always
// non-nil (StartServer constructs it and fails loudly if the service is
// mis-wired), and the connection service is wired even without a database
// (with a nil repo) so its routes return the typed 503 rather than a bare
// 404. A nil here would be a programmer error, so fail loudly rather than
// silently skipping the mount and serving 404s.
if connEndpoints == nil {
Comment thread
mrautela365 marked this conversation as resolved.
return nil, fmt.Errorf("buildMux: connEndpoints is nil (connection routes would be unmounted)")
}
connServer := connsvcsvr.New(connEndpoints, mux, goahttp.RequestDecoder, goahttp.ResponseEncoder, eh, nil)
connsvcsvr.Mount(mux, connServer)
Comment thread
mrautela365 marked this conversation as resolved.

return mux, nil
}

func handleHTTPServer(ctx context.Context, cfg *config.Config, endpoints *svc.Endpoints, connEndpoints *connsvc.Endpoints, cont *container.Container) error {
mux, err := buildMux(ctx, cfg, endpoints, connEndpoints)
if err != nil {
return err
}

var handler http.Handler = mux
handler = middleware.RequestIDMiddleware()(handler)
if cfg.Debug {
Expand Down
67 changes: 67 additions & 0 deletions cmd/campaign-service/server_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright The Linux Foundation and each contributor to LFX.
// SPDX-License-Identifier: MIT

package main

import (
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"

connsvc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_connections"
svc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_svc"
"github.com/linuxfoundation/lfx-v2-campaign-service/internal/infrastructure/config"
"github.com/linuxfoundation/lfx-v2-campaign-service/internal/service"
)

// TestConnectionRoutesAreMounted locks in the invariant this PR establishes: the
// connection routes are actually reachable on the mux. The bug being fixed —
// generated routes that compile but are never mounted — is invisible to the
// service-layer tests (which call handlers directly), so without this test a
// future deletion of the connsvcsvr.Mount call would silently reintroduce the
// 404 regression. We assert a known connection route resolves to a real handler
// (any non-404 status, e.g. 401/503, proves it is mounted).
func TestConnectionRoutesAreMounted(t *testing.T) {
endpoints := svc.NewEndpoints(service.NewCampaignService(nil))
// The no-DB connection service is a real Service whose routes return a typed
// 503 rather than 404 — perfect for proving the route is mounted.
connEndpoints := connsvc.NewEndpoints(service.NewConnectionService(nil, nil))

mux, err := buildMux(context.Background(), &config.Config{}, endpoints, connEndpoints)
if err != nil {
t.Fatalf("buildMux: %v", err)
}

// One route per mounted server is enough to lock in the mount.
cases := []struct {
name string
method string
path string
}{
{"connection google-ads create", http.MethodPost, "/projects/proj-123/connection-google-ads"},
{"campaign health livez", http.MethodGet, "/livez"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
rec := httptest.NewRecorder()
req := httptest.NewRequest(tc.method, tc.path, strings.NewReader("{}"))

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.

[nit] Cosmetic only — take-it-or-leave-it. The table sets a JSON body (strings.NewReader("{}")) and Content-Type: application/json for every case, including the GET /livez health row, which ignores the body. Harmless (it still returns 200), just slightly incongruous for a GET probe. Optional: set the body/content-type per-case, or add a one-line comment noting connsvcsvr.Mount is all-or-nothing so a single sentinel route is sufficient to detect a dropped mount. Neither is worth a revision.

req.Header.Set("Content-Type", "application/json")
mux.ServeHTTP(rec, req)
if rec.Code == http.StatusNotFound {
t.Errorf("%s %s returned 404 — route is not mounted", tc.method, tc.path)
}
})
}
}

// TestBuildMuxNilConnEndpointsFailsLoud verifies the fail-loud guard: a nil
// connEndpoints (a programmer-level mis-wiring) returns an error rather than
// silently building a mux with the connection routes unmounted.
func TestBuildMuxNilConnEndpointsFailsLoud(t *testing.T) {
endpoints := svc.NewEndpoints(service.NewCampaignService(nil))
if _, err := buildMux(context.Background(), &config.Config{}, endpoints, nil); err == nil {
t.Fatal("expected buildMux to fail loudly when connEndpoints is nil, got nil error")
}
}
12 changes: 11 additions & 1 deletion docs/knowledge/code/cmd-campaign-service.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,16 @@ resource: "cmd/campaign-service"

# cmd/campaign-service

The LFX V2 Campaign Service.
The LFX V2 Campaign Service. `server.go` builds the HTTP server and mounts each
wired Goa service's handlers — currently the health and connection servers.
Every service the container wires must also be mounted here: a service
constructed in the container but not mounted is unreachable (its routes 404)
even though the code compiles, which is the bug this change fixes for the
connection routes. `debug.LogPayloads()` is intentionally not applied to any
service: payloads carry bearer tokens and (for connections) plaintext provider
credentials, so DEBUG payload logging would leak secrets. `debug.HTTP()` is
still applied, but in clue v1.2.1 it does not log headers or statuses — it only
propagates the runtime `/debug` toggle into the request context (activating
debug-level logs elsewhere); it decodes no payload.

See [cmd/campaign-service](../../../cmd/campaign-service).
3 changes: 3 additions & 0 deletions docs/knowledge/log.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ strictly parsed to reject impossible calendar values; name lookups propagate
errors instead of masking them as not-found. Added the
`internal/platform/twitter` code concept and index entry.

**Update** — Mount connection routes in the HTTP server (LFXV2-2556): the
`cmd/campaign-service` concept now notes that every container-wired service
must also be mounted in `server.go`, or its routes 404 despite compiling.

**Update** — Dropped the Goa CLI path allowlist; twitter-api-secret FP is
fingerprint-only in `.gitleaksignore`. Clarified `.grype.yaml` rationale
Expand Down
Loading