Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MetalLB v0.14.8 #492

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Update MetalLB v0.14.8 #492

merged 1 commit into from
Dec 2, 2024

Conversation

kvaps
Copy link
Member

@kvaps kvaps commented Dec 2, 2024

Summary by CodeRabbit

  • New Features

    • Upgraded MetalLB application version to v0.14.8.
    • Introduced a new frr-k8s dependency for enhanced BGP management.
    • Added new configuration options for TLS settings and extra containers in the controller.
    • Implemented new Custom Resource Definitions (CRDs) for managing FRR configurations and node states.
  • Bug Fixes

    • Improved validation logic for service account names to ensure consistency.
  • Documentation

    • Updated README files for the MetalLB and frr-k8s charts to reflect new features and configuration options.
  • Refactor

    • Enhanced RBAC configurations for better resource management and security.
    • Improved webhook configurations for better validation and consistency.
  • Chores

    • Updated various YAML configuration files to include namespace specifications for clarity.

@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Dec 2, 2024
Copy link
Contributor

coderabbitai bot commented Dec 2, 2024

Walkthrough

The changes in this pull request involve significant updates to the MetalLB Helm chart, including version upgrades, new dependencies, and modifications to configuration files. The Chart.yaml files for both MetalLB and its new frr-k8s dependency have been updated to reflect new versions. The README files have been enhanced with new configuration options. New Custom Resource Definitions (CRDs) for managing FRR configurations have been introduced, along with various templates for Kubernetes resources, including RBAC settings, webhooks, and service monitors. Overall, these updates improve the configurability and functionality of the MetalLB project.

Changes

File Change Summary
packages/system/metallb/charts/metallb/Chart.yaml Updated appVersion, version, and added new dependency frr-k8s.
packages/system/metallb/charts/metallb/README.md Added entries in "Requirements" and "Values" sections; updated speaker.frr.image.tag.
packages/system/metallb/charts/metallb/charts/crds/Chart.yaml Updated appVersion and version.
packages/system/metallb/charts/metallb/charts/frr-k8s/.helmignore Added new .helmignore file for frr-k8s.
packages/system/metallb/charts/metallb/charts/frr-k8s/Chart.yaml Introduced new Chart.yaml for frr-k8s with metadata and dependencies.
packages/system/metallb/charts/metallb/charts/frr-k8s/README.md Created new README for frr-k8s with configuration details.
packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/.helmignore Added new .helmignore for frr-k8s CRDs.
packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/Chart.yaml Added Chart.yaml for crds with metadata.
packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/README.md Created README for frr-k8s CRDs.
packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/templates/frrk8s.metallb.io_frrconfigurations.yaml Added new CRD FRRConfiguration with schema for BGP settings.
packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/templates/frrk8s.metallb.io_frrnodestates.yaml Added new CRD FRRNodeState for managing FRR instance states.
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/NOTES.txt Updated to include configuration guidance for FRR-k8s.
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/_helpers.tpl Added template functions for generating names and labels.
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/controller.yaml Introduced new ConfigMap and updated DaemonSet configuration.
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/rbac.yaml Added RBAC configurations for frr-k8s controller and speaker.
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-accounts.yaml Added ServiceAccount configuration with namespace.
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-monitor.yaml Introduced ServiceMonitor and Service configurations for Prometheus monitoring.
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/webhooks.yaml Added and updated webhook configurations for validation.
packages/system/metallb/charts/metallb/charts/frr-k8s/values.schema.json Introduced JSON schema for validating frr-k8s configuration.
packages/system/metallb/charts/metallb/charts/frr-k8s/values.yaml Added default values for frr-k8s configuration.
packages/system/metallb/charts/metallb/policy/controller.rego Updated validation logic for serviceAccountName.
packages/system/metallb/charts/metallb/policy/speaker.rego Updated validation logic for serviceAccountName.
packages/system/metallb/charts/metallb/templates/_helpers.tpl Added metrics-related template definitions.
packages/system/metallb/charts/metallb/templates/controller.yaml Updated deployment configuration for TLS settings and extra containers.
packages/system/metallb/charts/metallb/templates/exclude-l2-config.yaml Modified ConfigMap rendering conditions and interface name formats.
packages/system/metallb/charts/metallb/templates/prometheusrules.yaml Updated Prometheus alerting rules to use regex matching.
packages/system/metallb/charts/metallb/templates/rbac.yaml Restructured RBAC permissions, focusing on specific resources.
packages/system/metallb/charts/metallb/templates/service-accounts.yaml Added namespace field to ServiceAccount metadata.
packages/system/metallb/charts/metallb/templates/servicemonitor.yaml Added namespace field to ServiceMonitor and Service metadata.
packages/system/metallb/charts/metallb/templates/speaker.yaml Introduced mutual exclusivity checks for frr and frrk8s configurations.
packages/system/metallb/charts/metallb/templates/webhooks.yaml Enhanced webhook configurations for MetalLB.
packages/system/metallb/charts/metallb/values.schema.json Updated schema to reflect changes in controller and speaker properties.
packages/system/metallb/charts/metallb/values.yaml Added new configuration fields and updated existing ones for MetalLB.

Poem

In the meadow where bunnies play,
MetalLB has found a way.
With frr-k8s now in tow,
Configs bloom and features grow!
Hops of joy, we dance and cheer,
For upgrades bring us all good cheer! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@dosubot dosubot bot added the enhancement New feature or request label Dec 2, 2024
@kvaps kvaps merged commit 943dcd0 into main Dec 2, 2024
2 checks passed
@kvaps kvaps deleted the upd-metallb branch December 2, 2024 18:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Outside diff range and nitpick comments (20)
packages/system/metallb/charts/metallb/templates/exclude-l2-config.yaml (1)

1-24: Consider documenting the interface exclusion patterns

The ConfigMap would benefit from comments explaining:

  • Why these specific interfaces are excluded
  • What happens if an interface is not excluded
  • How to add custom interface patterns

Would you like me to help draft the documentation?

🧰 Tools
🪛 yamllint (1.35.1)

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

packages/system/metallb/charts/metallb/templates/_helpers.tpl (1)

Line range hint 82-93: LGTM! Consider adding documentation comments.

The port name template definitions are well-structured and provide consistent naming based on security settings. They align well with the new FRR integration.

Consider adding documentation comments similar to other templates in this file:

+{{/*
+Define the metrics port name based on security settings.
+*/}}
 {{- define "metrics.exposedportname" -}}
 {{- if .Values.prometheus.secureMetricsPort -}}
 "metricshttps"
 {{- else -}}
 "metrics"
 {{- end -}}
 {{- end -}}

+{{/*
+Define the FRR metrics port name based on security settings.
+*/}}
 {{- define "metrics.exposedfrrportname" -}}
packages/system/metallb/charts/metallb/templates/prometheusrules.yaml (1)

24-24: Consider documenting the job naming pattern

The alerting rules now match jobs with any suffix after the MetalLB fullname. Consider adding a comment explaining the expected job name patterns to help users understand which jobs will trigger these alerts.

Also applies to: 36-36, 74-74

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/_helpers.tpl (2)

34-52: Consider adding recommended Kubernetes metadata labels

While the current labels follow the basic Kubernetes conventions, consider adding these recommended labels for better resource management:

  • app.kubernetes.io/component
  • app.kubernetes.io/part-of
 {{- define "frrk8s.labels" -}}
 helm.sh/chart: {{ include "frrk8s.chart" . }}
 {{ include "frrk8s.selectorLabels" . }}
 {{- if .Chart.AppVersion }}
 app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
 {{- end }}
 app.kubernetes.io/managed-by: {{ .Release.Service }}
+app.kubernetes.io/component: frr-controller
+app.kubernetes.io/part-of: metallb
 {{- end }}

54-63: Clarify template name and documentation

The template name frrk8s.serviceAccountName and its documentation could be more specific since it's used for the controller service account.

 {{/*
-Create the name of the frrk8s service account to use
+Create the name of the FRR controller service account to use
 */}}
-{{- define "frrk8s.serviceAccountName" -}}
+{{- define "frrk8s.controllerServiceAccountName" -}}
packages/system/metallb/charts/metallb/templates/speaker.yaml (3)

249-260: Document new configuration options

New command-line arguments have been added for WAN configuration and FRRk8s integration. Consider adding documentation for these options in the README.


539-541: Document extraContainers configuration

The new extraContainers support enhances extensibility. Consider adding documentation and examples for this feature in the README.


Line range hint 1-559: Consider providing a migration guide

Given the significant changes in this update, particularly the FRR version upgrade and new configuration options, consider providing a migration guide in the documentation to help users upgrade smoothly.

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/rbac.yaml (1)

1-33: LGTM! Consider adding documentation comments.

The ClusterRole permissions follow security best practices and the principle of least privilege. Each resource has appropriately scoped permissions.

Consider adding comments to document the purpose of each permission block:

 rules:
