Support Major Versions of Policies in API YAML#913
Conversation
WalkthroughThis pull request transitions the gateway's policy versioning model from semantic versioning (e.g., v1.0.0) to major-only versioning (e.g., v1), adding resolver logic to map major versions to full semantic versions at runtime and updating all examples and integration tests accordingly. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller as Gateway Controller
participant Validator as Policy Validator
participant Resolver as Version Resolver
participant Definitions as Policy Definitions
Client->>Controller: Submit RestApi with major-only policy version (e.g., v1)
Controller->>Definitions: Load policy definitions from image
Controller->>Validator: Validate API configuration
Validator->>Resolver: resolvePolicyVersion(policy_name, "v1")
Resolver->>Definitions: Lookup policy "policy_name" with major version v1
Definitions-->>Resolver: Return full semver (e.g., v1.0.0)
Resolver-->>Validator: Return resolved version
Validator->>Validator: Validate against definitions[resolved_version]
Validator-->>Controller: Validation complete
Controller->>Controller: convertAPIPolicyToModel with resolved_version
Controller-->>Client: Policy configuration ready (uses v1.0.0 internally)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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 |
|
Let's reject APIs with policy versions |
O-sura
force-pushed
the
policy-version-fix
branch
from
81a4ae1 to
e3bbae7
Compare
O-sura
requested review from
Krishanx92,
malinthaprasan and
pubudu538
as code owners
![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 (2)
concepts/api-yaml-specification.md (1)
57-63:⚠️ Potential issue | 🟡 MinorPolicy examples are missing the required
versionfield.The documentation correctly states that
versionis required for policy configuration, but the examples throughout the document (e.g., lines 84-86, 104-106, 118-123, 149-152, 157-162, etc.) do not include theversionfield in their policy definitions.Consider updating all policy examples to include the version field for consistency:
requestPolicies: - name: apiKey + version: v1 params: header: api-keygateway/gateway-controller/cmd/controller/main.go (1)
551-565:⚠️ Potential issue | 🟠 MajorGuard against zero-value policy instances when version resolution fails.
apiPoliciesonly contains resolved entries; appendingapiPolicies[name]without anokcheck inserts empty policy instances when resolution fails, which can corrupt the policy chain.🛠️ Proposed fix
- if _, exists := addedNames[apiPolicy.Name]; !exists { - finalPolicies = append(finalPolicies, apiPolicies[apiPolicy.Name]) - } + if _, exists := addedNames[apiPolicy.Name]; !exists { + if apiPolicyInstance, ok := apiPolicies[apiPolicy.Name]; ok { + finalPolicies = append(finalPolicies, apiPolicyInstance) + } + } @@ - for _, p := range *apiData.Policies { - finalPolicies = append(finalPolicies, apiPolicies[p.Name]) - } + for _, p := range *apiData.Policies { + if apiPolicyInstance, ok := apiPolicies[p.Name]; ok { + finalPolicies = append(finalPolicies, apiPolicyInstance) + } + }
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/config/policy_validator.go`:
- Around line 81-90: The inline comment above the call to
pv.resolvePolicyVersion (around the call using policy.Name, policy.Version) is
incorrect: update it to state that full semantic versions (vX.Y.Z) are rejected
while major-only versions (vX) are resolved to a single matching full version
from loaded policy definitions to match the behavior implemented in
ResolvePolicyVersion (the resolvePolicyVersion implementation that returns an
error for full semver patterns).
🧹 Nitpick comments (5)
gateway/gateway-controller/pkg/config/policy_validator.go (1)
21-24: Rebuild the gateway image after this gateway-controller change.Please run
cd gateway && make build-localto refresh the local images. As per coding guidelines: “gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components … rebuild Docker images usingcd gateway && make build-local”.gateway/gateway-controller/cmd/controller/main.go (1)
208-217: Rebuild gateway images after controller changes.As per coding guidelines
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local``.gateway/gateway-controller/pkg/api/handlers/handlers.go (2)
1662-1676: Lock released beforedefsis used in policy resolution loops.The read lock is acquired and released immediately after copying the reference to
s.policyDefinitions. While the map reference itself is safe to use after releasing the lock, if the map contents are modified concurrently (e.g., policy definitions reloaded), the iteration and lookups in the subsequent loops could see inconsistent data.However, reviewing the codebase, policy definitions appear to be loaded once at startup and not modified during runtime, so this is likely acceptable. If hot-reloading of policy definitions is ever added, the lock scope should be extended to cover the entire resolution phase.
1670-1674: Silent skip on policy version resolution failure may hinder debugging.When
ResolvePolicyVersionreturns an error, the policy is silently skipped with no logging. While validation should catch most issues earlier, this silent behavior could make troubleshooting difficult if a policy unexpectedly doesn't appear in the derived configuration.Consider adding debug-level logging when resolution fails:
🔧 Proposed logging addition
resolvedVersion, err := config.ResolvePolicyVersion(defs, p.Name, p.Version) if err != nil { + // Log at debug level - validation should have caught this earlier + slog.Debug("skipping policy due to version resolution failure", + slog.String("policy", p.Name), + slog.String("version", p.Version), + slog.Any("error", err)) continue }gateway/gateway-controller/pkg/config/policy_validator_test.go (1)
778-788: Consider usingstrings.Containsinstead of custom implementation.The
stringspackage is already imported (line 22) but not used for this helper. The customcontainsfunction is unnecessarily complex and could be replaced with the standard library function.♻️ Proposed simplification
func contains(s, substr string) bool { - return len(s) > 0 && len(substr) > 0 && (s == substr || len(s) >= len(substr) && s[:len(substr)] == substr || stringContains(s, substr)) -} - -func stringContains(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false + return strings.Contains(s, substr) }
O-sura
force-pushed
the
policy-version-fix
branch
from
e3bbae7 to
c855d29
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@gateway/gateway-builder/go.mod`:
- Line 13: The go.mod contains a duplicate requirement for the module
github.com/stretchr/testify (appearing in both the direct require block and the
indirect/other require block); remove the duplicate entry so the module is only
declared once (keep it in the intended direct require block where other test
deps live), then run go mod tidy to clean up go.sum and verify the module is
resolved; update any comments if needed.
In `@gateway/gateway-controller/cmd/controller/main.go`:
- Around line 521-530: The code builds apiPolicies map but skips unresolved
versions and later uses apiPolicies[apiPolicy.Name] without checking presence,
which can insert zero-value policy instances; update the read sites to use the
comma-ok pattern (e.g., when accessing apiPolicies by key) and only append/merge
when the key exists, and optionally log or return the ResolvePolicyVersion error
from ResolvePolicyVersion/convertAPIPolicyToModel so failures are visible;
ensure all locations that access apiPolicies (including the loop that appends
using apiPolicy.Name) perform this existence check before using the value.
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 1669-1675: The loop over cfg.GetPolicies() uses
config.ResolvePolicyVersion and convertAPIPolicy to populate the apiPolicies map
but when ResolvePolicyVersion fails the code continues and later merges read
apiPolicies[apiPolicy.Name] without checking presence, which can inject
zero-value policies; update the merge sites to perform an existence check (e.g.
val, ok := apiPolicies[apiPolicy.Name]) before appending/merging, skip and log
the unresolved policy when ok is false, and ensure the same guard is applied at
all three affected locations that reference apiPolicies (the loops that call
config.ResolvePolicyVersion, convertAPIPolicy, and the subsequent merges where
apiPolicies[apiPolicy.Name] is used).
- Around line 1662-1666: The code releases s.policyDefMu RLock before using defs
(s.policyDefinitions), which risks a race because defs is a shallow copy of the
map header; either hold the RLock across the subsequent policy resolution loop
or, while the RLock is held, deep-copy the map entries into a local map/slice
(same approach as ListPolicies at the earlier block) and then release the lock;
update the block around s.policyDefMu.RLock()/RUnlock() and the resolution code
to use one of these two safe patterns referencing s.policyDefMu,
s.policyDefinitions, and the existing ListPolicies copy pattern.
🧹 Nitpick comments (4)
gateway/examples/llm-proxy.yaml (1)
29-80: Remember to rebuild gateway images after this change.
Please runcd gateway && make build-localsince this file is in the gateway component path. As per coding guidelines, “gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local”.gateway/examples/self-signed-secure-api.yaml (1)
18-38: Rebuild gateway images after this gateway change.Please run
cd gateway && make build-localas part of the update process.As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/config/policy_validator.go (1)
114-116: Remove empty else block.The empty
elseblock serves no purpose and should be removed for cleaner code.🧹 Suggested fix
if policyDef.Parameters != nil { // If params is nil, validate against an empty object to enforce required fields params := make(map[string]interface{}) if policy.Params != nil { params = *policy.Params } schemaErrs := pv.validatePolicyParams(params, *policyDef.Parameters, fieldPath+".params") errors = append(errors, schemaErrs...) - } else { - }gateway/gateway-controller/cmd/controller/main.go (1)
208-217: Rebuild gateway images after gateway-controller changesRun
cd gateway && make build-localto refresh local gateway images for this change set.
As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images usingcd gateway && make build-local.
O-sura
force-pushed
the
policy-version-fix
branch
from
e7b5535 to
de7627a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
concepts/api-yaml-specification.md (1)
57-63:⚠️ Potential issue | 🟡 MinorPolicy examples still omit required
version.
The spec now requires a major-only policy version, but the examples below still show policy blocks withoutversion, which will produce invalid YAML if copied.📝 Suggested updates to example snippets
@@ - method: GET path: /{country_code}/{city} requestPolicies: - name: apiKey + version: v1 params: header: api-key @@ - method: GET path: / requestPolicies: - name: jwt + version: v1 params: header: Authorization @@ - method: GET path: /{id} requestPolicies: - name: jwt + version: v1 params: header: Authorization @@ - method: POST path: / requestPolicies: - name: jwt + version: v1 params: header: Authorization - name: rateLimit + version: v1 params: limit: 100 window: 60s @@ - method: PUT path: /{id} requestPolicies: - name: jwt + version: v1 params: header: Authorization @@ - method: DELETE path: /{id} requestPolicies: - name: jwt + version: v1 params: header: AuthorizationPlease mirror the same
version: v1addition in the “Common Policy Types” snippets as well.
🧹 Nitpick comments (5)
gateway/gateway-controller/pkg/utils/api_key_test.go (1)
731-774: Rebuild gateway images after controller updates.
Please runcd gateway && make build-localto refresh gateway images for these gateway-controller changes.As per coding guidelines:
gateway/**/*.{go,yaml,yml,Dockerfile}requires rebuilding Docker images usingcd gateway && make build-local.gateway/gateway-controller/pkg/config/policy_validator_test.go (1)
778-788: Simplifycontainshelper usingstrings.Contains.The custom
containshelper has complex logic that reimplementsstrings.Contains. Since thestringspackage is already imported (line 22), consider simplifying:♻️ Suggested refactor
func contains(s, substr string) bool { - return len(s) > 0 && len(substr) > 0 && (s == substr || len(s) >= len(substr) && s[:len(substr)] == substr || stringContains(s, substr)) -} - -func stringContains(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false + return strings.Contains(s, substr) }gateway/gateway-controller/pkg/config/policy_validator.go (1)
114-116: Remove empty else block.The empty
elseblock serves no purpose and should be removed.🧹 Suggested fix
// Validate policy parameters against JSON schema if schema is defined if policyDef.Parameters != nil { // If params is nil, validate against an empty object to enforce required fields params := make(map[string]interface{}) if policy.Params != nil { params = *policy.Params } schemaErrs := pv.validatePolicyParams(params, *policyDef.Parameters, fieldPath+".params") errors = append(errors, schemaErrs...) - } else { - }gateway/gateway-controller/cmd/controller/main_test.go (1)
45-65: Test inputs should use major-only versions to reflect the actual API contract.While
convertAPIPolicyToModelignores thep.Versionfield and uses only theresolvedVersionparameter, the test inputs at lines 49, 70, 95, 117 should be updated to use major-only versions (e.g.,v1,v0instead ofv1.0.0,v0.1.0) to match the API's validated input format. This makes the tests more realistic and consistent with the PR's objective of using major-only versions at the API level, even though the current tests are technically correct in their use ofresolvedVersion.🔧 Suggested fix
{ name: "Basic policy without params", policy: api.Policy{ Name: "rate-limit", - Version: "v1.0.0", + Version: "v1", }, attachedTo: policy.LevelAPI,gateway/gateway-controller/cmd/controller/main.go (1)
208-217: Reminder: rebuild gateway images after these changes.
Please runcd gateway && make build-localto refresh the gateway artifacts.
As per coding guidelines:gateway/**/*.{go,yaml,yml,Dockerfile}— rebuild Docker images usingcd gateway && make build-local.
O-sura
force-pushed
the
policy-version-fix
branch
from
de7627a to
2a4b50e
Compare
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)
concepts/api-yaml-specification.md (1)
57-63:⚠️ Potential issue | 🟡 MinorDocumentation examples are inconsistent with the new required
versionfield.The
versionfield is marked as Required in the Policy Configuration table, but none of the policy examples in this file include it (e.g., lines 84–86, 104–106, 121–124, 149–152, etc.). Users following these examples will produce invalid API YAML.Consider updating the examples to include the
versionfield, e.g.:requestPolicies: - name: rateLimit + version: v1 params: limit: 100 window: 60s
🤖 Fix all issues with AI agents
In `@gateway/gateway-controller/pkg/api/handlers/handlers.go`:
- Around line 1674-1678: The code is silently dropping policies when
ResolvePolicyVersion rejects full-semver strings (e.g., "v1.0.0") and the
handler just continues; update the resolution flow so full-semver inputs are
accepted: modify ResolvePolicyVersion in policy_validator.go to treat an exact
match to an installed definition as valid (return the same version instead of
error), or alternatively allow pass-through of full-semver from the caller; then
update the handler sites that call ResolvePolicyVersion (the spots assigning
resolvedVersion and calling convertAPIPolicy to populate apiPolicies) to no
longer continue on full-semver mismatch—ensure they use the returned version
when it matches a definition and only skip on genuine errors. Reference symbols:
ResolvePolicyVersion, convertAPIPolicy, apiPolicies, and the handler code that
assigns resolvedVersion and calls convertAPIPolicy.
🧹 Nitpick comments (4)
gateway/it/features/model-round-robin.feature (1)
36-37: Consider adding a negative test scenario for full semantic version rejection.Per the PR reviewer's comment, full semantic versions (e.g.,
v0.1.0) should now be rejected rather than silently accepted. A scenario that deploys an API withversion: v0.1.0and asserts a deployment/validation error would guard against regressions and confirm the new contract. This could live in this feature file or in the broader policy-version validation test suite.gateway/gateway-controller/pkg/config/policy_validator.go (1)
114-116: Remove emptyelseblock.This is dead code that adds no behavior and reduces readability.
♻️ Proposed fix
if policyDef.Parameters != nil { // If params is nil, validate against an empty object to enforce required fields params := make(map[string]interface{}) if policy.Params != nil { params = *policy.Params } schemaErrs := pv.validatePolicyParams(params, *policyDef.Parameters, fieldPath+".params") errors = append(errors, schemaErrs...) - } else { - }gateway/gateway-controller/pkg/config/policy_validator_test.go (1)
773-789: Replace hand-rolledcontains/stringContainswithstrings.Contains.Since
stringsis now imported (line 22), these custom helpers are redundant. Line 601 already usesstrings.Containsdirectly, while other call sites (e.g., lines 139, 456, 711, 768) use the customcontains— an unnecessary inconsistency.♻️ Proposed fix
Replace all calls to
contains(s, substr)withstrings.Contains(s, substr)and remove both helper functions:-func contains(s, substr string) bool { - return len(s) > 0 && len(substr) > 0 && (s == substr || len(s) >= len(substr) && s[:len(substr)] == substr || stringContains(s, substr)) -} - -func stringContains(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}Then update call sites, e.g.:
- if len(errors) > 0 && !contains(errors[0].Message, "not found") { + if len(errors) > 0 && !strings.Contains(errors[0].Message, "not found") {gateway/gateway-controller/cmd/controller/main.go (1)
208-218: Remember to rebuild gateway images after gateway-controller changes.Please run the local build step to keep the gateway image in sync with these changes.
As per coding guidelines: "gateway/**/*.{go,yaml,yml,Dockerfile}: When modifying code in gateway components (gateway-controller, policy-engine, router, etc., excluding gateway/it/), rebuild Docker images using
cd gateway && make build-local."
3 participants
Purpose
This change updates the gateway-controller to accept policy versions in API YAML as either full semantic versions (e.g., v1.0.0) or major-only versions (e.g., v1), resolving major-only values to the unique installed full version from the loaded policy definitions. The policy validator now performs this resolution before lookup and schema validation. The derived policy configuration and xDS flow continue to use resolved full versions end-to-end, preserving existing behavior in the policy engine.
Summary by CodeRabbit
New Features
Documentation