Fixes - Config DS K8s token refresh and envoy ext_proc to fail closed#3320
Conversation
📝 WalkthroughSummaryThis pull request addresses two issues: ensuring the config deployer service uses the latest Kubernetes service account tokens, and making the Envoy external processing filter fail closed on errors. ChangesKubernetes Token Refresh
Envoy External Processing Behavior
ImpactThese changes ensure the config deployer maintains valid Kubernetes authentication when tokens are rotated, and strengthen the security posture by requiring successful external processing for all requests to proceed. WalkthroughThe pull request contains two modifications to authentication and failure handling. The first change updates the Envoy ext_proc filter configuration to set 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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}
| }, | ||
| FailureModeAllow: true, | ||
| FailureModeAllow: false, |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| }, | |
| FailureModeAllow: true, | |
| FailureModeAllow: false, | |
| }, | |
| FailureModeAllow: false, | |
| log.Info("External process HTTP filter configured with FailureModeAllow set to false") |
| isolated function getK8sAuthHeader() returns map<string|string[]> { | ||
| string|error token = io:fileReadString(k8sConfiguration.serviceAccountPath + "/token"); | ||
| if token is string { | ||
| return {"Authorization": "Bearer " + token}; | ||
| } | ||
| return {}; |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| isolated function getK8sAuthHeader() returns map<string|string[]> { | |
| string|error token = io:fileReadString(k8sConfiguration.serviceAccountPath + "/token"); | |
| if token is string { | |
| return {"Authorization": "Bearer " + token}; | |
| } | |
| return {}; | |
| isolated function getK8sAuthHeader() returns map<string|string[]> { | |
| string|error token = io:fileReadString(k8sConfiguration.serviceAccountPath + "/token"); | |
| if token is string { | |
| log:printDebug("Successfully read SA token from disk"); | |
| return {"Authorization": "Bearer " + token}; | |
| } | |
| log:printError("Failed to read SA token", 'error = token); | |
| return {}; | |
| } |
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
|
@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.
♻️ Duplicate comments (1)
runtime/config-deployer-service/ballerina/K8sClient.bal (1)
47-53:⚠️ Potential issue | 🟡 MinorLog token read failures for observability.
When
io:fileReadStringfails, the function silently returns an empty header map. Downstream calls will then receive 401 responses with no indication of the root cause. Adding a log statement on the error path will make token-rotation or mount issues diagnosable in production.🪵 Proposed fix to add error logging
isolated function getK8sAuthHeader() returns map<string|string[]> { string|error token = io:fileReadString(k8sConfiguration.serviceAccountPath + "/token"); if token is string { return {"Authorization": "Bearer " + token}; } + log:printError("Failed to read Kubernetes service account token", 'error = token); return {}; }Note: this will require adding
import ballerina/log;if not already imported in this module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/config-deployer-service/ballerina/K8sClient.bal` around lines 47 - 53, The getK8sAuthHeader function currently swallows errors from io:fileReadString and returns an empty header; update getK8sAuthHeader to log the error when token is not a string (i.e., when io:fileReadString returns an error) using ballerina/log so failures reading the service account token are recorded, then continue to return the empty map; add the required import statement (import ballerina/log;) if it is not already present.
🧹 Nitpick comments (3)
adapter/internal/oasparser/envoyconf/http_filters.go (1)
249-249: Fail-closed behavior correctly configured; verify operational readiness.Setting
FailureModeAllow: falsealigns with the PR objective and is consistent with theext_authzfilter configuration inadapter/internal/operator/gateway-api/translator/extauth.go(lines 110-135). This is a meaningful behavioral change: any ext_proc unavailability, timeout, or connection failure will now result in request denials rather than bypassing the processor. Please ensure the enforcer's availability, response timeout (conf.Envoy.EnforcerResponseTimeoutInSeconds), and rollout/health-check posture are sized accordingly, and that monitoring/alerting on the ext_proc cluster is in place before release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adapter/internal/oasparser/envoyconf/http_filters.go` at line 249, You set FailureModeAllow: false (fail-closed) for ext_proc; ensure operational readiness by verifying the enforcer/ext_proc cluster is highly available, tuned for the configured conf.Envoy.EnforcerResponseTimeoutInSeconds, and has proper health checks and readiness probes before rollout; add/confirm monitoring and alerting for ext_proc latency/errors, update deployment/rollout docs to reflect the change, and cross-check ext_authz behavior in the related translator (ext_authz) to ensure consistent timeout and retry settings.runtime/config-deployer-service/ballerina/K8sClient.bal (2)
45-53: Consider caching the token to avoid per-request disk I/O.Kubernetes projected service account tokens rotate on the order of minutes to hours, so reading the token from disk on every Kubernetes API call is more frequent than required and adds file I/O to the hot path. A small in-memory cache invalidated by a short TTL (or by the file's modification time) would preserve the token-rotation fix while reducing I/O. Deferrable, but worth considering if the deployer handles a high request volume.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/config-deployer-service/ballerina/K8sClient.bal` around lines 45 - 53, The getK8sAuthHeader function currently reads the service account token from disk on every call; change it to cache the token in-memory with a short TTL (or validate against the token file's modification time) so repeated calls return the cached token until invalidated, then re-read and update the cache; update references to k8sConfiguration.serviceAccountPath and ensure the cache is safe for concurrent isolated function usage and returns the same map<string|string[]> shape as before when valid.
163-163: Use named argument for consistency with other call sites.Every other updated call site passes the header map via the named argument
headers = getK8sAuthHeader(), but this line uses a positional argument. Using named arguments improves readability and makes the code intent explicit.♻️ Proposed fix
- http:Response response = check k8sApiServerEp->get(endpoint, getK8sAuthHeader()); + http:Response response = check k8sApiServerEp->get(endpoint, headers = getK8sAuthHeader());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/config-deployer-service/ballerina/K8sClient.bal` at line 163, The call to k8sApiServerEp->get is using a positional argument for headers (k8sApiServerEp->get(endpoint, getK8sAuthHeader())) inconsistent with other sites; change it to pass the header map as a named argument (headers = getK8sAuthHeader()) so the http:Response response = check k8sApiServerEp->get(...) call uses headers = getK8sAuthHeader(), keeping the same endpoint and response handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@runtime/config-deployer-service/ballerina/K8sClient.bal`:
- Around line 47-53: The getK8sAuthHeader function currently swallows errors
from io:fileReadString and returns an empty header; update getK8sAuthHeader to
log the error when token is not a string (i.e., when io:fileReadString returns
an error) using ballerina/log so failures reading the service account token are
recorded, then continue to return the empty map; add the required import
statement (import ballerina/log;) if it is not already present.
---
Nitpick comments:
In `@adapter/internal/oasparser/envoyconf/http_filters.go`:
- Line 249: You set FailureModeAllow: false (fail-closed) for ext_proc; ensure
operational readiness by verifying the enforcer/ext_proc cluster is highly
available, tuned for the configured conf.Envoy.EnforcerResponseTimeoutInSeconds,
and has proper health checks and readiness probes before rollout; add/confirm
monitoring and alerting for ext_proc latency/errors, update deployment/rollout
docs to reflect the change, and cross-check ext_authz behavior in the related
translator (ext_authz) to ensure consistent timeout and retry settings.
In `@runtime/config-deployer-service/ballerina/K8sClient.bal`:
- Around line 45-53: The getK8sAuthHeader function currently reads the service
account token from disk on every call; change it to cache the token in-memory
with a short TTL (or validate against the token file's modification time) so
repeated calls return the cached token until invalidated, then re-read and
update the cache; update references to k8sConfiguration.serviceAccountPath and
ensure the cache is safe for concurrent isolated function usage and returns the
same map<string|string[]> shape as before when valid.
- Line 163: The call to k8sApiServerEp->get is using a positional argument for
headers (k8sApiServerEp->get(endpoint, getK8sAuthHeader())) inconsistent with
other sites; change it to pass the header map as a named argument (headers =
getK8sAuthHeader()) so the http:Response response = check
k8sApiServerEp->get(...) call uses headers = getK8sAuthHeader(), keeping the
same endpoint and response handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccd2eb16-ef70-491e-9906-c2a704299a1e
📒 Files selected for processing (2)
adapter/internal/oasparser/envoyconf/http_filters.goruntime/config-deployer-service/ballerina/K8sClient.bal
2 participants
Purpose
Related to -
#3318
Goals
Approach
Dynamically read the Kubernetes Service Account (SA) token for every API request, ensuring compatibility with token rotation, and consistently attaching the correct authentication header to all requests.
Kubernetes API Authentication Handling:
getK8sAuthHeader()function inK8sClient.balto read the SA token from disk on every call, ensuring that token rotation is handled seamlessly. All Kubernetes API requests now include the up-to-dateAuthorizationheader.K8sClient.balto use the dynamic authentication header by passingheaders = getK8sAuthHeader()for every request (GET, POST, PUT, DELETE).authblock from the HTTP client initialization, since the header is now dynamically attached per request.Set the
failure_mode_allow: falsein the ext proc configuration to ensure Envoy to fail close.Ext Proc failure Handling:
FailureModeAllowfromtruetofalsein the Envoy external processing filter configuration.