Add implementation for llm artifact deployment in gateway#1093
Add implementation for llm artifact deployment in gateway#1093
Conversation
WalkthroughControl plane client extended to support LLM provider/proxy lifecycle: added new event types, WebSocket handlers, API utilities to fetch/deploy definitions, and constructor wiring for an LLM deployment service plus lazy-resource and template dependencies. Changes
Sequence DiagramsequenceDiagram
participant WS as "WebSocket Event Source"
participant Client as "Gateway Client"
participant APIUtils as "API Utils Service"
participant LLM as "LLM Deployment Service"
participant CP as "Control Plane API"
WS->>Client: LLMProviderDeployed event (providerID, correlationID)
activate Client
Client->>APIUtils: FetchLLMProviderDefinition(providerID)
activate APIUtils
APIUtils->>CP: GET /llm-providers/{providerID}
CP-->>APIUtils: ZIP bytes
APIUtils-->>Client: ZIP bytes
deactivate APIUtils
Client->>APIUtils: Extract YAML from ZIP
activate APIUtils
APIUtils-->>Client: YAML bytes
deactivate APIUtils
Client->>APIUtils: CreateLLMProviderFromYAML(yaml, providerID, correlationID, LLM)
activate APIUtils
APIUtils->>LLM: DeployLLMProviderConfiguration(...)
LLM-->>APIUtils: APIDeploymentResult
APIUtils-->>Client: APIDeploymentResult
deactivate APIUtils
Client->>Client: Log result / emit status
deactivate Client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
gateway/gateway-controller/pkg/utils/api_utils.go (2)
122-234: Extract a commonfetchDefinitionZiphelper to eliminate duplication across all three Fetch methods.
FetchLLMProviderDefinition,FetchLLMProxyDefinition, and the existingFetchAPIDefinitionshare identical logic — only the URL path segment and log entity name differ. This is classic copy-paste duplication.Consider something like:
♻️ Sketch of a shared helper
+// fetchDefinitionZip downloads a zip definition from the given resource path. +func (s *APIUtilsService) fetchDefinitionZip(resourcePath, entityType, entityID string) ([]byte, error) { + url := s.config.BaseURL + resourcePath + s.logger.Info("Fetching "+entityType+" definition", + slog.String("id", entityID), + slog.String("url", url), + ) + + client := s.newHTTPClient() + + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + req.Header.Add("api-key", s.config.Token) + req.Header.Add("Accept", "application/zip") + + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("failed to fetch %s definition: %w", entityType, err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + return nil, fmt.Errorf("%s request failed with status %d: %s", entityType, resp.StatusCode, string(bodyBytes)) + } + + bodyBytes, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + + s.logger.Info("Successfully fetched "+entityType+" definition", + slog.String("id", entityID), + slog.Int("size_bytes", len(bodyBytes)), + ) + return bodyBytes, nil +}Then each public method becomes a one-liner:
func (s *APIUtilsService) FetchLLMProviderDefinition(providerID string) ([]byte, error) { return s.fetchDefinitionZip("/llm-providers/"+providerID, "LLM provider", providerID) }
132-140: MissingMinVersionin TLS config (flagged by static analysis).The new
tls.Configblocks omitMinVersion, defaulting to TLS 1.0 when acting as a server and TLS 1.2 as a client. While the client default is generally acceptable, explicitly settingMinVersion: tls.VersionTLS12(ortls.VersionTLS13) is a defense-in-depth best practice and silences the static analysis warning. Note that the same issue exists in the pre-existingFetchAPIDefinition(line 79-81) andNotifyAPIDeployment— this would be a good time to fix all of them together.🔒 Proposed fix (apply to all tls.Config instances)
TLSClientConfig: &tls.Config{ InsecureSkipVerify: s.config.InsecureSkipVerify, + MinVersion: tls.VersionTLS12, },Also applies to: 189-197
gateway/gateway-controller/pkg/controlplane/client.go (1)
824-1042: Consider extracting common logic from the four LLM event handlers.The deployed handlers (
handleLLMProviderDeployedEventandhandleLLMProxyDeployedEvent) share an identical flow: parse event → extract ID → fetch zip → extract YAML → check service nil → create from YAML. Similarly, the two undeployed handlers share: parse event → extract ID → check service nil → delete.A helper like
handleLLMDeployedEvent(event, entityType, fetchFunc, createFunc)could reduce ~100 lines of near-duplicate code.
Arshardh
force-pushed
the
llm-deployment-gateway
branch
from
170e901 to
d36ed24
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/cmd/controller/main.go`:
- Line 310: The code references non-existent cfg.GatewayController fields
causing a compile error; update the NewClient call that constructs cpClient to
use the correct Config fields: replace cfg.GatewayController.ControlPlane with
cfg.Controller.ControlPlane, replace &cfg.GatewayController.Router with
&cfg.Router, and replace &cfg.GatewayController.APIKey with &cfg.APIKey so the
call to controlplane.NewClient uses the existing Config struct fields (cpClient,
NewClient).
🧹 Nitpick comments (3)
gateway/gateway-controller/pkg/utils/api_utils.go (2)
122-234: Extract a shared fetch helper to eliminate near-identical code.
FetchLLMProviderDefinitionandFetchLLMProxyDefinitionare nearly verbatim copies ofFetchAPIDefinition— only the URL path segment and log entity name differ. This triplicates ~55 lines of HTTP/TLS/error-handling logic that must be kept in sync.Consider a private helper like
fetchDefinitionZip(resourceType, id string)and have all three public methods delegate to it.♻️ Sketch of shared helper
+// fetchDefinitionZip downloads a ZIP artifact from the control plane for the given resource path. +func (s *APIUtilsService) fetchDefinitionZip(resourcePath, resourceType, id string) ([]byte, error) { + url := s.config.BaseURL + "/" + resourcePath + "/" + id + s.logger.Info("Fetching "+resourceType+" definition", slog.String("id", id), slog.String("url", url)) + + client := &http.Client{ + Timeout: s.config.Timeout, + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: s.config.InsecureSkipVerify, + }, + }, + } + + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return nil, fmt.Errorf("failed to create request: %w", err) + } + req.Header.Add("api-key", s.config.Token) + req.Header.Add("Accept", "application/zip") + + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("failed to fetch %s definition: %w", resourceType, err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + return nil, fmt.Errorf("%s request failed with status %d: %s", resourceType, resp.StatusCode, string(bodyBytes)) + } + + bodyBytes, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + + s.logger.Info("Successfully fetched "+resourceType+" definition", slog.String("id", id), slog.Int("size_bytes", len(bodyBytes))) + return bodyBytes, nil +} + func (s *APIUtilsService) FetchAPIDefinition(apiID string) ([]byte, error) { - // ... 55 lines ... + return s.fetchDefinitionZip("apis", "API", apiID) } func (s *APIUtilsService) FetchLLMProviderDefinition(providerID string) ([]byte, error) { - // ... 55 lines ... + return s.fetchDefinitionZip("llm-providers", "LLM provider", providerID) } func (s *APIUtilsService) FetchLLMProxyDefinition(proxyID string) ([]byte, error) { - // ... 55 lines ... + return s.fetchDefinitionZip("llm-proxies", "LLM proxy", proxyID) }The same applies to
CreateLLMProviderFromYAML/CreateLLMProxyFromYAML(lines 291-327), though the duplication there is smaller.
132-140: MissingMinVersionin TLS config (also applies to lines 190-196).Static analysis flags
tls.ConfigwithoutMinVersion, defaulting to TLS 1.0 when acting as a server / TLS 1.2 as client. While TLS 1.2 client default is acceptable today, explicitly settingMinVersion: tls.VersionTLS12(ortls.VersionTLS13) is a best-practice hardening step. Note: the pre-existingFetchAPIDefinitionhas the same gap, so this could be addressed holistically (e.g., in the shared helper suggested above).gateway/gateway-controller/pkg/controlplane/client.go (1)
841-922: Consider extracting shared deployment logic for LLM provider and proxy handlers.
handleLLMProviderDeployedEventandhandleLLMProxyDeployedEventshare identical control flow: parse event → validate ID → fetch ZIP → extract YAML → nil-check service → create from YAML. They differ only in type names, ID field, and method calls. Similarly, the two undeployment handlers (lines 924-976, 1061-1113) share the same structure.This is less pressing than the
api_utils.goduplication since Go's type system makes generic extraction harder here, but worth noting for future maintenance.Also applies to: 978-1059
Arshardh
force-pushed
the
llm-deployment-gateway
branch
from
f243e5a to
92f2e8e
Compare
3 participants
Summary by CodeRabbit
New Features
Bug Fixes