Add gateway implementation to handle llm deployment #1059
Conversation
WalkthroughThe changes extend the control plane client to support LLM provider deployment lifecycle management. New event types capture LLM provider deployed and undeployed events, while the client now accepts a lazy resource manager and template definitions to initialize an LLM deployment service. Event handlers process LLM provider events by fetching provider definitions and delegating to the deployment service. Utility functions support fetching provider definitions and creating deployments from YAML. All test call sites are updated to pass the extended parameter signatures. Changes
Sequence DiagramsequenceDiagram
participant WS as WebSocket Connection
participant CM as Client (handleMessage)
participant LH as LLM Event Handler
participant AU as API Utils Service
participant LDS as LLM Deployment Service
WS->>CM: Receive llmprovider.deployed event
CM->>LH: handleLLMProviderDeployedEvent(payload)
LH->>AU: FetchLLMProviderDefinition(providerID)
AU-->>LH: Return provider definition (bytes)
LH->>AU: CreateLLMProviderFromYAML(yamlData, providerID, correlationID)
AU->>LDS: DeployLLMProviderConfiguration(...)
LDS-->>AU: Deployment result
AU-->>LH: Return deployment result
LH->>LH: Log deployment completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/utils/api_utils.go`:
- Around line 132-140: The tls.Config used when constructing the HTTP client
(see the tls.Config in the client := &http.Client{...} block) and the tls.Config
used in FetchAPIDefinition must explicitly set MinVersion (e.g., MinVersion:
tls.VersionTLS12 or tls.VersionTLS13) to avoid falling back to legacy TLS
versions; update both locations to include MinVersion and, if possible, factor
creation of the TLS config into a shared helper (e.g., newTLSConfig or
getTLSConfig) used by the HTTP client construction and FetchAPIDefinition so the
MinVersion is consistently applied.
🧹 Nitpick comments (2)
gateway/gateway-controller/pkg/utils/api_utils.go (1)
122-177: Consider extracting common HTTP fetch logic to reduce duplication.
FetchLLMProviderDefinitionis nearly identical toFetchAPIDefinition(lines 66–120) — only the URL path segment and log/error messages differ. A shared private helper (e.g.,fetchDefinition(resourceType, id string)) would eliminate ~50 lines of duplication and make future changes (e.g., adding TLSMinVersion, retry logic, or shared headers) easier to maintain.♻️ Sketch of a shared helper
+// fetchResourceDefinition downloads a resource definition (API or LLM provider) from the control plane. +func (s *APIUtilsService) fetchResourceDefinition(resourcePath, resourceID, resourceLabel string) ([]byte, error) { + url := s.config.BaseURL + "/" + resourcePath + "/" + resourceID + + s.logger.Info("Fetching "+resourceLabel+" definition", + slog.String("id", resourceID), + slog.String("url", url), + ) + + client := &http.Client{ + Timeout: s.config.Timeout, + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: s.config.InsecureSkipVerify, + MinVersion: tls.VersionTLS12, + }, + }, + } + + 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", resourceLabel, 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", resourceLabel, 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 "+resourceLabel+" definition", + slog.String("id", resourceID), + slog.Int("size_bytes", len(bodyBytes)), + ) + return bodyBytes, nil +} + func (s *APIUtilsService) FetchAPIDefinition(apiID string) ([]byte, error) { - // ... current implementation ... + return s.fetchResourceDefinition("apis", apiID, "API") } func (s *APIUtilsService) FetchLLMProviderDefinition(providerID string) ([]byte, error) { - // ... current implementation ... + return s.fetchResourceDefinition("llm-providers", providerID, "LLM provider") }gateway/gateway-controller/cmd/controller/main.go (1)
315-315:NewClientnow takes 14 positional parameters — consider a config/options struct.This single call is hard to read and fragile (easy to swap adjacent
nil-able arguments). A dedicatedClientDepsor options struct would improve readability and make future extensions safer. This isn't blocking, but worth tracking.
| // Create HTTP client with TLS configuration | ||
| client := &http.Client{ | ||
| Timeout: s.config.Timeout, | ||
| Transport: &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: s.config.InsecureSkipVerify, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Missing TLS MinVersion — flagged by static analysis.
The tls.Config lacks a MinVersion, defaulting to TLS 1.0 on older Go versions. The same issue exists in FetchAPIDefinition (line 79). Consider setting MinVersion: tls.VersionTLS12 (or tls.VersionTLS13) in both places — ideally in the shared helper suggested above.
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 135-137: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: s.config.InsecureSkipVerify,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/utils/api_utils.go` around lines 132 - 140,
The tls.Config used when constructing the HTTP client (see the tls.Config in the
client := &http.Client{...} block) and the tls.Config used in FetchAPIDefinition
must explicitly set MinVersion (e.g., MinVersion: tls.VersionTLS12 or
tls.VersionTLS13) to avoid falling back to legacy TLS versions; update both
locations to include MinVersion and, if possible, factor creation of the TLS
config into a shared helper (e.g., newTLSConfig or getTLSConfig) used by the
HTTP client construction and FetchAPIDefinition so the MinVersion is
consistently applied.
1 participant
Purpose
Summary by CodeRabbit