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 8e948ed..38da613 100644 --- a/internal/controller/dependencyrule_controller.go +++ b/internal/controller/dependencyrule_controller.go @@ -62,10 +62,11 @@ 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(), + newClient: client.New, + } return nil } @@ -165,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 { - rootCfg *rest.Config - 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 @@ -193,27 +195,41 @@ 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 { - // 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] + } else { + return fmt.Errorf("workspace path %q must have at least one parent segment", path) } - 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 := w.newClient(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 == "" { + log.FromContext(ctx).Info("skipping workspace with no logical cluster name (still provisioning?)", "workspace", ws.Name, "parent", parentPath) + continue + } + + 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 diff --git a/internal/controller/dependencyrule_controller_test.go b/internal/controller/dependencyrule_controller_test.go new file mode 100644 index 0000000..4e18216 --- /dev/null +++ b/internal/controller/dependencyrule_controller_test.go @@ -0,0 +1,177 @@ +// 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/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: testScheme, + newClient: factory, + } +} + +func TestWorkspaceResolver_EnsureResolved_ResolvesPath(t *testing.T) { + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + 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(testScheme). + 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(testScheme). + 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(testScheme). + 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: testScheme, + 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())