Support mux patterns in VW framework by ntnn · Pull Request #4543 · kcp-dev/kcp

ntnn · 2026-11-03T18:21:25Z

Summary

I saw the virtualWorkspaceClusterOffsets and became sad.
So I did something to not be sad.
Also adding some e2e tests for this since we only have unit tests to cover the case of a clusters APIExport.

What Type of PR Is This?

/kind cleanup

Related Issue(s)

Fixes #

Release Notes

BREAKING: The virtual-workspace-framework now requires at least one mux pattern to be set instead of parsing request URLs by hand

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

…PIExport Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

ntnn · 2026-11-03T18:27:16Z

@mjudeikis-bot review this, be nitpicky around the vw-framework API for external users

mjudeikis-bot

Mostly looks good as a cleanup, but there is one regression in the front-proxy and two framework-API concerns.

Not anchored

Breaking change for external VW implementers: RootPathResolverFunc/RootPathResolver.ResolveRootPath changes signature (arguments swapped and PathMatch replaces urlPath). This is intentional per the PR, but it has no release note (release-note-none), and the staging repo is consumed externally. Please add a release note describing the migration (Patterns + new resolver signature). Conflicting or duplicate patterns across VWs now panic at startup; worth stating in architecture.md.
Behaviour shift worth documenting: previously the first VW (in registration order) that accepted any path won; now the most specific mux pattern wins first and registration order only applies within an identical pattern string.

Checked

Read beyond the diff: pkg/server/proxy/lookup.go (WithClusterResolver, newMappingHandler, extractClusterFromPath), pkg/virtual/options/options.go (how VWs are composed), the full server.go/config.go/types.go in the framework, and all seven builders' resolvers.
Correctness: resolvers traced for admin, apiexport, replication, apiresourceschema (consumer/cluster mismatch check kept), initializing/terminating (three VWs sharing one pattern keep order), migrating. No other issue found. Findings above: front-proxy regression (verified with a throwaway test on this head), mux path-cleaning redirect (verified with a throwaway test), empty Patterns (traced).
Concurrency: mux built once at handler-chain build; n/a for races.
kcp specifics: the cluster is now resolved before the resolver runs; invalid cluster segments fall to the delegate, as before. Wildcard * handled.
API/storage: no API types/codegen touched. Security: no authorizer changes; the apiresourceschema cluster mismatch check preserved.
Tests: new unit tests in server_test.go and the builders cover the mux; the removed proxy tests are what would fail without the revert.
Commands: go build ./... OK; go vet ./pkg/virtual/... ./pkg/server/proxy/... clean; go test ./pkg/virtual/... ./pkg/server/proxy/... pass; go test ./... in the framework staging module passes. I did not run the e2e tests (they need a full kcp environment, and the proxy regression is already confirmed at unit level).

kcp-ci-bot · 2026-11-03T18:30:59Z

@mjudeikis-bot: changing LGTM is restricted to collaborators

Details

In response to this:

Mostly looks good as a cleanup, but there is one regression in the front-proxy and two framework-API concerns.

Not anchored

Breaking change for external VW implementers: RootPathResolverFunc/RootPathResolver.ResolveRootPath changes signature (arguments swapped and PathMatch replaces urlPath). This is intentional per the PR, but it has no release note (release-note-none), and the staging repo is consumed externally. Please add a release note describing the migration (Patterns + new resolver signature). Conflicting or duplicate patterns across VWs now panic at startup; worth stating in architecture.md.

Behaviour shift worth documenting: previously the first VW (in registration order) that accepted any path won; now the most specific mux pattern wins first and registration order only applies within an identical pattern string.

Checked

Read beyond the diff: pkg/server/proxy/lookup.go (WithClusterResolver, newMappingHandler, extractClusterFromPath), pkg/virtual/options/options.go (how VWs are composed), the full server.go/config.go/types.go in the framework, and all seven builders' resolvers.

Correctness: resolvers traced for admin, apiexport, replication, apiresourceschema (consumer/cluster mismatch check kept), initializing/terminating (three VWs sharing one pattern keep order), migrating. No other issue found. Findings above: front-proxy regression (verified with a throwaway test on this head), mux path-cleaning redirect (verified with a throwaway test), empty Patterns (traced).

Concurrency: mux built once at handler-chain build; n/a for races.

kcp specifics: the cluster is now resolved before the resolver runs; invalid cluster segments fall to the delegate, as before. Wildcard * handled.

API/storage: no API types/codegen touched. Security: no authorizer changes; the apiresourceschema cluster mismatch check preserved.

Tests: new unit tests in server_test.go and the builders cover the mux; the removed proxy tests are what would fail without the revert.

Commands: go build ./... OK; go vet ./pkg/virtual/... ./pkg/server/proxy/... clean; go test ./pkg/virtual/... ./pkg/server/proxy/... pass; go test ./... in the framework staging module passes. I did not run the e2e tests (they need a full kcp environment, and the proxy regression is already confirmed at unit level).

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

ntnn · 2026-11-03T21:59:16Z

/test pull-kcp-test-e2e

ntnn · 2026-11-04T06:20:20Z

-// When it returns `true`, it will also set the VirtualWorkspace name in the request context,
-// as well as the prefix that should be removed from the request URL Path before forwarding
-// the request to the given VirtualWorkspace delegated APIServer.
-type RootPathResolverFunc func(urlPath string, context context.Context) (accepted bool, prefixToStrip string, completedContext context.Context)


Could also leave this as is without a breaking change. Would be more wiring probably.
Question is how many people are using the vw-framework to implement VWs and if they shouldn't be migrating to a mux pattern instead of parsing themselves anyway.

