feat(lighthouse): add support for LB service #568
Conversation
WalkthroughAdds configurable P2P Service exposure and Grafana Operator dashboard support to the Lighthouse Helm chart: new values for grafana.operatorDashboards and lighthouse.p2p.service, a conditional P2P LoadBalancer Service template, an ENR branch in the StatefulSet for LoadBalancer addresses, and a template emitting GrafanaDashboard CRs from dashboards/. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Helm
participant K8s
participant Service as "P2P LoadBalancer Service"
participant Pods as "StatefulSet / Pods"
User->>Helm: Set p2p.service.enabled=true, type=LoadBalancer
Helm->>K8s: Render & apply Service + StatefulSet templates
K8s-->>Service: Create LoadBalancer Service
K8s-->>Pods: Create/Update StatefulSet Pods
rect rgb(220,240,255)
Note over Service,Pods: LoadBalancer routes P2P TCP/UDP ports to Pods
end
sequenceDiagram
autonumber
participant Values
participant Template
participant PodArgs
Values-->>Template: p2pHostPort.enabled=false\np2p.service.enabled=true\np2p.service.type=LoadBalancer
alt advertiseIP present
Template->>PodArgs: --enr-address=<advertiseIP>
else loadBalancerIP present
Template->>PodArgs: --enr-address=<loadBalancerIP>
else none
Template->>PodArgs: (no explicit ENR address)
end
Template->>PodArgs: --enr-udp-port=<discoveryPort>\n--enr-tcp-port=<port>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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: 0
🧹 Nitpick comments (4)
charts/lighthouse/templates/lighthouse/statefulset.yaml (1)
163-171: Guard against missing ENR address when P2P LB is enabledIf neither advertiseIP nor loadBalancerIP is set, ENR will lack a routable address while auto-update is disabled, likely breaking inbound peering. Add a Helm validation to fail fast when both are empty.
- {{- else if and $values.p2p $values.p2p.service $values.p2p.service.enabled (eq $values.p2p.service.type "LoadBalancer") }} + {{- else if and $values.p2p $values.p2p.service $values.p2p.service.enabled (eq $values.p2p.service.type "LoadBalancer") }} + {{- if not (or $values.p2p.service.advertiseIP $values.p2p.service.loadBalancerIP) }} + {{- fail "lighthouse.p2p.service enabled: set either advertiseIP or loadBalancerIP so ENR has a routable address (enr auto-update is disabled)" }} + {{- end }} {{- if $values.p2p.service.advertiseIP }} --enr-address={{ $values.p2p.service.advertiseIP }} \ {{- else if $values.p2p.service.loadBalancerIP }} --enr-address={{ $values.p2p.service.loadBalancerIP }} \ {{- end }} --enr-udp-port={{ include "lighthouse.discoveryPort" $values }} \ --enr-tcp-port={{ include "lighthouse.port" $values }} \charts/lighthouse/templates/lighthouse/service.yaml (1)
85-137: Avoid double exposure when hostPort is enabledIf p2pHostPort.enabled is true, we likely don’t want to also create a LoadBalancer P2P Service. Guard on not p2pHostPort.enabled to prevent conflicting exposure modes.
-{{- if and $values.p2p $values.p2p.service $values.p2p.service.enabled (eq (default "" $values.p2p.service.type) "LoadBalancer") }} +{{- if and (not $values.p2pHostPort.enabled) $values.p2p $values.p2p.service $values.p2p.service.enabled (eq (default "" $values.p2p.service.type) "LoadBalancer") }}charts/lighthouse/values.yaml (1)
176-196: Clarify advertiseIP semantics (IP or DNS) and expectationLighthouse accepts an IP or DNS name. Also hint that one of advertiseIP or loadBalancerIP should be set for stable ENR when using the LB path.
- # -- IP address to explicitly advertise in ENR (overrides autodetection and LB IP) + # -- IP or DNS name to explicitly advertise in ENR (overrides autodetection and LB IP). + # -- Recommended when using the P2P LoadBalancer; set this or `loadBalancerIP` to ensure a stable ENR address. advertiseIP: ""charts/lighthouse/README.md (1)
96-104: Expand advertiseIP description: allow DNS namesDocs currently imply IP-only. Lighthouse accepts IP or DNS; updating helps operators using stable hostnames.
-| lighthouse.p2p.service.advertiseIP | IP address to explicitly advertise in ENR (overrides autodetection and LB IP) | string | `""` | +| lighthouse.p2p.service.advertiseIP | IP or DNS name to explicitly advertise in ENR (overrides autodetection and LB IP) | string | `""` |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
charts/lighthouse/README.md(1 hunks)charts/lighthouse/templates/lighthouse/service.yaml(1 hunks)charts/lighthouse/templates/lighthouse/statefulset.yaml(1 hunks)charts/lighthouse/values.yaml(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
charts/lighthouse/templates/dashboards-operator.yaml (2)
46-48: Guard against key collisions when merging extraSpecInjecting extraSpec at the same level can duplicate keys (e.g., redefining folder/uid). If that’s intended, fine—the last key wins—but it’s brittle. Either document that extraSpec overrides built-ins or refactor to explicitly merge with precedence.
53-54: Remove trailing blank lineMinor cleanliness; avoids noisy linters.
-{{- end }} - +{{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/lighthouse/templates/dashboards-operator.yaml(1 hunks)charts/lighthouse/values.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lighthouse/templates/dashboards-operator.yaml
[warning] 53-53: too many blank lines (1 > 0)
(empty-lines)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (3)
charts/lighthouse/values.yaml (2)
69-91: Operator dashboards block looks solidDefaults and field names align with Grafana Operator’s GrafanaDashboard spec. No blockers from values perspective.
198-219: Enforce single exposure path and add optional LB fields
The chart’s templates already ensure mutual exclusivity between hostPort and Service exposure:p2pHostPort.enabledis evaluated first, and the LoadBalancer branch is only applied when hostPort is disabled. Add the following optional values tocharts/lighthouse/values.yamlto support broader LB compatibility:p2p: service: enabled: false type: LoadBalancer labels: {} annotations: {} externalTrafficPolicy: Local externalIPs: [] loadBalancerIP: "" loadBalancerSourceRanges: [] + # -- Set a specific LoadBalancer implementation (K8s >=1.24) + loadBalancerClass: "" + # -- Dual-stack controls (if cluster supports it) + ipFamilyPolicy: "" + ipFamilies: [] + # -- Preserve/assign node ports for LB backends (K8s behavior toggle) + allocateLoadBalancerNodePorts: null + # -- For providers requiring explicit HC port with Local policy + healthCheckNodePort: null # -- IP address to explicitly advertise in ENR (overrides autodetection and LB IP) advertiseIP: ""charts/lighthouse/templates/dashboards-operator.yaml (1)
1-1: YAMLlint “syntax error” is a template false-positiveThe linter doesn’t parse Helm directives. Safe to ignore or exclude templates from yamllint; rely on helm lint and CI render checks.
| apiVersion: grafana.integreatly.org/v1beta1 | ||
| kind: GrafanaDashboard | ||
| metadata: | ||
| name: {{ include "lighthouse.fullname" . }}-{{ base $path | replace "." "-" | trunc 63 | trimSuffix "-" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden resource name sanitization (handle underscores/uppercase/whitespace)
Current transform only replaces dots. K8s names must be DNS-1123 compatible; filenames with _, spaces, or uppercase could render invalid names. Suggest:
- name: {{ include "lighthouse.fullname" . }}-{{ base $path | replace "." "-" | trunc 63 | trimSuffix "-" }}
+ name: {{ include "lighthouse.fullname" . }}-{{ base $path | lower | regexReplaceAll "[^a-z0-9-]+" "-" | trunc 63 | trimSuffix "-" }}📝 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.
| name: {{ include "lighthouse.fullname" . }}-{{ base $path | replace "." "-" | trunc 63 | trimSuffix "-" }} | |
| name: {{ include "lighthouse.fullname" . }}-{{ base $path | lower | regexReplaceAll "[^a-z0-9-]+" "-" | trunc 63 | trimSuffix "-" }} |
🤖 Prompt for AI Agents
In charts/lighthouse/templates/dashboards-operator.yaml around line 10, the name
template only replaces dots and can produce invalid DNS-1123 names when source
contains underscores, spaces or uppercase; update the template to first
lower-case the input (use lower), replace underscores and spaces with hyphens,
use regexReplaceAll to convert any non [a-z0-9-] to hyphens, truncate to 63
chars, then trim leading/trailing hyphens to ensure a valid DNS-1123 label. For
example, pipe the value through lower, replace "_" and " " with "-", then
regexReplaceAll "[^a-z0-9-]" "-", apply trunc 63, and trimSuffix/trimPrefix "-"
before appending.
cjorge-graphops
force-pushed
the
feat/lighthouse-lb
branch
from
533c18d to
1361c4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
charts/lighthouse/templates/dashboards-operator.yaml (1)
10-10: Fix DNS-1123 name sanitization for dashboard resourcesUnderscores, spaces, and uppercase in filenames can yield invalid names; current transform only replaces dots. Harden the transform.
- name: {{ include "lighthouse.fullname" $ }}-{{ base $path | replace "." "-" | trunc 63 | trimSuffix "-" }} + name: {{ include "lighthouse.fullname" $ }}-{{ base $path | lower | regexReplaceAll "[^a-z0-9-]+" "-" | trunc 63 | trimSuffix "-" | trimPrefix "-" }}
🧹 Nitpick comments (3)
charts/lighthouse/templates/dashboards-operator.yaml (1)
11-15: Remove redundant nested if inside withThe inner if is unnecessary since with already checks emptiness.
- {{- with $op.namespace }} - {{- if . }} - namespace: {{ . }} - {{- end }} - {{- end }} + {{- with $op.namespace }} + namespace: {{ . }} + {{- end }}charts/lighthouse/values.yaml (2)
69-91: Values shape looks good; consider documenting/validating operator expectations
- Types align with the template usage.
- Optional: add schema (values.schema.json) to enforce types (bools/strings/maps) and acceptable patterns (e.g., resyncPeriod duration).
198-219: Clarify mutual exclusivity with p2pHostPort and add validationIf both p2pHostPort.enabled and p2p.service.enabled are true, define precedence or fail fast with a clear Helm error.
Example (in a helper or top-level template):
+{{- if and .Values.lighthouse.p2pHostPort.enabled .Values.lighthouse.p2p.service.enabled -}} +{{- fail "Configure either lighthouse.p2pHostPort.enabled or lighthouse.p2p.service.enabled, not both." -}} +{{- end -}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
charts/lighthouse/README.md(2 hunks)charts/lighthouse/templates/dashboards-operator.yaml(1 hunks)charts/lighthouse/values.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/lighthouse/README.md
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lighthouse/templates/dashboards-operator.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (2)
charts/lighthouse/templates/dashboards-operator.yaml (2)
46-48: Confirm override semantics of extraSpecextraSpec is emitted after built-in fields, so it can override folder/uid/etc. If that’s intended, keep; if not, move extraSpec before those fields.
1-3: Yamllint parser warning is a false positive on Helm templatesThe reported syntax error stems from yamllint not understanding Go templating. Consider excluding templates/ in yamllint or using a Helm-aware linter.
1 participant
Summary by CodeRabbit
New Features
Documentation