[metallb] Build patched image #945
Conversation
WalkthroughThe changes introduce an enhanced build process for the MetalLB system package, including a new Dockerfile and Makefile logic for building and tagging controller and speaker images. The Helm chart and CRDs are updated to support new features, such as dynamic ASN configuration and stricter conditional resource creation. Alert severities and documentation are also revised. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Make as Makefile
participant Docker as Docker Build
participant Helm as Helm Chart
participant K8s as Kubernetes
Dev->>Make: make build
Make->>Make: Build other system packages
Make->>Make: make -C packages/system/metallb image
Make->>Docker: Build controller and speaker images (multi-arch)
Docker->>Make: Output image digests
Make->>Helm: Update values.yaml with new image digests
Helm->>K8s: Deploy updated MetalLB with new images and CRDs
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
packages/system/metallb/charts/metallb/README.md (1)
19-21: Update frr-k8s dependency version in docs
The frr-k8s version has been bumped to0.0.16—good. Consider converting the bare URL in the table to reference-style or angle-bracket form to satisfy markdownlint (MD034).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Bare URL used
null(MD034, no-bare-urls)
packages/system/metallb/charts/metallb/values.yaml (1)
45-46: Clarify Prometheus RBAC comment
Minor wording suggestion:- # prometheus doesn't have the permission to scrape all namespaces so we give it permission to scrape metallb's one + # Grant Prometheus permission to scrape MetalLB's namespace when RBAC is enabledThis improves readability and grammar.
packages/system/metallb/charts/metallb/templates/prometheusrules.yaml (2)
22-23: Use standard YAML quoting for annotations.
The current backtick‐wrapped literals ({{\'…'`}}`) will embed extra single quotes in the rendered alert. It’s clearer and more idiomatic to write:summary: "Stale config on {{ $labels.pod }}" description: "{{ $labels.job }} - MetalLB {{ $labels.container }} on {{ $labels.pod }} has a stale config for > 1 minute"Apply this change for the
MetalLBStaleConfig,MetalLBConfigNotLoaded, andMetalLBAddressPoolExhaustedalerts.Also applies to: 34-35, 46-47
60-61: Unify annotation keys across alerts.
The first three alerts usedescription, butaddressPoolUsageandbgpSessionDownstill usemessage. For consistency and downstream tooling support, replacemessagewithdescription.Also applies to: 72-73
packages/system/metallb/Makefile (1)
14-33: DRY up the image build targets.
Theimage-controllerandimage-speakertargets share almost identical steps. Consider refactoring into a pattern rule or Makefile function to avoid duplication and ease maintenance. For example:TARGETS := controller speaker .PHONY: image $(addprefix image-,$(TARGETS)) image: $(addprefix image-,$(TARGETS)) define build-image $(eval VERSION := $(shell yq '.appVersion' charts/metallb/Chart.yaml)) docker buildx build images/metallb \ --provenance false \ --target $(1) \ --build-arg VERSION=$(VERSION) \ --tag $(REGISTRY)/metallb/$(1):$(VERSION) \ --cache-from type=registry,ref=$(REGISTRY)/metallb/$(1):latest \ --cache-to type=inline \ --metadata-file images/$(1).json \ --push=$(PUSH) \ --label "org.opencontainers.image.source=https://github.com/cozystack/cozystack" \ --load=1 # update values.yaml... endef $(foreach t,$(TARGETS),$(eval image-$(t):;$(call build-image,$(t))))This will make future tweaks (e.g., adding build args) easier.
packages/system/metallb/charts/metallb/templates/rbac.yaml (1)
76-76: Add spacing after commas in verb lists.
YAML lists like["get", "list", "watch","create","update","delete"]should include a space after each comma for readability:verbs: ["get", "list", "watch", "create", "update", "delete"]packages/system/metallb/images/metallb/Dockerfile (3)
14-20: Improve build robustness and log cleanliness.
- Remove the verbose
-vflag fromtarto avoid cluttering build logs (tar -xzf-instead oftar -xzvf-).- Prefix patch application with
set -euxo pipefailso failures abort the build immediately:RUN set -euxo pipefail \ && curl -sSL https://github.com/metallb/metallb/pull/2726.diff -o 2726.diff \ && git apply 2726.diff
54-67: Pin distroless base image for reproducibility.
Usinggcr.io/distroless/static:latestcan introduce unexpected changes. It’s better to pin to a specific digest or version tag (e.g.,gcr.io/distroless/static@sha256:<digest>).
70-76: Ensure the reloader script is executable.
Thefrr-reloader.shfile is copied but its executable bit may not be preserved. Add:COPY --from=builder ... /frr-reloader.sh RUN chmod +x /frr-reloader.shso that the reloader can run at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
packages/system/metallb/charts/metallb/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Makefile(1 hunks)packages/system/metallb/Makefile(2 hunks)packages/system/metallb/charts/metallb/Chart.yaml(2 hunks)packages/system/metallb/charts/metallb/README.md(2 hunks)packages/system/metallb/charts/metallb/charts/crds/Chart.yaml(1 hunks)packages/system/metallb/charts/metallb/charts/crds/templates/crds.yaml(10 hunks)packages/system/metallb/charts/metallb/templates/controller.yaml(1 hunks)packages/system/metallb/charts/metallb/templates/podmonitor.yaml(2 hunks)packages/system/metallb/charts/metallb/templates/prometheusrules.yaml(5 hunks)packages/system/metallb/charts/metallb/templates/rbac.yaml(7 hunks)packages/system/metallb/charts/metallb/templates/service-accounts.yaml(1 hunks)packages/system/metallb/charts/metallb/templates/servicemonitor.yaml(2 hunks)packages/system/metallb/charts/metallb/values.yaml(5 hunks)packages/system/metallb/images/metallb/Dockerfile(1 hunks)packages/system/metallb/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/system/metallb/charts/metallb/templates/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🪛 markdownlint-cli2 (0.17.2)
packages/system/metallb/charts/metallb/README.md
20-20: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (18)
packages/system/metallb/charts/metallb/templates/controller.yaml (1)
87-90: Ensure speaker enablement gate for FRR mode
Good catch adding thespeaker.enabledcheck alongsidespeaker.frr.enabledto prevent settingMETALLB_BGP_TYPE=frrwhen the speaker is turned off. This avoids misconfiguration and unintended behavior.packages/system/metallb/charts/metallb/charts/crds/Chart.yaml (1)
2-3: Bump CRDs chart version to v0.14.9
TheappVersionandversionfields have been correctly updated to v0.14.9. Please verify that the main MetalLB Helm chart (packages/system/metallb/charts/metallb/Chart.yaml) has the same version bump to maintain consistency across charts and that anyChart.lockor dependency references are updated accordingly.Also applies to: 10-10
Makefile (1)
23-23: Integrate MetalLB image build into standard flow
Nice addition of themake -C packages/system/metallb imagestep. Ensure theimagetarget inpackages/system/metallb/Makefilehandles push/load flags and errors gracefully. Also confirm CI pipelines invokemake buildso the new images are always built.packages/system/metallb/charts/metallb/values.yaml (5)
67-69: The addition of the comment forprometheus.podMonitor.additionalLabelsis self-explanatory and doesn’t require further action.
146-148: The new comment forprometheusRule.additionalLabelsfollows the same pattern and is clear as-is.
168-169: ElevateaddressPoolExhaustedseverity to Critical
Updating the severity tocriticalaligns with higher-impact alerts. Ensure theprometheusrules.yamltemplate reflects this default.
181-182: Elevate 95%addressPoolUsagethreshold to Critical
Good to mark the highest utilization threshold ascritical. This will trigger faster escalations when pools are nearly exhausted.
187-188: ElevatebgpSessionDownseverity to Critical
Marking BGP sessions down ascriticalis appropriate given its potential to disrupt service.packages/system/metallb/charts/metallb/templates/service-accounts.yaml (1)
16-16: Ensurespeaker.serviceAccount.createis defined in values.yaml
The new condition requires both.Values.speaker.enabledand.Values.speaker.serviceAccount.create. Please confirm that a default forspeaker.serviceAccount.createexists invalues.yaml(e.g.,false) to avoid unexpected omissions.packages/system/metallb/charts/metallb/Chart.yaml (1)
2-2: Verify coordinated chart and dependency version bumps
TheappVersion, dependency versions forcrdsandfrr-k8s, and chartversionhave been updated (0.14.9and0.0.16). Ensure these align with the upstream MetalLB and FRR-K8s releases and that all sub-charts remain compatible.Also applies to: 7-7, 11-11, 21-21
packages/system/metallb/values.yaml (1)
4-11: Explicitly pin controller and speaker images with digests
Great to see the controller and speaker images pinned by digest. Please verify that these digests match the artifacts produced by your build pipeline, and consider specifying an explicitimage.pullPolicy(e.g.,IfNotPresentorAlways) if you have specific pull semantics in mind.packages/system/metallb/charts/metallb/templates/servicemonitor.yaml (2)
1-3: Prevent conflicting ServiceMonitor and PodMonitor enablement
Adding afailwhen bothprometheus.serviceMonitor.enabledandprometheus.podMonitor.enabledare set prevents resource collisions—this is a strong safeguard against misconfiguration.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
6-97: Wrap speaker ServiceMonitor inspeaker.enabledcheck
The speaker-specific ServiceMonitor and Service definitions now only render when.Values.speaker.enabledis true, mirroring the RBAC and ServiceAccount templates. Please confirm thatspeaker.enableddefaults to a sensible value invalues.yamland update the chart README to document this new toggle.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
packages/system/metallb/charts/metallb/templates/podmonitor.yaml (1)
39-78: Conditionally create speaker PodMonitor
The speaker PodMonitor resource is now created only if.Values.speaker.enabledis true, aligning with other component toggles. Verify that this flag is defined (and documented) invalues.yaml, and update the chart documentation to cover enabling/disabling the speaker PodMonitor.packages/system/metallb/charts/metallb/templates/rbac.yaml (1)
22-27: Verify removal of mutating webhook permissions.
The controller ClusterRole no longer includesmutatingwebhookconfigurations. Ensure there is no active mutating webhook (e.g.,metallb-webhook-configuration) that still needs permissions; otherwise, this change may break webhook cleanup or updates.packages/system/metallb/charts/metallb/charts/crds/templates/crds.yaml (3)
5-5: Approve controller-gen version bumps.
Updating thecontroller-gen.kubebuilder.io/versionannotation tov0.16.3across all CRDs aligns with the chart version and the patched binaries.Also applies to: 126-126, 332-332, 682-682, 747-747, 957-957, 1137-1137
368-370: Approve deprecation of v1beta1 BGPPeer.
Marking thev1beta1version asdeprecated: truewith a clear deprecation warning is the correct approach to guide users towardv1beta2.
638-641: Approve updatedpeerASNdescription.
The inclusion of mutual-exclusivity guidance in thepeerASNfield’s description is helpful for users migrating todynamicASN.
85d7dc7 to
b38d387
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/system/metallb/charts/metallb/README.md (1)
20-20: Verify version bump and URL formatting.The update from
0.0.14to0.0.16for thefrr-k8sdependency aligns with the Chart metadata. To address the markdownlint MD034 warning for a bare URL, consider using link syntax:-| https://metallb.github.io/frr-k8s | frr-k8s | 0.0.16 | +| [https://metallb.github.io/frr-k8s](https://metallb.github.io/frr-k8s) | frr-k8s | 0.0.16 |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
packages/system/metallb/charts/metallb/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Makefile(1 hunks)packages/system/metallb/Makefile(2 hunks)packages/system/metallb/charts/metallb/Chart.yaml(2 hunks)packages/system/metallb/charts/metallb/README.md(2 hunks)packages/system/metallb/charts/metallb/charts/crds/Chart.yaml(1 hunks)packages/system/metallb/charts/metallb/charts/crds/templates/crds.yaml(10 hunks)packages/system/metallb/charts/metallb/templates/controller.yaml(1 hunks)packages/system/metallb/charts/metallb/templates/podmonitor.yaml(2 hunks)packages/system/metallb/charts/metallb/templates/prometheusrules.yaml(5 hunks)packages/system/metallb/charts/metallb/templates/rbac.yaml(7 hunks)packages/system/metallb/charts/metallb/templates/service-accounts.yaml(1 hunks)packages/system/metallb/charts/metallb/templates/servicemonitor.yaml(2 hunks)packages/system/metallb/charts/metallb/values.yaml(5 hunks)packages/system/metallb/images/metallb/Dockerfile(1 hunks)packages/system/metallb/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/system/metallb/charts/metallb/templates/controller.yaml
- packages/system/metallb/charts/metallb/templates/service-accounts.yaml
- packages/system/metallb/charts/metallb/values.yaml
- Makefile
- packages/system/metallb/values.yaml
- packages/system/metallb/charts/metallb/charts/crds/Chart.yaml
- packages/system/metallb/charts/metallb/templates/prometheusrules.yaml
- packages/system/metallb/charts/metallb/templates/podmonitor.yaml
- packages/system/metallb/charts/metallb/Chart.yaml
- packages/system/metallb/images/metallb/Dockerfile
- packages/system/metallb/charts/metallb/charts/crds/templates/crds.yaml
- packages/system/metallb/charts/metallb/templates/rbac.yaml
- packages/system/metallb/Makefile
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/system/metallb/charts/metallb/README.md
20-20: Bare URL used
null
(MD034, no-bare-urls)
🪛 YAMLlint (1.35.1)
packages/system/metallb/charts/metallb/templates/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (5)
packages/system/metallb/charts/metallb/templates/servicemonitor.yaml (2)
1-3: Fail early on conflicting monitor flags
Thisfailblock correctly halts chart rendering if bothprometheus.serviceMonitor.enabledandprometheus.podMonitor.enabledare set, avoiding ambiguous Prometheus configurations and preventing users from deploying an invalid setup.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
6-97: Scope speaker ServiceMonitor and Service to speaker.enabled
Adding{{- if .Values.speaker.enabled }}around the speakerServiceMonitorand its backingServiceensures these resources are only rendered when the speaker component is actually enabled. This matches the gating applied in RBAC, ServiceAccount, and PodMonitor templates, improving chart modularity and preventing orphaned resources.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 56-56: trailing spaces
(trailing-spaces)
packages/system/metallb/charts/metallb/README.md (3)
82-82: Approve severity increase for addressPoolExhausted.Bumping the default severity to
"critical"for theaddressPoolExhaustedalert is appropriate given the impact of pool exhaustion on cluster networking.
88-88: Approve severity increase for high address pool usage.Changing the 95% threshold severity to
"critical"accurately reflects the urgency when address pool utilization is critically high.
92-92: Approve severity increase for BGP session down alert.Updating the
bgpSessionDownalert severity to"critical"is consistent with its potential service-disrupting impact.
Since it's taking a while for metallb/metallb#2726 to get released, the binaries with the fix are recompiled in-tree. Workaround for #909. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
b38d387 to
5fe7b3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/system/metallb/charts/metallb/README.md (1)
7-7: Nit: convert bare URL to a named markdown link
To comply with MD034 (no bare URLs), consider rewriting**Homepage:** <https://metallb.universe.tf>as
**Homepage:** [metallb.universe.tf](https://metallb.universe.tf)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
packages/system/metallb/charts/metallb/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Makefile(1 hunks)packages/system/metallb/Makefile(2 hunks)packages/system/metallb/charts/metallb/Chart.yaml(2 hunks)packages/system/metallb/charts/metallb/README.md(2 hunks)packages/system/metallb/charts/metallb/charts/crds/Chart.yaml(1 hunks)packages/system/metallb/charts/metallb/charts/crds/templates/crds.yaml(10 hunks)packages/system/metallb/charts/metallb/templates/controller.yaml(1 hunks)packages/system/metallb/charts/metallb/templates/podmonitor.yaml(2 hunks)packages/system/metallb/charts/metallb/templates/prometheusrules.yaml(5 hunks)packages/system/metallb/charts/metallb/templates/rbac.yaml(7 hunks)packages/system/metallb/charts/metallb/templates/service-accounts.yaml(1 hunks)packages/system/metallb/charts/metallb/templates/servicemonitor.yaml(2 hunks)packages/system/metallb/charts/metallb/values.yaml(5 hunks)packages/system/metallb/images/metallb/Dockerfile(1 hunks)packages/system/metallb/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/system/metallb/charts/metallb/values.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/system/metallb/charts/metallb/templates/service-accounts.yaml
- packages/system/metallb/values.yaml
- packages/system/metallb/charts/metallb/templates/controller.yaml
- packages/system/metallb/charts/metallb/Chart.yaml
- packages/system/metallb/charts/metallb/charts/crds/Chart.yaml
- Makefile
- packages/system/metallb/charts/metallb/templates/prometheusrules.yaml
- packages/system/metallb/charts/metallb/templates/podmonitor.yaml
- packages/system/metallb/Makefile
- packages/system/metallb/images/metallb/Dockerfile
- packages/system/metallb/charts/metallb/templates/rbac.yaml
- packages/system/metallb/charts/metallb/charts/crds/templates/crds.yaml
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/system/metallb/charts/metallb/README.md
20-20: Bare URL used
null
(MD034, no-bare-urls)
🪛 YAMLlint (1.35.1)
packages/system/metallb/charts/metallb/templates/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (4)
packages/system/metallb/charts/metallb/README.md (2)
20-20: Confirmfrr-k8sdependency version bump
The README now reflectsfrr-k8sversion0.0.16, consistent with the Chart.yaml and CRD updates. Ensure the actual Chart.yaml dependency has been bumped accordingly and remind users to runhelm dependency updatewhen upgrading.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Bare URL used
null(MD034, no-bare-urls)
82-82: Approve severity level updates for Prometheus alerts
The default severities foraddressPoolExhausted, the 95%addressPoolUsagethreshold, andbgpSessionDownhave been correctly elevated to"critical", aligning the documentation with the changes invalues.yaml.Also applies to: 88-88, 92-92
packages/system/metallb/charts/metallb/templates/servicemonitor.yaml (2)
1-3: Add conflict guard for ServiceMonitor vs PodMonitor
The new validation check halts template rendering if both.Values.prometheus.serviceMonitor.enabledand.Values.prometheus.podMonitor.enabledare set, preventing duplicate scrape configurations. Good alignment with Helm best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
6-6: Wrap speaker resources in the speaker.enabled guard
Introducing{{- if .Values.speaker.enabled }}around the speaker ServiceMonitor and Service ensures these resources are only rendered when the speaker component is active, matching the patterns in other templates.
Since it's taking a while for metallb/metallb#2726 to get released, the binaries with the fix are recompiled in-tree. Workaround for #909. (cherry picked from commit 73fdc5d) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Since it's taking a while for metallb/metallb#2726 to get released, the binaries with the fix are recompiled in-tree. Workaround for #909. (cherry picked from commit 73fdc5d) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Add support for metallb multiarch build. Part of #519 and a follow-up to PR #945 (issue #909) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Improved Docker build process for image-controller and image-speaker to allow dynamic control over image loading and enhanced build configuration consistency. No changes to user-facing features. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Since it's taking a while for metallb/metallb#2726 to get released, the binaries with the fix are recompiled in-tree. Workaround for #909.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes