Move API Update related logic to api_deployment.go and reuse methods between createAPI and UpdateAPI #636
Conversation
WalkthroughThis PR integrates policy management into the gateway controller by centralizing policy-related logic within the APIDeploymentService. The policyManager dependency is threaded through the dependency chain (main entry point → control plane client → API handlers → deployment service), enabling the deployment service to manage policy derivation, updates, and lifecycle alongside API deployments and WebSub topic registration. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as API Handler
participant DeploymentSvc as APIDeploymentService
participant PolicyMgr as PolicyManager
participant TopicMgr as Topic Manager
participant XDS as XDS Snapshot Manager
rect rgb(200, 220, 255)
Note over Handler,XDS: UpdateAPIConfiguration Flow
end
Handler->>DeploymentSvc: UpdateAPIConfiguration(params)
activate DeploymentSvc
DeploymentSvc->>DeploymentSvc: Parse & Validate Config
alt Validation Fails
DeploymentSvc-->>Handler: Error (APIValidationError)
else Valid
DeploymentSvc->>DeploymentSvc: Update Store & DB
rect rgb(220, 255, 220)
Note over DeploymentSvc,TopicMgr: Topic Lifecycle Management
end
DeploymentSvc->>TopicMgr: handleTopicLifecycle()
activate TopicMgr
TopicMgr-->>DeploymentSvc: ✓ Complete
deactivate TopicMgr
rect rgb(255, 240, 200)
Note over DeploymentSvc,PolicyMgr: Policy Derivation & Update
end
DeploymentSvc->>DeploymentSvc: BuildStoredPolicyFromAPI(config)
DeploymentSvc->>PolicyMgr: updatePolicyConfiguration()
activate PolicyMgr
PolicyMgr-->>DeploymentSvc: ✓ Updated
deactivate PolicyMgr
rect rgb(240, 220, 255)
Note over DeploymentSvc,XDS: XDS Synchronization
end
DeploymentSvc->>XDS: triggerXDSSnapshotUpdate()
activate XDS
XDS-->>DeploymentSvc: ✓ Refreshed
deactivate XDS
DeploymentSvc-->>Handler: APIDeploymentResult
end
deactivate DeploymentSvc
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-controller/pkg/utils/api_deployment.go (1)
178-178: Remove debug print statement.This
fmt.Printlnstatement appears to be leftover debug code that should be removed before merging.🔎 Proposed fix
case api.RestApi: apiData, err := apiConfig.Spec.AsAPIConfigData() - fmt.Println("APIData: ", apiData) if err != nil {
🤖 Fix all issues with AI Agents
In @gateway/gateway-controller/pkg/utils/api_deployment.go:
- Line 731: BuildStoredPolicyFromAPI currently dereferences s.routerConfig
(e.g., accessing s.routerConfig.GatewayHost and s.routerConfig.VHosts) which can
be nil; add a defensive nil check at the start of BuildStoredPolicyFromAPI (or
before any access in AsyncWebSub/RestApi code paths) to handle a nil
s.routerConfig safely—either return early with appropriate default
behavior/error or use safe defaults for GatewayHost/VHosts when s.routerConfig
== nil so the code in the AsyncWebSub routeKey generation
(xds.GenerateRouteName("POST", apiData.Context, apiData.Version, ch.Path,
s.routerConfig.GatewayHost)) and the RestApi vhost handling never panics.
🧹 Nitpick comments (2)
gateway/gateway-controller/cmd/controller/main.go (1)
428-546: Consider consolidating policy derivation logic.The
derivePolicyFromAPIConfigandconvertAPIPolicyToModelfunctions duplicate logic that now exists inAPIDeploymentService.BuildStoredPolicyFromAPIandconvertAPIPolicy. Given the PR's goal to centralize policy-related logic in the deployment service, consider reusingdeploymentService.BuildStoredPolicyFromAPIduring startup instead of maintaining parallel implementations.This would prevent the two implementations from diverging over time.
🔎 Suggested approach
Create an
APIDeploymentServiceinstance for startup policy loading:// In the policy loading section (around line 199-221) tempDeploymentService := utils.NewAPIDeploymentService( configStore, db, snapshotManager, policyManager, validator, &cfg.GatewayController.Router) for _, apiConfig := range loadedAPIs { if apiConfig.Configuration.Kind == api.RestApi { storedPolicy := tempDeploymentService.BuildStoredPolicyFromAPI(apiConfig) if storedPolicy != nil { if err := policyStore.Set(storedPolicy); err != nil { // handle error } } } }Then remove the
derivePolicyFromAPIConfigandconvertAPIPolicyToModelfunctions.gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
984-995: Consider consolidating policy management for LLM/MCP handlers.The LLM and MCP handlers call
s.deploymentService.BuildStoredPolicyFromAPI()ands.policyManager.AddPolicy()inline after creation/update, whileCreateAPIandUpdateAPIdelegate this tos.deploymentService.updatePolicyConfiguration()internally.For consistency, consider either:
- Having all handlers delegate policy management to the deployment service, or
- Documenting why the approaches differ (e.g., LLM/MCP use different deployment services that don't handle policies internally).
This is a minor consistency observation rather than a blocking issue.
Also applies to: 1074-1095, 1221-1232, 1311-1332, 1463-1474, 1640-1661
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gateway/gateway-controller/cmd/controller/main.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/policy_ordering_test.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/utils/api_deployment.gogateway/gateway-controller/pkg/utils/websub_topic_registration_test.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-06T11:57:08.456Z
Learnt from: BLasan
Repo: wso2/api-platform PR: 140
File: gateway/gateway-controller/pkg/utils/api_deployment.go:88-105
Timestamp: 2025-12-06T11:57:08.456Z
Learning: In the gateway-controller Go codebase (file: gateway/gateway-controller/pkg/utils/api_deployment.go), only the async/websub API kind is currently supported. async/websocket and async/sse API kinds are not yet implemented.
Applied to files:
gateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/utils/websub_topic_registration_test.gogateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/utils/api_deployment.go
📚 Learning: 2025-11-08T13:06:22.133Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 110
File: platform-api/src/internal/service/api.go:432-476
Timestamp: 2025-11-08T13:06:22.133Z
Learning: In the platform-api Go codebase (file: platform-api/src/internal/service/api.go), the DeployAPIRevision method is designed to automatically create API-gateway associations during deployment if they don't already exist. There is no requirement that associations must pre-exist before deployment; the system checks for existing associations and creates them on-the-fly as needed.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/policy_ordering_test.gogateway/gateway-controller/pkg/utils/api_deployment.go
📚 Learning: 2025-12-23T10:41:47.584Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 389
File: gateway/gateway-controller/pkg/utils/api_key.go:257-380
Timestamp: 2025-12-23T10:41:47.584Z
Learning: In gateway/gateway-controller/pkg/utils/api_key.go, the RevokeAPIKey method intentionally proceeds with database deletion and xDS policy engine revocation even when the API key is not found in the memory store (apiKey is nil). This is by design to handle partial failure recovery: if a previous revocation attempt removed the key from memory but failed to clean up the database or policy engine, subsequent retries will still perform the cleanup operations to ensure eventual consistency.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
📚 Learning: 2025-12-22T16:00:12.851Z
Learnt from: O-sura
Repo: wso2/api-platform PR: 569
File: gateway/policy-engine/internal/analytics/constants.go:100-101
Timestamp: 2025-12-22T16:00:12.851Z
Learning: In gateway/policy-engine/internal/analytics/constants.go, the constant APITypeKey is intentionally named with "Type" terminology (instead of "Kind") to maintain consistency with the analytics publisher platform, which uses "type" rather than "kind". The constant resolves to "x-wso2-api-kind" but the name APITypeKey is kept for cross-platform naming consistency.
Applied to files:
gateway/gateway-controller/pkg/api/handlers/handlers.go
🧬 Code graph analysis (4)
gateway/gateway-controller/pkg/controlplane/client.go (2)
gateway/gateway-controller/pkg/policyxds/manager.go (1)
PolicyManager(33-37)gateway/gateway-controller/pkg/utils/api_deployment.go (1)
NewAPIDeploymentService(143-161)
gateway/gateway-controller/pkg/utils/websub_topic_registration_test.go (1)
gateway/gateway-controller/pkg/utils/api_deployment.go (1)
NewAPIDeploymentService(143-161)
gateway/gateway-controller/pkg/api/handlers/policy_ordering_test.go (4)
gateway/gateway-controller/pkg/config/config.go (1)
RouterConfig(171-184)gateway/gateway-controller/pkg/storage/memory.go (1)
NewConfigStore(49-61)gateway/gateway-controller/pkg/utils/api_deployment.go (1)
NewAPIDeploymentService(143-161)gateway/gateway-controller/pkg/api/handlers/handlers.go (1)
APIServer(50-68)
gateway/gateway-controller/cmd/controller/main.go (1)
gateway/gateway-controller/pkg/controlplane/client.go (1)
NewClient(111-154)
🔇 Additional comments (12)
gateway/gateway-controller/pkg/utils/websub_topic_registration_test.go (1)
21-21: LGTM!The
NewAPIDeploymentServicecalls are correctly updated to match the new constructor signature withpolicyManageras the 4th parameter. PassingnilforpolicyManagerin these WebSub topic registration tests is appropriate since the tests focus on topic lifecycle management rather than policy functionality.Also applies to: 66-66, 148-148, 252-252
gateway/gateway-controller/cmd/controller/main.go (1)
275-275: LGTM!The control plane client construction correctly passes
policyManagertoNewClient. ThepolicyManagermay benilwhen the policy server is disabled, which is handled gracefully downstream.gateway/gateway-controller/pkg/api/handlers/policy_ordering_test.go (2)
34-48: LGTM!The test setup correctly initializes the
APIDeploymentServicewith minimal dependencies needed forBuildStoredPolicyFromAPI. Passingnilfordb,snapshotManager,policyManager, andvalidatoris appropriate since the policy building logic only requiresrouterConfigfor vhost resolution.
203-203: LGTM!All test calls correctly updated to use
server.deploymentService.BuildStoredPolicyFromAPI(cfg), aligning with the refactor that centralizes policy construction in the deployment service.Also applies to: 317-317, 444-444
gateway/gateway-controller/pkg/controlplane/client.go (1)
110-154: LGTM!The
NewClientconstructor correctly threadspolicyManagerthrough toNewAPIDeploymentService. The design is clean - theClientstruct doesn't storepolicyManagerdirectly since it only needs it for the deployment service's policy handling.gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
84-84: LGTM!The
NewAPIServercorrectly wirespolicyManagerintoNewAPIDeploymentService, enabling centralized policy handling within the deployment service.
477-543: Well-structured error handling for UpdateAPI.The
UpdateAPImethod correctly delegates todeploymentService.UpdateAPIConfigurationand maps all custom error types (NotFoundError,HandleMismatchError,APIValidationError,ParseError,TopicOperationError,ConflictError,DatabaseUnavailableError) to appropriate HTTP status codes. This aligns well with the refactoring goals.gateway/gateway-controller/pkg/utils/api_deployment.go (5)
57-128: LGTM - Well-designed custom error types.The custom error types (
NotFoundError,HandleMismatchError,APIValidationError,TopicOperationError,ConflictError,ParseError,DatabaseUnavailableError) are well-structured and enable clean HTTP status code mapping in handlers.
381-465: LGTM - Properly synchronized topic lifecycle handling.The
handleTopicLifecyclemethod correctly uses:
sync.WaitGroupfor coordinating concurrent operationsatomic.AddInt32for thread-safe error counting- Nested goroutines for parallelizing individual topic operations
The error aggregation and reporting is well-implemented.
285-379: LGTM - UpdateAPIConfiguration is well-structured.The method correctly:
- Validates handle consistency between URL path and YAML metadata
- Runs configuration validation
- Checks for database availability
- Handles AsyncWebSub topic lifecycle
- Updates both database and memory store
- Triggers async xDS and policy updates
- Returns appropriate typed errors for HTTP status mapping
677-827: LGTM - BuildStoredPolicyFromAPI centralizes policy construction.The method correctly:
- Collects API-level policies into a lookup map
- Handles both
AsyncWebsubandRestApikinds- Merges operation-level policies with API-level policies (operation policies take precedence)
- Generates correct route keys using
xds.GenerateRouteName- Handles sandbox vhost routing when sandbox upstream is configured
- Returns
nilwhen no policies exist (avoiding unnecessary policy configurations)
846-887: LGTM - Clean helper methods for policy and xDS management.The
updatePolicyConfigurationandtriggerXDSSnapshotUpdatemethods cleanly encapsulate:
- Policy add/remove logic with appropriate logging levels
- Async xDS snapshot updates with timeout handling
| } | ||
| } | ||
|
|
||
| routeKey := xds.GenerateRouteName("POST", apiData.Context, apiData.Version, ch.Path, s.routerConfig.GatewayHost) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "api_deployment.go" -path "*/gateway-controller/*" | head -5Repository: wso2/api-platform
Length of output: 117
🏁 Script executed:
find . -name "policy_ordering_test.go" | head -5Repository: wso2/api-platform
Length of output: 130
🏁 Script executed:
# Find the BuildStoredPolicyFromAPI method
rg -n "BuildStoredPolicyFromAPI" --type=go -B 2 -A 15 | head -100Repository: wso2/api-platform
Length of output: 10210
🏁 Script executed:
sed -n '681,800p' ./gateway/gateway-controller/pkg/utils/api_deployment.goRepository: wso2/api-platform
Length of output: 4372
🏁 Script executed:
rg -n "func newTestAPIServer" --type=go -A 20Repository: wso2/api-platform
Length of output: 2329
🏁 Script executed:
rg -n "type APIDeploymentService struct" --type=go -A 15Repository: wso2/api-platform
Length of output: 1510
🏁 Script executed:
# Search for all callers of BuildStoredPolicyFromAPI
rg -n "BuildStoredPolicyFromAPI" --type=goRepository: wso2/api-platform
Length of output: 1742
🏁 Script executed:
rg -n "type APIServer struct" --type=go -A 10Repository: wso2/api-platform
Length of output: 1173
🏁 Script executed:
# Check NewAPIDeploymentService calls in handlers.go
rg -n "NewAPIDeploymentService" --type=go -B 2 -A 2Repository: wso2/api-platform
Length of output: 4533
🏁 Script executed:
# Check where APIServer is instantiated
rg -n "NewAPIServer\|&APIServer\{" --type=go -B 2 -A 5Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check what the websub tests do - do they call BuildStoredPolicyFromAPI?
sed -n '19,100p' ./gateway/gateway-controller/pkg/utils/websub_topic_registration_test.goRepository: wso2/api-platform
Length of output: 2385
🏁 Script executed:
# Check if any websub test calls BuildStoredPolicyFromAPI
rg -n "BuildStoredPolicyFromAPI" ./gateway/gateway-controller/pkg/utils/websub_topic_registration_test.goRepository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check what happens when service is created with nil routerConfig in tests - trace the code path
sed -n '1,50p' ./gateway/gateway-controller/pkg/utils/websub_topic_registration_test.goRepository: wso2/api-platform
Length of output: 1583
🏁 Script executed:
# Check if there's any code that validates/checks routerConfig before calling BuildStoredPolicyFromAPI
rg -n "routerConfig" ./gateway/gateway-controller/pkg/utils/api_deployment.goRepository: wso2/api-platform
Length of output: 433
🏁 Script executed:
# Check if websub tests ever trigger BuildStoredPolicyFromAPI through any method calls
sed -n '1,300p' ./gateway/gateway-controller/pkg/utils/websub_topic_registration_test.go | grep -E "store.Add|BuildStoredPolicy|DeployAPIConfiguration"Repository: wso2/api-platform
Length of output: 404
🏁 Script executed:
# Look at the full context of what happens in a websub test to see if it could trigger the issue
sed -n '1,100p' ./gateway/gateway-controller/pkg/utils/websub_topic_registration_test.goRepository: wso2/api-platform
Length of output: 2985
🏁 Script executed:
# Search for the Add method in storage to see if it calls BuildStoredPolicyFromAPI
rg -n "func.*Add" ./gateway/gateway-controller/pkg/storage/ --type=go -A 5 | head -50Repository: wso2/api-platform
Length of output: 1574
🏁 Script executed:
# Check if ConfigStore.Add calls any deployment service methods
rg -n "class ConfigStore\|type ConfigStore" ./gateway/gateway-controller/pkg/storage/ --type=go -A 20Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Look at where BuildStoredPolicyFromAPI is actually called - in handlers and api_deployment
sed -n '840,870p' ./gateway/gateway-controller/pkg/utils/api_deployment.goRepository: wso2/api-platform
Length of output: 1341
🏁 Script executed:
# Check where updatePolicyConfiguration is called
rg -n "updatePolicyConfiguration" --type=goRepository: wso2/api-platform
Length of output: 605
🏁 Script executed:
# Verify the full flow - where is updatePolicyConfiguration called from?
rg -n "updatePolicyConfiguration" --type=go -B 3 -A 3Repository: wso2/api-platform
Length of output: 2067
🏁 Script executed:
# Check where NewAPIServer is called to verify routerConfig is always provided
rg -n "NewAPIServer" --type=go -B 5 -A 5Repository: wso2/api-platform
Length of output: 2262
🏁 Script executed:
# Check the function signature of NewAPIServer to understand the flow
sed -n '75,105p' ./gateway/gateway-controller/pkg/api/handlers/handlers.goRepository: wso2/api-platform
Length of output: 1412
🏁 Script executed:
# Check if there's any place where routerConfig could be nil in production paths
rg -n "cfg.GatewayController.Router" --type=go -B 2 -A 2Repository: wso2/api-platform
Length of output: 3011
🏁 Script executed:
# Check if there are other callers of APIDeploymentService besides main.go and tests
rg -n "NewAPIDeploymentService" --type=go | grep -v test | grep -v "policy_ordering"Repository: wso2/api-platform
Length of output: 629
🏁 Script executed:
# Let's check what API kinds trigger the code path - does AsyncWebSub go through buildStoredPolicyFromAPI?
# Check if websub tests actually ever create RestApi which uses routerConfig
sed -n '20,80p' ./gateway/gateway-controller/pkg/utils/websub_topic_registration_test.go | grep -A 5 "kind:"Repository: wso2/api-platform
Length of output: 307
🏁 Script executed:
# Final verification - check if there's any conditional that prevents BuildStoredPolicyFromAPI from being called when routerConfig is nil
sed -n '677,700p' ./gateway/gateway-controller/pkg/utils/api_deployment.goRepository: wso2/api-platform
Length of output: 1032
🏁 Script executed:
# Verify the websub tests don't actually call BuildStoredPolicyFromAPI - they only call store.Add
rg -n "store.Add\|BuildStoredPolicy" ./gateway/gateway-controller/pkg/utils/websub_topic_registration_test.goRepository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Let me verify the complete flow - check updatePolicyConfiguration for any nil guards
sed -n '846,870p' ./gateway/gateway-controller/pkg/utils/api_deployment.goRepository: wso2/api-platform
Length of output: 1208
🏁 Script executed:
# Also check where DeployAPIConfiguration and UpdateAPIConfiguration are called to understand the actual usage
rg -n "DeployAPIConfiguration\|UpdateAPIConfiguration" --type=go | grep -v "func\|// " | head -20Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Final check - verify if there's a guard that prevents BuildStoredPolicyFromAPI from running when routerConfig is nil
rg -n "if s.routerConfig" --type=goRepository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check the exact lines 775-776 to confirm the accesses
sed -n '773,778p' ./gateway/gateway-controller/pkg/utils/api_deployment.goRepository: wso2/api-platform
Length of output: 359
Add nil check for routerConfig in BuildStoredPolicyFromAPI.
The method accesses s.routerConfig.GatewayHost (line 731 for AsyncWebSub) and s.routerConfig.VHosts (lines 775-776 for RestApi) without checking if routerConfig is nil. This will cause a panic if BuildStoredPolicyFromAPI is called when routerConfig is nil, such as in websub_topic_registration_test.go which passes nil for routerConfig.
Add a defensive nil check:
Proposed fix
func (s *APIDeploymentService) BuildStoredPolicyFromAPI(cfg *models.StoredConfig) *models.StoredPolicyConfig {
+ if s.routerConfig == nil {
+ return nil
+ }
apiCfg := &cfg.ConfigurationCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @gateway/gateway-controller/pkg/utils/api_deployment.go around line 731,
BuildStoredPolicyFromAPI currently dereferences s.routerConfig (e.g., accessing
s.routerConfig.GatewayHost and s.routerConfig.VHosts) which can be nil; add a
defensive nil check at the start of BuildStoredPolicyFromAPI (or before any
access in AsyncWebSub/RestApi code paths) to handle a nil s.routerConfig
safely—either return early with appropriate default behavior/error or use safe
defaults for GatewayHost/VHosts when s.routerConfig == nil so the code in the
AsyncWebSub routeKey generation (xds.GenerateRouteName("POST", apiData.Context,
apiData.Version, ch.Path, s.routerConfig.GatewayHost)) and the RestApi vhost
handling never panics.
1 participant
Purpose
$subject. With this change, we can keep handlers.go file cleaner.
Goals
Approach
User stories
Documentation
Automation tests
Security checks
Samples
Related PRs
Test environment
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.