fix(preview): address review — error handling, logging, and cleanup in handler and service
- commit
- a4b282f2f240ae835eb5bb6e6b0bc1f2d5b20ace
- author
- brice · · 2026-03-19 05:28 UTC
- parent
- 686191e
- signature
- unsigned
2 files · +26 -12
M internal/app/run.go +0 -1
| @@ -1382,7 +1382,6 @@ func registerEventHandlers(ctx context.Context, svc *services, cfg Config) (func | |||
| 1382 | 1382 | svc.configSvc, | |
| 1383 | 1383 | svc.blobStorage, | |
| 1384 | 1384 | svc.previewService, | |
| 1385 | - | cfg.Server.RegistryDomain, | |
| 1386 | 1385 | ) | |
| 1387 | 1386 | ||
| 1388 | 1387 | // Dispatcher routes image push events to either auto-route or preview handler | |
| @@ -1382,7 +1382,6 @@ func registerEventHandlers(ctx context.Context, svc *services, cfg Config) (func | |||||
| 1382 | svc.configSvc, | 1382 | svc.configSvc, | ||
| 1383 | svc.blobStorage, | 1383 | svc.blobStorage, | ||
| 1384 | svc.previewService, | 1384 | svc.previewService, | ||
| 1385 | - | cfg.Server.RegistryDomain, | |||
| 1386 | ) | 1385 | ) | ||
| 1387 | 1386 | ||||
| 1388 | // Dispatcher routes image push events to either auto-route or preview handler | 1387 | // Dispatcher routes image push events to either auto-route or preview handler | ||
M internal/usecase/auto/preview/handler.go +26 -11
| @@ -3,6 +3,8 @@ package preview | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | ||
| 6 | + | "github.com/bnema/zerowrap" | |
| 7 | + | ||
| 6 | 8 | "github.com/bnema/gordon/internal/boundaries/out" | |
| 7 | 9 | "github.com/bnema/gordon/internal/domain" | |
| 8 | 10 | "github.com/bnema/gordon/internal/usecase/auto" | |
| @@ -14,7 +16,6 @@ type AutoPreviewHandler struct { | |||
| 14 | 16 | config auto.AutoConfigProvider | |
| 15 | 17 | blobStorage out.BlobStorage | |
| 16 | 18 | previewService *Service | |
| 17 | - | registryDomain string | |
| 18 | 19 | } | |
| 19 | 20 | ||
| 20 | 21 | func NewAutoPreviewHandler( | |
| @@ -22,14 +23,12 @@ func NewAutoPreviewHandler( | |||
| 22 | 23 | config auto.AutoConfigProvider, | |
| 23 | 24 | blobStorage out.BlobStorage, | |
| 24 | 25 | previewService *Service, | |
| 25 | - | registryDomain string, | |
| 26 | 26 | ) *AutoPreviewHandler { | |
| 27 | 27 | return &AutoPreviewHandler{ | |
| 28 | 28 | serviceCtx: serviceCtx, | |
| 29 | 29 | config: config, | |
| 30 | 30 | blobStorage: blobStorage, | |
| 31 | 31 | previewService: previewService, | |
| 32 | - | registryDomain: registryDomain, | |
| 33 | 32 | } | |
| 34 | 33 | } | |
| 35 | 34 | ||
| @@ -38,6 +37,14 @@ func (h *AutoPreviewHandler) CanHandle(t domain.EventType) bool { | |||
| 38 | 37 | } | |
| 39 | 38 | ||
| 40 | 39 | func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) error { | |
| 40 | + | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | |
| 41 | + | zerowrap.FieldLayer: "usecase", | |
| 42 | + | zerowrap.FieldHandler: "AutoPreviewHandler", | |
| 43 | + | zerowrap.FieldEvent: string(event.Type), | |
| 44 | + | "event_id": event.ID, | |
| 45 | + | }) | |
| 46 | + | log := zerowrap.FromCtx(ctx) | |
| 47 | + | ||
| 41 | 48 | payload, ok := event.Data.(domain.ImagePushedPayload) | |
| 42 | 49 | if !ok { | |
| 43 | 50 | return nil | |
| @@ -46,6 +53,7 @@ func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) err | |||
| 46 | 53 | // Extract labels from manifest | |
| 47 | 54 | labels, err := auto.ExtractLabels(ctx, payload.Manifest, h.blobStorage) | |
| 48 | 55 | if err != nil { | |
| 56 | + | log.Debug().Err(err).Msg("failed to extract labels, skipping preview handler") | |
| 49 | 57 | return nil | |
| 50 | 58 | } | |
| 51 | 59 | ||
| @@ -55,6 +63,7 @@ func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) err | |||
| 55 | 63 | // Resolve base route from labels | |
| 56 | 64 | domains := collectDomains(labels) | |
| 57 | 65 | if len(domains) == 0 { | |
| 66 | + | log.Debug().Str("image", payload.Name).Msg("no gordon.domain label found, skipping preview handler") | |
| 58 | 67 | return nil | |
| 59 | 68 | } | |
| 60 | 69 | baseRouteDomain := domains[0] | |
| @@ -63,23 +72,29 @@ func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) err | |||
| 63 | 72 | allowedDomains := h.config.GetAllowedDomains() | |
| 64 | 73 | previewDomain, err := GeneratePreviewDomain(baseRouteDomain, previewName, previewConfig.Separator) | |
| 65 | 74 | if err != nil { | |
| 75 | + | log.Debug().Err(err).Str("base_domain", baseRouteDomain).Msg("failed to generate preview domain") | |
| 66 | 76 | return nil | |
| 67 | 77 | } | |
| 68 | 78 | if !auto.MatchesDomainAllowlist(previewDomain, allowedDomains) { | |
| 79 | + | log.Debug().Str("domain", previewDomain).Strs("patterns", allowedDomains).Msg("preview domain not in allowlist, skipping") | |
| 69 | 80 | return nil | |
| 70 | 81 | } | |
| 71 | 82 | ||
| 72 | 83 | imageName := payload.Name + ":" + payload.Reference | |
| 73 | 84 | ||
| 74 | 85 | // Launch async — volume cloning may exceed event bus timeout | |
| 75 | - | go h.previewService.CreatePreview(h.serviceCtx, CreatePreviewRequest{ | |
| 76 | - | Name: previewName, | |
| 77 | - | Domain: previewDomain, | |
| 78 | - | BaseRoute: baseRouteDomain, | |
| 79 | - | Image: imageName, | |
| 80 | - | HTTPS: true, | |
| 81 | - | PreviewConfig: previewConfig, | |
| 82 | - | }) | |
| 86 | + | go func() { | |
| 87 | + | if err := h.previewService.CreatePreview(h.serviceCtx, CreatePreviewRequest{ | |
| 88 | + | Name: previewName, | |
| 89 | + | Domain: previewDomain, | |
| 90 | + | BaseRoute: baseRouteDomain, | |
| 91 | + | Image: imageName, | |
| 92 | + | HTTPS: true, | |
| 93 | + | PreviewConfig: previewConfig, | |
| 94 | + | }); err != nil { | |
| 95 | + | log.Error().Err(err).Str("preview", previewName).Msg("failed to create preview") | |
| 96 | + | } | |
| 97 | + | }() | |
| 83 | 98 | ||
| 84 | 99 | return nil | |
| 85 | 100 | } | |
| @@ -3,6 +3,8 @@ package preview | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | 5 | ||||
| 6 | + | "github.com/bnema/zerowrap" | |||
| 7 | + | ||||
| 6 | "github.com/bnema/gordon/internal/boundaries/out" | 8 | "github.com/bnema/gordon/internal/boundaries/out" | ||
| 7 | "github.com/bnema/gordon/internal/domain" | 9 | "github.com/bnema/gordon/internal/domain" | ||
| 8 | "github.com/bnema/gordon/internal/usecase/auto" | 10 | "github.com/bnema/gordon/internal/usecase/auto" | ||
| @@ -14,7 +16,6 @@ type AutoPreviewHandler struct { | |||||
| 14 | config auto.AutoConfigProvider | 16 | config auto.AutoConfigProvider | ||
| 15 | blobStorage out.BlobStorage | 17 | blobStorage out.BlobStorage | ||
| 16 | previewService *Service | 18 | previewService *Service | ||
| 17 | - | registryDomain string | |||
| 18 | } | 19 | } | ||
| 19 | 20 | ||||
| 20 | func NewAutoPreviewHandler( | 21 | func NewAutoPreviewHandler( | ||
| @@ -22,14 +23,12 @@ func NewAutoPreviewHandler( | |||||
| 22 | config auto.AutoConfigProvider, | 23 | config auto.AutoConfigProvider, | ||
| 23 | blobStorage out.BlobStorage, | 24 | blobStorage out.BlobStorage, | ||
| 24 | previewService *Service, | 25 | previewService *Service, | ||
| 25 | - | registryDomain string, | |||
| 26 | ) *AutoPreviewHandler { | 26 | ) *AutoPreviewHandler { | ||
| 27 | return &AutoPreviewHandler{ | 27 | return &AutoPreviewHandler{ | ||
| 28 | serviceCtx: serviceCtx, | 28 | serviceCtx: serviceCtx, | ||
| 29 | config: config, | 29 | config: config, | ||
| 30 | blobStorage: blobStorage, | 30 | blobStorage: blobStorage, | ||
| 31 | previewService: previewService, | 31 | previewService: previewService, | ||
| 32 | - | registryDomain: registryDomain, | |||
| 33 | } | 32 | } | ||
| 34 | } | 33 | } | ||
| 35 | 34 | ||||
| @@ -38,6 +37,14 @@ func (h *AutoPreviewHandler) CanHandle(t domain.EventType) bool { | |||||
| 38 | } | 37 | } | ||
| 39 | 38 | ||||
| 40 | func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) error { | 39 | func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) error { | ||
| 40 | + | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | |||
| 41 | + | zerowrap.FieldLayer: "usecase", | |||
| 42 | + | zerowrap.FieldHandler: "AutoPreviewHandler", | |||
| 43 | + | zerowrap.FieldEvent: string(event.Type), | |||
| 44 | + | "event_id": event.ID, | |||
| 45 | + | }) | |||
| 46 | + | log := zerowrap.FromCtx(ctx) | |||
| 47 | + | ||||
| 41 | payload, ok := event.Data.(domain.ImagePushedPayload) | 48 | payload, ok := event.Data.(domain.ImagePushedPayload) | ||
| 42 | if !ok { | 49 | if !ok { | ||
| 43 | return nil | 50 | return nil | ||
| @@ -46,6 +53,7 @@ func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) err | |||||
| 46 | // Extract labels from manifest | 53 | // Extract labels from manifest | ||
| 47 | labels, err := auto.ExtractLabels(ctx, payload.Manifest, h.blobStorage) | 54 | labels, err := auto.ExtractLabels(ctx, payload.Manifest, h.blobStorage) | ||
| 48 | if err != nil { | 55 | if err != nil { | ||
| 56 | + | log.Debug().Err(err).Msg("failed to extract labels, skipping preview handler") | |||
| 49 | return nil | 57 | return nil | ||
| 50 | } | 58 | } | ||
| 51 | 59 | ||||
| @@ -55,6 +63,7 @@ func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) err | |||||
| 55 | // Resolve base route from labels | 63 | // Resolve base route from labels | ||
| 56 | domains := collectDomains(labels) | 64 | domains := collectDomains(labels) | ||
| 57 | if len(domains) == 0 { | 65 | if len(domains) == 0 { | ||
| 66 | + | log.Debug().Str("image", payload.Name).Msg("no gordon.domain label found, skipping preview handler") | |||
| 58 | return nil | 67 | return nil | ||
| 59 | } | 68 | } | ||
| 60 | baseRouteDomain := domains[0] | 69 | baseRouteDomain := domains[0] | ||
| @@ -63,23 +72,29 @@ func (h *AutoPreviewHandler) Handle(ctx context.Context, event domain.Event) err | |||||
| 63 | allowedDomains := h.config.GetAllowedDomains() | 72 | allowedDomains := h.config.GetAllowedDomains() | ||
| 64 | previewDomain, err := GeneratePreviewDomain(baseRouteDomain, previewName, previewConfig.Separator) | 73 | previewDomain, err := GeneratePreviewDomain(baseRouteDomain, previewName, previewConfig.Separator) | ||
| 65 | if err != nil { | 74 | if err != nil { | ||
| 75 | + | log.Debug().Err(err).Str("base_domain", baseRouteDomain).Msg("failed to generate preview domain") | |||
| 66 | return nil | 76 | return nil | ||
| 67 | } | 77 | } | ||
| 68 | if !auto.MatchesDomainAllowlist(previewDomain, allowedDomains) { | 78 | if !auto.MatchesDomainAllowlist(previewDomain, allowedDomains) { | ||
| 79 | + | log.Debug().Str("domain", previewDomain).Strs("patterns", allowedDomains).Msg("preview domain not in allowlist, skipping") | |||
| 69 | return nil | 80 | return nil | ||
| 70 | } | 81 | } | ||
| 71 | 82 | ||||
| 72 | imageName := payload.Name + ":" + payload.Reference | 83 | imageName := payload.Name + ":" + payload.Reference | ||
| 73 | 84 | ||||
| 74 | // Launch async — volume cloning may exceed event bus timeout | 85 | // Launch async — volume cloning may exceed event bus timeout | ||
| 75 | - | go h.previewService.CreatePreview(h.serviceCtx, CreatePreviewRequest{ | 86 | + | go func() { |
| 76 | - | Name: previewName, | 87 | + | if err := h.previewService.CreatePreview(h.serviceCtx, CreatePreviewRequest{ |
| 77 | - | Domain: previewDomain, | 88 | + | Name: previewName, |
| 78 | - | BaseRoute: baseRouteDomain, | 89 | + | Domain: previewDomain, |
| 79 | - | Image: imageName, | 90 | + | BaseRoute: baseRouteDomain, |
| 80 | - | HTTPS: true, | 91 | + | Image: imageName, |
| 81 | - | PreviewConfig: previewConfig, | 92 | + | HTTPS: true, |
| 82 | - | }) | 93 | + | PreviewConfig: previewConfig, |
| 94 | + | }); err != nil { | |||
| 95 | + | log.Error().Err(err).Str("preview", previewName).Msg("failed to create preview") | |||
| 96 | + | } | |||
| 97 | + | }() | |||
| 83 | 98 | ||||
| 84 | return nil | 99 | return nil | ||
| 85 | } | 100 | } | ||