Support exact endpoint configuration in interceptor policy#3319
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces support for custom resource paths in interceptor configurations. The implementation adds a 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| // validating the basepath to be same for all upstreams of an api | ||
| if strings.TrimSuffix(ep.Basepath, "/") != basePath { | ||
| if strings.TrimSuffix(ep.Basepath, "/") != strings.TrimSuffix(basePath, "/") { | ||
| return nil, nil, errors.New("endpoint basepath mismatched for " + ep.RawURL + ". expected : " + basePath + " but found : " + ep.Basepath) | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| // validating the basepath to be same for all upstreams of an api | |
| if strings.TrimSuffix(ep.Basepath, "/") != basePath { | |
| if strings.TrimSuffix(ep.Basepath, "/") != strings.TrimSuffix(basePath, "/") { | |
| return nil, nil, errors.New("endpoint basepath mismatched for " + ep.RawURL + ". expected : " + basePath + " but found : " + ep.Basepath) | |
| } | |
| // validating the basepath to be same for all upstreams of an api | |
| if strings.TrimSuffix(ep.Basepath, "/") != strings.TrimSuffix(basePath, "/") { | |
| log.Errorf("Endpoint basepath mismatch for %s. Expected: %s, Found: %s", ep.RawURL, basePath, ep.Basepath) | |
| return nil, nil, errors.New("endpoint basepath mismatched for " + ep.RawURL + ". expected : " + basePath + " but found : " + ep.Basepath) | |
| } |
| if bp, ok := paramMap[constants.InterceptorBasePath]; ok { | ||
| if bpStr, ok := bp.(string); ok && bpStr != "" { | ||
| resourcePath = &bpStr | ||
| } | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
| if bp, ok := paramMap[constants.InterceptorBasePath]; ok { | |
| if bpStr, ok := bp.(string); ok && bpStr != "" { | |
| resourcePath = &bpStr | |
| } | |
| } | |
| if bp, ok := paramMap[constants.InterceptorBasePath]; ok { | |
| if bpStr, ok := bp.(string); ok && bpStr != "" { | |
| log.Debug("Setting interceptor resource path: ", bpStr) | |
| resourcePath = &bpStr | |
| } | |
| } |
| if len(endpoints) > 0 { | ||
| policyParameters[constants.InterceptorEndpoints] = endpoints | ||
| policyParameters[constants.InterceptorServiceIncludes] = requestInterceptor.Includes |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
| if len(endpoints) > 0 { | |
| policyParameters[constants.InterceptorEndpoints] = endpoints | |
| policyParameters[constants.InterceptorServiceIncludes] = requestInterceptor.Includes | |
| if len(endpoints) > 0 { | |
| log.Debugf("Adding request interceptor for backend: %s with %d endpoints", backendName, len(endpoints)) | |
| policyParameters[constants.InterceptorEndpoints] = endpoints |
| if len(endpoints) > 0 { | ||
| policyParameters[constants.InterceptorEndpoints] = endpoints | ||
| policyParameters[constants.InterceptorServiceIncludes] = responseInterceptor.Includes |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
| if len(endpoints) > 0 { | |
| policyParameters[constants.InterceptorEndpoints] = endpoints | |
| policyParameters[constants.InterceptorServiceIncludes] = responseInterceptor.Includes | |
| if len(endpoints) > 0 { | |
| log.Debugf("Adding response interceptor for backend: %s with %d endpoints", backendName, len(endpoints)) | |
| policyParameters[constants.InterceptorEndpoints] = endpoints |
| endpoints := model.GetEndpoints(types.NamespacedName{Namespace: namespace, Name: interceptor.BackendRef.Name}, | ||
| gatewayBackendMapping) | ||
| basePath := model.GetBackendBasePath(types.NamespacedName{Namespace: namespace, Name: interceptor.BackendRef.Name}, |
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
| endpoints := model.GetEndpoints(types.NamespacedName{Namespace: namespace, Name: interceptor.BackendRef.Name}, | |
| gatewayBackendMapping) | |
| basePath := model.GetBackendBasePath(types.NamespacedName{Namespace: namespace, Name: interceptor.BackendRef.Name}, | |
| endpoints := model.GetEndpoints(types.NamespacedName{Namespace: namespace, Name: interceptor.BackendRef.Name}, | |
| gatewayBackendMapping) | |
| logger.Info("Retrieved endpoints for interceptor backend", "namespace", namespace, "backendName", interceptor.BackendRef.Name) |
| var resourcePath *string | ||
| if basePath != "" { | ||
| resourcePath = &basePath | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
| var resourcePath *string | |
| if basePath != "" { | |
| resourcePath = &basePath | |
| } | |
| if basePath != "" { | |
| resourcePath = &basePath | |
| logger.Debug("Setting resource path for interceptor endpoint", "basePath", basePath, "clusterName", clusterName) | |
| } |
| public void waitForMinute(int minute) throws InterruptedException { | ||
| Thread.sleep(minute * 1000); | ||
| Thread.sleep(minute * 60 * 1000); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
| public void waitForMinute(int minute) throws InterruptedException { | |
| Thread.sleep(minute * 1000); | |
| Thread.sleep(minute * 60 * 1000); | |
| } | |
| @Then("I wait for {int} minute") | |
| public void waitForMinute(int minute) throws InterruptedException { | |
| log.debug("Waiting for {} minute(s)", minute); | |
| Thread.sleep(minute * 60 * 1000); | |
| } |
| check ep0.attach(interceptorService, "/api/v1"); | ||
| check ep0.attach(customPathInterceptorService, "/custom/v2"); | ||
| check ep0.'start(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
| check ep0.attach(interceptorService, "/api/v1"); | |
| check ep0.attach(customPathInterceptorService, "/custom/v2"); | |
| check ep0.'start(); | |
| check ep0.attach(interceptorService, "/api/v1"); | |
| check ep0.attach(customPathInterceptorService, "/custom/v2"); | |
| log:printInfo("Attached interceptor services to listener on port 8080"); | |
| check ep0.'start(); |
| check ep1.attach(interceptorService, "/api/v1"); | ||
| check ep1.attach(customPathInterceptorService, "/custom/v2"); | ||
| check ep1.'start(); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
| check ep1.attach(interceptorService, "/api/v1"); | |
| check ep1.attach(customPathInterceptorService, "/custom/v2"); | |
| check ep1.'start(); | |
| check ep1.attach(interceptorService, "/api/v1"); | |
| check ep1.attach(customPathInterceptorService, "/custom/v2"); | |
| log:printInfo("Attached interceptor services to listener on port 8444"); | |
| check ep1.'start(); |
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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)
adapter/internal/oasparser/model/adapter_internal_api.go (1)
1781-1848:⚠️ Potential issue | 🟡 Minor
ResourcePathfield not populated from vendor-extension interceptors.
GetInterceptorbuildsInterceptEndpointfromx-wso2-request-interceptor/x-wso2-response-interceptorvendor extensions but does not setResourcePath. The method explicitly extracts and discards the basepath from theserviceURLwith a warning (lines 1798-1801), unlike operation-level interceptors sourced throughAPIPolicywhich extract and assign the basepath toResourcePath(seeapi_operation.go:164-169).If path-based routing or resource path awareness is required for vendor-extension interceptors, consider extracting the
serviceURLbasepath and assigning it toResourcePathinstead of discarding it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adapter/internal/oasparser/model/adapter_internal_api.go` around lines 1781 - 1848, GetInterceptor currently parses serviceURL via getHTTPEndpoint but logs and ignores endpoint.Basepath; populate the InterceptEndpoint.ResourcePath from that basepath instead of discarding it. Update the return value in GetInterceptor to set ResourcePath: endpoint.Basepath (or empty string if no basepath) when building the InterceptEndpoint so vendor-extension interceptors carry the resource path similar to operation-level interceptors (see api operation handling in APIOperation). Also ensure the code continues to warn if a basepath is present but still assigns it to ResourcePath.
🧹 Nitpick comments (1)
adapter/internal/oasparser/model/http_route.go (1)
129-140: LGTM: consistent injection ofInterceptorBasePathfor both request and response interceptors.
basePathis derived from the same resolved backend used to buildendpointsand is only attached when endpoints exist, matching the downstream consumer inapi_operation.go.Minor nit:
GetBackendBasePathis invoked before thelen(endpoints) > 0guard and its result is discarded when no endpoints are present. Moving the call inside the guard would be slightly cleaner, but it's a no-op read so this is optional.Also applies to: 152-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adapter/internal/oasparser/model/http_route.go` around lines 129 - 140, Move the GetBackendBasePath call inside the endpoints existence guard to avoid calling it when endpoints are empty: in the block that already calls GetEndpoints(backendName, backendMapping) and checks if len(endpoints) > 0, call GetBackendBasePath(backendName, backendMapping) there and then set policyParameters[constants.InterceptorBasePath] = basePath before appending the Policy to policies.Request; apply the same change to the analogous response interceptor block (the code around where Policy with PolicyName constants.PolicyResponseInterceptor is constructed, currently around the second block similar to lines 152-163).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/k8s-resources/interceptor-service.yaml`:
- Line 62: The manifest uses a personal Docker Hub image with a floating tag
("thivindu/interceptor_service:latest"); replace that image reference with an
organization-owned registry image and pin to an immutable tag or digest (e.g.,
orgName/interceptor_service:vX.Y.Z or orgName/interceptor_service@sha256:...) so
tests are reproducible; update the same image string wherever it appears in the
related test manifests (the other two test CR artifacts that reference the
identical image) to the new org-scoped, pinned tag/digest.
---
Outside diff comments:
In `@adapter/internal/oasparser/model/adapter_internal_api.go`:
- Around line 1781-1848: GetInterceptor currently parses serviceURL via
getHTTPEndpoint but logs and ignores endpoint.Basepath; populate the
InterceptEndpoint.ResourcePath from that basepath instead of discarding it.
Update the return value in GetInterceptor to set ResourcePath: endpoint.Basepath
(or empty string if no basepath) when building the InterceptEndpoint so
vendor-extension interceptors carry the resource path similar to operation-level
interceptors (see api operation handling in APIOperation). Also ensure the code
continues to warn if a basepath is present but still assigns it to ResourcePath.
---
Nitpick comments:
In `@adapter/internal/oasparser/model/http_route.go`:
- Around line 129-140: Move the GetBackendBasePath call inside the endpoints
existence guard to avoid calling it when endpoints are empty: in the block that
already calls GetEndpoints(backendName, backendMapping) and checks if
len(endpoints) > 0, call GetBackendBasePath(backendName, backendMapping) there
and then set policyParameters[constants.InterceptorBasePath] = basePath before
appending the Policy to policies.Request; apply the same change to the analogous
response interceptor block (the code around where Policy with PolicyName
constants.PolicyResponseInterceptor is constructed, currently around the second
block similar to lines 152-163).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ceaa2bc6-9b72-4d60-ba20-ba4d3ab5a3da
📒 Files selected for processing (31)
adapter/internal/interceptor/interceptor.goadapter/internal/oasparser/constants/constants.goadapter/internal/oasparser/envoyconf/routes_with_clusters.goadapter/internal/oasparser/model/adapter_internal_api.goadapter/internal/oasparser/model/api_operation.goadapter/internal/oasparser/model/http_route.goadapter/internal/operator/synchronizer/gateway_synchronizer.gogateway/router/resources/interceptor/lib/interceptor.luatest/cucumber-tests/CRs/agent-artifacts.yamltest/cucumber-tests/CRs/artifacts.yamltest/cucumber-tests/src/test/java/org/wso2/apk/integration/api/BaseSteps.javatest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withCustomPathOperationRequestAndResponse.apk-conftest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withCustomPathOperationRequestDefaultResponse.apk-conftest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withCustomPathOperationRequestInterceptor.apk-conftest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withCustomPathOperationResponseInterceptor.apk-conftest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withCustomPathRequestAndResponse.apk-conftest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withCustomPathRequestDefaultResponse.apk-conftest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withCustomPathRequestInterceptor.apk-conftest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withCustomPathResponseInterceptor.apk-conftest/cucumber-tests/src/test/resources/artifacts/apk-confs/interceptors/withDefaultRequestCustomPathResponse.apk-conftest/cucumber-tests/src/test/resources/tests/api/APIDefinitionEndpoint.featuretest/cucumber-tests/src/test/resources/tests/api/GlobalInterceptor.featuretest/cucumber-tests/src/test/resources/tests/api/Interceptor.featuretest/cucumber-tests/src/test/resources/tests/api/InterceptorCustomPath.featuretest/cucumber-tests/src/test/resources/tests/api/resourceInterceptor.featuretest/cucumber-tests/src/test/resources/tests/api/resourceInterceptorCustomPath.featuretest/interceptor-backend/ballerina/Dependencies.tomltest/interceptor-backend/ballerina/Dockerfiletest/interceptor-backend/ballerina/customPathInterceptor.baltest/interceptor-backend/ballerina/init.baltest/k8s-resources/interceptor-service.yaml
| spec: | ||
| containers: | ||
| - image: "tharindu1st/interceptor_service:latest" | ||
| - image: "thivindu/interceptor_service:latest" |
There was a problem hiding this comment.
Image now points to a personal Docker Hub namespace.
thivindu/interceptor_service:latest is a personal account repository, and the tag is floating (:latest). For a shared test manifest, consider:
- Publishing under an organization-owned registry (e.g., the WSO2 namespace used previously) so the image lifecycle is not tied to an individual contributor.
- Pinning to an immutable tag or digest to make test runs reproducible and avoid silent drift.
The same concern applies to the identical image references in test/cucumber-tests/CRs/artifacts.yaml (line 164) and test/cucumber-tests/CRs/agent-artifacts.yaml (line 164).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/k8s-resources/interceptor-service.yaml` at line 62, The manifest uses a
personal Docker Hub image with a floating tag
("thivindu/interceptor_service:latest"); replace that image reference with an
organization-owned registry image and pin to an immutable tag or digest (e.g.,
orgName/interceptor_service:vX.Y.Z or orgName/interceptor_service@sha256:...) so
tests are reproducible; update the same image string wherever it appears in the
related test manifests (the other two test CR artifacts that reference the
identical image) to the new org-scoped, pinned tag/digest.
| [ballerina] | ||
| dependencies-toml-version = "2" | ||
| distribution-version = "2201.7.0" | ||
| distribution-version = "2201.5.0" |
There was a problem hiding this comment.
why are we downgrading the versions?
There was a problem hiding this comment.
The Ballerina version in the Ballerina.toml is 2201.5.0 and the Dependencies.toml is auto generated when we build the project so that it is set to the ballerina version that is used in the development enviorment.
|
|
||
| // validating the basepath to be same for all upstreams of an api | ||
| if strings.TrimSuffix(ep.Basepath, "/") != basePath { | ||
| if strings.TrimSuffix(ep.Basepath, "/") != strings.TrimSuffix(basePath, "/") { |
There was a problem hiding this comment.
this could affect other flows as well
3 participants
Purpose
This pull request introduces support for specifying the exact path of the interceptor service that the request should hit in the interceptor policy.
Sample policy config in apk-conf -
Related to - #3317
Goals
Support exact endpoint configuration in interceptor policy
Approach
This specifies a
resource_pathfor interceptor service calls, enabling more flexible routing to interceptor endpoints. The changes ensure that theresource_pathis propagated throughout the configuration, set in generated Lua scripts, and defaults to/api/v1/handle-requestor/api/v1/handle-responseif not provided. Additionally, the handling of base paths and their consistency across endpoints is improved.Interceptor Resource Path Support
ResourcePathfield to the interceptor configuration structures (HTTPCallConfig,InterceptEndpoint) and ensured it is populated from configuration and policy parameters. This allows specifying custom resource paths for interceptor service calls. [1] [2] [3] [4] [5] [6] [7]resource_path, defaulting to/api/v1/handle-requestor/api/v1/handle-responseif not specified. This ensures backward compatibility and correct routing. [1] [2]resource_pathin both request and response interceptor flows. [1] [2]Configuration and Policy Handling
interceptorBasePathconstant and updated the logic to extract and propagate the interceptor base path from backend mappings and policy parameters, ensuring the correct resource path is passed through the system. [1] [2] [3]Testing and Validation
apk-conffiles) that specify custom resource paths for various interceptor scenarios, ensuring the feature is thoroughly tested. [1] [2] [3] [4] [5]Documentation and Type Updates
resource_pathandauthority_headerfields in the interceptor service list. [1] [2]