From 4b2b8e8e42206330a9c62ed2fcee433d180c6a7a Mon Sep 17 00:00:00 2001 From: Kevin Stellmacher Date: Thu, 7 May 2026 14:17:44 +0200 Subject: [PATCH 1/3] feat: enable workspace resolver to find nested workspaces Signed-off-by: Kevin Stellmacher --- .../controller/dependencyrule_controller.go | 52 ++++++++++++------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/internal/controller/dependencyrule_controller.go b/internal/controller/dependencyrule_controller.go index 8e948ed..f1f95b4 100644 --- a/internal/controller/dependencyrule_controller.go +++ b/internal/controller/dependencyrule_controller.go @@ -62,10 +62,7 @@ func (r *DependencyRuleReconciler) ensureInitialized() error { localMgr := r.DepCtrlManager.GetLocalManager() - // Create workspace resolver using the base kcp config (front-proxy URL). - rootCfg := rest.CopyConfig(r.BaseConfig) - rootCfg.Host += logicalcluster.NewPath("root").RequestPath() - r.wsResolver = &workspaceResolver{rootCfg: rootCfg, scheme: localMgr.GetScheme()} + r.wsResolver = &workspaceResolver{baseCfg: r.BaseConfig, scheme: localMgr.GetScheme()} return nil } @@ -165,7 +162,7 @@ func (r *DependencyRuleReconciler) handleDeletion(ctx context.Context, key, rule // workspaceResolver maps kcp workspace paths (e.g., "root:compute-provider") // to logical cluster names by reading Workspace objects from the root workspace. type workspaceResolver struct { - rootCfg *rest.Config + baseCfg *rest.Config // kcp front-proxy URL without any workspace path suffix scheme *runtime.Scheme mu sync.Mutex @@ -193,27 +190,42 @@ func (w *workspaceResolver) ensureResolved(ctx context.Context, paths []string) return nil } - c, err := client.New(w.rootCfg, client.Options{Scheme: w.scheme}) - if err != nil { - return fmt.Errorf("creating root workspace client: %w", err) - } - + // c, err := client.New(w.rootCfg, client.Options{Scheme: w.scheme}) + // if err != nil { + // return fmt.Errorf("creating root workspace client: %w", err) + // } for _, path := range missing { - // Extract the workspace name from path like "root:compute-provider". - parts := strings.Split(path, ":") - wsName := parts[len(parts)-1] + if _, ok := w.cache[path]; ok { + continue + } - var ws tenancyv1alpha1.Workspace - if err := c.Get(ctx, client.ObjectKey{Name: wsName}, &ws); err != nil { - return fmt.Errorf("getting workspace %s: %w", wsName, err) + var parentPath string + if i := strings.LastIndex(path, ":"); i >= 0 { + parentPath = path[:i] } - if ws.Spec.Cluster == "" { - return fmt.Errorf("workspace %s has no logical cluster name", wsName) + parentCfg := rest.CopyConfig(w.baseCfg) + parentCfg.Host += logicalcluster.NewPath(parentPath).RequestPath() + + c, err := client.New(parentCfg, client.Options{Scheme: w.scheme}) + if err != nil { + return fmt.Errorf("creating %s workspace client: %w", parentPath, err) + } + + var wsList tenancyv1alpha1.WorkspaceList + if err := c.List(ctx, &wsList); err != nil { + return fmt.Errorf("list workspaces in %s: %w", parentPath, err) } - w.cache[path] = ws.Spec.Cluster - log.FromContext(ctx).Info("resolved workspace path", "path", path, "cluster", ws.Spec.Cluster) + for _, ws := range wsList.Items { + if ws.Spec.Cluster == "" { + return fmt.Errorf("workspace %s has no logical cluster name", ws.Name) + } + + wsPath := parentPath + ":" + ws.Name + w.cache[wsPath] = ws.Spec.Cluster + log.FromContext(ctx).Info("resolved workspace path", "path", wsPath, "cluster", ws.Spec.Cluster) + } } return nil From e9915f2b5ccc1d21969533d3c32c48b996cfba9c Mon Sep 17 00:00:00 2001 From: Cyrill Berg Date: Thu, 7 May 2026 17:28:24 +0200 Subject: [PATCH 2/3] feat: enhance workspace resolver with new client function to enable unit testing Signed-off-by: Cyrill Berg --- .golangci.yaml | 5 - .yamllint.yaml | 31 ++++ .../controller/dependencyrule_controller.go | 17 +- .../dependencyrule_controller_test.go | 168 ++++++++++++++++++ test/e2e/suite_test.go | 2 +- 5 files changed, 209 insertions(+), 14 deletions(-) create mode 100644 .yamllint.yaml create mode 100644 internal/controller/dependencyrule_controller_test.go diff --git a/.golangci.yaml b/.golangci.yaml index 51d19f9..56f897a 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -8,11 +8,6 @@ linters: max-blank-identifiers: 4 exhaustive: default-signifies-exhaustive: true - gosec: - excludes: - - G101 # Look for hardcoded credentials - - G204 # Audit use of command execution - - G306 # Poor file permissions used when writing to a file modernize: disable: - omitzero diff --git a/.yamllint.yaml b/.yamllint.yaml new file mode 100644 index 0000000..97fcca5 --- /dev/null +++ b/.yamllint.yaml @@ -0,0 +1,31 @@ +extends: default + +rules: + # Kubernetes/kcp manifests and Helm values are often wide; 120 is a better fit + # than the 80-char default. + line-length: + max: 120 + level: warning + + # APIResourceSchema CRD embeds lots of nested markers; tolerate deep indentation. + indentation: + spaces: 2 + indent-sequences: consistent + + # kubectl-generated manifests sometimes emit `on:` as a bare key (GitHub Actions). + truthy: + check-keys: false + + # Allow long comments (e.g. linked kcp docs URLs). + comments: + min-spaces-from-content: 1 + + # Machine-generated CRD YAML occasionally omits a terminating newline. + new-line-at-end-of-file: + level: warning + +ignore: | + hack/tools/ + bin/ + portal/node_modules/ + portal/dist/ diff --git a/internal/controller/dependencyrule_controller.go b/internal/controller/dependencyrule_controller.go index f1f95b4..d3b5b12 100644 --- a/internal/controller/dependencyrule_controller.go +++ b/internal/controller/dependencyrule_controller.go @@ -62,7 +62,11 @@ func (r *DependencyRuleReconciler) ensureInitialized() error { localMgr := r.DepCtrlManager.GetLocalManager() - r.wsResolver = &workspaceResolver{baseCfg: r.BaseConfig, scheme: localMgr.GetScheme()} + r.wsResolver = &workspaceResolver{ + baseCfg: r.BaseConfig, + scheme: localMgr.GetScheme(), + newClient: client.New, + } return nil } @@ -162,8 +166,9 @@ func (r *DependencyRuleReconciler) handleDeletion(ctx context.Context, key, rule // workspaceResolver maps kcp workspace paths (e.g., "root:compute-provider") // to logical cluster names by reading Workspace objects from the root workspace. type workspaceResolver struct { - baseCfg *rest.Config // kcp front-proxy URL without any workspace path suffix - scheme *runtime.Scheme + baseCfg *rest.Config // kcp front-proxy URL without any workspace path suffix + scheme *runtime.Scheme + newClient func(cfg *rest.Config, options client.Options) (client.Client, error) mu sync.Mutex cache map[string]string // workspace path -> logical cluster name @@ -190,10 +195,6 @@ func (w *workspaceResolver) ensureResolved(ctx context.Context, paths []string) return nil } - // c, err := client.New(w.rootCfg, client.Options{Scheme: w.scheme}) - // if err != nil { - // return fmt.Errorf("creating root workspace client: %w", err) - // } for _, path := range missing { if _, ok := w.cache[path]; ok { continue @@ -207,7 +208,7 @@ func (w *workspaceResolver) ensureResolved(ctx context.Context, paths []string) parentCfg := rest.CopyConfig(w.baseCfg) parentCfg.Host += logicalcluster.NewPath(parentPath).RequestPath() - c, err := client.New(parentCfg, client.Options{Scheme: w.scheme}) + c, err := w.newClient(parentCfg, client.Options{Scheme: w.scheme}) if err != nil { return fmt.Errorf("creating %s workspace client: %w", parentPath, err) } diff --git a/internal/controller/dependencyrule_controller_test.go b/internal/controller/dependencyrule_controller_test.go new file mode 100644 index 0000000..0eb5729 --- /dev/null +++ b/internal/controller/dependencyrule_controller_test.go @@ -0,0 +1,168 @@ +// Copyright 2026 BWI GmbH and Dependency Controller contributors +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/kcp-dev/logicalcluster/v3" + tenancyv1alpha1 "github.com/kcp-dev/sdk/apis/tenancy/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +func newTestResolver(factory func(*rest.Config, client.Options) (client.Client, error)) *workspaceResolver { + return &workspaceResolver{ + baseCfg: &rest.Config{Host: "https://kcp.example"}, + scheme: scheme.Scheme, + newClient: factory, + } +} + +func TestWorkspaceResolver_EnsureResolved_ResolvesPath(t *testing.T) { + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithLists(&tenancyv1alpha1.WorkspaceList{ + Items: []tenancyv1alpha1.Workspace{ + { + ObjectMeta: metav1.ObjectMeta{Name: "compute-provider"}, + Spec: tenancyv1alpha1.WorkspaceSpec{Cluster: "abc123"}, + }, + }, + }). + Build() + + r := newTestResolver(func(*rest.Config, client.Options) (client.Client, error) { + return fakeClient, nil + }) + + if err := r.ensureResolved(context.Background(), []string{"root:compute-provider"}); err != nil { + t.Fatalf("ensureResolved: %v", err) + } + + if got := r.cache["root:compute-provider"]; got != "abc123" { + t.Errorf("cache[root:compute-provider] = %q, want %q", got, "abc123") + } +} + +func TestWorkspaceResolver_EnsureResolved_UsesCache(t *testing.T) { + r := newTestResolver(func(*rest.Config, client.Options) (client.Client, error) { + t.Fatal("newClient should not be called for cached entries") + return nil, nil + }) + r.cache = map[string]string{"root:compute-provider": "abc123"} + + if err := r.ensureResolved(context.Background(), []string{"root:compute-provider"}); err != nil { + t.Fatalf("ensureResolved: %v", err) + } +} + +func TestWorkspaceResolver_EnsureResolved_ListErrorPropagates(t *testing.T) { + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return errors.New("kaboom") + }, + }). + Build() + + r := newTestResolver(func(*rest.Config, client.Options) (client.Client, error) { + return fakeClient, nil + }) + + err := r.ensureResolved(context.Background(), []string{"root:compute-provider"}) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "list workspaces in root") { + t.Errorf("unexpected error: %v", err) + } +} + +// TestWorkspaceResolver_EnsureResolved_RoutesPerParent verifies that paths +// under different parent workspaces each trigger a list against the matching +// parent: the factory dispatches on cfg.Host (which the production code +// builds as baseCfg.Host + logicalcluster.NewPath(parent).RequestPath()). +func TestWorkspaceResolver_EnsureResolved_RoutesPerParent(t *testing.T) { + const baseHost = "https://kcp.example" + + clientsByHost := map[string]client.Client{ + baseHost + logicalcluster.NewPath("root:org-a").RequestPath(): fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithLists(&tenancyv1alpha1.WorkspaceList{ + Items: []tenancyv1alpha1.Workspace{{ + ObjectMeta: metav1.ObjectMeta{Name: "team-a"}, + Spec: tenancyv1alpha1.WorkspaceSpec{Cluster: "cluster-a"}, + }, { + ObjectMeta: metav1.ObjectMeta{Name: "team-b"}, + Spec: tenancyv1alpha1.WorkspaceSpec{Cluster: "cluster-b"}, + }}, + }).Build(), + baseHost + logicalcluster.NewPath("root:org-b").RequestPath(): fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithLists(&tenancyv1alpha1.WorkspaceList{ + Items: []tenancyv1alpha1.Workspace{{ + ObjectMeta: metav1.ObjectMeta{Name: "team-y"}, + Spec: tenancyv1alpha1.WorkspaceSpec{Cluster: "cluster-y"}, + }}, + }).Build(), + } + + calls := map[string]int{} + factory := func(cfg *rest.Config, _ client.Options) (client.Client, error) { + calls[cfg.Host]++ + c, ok := clientsByHost[cfg.Host] + if !ok { + t.Errorf("unexpected host %q", cfg.Host) + return nil, errors.New("unexpected host") + } + + return c, nil + } + + r := &workspaceResolver{ + baseCfg: &rest.Config{Host: baseHost}, + scheme: scheme.Scheme, + newClient: factory, + } + + paths := []string{"root:org-a:team-a", "root:org-b:team-y"} + if err := r.ensureResolved(context.Background(), paths); err != nil { + t.Fatalf("ensureResolved: %v", err) + } + + if got := r.cache["root:org-a:team-a"]; got != "cluster-a" { + t.Errorf("cache[root:org-a:team-a] = %q, want cluster-a", got) + } + // test if workspace is in cache which was not requested but is in the same parent workspace, + // to verify that the list result is cached for the entire parent workspace, not just the requested path + if got := r.cache["root:org-a:team-b"]; got != "cluster-b" { + t.Errorf("cache[root:org-a:team-b] = %q, want cluster-b", got) + } + if got := r.cache["root:org-b:team-y"]; got != "cluster-y" { + t.Errorf("cache[root:org-b:team-y] = %q, want cluster-y", got) + } + if len(calls) != 2 { + t.Errorf("expected exactly 2 distinct hosts called, got %v", calls) + } +} + +func TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop(t *testing.T) { + r := newTestResolver(func(*rest.Config, client.Options) (client.Client, error) { + t.Fatal("newClient should not be called for empty input") + return nil, nil + }) + + if err := r.ensureResolved(context.Background(), nil); err != nil { + t.Fatalf("ensureResolved: %v", err) + } +} diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index 634b443..a5ac42e 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -920,7 +920,7 @@ spec: pfCmd := exec.CommandContext(context.Background(), kubectlBin, "--context", "kind-"+kindClusterName, "-n", kcpNamespace, "port-forward", - "svc/"+shardSvc, fmt.Sprintf("%d:%s", localPort, shardPort)) + "svc/"+shardSvc, fmt.Sprintf("%d:%s", localPort, shardPort)) // #nosec G204 -- kubectl is a trusted binary and the arguments are controlled. pfCmd.Stdout = GinkgoWriter pfCmd.Stderr = GinkgoWriter Expect(pfCmd.Start()).To(Succeed()) From 4b995255966874f97a93b763232463e1347d349f Mon Sep 17 00:00:00 2001 From: Cyrill Berg Date: Thu, 7 May 2026 20:46:51 +0200 Subject: [PATCH 3/3] feat: improve error handling and logging in workspace resolver to not fail hard on unready workspaces Signed-off-by: Cyrill Berg --- .../controller/dependencyrule_controller.go | 5 +++- .../dependencyrule_controller_test.go | 23 +++++++++++++------ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/internal/controller/dependencyrule_controller.go b/internal/controller/dependencyrule_controller.go index d3b5b12..38da613 100644 --- a/internal/controller/dependencyrule_controller.go +++ b/internal/controller/dependencyrule_controller.go @@ -203,6 +203,8 @@ func (w *workspaceResolver) ensureResolved(ctx context.Context, paths []string) var parentPath string if i := strings.LastIndex(path, ":"); i >= 0 { parentPath = path[:i] + } else { + return fmt.Errorf("workspace path %q must have at least one parent segment", path) } parentCfg := rest.CopyConfig(w.baseCfg) @@ -220,7 +222,8 @@ func (w *workspaceResolver) ensureResolved(ctx context.Context, paths []string) for _, ws := range wsList.Items { if ws.Spec.Cluster == "" { - return fmt.Errorf("workspace %s has no logical cluster name", ws.Name) + log.FromContext(ctx).Info("skipping workspace with no logical cluster name (still provisioning?)", "workspace", ws.Name, "parent", parentPath) + continue } wsPath := parentPath + ":" + ws.Name diff --git a/internal/controller/dependencyrule_controller_test.go b/internal/controller/dependencyrule_controller_test.go index 0eb5729..4e18216 100644 --- a/internal/controller/dependencyrule_controller_test.go +++ b/internal/controller/dependencyrule_controller_test.go @@ -12,24 +12,33 @@ import ( "github.com/kcp-dev/logicalcluster/v3" tenancyv1alpha1 "github.com/kcp-dev/sdk/apis/tenancy/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes/scheme" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) +var testScheme *runtime.Scheme + +func init() { + testScheme = runtime.NewScheme() + _ = clientgoscheme.AddToScheme(testScheme) + _ = tenancyv1alpha1.AddToScheme(testScheme) +} + func newTestResolver(factory func(*rest.Config, client.Options) (client.Client, error)) *workspaceResolver { return &workspaceResolver{ baseCfg: &rest.Config{Host: "https://kcp.example"}, - scheme: scheme.Scheme, + scheme: testScheme, newClient: factory, } } func TestWorkspaceResolver_EnsureResolved_ResolvesPath(t *testing.T) { fakeClient := fake.NewClientBuilder(). - WithScheme(scheme.Scheme). + WithScheme(testScheme). WithLists(&tenancyv1alpha1.WorkspaceList{ Items: []tenancyv1alpha1.Workspace{ { @@ -67,7 +76,7 @@ func TestWorkspaceResolver_EnsureResolved_UsesCache(t *testing.T) { func TestWorkspaceResolver_EnsureResolved_ListErrorPropagates(t *testing.T) { fakeClient := fake.NewClientBuilder(). - WithScheme(scheme.Scheme). + WithScheme(testScheme). WithInterceptorFuncs(interceptor.Funcs{ List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { return errors.New("kaboom") @@ -97,7 +106,7 @@ func TestWorkspaceResolver_EnsureResolved_RoutesPerParent(t *testing.T) { clientsByHost := map[string]client.Client{ baseHost + logicalcluster.NewPath("root:org-a").RequestPath(): fake.NewClientBuilder(). - WithScheme(scheme.Scheme). + WithScheme(testScheme). WithLists(&tenancyv1alpha1.WorkspaceList{ Items: []tenancyv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{Name: "team-a"}, @@ -108,7 +117,7 @@ func TestWorkspaceResolver_EnsureResolved_RoutesPerParent(t *testing.T) { }}, }).Build(), baseHost + logicalcluster.NewPath("root:org-b").RequestPath(): fake.NewClientBuilder(). - WithScheme(scheme.Scheme). + WithScheme(testScheme). WithLists(&tenancyv1alpha1.WorkspaceList{ Items: []tenancyv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{Name: "team-y"}, @@ -131,7 +140,7 @@ func TestWorkspaceResolver_EnsureResolved_RoutesPerParent(t *testing.T) { r := &workspaceResolver{ baseCfg: &rest.Config{Host: baseHost}, - scheme: scheme.Scheme, + scheme: testScheme, newClient: factory, }