So it's not removed; just signature changes:

func (r RootPathResolverFunc) ResolveRootPath(ctx context.Context, match PathMatch) (accepted bool, prefixToStrip string, completedContext context.Context) { return r(ctx, match) }

so we should be ok

mjudeikis

mostly missing docs

mjudeikis · 2026-11-04T07:01:51Z

+const (
+	sliceClusterWildcard = "sliceCluster"
+	sliceNameWildcard    = "sliceName"
+)
+
+// pathPattern returns the mux pattern of the virtual workspace below rootPathPrefix.
+func pathPattern(rootPathPrefix string) string {
+	return path.Join(rootPathPrefix, "{sliceCluster}/{sliceName}/clusters/{cluster}")
+}
+


Can we get bit of godocs on ththese? what these sliceClusterX means?

Sure, I just followed what the virtualWorkspaceClusterOffsets map did:

// /services/replication/<slice cluster>/<slice name>/clusters/<cluster>/...

mjudeikis · 2026-11-04T07:03:41Z

-			require.Equal(t, tc.expectedLogicalPath, logicalPath, "LogicalPath should match expected value")
-		})
+	require.Equal(t,
+		"/services/replication/{sliceCluster}/{sliceName}/clusters/{cluster}",


same as above. This reads strange sliceCluster and later clusters...

mjudeikis · 2026-11-04T07:11:55Z

+// PathMatch is a request path matched against a virtual workspace pattern.
+type PathMatch struct {
+	// Path is the full request path.
+	Path string
+	// Trail is the kube-like API path after the pattern, "" if none.
+	Trail string
+
+	value func(name string) string
+}


As this is breaking change, lets add bit more extensive godocs what is path, how it could looks like (example), and trails itself. Smth user can read and understand how to use this new pathmatcher

mjudeikis · 2026-11-04T07:13:18Z

-// When it returns `true`, it will also set the VirtualWorkspace name in the request context,
-// as well as the prefix that should be removed from the request URL Path before forwarding
-// the request to the given VirtualWorkspace delegated APIServer.
-type RootPathResolverFunc func(urlPath string, context context.Context) (accepted bool, prefixToStrip string, completedContext context.Context)


So it's not removed; just signature changes:

func (r RootPathResolverFunc) ResolveRootPath(ctx context.Context, match PathMatch) (accepted bool, prefixToStrip string, completedContext context.Context) { return r(ctx, match) }

so we should be ok

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

mjudeikis

/lgtm
/approve

kcp-ci-bot · 2026-11-04T10:59:19Z

LGTM label has been added.

Details

Git tree hash: 87e39fb54ae0b26b41b67f8c98f7087651f6cc06

kcp-ci-bot · 2026-11-04T10:59:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjudeikis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mjudeikis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ntnn added 11 commits November 3, 2026 19:21

Support mux patterns in VW framework

5c6a6ab

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Update migratingworkspaces VW to use mux patterns

dc80609

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Update apiexport VW to use mux patterns

25ae9a6

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Update apiresourceschema VW to use mux patterns

98a0c7e

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Update initializingworkspaces to use mux patterns

4012231

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Update replication VW to use mux patterns

f3f9fe9

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Update terminatingworkspaces to use mux patterns

5bcc4d9

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Update admin VW to use mux patterns

9725406

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Update VW framework docs

2ba3d86

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Drop virtualWorkspaceClusterOffsets, handled in VW framework

ecb5805

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Preserve trailing separator

37e217e

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

ntnn force-pushed the vw-path-pattern branch from 1db9eaf to 37e217e Compare November 3, 2026 18:22

kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2026

Add e2e tests for apiexport and replication VW serving a clusters A…

08698c7

…PIExport Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

mjudeikis-bot suggested changes Nov 3, 2026

View reviewed changes

ntnn added 3 commits November 3, 2026 22:05

Bypass pattern mux for non-canonical paths

b33dcd3

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Reject VWs without path patterns

6582c72

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Make PathMatch safe to construct outside the framework

b885cb0

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 4, 2026

ntnn commented Nov 4, 2026

View reviewed changes

mjudeikis requested changes Nov 4, 2026

View reviewed changes

kcp-ci-bot assigned mjudeikis Nov 4, 2026

ntnn added 2 commits November 4, 2026 08:53

Add docs explaining the slice* variables

911ef47

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

Add docs for PathMatch

32a1f00

Signed-off-by: Nelo-T. Wallus <git@ntnn.de> Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>

mjudeikis approved these changes Nov 4, 2026

View reviewed changes

kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2026

kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2026

kcp-ci-bot merged commit c0d6984 into kcp-dev:main Nov 4, 2026
19 checks passed

Conversation

ntnn commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Type of PR Is This?

Related Issue(s)

Release Notes

Uh oh!

ntnn commented Nov 3, 2026

Uh oh!

mjudeikis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kcp-ci-bot commented Nov 3, 2026

Uh oh!

ntnn commented Nov 3, 2026

Uh oh!

ntnn Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

mjudeikis Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

mjudeikis left a comment

Choose a reason for hiding this comment

Uh oh!

mjudeikis Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

ntnn Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjudeikis Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

mjudeikis Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

mjudeikis Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

mjudeikis left a comment

Choose a reason for hiding this comment

Uh oh!

kcp-ci-bot commented Nov 4, 2026

Uh oh!

kcp-ci-bot commented Nov 4, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ntnn commented Nov 3, 2026 •

edited

Loading

ntnn Nov 4, 2026 •

edited

Loading