From 4a463c0ecf47906e7a8cf69da50a2decf2e6f4bb Mon Sep 17 00:00:00 2001 From: Simon Bein Date: Thu, 18 Jun 2026 16:52:37 +0200 Subject: [PATCH 1/3] refactor: use logicalcluster.Path in internal client parsing On-behalf-of: SAP Signed-off-by: Simon Bein --- internal/client/clients.go | 14 +++++++------- internal/client/frontproxy.go | 2 +- internal/controller/kubeconfig-rbac/controller.go | 4 ++-- internal/controller/shard/controller.go | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/client/clients.go b/internal/client/clients.go index 41a6421b..815b2fe9 100644 --- a/internal/client/clients.go +++ b/internal/client/clients.go @@ -33,29 +33,29 @@ import ( ) // NewRootShardClient returns a new client for talking to the kcp root shard service directly. -func NewRootShardClient(ctx context.Context, c ctrlruntimeclient.Client, rootShard *operatorv1alpha1.RootShard, cluster logicalcluster.Name, scheme *runtime.Scheme) (ctrlruntimeclient.Client, error) { +func NewRootShardClient(ctx context.Context, c ctrlruntimeclient.Client, rootShard *operatorv1alpha1.RootShard, cluster logicalcluster.Path, scheme *runtime.Scheme) (ctrlruntimeclient.Client, error) { baseUrl := fmt.Sprintf("https://%s.%s.svc.cluster.local:6443", resources.GetRootShardServiceName(rootShard), rootShard.Namespace) if !cluster.Empty() { - baseUrl = fmt.Sprintf("%s/clusters/%s", baseUrl, cluster.String()) + baseUrl += cluster.RequestPath() } return newClient(ctx, c, baseUrl, scheme, rootShard) } -// NewRootShardClient returns a new client that connects to the operator's internal front-proxy. -func NewRootShardProxyClient(ctx context.Context, c ctrlruntimeclient.Client, rootShard *operatorv1alpha1.RootShard, cluster logicalcluster.Name, scheme *runtime.Scheme) (ctrlruntimeclient.Client, error) { +// NewRootShardProxyClient returns a new client that connects to the operator's internal front-proxy. +func NewRootShardProxyClient(ctx context.Context, c ctrlruntimeclient.Client, rootShard *operatorv1alpha1.RootShard, cluster logicalcluster.Path, scheme *runtime.Scheme) (ctrlruntimeclient.Client, error) { baseUrl := fmt.Sprintf("https://%s.%s.svc.cluster.local:6443", resources.GetRootShardProxyServiceName(rootShard), rootShard.Namespace) if !cluster.Empty() { - baseUrl = fmt.Sprintf("%s/clusters/%s", baseUrl, cluster.String()) + baseUrl += cluster.RequestPath() } return newClient(ctx, c, baseUrl, scheme, rootShard) } // NewShardClient returns a new client for talking to a kcp shard service directly. -func NewShardClient(ctx context.Context, c ctrlruntimeclient.Client, shard *operatorv1alpha1.Shard, cluster logicalcluster.Name, scheme *runtime.Scheme) (ctrlruntimeclient.Client, error) { +func NewShardClient(ctx context.Context, c ctrlruntimeclient.Client, shard *operatorv1alpha1.Shard, cluster logicalcluster.Path, scheme *runtime.Scheme) (ctrlruntimeclient.Client, error) { rootShard, err := getRootShardForShard(ctx, c, shard) if err != nil { return nil, fmt.Errorf("failed to determine effective RootShard: %w", err) @@ -64,7 +64,7 @@ func NewShardClient(ctx context.Context, c ctrlruntimeclient.Client, shard *oper baseUrl := fmt.Sprintf("https://%s.%s.svc.cluster.local:6443", resources.GetShardServiceName(shard), shard.Namespace) if !cluster.Empty() { - baseUrl = fmt.Sprintf("%s/clusters/%s", baseUrl, cluster.String()) + baseUrl += cluster.RequestPath() } return newClient(ctx, c, baseUrl, scheme, rootShard) diff --git a/internal/client/frontproxy.go b/internal/client/frontproxy.go index a708c601..1f276f68 100644 --- a/internal/client/frontproxy.go +++ b/internal/client/frontproxy.go @@ -37,7 +37,7 @@ import ( // type of shard, the client will also directly connect to that shard, but for Kubeconfigs using // a FrontProxy, the client will instead use the operator-internal front-proxy (which specifically // does not drop groups/permissions). -func NewInternalKubeconfigClient(ctx context.Context, c ctrlruntimeclient.Client, kubeconfig *operatorv1alpha1.Kubeconfig, cluster logicalcluster.Name, scheme *runtime.Scheme) (ctrlruntimeclient.Client, error) { +func NewInternalKubeconfigClient(ctx context.Context, c ctrlruntimeclient.Client, kubeconfig *operatorv1alpha1.Kubeconfig, cluster logicalcluster.Path, scheme *runtime.Scheme) (ctrlruntimeclient.Client, error) { target := kubeconfig.Spec.Target switch { diff --git a/internal/controller/kubeconfig-rbac/controller.go b/internal/controller/kubeconfig-rbac/controller.go index 8fa3a38a..8b4c9dae 100644 --- a/internal/controller/kubeconfig-rbac/controller.go +++ b/internal/controller/kubeconfig-rbac/controller.go @@ -132,7 +132,7 @@ func (r *KubeconfigRBACReconciler) reconcile(ctx context.Context, config *operat } func (r *KubeconfigRBACReconciler) reconcileBindings(ctx context.Context, kc *operatorv1alpha1.Kubeconfig) error { - targetClient, err := client.NewInternalKubeconfigClient(ctx, r.Client, kc, logicalcluster.Name(kc.Spec.Authorization.ClusterRoleBindings.Cluster), nil) + targetClient, err := client.NewInternalKubeconfigClient(ctx, r.Client, kc, logicalcluster.NewPath(kc.Spec.Authorization.ClusterRoleBindings.Cluster), nil) if err != nil { return fmt.Errorf("failed to create client to kubeconfig target: %w", err) } @@ -207,7 +207,7 @@ func (r *KubeconfigRBACReconciler) unprovisionCluster(ctx context.Context, kc *o return nil } - targetClient, err := client.NewInternalKubeconfigClient(ctx, r.Client, kc, logicalcluster.Name(cluster), nil) + targetClient, err := client.NewInternalKubeconfigClient(ctx, r.Client, kc, logicalcluster.NewPath(cluster), nil) if err != nil { return fmt.Errorf("failed to create client to kubeconfig target: %w", err) } diff --git a/internal/controller/shard/controller.go b/internal/controller/shard/controller.go index 2e68ad4d..aa625620 100644 --- a/internal/controller/shard/controller.go +++ b/internal/controller/shard/controller.go @@ -338,7 +338,7 @@ func (r *ShardReconciler) handleDeletion(ctx context.Context, s *operatorv1alpha } // Create client to root shard - kcpClient, err := client.NewRootShardClient(ctx, r.Client, rootShard, logicalcluster.Name("root"), r.Scheme) + kcpClient, err := client.NewRootShardClient(ctx, r.Client, rootShard, logicalcluster.NewPath("root"), r.Scheme) if err != nil { return nil, fmt.Errorf("failed to create root shard client: %w", err) } From 537bd536c287f37e0005193d2e14095de60ceff5 Mon Sep 17 00:00:00 2001 From: Simon Bein Date: Thu, 18 Jun 2026 16:54:55 +0200 Subject: [PATCH 2/3] specify target kubeconfig url via spec On-behalf-of: SAP Signed-off-by: Simon Bein --- .../bases/operator.kcp.io_kubeconfigs.yaml | 20 +- .../controller/kubeconfig-rbac/controller.go | 6 +- internal/resources/kubeconfig/secret.go | 6 +- .../operator/v1alpha1/kubeconfig_types.go | 31 ++- .../v1alpha1/kubeconfig_types_test.go | 97 ++++++++++ .../operator/v1alpha1/kubeconfigspec.go | 9 + test/e2e/kubeconfig-rbac/frontproxies_test.go | 180 +++++++++++------- 7 files changed, 266 insertions(+), 83 deletions(-) create mode 100644 sdk/apis/operator/v1alpha1/kubeconfig_types_test.go diff --git a/config/crd/bases/operator.kcp.io_kubeconfigs.yaml b/config/crd/bases/operator.kcp.io_kubeconfigs.yaml index 66d5c3b6..c940b927 100644 --- a/config/crd/bases/operator.kcp.io_kubeconfigs.yaml +++ b/config/crd/bases/operator.kcp.io_kubeconfigs.yaml @@ -56,15 +56,17 @@ spec: clusterRoleBindings: properties: cluster: - description: Cluster can be either a cluster name or a workspace - path. + description: |- + Cluster can be either a cluster name or a workspace path. + + Deprecated: Use spec.targetWorkspace instead. This field is kept for backward + compatibility but cannot be set together with spec.targetWorkspace. type: string clusterRoles: items: type: string type: array required: - - cluster - clusterRoles type: object required: @@ -353,6 +355,14 @@ spec: type: object x-kubernetes-map-type: atomic type: object + targetWorkspace: + description: |- + TargetWorkspace specifies the workspace path this kubeconfig targets. + Used in the generated kubeconfig server URL and as the default RBAC provisioning target. + Accepts kcp workspace paths like "root:org:team". + Defaults to "root" if unset. + pattern: ^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(:[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*$ + type: string username: description: Username defines the username embedded in the TLS certificate generated for this kubeconfig. @@ -368,6 +378,10 @@ spec: - username - validity type: object + x-kubernetes-validations: + - message: Cannot set both targetWorkspace and authorization.clusterRoleBindings.cluster. + Use targetWorkspace only. + rule: '!(has(self.targetWorkspace) && has(self.authorization) && has(self.authorization.clusterRoleBindings.cluster))' status: description: KubeconfigStatus defines the observed state of Kubeconfig properties: diff --git a/internal/controller/kubeconfig-rbac/controller.go b/internal/controller/kubeconfig-rbac/controller.go index 8b4c9dae..a781f3d3 100644 --- a/internal/controller/kubeconfig-rbac/controller.go +++ b/internal/controller/kubeconfig-rbac/controller.go @@ -84,8 +84,8 @@ func (r *KubeconfigRBACReconciler) reconcile(ctx context.Context, config *operat if auth := config.Status.Authorization; auth != nil { oldCluster = auth.ProvisionedCluster } - if auth := config.Spec.Authorization; auth != nil { - newCluster = auth.ClusterRoleBindings.Cluster + if config.Spec.Authorization != nil { + newCluster = config.GetTargetWorkspace().String() } // All `return nil` here are because the Kubeconfig has been modified and will be requeued anyway. @@ -132,7 +132,7 @@ func (r *KubeconfigRBACReconciler) reconcile(ctx context.Context, config *operat } func (r *KubeconfigRBACReconciler) reconcileBindings(ctx context.Context, kc *operatorv1alpha1.Kubeconfig) error { - targetClient, err := client.NewInternalKubeconfigClient(ctx, r.Client, kc, logicalcluster.NewPath(kc.Spec.Authorization.ClusterRoleBindings.Cluster), nil) + targetClient, err := client.NewInternalKubeconfigClient(ctx, r.Client, kc, kc.GetTargetWorkspace(), nil) if err != nil { return fmt.Errorf("failed to create client to kubeconfig target: %w", err) } diff --git a/internal/resources/kubeconfig/secret.go b/internal/resources/kubeconfig/secret.go index 42db34a6..d53824a9 100644 --- a/internal/resources/kubeconfig/secret.go +++ b/internal/resources/kubeconfig/secret.go @@ -87,7 +87,7 @@ func KubeconfigSecretReconciler( } serverURL := resources.GetRootShardBaseURL(rootShard) - defaultURL, err := url.JoinPath(serverURL, "clusters", "root") + defaultURL, err := url.JoinPath(serverURL, kubeconfig.GetTargetWorkspace().RequestPath()) if err != nil { return nil, err } @@ -105,7 +105,7 @@ func KubeconfigSecretReconciler( } serverURL := resources.GetShardBaseURL(shard) - defaultURL, err := url.JoinPath(serverURL, "clusters", "root") + defaultURL, err := url.JoinPath(serverURL, kubeconfig.GetTargetWorkspace().RequestPath()) if err != nil { return nil, err } @@ -137,7 +137,7 @@ func KubeconfigSecretReconciler( serverURL = fmt.Sprintf("https://%s:%d", rootShard.Spec.External.Hostname, rootShard.Spec.External.Port) } - defaultURL, err := url.JoinPath(serverURL, "clusters", "root") + defaultURL, err := url.JoinPath(serverURL, kubeconfig.GetTargetWorkspace().RequestPath()) if err != nil { return nil, err } diff --git a/sdk/apis/operator/v1alpha1/kubeconfig_types.go b/sdk/apis/operator/v1alpha1/kubeconfig_types.go index 62e62e30..516665ba 100644 --- a/sdk/apis/operator/v1alpha1/kubeconfig_types.go +++ b/sdk/apis/operator/v1alpha1/kubeconfig_types.go @@ -19,15 +19,26 @@ package v1alpha1 import ( "fmt" + "github.com/kcp-dev/logicalcluster/v3" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // KubeconfigSpec defines the desired state of Kubeconfig. +// +kubebuilder:validation:XValidation:rule="!(has(self.targetWorkspace) && has(self.authorization) && has(self.authorization.clusterRoleBindings.cluster))",message="Cannot set both targetWorkspace and authorization.clusterRoleBindings.cluster. Use targetWorkspace only." type KubeconfigSpec struct { // Target configures which kcp-operator object this kubeconfig should be generated for (shard or front-proxy). Target KubeconfigTarget `json:"target"` + // TargetWorkspace specifies the workspace path this kubeconfig targets. + // Used in the generated kubeconfig server URL and as the default RBAC provisioning target. + // Accepts kcp workspace paths like "root:org:team". + // Defaults to "root" if unset. + // +optional + // +kubebuilder:validation:Pattern=`^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(:[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*$` + TargetWorkspace string `json:"targetWorkspace,omitempty"` + // Username defines the username embedded in the TLS certificate generated for this kubeconfig. Username string `json:"username"` // Username defines the groups embedded in the TLS certificate generated for this kubeconfig. @@ -59,7 +70,12 @@ type KubeconfigAuthorization struct { type KubeconfigClusterRoleBindings struct { // Cluster can be either a cluster name or a workspace path. - Cluster string `json:"cluster"` + // + // Deprecated: Use spec.targetWorkspace instead. This field is kept for backward + // compatibility but cannot be set together with spec.targetWorkspace. + // +optional + Cluster string `json:"cluster,omitempty"` + ClusterRoles []string `json:"clusterRoles"` } @@ -109,6 +125,19 @@ func (k *Kubeconfig) GetCertificateName() string { return fmt.Sprintf("kubeconfig-cert-%s", k.Name) } +// GetTargetWorkspace returns the resolved workspace path for this kubeconfig. +// It checks spec.targetWorkspace first, then falls back to +// spec.authorization.clusterRoleBindings.cluster (deprecated), and defaults to "root". +func (k *Kubeconfig) GetTargetWorkspace() logicalcluster.Path { + if k.Spec.TargetWorkspace != "" { + return logicalcluster.NewPath(k.Spec.TargetWorkspace) + } + if auth := k.Spec.Authorization; auth != nil && auth.ClusterRoleBindings.Cluster != "" { + return logicalcluster.NewPath(auth.ClusterRoleBindings.Cluster) + } + return logicalcluster.NewPath("root") +} + // +kubebuilder:object:root=true // KubeconfigList contains a list of Kubeconfig diff --git a/sdk/apis/operator/v1alpha1/kubeconfig_types_test.go b/sdk/apis/operator/v1alpha1/kubeconfig_types_test.go new file mode 100644 index 00000000..5c5be2ed --- /dev/null +++ b/sdk/apis/operator/v1alpha1/kubeconfig_types_test.go @@ -0,0 +1,97 @@ +/* +Copyright 2026 The kcp Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "testing" + + "github.com/kcp-dev/logicalcluster/v3" +) + +func TestGetTargetWorkspace(t *testing.T) { + tests := []struct { + name string + kc *Kubeconfig + expected logicalcluster.Path + }{ + { + name: "targetWorkspace set", + kc: &Kubeconfig{ + Spec: KubeconfigSpec{ + TargetWorkspace: "root:org:team", + }, + }, + expected: logicalcluster.NewPath("root:org:team"), + }, + { + name: "targetWorkspace empty, deprecated cluster set", + kc: &Kubeconfig{ + Spec: KubeconfigSpec{ + Authorization: &KubeconfigAuthorization{ + ClusterRoleBindings: KubeconfigClusterRoleBindings{ + Cluster: "root:legacy:workspace", + }, + }, + }, + }, + expected: logicalcluster.NewPath("root:legacy:workspace"), + }, + { + name: "both empty defaults to root", + kc: &Kubeconfig{ + Spec: KubeconfigSpec{}, + }, + expected: logicalcluster.NewPath("root"), + }, + { + name: "authorization set without cluster defaults to root", + kc: &Kubeconfig{ + Spec: KubeconfigSpec{ + Authorization: &KubeconfigAuthorization{ + ClusterRoleBindings: KubeconfigClusterRoleBindings{ + ClusterRoles: []string{"admin"}, + }, + }, + }, + }, + expected: logicalcluster.NewPath("root"), + }, + { + name: "targetWorkspace takes precedence over deprecated cluster", + kc: &Kubeconfig{ + Spec: KubeconfigSpec{ + TargetWorkspace: "root:new", + Authorization: &KubeconfigAuthorization{ + ClusterRoleBindings: KubeconfigClusterRoleBindings{ + Cluster: "root:old", + }, + }, + }, + }, + expected: logicalcluster.NewPath("root:new"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.kc.GetTargetWorkspace() + if !got.Equal(tt.expected) { + t.Errorf("got %q, want %q", got, tt.expected) + } + }) + } +} diff --git a/sdk/applyconfiguration/operator/v1alpha1/kubeconfigspec.go b/sdk/applyconfiguration/operator/v1alpha1/kubeconfigspec.go index f26cbee9..d8905269 100644 --- a/sdk/applyconfiguration/operator/v1alpha1/kubeconfigspec.go +++ b/sdk/applyconfiguration/operator/v1alpha1/kubeconfigspec.go @@ -27,6 +27,7 @@ import ( // with apply. type KubeconfigSpecApplyConfiguration struct { Target *KubeconfigTargetApplyConfiguration `json:"target,omitempty"` + TargetWorkspace *string `json:"targetWorkspace,omitempty"` Username *string `json:"username,omitempty"` Groups []string `json:"groups,omitempty"` Validity *v1.Duration `json:"validity,omitempty"` @@ -49,6 +50,14 @@ func (b *KubeconfigSpecApplyConfiguration) WithTarget(value *KubeconfigTargetApp return b } +// WithTargetWorkspace sets the TargetWorkspace field in the declarative configuration to the given value +// and returns the receiver, so that objects can be built by chaining "With" function invocations. +// If called multiple times, the TargetWorkspace field is set to the value of the last call. +func (b *KubeconfigSpecApplyConfiguration) WithTargetWorkspace(value string) *KubeconfigSpecApplyConfiguration { + b.TargetWorkspace = &value + return b +} + // WithUsername sets the Username field in the declarative configuration to the given value // and returns the receiver, so that objects can be built by chaining "With" function invocations. // If called multiple times, the Username field is set to the value of the last call. diff --git a/test/e2e/kubeconfig-rbac/frontproxies_test.go b/test/e2e/kubeconfig-rbac/frontproxies_test.go index 32f50053..1e11a777 100644 --- a/test/e2e/kubeconfig-rbac/frontproxies_test.go +++ b/test/e2e/kubeconfig-rbac/frontproxies_test.go @@ -87,83 +87,117 @@ func TestProvisionFrontProxyRBAC(t *testing.T) { t.Fatalf("Failed to wait for workspace to become available: %v", err) } - // create my-config kubeconfig - configSecretName := "kubeconfig-my-config-e2e" - - // as of now, this Kubeconfig will not grant any permissions yet - fpConfig := operatorv1alpha1.Kubeconfig{} - fpConfig.Name = "my-config" - fpConfig.Namespace = namespace.Name - fpConfig.Spec = operatorv1alpha1.KubeconfigSpec{ - Target: operatorv1alpha1.KubeconfigTarget{ - FrontProxyRef: &corev1.LocalObjectReference{ - Name: frontProxy.Name, + testcases := []struct { + name string + applyRBAC func(kc *operatorv1alpha1.Kubeconfig) + removeRBAC func(kc *operatorv1alpha1.Kubeconfig) + }{ + { + name: "using spec.targetWorkspace", + applyRBAC: func(kc *operatorv1alpha1.Kubeconfig) { + kc.Spec.TargetWorkspace = dummyCluster.String() + kc.Spec.Authorization = &operatorv1alpha1.KubeconfigAuthorization{ + ClusterRoleBindings: operatorv1alpha1.KubeconfigClusterRoleBindings{ + ClusterRoles: []string{"cluster-admin"}, + }, + } + }, + removeRBAC: func(kc *operatorv1alpha1.Kubeconfig) { + kc.Spec.TargetWorkspace = "" + kc.Spec.Authorization = nil }, }, - Username: "e2e", - Validity: metav1.Duration{Duration: 2 * time.Hour}, - SecretRef: corev1.LocalObjectReference{ - Name: configSecretName, - }, - } - - t.Log("Creating kubeconfig with no permissions attached…") - if err := client.Create(ctx, &fpConfig); err != nil { - t.Fatal(err) - } - utils.WaitForObject(t, ctx, client, &corev1.Secret{}, types.NamespacedName{Namespace: fpConfig.Namespace, Name: fpConfig.Spec.SecretRef.Name}) - - t.Log("Connecting to FrontProxy…") - kcpClient := utils.ConnectWithKubeconfig(t, ctx, client, namespace.Name, fpConfig.Name, dummyCluster) - - // This should not work yet. - t.Logf("Should not be able to list Secrets in %v.", dummyCluster) - if err := kcpClient.List(ctx, &corev1.SecretList{}); err == nil { - t.Fatal("Should not have been able to list Secrets, but was. Where have my permissions come from?") - } - - // Now we extend the Kubeconfig with additional permissions. - if err := client.Get(ctx, ctrlruntimeclient.ObjectKeyFromObject(&fpConfig), &fpConfig); err != nil { - t.Fatal(err) - } - - fpConfig.Spec.Authorization = &operatorv1alpha1.KubeconfigAuthorization{ - ClusterRoleBindings: operatorv1alpha1.KubeconfigClusterRoleBindings{ - Cluster: dummyCluster.String(), - ClusterRoles: []string{"cluster-admin"}, + { + name: "using deprecated authorization.clusterRoleBindings.cluster", + applyRBAC: func(kc *operatorv1alpha1.Kubeconfig) { + kc.Spec.Authorization = &operatorv1alpha1.KubeconfigAuthorization{ + ClusterRoleBindings: operatorv1alpha1.KubeconfigClusterRoleBindings{ + Cluster: dummyCluster.String(), + ClusterRoles: []string{"cluster-admin"}, + }, + } + }, + removeRBAC: func(kc *operatorv1alpha1.Kubeconfig) { + kc.Spec.Authorization = nil + }, }, } - t.Log("Updating kubeconfig with permissions attached…") - if err := client.Update(ctx, &fpConfig); err != nil { - t.Fatal(err) - } - - t.Logf("Should now be able to list Secrets in %v.", dummyCluster) - err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) { - return kcpClient.List(ctx, &corev1.SecretList{}) == nil, nil - }) - if err != nil { - t.Fatalf("Failed to list Secrets in dummy workspace: %v", err) - } - - // And now we remove the permissions again. - t.Log("Updating kubeconfig to remove the attached permissions…") - if err := client.Get(ctx, ctrlruntimeclient.ObjectKeyFromObject(&fpConfig), &fpConfig); err != nil { - t.Fatal(err) - } - - fpConfig.Spec.Authorization = nil - - if err := client.Update(ctx, &fpConfig); err != nil { - t.Fatal(err) - } - - t.Logf("Should no longer be able to list Secrets in %v.", dummyCluster) - err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) { - return kcpClient.List(ctx, &corev1.SecretList{}) != nil, nil - }) - if err != nil { - t.Fatalf("Failed to wait for permissions to be gone: %v", err) + for i, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + configSecretName := fmt.Sprintf("kubeconfig-rbac-e2e-%d", i) + + // as of now, this Kubeconfig will not grant any permissions yet + fpConfig := operatorv1alpha1.Kubeconfig{} + fpConfig.Name = fmt.Sprintf("rbac-test-%d", i) + fpConfig.Namespace = namespace.Name + fpConfig.Spec = operatorv1alpha1.KubeconfigSpec{ + Target: operatorv1alpha1.KubeconfigTarget{ + FrontProxyRef: &corev1.LocalObjectReference{ + Name: frontProxy.Name, + }, + }, + Username: "e2e", + Validity: metav1.Duration{Duration: 2 * time.Hour}, + SecretRef: corev1.LocalObjectReference{ + Name: configSecretName, + }, + } + + t.Log("Creating kubeconfig with no permissions attached…") + if err := client.Create(ctx, &fpConfig); err != nil { + t.Fatal(err) + } + utils.WaitForObject(t, ctx, client, &corev1.Secret{}, types.NamespacedName{Namespace: fpConfig.Namespace, Name: fpConfig.Spec.SecretRef.Name}) + + t.Log("Connecting to FrontProxy…") + kcpClient := utils.ConnectWithKubeconfig(t, ctx, client, namespace.Name, fpConfig.Name, dummyCluster) + + // This should not work yet. + t.Logf("Should not be able to list Secrets in %v.", dummyCluster) + if err := kcpClient.List(ctx, &corev1.SecretList{}); err == nil { + t.Fatal("Should not have been able to list Secrets, but was. Where have my permissions come from?") + } + + // Now we extend the Kubeconfig with additional permissions. + if err := client.Get(ctx, ctrlruntimeclient.ObjectKeyFromObject(&fpConfig), &fpConfig); err != nil { + t.Fatal(err) + } + + tc.applyRBAC(&fpConfig) + + t.Log("Updating kubeconfig with permissions attached…") + if err := client.Update(ctx, &fpConfig); err != nil { + t.Fatal(err) + } + + t.Logf("Should now be able to list Secrets in %v.", dummyCluster) + err := wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) { + return kcpClient.List(ctx, &corev1.SecretList{}) == nil, nil + }) + if err != nil { + t.Fatalf("Failed to list Secrets in dummy workspace: %v", err) + } + + // And now we remove the permissions again. + t.Log("Updating kubeconfig to remove the attached permissions…") + if err := client.Get(ctx, ctrlruntimeclient.ObjectKeyFromObject(&fpConfig), &fpConfig); err != nil { + t.Fatal(err) + } + + tc.removeRBAC(&fpConfig) + + if err := client.Update(ctx, &fpConfig); err != nil { + t.Fatal(err) + } + + t.Logf("Should no longer be able to list Secrets in %v.", dummyCluster) + err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) { + return kcpClient.List(ctx, &corev1.SecretList{}) != nil, nil + }) + if err != nil { + t.Fatalf("Failed to wait for permissions to be gone: %v", err) + } + }) } } From c0e8470190f857231d23b2352366fc9e7dfc3911 Mon Sep 17 00:00:00 2001 From: Simon Bein Date: Thu, 18 Jun 2026 17:12:21 +0200 Subject: [PATCH 3/3] do not set url when using clusterrolebindings.cluster field This is required so we are not creating a breaking change On-behalf-of: SAP Signed-off-by: Simon Bein --- .../controller/kubeconfig-rbac/controller.go | 4 +- .../operator/v1alpha1/kubeconfig_types.go | 15 +++++- .../v1alpha1/kubeconfig_types_test.go | 49 ++++++++++++++++++- 3 files changed, 63 insertions(+), 5 deletions(-) diff --git a/internal/controller/kubeconfig-rbac/controller.go b/internal/controller/kubeconfig-rbac/controller.go index a781f3d3..144f2d06 100644 --- a/internal/controller/kubeconfig-rbac/controller.go +++ b/internal/controller/kubeconfig-rbac/controller.go @@ -85,7 +85,7 @@ func (r *KubeconfigRBACReconciler) reconcile(ctx context.Context, config *operat oldCluster = auth.ProvisionedCluster } if config.Spec.Authorization != nil { - newCluster = config.GetTargetWorkspace().String() + newCluster = config.GetRBACTargetWorkspace().String() } // All `return nil` here are because the Kubeconfig has been modified and will be requeued anyway. @@ -132,7 +132,7 @@ func (r *KubeconfigRBACReconciler) reconcile(ctx context.Context, config *operat } func (r *KubeconfigRBACReconciler) reconcileBindings(ctx context.Context, kc *operatorv1alpha1.Kubeconfig) error { - targetClient, err := client.NewInternalKubeconfigClient(ctx, r.Client, kc, kc.GetTargetWorkspace(), nil) + targetClient, err := client.NewInternalKubeconfigClient(ctx, r.Client, kc, kc.GetRBACTargetWorkspace(), nil) if err != nil { return fmt.Errorf("failed to create client to kubeconfig target: %w", err) } diff --git a/sdk/apis/operator/v1alpha1/kubeconfig_types.go b/sdk/apis/operator/v1alpha1/kubeconfig_types.go index 516665ba..962f6f23 100644 --- a/sdk/apis/operator/v1alpha1/kubeconfig_types.go +++ b/sdk/apis/operator/v1alpha1/kubeconfig_types.go @@ -125,10 +125,21 @@ func (k *Kubeconfig) GetCertificateName() string { return fmt.Sprintf("kubeconfig-cert-%s", k.Name) } -// GetTargetWorkspace returns the resolved workspace path for this kubeconfig. +// GetTargetWorkspace returns the workspace path for the generated kubeconfig URL. +// Only spec.targetWorkspace is considered; defaults to "root". +// The deprecated authorization.clusterRoleBindings.cluster field does NOT influence +// the URL to avoid breaking existing users. +func (k *Kubeconfig) GetTargetWorkspace() logicalcluster.Path { + if k.Spec.TargetWorkspace != "" { + return logicalcluster.NewPath(k.Spec.TargetWorkspace) + } + return logicalcluster.NewPath("root") +} + +// GetRBACTargetWorkspace returns the workspace path for RBAC provisioning. // It checks spec.targetWorkspace first, then falls back to // spec.authorization.clusterRoleBindings.cluster (deprecated), and defaults to "root". -func (k *Kubeconfig) GetTargetWorkspace() logicalcluster.Path { +func (k *Kubeconfig) GetRBACTargetWorkspace() logicalcluster.Path { if k.Spec.TargetWorkspace != "" { return logicalcluster.NewPath(k.Spec.TargetWorkspace) } diff --git a/sdk/apis/operator/v1alpha1/kubeconfig_types_test.go b/sdk/apis/operator/v1alpha1/kubeconfig_types_test.go index 5c5be2ed..951b12d9 100644 --- a/sdk/apis/operator/v1alpha1/kubeconfig_types_test.go +++ b/sdk/apis/operator/v1alpha1/kubeconfig_types_test.go @@ -23,6 +23,53 @@ import ( ) func TestGetTargetWorkspace(t *testing.T) { + tests := []struct { + name string + kc *Kubeconfig + expected logicalcluster.Path + }{ + { + name: "targetWorkspace set", + kc: &Kubeconfig{ + Spec: KubeconfigSpec{ + TargetWorkspace: "root:org:team", + }, + }, + expected: logicalcluster.NewPath("root:org:team"), + }, + { + name: "deprecated cluster does NOT affect URL", + kc: &Kubeconfig{ + Spec: KubeconfigSpec{ + Authorization: &KubeconfigAuthorization{ + ClusterRoleBindings: KubeconfigClusterRoleBindings{ + Cluster: "root:legacy:workspace", + }, + }, + }, + }, + expected: logicalcluster.NewPath("root"), + }, + { + name: "both empty defaults to root", + kc: &Kubeconfig{ + Spec: KubeconfigSpec{}, + }, + expected: logicalcluster.NewPath("root"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.kc.GetTargetWorkspace() + if !got.Equal(tt.expected) { + t.Errorf("got %q, want %q", got, tt.expected) + } + }) + } +} + +func TestGetRBACTargetWorkspace(t *testing.T) { tests := []struct { name string kc *Kubeconfig @@ -88,7 +135,7 @@ func TestGetTargetWorkspace(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := tt.kc.GetTargetWorkspace() + got := tt.kc.GetRBACTargetWorkspace() if !got.Equal(tt.expected) { t.Errorf("got %q, want %q", got, tt.expected) }