carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

helm updates for flowgger and netflow #2823

Merged
mfreeman451 merged 1 commit from refs/pull/2823/head into staging 2026-02-01 05:23:43 +00:00
Conversation 2 Commits 1 Files changed 6 +18 -4
mfreeman451 commented 2026-02-01 05:23:26 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #2654
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2654
Original created: 2026-02-01T05:23:26Z
Original updated: 2026-02-01T05:25:00Z
Original head: carverauto/serviceradar:chore/k8s-updates
Original base: staging
Original merged: 2026-02-01T05:23:43Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

  • I have signed the DCO?
  • The build completes without errors?
  • All tests are passing when running make test?

PR Type

Enhancement

Description

  • Add externalTrafficPolicy configuration to Helm templates for flowgger and netflow services

  • Update demo values with new IP addresses and pool names for MetalLB

  • Set externalTrafficPolicy: Local in demo and production Kubernetes manifests

  • Add documentation comments explaining Calico BGP requirement for traffic policy

Diagram Walkthrough

flowchart LR
  A["Helm Templates"] -->|Add externalTrafficPolicy| B["flowgger.yaml & netflow-collector.yaml"]
  C["Values Files"] -->|Update IPs & pool names| D["values-demo.yaml & values.yaml"]
  E["K8s Manifests"] -->|Set Local policy| F["Demo & Prod Services"]
  B --> G["Enhanced Service Configuration"]
  D --> G
  F --> G

File Walkthrough
Relevant files
Enhancement
flowgger.yaml
Add externalTrafficPolicy to flowgger service                       

helm/serviceradar/templates/flowgger.yaml

  • Add conditional externalTrafficPolicy field to flowgger external
    service spec
  • Allow configuration via
    Values.flowgger.externalService.externalTrafficPolicy
  • Maintain backward compatibility with conditional rendering
 +3/-0     
netflow-collector.yaml
Add externalTrafficPolicy to netflow service                         

helm/serviceradar/templates/netflow-collector.yaml

  • Add conditional externalTrafficPolicy field to netflow collector
    service spec
  • Allow configuration via
    Values.netflowCollector.service.externalTrafficPolicy
  • Maintain backward compatibility with conditional rendering
 +3/-0     
serviceradar-netflow-collector.yaml
Add externalTrafficPolicy to netflow service                         

k8s/demo/base/serviceradar-netflow-collector.yaml

  • Add externalTrafficPolicy: Local to netflow collector service spec
 +1/-0     
serviceradar-flowgger-external.yaml
Add externalTrafficPolicy to flowgger service                       

k8s/demo/prod/serviceradar-flowgger-external.yaml

  • Add externalTrafficPolicy: Local to flowgger external service spec
 +1/-0     
Configuration changes
values.yaml
Add externalTrafficPolicy defaults with documentation       

helm/serviceradar/values.yaml

  • Add externalTrafficPolicy: Cluster default for flowgger external
    service
  • Add externalTrafficPolicy: Cluster default for netflow collector
    service
  • Add documentation comments explaining Calico BGP requirement
 +4/-0     
values-demo.yaml
Update demo IPs, pools, and traffic policies                         

helm/serviceradar/values-demo.yaml

  • Update flowgger loadBalancerIP from 192.168.6.81 to 23.138.124.24
  • Update netflow loadBalancerIP from 192.168.6.82 to 23.138.124.25
  • Change MetalLB address pool from k3s-lan-pool to k3s-pool for both
    services
  • Set externalTrafficPolicy: Local for both flowgger and netflow
    services
 +6/-4     
Imported from GitHub pull request. Original GitHub pull request: #2654 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2654 Original created: 2026-02-01T05:23:26Z Original updated: 2026-02-01T05:25:00Z Original head: carverauto/serviceradar:chore/k8s-updates Original base: staging Original merged: 2026-02-01T05:23:43Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Enhancement ___ ### **Description** - Add `externalTrafficPolicy` configuration to Helm templates for flowgger and netflow services - Update demo values with new IP addresses and pool names for MetalLB - Set `externalTrafficPolicy: Local` in demo and production Kubernetes manifests - Add documentation comments explaining Calico BGP requirement for traffic policy ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Helm Templates"] -->|Add externalTrafficPolicy| B["flowgger.yaml & netflow-collector.yaml"] C["Values Files"] -->|Update IPs & pool names| D["values-demo.yaml & values.yaml"] E["K8s Manifests"] -->|Set Local policy| F["Demo & Prod Services"] B --> G["Enhanced Service Configuration"] D --> G F --> G ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>flowgger.yaml</strong><dd><code>Add externalTrafficPolicy to flowgger service</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> helm/serviceradar/templates/flowgger.yaml <ul><li>Add conditional <code>externalTrafficPolicy</code> field to flowgger external <br>service spec<br> <li> Allow configuration via <br><code>Values.flowgger.externalService.externalTrafficPolicy</code><br> <li> Maintain backward compatibility with conditional rendering</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2654/files#diff-511cfbfe42e0c41cd2fddf67a04911b48724ca9ea9f6d1ddc1f4bb7bf07086ab">+3/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>netflow-collector.yaml</strong><dd><code>Add externalTrafficPolicy to netflow service</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> helm/serviceradar/templates/netflow-collector.yaml <ul><li>Add conditional <code>externalTrafficPolicy</code> field to netflow collector <br>service spec<br> <li> Allow configuration via <br><code>Values.netflowCollector.service.externalTrafficPolicy</code><br> <li> Maintain backward compatibility with conditional rendering</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2654/files#diff-2caca5e6b4a9d567d04bd326125e554b1f571be0f7746fb6f9144e0a33f65b07">+3/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>serviceradar-netflow-collector.yaml</strong><dd><code>Add externalTrafficPolicy to netflow service</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> k8s/demo/base/serviceradar-netflow-collector.yaml <ul><li>Add <code>externalTrafficPolicy: Local</code> to netflow collector service spec</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2654/files#diff-c29757a23e5f4021c9cd4d6d35a261edf9b56c706188bb24855fafa84454b797">+1/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>serviceradar-flowgger-external.yaml</strong><dd><code>Add externalTrafficPolicy to flowgger service</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> k8s/demo/prod/serviceradar-flowgger-external.yaml <ul><li>Add <code>externalTrafficPolicy: Local</code> to flowgger external service spec</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2654/files#diff-0e3d242dcaf4f3a83adc9de3f7183db6299c50bca47b9cd544ee02dfec263dcf">+1/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>values.yaml</strong><dd><code>Add externalTrafficPolicy defaults with documentation</code>&nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> helm/serviceradar/values.yaml <ul><li>Add <code>externalTrafficPolicy: Cluster</code> default for flowgger external <br>service<br> <li> Add <code>externalTrafficPolicy: Cluster</code> default for netflow collector <br>service<br> <li> Add documentation comments explaining Calico BGP requirement</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2654/files#diff-d4449c7cb70362554b274f81eae5a4b81a8e81df494282e383d1b7ea3871c452">+4/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>values-demo.yaml</strong><dd><code>Update demo IPs, pools, and traffic policies</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> helm/serviceradar/values-demo.yaml <ul><li>Update flowgger loadBalancerIP from <code>192.168.6.81</code> to <code>23.138.124.24</code><br> <li> Update netflow loadBalancerIP from <code>192.168.6.82</code> to <code>23.138.124.25</code><br> <li> Change MetalLB address pool from <code>k3s-lan-pool</code> to <code>k3s-pool</code> for both <br>services<br> <li> Set <code>externalTrafficPolicy: Local</code> for both flowgger and netflow <br>services</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2654/files#diff-3a2c6c76ca4d5e8a336cd917d39b1704c03ea94a5cba4da1eb20629c63a5b914">+6/-4</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></td></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2026-02-01 05:23:52 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2654#issuecomment-3830402974
Original created: 2026-02-01T05:23:52Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Public service exposure

Description: The demo Helm values introduce hardcoded public loadBalancerIP addresses (23.138.124.24
and 23.138.124.25) which can unintentionally expose syslog/NetFlow ingestion endpoints to
the public Internet when applied, increasing the risk of unauthorized traffic ingestion,
DDoS, or data leakage if these endpoints are not additionally protected (e.g., by source
IP restrictions, firewall rules, or private address pools).
values-demo.yaml [94-107]

