diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index f3ac65c05..d906a52df 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -17,7 +17,6 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/resource" @@ -32,6 +31,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/rbac" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation" "github.com/stacklok/toolhive/pkg/container/kubernetes" ) @@ -44,42 +44,9 @@ type MCPServerReconciler struct { detectedPlatform kubernetes.Platform platformOnce sync.Once ImageValidation validation.ImageValidation + RBACManager rbac.Manager } -// defaultRBACRules are the default RBAC rules that the -// ToolHive ProxyRunner and/or MCP server needs to have in order to run. -var defaultRBACRules = []rbacv1.PolicyRule{ - { - APIGroups: []string{"apps"}, - Resources: []string{"statefulsets"}, - Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete", "apply"}, - }, - { - APIGroups: []string{""}, - Resources: []string{"services"}, - Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete", "apply"}, - }, - { - APIGroups: []string{""}, - Resources: []string{"pods"}, - Verbs: []string{"get", "list", "watch"}, - }, - { - APIGroups: []string{""}, - Resources: []string{"pods/log"}, - Verbs: []string{"get"}, - }, - { - APIGroups: []string{""}, - Resources: []string{"pods/attach"}, - Verbs: []string{"create", "get"}, - }, - { - APIGroups: []string{""}, - Resources: []string{"configmaps"}, - Verbs: []string{"get", "list", "watch"}, - }, -} // mcpContainerName is the name of the mcp container used in pod templates const mcpContainerName = "mcp" @@ -545,82 +512,6 @@ func (r *MCPServerReconciler) performImmediateRestart(ctx context.Context, mcpSe return nil } -// ensureRBACResource is a generic helper function to ensure a Kubernetes resource exists and is up to date -func (r *MCPServerReconciler) ensureRBACResource( - ctx context.Context, - mcpServer *mcpv1alpha1.MCPServer, - resourceType string, - createResource func() client.Object, -) error { - current := createResource() - objectKey := types.NamespacedName{Name: current.GetName(), Namespace: current.GetNamespace()} - err := r.Get(ctx, objectKey, current) - - if errors.IsNotFound(err) { - return r.createRBACResource(ctx, mcpServer, resourceType, createResource) - } else if err != nil { - return fmt.Errorf("failed to get %s: %w", resourceType, err) - } - - return r.updateRBACResourceIfNeeded(ctx, mcpServer, resourceType, createResource, current) -} - -// createRBACResource creates a new RBAC resource -func (r *MCPServerReconciler) createRBACResource( - ctx context.Context, - mcpServer *mcpv1alpha1.MCPServer, - resourceType string, - createResource func() client.Object, -) error { - ctxLogger := log.FromContext(ctx) - desired := createResource() - if err := controllerutil.SetControllerReference(mcpServer, desired, r.Scheme); err != nil { - ctxLogger.Error(err, "Failed to set controller reference", "resourceType", resourceType) - return nil - } - - ctxLogger.Info( - fmt.Sprintf("%s does not exist, creating %s", resourceType, resourceType), - fmt.Sprintf("%s.Name", resourceType), - desired.GetName(), - ) - if err := r.Create(ctx, desired); err != nil { - return fmt.Errorf("failed to create %s: %w", resourceType, err) - } - ctxLogger.Info(fmt.Sprintf("%s created", resourceType), fmt.Sprintf("%s.Name", resourceType), desired.GetName()) - return nil -} - -// updateRBACResourceIfNeeded updates an RBAC resource if changes are detected -func (r *MCPServerReconciler) updateRBACResourceIfNeeded( - ctx context.Context, - mcpServer *mcpv1alpha1.MCPServer, - resourceType string, - createResource func() client.Object, - current client.Object, -) error { - ctxLogger := log.FromContext(ctx) - desired := createResource() - if err := controllerutil.SetControllerReference(mcpServer, desired, r.Scheme); err != nil { - ctxLogger.Error(err, "Failed to set controller reference", "resourceType", resourceType) - return nil - } - - if !reflect.DeepEqual(current, desired) { - ctxLogger.Info( - fmt.Sprintf("%s exists, updating %s", resourceType, resourceType), - fmt.Sprintf("%s.Name", resourceType), - desired.GetName(), - ) - if err := r.Update(ctx, desired); err != nil { - return fmt.Errorf("failed to update %s: %w", resourceType, err) - } - ctxLogger.Info(fmt.Sprintf("%s updated", resourceType), fmt.Sprintf("%s.Name", resourceType), desired.GetName()) - } - return nil -} - -// ensureRBACResources ensures that the RBAC resources are in place for the MCP server // handleToolConfig handles MCPToolConfig reference for an MCPServer func (r *MCPServerReconciler) handleToolConfig(ctx context.Context, m *mcpv1alpha1.MCPServer) error { @@ -667,71 +558,16 @@ func (r *MCPServerReconciler) handleToolConfig(ctx context.Context, m *mcpv1alph return nil } func (r *MCPServerReconciler) ensureRBACResources(ctx context.Context, mcpServer *mcpv1alpha1.MCPServer) error { - proxyRunnerNameForRBAC := proxyRunnerServiceAccountName(mcpServer.Name) - - // Ensure Role - if err := r.ensureRBACResource(ctx, mcpServer, "Role", func() client.Object { - return &rbacv1.Role{ - ObjectMeta: metav1.ObjectMeta{ - Name: proxyRunnerNameForRBAC, - Namespace: mcpServer.Namespace, - }, - Rules: defaultRBACRules, - } - }); err != nil { - return err - } - - // Ensure ServiceAccount - if err := r.ensureRBACResource(ctx, mcpServer, "ServiceAccount", func() client.Object { - return &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{ - Name: proxyRunnerNameForRBAC, - Namespace: mcpServer.Namespace, - }, - } - }); err != nil { - return err - } - - if err := r.ensureRBACResource(ctx, mcpServer, "RoleBinding", func() client.Object { - return &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: proxyRunnerNameForRBAC, - Namespace: mcpServer.Namespace, - }, - RoleRef: rbacv1.RoleRef{ - APIGroup: "rbac.authorization.k8s.io", - Kind: "Role", - Name: proxyRunnerNameForRBAC, - }, - Subjects: []rbacv1.Subject{ - { - Kind: "ServiceAccount", - Name: proxyRunnerNameForRBAC, - Namespace: mcpServer.Namespace, - }, - }, - } - }); err != nil { - return err - } - - // If a service account is specified, we don't need to create one - if mcpServer.Spec.ServiceAccount != nil { - return nil + // Initialize RBACManager if not already initialized + if r.RBACManager == nil { + r.RBACManager = rbac.NewManager(rbac.Config{ + Client: r.Client, + Scheme: r.Scheme, + DefaultRBACRules: nil, // Use default rules from the package + }) } - // otherwise, create a service account for the MCP server - mcpServerServiceAccountName := mcpServerServiceAccountName(mcpServer.Name) - return r.ensureRBACResource(ctx, mcpServer, "ServiceAccount", func() client.Object { - return &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{ - Name: mcpServerServiceAccountName, - Namespace: mcpServer.Namespace, - }, - } - }) + return r.RBACManager.EnsureRBACResources(ctx, mcpServer) } // deploymentForMCPServer returns a MCPServer Deployment object @@ -755,7 +591,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp // If service account is not specified, use the default MCP server service account serviceAccount := m.Spec.ServiceAccount if serviceAccount == nil { - defaultSA := mcpServerServiceAccountName(m.Name) + defaultSA := r.mcpServerServiceAccountName(m.Name) serviceAccount = &defaultSA } finalPodTemplateSpec := NewMCPServerPodTemplateSpecBuilder(m.Spec.PodTemplateSpec). @@ -817,7 +653,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp // If service account is not specified, use the default MCP server service account serviceAccount := m.Spec.ServiceAccount if serviceAccount == nil { - defaultSA := mcpServerServiceAccountName(m.Name) + defaultSA := r.mcpServerServiceAccountName(m.Name) serviceAccount = &defaultSA } finalPodTemplateSpec := NewMCPServerPodTemplateSpecBuilder(m.Spec.PodTemplateSpec). @@ -1065,7 +901,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp Annotations: deploymentTemplateAnnotations, }, Spec: corev1.PodSpec{ - ServiceAccountName: proxyRunnerServiceAccountName(m.Name), + ServiceAccountName: r.proxyRunnerServiceAccountName(m.Name), Containers: []corev1.Container{{ Image: getToolhiveRunnerImage(), Name: "toolhive", @@ -1466,7 +1302,7 @@ func (r *MCPServerReconciler) deploymentNeedsUpdate( // If service account is not specified, use the default MCP server service account serviceAccount := mcpServer.Spec.ServiceAccount if serviceAccount == nil { - defaultSA := mcpServerServiceAccountName(mcpServer.Name) + defaultSA := r.mcpServerServiceAccountName(mcpServer.Name) serviceAccount = &defaultSA } expectedPodTemplateSpec := NewMCPServerPodTemplateSpecBuilder(mcpServer.Spec.PodTemplateSpec). @@ -1540,7 +1376,7 @@ func (r *MCPServerReconciler) deploymentNeedsUpdate( // Check if the service account name has changed // ServiceAccountName: treat empty (not yet set) as equal to the expected default - expectedServiceAccountName := proxyRunnerServiceAccountName(mcpServer.Name) + expectedServiceAccountName := r.proxyRunnerServiceAccountName(mcpServer.Name) currentServiceAccountName := deployment.Spec.Template.Spec.ServiceAccountName if currentServiceAccountName != "" && currentServiceAccountName != expectedServiceAccountName { return true @@ -1626,12 +1462,20 @@ func resourceRequirementsForMCPServer(m *mcpv1alpha1.MCPServer) corev1.ResourceR } // proxyRunnerServiceAccountName returns the service account name for the proxy runner -func proxyRunnerServiceAccountName(mcpServerName string) string { +func (r *MCPServerReconciler) proxyRunnerServiceAccountName(mcpServerName string) string { + if r.RBACManager != nil { + return r.RBACManager.GetProxyRunnerServiceAccountName(mcpServerName) + } + // Fallback for cases where RBACManager is not initialized yet return fmt.Sprintf("%s-proxy-runner", mcpServerName) } // mcpServerServiceAccountName returns the service account name for the mcp server -func mcpServerServiceAccountName(mcpServerName string) string { +func (r *MCPServerReconciler) mcpServerServiceAccountName(mcpServerName string) string { + if r.RBACManager != nil { + return r.RBACManager.GetMCPServerServiceAccountName(mcpServerName) + } + // Fallback for cases where RBACManager is not initialized yet return fmt.Sprintf("%s-sa", mcpServerName) } diff --git a/cmd/thv-operator/controllers/mcpserver_rbac_test.go b/cmd/thv-operator/controllers/mcpserver_rbac_test.go index d5ea53e78..6870bdf70 100644 --- a/cmd/thv-operator/controllers/mcpserver_rbac_test.go +++ b/cmd/thv-operator/controllers/mcpserver_rbac_test.go @@ -17,6 +17,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/rbac" ) type testContext struct { @@ -31,12 +32,19 @@ func setupTest(name, namespace string) *testContext { testScheme := createTestScheme() fakeClient := fake.NewClientBuilder().WithScheme(testScheme).Build() proxyRunnerNameForRBAC := fmt.Sprintf("%s-proxy-runner", name) + rbacManager := rbac.NewManager(rbac.Config{ + Client: fakeClient, + Scheme: testScheme, + DefaultRBACRules: nil, // Use default rules + }) + return &testContext{ mcpServer: mcpServer, client: fakeClient, reconciler: &MCPServerReconciler{ - Client: fakeClient, - Scheme: testScheme, + Client: fakeClient, + Scheme: testScheme, + RBACManager: rbacManager, }, proxyRunnerNameForRBAC: proxyRunnerNameForRBAC, } @@ -68,7 +76,7 @@ func (tc *testContext) assertRoleExists(t *testing.T) { require.NoError(t, err) assert.Equal(t, tc.proxyRunnerNameForRBAC, role.Name) assert.Equal(t, tc.mcpServer.Namespace, role.Namespace) - assert.Equal(t, defaultRBACRules, role.Rules) + assert.Equal(t, rbac.GetDefaultRBACRules(), role.Rules) } func (tc *testContext) assertRoleBindingExists(t *testing.T) { @@ -276,7 +284,7 @@ func TestEnsureRBACResources_NoChangesNeeded(t *testing.T) { Name: tc.proxyRunnerNameForRBAC, Namespace: tc.mcpServer.Namespace, }, - Rules: defaultRBACRules, + Rules: rbac.GetDefaultRBACRules(), } err = tc.client.Create(context.TODO(), role) require.NoError(t, err) diff --git a/cmd/thv-operator/main.go b/cmd/thv-operator/main.go index 7a15e63ae..14415a69e 100644 --- a/cmd/thv-operator/main.go +++ b/cmd/thv-operator/main.go @@ -22,6 +22,7 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/cmd/thv-operator/controllers" + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/rbac" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation" "github.com/stacklok/toolhive/pkg/logger" "github.com/stacklok/toolhive/pkg/operator/telemetry" @@ -75,10 +76,18 @@ func main() { os.Exit(1) } + // Initialize RBAC Manager + rbacManager := rbac.NewManager(rbac.Config{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + DefaultRBACRules: nil, // Use default rules from the package + }) + rec := &controllers.MCPServerReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), ImageValidation: validation.ImageValidationAlwaysAllow, + RBACManager: rbacManager, } if err = rec.SetupWithManager(mgr); err != nil { diff --git a/cmd/thv-operator/pkg/rbac/manager.go b/cmd/thv-operator/pkg/rbac/manager.go new file mode 100644 index 000000000..6f3230c42 --- /dev/null +++ b/cmd/thv-operator/pkg/rbac/manager.go @@ -0,0 +1,322 @@ +package rbac + +import ( + "context" + "fmt" + "reflect" + + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/log" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +// Manager defines the interface for managing RBAC resources for MCPServers +type Manager interface { + // EnsureRBACResources ensures all required RBAC resources exist for the MCPServer + EnsureRBACResources(ctx context.Context, mcpServer *mcpv1alpha1.MCPServer) error + + // GetProxyRunnerServiceAccountName returns the service account name for the proxy runner + GetProxyRunnerServiceAccountName(mcpServerName string) string + + // GetMCPServerServiceAccountName returns the service account name for the MCP server + GetMCPServerServiceAccountName(mcpServerName string) string +} + +// ResourceFactory is a function type for creating Kubernetes resources +type ResourceFactory func() client.Object + +// Config holds configuration for the RBAC manager +type Config struct { + // DefaultRBACRules defines the default RBAC rules for proxy runners and MCP servers + DefaultRBACRules []rbacv1.PolicyRule + + // Client is the Kubernetes client used for RBAC operations + Client client.Client + + // Scheme is the runtime scheme for setting owner references + Scheme *runtime.Scheme +} + +// GetDefaultRBACRules returns the default RBAC rules for ToolHive ProxyRunner and MCP servers +func GetDefaultRBACRules() []rbacv1.PolicyRule { + return []rbacv1.PolicyRule{ + { + APIGroups: []string{"apps"}, + Resources: []string{"statefulsets"}, + Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete", "apply"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"services"}, + Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete", "apply"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list", "watch"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"pods/log"}, + Verbs: []string{"get"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"pods/attach"}, + Verbs: []string{"create", "get"}, + }, + { + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + Verbs: []string{"get", "list", "watch"}, + }, + } +} + +// ResourceType represents the type of RBAC resource +type ResourceType string + +const ( + // ResourceTypeRole represents a Kubernetes Role + ResourceTypeRole ResourceType = "Role" + // ResourceTypeServiceAccount represents a Kubernetes ServiceAccount + ResourceTypeServiceAccount ResourceType = "ServiceAccount" + // ResourceTypeRoleBinding represents a Kubernetes RoleBinding + ResourceTypeRoleBinding ResourceType = "RoleBinding" +) + +// manager implements the Manager interface +type manager struct { + client client.Client + scheme *runtime.Scheme + defaultRBACRules []rbacv1.PolicyRule +} + +// NewManager creates a new RBAC manager +func NewManager(cfg Config) Manager { + rules := cfg.DefaultRBACRules + if rules == nil { + rules = GetDefaultRBACRules() + } + + return &manager{ + client: cfg.Client, + scheme: cfg.Scheme, + defaultRBACRules: rules, + } +} + +// EnsureRBACResources ensures all required RBAC resources exist for the MCPServer +func (m *manager) EnsureRBACResources(ctx context.Context, mcpServer *mcpv1alpha1.MCPServer) error { + proxyRunnerSAName := m.GetProxyRunnerServiceAccountName(mcpServer.Name) + + // Ensure Role for proxy runner + if err := m.ensureResource(ctx, mcpServer, ResourceTypeRole, func() client.Object { + return &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: proxyRunnerSAName, + Namespace: mcpServer.Namespace, + }, + Rules: m.defaultRBACRules, + } + }); err != nil { + return fmt.Errorf("failed to ensure proxy runner Role: %w", err) + } + + // Ensure ServiceAccount for proxy runner + if err := m.ensureResource(ctx, mcpServer, ResourceTypeServiceAccount, func() client.Object { + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: proxyRunnerSAName, + Namespace: mcpServer.Namespace, + }, + } + }); err != nil { + return fmt.Errorf("failed to ensure proxy runner ServiceAccount: %w", err) + } + + // Ensure RoleBinding for proxy runner + if err := m.ensureResource(ctx, mcpServer, ResourceTypeRoleBinding, func() client.Object { + return &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: proxyRunnerSAName, + Namespace: mcpServer.Namespace, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: proxyRunnerSAName, + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: proxyRunnerSAName, + Namespace: mcpServer.Namespace, + }, + }, + } + }); err != nil { + return fmt.Errorf("failed to ensure proxy runner RoleBinding: %w", err) + } + + // If a service account is specified in the spec, we don't need to create one + if mcpServer.Spec.ServiceAccount != nil { + return nil + } + + // Otherwise, create a service account for the MCP server itself + mcpServerSAName := m.GetMCPServerServiceAccountName(mcpServer.Name) + if err := m.ensureResource(ctx, mcpServer, ResourceTypeServiceAccount, func() client.Object { + return &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: mcpServerSAName, + Namespace: mcpServer.Namespace, + }, + } + }); err != nil { + return fmt.Errorf("failed to ensure MCP server ServiceAccount: %w", err) + } + + return nil +} + +// GetProxyRunnerServiceAccountName returns the service account name for the proxy runner +func (m *manager) GetProxyRunnerServiceAccountName(mcpServerName string) string { + return fmt.Sprintf("%s-proxy-runner", mcpServerName) +} + +// GetMCPServerServiceAccountName returns the service account name for the MCP server +func (m *manager) GetMCPServerServiceAccountName(mcpServerName string) string { + return fmt.Sprintf("%s-sa", mcpServerName) +} + +// ensureResource is a generic helper to ensure a Kubernetes resource exists and is up to date +func (m *manager) ensureResource( + ctx context.Context, + mcpServer *mcpv1alpha1.MCPServer, + resourceType ResourceType, + createResource ResourceFactory, +) error { + current := createResource() + objectKey := types.NamespacedName{Name: current.GetName(), Namespace: current.GetNamespace()} + err := m.client.Get(ctx, objectKey, current) + + if errors.IsNotFound(err) { + return m.createResource(ctx, mcpServer, resourceType, createResource) + } else if err != nil { + return fmt.Errorf("failed to get %s: %w", resourceType, err) + } + + return m.updateResourceIfNeeded(ctx, mcpServer, resourceType, createResource, current) +} + +// createResource creates a new RBAC resource +func (m *manager) createResource( + ctx context.Context, + mcpServer *mcpv1alpha1.MCPServer, + resourceType ResourceType, + createResource ResourceFactory, +) error { + ctxLogger := log.FromContext(ctx) + desired := createResource() + + if err := controllerutil.SetControllerReference(mcpServer, desired, m.scheme); err != nil { + ctxLogger.Error(err, "Failed to set controller reference", "resourceType", resourceType) + // Continue without owner reference + // This will result in orphaned resources if the MCP server is deleted + // the only way to cleanup is to delete the resources manually + // This is a known limitation of the operator + // TODO: Add cleanup logic to delete the resources based on labels. + // TODO: This can be applied to other resources as well which requires a more + // TODO: holistic approach to cleanup. + } + + ctxLogger.Info( + fmt.Sprintf("%s does not exist, creating", resourceType), + "resource.Name", desired.GetName(), + "resource.Namespace", desired.GetNamespace(), + ) + + if err := m.client.Create(ctx, desired); err != nil { + return fmt.Errorf("failed to create %s: %w", resourceType, err) + } + + ctxLogger.Info( + fmt.Sprintf("%s created", resourceType), + "resource.Name", desired.GetName(), + "resource.Namespace", desired.GetNamespace(), + ) + return nil +} + +// updateResourceIfNeeded updates an RBAC resource if changes are detected +func (m *manager) updateResourceIfNeeded( + ctx context.Context, + mcpServer *mcpv1alpha1.MCPServer, + resourceType ResourceType, + createResource ResourceFactory, + current client.Object, +) error { + ctxLogger := log.FromContext(ctx) + desired := createResource() + + if err := controllerutil.SetControllerReference(mcpServer, desired, m.scheme); err != nil { + ctxLogger.Error(err, "Failed to set controller reference", "resourceType", resourceType) + // Continue without owner reference + // This will result in orphaned resources if the MCP server is deleted + // the only way to cleanup is to delete the resources manually + // This is a known limitation of the operator + // TODO: Add cleanup logic to delete the resources based on labels. + // TODO: This can be applied to other resources as well which requires a more + // TODO: holistic approach to cleanup. + + } + + // Check if update is needed based on resource type + needsUpdate := false + switch resourceType { + case ResourceTypeRole: + currentRole := current.(*rbacv1.Role) + desiredRole := desired.(*rbacv1.Role) + needsUpdate = !reflect.DeepEqual(currentRole.Rules, desiredRole.Rules) + case ResourceTypeRoleBinding: + currentBinding := current.(*rbacv1.RoleBinding) + desiredBinding := desired.(*rbacv1.RoleBinding) + needsUpdate = !reflect.DeepEqual(currentBinding.RoleRef, desiredBinding.RoleRef) || + !reflect.DeepEqual(currentBinding.Subjects, desiredBinding.Subjects) + case ResourceTypeServiceAccount: + // ServiceAccounts typically don't need updates unless metadata changes + needsUpdate = false + } + + if needsUpdate { + // Preserve the ResourceVersion for update + desired.SetResourceVersion(current.GetResourceVersion()) + + ctxLogger.Info( + fmt.Sprintf("%s exists but needs update", resourceType), + "resource.Name", desired.GetName(), + "resource.Namespace", desired.GetNamespace(), + ) + + if err := m.client.Update(ctx, desired); err != nil { + return fmt.Errorf("failed to update %s: %w", resourceType, err) + } + + ctxLogger.Info( + fmt.Sprintf("%s updated", resourceType), + "resource.Name", desired.GetName(), + "resource.Namespace", desired.GetNamespace(), + ) + } + + return nil +} diff --git a/cmd/thv-operator/pkg/rbac/manager_errors_test.go b/cmd/thv-operator/pkg/rbac/manager_errors_test.go new file mode 100644 index 000000000..f0d78cdad --- /dev/null +++ b/cmd/thv-operator/pkg/rbac/manager_errors_test.go @@ -0,0 +1,454 @@ +package rbac + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +// mockClient is a mock implementation of client.Client for testing error scenarios +type mockClient struct { + client.Client + getError error + createError error + updateError error + deleteError error + // Control which resource type should fail + failResourceType string + // Track calls for verification + getCalls int + createCalls int + updateCalls int + deleteCalls int + // Custom function overrides + GetFunc func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error + CreateFunc func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error +} + +func (m *mockClient) Get(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + m.getCalls++ + + // Use custom function if provided + if m.GetFunc != nil { + return m.GetFunc(ctx, key, obj, opts...) + } + + // Return specific error if configured + if m.getError != nil { + // Check if we should fail for this specific resource type + if m.failResourceType == "" || m.shouldFailForResource(obj) { + return m.getError + } + } + + // Default to NotFound to trigger create flow + return apierrors.NewNotFound(schema.GroupResource{}, key.Name) +} + +func (m *mockClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + m.createCalls++ + + // Use custom function if provided + if m.CreateFunc != nil { + return m.CreateFunc(ctx, obj, opts...) + } + + if m.createError != nil { + if m.failResourceType == "" || m.shouldFailForResource(obj) { + return m.createError + } + } + return nil +} + +func (m *mockClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.updateCalls++ + + if m.updateError != nil { + if m.failResourceType == "" || m.shouldFailForResource(obj) { + return m.updateError + } + } + return nil +} + +func (m *mockClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + m.deleteCalls++ + + if m.deleteError != nil { + if m.failResourceType == "" || m.shouldFailForResource(obj) { + return m.deleteError + } + } + return nil +} + +func (m *mockClient) shouldFailForResource(obj client.Object) bool { + switch obj.(type) { + case *rbacv1.Role: + return m.failResourceType == "Role" + case *corev1.ServiceAccount: + return m.failResourceType == "ServiceAccount" + case *rbacv1.RoleBinding: + return m.failResourceType == "RoleBinding" + default: + return false + } +} + +// Test error during Get operation (non-NotFound error) +func TestManager_EnsureRBACResources_GetError(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + failResourceType string + getError error + expectedError string + }{ + { + name: "Role Get fails", + failResourceType: "Role", + getError: errors.New("network error"), + expectedError: "failed to get Role: network error", + }, + { + name: "ServiceAccount Get fails", + failResourceType: "ServiceAccount", + getError: errors.New("permission denied"), + expectedError: "failed to get ServiceAccount: permission denied", + }, + { + name: "RoleBinding Get fails", + failResourceType: "RoleBinding", + getError: errors.New("timeout"), + expectedError: "failed to get RoleBinding: timeout", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + mockClient := &mockClient{ + getError: tc.getError, + failResourceType: tc.failResourceType, + } + + manager := &manager{ + client: mockClient, + scheme: createTestScheme(), + defaultRBACRules: GetDefaultRBACRules(), + } + + mcpServer := createTestMCPServer("test-server", "default") + err := manager.EnsureRBACResources(context.TODO(), mcpServer) + + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedError) + assert.Greater(t, mockClient.getCalls, 0, "Get should have been called") + }) + } +} + +// Test error during Create operation +func TestManager_EnsureRBACResources_CreateError(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + failResourceType string + createError error + expectedError string + }{ + { + name: "Role Create fails", + failResourceType: "Role", + createError: errors.New("quota exceeded"), + expectedError: "failed to create Role: quota exceeded", + }, + { + name: "ServiceAccount Create fails", + failResourceType: "ServiceAccount", + createError: errors.New("invalid name"), + expectedError: "failed to create ServiceAccount: invalid name", + }, + { + name: "RoleBinding Create fails", + failResourceType: "RoleBinding", + createError: errors.New("conflict"), + expectedError: "failed to create RoleBinding: conflict", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + mockClient := &mockClient{ + createError: tc.createError, + failResourceType: tc.failResourceType, + } + + manager := &manager{ + client: mockClient, + scheme: createTestScheme(), + defaultRBACRules: GetDefaultRBACRules(), + } + + mcpServer := createTestMCPServer("test-server", "default") + err := manager.EnsureRBACResources(context.TODO(), mcpServer) + + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedError) + assert.Greater(t, mockClient.createCalls, 0, "Create should have been called") + }) + } +} + +// Test error during Update operation +func TestManager_EnsureRBACResources_UpdateError(t *testing.T) { + t.Parallel() + + // For update tests, we need a mock that returns existing resources + mockClientForUpdate := &mockClient{ + updateError: errors.New("update failed"), + } + + // Override Get to return an existing resource that needs updating + mockClientForUpdate.GetFunc = func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + // Populate the object to simulate it exists but needs updating + switch v := obj.(type) { + case *rbacv1.Role: + v.ObjectMeta = metav1.ObjectMeta{ + Name: key.Name, + Namespace: key.Namespace, + } + // Set different rules to trigger update + v.Rules = []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get"}, + }, + } + case *rbacv1.RoleBinding: + v.ObjectMeta = metav1.ObjectMeta{ + Name: key.Name, + Namespace: key.Namespace, + } + // Set different roleRef to trigger update + v.RoleRef = rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: "different-role", + } + case *corev1.ServiceAccount: + v.ObjectMeta = metav1.ObjectMeta{ + Name: key.Name, + Namespace: key.Namespace, + } + } + return nil + } + + manager := &manager{ + client: mockClientForUpdate, + scheme: createTestScheme(), + defaultRBACRules: GetDefaultRBACRules(), + } + + mcpServer := createTestMCPServer("test-server", "default") + err := manager.EnsureRBACResources(context.TODO(), mcpServer) + + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to update") + assert.Greater(t, mockClientForUpdate.updateCalls, 0, "Update should have been called") +} + + +// Test multiple errors during EnsureRBACResources +func TestManager_EnsureRBACResources_MultipleErrors(t *testing.T) { + t.Parallel() + + // First resource creation fails, should stop early + mockClient := &mockClient{ + createError: errors.New("first resource fails"), + failResourceType: "Role", // Fail on the first resource type + } + + manager := &manager{ + client: mockClient, + scheme: createTestScheme(), + defaultRBACRules: GetDefaultRBACRules(), + } + + mcpServer := createTestMCPServer("test-server", "default") + err := manager.EnsureRBACResources(context.TODO(), mcpServer) + + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to ensure proxy runner Role") + // Should stop after first failure + assert.Equal(t, 1, mockClient.createCalls, "Should stop after first create failure") +} + +// Test with custom service account (should skip MCP server SA creation) +func TestManager_EnsureRBACResources_CustomServiceAccount_Error(t *testing.T) { + t.Parallel() + + // Should only try to create 3 resources (Role, SA for proxy, RoleBinding) + // and not the 4th (MCP server SA) + callCount := 0 + mockClient := &mockClient{} + mockClient.CreateFunc = func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + callCount++ + if callCount == 3 { // Fail on the third resource (RoleBinding) + return errors.New("rolebinding creation failed") + } + return nil + } + mockClient.GetFunc = func(ctx context.Context, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + return apierrors.NewNotFound(schema.GroupResource{}, key.Name) + } + + manager := &manager{ + client: mockClient, + scheme: createTestScheme(), + defaultRBACRules: GetDefaultRBACRules(), + } + + mcpServer := createTestMCPServer("test-server", "default") + customSA := "custom-sa" + mcpServer.Spec.ServiceAccount = &customSA + + err := manager.EnsureRBACResources(context.TODO(), mcpServer) + + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to ensure proxy runner RoleBinding") + assert.Equal(t, 3, callCount, "Should only try to create 3 resources when custom SA is specified") +} + +// Test concurrent calls (simulate race conditions) +func TestManager_EnsureRBACResources_ConcurrentCalls(t *testing.T) { + t.Parallel() + + // This test ensures the manager handles concurrent calls gracefully + mockClient := &mockClient{ + // Simulate occasional conflicts + createError: apierrors.NewAlreadyExists(schema.GroupResource{}, "test"), + } + + manager := &manager{ + client: mockClient, + scheme: createTestScheme(), + defaultRBACRules: GetDefaultRBACRules(), + } + + mcpServer := createTestMCPServer("test-server", "default") + + // Run multiple goroutines trying to create the same resources + done := make(chan error, 5) + for i := 0; i < 5; i++ { + go func() { + done <- manager.EnsureRBACResources(context.TODO(), mcpServer) + }() + } + + // Collect results + var errors []error + for i := 0; i < 5; i++ { + if err := <-done; err != nil { + errors = append(errors, err) + } + } + + // All calls should have encountered the "already exists" error + assert.Len(t, errors, 5, "All concurrent calls should have failed with already exists") + for _, err := range errors { + assert.Contains(t, err.Error(), "failed") + } +} + +// Test with nil MCPServer - should panic +func TestManager_EnsureRBACResources_NilMCPServer(t *testing.T) { + t.Parallel() + + manager := NewManager(Config{ + Client: &mockClient{}, + Scheme: createTestScheme(), + }) + + // This should panic when trying to access mcpServer.Name + assert.Panics(t, func() { + _ = manager.EnsureRBACResources(context.TODO(), nil) + }, "Should panic when MCPServer is nil") +} + +// Test with empty MCPServer name - resources will have prefixes only +func TestManager_EnsureRBACResources_EmptyName(t *testing.T) { + t.Parallel() + + createCount := 0 + mockClient := &mockClient{} + mockClient.CreateFunc = func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + createCount++ + // Resources will have names like "-proxy-runner" and "-sa" which are valid + // Check that the names have the expected suffixes + name := obj.GetName() + assert.True(t, name == "-proxy-runner" || name == "-sa", + "Unexpected resource name: %s", name) + return nil + } + + manager := &manager{ + client: mockClient, + scheme: createTestScheme(), + defaultRBACRules: GetDefaultRBACRules(), + } + + mcpServer := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "", // Empty name + Namespace: "default", + }, + } + + err := manager.EnsureRBACResources(context.TODO(), mcpServer) + + // Should succeed with empty name - resources will have suffix-only names + assert.NoError(t, err) + assert.Equal(t, 4, createCount, "Should create all 4 resources even with empty name") +} + +// Test with invalid namespace +func TestManager_EnsureRBACResources_InvalidNamespace(t *testing.T) { + t.Parallel() + + mockClient := &mockClient{ + createError: errors.New("namespace does not exist"), + } + + manager := &manager{ + client: mockClient, + scheme: createTestScheme(), + defaultRBACRules: GetDefaultRBACRules(), + } + + mcpServer := createTestMCPServer("test-server", "non-existent-namespace") + err := manager.EnsureRBACResources(context.TODO(), mcpServer) + + require.Error(t, err) + assert.Contains(t, err.Error(), "namespace does not exist") +} \ No newline at end of file diff --git a/cmd/thv-operator/pkg/rbac/manager_test.go b/cmd/thv-operator/pkg/rbac/manager_test.go new file mode 100644 index 000000000..4fe4a7deb --- /dev/null +++ b/cmd/thv-operator/pkg/rbac/manager_test.go @@ -0,0 +1,367 @@ +package rbac + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +// testContext provides a test environment for RBAC manager tests +type testContext struct { + mcpServer *mcpv1alpha1.MCPServer + client client.Client + manager Manager + proxyRunnerNameForRBAC string + mcpServerSAName string +} + +// setupTest creates a new test context with all necessary components +func setupTest(name, namespace string) *testContext { + mcpServer := createTestMCPServer(name, namespace) + testScheme := createTestScheme() + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(mcpServer).Build() + + manager := NewManager(Config{ + Client: fakeClient, + Scheme: testScheme, + DefaultRBACRules: nil, // Use default rules + }) + + return &testContext{ + mcpServer: mcpServer, + client: fakeClient, + manager: manager, + proxyRunnerNameForRBAC: fmt.Sprintf("%s-proxy-runner", name), + mcpServerSAName: fmt.Sprintf("%s-sa", name), + } +} + +// Helper methods for assertions +func (tc *testContext) assertServiceAccountExists(t *testing.T, name string) { + t.Helper() + sa := &corev1.ServiceAccount{} + err := tc.client.Get(context.TODO(), types.NamespacedName{ + Name: name, + Namespace: tc.mcpServer.Namespace, + }, sa) + require.NoError(t, err) + assert.Equal(t, name, sa.Name) + assert.Equal(t, tc.mcpServer.Namespace, sa.Namespace) + + // Check owner reference + assert.Len(t, sa.OwnerReferences, 1) + assert.Equal(t, "MCPServer", sa.OwnerReferences[0].Kind) + assert.Equal(t, tc.mcpServer.Name, sa.OwnerReferences[0].Name) +} + +func (tc *testContext) assertRoleExists(t *testing.T) { + t.Helper() + role := &rbacv1.Role{} + err := tc.client.Get(context.TODO(), types.NamespacedName{ + Name: tc.proxyRunnerNameForRBAC, + Namespace: tc.mcpServer.Namespace, + }, role) + require.NoError(t, err) + assert.Equal(t, tc.proxyRunnerNameForRBAC, role.Name) + assert.Equal(t, tc.mcpServer.Namespace, role.Namespace) + assert.Equal(t, GetDefaultRBACRules(), role.Rules) + + // Check owner reference + assert.Len(t, role.OwnerReferences, 1) + assert.Equal(t, "MCPServer", role.OwnerReferences[0].Kind) + assert.Equal(t, tc.mcpServer.Name, role.OwnerReferences[0].Name) +} + +func (tc *testContext) assertRoleBindingExists(t *testing.T) { + t.Helper() + rb := &rbacv1.RoleBinding{} + err := tc.client.Get(context.TODO(), types.NamespacedName{ + Name: tc.proxyRunnerNameForRBAC, + Namespace: tc.mcpServer.Namespace, + }, rb) + require.NoError(t, err) + assert.Equal(t, tc.proxyRunnerNameForRBAC, rb.Name) + assert.Equal(t, tc.mcpServer.Namespace, rb.Namespace) + + expectedRoleRef := rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: tc.proxyRunnerNameForRBAC, + } + assert.Equal(t, expectedRoleRef, rb.RoleRef) + + expectedSubjects := []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: tc.proxyRunnerNameForRBAC, + Namespace: tc.mcpServer.Namespace, + }, + } + assert.Equal(t, expectedSubjects, rb.Subjects) + + // Check owner reference + assert.Len(t, rb.OwnerReferences, 1) + assert.Equal(t, "MCPServer", rb.OwnerReferences[0].Kind) + assert.Equal(t, tc.mcpServer.Name, rb.OwnerReferences[0].Name) +} + +func (tc *testContext) assertAllRBACResourcesExist(t *testing.T) { + t.Helper() + tc.assertServiceAccountExists(t, tc.proxyRunnerNameForRBAC) + tc.assertRoleExists(t) + tc.assertRoleBindingExists(t) + // Also check MCP server SA if not using custom SA + if tc.mcpServer.Spec.ServiceAccount == nil { + tc.assertServiceAccountExists(t, tc.mcpServerSAName) + } +} + +func (tc *testContext) assertServiceAccountNotExists(t *testing.T, name string) { + t.Helper() + sa := &corev1.ServiceAccount{} + err := tc.client.Get(context.TODO(), types.NamespacedName{ + Name: name, + Namespace: tc.mcpServer.Namespace, + }, sa) + assert.True(t, client.IgnoreNotFound(err) == nil) +} + +// Test functions +func TestManager_EnsureRBACResources_Creation(t *testing.T) { + t.Parallel() + tc := setupTest("test-server", "default") + + err := tc.manager.EnsureRBACResources(context.TODO(), tc.mcpServer) + require.NoError(t, err) + + tc.assertAllRBACResourcesExist(t) +} + +func TestManager_EnsureRBACResources_WithCustomServiceAccount(t *testing.T) { + t.Parallel() + tc := setupTest("test-server-custom-sa", "default") + customSA := "custom-service-account" + tc.mcpServer.Spec.ServiceAccount = &customSA + + err := tc.manager.EnsureRBACResources(context.TODO(), tc.mcpServer) + require.NoError(t, err) + + // Should create proxy runner resources but not MCP server SA + tc.assertServiceAccountExists(t, tc.proxyRunnerNameForRBAC) + tc.assertRoleExists(t) + tc.assertRoleBindingExists(t) + tc.assertServiceAccountNotExists(t, tc.mcpServerSAName) +} + +func TestManager_EnsureRBACResources_UpdateRole(t *testing.T) { + t.Parallel() + tc := setupTest("test-server-update", "default") + + // Create existing role with different rules + existingRole := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.proxyRunnerNameForRBAC, + Namespace: tc.mcpServer.Namespace, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get"}, + }, + }, + } + err := tc.client.Create(context.TODO(), existingRole) + require.NoError(t, err) + + err = tc.manager.EnsureRBACResources(context.TODO(), tc.mcpServer) + require.NoError(t, err) + + // Role should be updated with default rules + tc.assertRoleExists(t) +} + +func TestManager_EnsureRBACResources_UpdateRoleBinding(t *testing.T) { + t.Parallel() + tc := setupTest("test-server-rb-update", "default") + + // Create existing role binding with different subjects + existingRB := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: tc.proxyRunnerNameForRBAC, + Namespace: tc.mcpServer.Namespace, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: "different-role", + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: "different-sa", + Namespace: tc.mcpServer.Namespace, + }, + }, + } + err := tc.client.Create(context.TODO(), existingRB) + require.NoError(t, err) + + err = tc.manager.EnsureRBACResources(context.TODO(), tc.mcpServer) + require.NoError(t, err) + + // RoleBinding should be updated + tc.assertRoleBindingExists(t) +} + +func TestManager_EnsureRBACResources_Idempotency(t *testing.T) { + t.Parallel() + tc := setupTest("test-server-idempotent", "default") + + // Run multiple times + for i := 0; i < 3; i++ { + err := tc.manager.EnsureRBACResources(context.TODO(), tc.mcpServer) + require.NoError(t, err, "Iteration %d failed", i) + } + + tc.assertAllRBACResourcesExist(t) +} + +func TestManager_GetProxyRunnerServiceAccountName(t *testing.T) { + t.Parallel() + manager := NewManager(Config{}) + + testCases := []struct { + mcpServerName string + expected string + }{ + {"test-server", "test-server-proxy-runner"}, + {"mcp-server-123", "mcp-server-123-proxy-runner"}, + {"my-server", "my-server-proxy-runner"}, + } + + for _, tc := range testCases { + t.Run(tc.mcpServerName, func(t *testing.T) { + actual := manager.GetProxyRunnerServiceAccountName(tc.mcpServerName) + assert.Equal(t, tc.expected, actual) + }) + } +} + +func TestManager_GetMCPServerServiceAccountName(t *testing.T) { + t.Parallel() + manager := NewManager(Config{}) + + testCases := []struct { + mcpServerName string + expected string + }{ + {"test-server", "test-server-sa"}, + {"mcp-server-123", "mcp-server-123-sa"}, + {"my-server", "my-server-sa"}, + } + + for _, tc := range testCases { + t.Run(tc.mcpServerName, func(t *testing.T) { + actual := manager.GetMCPServerServiceAccountName(tc.mcpServerName) + assert.Equal(t, tc.expected, actual) + }) + } +} + +func TestManager_CustomRBACRules(t *testing.T) { + t.Parallel() + customRules := []rbacv1.PolicyRule{ + { + APIGroups: []string{"custom.io"}, + Resources: []string{"customresources"}, + Verbs: []string{"get", "list"}, + }, + } + + mcpServer := createTestMCPServer("test-server", "default") + testScheme := createTestScheme() + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(mcpServer).Build() + + manager := NewManager(Config{ + Client: fakeClient, + Scheme: testScheme, + DefaultRBACRules: customRules, + }) + + err := manager.EnsureRBACResources(context.TODO(), mcpServer) + require.NoError(t, err) + + // Check that custom rules were applied + role := &rbacv1.Role{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{ + Name: "test-server-proxy-runner", + Namespace: "default", + }, role) + require.NoError(t, err) + assert.Equal(t, customRules, role.Rules) +} + +func TestManager_MultipleNamespaces(t *testing.T) { + t.Parallel() + testCases := []struct { + name string + namespace string + }{ + {"server1", "namespace1"}, + {"server2", "namespace2"}, + {"server3", "default"}, + } + + for _, testCase := range testCases { + t.Run(testCase.name+"-"+testCase.namespace, func(t *testing.T) { + t.Parallel() + tc := setupTest(testCase.name, testCase.namespace) + + err := tc.manager.EnsureRBACResources(context.TODO(), tc.mcpServer) + require.NoError(t, err) + + tc.assertAllRBACResourcesExist(t) + }) + } +} + +// Helper functions +func createTestMCPServer(name, namespace string) *mcpv1alpha1.MCPServer { + return &mcpv1alpha1.MCPServer{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "toolhive.stacklok.dev/v1alpha1", + Kind: "MCPServer", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + UID: types.UID("test-uid-" + name), + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Image: "test-image:latest", + Transport: "stdio", + Port: 8080, + }, + } +} + +func createTestScheme() *runtime.Scheme { + s := runtime.NewScheme() + _ = scheme.AddToScheme(s) + _ = mcpv1alpha1.AddToScheme(s) + return s +}