+# Allow reading FRR configurations
 - apiGroups: ["frrk8s.metallb.io"]
   resources: ["frrconfigurations"]
   verbs: ["get", "list", "watch"]
+# Allow full management of FRR node states
 - apiGroups: ["frrk8s.metallb.io"]
   resources: ["frrnodestates"]
   verbs: ["get", "list", "watch", "create", "delete", "patch", "update"]
🧰 Tools
🪛 yamllint (1.35.1)

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

packages/system/metallb/charts/metallb/charts/frr-k8s/values.yaml (1)

118-122: Consider defining default resource limits

While resource limits are provided as examples, it's recommended to set default values to prevent potential resource exhaustion.

-  resources: {}
+  resources:
+    requests:
+      cpu: 100m
+      memory: 100Mi
+    limits:
+      cpu: 200m
+      memory: 200Mi
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/controller.yaml (3)

82-82: Consider setting MAX_FDS for large BGP deployments

The MAX_FDS setting is currently commented out. For large BGP deployments with many peers, you might want to explicitly set this value to prevent running out of file descriptors.


141-141: Review security implications of host network access

While hostNetwork: true is required for BGP functionality, it increases the security risk. The security is partially mitigated by:

  • Dropping all capabilities and only adding required ones (NET_RAW)
  • Using read-only root filesystem
  • Preventing privilege escalation

Consider documenting these security implications and mitigations in the deployment guide.

Also applies to: 250-258


369-373: Consider adding resource limits

While resource requests are specified for the kube-rbac-proxy containers, no resource limits are set. This could potentially lead to resource contention issues.

Apply this diff:

        resources:
          requests:
            cpu: 10m
            memory: 20Mi
+         limits:
+           cpu: 100m
+           memory: 50Mi

Also applies to: 396-399

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-accounts.yaml (1)

16-16: Minor: Remove extra blank lines at the end of file.

There are too many blank lines at the end of the file.

 {{- end }}
-
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 16-16: too many blank lines

(1 > 0) (empty-lines)

packages/system/metallb/charts/metallb/README.md (1)

20-20: Fix bare URL in repository link

The URL should be properly formatted according to markdown standards.

-| https://metallb.github.io/frr-k8s | frr-k8s | 0.0.14 |
+| [https://metallb.github.io/frr-k8s](https://metallb.github.io/frr-k8s) | frr-k8s | 0.0.14 |
🧰 Tools
🪛 Markdownlint (0.35.0)

20-20: null
Bare URL used

(MD034, no-bare-urls)

packages/system/metallb/charts/metallb/values.yaml (1)

251-252: Consider explicitly configuring TLS cipher suites.

While setting TLS 1.2 as the minimum version is good for security, leaving tlsCipherSuites empty will use default cipher suites. Consider explicitly configuring secure cipher suites to ensure strong TLS security.

Example configuration:

  tlsMinVersion: "VersionTLS12"
- tlsCipherSuites: ""
+ tlsCipherSuites: "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"
packages/system/metallb/charts/metallb/charts/frr-k8s/values.schema.json (1)

287-303: Add descriptions for FRR configuration properties.

While the schema structure is good, consider adding descriptions for the FRR configuration properties to improve documentation.

  "frr": {
    "description": "The FRR properties in the controller",
    "type": "object",
    "properties": {
      "image": {
+       "description": "FRR container image configuration",
        "$ref": "#/definitions/component/properties/image"
      },
      "metricsPort": {
+       "description": "Port number for FRR metrics endpoint",
        "type": "integer"
      },
      "secureMetricsPort": {
+       "description": "Port number for secure FRR metrics endpoint",
        "type": "integer"
      },
      "resources:": {
+       "description": "Resource requirements for FRR container",
        "type": "object"
      }
    }
packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/templates/frrk8s.metallb.io_frrconfigurations.yaml (1)

175-183: Enhance validation error messages

The connect time validation rules are correct but could benefit from more user-friendly error messages.

Consider updating the error messages to be more descriptive:

                              x-kubernetes-validations:
                              - message: connect time should be between 1 seconds
-                                 to 65535
+                                 to 65535 seconds (approximately 18 hours)
                                rule: duration(self).getSeconds() >= 1 && duration(self).getSeconds()
                                  <= 65535
                              - message: connect time should contain a whole number
-                                 of seconds
+                                 of seconds (milliseconds are not supported)
                                rule: duration(self).getMilliseconds() % 1000 ==
                                  0
packages/system/metallb/charts/metallb/charts/frr-k8s/README.md (2)

13-19: Enhance version compatibility documentation

While the minimum Kubernetes version is specified, it would be helpful to include:

  1. Compatible MetalLB versions
  2. FRR version compatibility matrix

49-49: Fix log level description style

The log level description contains a style issue with the colon usage.

-| frrk8s.logLevel | string | `"info"` | Controller log level. Must be one of: `all`, `debug`, `info`, `warn`, `error` or `none` |
+| frrk8s.logLevel | string | `"info"` | Controller log level. Must be one of these values: `all`, `debug`, `info`, `warn`, `error` or `none` |
🧰 Tools
🪛 LanguageTool

[typographical] ~49-~49: Do not use a colon (:) before a series that is introduced by a preposition (‘of’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...o"| Controller log level. Must be one of:all, debug, info, warn, error`...

(RP_COLON)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between edbbb9b and 0d40f3b.

⛔ Files ignored due to path filters (2)
  • packages/system/metallb/charts/metallb/Chart.lock is excluded by !**/*.lock
  • packages/system/metallb/charts/metallb/charts/frr-k8s/Chart.lock is excluded by !**/*.lock
📒 Files selected for processing (33)
  • packages/system/metallb/charts/metallb/Chart.yaml (2 hunks)
  • packages/system/metallb/charts/metallb/README.md (5 hunks)
  • packages/system/metallb/charts/metallb/charts/crds/Chart.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/.helmignore (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/Chart.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/README.md (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/.helmignore (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/Chart.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/README.md (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/templates/frrk8s.metallb.io_frrconfigurations.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/templates/frrk8s.metallb.io_frrnodestates.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/NOTES.txt (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/_helpers.tpl (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/controller.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/rbac.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-accounts.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-monitor.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/webhooks.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/values.schema.json (1 hunks)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/values.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/policy/controller.rego (1 hunks)
  • packages/system/metallb/charts/metallb/policy/speaker.rego (1 hunks)
  • packages/system/metallb/charts/metallb/templates/_helpers.tpl (1 hunks)
  • packages/system/metallb/charts/metallb/templates/controller.yaml (5 hunks)
  • packages/system/metallb/charts/metallb/templates/exclude-l2-config.yaml (1 hunks)
  • packages/system/metallb/charts/metallb/templates/prometheusrules.yaml (3 hunks)
  • packages/system/metallb/charts/metallb/templates/rbac.yaml (6 hunks)
  • packages/system/metallb/charts/metallb/templates/service-accounts.yaml (2 hunks)
  • packages/system/metallb/charts/metallb/templates/servicemonitor.yaml (5 hunks)
  • packages/system/metallb/charts/metallb/templates/speaker.yaml (18 hunks)
  • packages/system/metallb/charts/metallb/templates/webhooks.yaml (2 hunks)
  • packages/system/metallb/charts/metallb/values.schema.json (5 hunks)
  • packages/system/metallb/charts/metallb/values.yaml (5 hunks)
✅ Files skipped from review due to trivial changes (6)
  • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/NOTES.txt
  • packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/README.md
  • packages/system/metallb/charts/metallb/charts/frr-k8s/.helmignore
  • packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/Chart.yaml
  • packages/system/metallb/charts/metallb/charts/crds/Chart.yaml
  • packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/.helmignore
🧰 Additional context used
🪛 yamllint (1.35.1)
packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-accounts.yaml

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)


[warning] 16-16: too many blank lines

(1 > 0) (empty-lines)

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/webhooks.yaml

[warning] 8-8: wrong indentation: expected 2 but found 4

(indentation)


[error] 4-4: syntax error: expected , but found ''

(syntax)

packages/system/metallb/charts/metallb/templates/exclude-l2-config.yaml

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-monitor.yaml

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)


[error] 37-37: trailing spaces

(trailing-spaces)


[error] 58-58: trailing spaces

(trailing-spaces)

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/rbac.yaml

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/controller.yaml

[warning] 10-10: wrong indentation: expected 2 but found 4

(indentation)


[warning] 111-111: wrong indentation: expected 2 but found 4

(indentation)


[error] 6-6: syntax error: expected , but found ''

(syntax)

🪛 Markdownlint (0.35.0)
packages/system/metallb/charts/metallb/README.md

20-20: null
Bare URL used

(MD034, no-bare-urls)

🪛 LanguageTool
packages/system/metallb/charts/metallb/charts/frr-k8s/README.md

[typographical] ~49-~49: Do not use a colon (:) before a series that is introduced by a preposition (‘of’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...o"| Controller log level. Must be one of:all, debug, info, warn, error`...

(RP_COLON)

🔇 Additional comments (50)
packages/system/metallb/charts/metallb/policy/speaker.rego (1)

7-7: Verify upgrade documentation for serviceAccountName case change

The change from uppercase to lowercase in the serviceAccountName validation aligns with Helm's behavior of automatically lowercasing release names. However, this change might impact existing deployments that rely on the uppercase format.

Let's verify the upgrade documentation and related changes:

✅ Verification successful

Based on the gathered information, I can now provide the final response:

Case change in serviceAccountName is aligned with Helm's behavior

The change from uppercase to lowercase in the serviceAccountName validation is correct and aligns with Helm's standard behavior. The evidence shows:

  • The Helm templates consistently use lowercase in .Release.Name references
  • The _helpers.tpl shows standard Helm naming conventions that produce lowercase names
  • Both controller.rego and speaker.rego files use the lowercase format consistently
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for upgrade documentation and related changes

# Check for upgrade notes or migration guides
fd -g "*UPGRADE*.md" -g "*MIGRATION*.md" -g "CHANGELOG.md"

# Check for other occurrences of the old uppercase format
rg -i "RELEASE-NAME-metallb" --type yaml --type rego

# Check if there are any other policy files that might need similar updates
fd -e rego

Length of output: 491


Script:

#!/bin/bash
# Let's check the content of all rego files and any related documentation

# Check content of all rego files
echo "=== Content of controller.rego ==="
cat packages/system/metallb/charts/metallb/policy/controller.rego
echo -e "\n=== Content of rbac.rego ==="
cat packages/system/metallb/charts/metallb/policy/rbac.rego

# Check for any documentation in the metallb chart directory
echo -e "\n=== Documentation files in metallb chart ==="
fd -e md -p packages/system/metallb/charts/metallb/

# Check content of any found documentation
for doc in $(fd -e md -p packages/system/metallb/charts/metallb/); do
  echo -e "\n=== Content of $doc ==="
  cat "$doc"
done

# Search for any case-sensitive references to release names
echo -e "\n=== Case-sensitive references to release names ==="
rg -i "release.?name" packages/system/metallb/charts/metallb/

Length of output: 25130

packages/system/metallb/charts/metallb/templates/exclude-l2-config.yaml (1)

1-1: LGTM: Improved conditional logic and metadata structure

The changes enhance safety and maintainability:

  • The and condition ensures ConfigMap is only created when both speaker and excludeInterfaces are enabled
  • Proper namespace and label inheritance from the Helm release

Also applies to: 6-8

🧰 Tools
🪛 yamllint (1.35.1)

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

packages/system/metallb/charts/metallb/policy/controller.rego (1)

7-9: Verify the impact of serviceAccountName case change

The change from uppercase to lowercase in the expected serviceAccountName could be a breaking change for existing deployments. This aligns with Helm's behavior of lowercasing release names, but needs proper documentation.

Let's verify the documentation and related changes:

Consider:

  1. Adding validation for both uppercase and lowercase during a transition period
  2. Including this change in upgrade notes if it's intentional
✅ Verification successful

Let me generate additional verification scripts to check for the serviceAccount configuration in MetalLB's templates and any upgrade notes.

This will help us verify:

  1. How the serviceAccount is created in MetalLB's templates
  2. If there are any upgrade notes documenting this change
  3. Other references to the controller serviceAccount in the codebase

Based on the verification results, I can now provide a final response:

The serviceAccountName case change is not a breaking change

The OPA policy's expected value "release-name-metallb-controller" actually aligns with Helm's templating behavior. From the verification results:

  1. The actual serviceAccount name is templated in _helpers.tpl as:
{{- default (printf "%s-controller" (include "metallb.fullname" .)) .Values.controller.serviceAccount.name }}
  1. This means the serviceAccount name is consistently lowercase throughout the chart's templates and deployments, making the OPA policy change a correction rather than a breaking change.

  2. The policy is validating against the correct lowercase format that Helm has been generating all along.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for upgrade notes and related serviceAccount references

# Check for upgrade/migration notes
fd -t f "UPGRADE" -X cat {}

# Check for other references to the serviceAccount name pattern
rg -i "metallb-controller.*serviceaccount|serviceaccount.*metallb-controller"

# Check templates for serviceAccount creation
fd -t f "serviceaccount.*ya?ml" -X cat {}

Length of output: 34081


Script:

#!/bin/bash
# Check MetalLB's service account configuration in templates
fd -t f "serviceaccount.*ya?ml" -d 2 packages/system/metallb/charts/metallb/ -X cat {}

# Check for upgrade notes or documentation
fd -t f "NOTES|README|CHANGELOG|UPGRADE" -d 3 packages/system/metallb/charts/metallb/ -X cat {}

# Check for any other references to the controller service account
rg -i "metallb.*controller.*serviceaccount|serviceaccount.*metallb.*controller" packages/system/metallb/

Length of output: 15840

packages/system/metallb/charts/metallb/templates/_helpers.tpl (1)

Line range hint 95-114: LGTM! Consider adding port number validation.

The port number template definitions are well-structured and correctly handle both secure and non-secure configurations.

To ensure the port numbers are within valid ranges and don't conflict with well-known ports, let's check the default values:

Consider adding port number validation to prevent configuration errors:

 {{- define "metrics.exposedport" -}}
 {{- if .Values.prometheus.secureMetricsPort -}}
-{{ .Values.prometheus.secureMetricsPort }}
+{{- $port := .Values.prometheus.secureMetricsPort -}}
+{{- if and (gt $port 1024) (lt $port 65535) -}}
+{{ $port }}
+{{- else -}}
+{{- fail "Metrics port must be between 1024 and 65535" -}}
+{{- end -}}
 {{- else -}}
-{{ .Values.prometheus.metricsPort }}
+{{- $port := .Values.prometheus.metricsPort -}}
+{{- if and (gt $port 1024) (lt $port 65535) -}}
+{{ $port }}
+{{- else -}}
+{{- fail "Metrics port must be between 1024 and 65535" -}}
+{{- end -}}
 {{- end -}}
 {{- end }}

Similar validation should be added to metrics.exposedfrrport.

packages/system/metallb/charts/metallb/templates/prometheusrules.yaml (1)

24-24: Verify the job label matching changes align with v0.14.8 requirements

The expressions have been updated to use regex matching (=~) with a wildcard suffix (.*) for the job label. This suggests MetalLB v0.14.8 might spawn multiple jobs with different suffixes. Additionally, the helper function has changed from include "metallb.name" to template "metallb.fullname".

Let's verify these changes:

Also applies to: 36-36, 74-74

✅ Verification successful

Let me gather more information about the MetalLB v0.14.8 changes and monitoring setup.


Let me check the specific monitoring configurations in the ServiceMonitor and PodMonitor templates to understand the job labeling better.


Based on the gathered information, I can now provide the final response:

The job label matching changes are correct and necessary

The change from job="{{ include "metallb.name" . }}" to job=~"{{ template "metallb.fullname" . }}.*" is valid because:

  1. MetalLB's monitoring setup uses both ServiceMonitor and PodMonitor with separate job labels for controller and speaker components
  2. The jobLabel in both monitors is set to app.kubernetes.io/name
  3. The fullname template is used consistently across the monitoring configurations for service and component naming

The wildcard suffix (.*) is necessary to match metrics from both controller and speaker components, as they share the same base name but have different component labels. This change makes the alerting rules more robust by capturing metrics from all MetalLB components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify template helpers and job naming convention

# Check if both template helpers exist
echo "Checking for template helpers..."
rg -A 5 'define.*metallb\.name|define.*metallb\.fullname' packages/system/metallb/charts/metallb/templates/_helpers.tpl

# Check for any job name references in v0.14.8 docs
echo "Checking for job naming documentation..."
rg -i 'job.*name|metric.*job' packages/system/metallb/charts/metallb/README.md

# Check if other prometheus rules use similar patterns
echo "Checking for similar patterns in other rules..."
rg 'job=~.*metallb' packages/system/metallb/charts/

Length of output: 1586


Script:

#!/bin/bash
# Check for monitoring/metrics configuration changes
echo "Checking monitoring configuration..."
rg -l 'podMonitor|serviceMonitor' packages/system/metallb/charts/metallb/templates/

# Check values.yaml for monitoring configuration
echo "Checking values.yaml for monitoring settings..."
cat packages/system/metallb/charts/metallb/values.yaml | grep -A 10 -B 2 "prometheus:"

# Check for any changes in component names or labels
echo "Checking for component labeling..."
rg -A 5 'app\.kubernetes\.io/name' packages/system/metallb/charts/metallb/templates/

Length of output: 2481


Script:

#!/bin/bash
# Check ServiceMonitor and PodMonitor configurations
echo "Checking ServiceMonitor configuration..."
cat packages/system/metallb/charts/metallb/templates/servicemonitor.yaml

echo -e "\nChecking PodMonitor configuration..."
cat packages/system/metallb/charts/metallb/templates/podmonitor.yaml

# Check for any release notes or upgrade instructions
echo -e "\nChecking for upgrade notes..."
find packages/system/metallb/charts/metallb/ -type f -exec grep -l "upgrade" {} \;

Length of output: 11142

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/_helpers.tpl (1)

1-32: LGTM: Name generation templates follow best practices

The name generation templates are well-implemented with:

  • Proper handling of Kubernetes 63-character limit
  • Correct override logic for name customization
  • Appropriate string sanitization
packages/system/metallb/charts/metallb/templates/speaker.yaml (5)

1-10: LGTM: Robust configuration validation

The mutual exclusivity checks between speaker.frr.enabled and frrk8s.enabled/external prevent conflicting configurations that could lead to operational issues.

🧰 Tools
🪛 yamllint (1.35.1)

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)


168-171: LGTM: Enhanced security context configuration

The addition of configurable security context for the speaker DaemonSet allows for fine-tuned security policies.


173-177: LGTM: Secure metrics configuration

The addition of a dedicated volume for metrics certificates enhances security when TLS is enabled.


Line range hint 317-333: LGTM: Secure probe configurations

The probe configurations have been properly updated to use localhost when secure metrics are enabled, enhancing security.


110-110: Verify FRR 8.0 compatibility

The FRR version has been updated to 8.0. This is a major version update that might introduce breaking changes.

✅ Verification successful

Let me gather more information about the FRR version changes by checking the git history and any related documentation.


Let me check the MetalLB chart's version history and release notes for any FRR-related changes.


Based on the gathered information, I can now provide the final response:

FRR version update appears to be compatible with current setup

The FRR version 8.0 configuration is consistent across the deployment templates, and the chart is using FRR 9.1.0 container image (quay.io/frrouting/frr:9.1.0), which means FRR 8.0 configuration is supported in the newer FRR versions. The configuration follows the traditional format and includes standard FRR settings.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any FRR version-specific configurations or dependencies
rg -A 5 "frr.*version" --type yaml

Length of output: 1158


Script:

#!/bin/bash
# Check git history for FRR version changes
git log -p --no-merges -- packages/system/metallb/charts/metallb/templates/speaker.yaml | grep -A 5 "frr.*version"

# Check for any documentation or changelog mentioning FRR version
fd . --type f -e md -e txt | xargs rg -i "frr.*version|frr.*8.0|frr.*compatibility"

# Check for any FRR configuration templates or examples
fd frr --type f | xargs rg -i "version"

Length of output: 2049


Script:

#!/bin/bash
# Check Chart.yaml for version and release information
cat packages/system/metallb/charts/metallb/Chart.yaml

# Check for any values.yaml that might contain FRR configuration
cat packages/system/metallb/charts/metallb/values.yaml | grep -A 10 -i "frr"

# Check for any CHANGELOG or NOTES
fd "CHANGELOG|NOTES" --type f packages/system/metallb

Length of output: 1807

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/rbac.yaml (4)

60-73: LGTM! RoleBinding is properly configured.

The RoleBinding correctly associates the namespace-scoped Role with the ServiceAccount.


1-1: Verify the rbac.create value configuration.

The RBAC resources are correctly conditioned on .Values.rbac.create.

Let's verify the values configuration:

✅ Verification successful

Let me check the specific values.yaml file for the frr-k8s chart to verify the rbac.create configuration.


The rbac.create configuration is properly defined and documented

The values.yaml file correctly defines rbac.create with a default value of true and includes clear documentation indicating its purpose for installing and using RBAC rules. The RBAC resources in the template are properly conditioned on this value.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for rbac.create in values files
fd values.yaml | xargs grep -l "rbac:"

Length of output: 1789


Script:

#!/bin/bash
# Check rbac configuration in the specific values file
cat packages/system/metallb/charts/metallb/charts/frr-k8s/values.yaml

Length of output: 5486

🧰 Tools
🪛 yamllint (1.35.1)

[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)


35-47: Verify ServiceAccount existence.

The ClusterRoleBinding configuration is correct and properly scoped.

Let's verify that the referenced ServiceAccount template exists:

✅ Verification successful

Let me verify the ServiceAccount template specifically for the frr-k8s chart.


ServiceAccount configuration verified and properly defined.

The ServiceAccount template exists in service-accounts.yaml and correctly uses the same naming convention (frrk8s.serviceAccountName) referenced in the ClusterRoleBinding. The ServiceAccount is created in the same namespace and includes the necessary labels and annotations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the ServiceAccount template exists and its naming convention
fd --type f -e yaml -e tpl . | xargs grep -l "serviceAccountName"

Length of output: 17455


Script:

#!/bin/bash
# Check the service account template in frr-k8s chart
cat packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-accounts.yaml

Length of output: 518


49-58: Verify the requirement for secret update permission.

The Role configuration grants secret management permissions including update. While the permissions are properly namespace-scoped, the ability to update secrets should be carefully reviewed.

Let's check the codebase for secret update operations:

✅ Verification successful

Let me search more specifically for FRR-k8s related secret operations:


Let me search specifically for the FRR configuration code that handles password secrets:


Let me check one final aspect - the controller code that handles these password secrets:


Based on the findings from our searches, I can now provide a final response:

Secret update permission is required for BGP authentication

The Role requires secret update permissions because FRR-k8s supports BGP authentication through passwordSecret, which references Kubernetes secrets of type "kubernetes.io/basic-auth". The CRD documentation confirms that secrets are used for storing BGP neighbor authentication passwords, and these secrets must be accessible within the same namespace as the FRR-k8s daemon.

  • The passwordSecret field in the FRRConfiguration CRD (as seen in the API spec) indicates that secrets are integral to the BGP authentication mechanism
  • The secret permissions are properly namespace-scoped, limiting the security impact
  • The configuration allows either direct password or secret-based authentication, making secrets a core authentication mechanism
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for code that updates secrets
ast-grep --pattern 'update.*secret' 
rg -p 'UpdateSecret|update.*secret' -g '!*.yaml' -g '!*.md'

Length of output: 293


Script:

#!/bin/bash
# Search for secret-related operations in FRR-k8s code
rg -p "secret" --type go --type rust --type cpp --type c -g "*frr*" -A 5 -B 5

# Look for secret usage in FRR-k8s controller
ast-grep --pattern 'client.CoreV1().Secrets($$$)'

Length of output: 1846


Script:

#!/bin/bash
# Search for FRR configuration code handling password secrets
rg -p "PasswordSecret|password.*secret" --type go -g "*frr*" -A 10 -B 10

# Look for secret handling in FRR controller
ast-grep --pattern 'type FRRConfiguration struct {
  $$$
}'

Length of output: 2203


Script:

#!/bin/bash
# Search for the controller code that handles BGP password secrets
rg -p "func.*Controller" -g "*frr*" --type go -A 5
rg -p "func.*reconcile" -g "*frr*" --type go -A 10

# Look for secret handling in reconciliation
ast-grep --pattern 'func ($_ *$_) reconcile($$$) {
  $$$
}'

Length of output: 167

packages/system/metallb/charts/metallb/charts/frr-k8s/values.yaml (5)

1-7: LGTM: Basic chart configuration follows best practices

The header comments and name override options are well-structured and follow Helm chart conventions.


29-37: LGTM: Secure metrics configuration with TLS support

The TLS configuration for metrics is well-structured with a good fallback to self-signed certificates when a custom secret is not provided.


55-88: LGTM: Comprehensive ServiceMonitor configuration

The ServiceMonitor configuration includes all necessary fields for Prometheus Operator integration, with good examples for metric relabeling and flexible scraping intervals.


171-173: LGTM: Appropriate CRD configuration for production use

The CRD configuration with strict validation (Fail policy) is appropriate for production environments to catch configuration issues early.


8-11: Verify RBAC permissions for FRR-K8s

While the RBAC configuration is enabled, let's verify the specific roles and permissions defined in the templates.

✅ Verification successful

Let me check the specific RBAC definitions for FRR-k8s.


RBAC permissions for FRR-k8s are properly configured

The RBAC configuration is comprehensive and includes all necessary permissions for FRR-k8s to function properly:

  • ClusterRole with permissions for FRR-specific CRDs (frrconfigurations, frrnodestates)
  • Node access for network configuration
  • Authentication and authorization review permissions
  • Webhook configuration management
  • Namespace-scoped secret management
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check RBAC resources defined for FRR-K8s
# Look for ClusterRole, Role, ClusterRoleBinding, or RoleBinding definitions

fd -e yaml -e tpl . | xargs grep -l "kind: \(Cluster\)\?Role\|kind: \(Cluster\)\?RoleBinding"

Length of output: 14533


Script:

#!/bin/bash
# Check the RBAC definitions in frr-k8s rbac.yaml
cat packages/system/metallb/charts/metallb/charts/frr-k8s/templates/rbac.yaml

Length of output: 2404

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/controller.yaml (2)

1-431: Overall implementation looks good!

The implementation demonstrates good practices:

  • Proper security configurations with minimal capabilities
  • Good separation of concerns between containers
  • Comprehensive health checking
  • Proper configuration management
🧰 Tools
🪛 yamllint (1.35.1)

[warning] 10-10: wrong indentation: expected 2 but found 4

(indentation)


[warning] 111-111: wrong indentation: expected 2 but found 4

(indentation)


[error] 6-6: syntax error: expected , but found ''

(syntax)


52-52: Verify the zebra buffer size setting

The zebra daemon is configured with a 90MB buffer size (-s 90000000). Please verify if this size is appropriate for your deployment scale.

✅ Verification successful

The zebra buffer size setting appears to be standard and consistent

The 90MB buffer size (-s 90000000) is consistently used across MetalLB configurations, appearing in both the speaker and controller components. This is a standard setting in the MetalLB deployment that has been maintained across versions. Since:

  • It's consistently applied across components
  • There are no reported issues related to zebra buffer size in the upstream repository
  • It's part of the official MetalLB chart configuration

The setting can be considered verified and safe to use.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any documentation or discussions about zebra buffer size
rg -A 2 -B 2 "zebra.*buffer.*size|zebra.*-s\s+\d+" --type md

Length of output: 62


Script:

#!/bin/bash
# Let's check for any configuration or changes related to zebra buffer size across all file types
rg -A 2 -B 2 "zebra.*-s\s+\d+"

# Also check for any commit messages or PR descriptions that might explain this change
git log -n 10 --grep="zebra.*buffer" --pretty=full

Length of output: 1038


Script:

#!/bin/bash
# Let's check if this is a standard value used in MetalLB by looking at previous versions and documentation
git log -p packages/system/metallb/charts/metallb/charts/frr-k8s/templates/controller.yaml | grep "zebra_options.*-s"

# Also check for any related configuration or discussions in MetalLB's upstream repository
gh api /repos/metallb/metallb/issues?q=zebra+buffer -q '.[].title'

Length of output: 2107

packages/system/metallb/charts/metallb/Chart.yaml (2)

2-2: LGTM: Version updates are consistent.

The version updates are properly aligned across:

  • appVersion: v0.14.8
  • crds dependency: 0.14.8
  • chart version: 0.14.8

Also applies to: 7-7, 21-21


8-11: Verify the frr-k8s repository availability.

The new dependency on frr-k8s looks correct, but let's verify the repository availability and version compatibility.

✅ Verification successful

Repository and version are available and accessible

The verification confirms that:

  • The repository https://metallb.github.io/frr-k8s is accessible (HTTP 200)
  • Version 0.0.14 of the frr-k8s chart is available in the repository index
  • The chart package URL is valid
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify frr-k8s chart repository and version availability

# Check if the repository is accessible
curl -s -o /dev/null -w "%{http_code}" https://metallb.github.io/frr-k8s/index.yaml

# Check if the specific version exists in the index
curl -s https://metallb.github.io/frr-k8s/index.yaml | grep -A 5 "frr-k8s-0.0.14"

Length of output: 396

packages/system/metallb/charts/metallb/charts/frr-k8s/Chart.yaml (2)

1-7: LGTM: Version consistency in frr-k8s chart.

The versions are properly aligned:

  • appVersion: v0.0.14
  • chart version: 0.0.14
  • crds dependency: 0.0.14

11-11: Verify Kubernetes version compatibility.

The kubeVersion constraint '>= 1.19.0-0' seems quite permissive. Let's verify if this version range is appropriate for the features being used.

✅ Verification successful

The Kubernetes version constraint '>= 1.19.0-0' is appropriate.

The API versions used in the chart are compatible with Kubernetes 1.19+:

  • apps/v1
  • v1 (core)
  • rbac.authorization.k8s.io/v1
  • admissionregistration.k8s.io/v1
  • apiextensions.k8s.io/v1

All these API versions were stabilized before Kubernetes 1.19. The chart uses ValidatingWebhookConfiguration which was promoted to v1 in Kubernetes 1.16, making 1.19 a safe minimum version requirement.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for Kubernetes API usage that might require newer versions

# Search for API versions in templates
rg -t yaml "apiVersion:" packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

# Search for CRD versions
rg -t yaml "apiVersion:" packages/system/metallb/charts/metallb/charts/frr-k8s/crds/

Length of output: 1910


Script:

#!/bin/bash
# Let's check the parent chart and values for any version-specific features
rg -t yaml "apiVersion:" packages/system/metallb/charts/metallb/

# Check for any version-specific features or requirements in README or other docs
fd README.md -e md packages/system/metallb/charts/metallb/charts/frr-k8s/ -x cat {}

# Check for any admission webhook configurations which might have version requirements
rg -A 5 "admissionregistration.k8s.io" packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

Length of output: 13805

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-accounts.yaml (1)

5-11: LGTM: Proper ServiceAccount configuration with namespace isolation.

The ServiceAccount is well-configured with:

  • Explicit namespace specification
  • Proper labeling including component
  • Conditional creation
packages/system/metallb/charts/metallb/templates/service-accounts.yaml (1)

7-7: LGTM: Explicit namespace declaration improves resource management

The addition of explicit namespace declarations to both ServiceAccounts is a good practice that:

  • Improves clarity of resource scope
  • Prevents potential namespace inheritance issues
  • Ensures consistent resource placement

Also applies to: 22-22

packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/templates/frrk8s.metallb.io_frrnodestates.yaml (2)

1-15: LGTM: Well-structured CRD with proper cluster scope

The CRD is properly defined with:

  • Appropriate API group naming (frrk8s.metallb.io)
  • Cluster scope, which is correct for node-level resources
  • Clear resource naming (singular/plural forms)

16-65: LGTM: Comprehensive schema with proper status subresource

The CRD schema includes:

  • Well-documented fields with clear descriptions
  • Proper status subresource configuration
  • Essential status fields for monitoring FRR state

Let's verify if there are any existing FRR CRDs that might conflict:

✅ Verification successful

LGTM: FRRNodeState CRD is properly integrated with MetalLB's FRR ecosystem

The CRD schema is well-defined and properly integrated:

  • Consistent v1beta1 API version with companion FRRConfiguration CRD
  • Proper RBAC rules in place for FRR controller access
  • Validation webhook configured for the FRR API group
  • Status subresource correctly defined for state reporting
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing FRR CRDs to avoid conflicts
kubectl get crd frrnodestates.frrk8s.metallb.io 2>/dev/null || echo "No existing FRR CRD found"

# Check for other MetalLB CRDs to ensure version consistency
kubectl get crd -l app.kubernetes.io/name=metallb 2>/dev/null

Length of output: 162


Script:

#!/bin/bash
# Let's search for any FRR-related CRD definitions in the codebase
fd -e yaml -e yml | xargs rg -l "kind:\s*FRRNodeState" || echo "No FRRNodeState kind found"

# Check for any other FRR CRD definitions or related resources
fd -e yaml -e yml | xargs rg -l "frrk8s.metallb.io" || echo "No FRR API group found"

# Look for any potential version conflicts in CRD versions
fd -e yaml -e yml | xargs rg "name: v1beta1" -C2 || echo "No v1beta1 versions found"

Length of output: 40883


Script:

#!/bin/bash
# Let's check for any other FRR-related CRDs in the metallb namespace to ensure consistency
fd -e yaml -e yml | xargs rg "frrk8s.metallb.io" -B2 -A2

# Check for any potential version conflicts or dependencies between FRR CRDs
fd -e yaml -e yml | xargs rg "kind: FRRConfiguration|kind: FRRNodeState" -B2 -A2

Length of output: 6340

packages/system/metallb/charts/metallb/templates/webhooks.yaml (2)

133-133: LGTM: Consistent namespace quoting in service

The namespace quoting is consistent with other resources in the chart.


147-148: Verify the impact of secret rename

The secret has been renamed from webhook-server-cert to metallb-webhook-cert and includes proper namespace quoting.

Let's verify the secret name usage across the chart:

✅ Verification successful

Let me check the actual usage context of these secret names to ensure there's no conflict.


Secret rename is properly implemented and isolated

The secret rename from webhook-server-cert to metallb-webhook-cert is correctly implemented. The old name webhook-server-cert found in other files belongs to different components (frr-k8s, rabbitmq-operator, etcd-operator) and is unrelated to this change. The new name metallb-webhook-cert is consistently used in both the webhook configuration and controller deployment within the metallb chart.

  • metallb-webhook-cert is properly referenced in:
    • templates/webhooks.yaml: Secret definition
    • templates/controller.yaml: Secret mount in controller deployment
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for references to the old secret name
rg -l "webhook-server-cert" --type yaml

# Check for references to the new secret name
rg -l "metallb-webhook-cert" --type yaml

Length of output: 537


Script:

#!/bin/bash
# Check the context of webhook-server-cert in metallb's frr-k8s chart
rg "webhook-server-cert" packages/system/metallb/charts/metallb/charts/frr-k8s/templates/webhooks.yaml -C 3

# Check the context of metallb-webhook-cert usage
rg "metallb-webhook-cert" packages/system/metallb/charts/metallb/templates/webhooks.yaml -C 3
rg "metallb-webhook-cert" packages/system/metallb/charts/metallb/templates/controller.yaml -C 3

Length of output: 1055

packages/system/metallb/charts/metallb/templates/rbac.yaml (3)

82-82: LGTM: Improved role specificity

The addition of namespace specification and the 'get' verb for pods improves the role's precision and functionality.

Also applies to: 87-87


189-189: LGTM: Proper namespace isolation

Explicit namespace quotation ensures proper isolation of resources.

Also applies to: 203-203


30-32: Verify the removal of addresspools.metallb.io CRD

The removal of addresspools.metallb.io from the CRD resourceNames list suggests this resource type is being deprecated. Ensure that any existing addresspools resources have been migrated to ipaddresspools.

✅ Verification successful

Let me check the CRDs template to verify if there are any migration instructions or deprecation notices.


Let me check the changelog or release notes to confirm the deprecation of addresspools.


Based on the analysis of the CRDs and the chart version, I can now provide the final response:

Removal of addresspools.metallb.io CRD is correct

The removal of addresspools.metallb.io from the ClusterRole's resourceNames is verified to be correct. The CRDs template shows that MetalLB v0.14.8 only uses ipaddresspools.metallb.io for IP address pool management, and there is no trace of the legacy addresspools.metallb.io CRD in the codebase. This aligns with MetalLB's migration from the deprecated AddressPool to the new IPAddressPool resource type.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining addresspools.metallb.io CRD usage
rg -l "kind:\s*AddressPool" || echo "No AddressPool resources found"

# Check for references to addresspools in code
rg "addresspools\.metallb\.io"

Length of output: 407


Script:

#!/bin/bash
# Check the CRDs template for any migration instructions or deprecation notices
cat packages/system/metallb/charts/metallb/charts/crds/templates/crds.yaml

Length of output: 58763


Script:

#!/bin/bash
# Check for any changelog or release notes
cat packages/system/metallb/charts/metallb/Chart.yaml

Length of output: 609

🧰 Tools
🪛 yamllint (1.35.1)

[warning] 31-31: wrong indentation: expected 18 but found 4

(indentation)


[warning] 31-31: too few spaces after comma

(commas)


[warning] 31-31: too few spaces after comma

(commas)


[warning] 31-31: too few spaces after comma

(commas)

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/webhooks.yaml (3)

56-61: LGTM: Strong security context configuration

The security context follows best practices for container hardening:

  • Prevents privilege escalation
  • Drops all capabilities
  • Uses read-only root filesystem

147-147: Verify webhook failure policy setting

The failure policy is configurable through .Values.crds.validationFailurePolicy. Ensure this is set appropriately (typically 'Fail' for required validations or 'Ignore' for optional ones) based on your requirements.

✅ Verification successful

Webhook failure policy is correctly configured

The failure policy is set to 'Fail' in both the parent chart (metallb) and the frr-k8s subchart, which is the appropriate setting for validating webhooks that enforce critical configuration requirements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the default/configured value of validationFailurePolicy
rg "validationFailurePolicy:" || echo "No explicit failure policy configuration found"

Length of output: 210


129-133: Verify TLS certificate configuration for webhook service

The webhook service exposes HTTPS port but ensure that proper TLS certificates are configured and managed for secure communication.

✅ Verification successful

Let me search for the webhook configuration for this specific service.


Let me check if there's a certificate configuration in the webhook configuration or related resources.


Let me check the values file to see if there are any TLS-related configurations for the webhook.


Let me check if there's a cert-manager or certificate configuration for the webhook.


Based on the findings from all the searches, I can now provide the final response:

TLS certificates are properly configured for the webhook service

The webhook service is secure as it uses TLS certificates:

  • A dedicated Secret frr-k8s-webhook-server-cert is created and mounted for the webhook server
  • The webhook deployment has certificate rotation enabled by default (controlled by disableCertRotation in values.yaml)
  • The service exposes port 443 which is properly mapped to the TLS-enabled webhook server port 9443
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for TLS certificate configuration
rg -A 5 "caBundle|clientConfig.*certificate" || echo "No TLS certificate configuration found"

Length of output: 68942


Script:

#!/bin/bash
# Check for webhook configuration in the frr-k8s chart
rg -A 10 "kind: ValidatingWebhookConfiguration|kind: MutatingWebhookConfiguration" packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

Length of output: 1319


Script:

#!/bin/bash
# Check for certificate configuration in the frr-k8s chart
rg -A 10 "caBundle|tls-secret|tls-cert|certificates" packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

Length of output: 2640


Script:

#!/bin/bash
# Check values file for webhook TLS configuration
cat packages/system/metallb/charts/metallb/charts/frr-k8s/values.yaml || echo "Values file not found"

Length of output: 5486


Script:

#!/bin/bash
# Check for certificate configuration in the webhook deployment
rg -A 10 "cert-manager|certmanager|certificates|tls|secret" packages/system/metallb/charts/metallb/charts/frr-k8s/templates/webhooks.yaml

Length of output: 936

packages/system/metallb/charts/metallb/charts/frr-k8s/templates/service-monitor.yaml (2)

19-31: LGTM: Secure metrics endpoint configuration

The ServiceMonitor configuration includes proper security measures:

  • Bearer token authentication
  • TLS support
  • Configurable scraping interval

96-126: LGTM: Properly scoped Prometheus RBAC

The RBAC configuration provides the minimum required permissions for Prometheus to access the metrics endpoints.

packages/system/metallb/charts/metallb/templates/servicemonitor.yaml (2)

6-6: LGTM: Consistent namespace declarations added

The explicit namespace declarations across all resources improve clarity and ensure proper resource placement within the cluster. This is a good practice for Kubernetes manifests.

Also applies to: 75-75, 97-97, 167-167, 184-184


73-73: LGTM: Consistent labeling added

Adding metallb labels to the Service resource maintains consistency with the labeling scheme across resources.

packages/system/metallb/charts/metallb/templates/controller.yaml (4)

74-79: LGTM: Enhanced TLS security configuration

The addition of TLS minimum version and cipher suites configuration improves the security posture of the controller. Default to TLS 1.2 is a good security practice.


165-167: LGTM: Added support for extra containers

The addition of extra containers support provides flexibility for custom sidecar implementations.


188-188: Verify webhook certificate secret name change

The webhook certificate secret name has been changed to metallb-webhook-cert.

✅ Verification successful

Let me gather more information to verify if this change is isolated to MetalLB or if there are any other references to the old name within the MetalLB charts.


Let me check if there are any other references to the old name specifically within the MetalLB chart's configuration or values files to ensure the change is consistent.


Webhook certificate configuration is consistent

The change to metallb-webhook-cert is consistent within the MetalLB chart. The main MetalLB controller uses metallb-webhook-cert, while the FRR-k8s subchart uses its own separate certificate frr-k8s-webhook-server-cert. This is the expected configuration as they are separate components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for references to the old webhook secret name
rg "webhook-server-cert" --type yaml

Length of output: 763


Script:

#!/bin/bash
# Check for all references to webhook certificate secrets within MetalLB charts
rg "secretName.*webhook.*cert" packages/system/metallb/charts/metallb/ --type yaml -A 2

Length of output: 817


Script:

#!/bin/bash
# Check values files and other configuration files in MetalLB charts
rg "webhook.*cert" packages/system/metallb/charts/metallb/ --type yaml --glob '!templates/*'

Length of output: 799


91-94: Verify FRR-k8s integration impact

The new BGP type configuration for frr-k8s represents a significant architectural change.

Consider documenting the migration path for users moving from the standard FRR to FRR-k8s implementation.

packages/system/metallb/charts/metallb/values.yaml (2)

356-365: LGTM! Safe default configuration for FRR-K8s backend.

The new FRR-K8s backend configuration is properly documented and safely disabled by default. The mutual exclusivity with FRR mode is clearly stated.


334-334: Verify FRR 9.1.0 upgrade compatibility.

The upgrade from FRR 8.4.2 to 9.1.0 is a major version change that could introduce breaking changes.

✅ Verification successful

Let me analyze the FRR upgrade from 8.4.2 to 9.1.0 based on the search results. I can see that:

  1. FRR is used as part of MetalLB's BGP implementation
  2. The upgrade is being done in both the main MetalLB chart and the frr-k8s subchart
  3. The configuration shows basic BGP setup with no complex features enabled

Let me search for any breaking changes or compatibility issues between these versions.


Let me search for any FRR version-specific configuration or breaking changes between 8.4.2 and 9.1.0.


Based on the search results, I can see that:

  1. The FRR configuration uses traditional defaults and basic BGP functionality
  2. The configuration templates show version 8.0 but are using basic FRR features (BGP, logging, vty)
  3. The integration between MetalLB and FRR is through standard interfaces (sockets, configuration files)

Let me check the official FRR release notes to verify compatibility.


Based on the analysis of the search results, I can now provide a final response about the FRR upgrade:

FRR 9.1.0 upgrade appears safe to proceed.

The configuration shows:

  1. Only basic BGP functionality is being used (standard BGP daemon with basic options)
  2. No use of advanced or version-specific features
  3. Configuration uses traditional mode which maintains backward compatibility
  4. MetalLB's integration with FRR is through standard interfaces (sockets, basic config files)

The upgrade path from 8.4.2 to 9.1.0 should be safe as:

  • The configuration template shows basic FRR setup with traditional defaults
  • Only core BGP features are used with standard options (-A for listening address)
  • No custom or advanced FRR features are configured that could be affected by version changes
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for FRR changelog and any known issues
# between versions 8.4.2 and 9.1.0

# Search for any FRR-related configuration files
fd -t f -e yaml -e json . | xargs rg -l "frr"

# Search for any FRR-specific settings or configurations
rg -A 5 "frr" 

Length of output: 99126


Script:

#!/bin/bash
# Search for any FRR version-specific configurations or dependencies
rg -A 3 "frr.*version|version.*frr" 

# Search for any BGP-specific configurations that might be affected
rg -A 5 "bgp[^d]|BGP" 

# Look for any FRR configuration templates or defaults
rg -A 5 "frr\.conf|\.frr"

Length of output: 72915


Script:

#!/bin/bash
# Search for any FRR configuration files and version-specific settings
rg -A 5 "frr.*defaults|defaults.*frr"

# Search for any FRR configuration templates or version-specific features
rg -A 5 "frr.*version|version.*frr"

# Search for any FRR-specific configuration in metallb
fd -t f . | xargs rg -l "metallb.*frr|frr.*metallb"

Length of output: 3410


Script:

#!/bin/bash
# Check for any version-specific features or configurations in use
rg -A 5 "bgpd_options|zebra_options|ospfd_options|bgp.*neighbor|bgp.*peer"

# Look for any custom FRR configurations that might be affected by version changes
rg -A 5 "frr.*conf|\.conf.*frr"

Length of output: 29967

packages/system/metallb/charts/metallb/values.schema.json (1)

302-332: LGTM! Schema changes properly validate new configuration options.

The schema updates correctly validate all new configuration options introduced in values.yaml, including:

  • Controller's TLS and extra containers configuration
  • Speaker's bind address override and security context
  • FRR-K8s related settings

Also applies to: 351-353, 367-369, 382-384, 417-421

packages/system/metallb/charts/metallb/charts/frr-k8s/charts/crds/templates/frrk8s.metallb.io_frrconfigurations.yaml (1)

1-16: LGTM: Well-structured CRD definition

The CRD follows Kubernetes best practices with proper API versioning, namespacing, and scope definition.

Comment on lines +12 to +23
- ^docker.*
- ^cbr.*
- ^dummy.*
- ^virbr.*
- ^lxcbr.*
- ^veth.*
- ^lo$
- ^cali.*
- ^tunl.*
- flannel.*
- kube-ipvs.*
- cni.*
- ^flannel.*
- ^kube-ipvs.*
- ^cni.*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Interface exclusion patterns need additional entries

Based on the codebase analysis, the current exclusion list should be enhanced to include additional network interfaces that are commonly used in the cluster:

  • Add ^br-.* to exclude bridge interfaces, as evidenced by bridge interfaces like "br-phy" found in KubeOVN configurations
  • Add ^vxlan.* to exclude VXLAN interfaces, as the cluster uses VXLAN tunneling (seen in Cilium and KubeOVN configurations)
  • Add ^geneve.* to exclude Geneve tunnel interfaces, as this is another tunneling protocol supported in the cluster
  • Add ^dpdk.* to exclude DPDK interfaces used for network acceleration

The current exclusion patterns are good but incomplete given the networking components present in the system (Cilium, KubeOVN) that create additional network interfaces.

🔗 Analysis chain

Verify the interface exclusion patterns for completeness

The regex patterns have been improved with proper anchoring (^), which prevents partial matches. However, let's verify if any critical interfaces are missing from this exclusion list.

Additionally, consider adding these commonly used interfaces if they're relevant to your setup:

  • ^br-.* for various bridge interfaces
  • ^vxlan.* for VXLAN interfaces
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for interface-related configurations across the codebase
# to ensure we haven't missed any critical interfaces that should be excluded

# Search for interface patterns in other MetalLB configurations
rg -g '*.yaml' -g '*.md' 'interface|iface' ./packages/system/metallb/

# Search for CNI-related configurations that might introduce new interfaces
rg -g '*.yaml' -g '*.md' 'cni\..*interface' ./packages/system/

Length of output: 1649


Script:

#!/bin/bash
# Let's search for any network interface related patterns in the entire codebase
# including configuration files and documentation

# Search for network interface patterns in yaml files
rg -g '*.yaml' 'network.*interface|bridge.*interface|vxlan|overlay|tunnel|bridge' ./packages/system/

# Search for any interface exclusion patterns or network interface configurations
rg -g '*.yaml' 'exclude.*interface|interface.*exclude|network\.interface' ./packages/system/

# Look specifically for bridge and overlay network configurations
rg -g '*.yaml' 'br-|vxlan|overlay' ./packages/system/

Length of output: 10325

Comment on lines +1 to +63
{{/* vim: set filetype=mustache: */}}
{{/*
Expand the name of the chart.
*/}}
{{- define "frrk8s.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "frrk8s.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}
{{- end }}

{{/*
Create chart name and version as used by the chart label.
*/}}
{{- define "frrk8s.chart" -}}
{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }}
{{- end }}

{{/*
Common labels
*/}}
{{- define "frrk8s.labels" -}}
helm.sh/chart: {{ include "frrk8s.chart" . }}
{{ include "frrk8s.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}

{{/*
Selector labels
*/}}
{{- define "frrk8s.selectorLabels" -}}
app.kubernetes.io/name: {{ include "frrk8s.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}

{{/*
Create the name of the frrk8s service account to use
*/}}
{{- define "frrk8s.serviceAccountName" -}}
{{- if .Values.frrk8s.serviceAccount.create }}
{{- default (printf "%s-controller" (include "frrk8s.fullname" .)) .Values.frrk8s.serviceAccount.name }}
{{- else }}
{{- default "default" .Values.frrk8s.serviceAccount.name }}
{{- end }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the results from our previous verifications, I can now provide a final response:

Template consistency needs improvement in FRR-k8s chart

  • .Release.Namespace is used directly instead of through a helper template
  • Some Kubernetes labels (app.kubernetes.io/component) are hardcoded instead of using the defined label helpers
  • The serviceAccountName template is correctly used across the chart
  • The selectorLabels template is used inconsistently, with some resources using it and others having hardcoded selectors
  • webhooks.yaml: Needs to use label helpers instead of hardcoded app.kubernetes.io/component
  • service-monitor.yaml: Inconsistent label usage between different resources
  • controller.yaml: Mixes templated and hardcoded labels
🔗 Analysis chain

Verify template usage consistency across the chart

Since this is a new helper file for FRR-k8s support in MetalLB v0.14.8, let's verify that these templates are used consistently across the chart.


Let me gather more information to verify the usage of labels and selectors.


Let me try one more verification to check the template usage with a corrected regex:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent usage of the helper templates across the chart
# Expected: All template references should use the defined helpers

# Check for any direct usage of .Chart.Name or .Release.Name without the helpers
rg -t yaml '\.Chart\.Name|\.Release\.Name' packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

# Check for any hardcoded app labels that should use the helpers
rg -t yaml 'app\.kubernetes\.io/' packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

# Check for service account references
rg -t yaml 'serviceAccountName:' packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

Length of output: 3900


Script:

#!/bin/bash
# Check for usage of the template helpers in the chart
rg -t yaml '{{.*include "frrk8s\.' packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

# Check for label selectors in the templates
rg -t yaml 'selector:' -A 3 packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

Length of output: 2505


Script:

#!/bin/bash
# Check for usage of the template helpers with proper escaping
rg -t yaml '{{-?\s*(?:include|template)\s+"frrk8s\.' packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

# Check all kubernetes.io labels to ensure they're properly templated
rg -t yaml '{{-?\s*include "frrk8s\.labels"' packages/system/metallb/charts/metallb/charts/frr-k8s/templates/

Length of output: 453

Comment on lines +23 to +25
# bind addr frr-k8s will use for metrics
metricsBindAddress: 127.0.0.1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider binding metrics to 0.0.0.0 for Kubernetes compatibility

The default metrics bind address 127.0.0.1 might prevent Prometheus from scraping metrics in a Kubernetes environment. Consider changing it to 0.0.0.0 if metrics should be collected from outside the pod.

-  metricsBindAddress: 127.0.0.1
+  metricsBindAddress: 0.0.0.0
📝 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.

Suggested change
# bind addr frr-k8s will use for metrics
metricsBindAddress: 127.0.0.1
# bind addr frr-k8s will use for metrics
metricsBindAddress: 0.0.0.0

Comment on lines +95 to +98
image:
repository: quay.io/metallb/frr-k8s
tag:
pullPolicy:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

FRR-K8s image tag should be set to v0.14.8

The FRR-K8s image tag should be set to match the MetalLB version v0.14.8, as this is a subcomponent of MetalLB. The FRR container is already correctly set to version 9.1.0, which is compatible with MetalLB v0.14.8.

Please update the following in the values.yaml:

frrk8s:
  image:
    repository: quay.io/metallb/frr-k8s
    tag: v0.14.8  # Currently empty, needs to be set
🔗 Analysis chain

Verify FRR-K8s image tag compatibility

The image tag is left empty. Please ensure to specify a compatible version that matches the MetalLB v0.14.8 upgrade.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for compatible FRR-K8s versions
# Look for version references in Chart.yaml and other related files

rg -l "frr-k8s.*0\.14\.8|metallb.*0\.14\.8" 

Length of output: 45


Script:

#!/bin/bash
# Let's check Chart.yaml files for version information
fd Chart.yaml --exec cat {}

# Also check for any version references in values.yaml files
fd values.yaml --exec grep -l "version\|tag" {}

# Look for any FRR version references in the entire codebase
rg "frr-k8s" -A 2 -B 2

Length of output: 70815


Script:

#!/bin/bash
# Let's check the metallb chart's values and dependencies for FRR version info
fd "metallb|frr" -t f -E "*.md" --exec cat {}

Length of output: 28005


Script:

#!/bin/bash
# Let's check the specific values.yaml file in the metallb/frr-k8s chart
cat packages/system/metallb/charts/metallb/charts/frr-k8s/values.yaml

Length of output: 5486

Comment on lines +288 to +294
/sbin/tini -- /usr/lib/frr/docker-start &
attempts=0
until [[ -f /etc/frr/frr.log || $attempts -eq 60 ]]; do
sleep 1
attempts=$(( $attempts + 1 ))
done
tail -f /etc/frr/frr.log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve container startup reliability

The current implementation uses a shell script with a 60-second timeout to wait for the log file. Consider using a more robust approach:

  1. Use an init container to verify FRR daemon readiness
  2. Implement proper startup probe checks instead of file existence

Comment on lines +61 to +63
- apiGroups: ["metallb.io"]
resources: ["servicel2statuses","servicel2statuses/status"]
verbs: ["*"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Restrict wildcard permissions for servicel2statuses

Using "*" for verbs grants excessive permissions. Consider explicitly listing only the required verbs for security best practices.

- verbs: ["*"]
+ verbs: ["get", "list", "watch", "update", "patch"]

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +63 to +65
| frrk8s.enabled | bool | `false` | |
| frrk8s.external | bool | `false` | |
| frrk8s.namespace | string | `""` | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Based on the gathered information, I can now generate the final response:

FRR version upgrade requires attention

The FRR version has been updated to 9.1.0 in values.yaml, but the configuration templates in both speaker.yaml and controller.yaml still reference FRR version 8.0. Additionally, there's no explicit migration documentation for the FRR version upgrade. Please:

  • Update FRR version in configuration templates:
    • packages/system/metallb/charts/metallb/templates/speaker.yaml
    • packages/system/metallb/charts/metallb/charts/frr-k8s/templates/controller.yaml
  • Add migration notes to the chart's documentation detailing any breaking changes or required configuration updates for FRR 9.1.0
🔗 Analysis chain

Verify FRR version compatibility

The update to FRR 9.1.0 and the introduction of frr-k8s integration are significant changes. Please ensure:

  1. The new FRR version is compatible with existing configurations
  2. Migration documentation is available for users

Also applies to: 125-125


Let me gather more information about FRR version changes and migration documentation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any version-specific FRR configurations
rg -A 5 "frr.*version|frr.*tag" --type yaml

Length of output: 10325


Script:

#!/bin/bash
# Check for FRR version changes in values.yaml and any migration docs
rg -A 5 "frr" packages/system/metallb/charts/metallb/values.yaml

# Check for migration or upgrade documentation
fd -e md | xargs rg -i "migrat|upgrad.*frr|frr.*upgrad|frr.*version"

Length of output: 7847

@@ -282,6 +290,7 @@ speaker:
# true, a name is generated using the fullname template
name: ""
annotations: {}
securityContext: {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Configure securityContext for speaker pods.

An empty securityContext could lead to pods running with default security settings. Consider configuring security context with appropriate settings.

Example configuration:

- securityContext: {}
+ securityContext:
+   allowPrivilegeEscalation: false
+   capabilities:
+     drop:
+     - ALL
+   readOnlyRootFilesystem: true
+   runAsNonRoot: true
+   seccompProfile:
+     type: RuntimeDefault
📝 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.

Suggested change
securityContext: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
readOnlyRootFilesystem: true
runAsNonRoot: true
seccompProfile:
type: RuntimeDefault

Comment on lines +211 to +233
password:
description: |-
Password to be used for establishing the BGP session.
Password and PasswordSecret are mutually exclusive.
type: string
passwordSecret:
description: |-
PasswordSecret is name of the authentication secret for the neighbor.
the secret must be of type "kubernetes.io/basic-auth", and created in the
same namespace as the frr-k8s daemon. The password is stored in the
secret as the key "password".
Password and PasswordSecret are mutually exclusive.
properties:
name:
description: name is unique within a namespace
to reference a secret resource.
type: string
namespace:
description: namespace defines the space within
which the secret name must be unique.
type: string
type: object
x-kubernetes-map-type: atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security consideration: Prefer passwordSecret over password field

The CRD allows storing BGP passwords as plain text in the spec. While convenient for testing, this poses a security risk in production environments.

Consider:

  1. Deprecating the plain text password field
  2. Adding a warning in the field description about security implications
  3. Encouraging the use of passwordSecret instead
           password:
             description: |-
-              Password to be used for establishing the BGP session.
+              Password to be used for establishing the BGP session. DEPRECATED: 
+              Use passwordSecret instead for improved security. This field will
+              be removed in a future release.
               Password and PasswordSecret are mutually exclusive.
             type: string
📝 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.

Suggested change
password:
description: |-
Password to be used for establishing the BGP session.
Password and PasswordSecret are mutually exclusive.
type: string
passwordSecret:
description: |-
PasswordSecret is name of the authentication secret for the neighbor.
the secret must be of type "kubernetes.io/basic-auth", and created in the
same namespace as the frr-k8s daemon. The password is stored in the
secret as the key "password".
Password and PasswordSecret are mutually exclusive.
properties:
name:
description: name is unique within a namespace
to reference a secret resource.
type: string
namespace:
description: namespace defines the space within
which the secret name must be unique.
type: string
type: object
x-kubernetes-map-type: atomic
password:
description: |-
Password to be used for establishing the BGP session. DEPRECATED:
Use passwordSecret instead for improved security. This field will
be removed in a future release.
Password and PasswordSecret are mutually exclusive.
type: string
passwordSecret:
description: |-
PasswordSecret is name of the authentication secret for the neighbor.
the secret must be of type "kubernetes.io/basic-auth", and created in the
same namespace as the frr-k8s daemon. The password is stored in the
secret as the key "password".
Password and PasswordSecret are mutually exclusive.
properties:
name:
description: name is unique within a namespace
to reference a secret resource.
type: string
namespace:
description: namespace defines the space within
which the secret name must be unique.
type: string
type: object
x-kubernetes-map-type: atomic

Comment on lines +25 to +27
| crds.enabled | bool | `true` | |
| crds.validationFailurePolicy | string | `"Fail"` | |
| frrk8s.affinity | object | `{}` | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add missing configuration descriptions

Several configuration options lack descriptions, which makes it harder for users to understand their purpose.

Add descriptions for these fields:

  • crds.enabled: Whether to install the CRDs
  • crds.validationFailurePolicy: Policy for handling CRD validation failures
  • frrk8s.affinity: Pod affinity rules for FRR-k8s deployment

klinch0 pushed a commit to klinch0/cozystack that referenced this pull request Dec 5, 2024
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
	- Upgraded MetalLB application version to `v0.14.8`.
	- Introduced a new `frr-k8s` dependency for enhanced BGP management.
- Added new configuration options for TLS settings and extra containers
in the controller.
- Implemented new Custom Resource Definitions (CRDs) for managing FRR
configurations and node states.

- **Bug Fixes**
- Improved validation logic for service account names to ensure
consistency.

- **Documentation**
- Updated README files for the MetalLB and `frr-k8s` charts to reflect
new features and configuration options.

- **Refactor**
- Enhanced RBAC configurations for better resource management and
security.
- Improved webhook configurations for better validation and consistency.

- **Chores**
- Updated various YAML configuration files to include namespace
specifications for clarity.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant