fix: add service for filebeat#112
Conversation
WalkthroughThis pull request modifies the Kubernetes Filebeat Helm chart by introducing a new Service template for UDP syslog traffic, simplifying the ConfigMap module configuration (removing Juniper, retaining Cisco), and adding service-related configuration options to values.yaml including LoadBalancer support, external IP, elasticsearch admin credentials, and pod affinity settings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kubernetes/filebeat/templates/cm.yml (1)
38-45:⚠️ Potential issue | 🔴 CriticalPort mismatch between ConfigMap and Service.
The Cisco IOS syslog module listens on port 9001 (
var.syslog_port: 9001), but the new Service (filebeat-service.yml) exposes port 5066 withtargetPort: 5066. UDP syslog traffic arriving on the Service will never reach the filebeat syslog listener. Either thetargetPortin the Service should be9001, orvar.syslog_porthere should be5066.
🤖 Fix all issues with AI agents
In `@kubernetes/filebeat/templates/filebeat-service.yml`:
- Line 1: The file kubernetes/filebeat/templates/filebeat-service.yml uses
Windows CRLF line endings causing YAMLlint "wrong new line character" errors;
convert the file to Unix LF endings (e.g., run dos2unix or configure your
editor/git to use LF) and recommit so the linter sees valid newlines—leave the
Helm template syntax unchanged (the apparent syntax error at line 5 is a
YAMLlint false positive).
- Around line 27-34: The Service template is using the wrong value for
loadBalancerIP: replace the reference to {{
.Values.global.logstash.loadBalancerIP }} with the filebeat-specific value used
in the guard (e.g. {{ .Values.service.loadBalancerIP }} or a dedicated {{
.Values.global.filebeat.loadBalancerIP }} if you prefer global-scoped settings),
and add a corresponding default entry in values.yaml (service.loadBalancerIP: ""
or global.filebeat.loadBalancerIP: "") so the conditional and the value are
consistent (update the filebeat Service template and values.yaml accordingly).
In `@kubernetes/filebeat/values.yaml`:
- Around line 6-9: The adminUser block contains hardcoded credentials
(adminUser.name / adminUser.pass / adminUser.role); remove the plaintext pass
value from values.yaml and instead reference a Kubernetes Secret (or
sealed-secret/external secret manager). Update the Helm values to accept a
secret reference (e.g., adminUser.secretName or adminUser.secretRef) and change
templates that read adminUser.pass to pull from a secretKeyRef or env var
sourced from that Secret; ensure creation of the Secret is documented or handled
outside version control rather than storing the password in values.yaml.
- Around line 31-32: The template references .Values.service.loadBalancerIP in
conditionals but values.yaml only defines service.type, causing rendering
errors; add a default entry for service.loadBalancerIP in values.yaml (e.g.,
under the existing service block add loadBalancerIP with an empty value) so
.Values.service.loadBalancerIP always exists, preserving current behavior when
unset and satisfying the template checks for (not (empty
.Values.service.loadBalancerIP)).
🧹 Nitpick comments (1)
kubernetes/filebeat/templates/filebeat-service.yml (1)
17-18: HardcodednodePortvalue.The nodePort
30901is hardcoded. If this port is already in use in the cluster, the service will fail to create. Consider making it configurable viaValues.service.nodePortor removing it to let Kubernetes auto-assign.Proposed fix
{{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }} - nodePort: 30901 + nodePort: {{ .Values.service.nodePort | default 30901 }} {{- end }}
| @@ -0,0 +1,34 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
Static analysis: wrong newline character.
YAMLlint reports wrong new line character: expected \n on line 1. The file likely uses Windows-style line endings (\r\n). Convert to Unix line endings (\n). The syntax error on line 5 is a false positive caused by Helm template syntax that YAMLlint cannot parse.
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: wrong new line character: expected \n
(new-lines)
🤖 Prompt for AI Agents
In `@kubernetes/filebeat/templates/filebeat-service.yml` at line 1, The file
kubernetes/filebeat/templates/filebeat-service.yml uses Windows CRLF line
endings causing YAMLlint "wrong new line character" errors; convert the file to
Unix LF endings (e.g., run dos2unix or configure your editor/git to use LF) and
recommit so the linter sees valid newlines—leave the Helm template syntax
unchanged (the apparent syntax error at line 5 is a YAMLlint false positive).
| {{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }} | ||
| externalTrafficPolicy: Local | ||
| {{- if .Values.global.externalIP }} | ||
| externalIPs: | ||
| - {{ .Values.global.externalIP }} | ||
| {{- end }} | ||
| loadBalancerIP: {{ .Values.global.logstash.loadBalancerIP }} | ||
| {{- end }} |
There was a problem hiding this comment.
loadBalancerIP is sourced from global.logstash.loadBalancerIP — likely a copy-paste error.
Line 33 sets the Service's loadBalancerIP to {{ .Values.global.logstash.loadBalancerIP }} (the logstash load balancer IP), but the conditional on line 17/27 checks .Values.service.loadBalancerIP. This is inconsistent:
- The guard condition references a value (
service.loadBalancerIP) that doesn't exist invalues.yaml. - The actual IP used belongs to logstash, not filebeat.
This likely means the filebeat Service will be assigned logstash's IP, which is almost certainly unintended.
Proposed fix — use a consistent, filebeat-specific value
{{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }}
externalTrafficPolicy: Local
{{- if .Values.global.externalIP }}
externalIPs:
- {{ .Values.global.externalIP }}
{{- end }}
- loadBalancerIP: {{ .Values.global.logstash.loadBalancerIP }}
+ loadBalancerIP: {{ .Values.service.loadBalancerIP }}
{{- end }}And add a default in values.yaml:
service:
type: LoadBalancer
loadBalancerIP: ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }} | |
| externalTrafficPolicy: Local | |
| {{- if .Values.global.externalIP }} | |
| externalIPs: | |
| - {{ .Values.global.externalIP }} | |
| {{- end }} | |
| loadBalancerIP: {{ .Values.global.logstash.loadBalancerIP }} | |
| {{- end }} | |
| {{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }} | |
| externalTrafficPolicy: Local | |
| {{- if .Values.global.externalIP }} | |
| externalIPs: | |
| - {{ .Values.global.externalIP }} | |
| {{- end }} | |
| loadBalancerIP: {{ .Values.service.loadBalancerIP }} | |
| {{- end }} |
🤖 Prompt for AI Agents
In `@kubernetes/filebeat/templates/filebeat-service.yml` around lines 27 - 34, The
Service template is using the wrong value for loadBalancerIP: replace the
reference to {{ .Values.global.logstash.loadBalancerIP }} with the
filebeat-specific value used in the guard (e.g. {{
.Values.service.loadBalancerIP }} or a dedicated {{
.Values.global.filebeat.loadBalancerIP }} if you prefer global-scoped settings),
and add a corresponding default entry in values.yaml (service.loadBalancerIP: ""
or global.filebeat.loadBalancerIP: "") so the conditional and the value are
consistent (update the filebeat Service template and values.yaml accordingly).
There was a problem hiding this comment.
@huongpt11 @kiennkt Logstash value?
Use its own value .Values.service.loadBalancerIP
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| adminUser: | ||
| name: elastic | ||
| pass: juniper@123 | ||
| role: superuser |
There was a problem hiding this comment.
Hardcoded credentials in values.yaml.
The adminUser block contains a plaintext password (juniper@123). Even in a Helm values file, committing secrets to version control is a security risk. Consider using Kubernetes Secrets, sealed-secrets, or an external secret manager, and reference them via environment variables or secret volume mounts instead.
🤖 Prompt for AI Agents
In `@kubernetes/filebeat/values.yaml` around lines 6 - 9, The adminUser block
contains hardcoded credentials (adminUser.name / adminUser.pass /
adminUser.role); remove the plaintext pass value from values.yaml and instead
reference a Kubernetes Secret (or sealed-secret/external secret manager). Update
the Helm values to accept a secret reference (e.g., adminUser.secretName or
adminUser.secretRef) and change templates that read adminUser.pass to pull from
a secretKeyRef or env var sourced from that Secret; ensure creation of the
Secret is documented or handled outside version control rather than storing the
password in values.yaml.
There was a problem hiding this comment.
@huongpt11 @kiennkt leave it as a blank or dummy user/pass
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| service: | ||
| type: LoadBalancer |
There was a problem hiding this comment.
service.loadBalancerIP is missing from defaults but referenced in the service template.
The service template checks (not (empty .Values.service.loadBalancerIP)) in multiple conditionals, but values.yaml only defines service.type. This will cause a template rendering error if .Values.service.loadBalancerIP is not supplied by the user at install time.
Proposed fix
service:
type: LoadBalancer
+ loadBalancerIP: ""
+ annotations: {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| service: | |
| type: LoadBalancer | |
| service: | |
| type: LoadBalancer | |
| loadBalancerIP: "" | |
| annotations: {} |
🤖 Prompt for AI Agents
In `@kubernetes/filebeat/values.yaml` around lines 31 - 32, The template
references .Values.service.loadBalancerIP in conditionals but values.yaml only
defines service.type, causing rendering errors; add a default entry for
service.loadBalancerIP in values.yaml (e.g., under the existing service block
add loadBalancerIP with an empty value) so .Values.service.loadBalancerIP always
exists, preserving current behavior when unset and satisfying the template
checks for (not (empty .Values.service.loadBalancerIP)).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| annotations: {{- include "common.tplvalues.render" (dict "value" .Values.service.annotations "context" $) | nindent 4 }} | ||
| {{- end }} | ||
| spec: | ||
| type: {{ .Values.service.type }} |
There was a problem hiding this comment.
Update to work with running CI via ternary template
Example:
{{ ternary "ClusterIP" .Values.service.type .Values.global.ci }}
| # {{- end }} | ||
| {{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }} | ||
| externalTrafficPolicy: Local | ||
| {{- if .Values.global.externalIP }} |
There was a problem hiding this comment.
Move block externalIPs out of
{{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }}
{{- end }}
| service: | ||
| type: LoadBalancer |
| adminUser: | ||
| name: elastic | ||
| pass: juniper@123 | ||
| role: superuser |
There was a problem hiding this comment.
@huongpt11 @kiennkt leave it as a blank or dummy user/pass
| {{- if and (eq .Values.service.type "LoadBalancer") (not (empty .Values.service.loadBalancerIP)) }} | ||
| externalTrafficPolicy: Local | ||
| {{- if .Values.global.externalIP }} | ||
| externalIPs: | ||
| - {{ .Values.global.externalIP }} | ||
| {{- end }} | ||
| loadBalancerIP: {{ .Values.global.logstash.loadBalancerIP }} | ||
| {{- end }} |
There was a problem hiding this comment.
@huongpt11 @kiennkt Logstash value?
Use its own value .Values.service.loadBalancerIP
2 participants
Summary by CodeRabbit