Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors Dalec’s frontend routing from a hierarchical, nested BuildMux to a single flat frontend.Router with fully-qualified routes. It also changes the plugin model to provide routes at request time (enabling spec-aware route availability), and extends the targets listing payload with dalec-specific metadata (specDefined, hidden) to help clients.
Changes:
- Replace
frontend.BuildMuxwith a flatfrontend.Routerand update dispatch/listing/subrequest handling accordingly. - Switch target plugins from “build-target” handlers to “route-provider” plugins returning
[]frontend.Route. - Add
internal/gwutilwrappers to preserveCurrentFrontend/SpecLoaderacross gwclient wrappers; update docs and tooling to the new targets and routing flow.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/examples/targets.md | Updates the documented target list to include new RPM debug subroutes. |
| test/testenv/buildx.go | Wraps gateway client wrappers with gwutil.WithCurrentFrontend to preserve interfaces. |
| test/fixtures/phony/main.go | Migrates test fixture frontend from BuildMux to Router. |
| targets/windows/handler.go | Converts Windows target handling into a route provider (Routes) returning flat routes. |
| targets/register.go | Replaces build-target registration with route-provider registration. |
| targets/plugin/init.go | Registers distro route providers instead of build-target handlers. |
| targets/linux/rpm/rockylinux/common.go | Removes old Handlers/builtin mux target map usage. |
| targets/linux/rpm/distro/distro.go | Converts RPM distro config from mux-based handling to flat Routes, includes debug routes. |
| targets/linux/rpm/distro/debug.go | Replaces nested debug mux with DebugRoutes returning flat debug routes. |
| targets/linux/rpm/azlinux/common.go | Removes old Handlers/builtin mux target map usage. |
| targets/linux/rpm/almalinux/common.go | Removes old Handlers/builtin mux target map usage. |
| targets/linux/distro_handler.go | Refactors base image config resolution helper (resolveBaseConfig). |
| targets/linux/deb/ubuntu/common.go | Removes old Handlers/builtin mux target map usage. |
| targets/linux/deb/distro/distro.go | Converts DEB distro config from mux-based handling to flat Routes. |
| targets/linux/deb/debian/common.go | Removes old Handlers/builtin mux target map usage. |
| internal/plugins/types.go | Changes plugin type contract to TypeRouteProvider with RouteProvider interface. |
| internal/gwutil/gwutil.go | Introduces helper to preserve CurrentFrontend and SpecLoader across client wrapping. |
| internal/frontendapi/router.go | Replaces build-router creation with NewRouter that loads route providers and caches spec loads. |
| frontend/router_test.go | Adds tests for router dispatch (exact match, prefix match, default route, not-found). |
| frontend/router.go | Adds the new flat router, extended target list response, and spec-loading helper. |
| frontend/mux_test.go | Removes old mux tests (replaced by router tests). |
| frontend/mux.go | Removes the old hierarchical BuildMux implementation. |
| frontend/debug/plugin/init.go | Removes debug plugin registration via the old build-target plugin type. |
| frontend/debug/handler.go | Converts debug routing to flat debug.Routes(prefix) route definitions. |
| frontend/client.go | Extracts shared client/opts helpers and subrequest handlers from the removed mux. |
| frontend/build.go | Updates platform wrapper to preserve CurrentFrontend via gwutil. |
| cmd/worker-img-matrix/main.go | Updates worker matrix generation to load route providers and discover /worker routes. |
| cmd/gen-doc-targets/main.go | Updates doc target generation to use the new router and provide a stub client for empty-spec loading. |
| cmd/frontend/main.go | Creates the router per request and dispatches through Router.Handler(WithTargetForwardingHandler). |
2da7548 to
5c5da9a
Compare
5c5da9a to
aeebc40
Compare
aeebc40 to
6aec426
Compare
Replace the hierarchical BuildMux (nested sub-muxes) with a single flat Router that uses fully-qualified route paths like "azlinux3/container" and "debug/resolve". All routes are registered eagerly at startup via RouteProvider plugins instead of being lazily discovered through handler invocation. Key changes: - New Router type in frontend/router.go with exact and longest-prefix match dispatch - New RouteProvider plugin interface replaces BuildHandler/BuildTarget - Distro packages export Routes(prefix) instead of Handle() methods - Target list API returns SpecDefined annotation without invoking handlers - Extract shared client helpers from mux.go into frontend/client.go - Remove all dead BuildMux code, unused Handlers() funcs, and the debug/plugin/init.go registration shim Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Change RouteProvider.Routes() to accept (prefix, ctx, client) so that providers can load the spec at registration time. This enables: - SpecDefined annotation on each route (whether the target key appears in spec.Targets), used by the target list API - Per-request NewRouter(ctx, client) in the BuildFunc, since route registration now needs the gateway client - LoadSpecFromClient() helper in frontend/router.go for providers - Each distro registers as a separate RouteProvider via RegisterRouteProvider() - Bare distro prefix routes (e.g. "mariner2") as hidden aliases for the default container handler - Hidden field on frontend.Target to exclude alias routes from the target list while keeping them dispatchable - DebugRoutes now registers individual sub-routes (buildroot, sources, spec) instead of a single prefix handler - gen-doc-targets uses a stub gwclient.Client to set up routes for documentation generation Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Add internal/gwutil package with WithCurrentFrontend helper that preserves CurrentFrontend and SpecLoader interfaces when wrapping a gwclient.Client. This replaces ad-hoc manual CurrentFrontend() forwarding methods that were fragile and easy to forget when adding new wrappers. Update all wrapping sites (setClientOptOption, maybeSetDalecTargetKey, WithDefaultPlatform, and test wrappers) to use gwutil.WithCurrentFrontend. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Add a cachedSpecClient wrapper that implements gwutil.SpecLoader and uses sync.Once to cache the spec loaded by LoadSpecFromClient. This eliminates redundant spec loads during route registration where each distro's Routes() independently calls LoadSpecFromClient (14+ times per build request). LoadSpecFromClient now checks for the SpecLoader interface before falling back to the full dockerui load path. NewRouter wraps the client internally so the cache is scoped to registration only. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
6aec426 to
6417397
Compare
| r, err := frontendapi.NewRouter(ctx, client) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return r.Handler(frontend.WithTargetForwardingHandler)(ctx, client) |
There was a problem hiding this comment.
NewRouter wraps the client with a cached-spec loader internally for route registration, but the subsequent r.Handler(...)(ctx, client) call uses the original client. This means WithTargetForwardingHandler (and any other option that loads the spec) will likely reload the spec, losing the caching benefit. Consider passing the same cached wrapper through the whole request lifecycle (e.g., have NewRouter return the wrapped client, or expose a helper to wrap the client before both NewRouter and r.Handler are invoked).
| r, err := frontendapi.NewRouter(ctx, client) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return r.Handler(frontend.WithTargetForwardingHandler)(ctx, client) | |
| // Wrap the gateway client with a spec-caching layer so that both route | |
| // registration and request handling reuse the same cached spec loader. | |
| cachedClient := frontendapi.NewCachedGatewayClient(client) | |
| r, err := frontendapi.NewRouter(ctx, cachedClient) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return r.Handler(frontend.WithTargetForwardingHandler)(ctx, cachedClient) |
| r.routes = make(map[string]Route) | ||
| } | ||
| r.routes[route.FullPath] = route | ||
| bklog.G(ctx).WithField("route", route.FullPath).Info("Added route to router") |
There was a problem hiding this comment.
Router.Add logs every route registration at Info level. Since the router is built per-request in cmd/frontend (NewRouter is called inside the build handler), this will emit dozens of Info logs for every build and can significantly increase log volume. Consider lowering this to Debug or gating it behind a verbose flag to avoid noisy production logs.
| bklog.G(ctx).WithField("route", route.FullPath).Info("Added route to router") | |
| bklog.G(ctx).WithField("route", route.FullPath).Debug("Added route to router") |
| targets := make([]Target, 0, len(remote.Targets)) | ||
| for _, t := range remote.Targets { | ||
| t.Name = path.Join(prefix, t.Name) | ||
| targets = append(targets, Target{Target: t}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forwarded target entries returned from queryForwardedTargets drop Dalec-specific metadata (e.g., SpecDefined/Hidden) because they are converted from bktargets.Target into frontend.Target without setting those fields. Since SpecDefined is intended to help clients know whether a target comes from the spec, consider setting SpecDefined=true for all returned targets under a forwarded prefix (and for the forwarding route itself) so the JSON targets API is consistent.
| targets = append(targets, Target{Target: t}) | |
| targets = append(targets, Target{ | |
| Target: t, | |
| SpecDefined: true, | |
| }) |
There was a problem hiding this comment.
Or pass if spec is defined from the list()?
| Target: bktargets.Target{ | ||
| Name: frontendKey, | ||
| Description: fmt.Sprintf("Forwarded to custom frontend: %s", frontendTarget.Frontend.Image), | ||
| }, |
There was a problem hiding this comment.
The forwarding route metadata (Info) does not set SpecDefined. If the forwarded frontend came from spec.Targets (which is the only way this route is added), setting SpecDefined=true here would make the target list output more accurate, especially in the fallback path when querying the forwarded frontend fails.
| Target: bktargets.Target{ | |
| Name: frontendKey, | |
| Description: fmt.Sprintf("Forwarded to custom frontend: %s", frontendTarget.Frontend.Image), | |
| }, | |
| Target: bktargets.Target{ | |
| Name: frontendKey, | |
| Description: fmt.Sprintf("Forwarded to custom frontend: %s", frontendTarget.Frontend.Image), | |
| }, | |
| SpecDefined: true, |
There was a problem hiding this comment.
Comments for frontend: replace BuildMux with flat Router.
| t.Errorf("expected real handler call count to be %d, got %d", expectedRealCount, count) | ||
| } | ||
|
|
||
| // Register a sub-route at a fully qualified path. |
There was a problem hiding this comment.
Could those tests benefit from sub-tests to reduce the number of assertions and shared state between the test cases to drive down the complexity?
| func NewBuildRouter(ctx context.Context) (*frontend.BuildMux, error) { | ||
| var mux frontend.BuildMux | ||
| mux.Add(debug.DebugRoute, debug.Handle, nil) | ||
| // NewRouter creates a flat Router with all routes registered eagerly. |
There was a problem hiding this comment.
eagerly? What does it mean in this context?
| @@ -11,23 +12,49 @@ import ( | |||
| ) | |||
|
|
|||
| func init() { | |||
There was a problem hiding this comment.
While we're at it, if we use plugins only for building routes and we already have some statically defined routes, I wonder if we could load those explicitly in internal/frontendapi/router.go or somewhere along the way in the composition tree, while I guess still keeping logic for loading from plugins from some external sources (although perhaps exposing Dalec as a library would still be better so wrappers can do their own transparent composition?)
That might be out of scope of course.
| @@ -1,683 +0,0 @@ | |||
| package frontend | |||
There was a problem hiding this comment.
nit: split of mux.go into client.go and router.go could be done in a separate commit so changes in those files are better represented, otherwise it's hard to follow what has changed since changes are done in separate files now.
frontend/router.go
Outdated
| tlk := topLevelKey(key) | ||
| if _, ok := spec.Targets[tlk]; ok { |
There was a problem hiding this comment.
nit: maybe spec.Targets[topLevelKey(key)] to avoid temporary variable naming?
Also conditional can be avoided:
_, ok := spec.Targets[topLevelKey(key)]
dt.SpecDefined = ok| } | ||
| } | ||
|
|
||
| // Forwarding route: query the remote frontend for its sub-targets. |
There was a problem hiding this comment.
nit: maybe:
targets := []Target{dt}
if route.Forward != nil {
subTargets, err := queryForwardedTargets(ctx, client, key, route.Forward)
if err != nil {
bklog.G(ctx).WithError(err).Warn("Could not query forwarded frontend targets")
} else {
targets = subTargets
}
}
ls.Targets = append(ls.Targets, targets...)That yields simpler flow I think, we only write once and we don't short-circut with continue.
| targets := make([]Target, 0, len(remote.Targets)) | ||
| for _, t := range remote.Targets { | ||
| t.Name = path.Join(prefix, t.Name) | ||
| targets = append(targets, Target{Target: t}) |
There was a problem hiding this comment.
Or pass if spec is defined from the list()?
There was a problem hiding this comment.
For frontend: make route registration spec-aware
| return nil, err | ||
| } | ||
|
|
||
| _, specDefined := spec.Targets[prefix] |
There was a problem hiding this comment.
Couldn't we set the spec defined by mutating the routes received from different targets to avoid having each target load the spec explicitly?
There was a problem hiding this comment.
Oh, it was done centrally first, right. Could you elaborate why is it better to do it per routes source, considering that requires a client, which is a god dependency pretty much?
There was a problem hiding this comment.
I did this for simplicity while moving everything around and figuring out what the shape is.
I figured we can come back to it easily enough, and did slightly with the cached client wrapper in a later commit.
| 7082 for _, route := range provider.Routes() { | ||
| routes, err := provider.Routes(ctx, client) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
nit: would be nice to annotate errors with fmt.Errorf for new code, that helps tracing down errors a lot when they occur.
|
|
||
| // Preserve SpecLoader if present on either wrapper or inner. | ||
| // Prefer wrapper (it's the more specific layer). | ||
| switch { |
There was a problem hiding this comment.
nit: I think you should be able to do something like:
switch v := i.(type) {
case int:
fmt.Printf("Twice %v is %v\n", v, v*2)
case string:
fmt.Printf("%q is %v bytes long\n", v, len(v))
default:
fmt.Printf("I don't know about type %T!\n", v)
}There was a problem hiding this comment.
These is switching on two different variables. Probably shouldn't even bother with the switch here, I think.
There was a problem hiding this comment.
Its weird I thought I did this totally differently, but maybe messed up while re-arranging commits.
The solution I'd come up with is to just have clientWIthCurrentFrontend always implement SpecLoader since the LoadSpecFromClient function will handle the check anyway.
There was a problem hiding this comment.
Oh right... import cycle issues.
This is in preparation of #986
It also just generally fixes issues with routing for routes with sub-routers (e.g. see that targets.md now populates certain debug routes).
It moves to using a single, flat router for all routes.
Route registration happens at request time since available routes can be spec-dependent.
For the targets json API (docker build --call frontend.targets,format=json) it now includes a field to give extra hints to clients, such as the vscode extension, on if the targets are explicit in the spec or not.