Skip to content

Conversation

@Tharsanan1
Copy link
Contributor

@Tharsanan1 Tharsanan1 commented Jan 15, 2026

$subject

Summary by CodeRabbit

  • New Features

    • Policies now track their attachment level (API-wide or route-specific).
    • Rate limiting now supports per-route quota enforcement with independent limits per route.
  • Tests

    • Added test scenarios validating per-route rate limiting and quota isolation.
    • Enhanced test coverage for policy attachment behavior and quota enforcement patterns across routes.

✏️ Tip: You can customize this high-level summary in your review settings.

…Limit (#1)

* Implement AttachedTo metadata for policy differentiation

- Updated SDK PolicyMetadata to include AttachedTo field
- Updated Gateway Controller to inject AttachedTo parameter ("api" or "route") during policy conversion
- Updated Policy Engine to extract AttachedTo from parameters and set it in PolicyMetadata
- Updated Basic Rate Limit policy to use "apiname" key extractor when AttachedTo is "api"

* Implement AttachedTo metadata for policy differentiation

- Updated SDK PolicyMetadata to include AttachedTo field
- Updated Gateway Controller to inject AttachedTo parameter ("api" or "route") during policy conversion
- Updated Policy Engine to extract AttachedTo from parameters and set it in PolicyMetadata
- Updated Basic Rate Limit policy to use "apiname" key extractor when AttachedTo is "api"
- Updated integration tests to verify new behavior and fix regressions caused by scoping changes

* Resolve conflicts and fix tests for rate limit scoping

- Merged latest changes from ratelimit-latest.
- Updated `gateway/it/features/basic-ratelimit.feature` to align with the new rate limiting behavior:
  - API-level attachment now implies a shared bucket (per API).
  - Route-level attachment (per operation) implies independent buckets.
  - Adjusted test expectations and configurations accordingly.
- Verified that all integration tests pass.

* Resolve conflicts and update rate limit tests

- Merged latest changes from ratelimit-latest.
- Updated `gateway/it/features/basic-ratelimit.feature` to align with the new rate limiting behavior:
  - API-level attachment now implies a shared bucket (per API).
  - Route-level attachment (per operation) implies independent buckets.
  - Adjusted test expectations and configurations accordingly.
- Verified that all integration tests pass.

---------

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: Tharsanan1 <ktharsanan@gmail.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 15, 2026

Walkthrough

The changes introduce an attachedTo metadata field that tracks whether policies are attached at the API level or route/operation level. This metadata is populated during policy conversion in the controller, propagated through the policy engine, and utilized by policy implementations (e.g., rate limiting) to enforce scope-specific behavior.

Changes

Cohort / File(s) Summary
Policy Metadata Extension
sdk/gateway/policy/v1alpha/interface.go
Added AttachedTo string field to PolicyMetadata struct to track policy attachment scope.
Policy Engine Metadata Population
gateway/policy-engine/internal/xdsclient/handler.go
Modified buildPolicyChain to read attachedTo from policy parameters and populate metadata.AttachedTo before creating policy instance.
Policy Conversion with Attachment
gateway/gateway-controller/cmd/controller/main.go, gateway/gateway-controller/pkg/api/handlers/handlers.go
Updated convertAPIPolicy to accept attachedTo parameter and inject it into policy parameters; all call sites now pass "api" for API-level and "route" for operation-level policies.
Rate Limit Policy Scope Logic
gateway/policies/basic-ratelimit/v0.1.0/basic_ratelimit.go
Modified transformToRatelimitParams to accept metadata and dynamically set key extraction type based on metadata.AttachedTo: "apiname" for API-level, "routename" otherwise.
Integration Tests
gateway/it/features/basic-ratelimit.feature
Restructured rate-limit-per-route scenario to use per-operation policies instead of API-wide policy; added new scenario validating per-route vs. API-level quota isolation and 429 enforcement.
Integration Tests
gateway/it/features/ratelimit.feature
Removed trailing 429 assertion in final scenario step.

Sequence Diagram

sequenceDiagram
    participant Controller as Gateway Controller
    participant Engine as Policy Engine
    participant PolicyImpl as Rate Limit Policy
    
    Controller->>Controller: convertAPIPolicy(policy, attachedTo)
    Note over Controller: Insert attachedTo into Parameters
    Controller->>Engine: Policy with attachedTo in Parameters
    
    Engine->>Engine: buildPolicyChain(policyConfig)
    Engine->>Engine: Read attachedTo from Parameters
    Engine->>Engine: Populate metadata.AttachedTo
    Note over Engine: metadata = {AttachedTo: "api"|"route"}
    
    Engine->>PolicyImpl: CreateInstance(metadata)
    PolicyImpl->>PolicyImpl: transformToRatelimitParams(params, metadata)
    alt metadata.AttachedTo == "api"
        PolicyImpl->>PolicyImpl: keyExtraction = "apiname"
    else
        PolicyImpl->>PolicyImpl: keyExtraction = "routename"
    end
    Note over PolicyImpl: Enforce limits per scope
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 Hop, hop—policies now find their place,
API or route, each has its space,
AttachedTo marks the scope so true,
Per-limit quotas, fresh and new,
Metadata magic makes it sing! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is minimal, containing only '$subject' placeholder text and does not follow the required template structure with Purpose, Goals, Approach, and other sections. Fill in the description template with Purpose, Goals, Approach, test coverage, security checks, and other required sections to provide adequate context for reviewers.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding AttachedTo metadata support for policies across the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
gateway/gateway-controller/pkg/api/handlers/handlers.go (1)

1756-1776: Implementation is correct; consider extracting to reduce duplication.

The function correctly injects attachedTo into the policy parameters. However, this function is nearly identical to convertAPIPolicyToModel in gateway/gateway-controller/cmd/controller/main.go. Consider extracting this to a shared utility to avoid maintaining duplicate code.


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d422545 and c99aa06.

📒 Files selected for processing (7)
  • gateway/gateway-controller/cmd/controller/main.go
  • gateway/gateway-controller/pkg/api/handlers/handlers.go
  • gateway/it/features/basic-ratelimit.feature
  • gateway/it/features/ratelimit.feature
  • gateway/policies/basic-ratelimit/v0.1.0/basic_ratelimit.go
  • gateway/policy-engine/internal/xdsclient/handler.go
  • sdk/gateway/policy/v1alpha/interface.go
💤 Files with no reviewable changes (1)
  • gateway/it/features/ratelimit.feature
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Thenujan-Nagaratnam
Repo: wso2/api-platform PR: 385
File: gateway/policies/url-guardrail/v0.1.0/urlguardrail.go:261-281
Timestamp: 2025-12-12T04:05:38.530Z
Learning: In the api-platform repository, when adding new guardrail policies, security enhancements that are not present in existing/old policies are deferred to future versions to maintain consistency across policy implementations.
📚 Learning: 2026-01-13T15:42:24.281Z
Learnt from: O-sura
Repo: wso2/api-platform PR: 665
File: gateway/policy-engine/internal/analytics/analytics.go:277-277
Timestamp: 2026-01-13T15:42:24.281Z
Learning: In gateway/policy-engine/internal/analytics/analytics.go, when processing AI-related metadata for LLM providers, the aiMetadata.VendorVersion field should be populated from APIVersionKey (the API's version) rather than AIProviderAPIVersionMetadataKey (the AI provider's API version). This is intentional based on how LLM providers are handled in the system.

Applied to files:

  • gateway/policies/basic-ratelimit/v0.1.0/basic_ratelimit.go
🧬 Code graph analysis (3)
gateway/policies/basic-ratelimit/v0.1.0/basic_ratelimit.go (1)
sdk/gateway/policy/v1alpha/interface.go (1)
  • PolicyMetadata (5-20)
gateway/gateway-controller/cmd/controller/main.go (2)
gateway/gateway-controller/pkg/api/generated/generated.go (1)
  • Policy (1171-1183)
sdk/gateway/policyengine/v1/config.go (1)
  • PolicyInstance (18-37)
gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
gateway/gateway-controller/pkg/api/generated/generated.go (1)
  • Policy (1171-1183)
sdk/gateway/policyengine/v1/config.go (1)
  • PolicyInstance (18-37)
🔇 Additional comments (12)
sdk/gateway/policy/v1alpha/interface.go (1)

17-19: LGTM!

The new AttachedTo field is a clean addition to PolicyMetadata. The comment clearly documents its purpose and expected values. This field will enable policy implementations to differentiate between API-level and route-level attachments.

gateway/policies/basic-ratelimit/v0.1.0/basic_ratelimit.go (2)

41-41: LGTM!

Correctly passes the metadata to transformToRatelimitParams to enable AttachedTo-aware key extraction.


55-70: LGTM!

The key extractor logic correctly differentiates between API-level and route-level attachments:

  • "api""apiname" key extractor for shared bucket across all routes
  • Otherwise → "routename" key extractor for per-route isolation

This aligns with the PR objectives for rate limiting behavior based on policy attachment scope.

gateway/policy-engine/internal/xdsclient/handler.go (1)

236-241: LGTM!

The extraction logic correctly propagates the attachedTo parameter from policy configuration into PolicyMetadata. The double type assertion pattern safely handles cases where the parameter may be missing or of unexpected type, defaulting to an empty string which is acceptable behavior.

gateway/gateway-controller/pkg/api/handlers/handlers.go (3)

1608-1611: LGTM!

API-level policies are correctly tagged with "api" for the attachedTo parameter.


1631-1635: LGTM!

Channel-level policies in WebSub APIs are correctly tagged with "route" for route-level attachment behavior.


1676-1680: LGTM!

Operation-level policies in REST APIs are correctly tagged with "route" for route-level attachment behavior.

gateway/gateway-controller/cmd/controller/main.go (3)

439-443: LGTM!

API-level policies are correctly tagged with "api" for the attachedTo parameter during startup policy derivation.


454-458: LGTM!

Operation-level policies are correctly tagged with "route" for route-level attachment behavior.


532-551: LGTM!

The function correctly injects attachedTo into the policy parameters. As noted in the review of handlers.go, this function is duplicated. The existing TODO comment at line 1603 in handlers.go acknowledges duplication in buildStoredPolicyFromAPI; this convertAPIPolicyToModel function could be included in that refactoring effort.

gateway/it/features/basic-ratelimit.feature (2)

164-179: LGTM!

The per-route policy configuration correctly tests independent rate limit buckets. Each operation defines its own policy, and the subsequent test steps (lines 184-199) properly validate that route1 and route2 maintain separate quotas.


240-318: Well-designed test scenario for attachment-level scoping.

This scenario provides thorough coverage of the new AttachedTo metadata behavior:

  • Route-level isolation (resource-b): Independent bucket with limit 3
  • API-level sharing (resource-a/c): Shared bucket with limit 5 across both operations
  • Bucket independence: Verifies route-level and API-level buckets don't interfere

The /health endpoint having a route-level policy with limit 100 is a good design choice to prevent the readiness check from consuming the API-level quota being tested.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant