filter_kubernetes: use service account issuer to detect EKS env #11002
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaced ConfigMap-based EKS detection with serviceaccount JWT issuer inspection and removed the exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant FLB as Fluent Bit
participant K8S as kubernetes_aws.c
participant FS as Filesystem
Note right of K8S `#F0F4FF`: determine_platform(ctx)
K8S->>FS: read serviceaccount token file
alt read fails
K8S-->>FLB: return -1
else token read
K8S->>K8S: split JWT (header.payload.signature)
alt malformed JWT
K8S-->>FLB: return -1
else JWT ok
K8S->>K8S: base64url-decode payload (handle padding, -/_)
alt decode fails
K8S-->>FLB: return -1
else decoded payload
K8S->>K8S: parse JSON, extract "iss"
alt issuer matches oidc.eks.*.amazonaws.com/id/...
Note right of K8S `#DFF2E1`: EKS detected
K8S-->>FLB: return 1
else no match
K8S-->>FLB: return -1
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)plugins/filter_kubernetes/kubernetes_aws.c (2)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/filter_kubernetes/kubernetes_aws.c (1)
297-300: Validate token_size after reading the Kubernetes token
Token constant is defined (kube_meta.h:55). Add a check thattoken_size > 0andtoken_size <= FLB_KUBE_TOKEN_BUF_SIZEbefore using the buffer to guard against empty or overly large token reads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/filter_kubernetes/kube_conf.h(1 hunks)plugins/filter_kubernetes/kubernetes_aws.c(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_kubernetes/kubernetes_aws.c (2)
src/flb_utils.c (1)
flb_utils_read_file(1937-1940)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (5)
plugins/filter_kubernetes/kube_conf.h (1)
77-80: LGTM!The updated comment accurately reflects the new platform detection approach using serviceaccount token issuer inspection. The removal of the AWS_AUTH_CONFIG_MAP macro (mentioned in the summary) aligns with the shift away from ConfigMap-based detection.
plugins/filter_kubernetes/kubernetes_aws.c (4)
26-26: LGTM!The inclusion of
flb_utils.his necessary for theflb_utils_read_filefunction used in the new platform detection logic.
302-313: LGTM!The JWT structure validation correctly checks for the expected three-part format (header.payload.signature) and properly handles cleanup on parse failures.
315-346: Locate flb_base64_decode implementation.Run:
#!/bin/bash rg -n "flb_base64_decode" -g '*.c' -g '*.h' -C3
286-386: No changes needed for determine_platform return handling.The only caller in kube_meta.c correctly treats
ret == -1as non-EKS and all other values (i.e.1) as EKS.
| /* Look for "iss" field in the JSON payload */ | ||
| issuer_start = strstr(payload, "\"iss\":"); | ||
| if (!issuer_start) { | ||
| flb_free(payload); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Skip to the value part */ | ||
| issuer_start = strchr(issuer_start, ':'); | ||
| if (!issuer_start) { | ||
| flb_free(payload); | ||
| return -1; | ||
| } | ||
| return -1; | ||
| issuer_start++; | ||
|
|
||
| /* Skip whitespace and opening quote */ | ||
| while (*issuer_start == ' ' || *issuer_start == '\t') issuer_start++; | ||
| if (*issuer_start != '"') { | ||
| flb_free(payload); | ||
| return -1; | ||
| } | ||
| issuer_start++; | ||
|
|
||
| /* Find closing quote */ | ||
| issuer_end = strchr(issuer_start, '"'); | ||
| if (!issuer_end) { | ||
| flb_free(payload); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace manual JSON parsing with a proper parser.
The current string-based parsing for the "iss" field is fragile and may fail or produce false positives in the following scenarios:
- The "iss" field appears in a nested JSON object
- The issuer value contains escaped quotes
- Another field name contains "iss" as a substring (e.g., "missing")
Since the file already includes flb_jsmn.h (line 23), use the jsmn JSON parser for robust and reliable extraction of the issuer field.
Consider applying this approach:
/* Parse JSON payload using jsmn */
jsmn_parser parser;
jsmntok_t tokens[64]; /* Adjust size as needed */
int token_count;
jsmn_init(&parser);
token_count = jsmn_parse(&parser, payload, payload_len, tokens, sizeof(tokens)/sizeof(tokens[0]));
if (token_count < 0) {
flb_free(payload);
return -1;
}
/* Find and extract "iss" field value */
char *issuer = NULL;
size_t issuer_len = 0;
for (int i = 1; i < token_count; i++) {
if (tokens[i].type == JSMN_STRING &&
strncmp(payload + tokens[i].start, "iss", tokens[i].end - tokens[i].start) == 0) {
/* Next token is the value */
if (i + 1 < token_count && tokens[i + 1].type == JSMN_STRING) {
issuer = payload + tokens[i + 1].start;
issuer_len = tokens[i + 1].end - tokens[i + 1].start;
break;
}
}
}
if (!issuer) {
flb_free(payload);
return -1;
}🤖 Prompt for AI Agents
plugins/filter_kubernetes/kubernetes_aws.c lines 347-375: Replace the brittle
string-based "iss" extraction with jsmn parsing: initialize a jsmn_parser, parse
payload using payload_len into a tokens array (resize tokens if needed), check
token_count for errors and free payload on failure, then iterate tokens to find
a JSMN_STRING token whose content exactly equals "iss" (use
tokens[i].start/end), verify the next token exists and is a string, and set
issuer_start = payload + tokens[i+1].start and issuer_end = payload +
tokens[i+1].end; on any failure free payload and return -1; this handles nested
objects and escaped characters correctly.
movence
force-pushed
the
aws-eks-detection
branch
from
721ae39 to
1a27fbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/filter_kubernetes/kube_conf.h(1 hunks)plugins/filter_kubernetes/kubernetes_aws.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/filter_kubernetes/kubernetes_aws.c
🔇 Additional comments (1)
plugins/filter_kubernetes/kube_conf.h (1)
77-80: All references to AWS_AUTH_CONFIG_MAP have been removed
Search forAWS_AUTH_CONFIG_MAPacross.cand.hfiles returned no results, confirming safe removal.
movence
force-pushed
the
aws-eks-detection
branch
from
1a27fbf to
1c0104a
Compare
| * on EKS or native Kubernetes by inspecting serviceaccount token issuer | ||
| */ | ||
| #define KUBE_SYSTEM_NAMESPACE "kube-system" | ||
| #define AWS_AUTH_CONFIG_MAP "aws-auth" |
There was a problem hiding this comment.
Should we keep this as a fallback for old clusters? Or does the issuer exist on both old and new cluster so it should just be the definitive mechanism for determining platforms?
There was a problem hiding this comment.
That is a good point. We found that the EKS service account issuer was introduced starting with EKS version 1.21, which has been EOL for quite some time now. This gave us confidence to move forward with this approach without a fallback.
movence
force-pushed
the
aws-eks-detection
branch
from
10f6052 to
8b53611
Compare
|
Hey @edsiper could you check this out since we've discussed it before? |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ret = flb_base64_decode((unsigned char *)payload, payload_b64_len * 3 / 4 + 4, | ||
| &payload_len, (unsigned char *)payload_b64, payload_b64_len); | ||
|
|
||
| flb_free(token_buf); | ||
| flb_free(payload_b64); |
There was a problem hiding this comment.
The new EKS detection reads the service account JWT and decodes the payload with flb_base64_decode. That helper only understands standard base64 (+ and /), while Kubernetes service account tokens are base64url encoded (- and _, no padding). Tokens containing those characters will make the decode call fail with FLB_BASE64_ERR_INVALID_CHARACTER, so determine_platform returns -1 even when running on EKS. This means most clusters will now be misclassified as generic Kubernetes. The payload should be converted from base64url to base64 or decoded with a base64url-capable routine before inspecting the iss field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Really appreciate your review @edsiper!! I have addressed the finding.
movence
force-pushed
the
aws-eks-detection
branch
from
79f7d75 to
7ee7a2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugins/filter_kubernetes/kubernetes_aws.c (3)
343-344: Consider using a separate variable for padding loop counter.The variable
payload_b64_lenis reused as a loop counter when adding padding, which changes its semantic meaning from "original length" to "padded length". While functional, using a separate loop variable or explicitly renaming would improve clarity.Example:
- while (payload_b64_len < padded_len) { - payload_b64[payload_b64_len++] = '='; + for (size_t i = payload_b64_len; i < padded_len; i++) { + payload_b64[i] = '='; } + payload_b64_len = padded_len;
369-397: Consider using jsmn parser for more robust JSON parsing.The current manual string parsing works for typical Kubernetes JWT payloads but may be fragile in edge cases:
- Escaped quotes in the issuer value
- The "iss" substring appearing in other field names
- Unusual JSON formatting or whitespace
Since the file already includes
flb_jsmn.h(line 23), using the jsmn parser would provide more robust and maintainable parsing. This was suggested in a previous review.However, given that Kubernetes service account JWTs have a predictable structure, the current approach is functional for the intended use case.
Based on previous review suggestions.
385-385: Whitespace handling could be more comprehensive.The code skips only spaces and tabs after the colon, but not other whitespace characters like newlines (
\n), carriage returns (\r), or other Unicode whitespace. While Kubernetes JWTs are typically compact, adding\nand\rto the skip list would make the parsing more robust.- while (*issuer_start == ' ' || *issuer_start == '\t') issuer_start++; + while (*issuer_start == ' ' || *issuer_start == '\t' || + *issuer_start == '\n' || *issuer_start == '\r') issuer_start++;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/filter_kubernetes/kube_conf.h(1 hunks)plugins/filter_kubernetes/kubernetes_aws.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/filter_kubernetes/kube_conf.h
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_kubernetes/kubernetes_aws.c (2)
src/flb_utils.c (1)
flb_utils_read_file(1937-1940)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (4)
plugins/filter_kubernetes/kubernetes_aws.c (4)
26-26: LGTM: Include added for file reading utility.The
flb_utils.hinclude is necessary forflb_utils_read_fileused to read the service account token.
286-308: LGTM: Variable declarations and token reading follow conventions.The function properly declares all variables at the top per coding guidelines and handles token reading errors appropriately.
310-365: LGTM: JWT structure validation and base64url handling correctly implemented.The code properly:
- Validates JWT structure (two dots separating three parts)
- Converts base64url to standard base64 (- to +, _ to /)
- Adds padding to ensure length is a multiple of 4
- Decodes the payload with proper error handling
This addresses the base64url decoding concerns raised in previous reviews.
399-417: LGTM: Pattern matching correctly scoped to issuer value.The code now extracts the issuer value into a separate null-terminated string before performing pattern matching. This properly addresses the previous review concern about searching beyond the issuer field boundaries.
|
@movence pls cleanup the git commit history according the guidelines |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
movence
force-pushed
the
aws-eks-detection
branch
from
7ee7a2c to
630081e
Compare
|
@edsiper updated the commit history. Thanks!! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/filter_kubernetes/kube_conf.h(1 hunks)plugins/filter_kubernetes/kubernetes_aws.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/filter_kubernetes/kube_conf.h
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_kubernetes/kubernetes_aws.c (2)
src/flb_utils.c (1)
flb_utils_read_file(1937-1940)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
movence
force-pushed
the
aws-eks-detection
branch
from
630081e to
4558c6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/filter_kubernetes/kube_conf.h(1 hunks)plugins/filter_kubernetes/kubernetes_aws.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/filter_kubernetes/kube_conf.h
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_kubernetes/kubernetes_aws.c (2)
src/flb_utils.c (1)
flb_utils_read_file(1937-1940)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
More reliable AWS EKS detection by inspecting service account token issuer Signed-off-by: Hyunsoo Kim <[email protected]>
movence
force-pushed
the
aws-eks-detection
branch
from
4558c6b to
176bf18
Compare
3 participants
Background
Explored Related introduced
filter_kubernetesto add Kubernetes metadata to logs and metrics. The filter historically relied on the presence of theaws-authConfigMap to determine if the cluster is running on Amazon EKS. However, with the launch of EKS access entries, theaws-authConfigMap is no longer guaranteed to be present in EKS clusters.Problem
With the launch of EKS access entries,
aws-authConfigMap is no longer guaranteed to be present in EKS clusters. Missing ConfigMap causes two issues:- Noisy audit logs: Generates 404 not found errors in k8s audit logs when the
- Incorrect platform tagging: EKS clusters using access entries (
NAaws-authConfigMap doesn't existAPI) are falsely tagged asGenericorK8splatform instead ofAWS::EKSfor "Explore Related" functionalityEnter
[N/A]in the box, if an item is not applicable to your change.Testing
Using an EKS cluster with
authemticationModeset toAPIto preventaws-authconfigmap from being createdUse CloudWatch LogInsights to search application log entries with metadata. Note the value for
@entity.Attributes.PlatformTypeIf this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation