carverauto/serviceradar

Fork 0

k8s fixes #2274

Merged

mfreeman451 merged 1 commit from refs/pull/2274/head into main

2025-10-05 17:04:11 +00:00

mfreeman451 commented

2025-10-05 16:42:18 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1702
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1702
Original created: 2025-10-05T16:42:18Z
Original updated: 2025-10-05T17:16:54Z
Original head: carverauto/serviceradar:updates/poller_k8s_fix
Original base: main
Original merged: 2025-10-05T17:04:11Z by @mfreeman451

PR Type

Bug fix, Enhancement

Description

Fix TLS server name configuration for external checkers
Improve Kubernetes deployment configurations and resource allocation
Add comprehensive service aliases for demo environment
Enhance health checks and DNS policy settings

Diagram Walkthrough

flowchart LR
  A["External Checker"] -- "Clone security config" --> B["TLS ServerName Fix"]
  C["K8s Agent"] -- "Update resources & DNS" --> D["Improved Deployment"]
  E["K8s Poller"] -- "Better health checks" --> F["Enhanced Monitoring"]
  G["Service Aliases"] -- "Add missing services" --> H["Complete Service Map"]

File Walkthrough

Relevant files

Bug fix

external_checker.go

Fix TLS ServerName configuration for external checkers

pkg/agent/external_checker.go

Clone security configuration to avoid mutation
Override TLS ServerName with target host when different
Add logging for TLS server name adjustments
Preserve original security config integrity

+22/-2

Enhancement

serviceradar-agent.yaml `Improve agent DNS policy and resource allocation` k8s/demo/base/serviceradar-agent.yaml Add `dnsPolicy: ClusterFirstWithHostNet` for proper DNS resolution Increase memory limits from 128Mi to 512Mi Increase memory requests from 64Mi to 128Mi	+3/-2
serviceradar-poller.yaml `Enhance poller deployment with better health checks` k8s/demo/base/serviceradar-poller.yaml Add Kubernetes standard labels for better organization Replace exec health checks with tcpSocket probes Increase memory limits from 256Mi to 512Mi Increase memory requests from 128Mi to 256Mi	+10/-14
service-aliases.demo.yaml `Expand service aliases with comprehensive mapping` k8s/demo/prod/service-aliases.demo.yaml Add explicit `type: ExternalName` to all services Add 9 new service aliases (agent, sync, mapper, etc.) Reorder existing services alphabetically Complete service discovery mapping	+97/-4

Configuration changes

kustomization.yaml

Simplify kustomization configuration

k8s/demo/prod/kustomization.yaml

Remove patchesStrategicMerge section
Simplify kustomization structure

+0/-2

Imported from GitHub pull request. Original GitHub pull request: #1702 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1702 Original created: 2025-10-05T16:42:18Z Original updated: 2025-10-05T17:16:54Z Original head: carverauto/serviceradar:updates/poller_k8s_fix Original base: main Original merged: 2025-10-05T17:04:11Z by @mfreeman451 --- ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Fix TLS server name configuration for external checkers - Improve Kubernetes deployment configurations and resource allocation - Add comprehensive service aliases for demo environment - Enhance health checks and DNS policy settings ___ ### Diagram Walkthrough ```mermaid flowchart LR A["External Checker"] -- "Clone security config" --> B["TLS ServerName Fix"] C["K8s Agent"] -- "Update resources & DNS" --> D["Improved Deployment"] E["K8s Poller"] -- "Better health checks" --> F["Enhanced Monitoring"] G["Service Aliases"] -- "Add missing services" --> H["Complete Service Map"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>external_checker.go</strong><dd><code>Fix TLS ServerName configuration for external checkers</code>      </dd></summary> <hr> pkg/agent/external_checker.go <ul><li>Clone security configuration to avoid mutation<br> <li> Override TLS ServerName with target host when different<br> <li> Add logging for TLS server name adjustments<br> <li> Preserve original security config integrity</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1702/files#diff-aef4501be72b4e79253b999691c56d7a751dd49548d87eed3284215a8e76aa77">+22/-2</a>    </td> </tr> </table></td></tr><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>serviceradar-agent.yaml</strong><dd><code>Improve agent DNS policy and resource allocation</code>                  </dd></summary> <hr> k8s/demo/base/serviceradar-agent.yaml <ul><li>Add <code>dnsPolicy: ClusterFirstWithHostNet</code> for proper DNS resolution<br> <li> Increase memory limits from 128Mi to 512Mi<br> <li> Increase memory requests from 64Mi to 128Mi</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1702/files#diff-750aaa803a43f0993450026e4174b8a7d20fe016b9ff726f154a77a4f0fb4e19">+3/-2</a>      </td> </tr> <tr> <td> <details> <summary><strong>serviceradar-poller.yaml</strong><dd><code>Enhance poller deployment with better health checks</code>            </dd></summary> <hr> k8s/demo/base/serviceradar-poller.yaml <ul><li>Add Kubernetes standard labels for better organization<br> <li> Replace exec health checks with tcpSocket probes<br> <li> Increase memory limits from 256Mi to 512Mi<br> <li> Increase memory requests from 128Mi to 256Mi</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1702/files#diff-20492d7f5153e92f95cbbf2f62fb75b1a43f530304372a5e7731fdf95b583f3b">+10/-14</a>  </td> </tr> <tr> <td> <details> <summary><strong>service-aliases.demo.yaml</strong><dd><code>Expand service aliases with comprehensive mapping</code>                </dd></summary> <hr> k8s/demo/prod/service-aliases.demo.yaml <ul><li>Add explicit <code>type: ExternalName</code> to all services<br> <li> Add 9 new service aliases (agent, sync, mapper, etc.)<br> <li> Reorder existing services alphabetically<br> <li> Complete service discovery mapping</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1702/files#diff-2a652ee3803d2decc7bf62439ee5712d719b72ebbf146d2a02093d4c06638f13">+97/-4</a>    </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>kustomization.yaml</strong><dd><code>Simplify kustomization configuration</code>                                          </dd></summary> <hr> k8s/demo/prod/kustomization.yaml <ul><li>Remove <code>patchesStrategicMerge</code> section<br> <li> Simplify kustomization structure</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1702/files#diff-0527e7f19d087f3576d5755a79554797ffbab78b1a7efaa38984b4f3241f6fc9">+0/-2</a>      </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-10-05 16:43:10 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1702#issuecomment-3369174728
Original created: 2025-10-05T16:43:10Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
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
⚪	No custom compliance provided Follow the guide to enable custom compliance check.

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/1702#issuecomment-3369174728 Original created: 2025-10-05T16:43:10Z --- ## PR Compliance Guide 🔍  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>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </summary></strong> - [ ] Create ticket/issue  </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>⚪</td><td><details><summary><strong>No custom compliance provided</strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/'>guide</a> to enable custom compliance check. </details></td></tr> <tr><td align="center" colspan="2">   </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

2025-10-05 16:44:07 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1702#issuecomment-3369175289
Original created: 2025-10-05T16:44:07Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Kustomize configuration appears to be broken The `kustomization.yaml` file incorrectly removes `service-aliases.demo.yaml` from `patchesStrategicMerge` but fails to add it to the `resources` list. This omission will prevent the new service aliases from being applied by Kustomize. Examples: k8s/demo/prod/kustomization.yaml [7-10] `resources: - ../base - namespace.yaml - service-aliases.demo.yaml` k8s/demo/prod/service-aliases.demo.yaml [1-117] `--- apiVersion: v1 kind: Service metadata: name: core namespace: demo spec: type: ExternalName externalName: serviceradar-core.demo.svc.cluster.local --- ... (clipped 107 lines)` Solution Walkthrough: Before: `# k8s/demo/prod/kustomization.yaml apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization namespace: demo resources: - ../base - namespace.yaml # 'patchesStrategicMerge' was removed, but the file # was not added to the 'resources' list.` After: `# k8s/demo/prod/kustomization.yaml apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization namespace: demo resources: - ../base - namespace.yaml - service-aliases.demo.yaml # Add the file as a resource` Suggestion importance[1-10]: 10 __ Why: This suggestion correctly identifies a critical bug in the Kustomize configuration that would cause the deployment of all new service aliases to fail, directly contradicting a primary goal of the PR.	High
Possible issue	Implement deep copy for security configuration The shallow copy of the `security` configuration is insufficient and may lead to unintended mutations. Implement a deep copy, particularly for the `TLS` field, to ensure the original configuration remains unchanged. pkg/agent/external_checker.go [105-120] `if security != nil { + // Deep copy the security config to prevent side effects. cfgCopy := security - cfgCopy.TLS = security.TLS + if security.TLS != nil { + tlsCopy := security.TLS + cfgCopy.TLS = &tlsCopy + } if host != "" && cfgCopy.ServerName != host { log.Info(). Str("original", cfgCopy.ServerName). Str("override", host). Str("service", serviceName). Msg("Adjusting TLS server_name for external checker") cfgCopy.ServerName = host } securityForService = &cfgCopy }` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential bug where a shallow copy of the `security` configuration could lead to unintended side effects if nested pointer fields, like `TLS`, are modified by downstream functions.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1702#issuecomment-3369175289 Original created: 2025-10-05T16:44:07Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Kustomize configuration appears to be broken</summary> ___ **The <code>kustomization.yaml</code> file incorrectly removes <code>service-aliases.demo.yaml</code> from <br><code>patchesStrategicMerge</code> but fails to add it to the <code>resources</code> list. This omission <br>will prevent the new service aliases from being applied by Kustomize.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1702/files#diff-0527e7f19d087f3576d5755a79554797ffbab78b1a7efaa38984b4f3241f6fc9R7-R10">k8s/demo/prod/kustomization.yaml [7-10]</a> </summary> ```yaml resources: - ../base - namespace.yaml - service-aliases.demo.yaml ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1702/files#diff-2a652ee3803d2decc7bf62439ee5712d719b72ebbf146d2a02093d4c06638f13R1-R117">k8s/demo/prod/service-aliases.demo.yaml [1-117]</a> </summary> ```yaml --- apiVersion: v1 kind: Service metadata: name: core namespace: demo spec: type: ExternalName externalName: serviceradar-core.demo.svc.cluster.local --- ... (clipped 107 lines) ``` </details> ### Solution Walkthrough: #### Before: ```yaml # k8s/demo/prod/kustomization.yaml apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization namespace: demo resources: - ../base - namespace.yaml # 'patchesStrategicMerge' was removed, but the file # was not added to the 'resources' list. ``` #### After: ```yaml # k8s/demo/prod/kustomization.yaml apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization namespace: demo resources: - ../base - namespace.yaml - service-aliases.demo.yaml # Add the file as a resource ``` <details><summary>Suggestion importance[1-10]: 10</summary> __ Why: This suggestion correctly identifies a critical bug in the Kustomize configuration that would cause the deployment of all new service aliases to fail, directly contradicting a primary goal of the PR. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Implement deep copy for security configuration</summary> ___ **The shallow copy of the <code>security</code> configuration is insufficient and may lead to <br>unintended mutations. Implement a deep copy, particularly for the <code>TLS</code> field, to <br>ensure the original configuration remains unchanged.** [pkg/agent/external_checker.go [105-120]](https://github.com/carverauto/serviceradar/pull/1702/files#diff-aef4501be72b4e79253b999691c56d7a751dd49548d87eed3284215a8e76aa77R105-R120) ```diff if security != nil { + // Deep copy the security config to prevent side effects. cfgCopy := *security - cfgCopy.TLS = security.TLS + if security.TLS != nil { + tlsCopy := *security.TLS + cfgCopy.TLS = &tlsCopy + } if host != "" && cfgCopy.ServerName != host { log.Info(). Str("original", cfgCopy.ServerName). Str("override", host). Str("service", serviceName). Msg("Adjusting TLS server_name for external checker") cfgCopy.ServerName = host } securityForService = &cfgCopy } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a potential bug where a shallow copy of the `security` configuration could lead to unintended side effects if nested pointer fields, like `TLS`, are modified by downstream functions. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>