Add event for API deletion #1079
Conversation
WalkthroughThe changes introduce API deletion event handling across gateway and platform services, rename RevisionID to DeploymentID in deployment events for semantic clarity, and integrate a new XDS manager dependency in the control plane client for API key management. Changes
Sequence Diagram(s)sequenceDiagram
participant ControlPlane as Control Plane
participant Client as Client
participant DB as Database
participant Store as Config Store
participant XDS as XDS Manager
participant PolicyEng as Policy Engine
ControlPlane->>Client: WebSocket: api.deleted event
Client->>Client: Parse DeletedEvent
alt API exists in DB
Client->>DB: Query API config
DB-->>Client: Return config
Client->>DB: Delete API config
Client->>Store: Remove from in-memory store
Client->>Store: Purge API keys
alt Config data available
Client->>PolicyEng: Update via XDS (delete)
end
Client->>XDS: Update snapshot (async)
Client->>Store: Delete derived policies
else API not found (orphan)
Client->>Store: Cleanup orphaned API keys
Client->>PolicyEng: Remove policy references
Client->>XDS: Conditional snapshot update
end
Client-->>ControlPlane: Cleanup complete + logging
sequenceDiagram
participant API as Platform API
participant Service as API Service
participant GW as Gateway Service
participant Event as Event Store
API->>Service: Delete API request
Service->>Event: Fetch gateway associations
Event-->>Service: Return gateways
Service->>Event: Delete API (cascades)
loop For each associated gateway
Service->>GW: BroadcastAPIDeletionEvent
GW->>GW: Generate CorrelationID
GW->>GW: Serialize deletion event
GW->>GW: Build gateway event DTO (type: api.deleted)
GW->>Event: Fetch gateway connections
Event-->>GW: Return active connections
loop For each connection
GW->>Event: Send event
GW->>GW: Track delivery status
end
GW-->>Service: Broadcast result + stats
end
Service-->>API: Deletion complete with notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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/controlplane/client.go`:
- Around line 876-899: The cleanup currently treats any error from
c.db.RemoveAPIKeysAPI and c.store.RemoveAPIKeysByAPI as "no API keys found" and
logs at Debug; update the error handling in the cleanup block so you distinguish
a not-found result from real errors (inspect the returned error or use the
package's sentinel/generic IsNotFound helper) and only log the not-found case as
Info/Debug, while logging genuine failures at Warn or Error and include the
error details (use c.logger.Warn/c.logger.Error with slog.String("api_id",
apiID) and slog.Any("err", err)); keep setting hasOrphanedResources=true only
when removal succeeded and ensure both c.db.RemoveAPIKeysAPI and
c.store.RemoveAPIKeysByAPI follow this corrected pattern.
🧹 Nitpick comments (4)
gateway/gateway-controller/pkg/api/handlers/handlers_test.go (1)
397-398: Reminder: rebuild gateway images after gateway changes.
Please runcd gateway && make build-localas part of this update. As per coding guidelines: “gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components … rebuild Docker images usingcd gateway && make build-local.”platform-api/src/internal/service/api.go (1)
340-345: Hardcoded"production"environment — consistent but potentially fragile.The environment is hardcoded to
"production"here, matching the same pattern indeployment.go(lines 194, 262, 325). If multi-environment support is planned, this should eventually be derived from the deployment or gateway configuration. Not a blocker since it's consistent with existing code.platform-api/src/internal/service/gateway_events.go (1)
211-282: Consider extracting a generic broadcast helper to reduce duplication.All
Broadcast*Eventmethods in this file share ~95% identical logic (correlationID generation, payload serialization, size validation, DTO creation, connection iteration, stats tracking, summary logging). A generic helper parameterized by event type string and payload would eliminate this repeated boilerplate. Not urgent given the existing pattern, but the surface area keeps growing with each new event type.gateway/gateway-controller/pkg/controlplane/client.go (1)
804-1083: Consider extracting orphan cleanup and full cleanup into private helpers.
handleAPIDeletedEventis ~280 lines with two distinct code paths (orphan cleanup vs. full cleanup). Extracting these intocleanupOrphanedResources(apiID, correlationID)andperformFullAPIDeletion(apiID, apiConfig, correlationID)helpers would improve readability and testability.
| if c.db != nil { | ||
| if err := c.db.RemoveAPIKeysAPI(apiID); err != nil { | ||
| c.logger.Debug("No API keys found in database to clean up", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } else { | ||
| hasOrphanedResources = true | ||
| c.logger.Info("Cleaned up orphaned API keys from database", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Check and clean up orphaned API keys from memory store | ||
| if err := c.store.RemoveAPIKeysByAPI(apiID); err != nil { | ||
| c.logger.Debug("No API keys found in memory store to clean up", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } else { | ||
| hasOrphanedResources = true | ||
| c.logger.Info("Cleaned up orphaned API keys from memory store", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Orphan cleanup error handling may swallow real errors.
The RemoveAPIKeysAPI and RemoveAPIKeysByAPI calls can fail for reasons beyond "not found" (e.g., database connectivity issues). Logging all errors at Debug level with the message "No API keys found" is misleading and may mask real failures.
Consider distinguishing between "not found" and genuine errors, or at minimum logging at Warn level with the actual error:
Proposed fix for DB orphan cleanup (apply analogous change for memory store)
if c.db != nil {
if err := c.db.RemoveAPIKeysAPI(apiID); err != nil {
- c.logger.Debug("No API keys found in database to clean up",
+ c.logger.Warn("Failed to clean up API keys from database (may not exist)",
slog.String("api_id", apiID),
+ slog.Any("error", err),
)
} else {🤖 Prompt for AI Agents
In `@gateway/gateway-controller/pkg/controlplane/client.go` around lines 876 -
899, The cleanup currently treats any error from c.db.RemoveAPIKeysAPI and
c.store.RemoveAPIKeysByAPI as "no API keys found" and logs at Debug; update the
error handling in the cleanup block so you distinguish a not-found result from
real errors (inspect the returned error or use the package's sentinel/generic
IsNotFound helper) and only log the not-found case as Info/Debug, while logging
genuine failures at Warn or Error and include the error details (use
c.logger.Warn/c.logger.Error with slog.String("api_id", apiID) and
slog.Any("err", err)); keep setting hasOrphanedResources=true only when removal
succeeded and ensure both c.db.RemoveAPIKeysAPI and c.store.RemoveAPIKeysByAPI
follow this corrected pattern.
There was a problem hiding this comment.
Pull request overview
Adds an explicit api.deleted gateway event so gateways can proactively clean up API config/routes/keys when an API is permanently removed from the platform. This also standardizes deployment event payload naming by switching from revisionId to deploymentId across platform-api and gateway-controller.
Changes:
- Add
api.deletedevent model + broadcaster in platform-api and emit it during API deletion. - Rename deployment event field
revisionId→deploymentIdacross DTOs/models and gateway-controller consumers/tests. - Add gateway-controller handling for
api.deletedto clean up stored configs, API keys, policy artifacts, and update snapshots.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| platform-api/src/internal/service/gateway_events.go | Adds BroadcastAPIDeletionEvent to publish api.deleted over WebSocket. |
| platform-api/src/internal/service/deployment.go | Updates deployment event payload to use DeploymentID instead of RevisionID. |
| platform-api/src/internal/service/api.go | Fetches gateway associations pre-delete and broadcasts api.deleted to associated gateways. |
| platform-api/src/internal/model/gateway_event.go | Extends gateway event model: adds APIDeletionEvent and renames RevisionID → DeploymentID. |
| platform-api/src/internal/dto/gateway_event.go | Renames DTO field RevisionID → DeploymentID. |
| gateway/gateway-controller/pkg/utils/api_utils_test.go | Comment-only update reflecting deployment terminology. |
| gateway/gateway-controller/pkg/controlplane/events.go | Adds APIDeletedEvent structs and renames deployed payload field to DeploymentID. |
| gateway/gateway-controller/pkg/controlplane/controlplane_test.go | Updates tests to validate DeploymentID field. |
| gateway/gateway-controller/pkg/controlplane/client_integration_test.go | Updates deployed event JSON + adds api.deleted to “all event types” coverage. |
| gateway/gateway-controller/pkg/controlplane/client.go | Handles api.deleted and performs cleanup + snapshot updates; adds apiKeyXDSManager to support policy-engine API key cleanup. |
| gateway/gateway-controller/pkg/api/handlers/handlers_test.go | Updates mock signature parameter name (revisionID → deploymentID). |
| // handleAPIDeletedEvent handles API deletion events | ||
| func (c *Client) handleAPIDeletedEvent(event map[string]interface{}) { | ||
| c.logger.Info("API Deletion Event", | ||
| slog.Any("payload", event["payload"]), | ||
| slog.Any("timestamp", event["timestamp"]), | ||
| slog.Any("correlationId", event["correlationId"]), | ||
| ) | ||
|
|
||
| // Parse the event into structured format | ||
| eventBytes, err := json.Marshal(event) | ||
| if err != nil { | ||
| c.logger.Error("Failed to marshal event for parsing", | ||
| slog.Any("error", err), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| var deletedEvent APIDeletedEvent | ||
| if err := json.Unmarshal(eventBytes, &deletedEvent); err != nil { | ||
| c.logger.Error("Failed to parse API deletion event", | ||
| slog.Any("error", err), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| // Extract API ID | ||
| apiID := deletedEvent.Payload.APIID | ||
| if apiID == "" { | ||
| c.logger.Error("API ID is empty in deletion event") | ||
| return | ||
| } | ||
|
|
||
| c.logger.Info("Processing API deletion", | ||
| slog.String("api_id", apiID), | ||
| slog.String("environment", deletedEvent.Payload.Environment), | ||
| slog.String("vhost", deletedEvent.Payload.VHost), | ||
| slog.String("correlation_id", deletedEvent.CorrelationID), | ||
| ) | ||
|
|
||
| // Check if API exists on this gateway and retrieve config for metadata | ||
| var apiConfig *models.StoredConfig | ||
| var apiExists bool | ||
|
|
||
| // Check database first (source of truth when available) | ||
| if c.db != nil { | ||
| var err error | ||
| apiConfig, err = c.db.GetConfig(apiID) | ||
| if err == nil { | ||
| apiExists = true | ||
| } | ||
| } | ||
|
|
||
| // If not in database, check in-memory store | ||
| if !apiExists { | ||
| var err error | ||
| apiConfig, err = c.store.Get(apiID) | ||
| if err == nil { | ||
| apiExists = true | ||
| } | ||
| } | ||
|
|
||
| if !apiExists { | ||
| // API config doesn't exist, but there might be orphaned resources (API keys, policies) | ||
| // Attempt cleanup of these resources silently | ||
| c.logger.Info("API configuration not found on this gateway, checking for orphaned resources", | ||
| slog.String("api_id", apiID), | ||
| slog.String("correlation_id", deletedEvent.CorrelationID), | ||
| ) | ||
|
|
||
| hasOrphanedResources := false | ||
|
|
||
| // Check and clean up orphaned API keys from database | ||
| if c.db != nil { | ||
| if err := c.db.RemoveAPIKeysAPI(apiID); err != nil { | ||
| c.logger.Debug("No API keys found in database to clean up", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } else { | ||
| hasOrphanedResources = true | ||
| c.logger.Info("Cleaned up orphaned API keys from database", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Check and clean up orphaned API keys from memory store | ||
| if err := c.store.RemoveAPIKeysByAPI(apiID); err != nil { | ||
| c.logger.Debug("No API keys found in memory store to clean up", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } else { | ||
| hasOrphanedResources = true | ||
| c.logger.Info("Cleaned up orphaned API keys from memory store", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } | ||
|
|
||
| // Note: Cannot remove orphaned API keys from policy engine via xDS without API config | ||
| // (requires API name and version which are only available in the config) | ||
| // The xDS snapshot update below will help clean up stale routes | ||
| c.logger.Debug("Skipping API key removal from policy engine (requires API config metadata)", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
|
|
||
| // Check and clean up orphaned policy configuration | ||
| if c.policyManager != nil { | ||
| policyID := apiID + "-policies" | ||
| if err := c.policyManager.RemovePolicy(policyID); err != nil { | ||
| c.logger.Debug("No orphaned policy configuration found", | ||
| slog.String("policy_id", policyID), | ||
| ) | ||
| } else { | ||
| hasOrphanedResources = true | ||
| c.logger.Info("Cleaned up orphaned policy configuration", | ||
| slog.String("policy_id", policyID), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Update xDS snapshot to remove any stale routes | ||
| if c.snapshotManager != nil { | ||
| go func() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := c.snapshotManager.UpdateSnapshot(ctx, deletedEvent.CorrelationID); err != nil { | ||
| c.logger.Warn("Failed to update xDS snapshot for orphaned resource cleanup", | ||
| slog.String("api_id", apiID), | ||
| slog.Any("error", err), | ||
| ) | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| if hasOrphanedResources { | ||
| c.logger.Info("Orphaned resource cleanup completed", | ||
| slog.String("api_id", apiID), | ||
| slog.String("correlation_id", deletedEvent.CorrelationID), | ||
| ) | ||
| } else { | ||
| c.logger.Info("No orphaned resources found, API was never deployed to this gateway", | ||
| slog.String("api_id", apiID), | ||
| slog.String("correlation_id", deletedEvent.CorrelationID), | ||
| ) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
handleAPIDeletedEvent introduces substantial new behavior (DB/store deletion, API key cleanup, policy cleanup, snapshot updates) but the current tests only validate that handleMessage doesn’t panic for an api.deleted message. Add focused unit tests that assert the expected interactions for key scenarios (API exists in DB, exists only in in-memory store, does not exist, policy manager present/absent, storage errors), so regressions in cleanup behavior are caught.
| // Get all gateway associations BEFORE deletion (associations will be cascade deleted) | ||
| gatewayAssociations, err := s.apiRepo.GetAPIAssociations(apiUUID, constants.AssociationTypeGateway, orgUUID) | ||
| if err != nil { | ||
| log.Printf("[WARN] Failed to get gateway associations for API deletion: apiUUID=%s error=%v", apiUUID, err) | ||
| } |
There was a problem hiding this comment.
If GetAPIAssociations fails here, the API is still deleted but no gateway deletion events will be sent (and associations are cascade-deleted, so you can’t recover the list later). That can leave gateways serving stale routes/config for a deleted API. Consider treating this as a hard failure (return the error) or otherwise ensuring you can still notify all relevant gateways before proceeding with the repository delete.
| // Check and clean up orphaned API keys from database | ||
| if c.db != nil { | ||
| if err := c.db.RemoveAPIKeysAPI(apiID); err != nil { | ||
| c.logger.Debug("No API keys found in database to clean up", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } else { | ||
| hasOrphanedResources = true | ||
| c.logger.Info("Cleaned up orphaned API keys from database", | ||
| slog.String("api_id", apiID), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
RemoveAPIKeysAPI (sqlStore) returns nil even when there are no keys (it doesn’t check RowsAffected), so using err == nil to infer that orphaned keys existed will always set hasOrphanedResources = true when c.db != nil. Also, when this call does return an error, it indicates a real DB failure (not “no keys found”), so logging it as Debug is misleading. Consider (a) logging errors from RemoveAPIKeysAPI at Warn/Error, and (b) not using its return value to decide whether orphaned resources existed (or add a storage method that returns affected row count).
| if c.policyManager != nil { | ||
| policyID := apiID + "-policies" | ||
| if err := c.policyManager.RemovePolicy(policyID); err != nil { | ||
| c.logger.Debug("No orphaned policy configuration found", | ||
| slog.String("policy_id", policyID), | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
This treats any error from policyManager.RemovePolicy as “no orphaned policy configuration found”. But RemovePolicy can also fail after deleting the policy (e.g., snapshot update failure) or fail for reasons other than “not found”, which should be surfaced as a warning at least. Consider distinguishing not-found errors from other failures (e.g., by checking the error text/type) and logging non-not-found failures at Warn/Error so policy snapshot issues aren’t silently ignored.
1 participant
Purpose
Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment
Summary by CodeRabbit
Release Notes
New Features
Improvements