Referred Code 
      metallb.universe.tf/address-pool: k3s-pool
    loadBalancerIP: "23.138.124.24"
    externalTrafficPolicy: Local

netflowCollector:
  enabled: true
  config:
    stream_max_bytes: 1073741824
  service:
    type: LoadBalancer
    annotations:
      metallb.universe.tf/address-pool: k3s-pool
    loadBalancerIP: "23.138.124.25"
    externalTrafficPolicy: Local
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2654#issuecomment-3830402974 Original created: 2026-02-01T05:23:52Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/b10222c36e9be0c0cf97835da786e6639be7f92c --> Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=1>⚪</td> <td><details><summary><strong>Public service exposure </strong></summary><br> <b>Description:</b> The demo Helm values introduce hardcoded public <code>loadBalancerIP</code> addresses (<code>23.138.124.24</code> <br>and <code>23.138.124.25</code>) which can unintentionally expose syslog/NetFlow ingestion endpoints to <br>the public Internet when applied, increasing the risk of unauthorized traffic ingestion, <br>DDoS, or data leakage if these endpoints are not additionally protected (e.g., by source <br>IP restrictions, firewall rules, or private address pools).<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2654/files#diff-3a2c6c76ca4d5e8a336cd917d39b1704c03ea94a5cba4da1eb20629c63a5b914R94-R107'>values-demo.yaml [94-107]</a></strong><br> <details open><summary>Referred Code</summary> ```yaml metallb.universe.tf/address-pool: k3s-pool loadBalancerIP: "23.138.124.24" externalTrafficPolicy: Local netflowCollector: enabled: true config: stream_max_bytes: 1073741824 service: type: LoadBalancer annotations: metallb.universe.tf/address-pool: k3s-pool loadBalancerIP: "23.138.124.25" externalTrafficPolicy: Local ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=6>🟢</td><td> <details><summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> <!-- placeholder --> <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>
qodo-code-review[bot] commented 2026-02-01 05:25:00 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2654#issuecomment-3830404070
Original created: 2026-02-01T05:25:00Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent invalid Kubernetes service manifest

Conditionally render the externalTrafficPolicy field only when the service type
is not ClusterIP to prevent Kubernetes API validation errors during Helm
installation.

helm/serviceradar/templates/netflow-collector.yaml [145-152]

 spec:
   type: {{ .Values.netflowCollector.service.type | default "ClusterIP" }}
-  {{- with .Values.netflowCollector.service.externalTrafficPolicy }}
-  externalTrafficPolicy: {{ . }}
+  {{- if and .Values.netflowCollector.service.externalTrafficPolicy (ne (.Values.netflowCollector.service.type | default "ClusterIP") "ClusterIP") }}
+  externalTrafficPolicy: {{ .Values.netflowCollector.service.externalTrafficPolicy }}
   {{- end }}
   {{- if .Values.netflowCollector.service.loadBalancerIP }}
   loadBalancerIP: {{ .Values.netflowCollector.service.loadBalancerIP | quote }}
   {{- end }}
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a bug where helm install would fail with default values because externalTrafficPolicy is invalid for the default service type ClusterIP. The proposed fix prevents this deployment failure, making it a critical correction.

High
Fix template field indentation

Replace the with block with an if block and add proper indentation to ensure the
externalTrafficPolicy field is rendered correctly in the final YAML.

helm/serviceradar/templates/flowgger.yaml [193-195]

-{{- with .Values.flowgger.externalService.externalTrafficPolicy }}
-externalTrafficPolicy: {{ . }}
+{{- if .Values.flowgger.externalService.externalTrafficPolicy }}
+  externalTrafficPolicy: {{ . }}
 {{- end }}
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that the current {{- with ... }} block with a leading space on the next line will produce incorrect YAML indentation. The proposed fix using {{- if ... }} and adding indentation is a necessary correction for the Helm template to be valid.

Low
Correct YAML indentation in template

Replace the with block with an if block and add proper indentation to ensure the
externalTrafficPolicy field is correctly aligned in the generated service
manifest.

helm/serviceradar/templates/netflow-collector.yaml [147-149]

-{{- with .Values.netflowCollector.service.externalTrafficPolicy }}
-externalTrafficPolicy: {{ . }}
+{{- if .Values.netflowCollector.service.externalTrafficPolicy }}
+  externalTrafficPolicy: {{ . }}
 {{- end }}
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that the current {{- with ... }} block combined with a leading space on the next line will generate invalid YAML due to incorrect indentation. The proposed fix using {{- if ... }} and adding indentation is required for the Helm template to function correctly.

Low
High-level
Consolidate Kubernetes manifests into Helm

The suggestion recommends consolidating Kubernetes configurations into Helm
charts. It points out that maintaining both Helm and static manifests increases
overhead and risks inconsistencies.

Examples:

k8s/demo/base/serviceradar-netflow-collector.yaml [157] 
  externalTrafficPolicy: Local
helm/serviceradar/values-demo.yaml [107] 
    externalTrafficPolicy: Local

Solution Walkthrough:

Before:

# The same configuration is applied in two separate systems.

# In helm/serviceradar/values-demo.yaml
netflowCollector:
  service:
    ...
    externalTrafficPolicy: Local

# In k8s/demo/base/serviceradar-netflow-collector.yaml
apiVersion: v1
kind: Service
...
spec:
  ...
  externalTrafficPolicy: Local
  ...

After:

# Configuration is consolidated into a single system (Helm).

# In helm/serviceradar/values-demo.yaml
# This becomes the single source of truth for deployment.
netflowCollector:
  service:
    ...
    externalTrafficPolicy: Local

# The file k8s/demo/base/serviceradar-netflow-collector.yaml
# is removed, and deployments are generated from the Helm chart.
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a significant architectural issue of maintaining parallel Helm and static Kubernetes configurations, which the PR perpetuates, and proposes a valid long-term solution.

Medium
General
Clarify default policy comment

Update the comment for externalTrafficPolicy to clarify that the default is
Cluster and explain when to set it to Local.

helm/serviceradar/values.yaml [255-256]

-# Only advertise from nodes with local endpoints (required for Calico BGP)
+# Set to 'Local' to only advertise from nodes with local endpoints (required for Calico BGP)
 externalTrafficPolicy: Cluster
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the comment for externalTrafficPolicy is misleading, as it describes the behavior of Local while the default value is Cluster. Fixing this improves clarity and prevents potential misconfiguration by users.

Low
  • More
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2654#issuecomment-3830404070 Original created: 2026-02-01T05:25:00Z --- ## PR Code Suggestions ✨ <!-- b10222c --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=3>Possible issue</td> <td> <details><summary>Prevent invalid Kubernetes service manifest</summary> ___ **Conditionally render the <code>externalTrafficPolicy</code> field only when the service type <br>is not <code>ClusterIP</code> to prevent Kubernetes API validation errors during Helm <br>installation.** [helm/serviceradar/templates/netflow-collector.yaml [145-152]](https://github.com/carverauto/serviceradar/pull/2654/files#diff-2caca5e6b4a9d567d04bd326125e554b1f571be0f7746fb6f9144e0a33f65b07R145-R152) ```diff spec: type: {{ .Values.netflowCollector.service.type | default "ClusterIP" }} - {{- with .Values.netflowCollector.service.externalTrafficPolicy }} - externalTrafficPolicy: {{ . }} + {{- if and .Values.netflowCollector.service.externalTrafficPolicy (ne (.Values.netflowCollector.service.type | default "ClusterIP") "ClusterIP") }} + externalTrafficPolicy: {{ .Values.netflowCollector.service.externalTrafficPolicy }} {{- end }} {{- if .Values.netflowCollector.service.loadBalancerIP }} loadBalancerIP: {{ .Values.netflowCollector.service.loadBalancerIP | quote }} {{- end }} ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=0 --> <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This suggestion correctly identifies a bug where `helm install` would fail with default values because `externalTrafficPolicy` is invalid for the default service type `ClusterIP`. The proposed fix prevents this deployment failure, making it a critical correction. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>Fix template field indentation</summary> ___ **Replace the <code>with</code> block with an <code>if</code> block and add proper indentation to ensure the <br><code>externalTrafficPolicy</code> field is rendered correctly in the final YAML.** [helm/serviceradar/templates/flowgger.yaml [193-195]](https://github.com/carverauto/serviceradar/pull/2654/files#diff-511cfbfe42e0c41cd2fddf67a04911b48724ca9ea9f6d1ddc1f4bb7bf07086abR193-R195) ```diff -{{- with .Values.flowgger.externalService.externalTrafficPolicy }} -externalTrafficPolicy: {{ . }} +{{- if .Values.flowgger.externalService.externalTrafficPolicy }} + externalTrafficPolicy: {{ . }} {{- end }} ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly points out that the current `{{- with ... }}` block with a leading space on the next line will produce incorrect YAML indentation. The proposed fix using `{{- if ... }}` and adding indentation is a necessary correction for the Helm template to be valid. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Correct YAML indentation in template</summary> ___ **Replace the <code>with</code> block with an <code>if</code> block and add proper indentation to ensure the <br><code>externalTrafficPolicy</code> field is correctly aligned in the generated service <br>manifest.** [helm/serviceradar/templates/netflow-collector.yaml [147-149]](https://github.com/carverauto/serviceradar/pull/2654/files#diff-2caca5e6b4a9d567d04bd326125e554b1f571be0f7746fb6f9144e0a33f65b07R147-R149) ```diff -{{- with .Values.netflowCollector.service.externalTrafficPolicy }} -externalTrafficPolicy: {{ . }} +{{- if .Values.netflowCollector.service.externalTrafficPolicy }} + externalTrafficPolicy: {{ . }} {{- end }} ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=2 --> <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies that the current `{{- with ... }}` block combined with a leading space on the next line will generate invalid YAML due to incorrect indentation. The proposed fix using `{{- if ... }}` and adding indentation is required for the Helm template to function correctly. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Consolidate Kubernetes manifests into Helm</summary> ___ **The suggestion recommends consolidating Kubernetes configurations into Helm <br>charts. It points out that maintaining both Helm and static manifests increases <br>overhead and risks inconsistencies.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2654/files#diff-c29757a23e5f4021c9cd4d6d35a261edf9b56c706188bb24855fafa84454b797R157-R157">k8s/demo/base/serviceradar-netflow-collector.yaml [157]</a> </summary> ```yaml externalTrafficPolicy: Local ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2654/files#diff-3a2c6c76ca4d5e8a336cd917d39b1704c03ea94a5cba4da1eb20629c63a5b914R107-R107">helm/serviceradar/values-demo.yaml [107]</a> </summary> ```yaml externalTrafficPolicy: Local ``` </details> ### Solution Walkthrough: #### Before: ```yaml # The same configuration is applied in two separate systems. # In helm/serviceradar/values-demo.yaml netflowCollector: service: ... externalTrafficPolicy: Local # In k8s/demo/base/serviceradar-netflow-collector.yaml apiVersion: v1 kind: Service ... spec: ... externalTrafficPolicy: Local ... ``` #### After: ```yaml # Configuration is consolidated into a single system (Helm). # In helm/serviceradar/values-demo.yaml # This becomes the single source of truth for deployment. netflowCollector: service: ... externalTrafficPolicy: Local # The file k8s/demo/base/serviceradar-netflow-collector.yaml # is removed, and deployments are generated from the Helm chart. ``` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a significant architectural issue of maintaining parallel Helm and static Kubernetes configurations, which the PR perpetuates, and proposes a valid long-term solution. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Clarify default policy comment</summary> ___ **Update the comment for <code>externalTrafficPolicy</code> to clarify that the default is <br><code>Cluster</code> and explain when to set it to <code>Local</code>.** [helm/serviceradar/values.yaml [255-256]](https://github.com/carverauto/serviceradar/pull/2654/files#diff-d4449c7cb70362554b274f81eae5a4b81a8e81df494282e383d1b7ea3871c452R255-R256) ```diff -# Only advertise from nodes with local endpoints (required for Calico BGP) +# Set to 'Local' to only advertise from nodes with local endpoints (required for Calico BGP) externalTrafficPolicy: Cluster ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=4 --> <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that the comment for `externalTrafficPolicy` is misleading, as it describes the behavior of `Local` while the default value is `Cluster`. Fixing this improves clarity and prevents potential misconfiguration by users. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More <!-- /improve --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2823
No description provided.