From a0393a25adfae2c4bb74c4307ae3f78f67bcf248 Mon Sep 17 00:00:00 2001 From: Allison Durham Date: Wed, 14 May 2025 14:36:57 -0700 Subject: [PATCH 1/3] add support for namespace creation --- acp/config/rbac/kustomization.yaml | 2 + .../rbac/namespace_permissions_binding.yaml | 13 + .../rbac/namespace_permissions_patch.yaml | 14 ++ acp/internal/server/server.go | 48 ++++ acp/internal/server/server_test.go | 222 ++++++++++++++++++ 5 files changed, 299 insertions(+) create mode 100644 acp/config/rbac/namespace_permissions_binding.yaml create mode 100644 acp/config/rbac/namespace_permissions_patch.yaml diff --git a/acp/config/rbac/kustomization.yaml b/acp/config/rbac/kustomization.yaml index 8188fb90..c7dff454 100644 --- a/acp/config/rbac/kustomization.yaml +++ b/acp/config/rbac/kustomization.yaml @@ -9,6 +9,8 @@ resources: - role_binding.yaml - leader_election_role.yaml - leader_election_role_binding.yaml +- namespace_permissions_patch.yaml +- namespace_permissions_binding.yaml # The following RBAC configurations are used to protect # the metrics endpoint with authn/authz. These configurations # ensure that only authorized users and service accounts diff --git a/acp/config/rbac/namespace_permissions_binding.yaml b/acp/config/rbac/namespace_permissions_binding.yaml new file mode 100644 index 00000000..84a25661 --- /dev/null +++ b/acp/config/rbac/namespace_permissions_binding.yaml @@ -0,0 +1,13 @@ +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: namespace-manager-rolebinding +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: namespace-manager-role +subjects: +- kind: ServiceAccount + name: controller-manager + namespace: system \ No newline at end of file diff --git a/acp/config/rbac/namespace_permissions_patch.yaml b/acp/config/rbac/namespace_permissions_patch.yaml new file mode 100644 index 00000000..be3b7372 --- /dev/null +++ b/acp/config/rbac/namespace_permissions_patch.yaml @@ -0,0 +1,14 @@ +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: namespace-manager-role +rules: +- apiGroups: + - "" + resources: + - namespaces + verbs: + - get + - list + - create \ No newline at end of file diff --git a/acp/internal/server/server.go b/acp/internal/server/server.go index 833317eb..ff27650e 100644 --- a/acp/internal/server/server.go +++ b/acp/internal/server/server.go @@ -215,6 +215,13 @@ func (s *APIServer) createAgent(c *gin.Context) { // Default namespace to "default" if not provided namespace := defaultIfEmpty(req.Namespace, "default") + // Ensure the namespace exists + if err := s.ensureNamespaceExists(ctx, namespace); err != nil { + logger.Error(err, "Failed to ensure namespace exists") + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to ensure namespace exists: " + err.Error()}) + return + } + // Check if agent already exists exists, err := s.resourceExists(ctx, &acp.Agent{}, namespace, req.Name) if err != nil { @@ -626,6 +633,40 @@ func defaultIfEmpty(val, defaultVal string) string { return val } +// ensureNamespaceExists checks if a namespace exists and creates it if it doesn't +func (s *APIServer) ensureNamespaceExists(ctx context.Context, namespaceName string) error { + logger := log.FromContext(ctx) + + // Check if namespace exists + var namespace corev1.Namespace + err := s.client.Get(ctx, client.ObjectKey{Name: namespaceName}, &namespace) + if err == nil { + // Namespace exists, nothing to do + return nil + } + + if !apierrors.IsNotFound(err) { + // Error other than "not found" occurred + logger.Error(err, "Failed to check namespace existence", "namespace", namespaceName) + return fmt.Errorf("failed to check namespace existence: %w", err) + } + + // Namespace doesn't exist, create it + namespace = corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespaceName, + }, + } + + if err := s.client.Create(ctx, &namespace); err != nil { + logger.Error(err, "Failed to create namespace", "namespace", namespaceName) + return fmt.Errorf("failed to create namespace: %w", err) + } + + logger.Info("Created namespace", "namespace", namespaceName) + return nil +} + // updateAgent handles updating an existing agent and its associated MCP servers func (s *APIServer) updateAgent(c *gin.Context) { ctx := c.Request.Context() @@ -888,6 +929,13 @@ func (s *APIServer) createTask(c *gin.Context) { namespace = "default" } + // Ensure the namespace exists + if err := s.ensureNamespaceExists(ctx, namespace); err != nil { + logger.Error(err, "Failed to ensure namespace exists") + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to ensure namespace exists: " + err.Error()}) + return + } + // Handle both responseURL and responseUrl fields (with responseURL taking precedence) responseURL := req.ResponseURL if responseURL == "" && req.ResponseUrl != "" { diff --git a/acp/internal/server/server_test.go b/acp/internal/server/server_test.go index d4c7c20b..ff56363a 100644 --- a/acp/internal/server/server_test.go +++ b/acp/internal/server/server_test.go @@ -52,6 +52,57 @@ var _ = Describe("API Server", func() { router = server.router recorder = httptest.NewRecorder() }) + + Describe("ensureNamespaceExists", func() { + It("should do nothing if namespace already exists", func() { + // Create a namespace + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "existing-namespace", + }, + } + Expect(k8sClient.Create(ctx, namespace)).To(Succeed()) + + // Call ensureNamespaceExists + err := server.ensureNamespaceExists(ctx, "existing-namespace") + Expect(err).NotTo(HaveOccurred()) + + // Verify no new namespace was created (count should still be 1) + var namespaceList corev1.NamespaceList + Expect(k8sClient.List(ctx, &namespaceList)).To(Succeed()) + count := 0 + for _, ns := range namespaceList.Items { + if ns.Name == "existing-namespace" { + count++ + } + } + Expect(count).To(Equal(1)) + }) + + It("should create a namespace if it doesn't exist", func() { + // Call ensureNamespaceExists for a new namespace + err := server.ensureNamespaceExists(ctx, "new-namespace") + Expect(err).NotTo(HaveOccurred()) + + // Verify namespace was created + var namespace corev1.Namespace + err = k8sClient.Get(ctx, client.ObjectKey{Name: "new-namespace"}, &namespace) + Expect(err).NotTo(HaveOccurred()) + Expect(namespace.Name).To(Equal("new-namespace")) + }) + + It("should handle Kubernetes API errors", func() { + // Create a client that returns an error for Get operations + errorClient := &errorK8sClient{Client: k8sClient} + errorClient.returnErrorOnGetNamespace = true + errorServer := NewAPIServer(errorClient, ":8080") + + // Call ensureNamespaceExists + err := errorServer.ensureNamespaceExists(ctx, "error-namespace") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to check namespace existence")) + }) + }) Describe("POST /v1/tasks", func() { It("should create a new task with valid input", func() { @@ -323,6 +374,7 @@ var _ = Describe("API Server", func() { // Custom client that forces an error on Create type errorK8sClient struct { client.Client + returnErrorOnGetNamespace bool } func (e *errorK8sClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { @@ -337,10 +389,33 @@ func (e *errorK8sClient) Create(ctx context.Context, obj client.Object, opts ... }, } } + // Return an error for Namespace objects if specified + if _, ok := obj.(*corev1.Namespace); ok && e.returnErrorOnGetNamespace { + return &errors.StatusError{ + ErrStatus: metav1.Status{ + Status: "Failure", + Message: "Simulated internal server error", + Reason: "InternalError", + Code: http.StatusInternalServerError, + }, + } + } return e.Client.Create(ctx, obj, opts...) } func (e *errorK8sClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if e.returnErrorOnGetNamespace && key.Name != "" { + if _, ok := obj.(*corev1.Namespace); ok { + return &errors.StatusError{ + ErrStatus: metav1.Status{ + Status: "Failure", + Message: "Simulated get namespace error", + Reason: "InternalError", + Code: http.StatusInternalServerError, + }, + } + } + } return e.Client.Get(ctx, key, obj, opts...) } @@ -1179,3 +1254,150 @@ var _ = Describe("Agent API", func() { }) }) }) + +var _ = Describe("Namespace Auto-Creation", func() { + var ( + ctx context.Context + k8sClient client.Client + server *APIServer + router *gin.Engine + recorder *httptest.ResponseRecorder + ) + + BeforeEach(func() { + ctx = context.Background() + // Create a scheme with our API types registered + scheme := runtime.NewScheme() + Expect(corev1.AddToScheme(scheme)).To(Succeed()) + Expect(acp.AddToScheme(scheme)).To(Succeed()) + + // Create a fake client + k8sClient = fake.NewClientBuilder().WithScheme(scheme).Build() + + // Create the API server with the client + server = NewAPIServer(k8sClient, ":8080") + router = server.router + recorder = httptest.NewRecorder() + }) + + It("should automatically create a non-existent namespace for agent creation", func() { + // Create an LLM in the namespace we'll use + llm := &acp.LLM{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auto-ns-llm", + Namespace: "auto-created-namespace", + }, + Spec: acp.LLMSpec{ + Provider: "test-provider", + Parameters: acp.BaseConfig{ + Model: "test-model", + }, + }, + } + Expect(k8sClient.Create(ctx, llm)).To(Succeed()) + + // Create agent in a namespace that doesn't exist yet + reqBody := CreateAgentRequest{ + Namespace: "auto-created-namespace", + Name: "auto-ns-agent", + LLM: "auto-ns-llm", + SystemPrompt: "Testing auto namespace creation", + } + jsonBody, err := json.Marshal(reqBody) + Expect(err).NotTo(HaveOccurred()) + + req := httptest.NewRequest(http.MethodPost, "/v1/agents", bytes.NewBuffer(jsonBody)) + req.Header.Set("Content-Type", "application/json") + recorder = httptest.NewRecorder() + router.ServeHTTP(recorder, req) + + // Verify the agent was created successfully + Expect(recorder.Code).To(Equal(http.StatusCreated)) + + // Verify the namespace exists + var namespace corev1.Namespace + err = k8sClient.Get(ctx, client.ObjectKey{Name: "auto-created-namespace"}, &namespace) + Expect(err).NotTo(HaveOccurred()) + Expect(namespace.Name).To(Equal("auto-created-namespace")) + + // Verify the agent was created in the namespace + var agent acp.Agent + err = k8sClient.Get(ctx, client.ObjectKey{ + Namespace: "auto-created-namespace", + Name: "auto-ns-agent", + }, &agent) + Expect(err).NotTo(HaveOccurred()) + Expect(agent.Namespace).To(Equal("auto-created-namespace")) + }) + + It("should automatically create a non-existent namespace for task creation", func() { + // Create an agent first + agent := &acp.Agent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "task-test-agent", + Namespace: "task-namespace", + }, + Spec: acp.AgentSpec{}, + } + + // Create the task namespace + taskNs := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "task-namespace", + }, + } + Expect(k8sClient.Create(ctx, taskNs)).To(Succeed()) + Expect(k8sClient.Create(ctx, agent)).To(Succeed()) + + // Create a task in a namespace that doesn't exist yet + reqBody := CreateTaskRequest{ + Namespace: "task-namespace", + AgentName: "task-test-agent", + UserMessage: "Testing auto namespace creation for tasks", + } + jsonBody, err := json.Marshal(reqBody) + Expect(err).NotTo(HaveOccurred()) + + req := httptest.NewRequest(http.MethodPost, "/v1/tasks", bytes.NewBuffer(jsonBody)) + req.Header.Set("Content-Type", "application/json") + recorder = httptest.NewRecorder() + router.ServeHTTP(recorder, req) + + // Verify the task was created successfully + Expect(recorder.Code).To(Equal(http.StatusCreated)) + + // Confirm a task was created + var taskList acp.TaskList + Expect(k8sClient.List(ctx, &taskList, client.InNamespace("task-namespace"))).To(Succeed()) + Expect(len(taskList.Items)).To(Equal(1)) + }) + + It("should handle namespace creation errors", func() { + // Create an error client that fails on namespace operations + errorClient := &errorK8sClient{Client: k8sClient, returnErrorOnGetNamespace: true} + errorServer := NewAPIServer(errorClient, ":8080") + errorRouter := errorServer.router + + // Create the request body + reqBody := CreateAgentRequest{ + Namespace: "error-namespace", + Name: "error-ns-agent", + LLM: "test-llm", + SystemPrompt: "Testing namespace error handling", + } + jsonBody, err := json.Marshal(reqBody) + Expect(err).NotTo(HaveOccurred()) + + req := httptest.NewRequest(http.MethodPost, "/v1/agents", bytes.NewBuffer(jsonBody)) + req.Header.Set("Content-Type", "application/json") + recorder = httptest.NewRecorder() + errorRouter.ServeHTTP(recorder, req) + + // Verify we get an internal server error + Expect(recorder.Code).To(Equal(http.StatusInternalServerError)) + var errorResponse map[string]string + err = json.Unmarshal(recorder.Body.Bytes(), &errorResponse) + Expect(err).NotTo(HaveOccurred()) + Expect(errorResponse["error"]).To(ContainSubstring("Failed to ensure namespace exists")) + }) +}) From ca1cb8af7c6e2ea3fbfac588132bbf6948c71aa9 Mon Sep 17 00:00:00 2001 From: Sundeep Malladi Date: Wed, 14 May 2025 14:42:17 -0700 Subject: [PATCH 2/3] make fmt --- acp/internal/server/server.go | 10 +++---- acp/internal/server/server_test.go | 44 +++++++++++++++--------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/acp/internal/server/server.go b/acp/internal/server/server.go index ff27650e..e278827c 100644 --- a/acp/internal/server/server.go +++ b/acp/internal/server/server.go @@ -636,7 +636,7 @@ func defaultIfEmpty(val, defaultVal string) string { // ensureNamespaceExists checks if a namespace exists and creates it if it doesn't func (s *APIServer) ensureNamespaceExists(ctx context.Context, namespaceName string) error { logger := log.FromContext(ctx) - + // Check if namespace exists var namespace corev1.Namespace err := s.client.Get(ctx, client.ObjectKey{Name: namespaceName}, &namespace) @@ -644,25 +644,25 @@ func (s *APIServer) ensureNamespaceExists(ctx context.Context, namespaceName str // Namespace exists, nothing to do return nil } - + if !apierrors.IsNotFound(err) { // Error other than "not found" occurred logger.Error(err, "Failed to check namespace existence", "namespace", namespaceName) return fmt.Errorf("failed to check namespace existence: %w", err) } - + // Namespace doesn't exist, create it namespace = corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: namespaceName, }, } - + if err := s.client.Create(ctx, &namespace); err != nil { logger.Error(err, "Failed to create namespace", "namespace", namespaceName) return fmt.Errorf("failed to create namespace: %w", err) } - + logger.Info("Created namespace", "namespace", namespaceName) return nil } diff --git a/acp/internal/server/server_test.go b/acp/internal/server/server_test.go index ff56363a..cd5b4804 100644 --- a/acp/internal/server/server_test.go +++ b/acp/internal/server/server_test.go @@ -52,7 +52,7 @@ var _ = Describe("API Server", func() { router = server.router recorder = httptest.NewRecorder() }) - + Describe("ensureNamespaceExists", func() { It("should do nothing if namespace already exists", func() { // Create a namespace @@ -62,11 +62,11 @@ var _ = Describe("API Server", func() { }, } Expect(k8sClient.Create(ctx, namespace)).To(Succeed()) - + // Call ensureNamespaceExists err := server.ensureNamespaceExists(ctx, "existing-namespace") Expect(err).NotTo(HaveOccurred()) - + // Verify no new namespace was created (count should still be 1) var namespaceList corev1.NamespaceList Expect(k8sClient.List(ctx, &namespaceList)).To(Succeed()) @@ -78,25 +78,25 @@ var _ = Describe("API Server", func() { } Expect(count).To(Equal(1)) }) - + It("should create a namespace if it doesn't exist", func() { // Call ensureNamespaceExists for a new namespace err := server.ensureNamespaceExists(ctx, "new-namespace") Expect(err).NotTo(HaveOccurred()) - + // Verify namespace was created var namespace corev1.Namespace err = k8sClient.Get(ctx, client.ObjectKey{Name: "new-namespace"}, &namespace) Expect(err).NotTo(HaveOccurred()) Expect(namespace.Name).To(Equal("new-namespace")) }) - + It("should handle Kubernetes API errors", func() { // Create a client that returns an error for Get operations errorClient := &errorK8sClient{Client: k8sClient} errorClient.returnErrorOnGetNamespace = true errorServer := NewAPIServer(errorClient, ":8080") - + // Call ensureNamespaceExists err := errorServer.ensureNamespaceExists(ctx, "error-namespace") Expect(err).To(HaveOccurred()) @@ -1295,7 +1295,7 @@ var _ = Describe("Namespace Auto-Creation", func() { }, } Expect(k8sClient.Create(ctx, llm)).To(Succeed()) - + // Create agent in a namespace that doesn't exist yet reqBody := CreateAgentRequest{ Namespace: "auto-created-namespace", @@ -1305,21 +1305,21 @@ var _ = Describe("Namespace Auto-Creation", func() { } jsonBody, err := json.Marshal(reqBody) Expect(err).NotTo(HaveOccurred()) - + req := httptest.NewRequest(http.MethodPost, "/v1/agents", bytes.NewBuffer(jsonBody)) req.Header.Set("Content-Type", "application/json") recorder = httptest.NewRecorder() router.ServeHTTP(recorder, req) - + // Verify the agent was created successfully Expect(recorder.Code).To(Equal(http.StatusCreated)) - + // Verify the namespace exists var namespace corev1.Namespace err = k8sClient.Get(ctx, client.ObjectKey{Name: "auto-created-namespace"}, &namespace) Expect(err).NotTo(HaveOccurred()) Expect(namespace.Name).To(Equal("auto-created-namespace")) - + // Verify the agent was created in the namespace var agent acp.Agent err = k8sClient.Get(ctx, client.ObjectKey{ @@ -1329,7 +1329,7 @@ var _ = Describe("Namespace Auto-Creation", func() { Expect(err).NotTo(HaveOccurred()) Expect(agent.Namespace).To(Equal("auto-created-namespace")) }) - + It("should automatically create a non-existent namespace for task creation", func() { // Create an agent first agent := &acp.Agent{ @@ -1339,7 +1339,7 @@ var _ = Describe("Namespace Auto-Creation", func() { }, Spec: acp.AgentSpec{}, } - + // Create the task namespace taskNs := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -1348,7 +1348,7 @@ var _ = Describe("Namespace Auto-Creation", func() { } Expect(k8sClient.Create(ctx, taskNs)).To(Succeed()) Expect(k8sClient.Create(ctx, agent)).To(Succeed()) - + // Create a task in a namespace that doesn't exist yet reqBody := CreateTaskRequest{ Namespace: "task-namespace", @@ -1357,27 +1357,27 @@ var _ = Describe("Namespace Auto-Creation", func() { } jsonBody, err := json.Marshal(reqBody) Expect(err).NotTo(HaveOccurred()) - + req := httptest.NewRequest(http.MethodPost, "/v1/tasks", bytes.NewBuffer(jsonBody)) req.Header.Set("Content-Type", "application/json") recorder = httptest.NewRecorder() router.ServeHTTP(recorder, req) - + // Verify the task was created successfully Expect(recorder.Code).To(Equal(http.StatusCreated)) - + // Confirm a task was created var taskList acp.TaskList Expect(k8sClient.List(ctx, &taskList, client.InNamespace("task-namespace"))).To(Succeed()) Expect(len(taskList.Items)).To(Equal(1)) }) - + It("should handle namespace creation errors", func() { // Create an error client that fails on namespace operations errorClient := &errorK8sClient{Client: k8sClient, returnErrorOnGetNamespace: true} errorServer := NewAPIServer(errorClient, ":8080") errorRouter := errorServer.router - + // Create the request body reqBody := CreateAgentRequest{ Namespace: "error-namespace", @@ -1387,12 +1387,12 @@ var _ = Describe("Namespace Auto-Creation", func() { } jsonBody, err := json.Marshal(reqBody) Expect(err).NotTo(HaveOccurred()) - + req := httptest.NewRequest(http.MethodPost, "/v1/agents", bytes.NewBuffer(jsonBody)) req.Header.Set("Content-Type", "application/json") recorder = httptest.NewRecorder() errorRouter.ServeHTTP(recorder, req) - + // Verify we get an internal server error Expect(recorder.Code).To(Equal(http.StatusInternalServerError)) var errorResponse map[string]string From 7150defc686466c078c0c3179dc0fc7f4c365ac7 Mon Sep 17 00:00:00 2001 From: Sundeep Malladi Date: Wed, 14 May 2025 14:49:17 -0700 Subject: [PATCH 3/3] some lint fixes (but the cycomatic complexity one) --- acp/internal/server/server.go | 13 +++++++++---- acp/internal/server/server_test.go | 4 ++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/acp/internal/server/server.go b/acp/internal/server/server.go index e278827c..cdcda394 100644 --- a/acp/internal/server/server.go +++ b/acp/internal/server/server.go @@ -21,6 +21,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" ) +const ( + transportTypeStdio = "stdio" + transportTypeHTTP = "http" +) + // CreateTaskRequest defines the structure of the request body for creating a task type CreateTaskRequest struct { Namespace string `json:"namespace,omitempty"` // Optional, defaults to "default" @@ -492,19 +497,19 @@ func validateMCPConfig(config MCPServerConfig) error { // Default to stdio transport if not specified transport := config.Transport if transport == "" { - transport = "stdio" + transport = transportTypeStdio } // Validate the transport type - if transport != "stdio" && transport != "http" { + if transport != transportTypeStdio && transport != transportTypeHTTP { return fmt.Errorf("invalid transport: %s", transport) } // Validate transport-specific requirements - if transport == "stdio" && (config.Command == "" || len(config.Args) == 0) { + if transport == transportTypeStdio && (config.Command == "" || len(config.Args) == 0) { return fmt.Errorf("command and args required for stdio transport") } - if transport == "http" && config.URL == "" { + if transport == transportTypeHTTP && config.URL == "" { return fmt.Errorf("url required for http transport") } diff --git a/acp/internal/server/server_test.go b/acp/internal/server/server_test.go index cd5b4804..1dc2c386 100644 --- a/acp/internal/server/server_test.go +++ b/acp/internal/server/server_test.go @@ -1065,7 +1065,7 @@ var _ = Describe("Agent API", func() { }, &updatedAgent)).To(Succeed()) Expect(updatedAgent.Spec.System).To(Equal("Updated prompt")) - Expect(len(updatedAgent.Spec.MCPServers)).To(Equal(2)) + Expect(updatedAgent.Spec.MCPServers).To(HaveLen(2)) // Extract MCP server names from the updated agent mcpServerNames := []string{} @@ -1369,7 +1369,7 @@ var _ = Describe("Namespace Auto-Creation", func() { // Confirm a task was created var taskList acp.TaskList Expect(k8sClient.List(ctx, &taskList, client.InNamespace("task-namespace"))).To(Succeed()) - Expect(len(taskList.Items)).To(Equal(1)) + Expect(taskList.Items).To(HaveLen(1)) }) It("should handle namespace creation errors", func() {