Conversation
cwrau
requested review from
marvinWolff,
tasches and
teutonet-bot
as code owners
WalkthroughThis pull request updates the Helm chart configuration to replace the ingress boolean Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Helm as Helm Template Engine
participant Validator as Ingress Validation
participant Resource as Resource Deployment
User->>Helm: Submit configuration (e.g., ingress.provider = traefik)
Helm->>Validator: Check for conflicting ingress releases
Validator-->>Helm: Return validation status (pass/fail)
alt Validation Passes
Helm->>Resource: Render & deploy resources for selected provider
else Validation Fails
Helm-->>User: Return error message about conflicting ingress configurations
end
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml (1)
34-37: 💡 Verification agent🧩 Analysis chain
Ingress Class Parameterization for HTTP01 Solver:
In the HTTP01 challenge section, the ingress configuration is currently hardcoded as:ingress: class: nginx serviceType: ClusterIPSince this PR introduces support for multiple ingress controllers (including traefik), it would be prudent to parameterize the ingress class. For example, you might consider replacing
"nginx"with a value derived from.Values.ingress.provider(ensuring you handle cases where the provider might be"none"). An illustrative diff could be:- class: nginx + class: {{ .Values.ingress.provider }}Make sure to adjust this carefully so that the template remains valid in all scenarios.
I'll run a quick shell script to inspect the relevant section of the file in question. This script will print lines 30–40 from
charts/base-cluster/templates/cert-manager/clusterissuer.yamland check if there's already any parameterization (e.g. usage of.Values.ingress.provider) rather than the hardcoded"nginx". This will help us confirm whether the review comment's suggestion to parameterize the ingress class is still applicable.
🏁 Script executed:
#!/bin/bash # Check if the file exists if [ -f charts/base-cluster/templates/cert-manager/clusterissuer.yaml ]; then echo "Displaying lines 30 to 40 from charts/base-cluster/templates/cert-manager/clusterissuer.yaml:" sed -n '30,40p' charts/base-cluster/templates/cert-manager/clusterissuer.yaml echo -e "\nSearching for parameterized ingress provider usage (e.g., .Values.ingress.provider):" grep -R 'Values.ingress.provider' charts/base-cluster/templates/cert-manager/clusterissuer.yaml || echo "No parameterization found." else echo "File charts/base-cluster/templates/cert-manager/clusterissuer.yaml not found." fiLength of output: 1005
Parameterize HTTP01 Ingress Class in Cluster Issuer Template
The configuration for the HTTP01 challenge is still hardcoded to use"nginx". Although parameterization of the ingress provider is conditionally used elsewhere in the template (as seen with.Values.ingress.providerin the surrounding logic), the HTTP01 section remains unchanged. To support multiple ingress controllers (e.g., traefik), please replace the hardcoded value with the parameterized value, taking into account a fallback or handling for"none". For example:- class: nginx + class: {{ .Values.ingress.provider }}
- File:
charts/base-cluster/templates/cert-manager/clusterissuer.yaml- Lines: 34-37
Please ensure the resulting template remains valid for all cases (including when
.Values.ingress.provideris"none").
🧹 Nitpick comments (9)
charts/base-cluster/templates/kyverno/kyverno.yaml (1)
60-60: Enhanced Label Rendering for MonitoringThe new use of
include "common.tplvalues.render"replaces the basic YAML conversion. Passing a dictionary with"value": .Values.monitoring.labelsand"context": .and then indenting by 8 spaces provides a more flexible and maintainable approach to rendering labels. Ensure that downstream consumers of these labels can handle the new structure as expected.charts/base-cluster/ci/artifacthub-values-2.yaml (1)
1-7: Artifact Hub Global and Ingress SettingsThe file introduces global cluster settings (like
clusterName,baseDomain, andserviceLevelAgreement) and configures the ingress provider astraefik. This addition conforms to the broader change from a boolean flag to a descriptive provider model. Double-check that these values are consistent with other configuration sources and documentation.charts/base-cluster/templates/global/cluster-ingress.yaml (1)
1-22: Updated Ingress Conditional LogicChanging from a boolean check to
(ne .Values.ingress.provider "none")makes the decision logic for rendering the ingress resource more explicit and aligned with the new configuration model. This ensures that ingress resources are deployed only when an actual provider (other than"none") is specified. Verify that all related documentation and values files reflect this updated conditional logic.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/ci/artifacthub-values.yaml (1)
55-56: Ingress configuration updated.
The booleanenabledflag has been replaced with a descriptiveproviderfield (set tonginxhere). Ensure that the documentation reflects that multiple providers (e.g.,nginxandtraefik) are supported and guides users on how to select the appropriate option.charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml (1)
11-12: Consistent Label Rendering Update:
The change from using a direct YAML conversion (e.g.,toYaml) to the templated rendering viacommon.tplvalues.renderis well applied here. It improves consistency across your Helm templates. Please verify that the combined output (from the standard labels and the additional monitoring labels) renders with the correct YAML structure and indentation as expected by Prometheus.charts/base-cluster/templates/NOTES.txt (1)
23-24: Refined Grafana Ingress Access Logic:
The condition on line 23,{{- if and (ne .Values.ingress.provider "none") .Values.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain) }}appropriately distinguishes scenarios where an ingress is active versus when it is disabled (i.e., set to
"none"). This makes the output more robust when the ingress provider switches (e.g., totraefik).Minor Style Note: Static analysis flagged some repetitive wording in this conditional. While this is not critical, consider revising for improved clarity if it impacts maintainability or readability.
🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (1)
12-12: YAML Indentation Warning:
The static analysis tool (YAMLlint) indicates that the indentation on line 12 appears to be off—expecting an indent of 2 spaces rather than 4. To address this, you might adjust thenindentparameter. For example:- {{- with .Values.monitoring.labels }}{{- include "common.tplvalues.render" (dict "value" . "context" $) | nindent 4 }}{{- end }} + {{- with .Values.monitoring.labels }}{{- include "common.tplvalues.render" (dict "value" . "context" $) | nindent 2 }}{{- end }}Please verify that this change produces valid YAML once rendered and that it aligns with the expected structure in the metadata.labels section.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/monitoring/kdave/kdave.yaml (1)
44-44: Review YAML Label Concatenation.
The concatenation of the outputs frominclude "common.helm.labels"andinclude "common.tplvalues.render"on a single line might lead to formatting issues in the rendered YAML. It is recommended to insert a newline between the two includes to ensure that the resulting mapping is valid and easily readable.A suggested diff:
- labels: {{- include "common.helm.labels" (dict) | nindent 4 }}{{- include "common.tplvalues.render" (dict "value" .Values.monitoring.labels "context" .) | nindent 4 }} + labels: + {{- include "common.helm.labels" (dict) | nindent 4 }} + {{- include "common.tplvalues.render" (dict "value" .Values.monitoring.labels "context" .) | nindent 4 }}charts/base-cluster/templates/dns/external-dns.yaml (1)
54-58: Enhance Gateway API support in the external-dns sources.
The addition of new source entries—gateway-httproute,gateway-grpcroute,gateway-tlsroute,gateway-tcproute, andgateway-udproute(lines 54–58)—broadens the lookup from DNS-related objects. Please verify that your external-DNS configuration (and the documentation) is updated accordingly so users know how these new sources affect DNS record generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
charts/base-cluster/README.md.gotmpl(3 hunks)charts/base-cluster/ci/artifacthub-values-2.yaml(1 hunks)charts/base-cluster/ci/artifacthub-values.yaml(1 hunks)charts/base-cluster/ci/disabled-ingress-values.yaml(1 hunks)charts/base-cluster/ci/traefik-ingress-values.yaml(1 hunks)charts/base-cluster/templates/NOTES.txt(1 hunks)charts/base-cluster/templates/backup/velero.yaml(1 hunks)charts/base-cluster/templates/cert-manager/cert-manager.yaml(2 hunks)charts/base-cluster/templates/cert-manager/clusterissuer.yaml(1 hunks)charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml(1 hunks)charts/base-cluster/templates/descheduler/descheduler.yaml(1 hunks)charts/base-cluster/templates/dns/external-dns.yaml(3 hunks)charts/base-cluster/templates/flux/podMonitor.yaml(1 hunks)charts/base-cluster/templates/flux/rules/flux-status.yaml(1 hunks)charts/base-cluster/templates/global/cluster-ingress.yaml(1 hunks)charts/base-cluster/templates/ingress/gateway-api.yaml(1 hunks)charts/base-cluster/templates/ingress/nginx.yaml(3 hunks)charts/base-cluster/templates/ingress/traefik.yaml(1 hunks)charts/base-cluster/templates/ingress/validation.tpl(2 hunks)charts/base-cluster/templates/kyverno/kyverno.yaml(1 hunks)charts/base-cluster/templates/monitoring/alloy.yaml(2 hunks)charts/base-cluster/templates/monitoring/kdave/kdave.yaml(1 hunks)charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml(3 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml(1 hunks)charts/base-cluster/templates/monitoring/logs/loki.yaml(1 hunks)charts/base-cluster/templates/monitoring/security/trivy.yaml(1 hunks)charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml(1 hunks)charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml(1 hunks)charts/base-cluster/templates/tetragon/tetragon.yaml(1 hunks)charts/base-cluster/values.schema.json(2 hunks)charts/base-cluster/values.yaml(5 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
charts/base-cluster/templates/global/cluster-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/gateway-api.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/traefik.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🪛 LanguageTool
charts/base-cluster/templates/NOTES.txt
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: generateDiffCommentBody
- GitHub Check: lint helm chart (base-cluster)
- GitHub Check: wait-for-checks
🔇 Additional comments (49)
charts/base-cluster/ci/traefik-ingress-values.yaml (1)
1-3: New Traefik Ingress ConfigurationThis new configuration file clearly defines the ingress provider as
traefik, aligning with the PR objective to support multiple ingress controllers. It helps distinguish between different ingress options and simplifies future changes.charts/base-cluster/ci/disabled-ingress-values.yaml (1)
1-3: Explicitly Disabling IngressThe modification replaces the old enabled/disabled boolean with
provider: none, which makes the intent explicit. This consistent approach across your configuration files improves clarity for users who wish to disable ingress.charts/base-cluster/templates/global/cluster-ingress.yaml (1)
1-1: Template Syntax NoteThe line
{{- if and .Values.dns.provider .Values.global.baseDomain (ne .Values.ingress.provider "none") }}uses Helm templating syntax. YAMLlint may incorrectly flag this as a syntax error. This is a known false positive, so it can safely be ignored.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml (1)
98-100: Improved label rendering for kube-state-metrics.
Switching toinclude "common.tplvalues.render"foradditionalLabelsmakes label processing more flexible and consistent with the new templating standards across the Helm chart.charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml (1)
24-26: Enhanced commonLabels templating in Prometheus stack configuration.
Replacing the previous YAML conversion with a templated render call viainclude "common.tplvalues.render"ensures consistent formatting and dynamic processing for common labels. This change promotes uniformity across monitoring components.charts/base-cluster/templates/monitoring/logs/loki.yaml (1)
100-102: Updated label rendering in Loki configuration.
Using the templated approach foradditionalLabelsviainclude "common.tplvalues.render"improves consistency with other monitoring templates. Ensure that label values render as expected without introducing extra whitespace or formatting issues.charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml (1)
35-36: Enhanced alertmanager matchLabels rendering.
The updated use ofinclude "common.tplvalues.render"formatchLabelsaligns with the new templating approach. This promotes consistency across your monitoring configurations. Double-check that the rendered output meets the expected format for label matching in Alertmanager.charts/base-cluster/templates/monitoring/security/trivy.yaml (1)
75-75: Updated Label Rendering via Templated FunctionThe change replaces the direct YAML conversion with a call to
common.tplvalues.render. This improves consistency and flexibility in label processing. Please verify that the output structure fromcommon.tplvalues.rendermeets our expectations, especially in terms of formatting given the 8-space indentation.charts/base-cluster/templates/tetragon/tetragon.yaml (1)
46-46: Standardized LabelsOverride RenderingThe new implementation for
labelsOverridenow usescommon.tplvalues.renderwith a 12-space indent. This is consistent with our overall refactor. Just ensure that the extra nesting level (nindent 12) produces the intended YAML hierarchy for the service monitor configuration.charts/base-cluster/templates/flux/rules/flux-status.yaml (1)
12-12: Refined Labels Inclusion for PrometheusRuleSwitching to templated rendering here—with context passed as
$—ensures more robust label formatting. It’s important to confirm that the inherited context (via$) remains correct in all cases and that the rendered output integrates seamlessly with the surrounding metadata.charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml (1)
57-57: Consistent Label Rendering in Tracing ServiceMonitorThe updated line for rendering labels with
common.tplvalues.render(nindent 10) aligns with our templating refactor across helm charts. Please double-check that the indentation and resulting YAML comply with the Grafana Tempo serviceMonitor requirements.charts/base-cluster/templates/descheduler/descheduler.yaml (1)
43-43: Harmonized AdditionalLabels RenderingThe adoption of the templated rendering for
additionalLabelsbrings descheduler in line with our standards. As long as.Values.monitoring.labelsexists and is structured as expected, the new approach (with 8-space indent) will provide the necessary flexibility and consistency.charts/base-cluster/templates/cert-manager/clusterissuer.yaml (1)
1-1: Enhanced Conditional for ClusterIssuer Creation:
Using the condition{{- if and .Values.certManager.email (ne .Values.ingress.provider "none") }}provides a more descriptive check than the old boolean flag. This ensures that the ClusterIssuer is deployed only when a cert-manager email is provided and when an ingress provider is selected. Please review that this logic aligns with all targeted use cases, including when
traefikis chosen.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml (1)
11-12: Standardized Label Rendering in Monitoring Rule:
The update replacing direct YAML conversion with the use ofcommon.tplvalues.renderis consistent with similar changes across the repository. This should help maintain uniformity in label formatting across various monitoring configurations.charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml (1)
14-14: Correct Label Rendering Update.
The updated line now usesinclude "common.tplvalues.render"with proper indentation, aligning with the new templating approach for label generation. This change improves consistency across the charts.charts/base-cluster/templates/flux/podMonitor.yaml (1)
12-12: Consistent Update for Monitoring Labels.
The change wraps the rendering ofmonitoring.labelsin awithblock and usesinclude "common.tplvalues.render", which is consistent with the standardized approach applied elsewhere. The use of$as the context is appropriate in this template.charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml (1)
47-48: Enhanced Ingress Enablement Logic.
The updated conditional now checks that.Values.ingress.provideris not set to"none"before enabling ingress along with verifying if the cert manager is configured and the domain is set. This aligns well with the new descriptive provider model (supporting options liketraefikandnginx).One consideration: verify that the default behavior (when
.Values.ingress.provideris not explicitly set) matches the intended fallback.charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml (1)
36-42: Consistent Selector Label Rendering.
The selectors (ruleSelector,serviceMonitorSelector,podMonitorSelector, andprobeSelector) now use theinclude "common.tplvalues.render"function with the appropriate context and indentation. This change standardizes label rendering, which should help maintain consistency in the Prometheus configuration.Please verify that the output structure remains valid for Prometheus label selectors.
charts/base-cluster/templates/ingress/gateway-api.yaml (1)
1-33: Gateway API configuration for Traefik looks goodThis new configuration correctly sets up the Gateway API resources when Traefik is selected as the ingress provider. The GitRepository resource references a specific version (1.2.1) of the Gateway API, and the Kustomization resource is properly configured to apply only the experimental CRDs.
The conditional rendering ensures these resources are only created when appropriate, and the labels are consistently applied using the standard template helper.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml (3)
1-1: Provider-specific conditional looks goodThe change from checking if ingress is enabled to checking if the provider is "nginx" aligns with the new approach of specifying ingress providers rather than using a boolean flag.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
29-29: Template function replacement is consistentReplacing
toYamlwithcommon.tplvalues.renderallows for more sophisticated template processing with context awareness, which is a good improvement for handling labels.
94-94: Whitespace trimming improvementAdding the dash for whitespace trimming in the end tag will result in cleaner rendered YAML output.
charts/base-cluster/templates/ingress/validation.tpl (4)
1-3: Good validation for incompatible configurationThis validation correctly prevents users from enabling nginx-specific configuration when using Traefik as the ingress provider, which would lead to confusion or errors.
5-15: Excellent provider transition validationThis validation logic provides clear guidance to users when switching between ingress providers. The detailed error messages explain exactly what steps are needed to transition safely, including preserving the LoadBalancer IP if desired.
19-20: Dynamic service resolution based on providerUsing the ternary operator to dynamically determine the service name and namespace based on the selected provider is an elegant solution that centralizes the provider-specific logic.
42-42: Improved error message with dynamic valuesThe updated error message now uses the dynamically determined service details, making it more accurate and helpful regardless of which ingress provider is in use.
charts/base-cluster/templates/cert-manager/cert-manager.yaml (2)
43-48: Gateway API support for cert-manager with TraefikThis addition enables Gateway API support in cert-manager when using Traefik, which is necessary for proper integration between the two components. The configuration correctly specifies the required ControllerConfiguration settings.
74-74: Consistent label rendering approachThis change aligns with the pattern used elsewhere in the codebase, replacing direct YAML conversion with the more flexible template rendering function for labels.
charts/base-cluster/templates/monitoring/alloy.yaml (2)
45-115: Improved code formatting and readability.The restructuring of the
discovery.kubernetesanddiscovery.relabelsections enhances readability with consistent indentation and spacing between rules. This makes the configuration easier to understand and maintain.
258-258: Standardized label rendering method.Changed from using direct
toYamlto the template functioncommon.tplvalues.render, which is a good practice for consistency across the codebase and potentially provides more flexibility in how labels are processed.charts/base-cluster/README.md.gotmpl (3)
110-119: Well-documented ingress controller options.Clear explanation of the two supported ingress controllers with their capabilities. The recommendation for Traefik is helpful, and highlighting Gateway API support is valuable for users considering more advanced ingress configurations.
133-133: Updated IP address retrieval commands for both controllers.The commands are now tailored to the selected ingress controller, which is essential for users to find their load balancer IP correctly.
347-384: Comprehensive migration guide for version 9.0.0.Excellent migration documentation that:
- Clearly explains the configuration changes from boolean flag to provider field
- Provides code examples for different scenarios
- Warns about potential downtime and IP changes
- Notes the differences in configuration approaches between nginx and traefik
This will help users smoothly transition to the new ingress model.
charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml (3)
56-56: Standardized label rendering method.Changed from using direct
toYamlto the template functioncommon.tplvalues.render, which is consistent with changes in other files and follows best practices for label rendering.
121-131: Enhanced dashboard inclusion logic for different ingress providers.The template now intelligently includes the appropriate Grafana dashboard based on the selected ingress provider:
- nginx dashboard for the nginx provider
- traefik dashboard for the traefik provider
This ensures users have the right monitoring tools for their chosen ingress controller.
183-183: Updated condition for Grafana configuration merging.Changed from checking if ingress is enabled to checking if the provider is not "none", which aligns with the new ingress provider approach. This ensures proper configuration of Grafana when any ingress provider is active.
charts/base-cluster/templates/backup/velero.yaml (1)
93-93: Standardized label rendering method.Changed from using direct
toYamlto the template functioncommon.tplvalues.renderfor both serviceMonitor and prometheusRule labels. This is consistent with similar changes across other monitoring templates and follows best practices for label rendering.Also applies to: 97-97
charts/base-cluster/templates/dns/external-dns.yaml (2)
34-37: Clarify CRD installation skipping for DNS resources.
The newly addedinstallandupgradeblocks (lines 34–37) setcrds: Skip. This makes it clear that CRDs should neither be installed nor updated during these operations. Ensure that this behavior aligns with your operational requirements and that no dependent resources rely on automatic CRD management.
83-83: Adopt templated label rendering for serviceMonitor labels.
Switching from a plain YAML conversion usingtoYamlto using theinclude "common.tplvalues.render"function (line 83) provides more controlled formatting for the serviceMonitor labels. Confirm that the rendered output meets your expectations regarding indentation and label structure.charts/base-cluster/templates/ingress/traefik.yaml (3)
1-3: Conditionally enable Traefik configuration.
The file starts with a condition that renders the Traefik configuration only when{{ eq .Values.ingress.provider "traefik" }}(lines 1–3). This is a clean and explicit check. Just ensure that any upstream or downstream references expecting a boolean flag are updated to work with the new provider string.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
20-23: Consistent CRD handling across ingress configurations.
Similar to the external-dns resource, theinstallandupgradesections here (lines 20–23) usecrds: Skip. Confirm that this behavior is consistent with how you manage CRD lifecycle across the chart and that it won’t inadvertently prevent required CRD updates.
76-86: Telemetry configuration integration is well encapsulated.
Lines 76–86 encapsulate telemetry configuration for tracing using a templated block. Verify that the included helper functioncommon.telemetry.confreturns the expected YAML mapping and that the optional insecure flag is properly handled.charts/base-cluster/values.schema.json (2)
1352-1360: Migrate ingress flag to a provider string.
The ingress configuration now includes a newproviderproperty (lines 1352–1360) with an enumerated set of"nginx","traefik", and"none". This explicit declaration is a significant improvement over a simple boolean flag. Please ensure that all templates and documentation referencing ingress behavior are updated accordingly and that downstream validation (e.g., in CI pipelines) considers this change.
153-166: Updated flexible label types in networkPolicy.
The use of aoneOfconstruct for properties likeingressLabels(lines 153–166) increases flexibility by permitting either an object or a string. Verify that the consuming templates correctly interpret both types without any issues.charts/base-cluster/values.yaml (5)
9-11: Namespace selection based on ingress provider and DNS configuration.
The condition for theingressnamespace (lines 9–11) now uses{{ or (not (empty .Values.dns.provider)) (eq .Values.ingress.provider "traefik") }}to determine if the namespace should be applied. Confirm that this logic correctly reflects your intent—specifically, that when using Traefik (or a defined DNS provider), the appropriate namespace is chosen.
27-29: Target namespace for NGINX-specific ingress.
Theingress-nginxnamespace now has a condition that checks if the ingress provider equals"nginx"(lines 27–29). This clear segregation aids in avoiding conflicts between multiple ingress controllers. Ensure that this separation is well documented and that users are aware of which namespace is used for which provider.
95-97: Dynamic assignment of ingress labels.
The labels undernetworkPolicy.ingressLabels(lines 95–97) now use a ternary operator to choose between"ingress-nginx"and"ingress"for the namespace, and between"ingress-nginx"and"traefik"for the name. Double-check that the ternary logic and quotations are interpreted as intended by your templating engine.
129-138: Helm repository conditions for ingress controllers.
New helm repository entries for bothnginx(lines 129–133) andtraefik(lines 134–138) now conditionally deploy based on the selected ingress provider. This clearly separates the configuration for the two controllers. Verify that the repository URLs and version constraints (e.g.,4.x.xfor NGINX and34.x.xfor Traefik) meet your deployment needs.
428-436: Default ingress section reflects provider migration.
In theingressblock (lines 428–436), the booleanenabledfield has been replaced by aproviderfield set to"nginx". This change is central to the PR’s objective. Ensure that all related templates, tests, and documentation reference the new configuration option to avoid any deployment misconfigurations.
🤖 I have diffed this beep boop"/$namespace/$kind/$name.yaml" for normal resources
|
cwrau
force-pushed
the
feat/base-cluster/add-traefik-ingress-controller
branch
from
4910662 to
e97ce9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
charts/base-cluster/templates/global/cluster-ingress.yaml (1)
2-6: Dead Code in API Version Selection BlockThere is an
if falsebranch that renders a hardcoded API version which is never executed. If this branch isn’t intended for future use or is kept for historical reasons, consider removing it to reduce confusion. If it may be reused later, consider adding a clarifying comment.charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (1)
12-12: Indentation of Rendered Monitoring LabelsA YAMLlint warning suggests that the indentation in the rendered monitoring labels (line 12) might be off (expecting 2 spaces but finding 4). This could be a false positive due to templating; however, please verify that the rendered YAML structure is correct. If necessary, adjusting the
nindentvalue (for example, from 4 to 2) may resolve the warning.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/NOTES.txt (1)
23-23: Minor Language Style ImprovementStatic analysis noted that several successive sentences (or templated outputs) begin with the same word. While this does not affect functionality, consider rewording the output text for improved readability.
🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml (1)
34-36: HTTP01 Solver – Ingress Configuration CheckIn the HTTP01 solver block (lines 34–36), the prior ingress class specification has been removed, leaving only a
serviceType: ClusterIPunderingress. Please confirm that omitting an explicit ingress class will work as intended with your supported ingress controllers (e.g. Traefik or NGINX). If an ingress class (or additional parameters) is needed for proper challenge routing in certain environments, you might need to add that parameter back.charts/base-cluster/templates/ingress/gateway-api.yaml (1)
23-24: Label Block Indentation in Kustomization ResourceA static analysis warning notes that the indentation for the label on line 24 (within the Kustomization metadata) may be off (expected 2 spaces but found 4). While this could be a false positive due to templating, please verify that the rendered YAML meets the expected indentation. If an adjustment is needed, consider modifying the
nindentvalue in the label inclusion.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/dns/external-dns.yaml (1)
34-37: Skipping CRD Installation/Upgrade for External-DNS
The new configuration specifyingcrds: Skipunder both theinstallandupgradesections ensures that CRDs are managed externally rather than by this HelmRelease. Confirm that the CRD lifecycle is handled elsewhere (e.g. via a separate release or manual process) and is documented accordingly.charts/base-cluster/values.yaml (3)
10-10: Clarify Ingress Namespace Condition:
The condition now enables theingressnamespace when either a DNS provider is defined or the ingress provider is explicitly set to "traefik". Please verify that this combined logic accurately reflects the intended deployment scenarios. Consider adding an inline comment to clarify the rationale behind the use of this logical OR for future maintainers.
96-97: Dynamic Ingress Labels in NetworkPolicy:
The updatedingressLabelsnow dynamically assign values based on the ingress provider using a ternary operator. When the provider is "nginx", the labels resolve to "ingress-nginx"; otherwise, they default to "ingress" for the namespace and "traefik" for the app name. Please ensure that this behavior is properly documented and that it gracefully handles any unexpected values (for example, if the provider is set to "none").
429-429: Default Ingress Provider Setting:
Settingprovider: nginxin the ingress configuration clearly communicates the default behavior. Confirm that this default aligns with existing deployments and documentation. If there is an expectation for users to switch to "traefik" without manual changes in this file, consider making this value configurable via a centralized parameter or providing a migration note in the release documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
charts/base-cluster/README.md.gotmpl(3 hunks)charts/base-cluster/ci/artifacthub-values-2.yaml(1 hunks)charts/base-cluster/ci/artifacthub-values.yaml(1 hunks)charts/base-cluster/ci/disabled-ingress-values.yaml(1 hunks)charts/base-cluster/ci/traefik-ingress-values.yaml(1 hunks)charts/base-cluster/templates/NOTES.txt(1 hunks)charts/base-cluster/templates/backup/velero.yaml(1 hunks)charts/base-cluster/templates/cert-manager/cert-manager.yaml(2 hunks)charts/base-cluster/templates/cert-manager/clusterissuer.yaml(1 hunks)charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml(1 hunks)charts/base-cluster/templates/descheduler/descheduler.yaml(1 hunks)charts/base-cluster/templates/dns/external-dns.yaml(3 hunks)charts/base-cluster/templates/flux/podMonitor.yaml(1 hunks)charts/base-cluster/templates/flux/rules/flux-status.yaml(1 hunks)charts/base-cluster/templates/global/cluster-ingress.yaml(1 hunks)charts/base-cluster/templates/ingress/gateway-api.yaml(1 hunks)charts/base-cluster/templates/ingress/nginx.yaml(3 hunks)charts/base-cluster/templates/ingress/traefik.yaml(1 hunks)charts/base-cluster/templates/ingress/validation.tpl(2 hunks)charts/base-cluster/templates/kyverno/kyverno.yaml(1 hunks)charts/base-cluster/templates/monitoring/alloy.yaml(2 hunks)charts/base-cluster/templates/monitoring/kdave/kdave.yaml(1 hunks)charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml(3 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml(1 hunks)charts/base-cluster/templates/monitoring/logs/loki.yaml(1 hunks)charts/base-cluster/templates/monitoring/security/trivy.yaml(1 hunks)charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml(1 hunks)charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml(1 hunks)charts/base-cluster/templates/tetragon/tetragon.yaml(1 hunks)charts/base-cluster/values.schema.json(2 hunks)charts/base-cluster/values.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (26)
- charts/base-cluster/ci/traefik-ingress-values.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml
- charts/base-cluster/ci/artifacthub-values-2.yaml
- charts/base-cluster/templates/monitoring/security/trivy.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml
- charts/base-cluster/templates/monitoring/logs/loki.yaml
- charts/base-cluster/ci/disabled-ingress-values.yaml
- charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml
- charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml
- charts/base-cluster/templates/flux/podMonitor.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml
- charts/base-cluster/templates/descheduler/descheduler.yaml
- charts/base-cluster/ci/artifacthub-values.yaml
- charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml
- charts/base-cluster/templates/tetragon/tetragon.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml
- charts/base-cluster/templates/monitoring/kdave/kdave.yaml
- charts/base-cluster/templates/ingress/validation.tpl
- charts/base-cluster/templates/kyverno/kyverno.yaml
- charts/base-cluster/README.md.gotmpl
- charts/base-cluster/templates/monitoring/alloy.yaml
- charts/base-cluster/templates/backup/velero.yaml
- charts/base-cluster/templates/flux/rules/flux-status.yaml
🧰 Additional context used
🪛 LanguageTool
charts/base-cluster/templates/NOTES.txt
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 YAMLlint (1.35.1)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/global/cluster-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/gateway-api.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/traefik.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: generateDiffCommentBody
- GitHub Check: lint helm chart (base-cluster)
- GitHub Check: wait-for-checks
🔇 Additional comments (23)
charts/base-cluster/templates/global/cluster-ingress.yaml (1)
1-1: Update Conditional Logic for Ingress RenderingThe updated condition now checks for DNS provider, baseDomain, and ensures that the ingress provider is not set to
"none", which aligns well with the new configuration paradigm. Please ensure that these prerequisite values are clearly documented for users.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (1)
1-3: Validation of Storage Size Rule InclusionThe conditional check on lines 1–3 ensures that the storage resource is only rendered when the
readWriteManyoption is enabled. This is in line with the overall configuration; no issues here.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/NOTES.txt (1)
22-29: Updated Grafana Access URL LogicThe updated conditional on lines 22–29 now relies on the new ingress provider configuration (i.e. checking that it is not
"none") along with other prerequisites (such as the Grafana ingress flag, certManager email, and domain settings). This overall looks correct. Please ensure that the documentation clearly reflects these new conditions so that users understand under what circumstances the Grafana URL will be output.🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml (1)
1-6: Conditional Rendering for ClusterIssuer ResourcesThe change on line 1 now checks that a cert-manager email is provided and that the ingress provider is not
"none". This updated logic is consistent with the new ingress configuration pattern. No issues are detected here.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/gateway-api.yaml (1)
1-1: Conditional Inclusion of Gateway API ResourcesThe file now correctly uses a condition to check if
.Values.ingress.provideris set to"traefik"before rendering the Gateway API resources. This selective inclusion helps ensure that these resources are only created when Traefik is the chosen ingress controller.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/cert-manager/cert-manager.yaml (1)
43-48: Traefik-specific Cert-Manager Configuration Block
This conditional block adds a configuration section that enables the Gateway API for cert-manager when the ingress provider is set to"traefik". The implementation is clear and consistent with the new ingress provider scheme. Please verify in downstream testing that no unintended configuration conflicts occur when an alternative provider (e.g."nginx") is used.charts/base-cluster/templates/ingress/nginx.yaml (2)
1-1: Explicit Conditional for NGINX Ingress
Replacing the old generic ingress flag with an explicit check against.Values.ingress.provider(equal to"nginx") improves clarity and configuration precision.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
29-29: Enhanced Label Rendering for ServiceMonitor
The change from usingtoYamlto the templated functioncommon.tplvalues.renderfor renderingadditionalLabelsmakes the behavior more robust and consistent with recent updates. Ensure that the output matches expected label formats.charts/base-cluster/templates/dns/external-dns.yaml (1)
83-83: Consistent Label Rendering for ServiceMonitor
Updating the label rendering to usecommon.tplvalues.renderhere—similar to other components—ensures uniformity across chart templates. Please validate that the rendered YAML labels are in the expected format.charts/base-cluster/templates/ingress/traefik.yaml (8)
1-9: New Traefik HelmRelease Metadata & Conditional Rendering
This new file introduces a HelmRelease for Traefik that is conditionally rendered when.Values.ingress.providerequals"traefik". The metadata (name, namespace, labels) is set correctly. Verify that the use of the"ingress"namespace aligns with your environment’s conventions.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
20-23: Skipping CRDs for Traefik Deployment
Theinstallandupgradesections properly skip CRD management (usingcrds: Skip), which is consistent with similar configurations elsewhere in the charts. Ensure that any CRDs required by Traefik are installed by an external process.
29-41: Deployment and Port Configuration
The deployment block sets replica counts and configures port settings (including redirection fromwebtowebsecureand proxy protocol flags). Confirm that the values provided in.Values.ingress.replicasand.Values.ingress.useProxyProtocolproduce the expected behavior in your test environment.
42-53: Service Annotations and LoadBalancer IP Handling
The service definition includes annotations for proxy protocol and, conditionally, a floating IP configuration when.Values.ingress.IPis set. The use of quoted values is appropriate for boolean flags. Please double-check that when an IP is provided, the rendered configuration is accepted by your cloud/load-balancer provider.
59-63: Metrics and ServiceMonitor Configuration
Enabling metrics via Prometheus and rendering theadditionalLabelsthroughcommon.tplvalues.renderensures consistency across monitoring components. It is advisable to test the output label structure to avoid any mismatches with selectors in Prometheus.
64-69: Providers Block for Ingress Resources
Disabling the Kubernetes CRD provider while enabling the Kubernetes Gateway (with the experimental channel) is a deliberate configuration aimed at using newer API patterns. Ensure that this experimental channel is acceptable in your deployment context and that fallback mechanisms are in place if needed.
76-86: Telemetry Configuration Under Conditional Tracing
The telemetry block conditionally enables OTLP tracing based on the telemetry configuration. The nested structure is clear, and using a templated check for$telemetryConf.insecureensures flexibility. Please verify that these settings correctly integrate with your observability stack.
1-1: Note on YAMLlint Warning
Static analysis (YAMLlint) reported a syntax error on line 1 (related to the templating syntax such as{{- if ... }}), which is a known false positive when linting Helm templates. No change is needed here.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/values.schema.json (4)
123-136: Enhanced Schema for Metrics Labels
ThemetricsLabelsproperty now accepts either an object (with at least one string key/value pair) or a simple string. This added flexibility can accommodate various label formats. Ensure that all consumers of this field can handle both types.
138-151: Flexible Schema for DNS Labels
Similar tometricsLabels, thednsLabelsproperty definition now leverages aoneOfconstruct to accept both objects and strings. Confirm that the components using these values perform the necessary normalization or conversion as needed.
153-166: Versatile Definition for Ingress Labels
TheingressLabelsproperty now supports both an object form and a string form. This change provides greater configuration flexibility for users. Verify that the Helm templates consuming these labels correctly interpret either structure.
1352-1360: New Ingress Provider Schema Property
The addition of theproviderproperty within theingresssection (with allowed values"nginx","traefik", and"none") clearly defines the expected input for specifying the ingress controller. Please ensure the corresponding documentation and default values in yourvalues.yamlor supporting docs are updated to reflect this change.charts/base-cluster/values.yaml (2)
28-28: Ingress-nginx Namespace Activation:
The condition now strictly checks if.Values.ingress.providerequals "nginx", ensuring that theingress-nginxnamespace is activated only when the NGINX ingress controller is selected. This change fits well with the new ingress provider model.
133-138: Helm Repository Configuration for Ingress Controllers:
The new conditional entries for helm repositories—specifically, adding a condition for the nginx repository and introducing a dedicatedtraefiksection—are a strong step toward isolating configuration per ingress controller. Please verify that the chart versions (e.g.,traefik: 34.x.x) are current and meet project requirements. Additionally, ensure that these conditions prevent conflicts if a user switches the ingress provider.
cwrau
force-pushed
the
feat/base-cluster/add-traefik-ingress-controller
branch
from
e97ce9a to
3e6eb13
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
charts/base-cluster/templates/NOTES.txt (2)
21-24: Updated Grafana URL rendering condition.
The new conditional in line ~23 now combines several factors—ensuring that the ingress provider is not "none", Grafana ingress is enabled, a cert-manager email is set, and a domain (global baseDomain or custom) is provided. This tightly controls when the Grafana URL is printed. Consider adding an inline comment to explain this multi-faceted condition for future maintainers.🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
23-23: Consider simplifying complex condition.
The condition in line ~23 is somewhat complex. A brief inline comment or refactor for clarity may help improve maintainability.🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
charts/base-cluster/templates/ingress/gateway-api.yaml (1)
23-24: Label indentation warning.
YAMLlint flags a warning on line 24 regarding indentation (expected 2 spaces but found 4). Please verify that the labelapp.kubernetes.io/component: gateway-apiis indented correctly to conform with YAML standards. A minor adjustment in the nindent value or merging it with the templated labels output might resolve this.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml (1)
29-29: Enhanced label rendering for service monitor.
Line 29 now uses thecommon.tplvalues.renderhelper to outputadditionalLabelsfor the service monitor. This change enables more dynamic label rendering. Ensure that this helper correctly handles all expected edge cases in your monitoring setup.charts/base-cluster/templates/ingress/traefik.yaml (1)
1-1: YAMLlint Static Analysis Notice
YAMLlint flagged a syntax error at the very start of the file due to the Helm templating syntax. This is a common issue when linting Helm templates. Consider adding a YAMLlint ignore directive or configuring your linter to skip these files.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
charts/base-cluster/README.md.gotmpl(3 hunks)charts/base-cluster/ci/artifacthub-values-2.yaml(1 hunks)charts/base-cluster/ci/artifacthub-values.yaml(1 hunks)charts/base-cluster/ci/disabled-ingress-values.yaml(1 hunks)charts/base-cluster/ci/traefik-ingress-values.yaml(1 hunks)charts/base-cluster/templates/NOTES.txt(1 hunks)charts/base-cluster/templates/backup/velero.yaml(1 hunks)charts/base-cluster/templates/cert-manager/cert-manager.yaml(2 hunks)charts/base-cluster/templates/cert-manager/clusterissuer.yaml(1 hunks)charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml(1 hunks)charts/base-cluster/templates/descheduler/descheduler.yaml(1 hunks)charts/base-cluster/templates/dns/external-dns.yaml(3 hunks)charts/base-cluster/templates/flux/podMonitor.yaml(1 hunks)charts/base-cluster/templates/flux/rules/flux-status.yaml(1 hunks)charts/base-cluster/templates/global/cluster-ingress.yaml(1 hunks)charts/base-cluster/templates/ingress/gateway-api.yaml(1 hunks)charts/base-cluster/templates/ingress/nginx.yaml(3 hunks)charts/base-cluster/templates/ingress/traefik.yaml(1 hunks)charts/base-cluster/templates/ingress/validation.tpl(2 hunks)charts/base-cluster/templates/kyverno/kyverno.yaml(1 hunks)charts/base-cluster/templates/monitoring/alloy.yaml(2 hunks)charts/base-cluster/templates/monitoring/kdave/kdave.yaml(1 hunks)charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml(3 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml(1 hunks)charts/base-cluster/templates/monitoring/logs/loki.yaml(1 hunks)charts/base-cluster/templates/monitoring/security/trivy.yaml(1 hunks)charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml(1 hunks)charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml(1 hunks)charts/base-cluster/templates/tetragon/tetragon.yaml(1 hunks)charts/base-cluster/values.schema.json(2 hunks)charts/base-cluster/values.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (30)
- charts/base-cluster/templates/flux/rules/flux-status.yaml
- charts/base-cluster/ci/artifacthub-values-2.yaml
- charts/base-cluster/templates/flux/podMonitor.yaml
- charts/base-cluster/ci/traefik-ingress-values.yaml
- charts/base-cluster/templates/monitoring/security/trivy.yaml
- charts/base-cluster/templates/monitoring/logs/loki.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml
- charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml
- charts/base-cluster/templates/kyverno/kyverno.yaml
- charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml
- charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml
- charts/base-cluster/ci/artifacthub-values.yaml
- charts/base-cluster/templates/descheduler/descheduler.yaml
- charts/base-cluster/templates/monitoring/kdave/kdave.yaml
- charts/base-cluster/ci/disabled-ingress-values.yaml
- charts/base-cluster/templates/ingress/validation.tpl
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml
- charts/base-cluster/templates/cert-manager/cert-manager.yaml
- charts/base-cluster/templates/tetragon/tetragon.yaml
- charts/base-cluster/README.md.gotmpl
- charts/base-cluster/templates/dns/external-dns.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml
- charts/base-cluster/templates/backup/velero.yaml
- charts/base-cluster/templates/monitoring/alloy.yaml
- charts/base-cluster/values.schema.json
- charts/base-cluster/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
charts/base-cluster/templates/ingress/traefik.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/global/cluster-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/gateway-api.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
🪛 LanguageTool
charts/base-cluster/templates/NOTES.txt
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: generateDiffCommentBody
- GitHub Check: lint helm chart (base-cluster)
🔇 Additional comments (23)
charts/base-cluster/templates/global/cluster-ingress.yaml (1)
1-22: Ingress configuration conditional update.
The updated conditional now uses(ne .Values.ingress.provider "none")along with checks for.Values.dns.providerand.Values.global.baseDomainto decide when to render the ingress configuration. This change is aligned with the PR’s objective to support multiple ingress controllers.
Note: The YAMLlint error on line 1 is a false positive due to the use of Go templating syntax.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/NOTES.txt (1)
1-17: Flux GitRepositories and decryption instructions unchanged.
The initial block (lines 1–17) that iterates over Flux GitRepositories and handles decryption remains unaffected. The formatting and logic are clear.charts/base-cluster/templates/cert-manager/clusterissuer.yaml (2)
1-5: Updated ClusterIssuer inclusion condition.
The condition now checks that.Values.certManager.emailis set and that the ingress provider is not "none" before including the ClusterIssuer resources. This fits well with the overall ingress provider changes.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
7-40: ClusterIssuer template clarity and potential impact.
The ClusterIssuer template is clear and well-structured. One point to verify is the removal of any custom ingress class configuration for the HTTP01 solver—ensure that using a fixedserviceType: ClusterIPmeets your ACME challenge requirements.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
[warning] 21-21: wrong indentation: expected 4 but found 6
(indentation)
[warning] 23-23: wrong indentation: expected 8 but found 10
(indentation)
[warning] 24-24: wrong indentation: expected 8 but found 10
(indentation)
charts/base-cluster/templates/ingress/gateway-api.yaml (1)
1-17: Conditional resource creation for Traefik.
The entire file is wrapped in a conditional that only triggers when.Values.ingress.providerequals"traefik". This appropriately ensures that the Gateway API resources (the FluxCD GitRepository and Kustomization) are only deployed for Traefik, aligning with the PR objective.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/nginx.yaml (2)
1-1: Conditional rendering for Nginx ingress.
The template now renders only when.Values.ingress.provideris set to"nginx", which is consistent with the new ingress provider model.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
20-94: Well-structured HelmRelease specification.
The HelmRelease definition for the Nginx ingress is comprehensive, covering chart specification, metric integration, telemetry, security contexts, and resources. It aligns well with best practices and the updated ingress provider framework.charts/base-cluster/templates/ingress/traefik.yaml (14)
1-1: Conditional Rendering Check
The use of the conditional{{- if eq .Values.ingress.provider "traefik" -}}appropriately ensures that this HelmRelease is rendered only when Traefik is the selected ingress provider.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
2-8: Metadata and Labeling Structure
The metadata section defines the resource name, namespace, and labels correctly. Utilizinginclude "common.labels.standard"promotes consistency across deployments.
10-11: Chart Specification Setup
The chart configuration leverages the helperinclude "base-cluster.helm.chartSpec"with a well-formed dictionary containing repo, chart, and context. Please verify that this helper returns the expected specification for the Traefik chart.
15-19: Conditional Monitoring Dependency
The block that conditionally adds a dependency on thekube-prometheus-stackwhen monitoring is enabled is well implemented. Ensure that.Values.monitoring.prometheus.enabledis properly defined in your values file.
20-23: CRDs Installation and Upgrade Handling
Skipping CRDs installation and upgrade usingcrds: Skipis a prudent choice when these resources are managed separately.
24-28: Global Image Registry Configuration
The use of thewithclause to conditionally inject the global image registry under theimagekey is neat and effective. Confirm that.Values.global.imageRegistryis set when needed.
29-30: Deployment Replica Configuration
Setting the number of replicas through.Values.ingress.replicasprovides the needed flexibility for scaling the Traefik deployment.
31-42: Ports and Proxy Protocol Configuration
The configuration for thewebandwebsecureports is well structured, including redirection details and conditional proxy protocol settings. Verify that.Values.ingress.useProxyProtocolis of the correct type (boolean or string) for proper rendering.
43-54: Service Annotations and Load Balancer Configuration
Service annotations for both OpenStack and Hetzner load balancers are clearly defined. The conditional block that adds floating IP settings based on.Values.ingress.IPis effective and ensures thatspec.loadBalancerIPis only set when needed.
55-58: Gateway Configuration
ThegatewayClassis correctly set with a default name, and the explicit disabling of a clusterwide gateway (with an explanatory comment) reinforces best practices for hostname-specific gateway creation.
59-64: Resources and Metrics Setup
Resource definitions and metrics configuration (including the conditional serviceMonitor settings) are handled gracefully. The use ofnindentensures correct YAML formatting.
65-70: Providers Configuration
Disabling the Kubernetes CRD provider and enabling the Kubernetes Gateway with the experimental channel is appropriate for this setup. It would be beneficial to document the implications of using experimental features for users.
71-76: Logging and Global Arguments
The logging configuration (JSON format) and global argument settings to disable anonymous usage reporting and version checks are correctly applied for controlled deployment behavior.
77-88: Telemetry and Tracing Configuration
The telemetry block leveragesinclude "common.telemetry.conf"and conditionally enables OTLP tracing. This addition enhances observability, provided that the telemetry configuration in values is valid.charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (2)
11-14: Consistent Label Rendering ApproachThe change from using
toYamlto leveraging theinclude "common.tplvalues.render"function enhances the consistency of monitoring label rendering across the charts. This update aligns with similar improvements in other templates and should offer more flexible label processing.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[warning] 13-13: wrong indentation: expected 2 but found 4
(indentation)
12-12:Details
❓ Verification inconclusive
Adjust Indentation for Monitoring Labels
YAMLlint has flagged an indentation issue on line 12—expected an indentation of 2 spaces but found 4. Consider reducing the indent level by changing the
nindentvalue from 4 to 2 to ensure proper YAML structure.- {{- with .Values.monitoring.labels }}{{- include "common.tplvalues.render" (dict "value" . "context" $) | nindent 4 }}{{- end }} + {{- with .Values.monitoring.labels }}{{- include "common.tplvalues.render" (dict "value" . "context" $) | nindent 2 }}{{- end }}
Below is a verification shell script to check the reported indentation issue in the file. This script will search for the occurrences of
nindent 4in the file, display the context around the flagged line, and list similar usage ofnindent 2in the chart templates for comparison.Please run the following shell script:
#!/bin/bash # 1. Verify occurrences of "nindent 4" in the affected file. echo "Searching for 'nindent 4' occurrences in charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml:" rg "nindent 4" -n charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml # 2. Display context around the reported indentation issue (lines 10 to 15). echo -e "\nDisplaying context (lines 10-15) of charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml:" sed -n '10,15p' charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml # 3. List occurrences of "nindent 2" in the chart templates for comparison. echo -e "\nListing occurrences of 'nindent 2' in charts/base-cluster/templates/ for comparison:" rg "nindent 2" -n charts/base-cluster/templates/After reviewing the output of these scripts, we can confidently confirm whether reducing the
nindentvalue from 4 to 2 resolves the YAML lint indentation issue.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
cwrau
force-pushed
the
feat/base-cluster/add-traefik-ingress-controller
branch
2 times, most recently
from
8c25e86 to
cf27158
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
charts/base-cluster/templates/ingress/nginx.yaml (1)
34-37:⚠️ Potential issueOpentelemetry Image Conditional Logic
Within the opentelemetry block, the nested condition{{- if and .Values.global.imageRegistry false }}will always evaluate to false because of the literalfalse. If the intent is to disable image mirroring until an upstream adjustment is made, it might be clearer to remove or comment out this block entirely. If not, consider revising the condition to correctly reflect the intended behavior.
🧹 Nitpick comments (6)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (1)
12-12: Indentation Adjustment for YAML ComplianceYAML linting flags an indentation issue on line 12: the linter expected an indentation of 2 spaces but encountered 4. This is likely due to the use of
nindent 4in the include call. Consider adjusting the indentation to ensure that the rendered YAML adheres to expected YAML structure. For example, you might modify the line as follows:- {{- with .Values.monitoring.labels }}{{- include "common.tplvalues.render" (dict "value" . "context" $) | nindent 4 }}{{- end }} + {{- with .Values.monitoring.labels }}{{- include "common.tplvalues.render" (dict "value" . "context" $) | nindent 2 }}{{- end }}This change should align the indentation with that expected for mapping values under
labelsand remove the lint warning.[tags: ]
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/global/cluster-ingress.yaml (1)
1-6: Helm Template Conditional and Dead-Code Block
The conditional now verifies that both a DNS provider and a base domain are set and that the ingress provider is not"none", which aligns with the new design. However, the nested{{- if false }}branch (lines 2–6) is dead code. If this branch is no longer necessary, consider removing it or adding a comment explaining its purpose for future maintainers.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/NOTES.txt (1)
23-23: Complex Grafana Access Condition Review
The conditional on this line now checks multiple parameters—including the ingress provider not being"none", Grafana ingress enablement, cert-manager email, and domain settings—to determine whether to render the external URL. Given its complexity, consider adding an inline comment or refactoring the condition for improved clarity and maintainability.🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
charts/base-cluster/templates/ingress/gateway-api.yaml (1)
7-9: Labels in GitRepository Metadata
The labels section for the GitRepository uses the standard labels inclusion with an additional override for the component. Please verify that the chosen indentation (usingnindent 4) results in the intended YAML structure after rendering.charts/base-cluster/templates/ingress/traefik.yaml (1)
1-89: Well-structured Traefik HelmRelease implementationThe implementation correctly:
- Uses conditional rendering based on the provider setting
- Configures appropriate chart reference and dependencies
- Sets up proxy protocol based on user configuration
- Integrates with the monitoring system when enabled
- Configures Gateway API support
- Disables anonymous usage reporting and version checks
- Adds telemetry configuration when applicable
One suggestion for improvement:
Consider adding a comment explaining why Gateway creation is disabled by default. While the existing comment does explain that "Gateways need to be created for each set of hostnames", it might be helpful to provide an example or link to documentation on how users should create their own Gateway resources.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/values.schema.json (1)
1361-1364: Consider updating allowNginxConfigurationSnippets property nameThe property
allowNginxConfigurationSnippetsis now provider-specific but still has "Nginx" in the name. Since the documentation mentions this is not applicable for Traefik, consider renaming it to something provider-agnostic or adding a condition to hide/disable this option when Traefik is selected.- "allowNginxConfigurationSnippets": { + "configurationSnippets": {Or add a clarification in the description that this only applies to the nginx provider.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
charts/base-cluster/README.md.gotmpl(3 hunks)charts/base-cluster/ci/artifacthub-values-2.yaml(1 hunks)charts/base-cluster/ci/artifacthub-values.yaml(1 hunks)charts/base-cluster/ci/disabled-ingress-values.yaml(1 hunks)charts/base-cluster/ci/traefik-ingress-values.yaml(1 hunks)charts/base-cluster/templates/NOTES.txt(1 hunks)charts/base-cluster/templates/backup/velero.yaml(1 hunks)charts/base-cluster/templates/cert-manager/cert-manager.yaml(2 hunks)charts/base-cluster/templates/cert-manager/clusterissuer.yaml(1 hunks)charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml(1 hunks)charts/base-cluster/templates/descheduler/descheduler.yaml(1 hunks)charts/base-cluster/templates/dns/external-dns.yaml(3 hunks)charts/base-cluster/templates/flux/podMonitor.yaml(1 hunks)charts/base-cluster/templates/flux/rules/flux-status.yaml(1 hunks)charts/base-cluster/templates/global/cluster-ingress.yaml(1 hunks)charts/base-cluster/templates/ingress/gateway-api.yaml(1 hunks)charts/base-cluster/templates/ingress/nginx.yaml(3 hunks)charts/base-cluster/templates/ingress/traefik.yaml(1 hunks)charts/base-cluster/templates/ingress/validation.tpl(2 hunks)charts/base-cluster/templates/kyverno/kyverno.yaml(1 hunks)charts/base-cluster/templates/monitoring/alloy.yaml(2 hunks)charts/base-cluster/templates/monitoring/kdave/kdave.yaml(1 hunks)charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml(3 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml(1 hunks)charts/base-cluster/templates/monitoring/logs/loki.yaml(1 hunks)charts/base-cluster/templates/monitoring/security/trivy.yaml(1 hunks)charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml(1 hunks)charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml(1 hunks)charts/base-cluster/templates/tetragon/tetragon.yaml(1 hunks)charts/base-cluster/values.schema.json(2 hunks)charts/base-cluster/values.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (28)
- charts/base-cluster/ci/artifacthub-values-2.yaml
- charts/base-cluster/ci/traefik-ingress-values.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml
- charts/base-cluster/ci/artifacthub-values.yaml
- charts/base-cluster/templates/kyverno/kyverno.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml
- charts/base-cluster/templates/monitoring/logs/loki.yaml
- charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml
- charts/base-cluster/templates/monitoring/security/trivy.yaml
- charts/base-cluster/templates/flux/rules/flux-status.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml
- charts/base-cluster/templates/tetragon/tetragon.yaml
- charts/base-cluster/ci/disabled-ingress-values.yaml
- charts/base-cluster/templates/ingress/validation.tpl
- charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml
- charts/base-cluster/templates/flux/podMonitor.yaml
- charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml
- charts/base-cluster/templates/monitoring/kdave/kdave.yaml
- charts/base-cluster/templates/descheduler/descheduler.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml
- charts/base-cluster/templates/dns/external-dns.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml
- charts/base-cluster/templates/backup/velero.yaml
- charts/base-cluster/templates/cert-manager/cert-manager.yaml
- charts/base-cluster/templates/monitoring/alloy.yaml
- charts/base-cluster/values.yaml
🧰 Additional context used
🪛 LanguageTool
charts/base-cluster/templates/NOTES.txt
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 YAMLlint (1.35.1)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/global/cluster-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/gateway-api.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/traefik.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint helm chart (base-cluster)
- GitHub Check: generateDiffCommentBody
- GitHub Check: wait-for-checks
🔇 Additional comments (15)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (2)
1-3: Review Conditional Rendering and Inclusion UsageThe block is conditionally rendering the resource only when
.Values.storage.readWriteMany.enabledis true. The use of theincludefunction to invoke"base-cluster.helm.resourceWithDependencies"and pass the nested resource definition along with additional labels is clear and consistent with the intended templating approach.[tags: ]
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
11-14: Monitor Labels Rendering: Verify Additional Labels InclusionThe metadata block’s label configuration now utilizes the new rendering approach. The primary labels are added via the
"common.labels.standard"include, and then additional labels are conditionally merged using the"common.tplvalues.render"function when.Values.monitoring.labelsis provided. This makes the template more modular and consistent with recent updates.[tags: ]
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[warning] 13-13: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/global/cluster-ingress.yaml (1)
14-20: Ingress Rules Structure Verification
The ingress rules section correctly uses YAML anchors for the primary host and its wildcard. Please verify that having two separate entries underrulesmeets your deployment requirements and does not conflict with any other templated logic in your charts.charts/base-cluster/templates/cert-manager/clusterissuer.yaml (2)
1-1: Conditional for ClusterIssuer Resource
The updated condition now ensures that the ClusterIssuer are rendered only if a certManager email is provided and the ingress provider is not"none". Confirm that this behavior matches all of your intended deployment scenarios, especially when cert-manager resources are required.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
7-7: Define Block for ClusterIssuer Resource
Thedefineblock forbase-cluster.cert-manager.clusterIssueris well encapsulated. Just ensure that merging values withmustMergedoes not unintentionally override any custom configurations provided by users in their values.charts/base-cluster/templates/ingress/gateway-api.yaml (3)
1-1: Ingress Provider Check for Traefik
The conditional correctly renders this new resource only when the ingress provider is set to"traefik". This helps avoid deploying the Gateway API resources in environments where Traefik is not used.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
14-17: GitRepository Ignore Field Validation
Theignorefield is set to exclude everything except files underconfig/crd/experimental. Confirm that this exclusion pattern accurately captures your intent to only manage the relevant CRDs via FluxCD.
18-33: Kustomization Resource Configuration
The Kustomization resource correctly references the GitRepository and enables pruning. Double-check that the specified interval (1h) and path (config/crd/experimental) align with your operational expectations.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml (2)
1-1: Ingress Provider Condition Check
The condition now specifically targets cases where the ingress provider is"nginx". Ensure that all related configurations are activated only in this scenario and that it does not conflict with the Traefik or disabled ingress configurations.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
29-29: Additional Labels Rendering Enhancement
Switching from direct YAML conversion to using thecommon.tplvalues.renderfunction for renderingadditionalLabelsimproves flexibility and context handling. Please verify that the rendered labels produce valid YAML and meet your labeling conventions.charts/base-cluster/README.md.gotmpl (3)
110-119: Great addition of Traefik as an ingress controller option!Clear documentation of the available ingress controllers and their capabilities. The description is concise and highlights the key features of each option including their IngressClassName behavior and built-in capabilities.
133-134: Good update to IP retrieval instructionsThe command instructions now properly cover both ingress controllers, making it easier for users to retrieve their load balancer IP regardless of which provider they're using.
347-384: Well-documented migration path to the new ingress configurationThe migration section provides comprehensive guidance for users transitioning from version 8.x to 9.0.0, including:
- Clear instructions for those with disabled ingress
- Recommendations for switching to Traefik
- Warnings about potential downtime during transition
- Information about IP preservation
- Explanation of configuration differences between nginx and traefik
This is essential for helping users navigate the breaking change from a boolean flag to a provider-based configuration.
charts/base-cluster/values.schema.json (2)
125-166: Improved flexibility for label configurationsThe schema changes for
metricsLabels,dnsLabels, andingressLabelsnow properly support either object or string formats using theoneOfconstruct. This provides more flexibility for label configurations while maintaining schema validation.
1352-1360: Good replacement of boolean flag with descriptive provider enumReplacing the boolean
enabledwith a stringproviderenum that explicitly lists the supported options ("nginx", "traefik", "none") makes the configuration more descriptive and self-documenting. The enum values are consistent with the documentation and implementation.
cwrau
force-pushed
the
feat/base-cluster/add-traefik-ingress-controller
branch
from
cf27158 to
cf54bb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (1)
12-12: Review labels indentation.
YAMLlint reports a warning regarding indentation (expected 2 but found 4) for the rendered monitoring labels. Please verify that the final YAML output is correctly indented. If the extra indent is unintentional, consider adjusting thenindentvalue (e.g., tonindent 2) to match the expected structure.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/gateway-api.yaml (2)
1-8: Review Traefik Gateway API GitRepository configuration.
The conditional correctly limits the deployment of this resource to cases where the ingress provider is set to"traefik". Please ensure that the usage ofnindentfor the labels (e.g., in line 7) produces the desired YAML structure. YAMLlint errors at the template start can be safely ignored as they stem from Helm syntax.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
23-24: Adjust label indentation in Kustomization block.
YAMLlint flags a potential indentation issue on line 24 (expected 2 spaces but found 4). Verify that the rendered labels in the Kustomization metadata are correctly indented, and consider adjusting the indent level in theincludefunction if necessary.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
charts/base-cluster/README.md.gotmpl(3 hunks)charts/base-cluster/ci/artifacthub-values-2.yaml(1 hunks)charts/base-cluster/ci/artifacthub-values.yaml(1 hunks)charts/base-cluster/ci/disabled-ingress-values.yaml(1 hunks)charts/base-cluster/ci/traefik-ingress-values.yaml(1 hunks)charts/base-cluster/templates/NOTES.txt(1 hunks)charts/base-cluster/templates/backup/velero.yaml(1 hunks)charts/base-cluster/templates/cert-manager/cert-manager.yaml(2 hunks)charts/base-cluster/templates/cert-manager/clusterissuer.yaml(1 hunks)charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml(1 hunks)charts/base-cluster/templates/descheduler/descheduler.yaml(1 hunks)charts/base-cluster/templates/dns/external-dns.yaml(3 hunks)charts/base-cluster/templates/flux/podMonitor.yaml(1 hunks)charts/base-cluster/templates/flux/rules/flux-status.yaml(1 hunks)charts/base-cluster/templates/global/cluster-ingress.yaml(1 hunks)charts/base-cluster/templates/ingress/gateway-api.yaml(1 hunks)charts/base-cluster/templates/ingress/nginx.yaml(3 hunks)charts/base-cluster/templates/ingress/traefik.yaml(1 hunks)charts/base-cluster/templates/ingress/validation.tpl(2 hunks)charts/base-cluster/templates/kyverno/kyverno.yaml(1 hunks)charts/base-cluster/templates/monitoring/alloy.yaml(2 hunks)charts/base-cluster/templates/monitoring/kdave/kdave.yaml(1 hunks)charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml(3 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml(1 hunks)charts/base-cluster/templates/monitoring/logs/loki.yaml(1 hunks)charts/base-cluster/templates/monitoring/security/trivy.yaml(1 hunks)charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml(1 hunks)charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml(1 hunks)charts/base-cluster/templates/tetragon/tetragon.yaml(1 hunks)charts/base-cluster/values.schema.json(2 hunks)charts/base-cluster/values.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (28)
- charts/base-cluster/ci/traefik-ingress-values.yaml
- charts/base-cluster/templates/monitoring/security/trivy.yaml
- charts/base-cluster/ci/artifacthub-values-2.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml
- charts/base-cluster/templates/kyverno/kyverno.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml
- charts/base-cluster/templates/flux/podMonitor.yaml
- charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml
- charts/base-cluster/ci/artifacthub-values.yaml
- charts/base-cluster/templates/descheduler/descheduler.yaml
- charts/base-cluster/templates/monitoring/logs/loki.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml
- charts/base-cluster/templates/flux/rules/flux-status.yaml
- charts/base-cluster/ci/disabled-ingress-values.yaml
- charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml
- charts/base-cluster/templates/monitoring/kdave/kdave.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml
- charts/base-cluster/templates/tetragon/tetragon.yaml
- charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml
- charts/base-cluster/templates/ingress/validation.tpl
- charts/base-cluster/templates/backup/velero.yaml
- charts/base-cluster/templates/dns/external-dns.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml
- charts/base-cluster/templates/monitoring/alloy.yaml
- charts/base-cluster/values.schema.json
- charts/base-cluster/values.yaml
🧰 Additional context used
🪛 LanguageTool
charts/base-cluster/templates/NOTES.txt
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 YAMLlint (1.35.1)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/global/cluster-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/gateway-api.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/traefik.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint helm chart (base-cluster)
- GitHub Check: generateDiffCommentBody
- GitHub Check: Update release-please config file for a possibly new chart
- GitHub Check: wait-for-checks
🔇 Additional comments (15)
charts/base-cluster/templates/global/cluster-ingress.yaml (1)
1-1: Clarify ingress rendering condition.
The updated condition using(ne .Values.ingress.provider "none")ensures that the ingress configuration is only rendered when a valid provider is specified, aligning with the PR’s objective of shifting from a boolean flag to a provider-based approach.
Note: YAMLlint’s syntax error on this line is a false positive due to Helm template syntax.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/NOTES.txt (1)
23-24: Update Grafana ingress condition.
The modified condition now checks that.Values.ingress.provideris not"none", in addition to verifying.Values.monitoring.grafana.ingress.enabled, a validcertManager.email, and a domain value. Ensure that this comprehensive condition meets your intended criteria for generating the Grafana access URL. Additionally, if any style warnings regarding repetitive sentence starts appear in the rendered output, consider rephrasing as needed.🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml (1)
1-1: Validate ClusterIssuer condition logic.
The change now requires both a validcertManager.emailand an ingress provider that is not"none", which is consistent with the new ingress configuration approach. Any YAMLlint errors at this starting delimiter (due to the{{- if ... }}syntax) can be safely disregarded as they are expected in Helm templates.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/cert-manager/cert-manager.yaml (2)
43-48: Good implementation of Gateway API support for TraefikThe conditional block correctly enables Gateway API support in cert-manager when Traefik is selected as the ingress provider. This is essential for Traefik to function properly with cert-manager for certificate management in Gateway API mode.
74-74: Improved template rendering for monitoring labelsSwitching from
toYamltoinclude "common.tplvalues.render"provides more flexibility for template rendering and maintains consistency with other similar changes throughout the codebase.charts/base-cluster/templates/ingress/nginx.yaml (3)
1-1: Proper conditional statement for nginx providerThe condition has been updated to specifically check for the nginx provider rather than using a generic enabled flag. This matches the new approach to ingress provider selection.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
29-29: Consistent template rendering for labelsUsing
common.tplvalues.renderfor the additional labels maintains consistency with the approach used in other files.
97-97: Appropriate whitespace control in templateAdding whitespace trimming (
-) to the end conditional marker results in cleaner rendered YAML output.charts/base-cluster/README.md.gotmpl (3)
110-120: Comprehensive documentation of ingress optionsThe documentation clearly describes both supported ingress controllers, their capabilities, and key features. The recommendation of Traefik with reference to the Gateway API support is well-justified.
133-133: Updated IP lookup commands for both controllersThe command to retrieve the load balancer IP has been updated to work with both ingress controllers, which is essential for users following the documentation.
348-386: Detailed migration guidanceThe migration section provides clear instructions for users updating from previous versions, including how to:
- Change from
enabled: falsetoprovider: none- Specify nginx explicitly if needed
- Handle IP address changes when switching providers
- Understand the differences between nginx configuration snippets and Gateway API
This helps ensure a smooth transition for users.
charts/base-cluster/templates/ingress/traefik.yaml (4)
1-24: Well-structured HelmRelease definition for TraefikThe HelmRelease is properly conditioned on the ingress provider setting and follows best practices:
- Appropriate namespace and labels
- Drift detection enabled
- Monitoring dependency correctly specified
- Skip CRDs during install and upgrade to avoid conflicts
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
25-55: Comprehensive service and port configurationThe configuration includes:
- Image registry settings
- Replica count from values
- HTTP to HTTPS redirections
- Proxy protocol settings based on values
- Load balancer annotations with conditional floating IP handling
The service configuration is thorough and aligns with the functionality provided by the nginx controller.
56-72: Proper Gateway API configurationThe Gateway API settings are well-configured:
- Default gateway class name
- Gateway creation disabled with a good explanation (needs to be hostname-specific)
- Resources properly templated
- Kubernetes CRD disabled in favor of Gateway API
- Gateway API enabled with experimental features
This correctly implements the Gateway API approach mentioned in the documentation.
73-88: Good security and observability configurationThe configuration includes:
- JSON format for logs for better parsing
- Disabled anonymous usage and version checking for privacy
- Conditional OpenTelemetry tracing configuration based on global settings
These settings enhance security and align with observability best practices.
cwrau
force-pushed
the
feat/base-cluster/add-traefik-ingress-controller
branch
from
cf54bb4 to
0c99d48
Compare
cwrau
changed the title
cwrau
force-pushed
the
feat/base-cluster/add-traefik-ingress-controller
branch
from
0c99d48 to
7d1033d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
charts/base-cluster/templates/ingress/gateway-api.yaml (2)
2-17: FluxCD GitRepository for Gateway API
The resource correctly points tokubernetes-sigs/gateway-api@1.2.1. To improve reuse, consider making thenamespacefield dynamic instead of hard‐coding"ingress".Example diff:
- namespace: ingress + namespace: {{ .Release.Namespace }}
18-33: FluxCD Kustomization for Gateway API
This Kustomization references the GitRepository and applies only theconfig/crd/experimentaldirectory. Consider exposingpath,prune, orintervalas chart values for greater configurability.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/traefik.yaml (1)
78-88: Conditional telemetry tracing.
The OTLP tracing injection with{{- if $telemetryConf.enabled }}matches the pattern in other monitoring templates. Consider standardizing thecommon.telemetry.confhelper invocation across both the nginx and traefik templates for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
charts/base-cluster/README.md.gotmpl(3 hunks)charts/base-cluster/ci/artifacthub-values-2.yaml(1 hunks)charts/base-cluster/ci/artifacthub-values.yaml(1 hunks)charts/base-cluster/ci/disabled-ingress-values.yaml(1 hunks)charts/base-cluster/ci/traefik-ingress-values.yaml(1 hunks)charts/base-cluster/templates/NOTES.txt(1 hunks)charts/base-cluster/templates/backup/velero.yaml(1 hunks)charts/base-cluster/templates/cert-manager/cert-manager.yaml(2 hunks)charts/base-cluster/templates/cert-manager/clusterissuer.yaml(1 hunks)charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml(1 hunks)charts/base-cluster/templates/descheduler/descheduler.yaml(1 hunks)charts/base-cluster/templates/dns/external-dns.yaml(3 hunks)charts/base-cluster/templates/flux/podMonitor.yaml(1 hunks)charts/base-cluster/templates/flux/rules/flux-status.yaml(1 hunks)charts/base-cluster/templates/global/cluster-ingress.yaml(1 hunks)charts/base-cluster/templates/ingress/gateway-api.yaml(1 hunks)charts/base-cluster/templates/ingress/nginx.yaml(3 hunks)charts/base-cluster/templates/ingress/traefik.yaml(1 hunks)charts/base-cluster/templates/ingress/validation.tpl(2 hunks)charts/base-cluster/templates/kyverno/kyverno.yaml(1 hunks)charts/base-cluster/templates/monitoring/alloy.yaml(2 hunks)charts/base-cluster/templates/monitoring/kdave/kdave.yaml(1 hunks)charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml(3 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml(1 hunks)charts/base-cluster/templates/monitoring/logs/loki.yaml(1 hunks)charts/base-cluster/templates/monitoring/security/trivy.yaml(1 hunks)charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml(1 hunks)charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml(1 hunks)charts/base-cluster/templates/tetragon/tetragon.yaml(1 hunks)charts/base-cluster/values.schema.json(2 hunks)charts/base-cluster/values.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (30)
- charts/base-cluster/templates/monitoring/logs/loki.yaml
- charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml
- charts/base-cluster/templates/monitoring/security/trivy.yaml
- charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml
- charts/base-cluster/templates/monitoring/kdave/kdave.yaml
- charts/base-cluster/templates/descheduler/descheduler.yaml
- charts/base-cluster/ci/artifacthub-values-2.yaml
- charts/base-cluster/ci/artifacthub-values.yaml
- charts/base-cluster/templates/flux/rules/flux-status.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml
- charts/base-cluster/templates/tetragon/tetragon.yaml
- charts/base-cluster/templates/kyverno/kyverno.yaml
- charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml
- charts/base-cluster/templates/cert-manager/cert-manager.yaml
- charts/base-cluster/templates/flux/podMonitor.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml
- charts/base-cluster/ci/disabled-ingress-values.yaml
- charts/base-cluster/templates/ingress/validation.tpl
- charts/base-cluster/templates/backup/velero.yaml
- charts/base-cluster/templates/monitoring/alloy.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml
- charts/base-cluster/templates/dns/external-dns.yaml
- charts/base-cluster/README.md.gotmpl
- charts/base-cluster/values.schema.json
- charts/base-cluster/ci/traefik-ingress-values.yaml
- charts/base-cluster/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
charts/base-cluster/templates/ingress/traefik.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/global/cluster-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/gateway-api.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
🪛 LanguageTool
charts/base-cluster/templates/NOTES.txt
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generateDiffCommentBody
🔇 Additional comments (21)
charts/base-cluster/templates/global/cluster-ingress.yaml (1)
1-1: Ingress conditional updated to provider model
The check now uses(.Values.ingress.provider != "none")instead of the deprecated boolean flag, aligning with the new multi‐provider design. Ensure thatingress.provideris defined and defaulted appropriately in yourvalues.yamlandvalues.schema.json.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (1)
12-12: Standardize monitoring label rendering
Switched from inlinetoYamlto the sharedcommon.tplvalues.renderhelper for.Values.monitoring.labels, matching other PrometheusRule templates. The YAML-lint warning about indentation can be ignored in this Helm context.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml (1)
1-1: Adapt ClusterIssuer rendering to provider model
The inclusion guard now checksingress.provider != "none"alongsidecertManager.email, deprecating the old boolean flag. Please verify that removing the explicit nginx ingressClass (if present elsewhere) doesn’t break HTTP01 challenges under your custom provider setup.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/NOTES.txt (1)
23-23: Update Grafana NOTES conditional to provider
Replaced.Values.ingress.enabledwith(ne .Values.ingress.provider "none")so Grafana’s URL is displayed only when an ingress provider is active. Confirm this renders as expected in your release.🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
charts/base-cluster/templates/ingress/gateway-api.yaml (1)
1-1: Gate Gateway API resources on Traefik
Wrapped the GitRepository/Kustomization in anif .Values.ingress.provider == "traefik"block, ensuring these CRDs are only managed when Traefik is selected.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/nginx.yaml (3)
1-1: Switch to provider-based rendering is correct.
The new conditional{{- if eq .Values.ingress.provider "nginx" -}}properly restricts this HelmRelease to wheningress.provideris set to"nginx", replacing the old boolean flag. Whitespace trimming ({{-and-}}) is applied consistently for cleaner output.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
29-29: Standardize label rendering using helper.
Switching from directtoYamlconversion toinclude "common.tplvalues.render"ensures that labels are processed uniformly and supports advanced templating logic. Indentation (nindent 12) aligns with surrounding blocks.
97-97: Clean up whitespace on closing conditional.
Using{{- end -}}trims any trailing whitespace from this block, matching the opening statement’s style and producing neat YAML output.charts/base-cluster/templates/ingress/traefik.yaml (13)
1-1: Enable rendering only for Traefik provider.
The conditional{{- if eq .Values.ingress.provider "traefik" -}}correctly gates this HelmRelease to the Traefik ingress use-case, aligning with the chart’s new provider model.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
5-6: Verify existence of theingressnamespace.
The release is targeted to namespaceingress. Ensure that this namespace is created or managed by another part of the chart; otherwise, the HelmRelease will fail to install.Could you confirm that a
Namespaceresource foringressexists in your chart (e.g., intemplates/namespace.yaml)?
10-11: Chart source specification.
Thechart.specreference to thetraefikrepository and chart name is in line with the new provider option. Verify that the global repositories list (inChart.yamlorvalues.yaml) includes an entry for"traefik".Would you mind confirming the repository entry for
"traefik"is present inChart.yamlunderdependenciesor in.Values.global.repositories?
15-19: Conditional dependency on Prometheus stack.
UsingdependsOnto ensurekube-prometheus-stackis installed before Traefik when monitoring is enabled is sound. This matches the pattern used for the nginx ingress.
20-23: Skip CRD management in install/upgrade hooks.
Configuring bothinstall.crds: Skipandupgrade.crds: Skipprevents accidental CRD reinstallation. This is a best practice for managing CRDs outside of HelmRelease.
25-29: Override name and registry context.
ThefullnameOverride: ingress-controllerand conditional image registry injection viawith .Values.global.imageRegistryprovide flexibility for naming and pulling images from private registries.
31-38: Configure HTTP→HTTPS redirection and proxy protocol.
Theports.webredirection towebsecureand proxy protocol toggles are set up correctly. Quoting the boolean forinsecureensures valid YAML when templating.
44-55: Service annotations and optional LoadBalancerIP.
Annotations for OpenStack and Hetzner load balancers are applied sensibly, and the conditionalloadBalancerIPassignment handles explicit IP bindings. Indentation is consistent with other service blocks.
56-60: Gateway API defaults.
SettinggatewayClass.name: defaultand disabling thegatewayby default aligns with the comment’s rationale. If users need a cluster-wide gateway, they can enable this manually.
60-65: Resource and metrics configuration.
Including common ingress resources and mirroring the PrometheusserviceMonitorsettings (with templated labels) ensures observability defaults match the rest of the chart.
66-71: Provider backends for Traefik.
Disabling the KubernetesCRD provider and enabling the KubernetesGateway provider (withexperimentalChannel: true) is appropriate for leveraging the Gateway API.
73-78: Logging and global arguments.
Switching to JSON logs and disabling anonymous usage reporting and version checks by default makes the deployment more secure and logging-friendly.
89-89: Clean closing of the conditional block.
Using{{- end -}}here neatly trims whitespace, matching the opening statement’s style.
…r and make it default
cwrau
force-pushed
the
feat/base-cluster/add-traefik-ingress-controller
branch
from
7d1033d to
eda7812
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
charts/base-cluster/templates/ingress/traefik.yaml (7)
5-6: Consider parameterizing the target namespace
The resource is hard-coded tonamespace: ingress. If users install multiple environments or use a different namespace, this may conflict. You might expose.Values.ingress.namespace(with a default of"ingress") or leverage{{ .Release.Namespace }}to make the deployment location configurable.
7-8: Merge customcomponentlabel into the standard labels helper
You include standard labels viacommon.labels.standardand then appendapp.kubernetes.io/component: ingress. Consider extending the standard helper to include acomponentparameter or merging both into one call to reduce boilerplate.
24-31: Custom image registry override is well scoped
Using awith .Values.global.imageRegistryto override the registry is clean. If you foresee needing to override the repository or tag as well, consider addingimage.repositoryandimage.tagunder.Values.ingressor pass them through values.
32-43: Port and proxy-protocol settings look good—consider entryPoint customization
You hard-code entryPointswebandwebsecure. For environments with custom entryPoint names, you might expose these strings via.Values.ingress.entryPoints.weband.Values.ingress.entryPoints.websecure.
56-60: GatewayClass and gateway.enabled defaults are sensible
SettinggatewayClass.name: defaultand disabling the cluster-wide Gateway by default makes sense given per-hostname requirements. If users need to enable the Gateway, consider exposingingress.gateway.enabledandingress.gatewayClass.namefor flexibility.
72-78: Logging and global arguments are explicit—consider making globalArgs configurable
Hard-coding--global.sendanonymoususage=falseand version checks here is fine, but if you anticipate more flags in the future, you could introduce a.Values.ingress.globalArgumentslist to allow overrides or additions.
1-90: Suppress YAMLlint false positive
YAMLlint flags the leading-in{{- if ... -}}as a syntax error. This is expected in Helm templates. To avoid distraction, consider adding a top-of-file directive like:# yamllint disableor adjusting your YAMLlint config to ignore Helm templating syntax.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
charts/base-cluster/README.md.gotmpl(3 hunks)charts/base-cluster/ci/artifacthub-values-2.yaml(1 hunks)charts/base-cluster/ci/artifacthub-values.yaml(1 hunks)charts/base-cluster/ci/disabled-ingress-values.yaml(1 hunks)charts/base-cluster/ci/traefik-ingress-values.yaml(1 hunks)charts/base-cluster/templates/NOTES.txt(1 hunks)charts/base-cluster/templates/backup/velero.yaml(1 hunks)charts/base-cluster/templates/cert-manager/cert-manager.yaml(2 hunks)charts/base-cluster/templates/cert-manager/clusterissuer.yaml(1 hunks)charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml(1 hunks)charts/base-cluster/templates/descheduler/descheduler.yaml(1 hunks)charts/base-cluster/templates/dns/external-dns.yaml(3 hunks)charts/base-cluster/templates/flux/podMonitor.yaml(1 hunks)charts/base-cluster/templates/flux/rules/flux-status.yaml(1 hunks)charts/base-cluster/templates/global/cluster-ingress.yaml(1 hunks)charts/base-cluster/templates/ingress/gateway-api.yaml(1 hunks)charts/base-cluster/templates/ingress/nginx.yaml(3 hunks)charts/base-cluster/templates/ingress/traefik.yaml(1 hunks)charts/base-cluster/templates/ingress/validation.tpl(2 hunks)charts/base-cluster/templates/kyverno/kyverno.yaml(1 hunks)charts/base-cluster/templates/monitoring/alloy.yaml(2 hunks)charts/base-cluster/templates/monitoring/kdave/kdave.yaml(1 hunks)charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml(3 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml(1 hunks)charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml(1 hunks)charts/base-cluster/templates/monitoring/logs/loki.yaml(1 hunks)charts/base-cluster/templates/monitoring/security/trivy.yaml(1 hunks)charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml(1 hunks)charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml(1 hunks)charts/base-cluster/templates/tetragon/tetragon.yaml(1 hunks)charts/base-cluster/values.schema.json(2 hunks)charts/base-cluster/values.yaml(5 hunks)
✅ Files skipped from review due to trivial changes (2)
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_alertmanager-config.yaml
- charts/base-cluster/templates/monitoring/alloy.yaml
🚧 Files skipped from review as they are similar to previous changes (28)
- charts/base-cluster/ci/traefik-ingress-values.yaml
- charts/base-cluster/ci/artifacthub-values.yaml
- charts/base-cluster/templates/backup/velero.yaml
- charts/base-cluster/ci/disabled-ingress-values.yaml
- charts/base-cluster/templates/cert-manager/cert-manager.yaml
- charts/base-cluster/templates/cert-manager/rules/certificate-expiration.yaml
- charts/base-cluster/templates/flux/rules/flux-status.yaml
- charts/base-cluster/templates/monitoring/logs/loki.yaml
- charts/base-cluster/templates/monitoring/kdave/rules/releases-with-deprecation.yaml
- charts/base-cluster/ci/artifacthub-values-2.yaml
- charts/base-cluster/templates/monitoring/tracing/grafana-tempo.yaml
- charts/base-cluster/templates/flux/podMonitor.yaml
- charts/base-cluster/templates/descheduler/descheduler.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_node-exporter-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus-stack-config.yaml
- charts/base-cluster/templates/tetragon/tetragon.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_kube-state-metrics-config.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_helpers.yaml
- charts/base-cluster/templates/monitoring/kdave/kdave.yaml
- charts/base-cluster/templates/ingress/validation.tpl
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_grafana-config.yaml
- charts/base-cluster/templates/dns/external-dns.yaml
- charts/base-cluster/templates/monitoring/security/trivy.yaml
- charts/base-cluster/README.md.gotmpl
- charts/base-cluster/values.schema.json
- charts/base-cluster/templates/kyverno/kyverno.yaml
- charts/base-cluster/values.yaml
- charts/base-cluster/templates/monitoring/kube-prometheus-stack/_prometheus_config.yaml
🧰 Additional context used
🪛 LanguageTool
charts/base-cluster/templates/NOTES.txt
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 YAMLlint (1.35.1)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/global/cluster-ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/gateway-api.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 24-24: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/ingress/nginx.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/traefik.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: generateDiffCommentBody
- GitHub Check: lint helm chart (base-cluster)
- GitHub Check: wait-for-checks
🔇 Additional comments (18)
charts/base-cluster/templates/nfs-server-provisioner/rules/storage-size.yaml (1)
12-12: Standardize monitoring labels with shared helper
Great job replacing the rawtoYamlcall withcommon.tplvalues.render—this aligns label rendering here with the rest of the chart. Please verify that the rendered indentation still produces valid YAML in the final manifest (the linter hint about “wrong indentation” is likely a false positive against the template).🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
charts/base-cluster/templates/global/cluster-ingress.yaml (1)
1-1: Ingress inclusion now driven by provider
Replaced the boolean check.Values.ingress.enabledwith(ne .Values.ingress.provider "none"), aligning this template with the new provider-based ingress model.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/cert-manager/clusterissuer.yaml (2)
1-1: ClusterIssuer rendering tied to provider field
The conditional now uses(ne .Values.ingress.provider "none")instead of.Values.ingress.enabled, matching the chart-wide ingress provider changes.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
34-39: Verify HTTP01 solver ingressClassName for Traefik
The HTTP01 solver omits an explicitingressClassName, relying on the cluster default. To ensure cert-manager routes challenges correctly with Traefik, confirm that the default ingress class matches Traefik’s class or conditionally set:solvers: - http01: ingress: class: traefik serviceType: ClusterIPWould you like to add conditional logic to specify
ingressClassName: traefikwhen.Values.ingress.provideris"traefik"?charts/base-cluster/templates/ingress/nginx.yaml (3)
1-1: Scope NGINX HelmRelease tonginxprovider only
Updated the guard toeq .Values.ingress.provider "nginx", ensuring this resource is only rendered for the NGINX controller.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
29-29: Standardize label rendering for Prometheus
Switched tocommon.tplvalues.renderforadditionalLabelsto maintain consistent templating across all monitoring resources.
97-97: Trim whitespace on closing conditional
Using{{- end -}}cleans up extra blank lines in the rendered manifest.charts/base-cluster/templates/NOTES.txt (1)
23-23: Align Grafana access logic with provider model
Changed the condition to(ne .Values.ingress.provider "none")for displaying the Grafana URL, consistent with other ingress templates.🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.monitoring.grafana.ingress.enabled .Values.certManager.email (or .Values.global.ba...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....enabled .Values.certManager.email (or .Values.global.baseDomain .Values.monitoring.gr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...er.email (or .Values.global.baseDomain .Values.monitoring.grafana.ingress.customDomain...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
charts/base-cluster/templates/ingress/gateway-api.yaml (1)
1-1: Enable Gateway API resources for Traefik
Wrapped GitRepository and Kustomization definitions inif eq .Values.ingress.provider "traefik", correctly gating Gateway API support to the Traefik provider.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/base-cluster/templates/ingress/traefik.yaml (9)
1-1: Conditional rendering based on ingress.provider is correct
Using{{- if eq .Values.ingress.provider "traefik" -}}ensures this HelmRelease only renders when Traefik is selected.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
15-19: Conditional dependency on Prometheus stack
ThedependsOnblock ties Traefik’s rollout to the kube-prometheus-stack when Prometheus is enabled. This is good for ordering; just verify your Flux version supportsdependsOnin HelmRelease v2.
20-23: Skipping CRD management—verify CRDs are handled elsewhere
You setcrds: Skipfor both install and upgrade. Make sure Traefik’s required CRDs (IngressRoute, Middleware, etc.) are installed by another chart or via Gateway API CRDs so that nothing breaks at runtime.
44-51: Load-balancer annotations quoting is correct—verify provider expectations
Quoting boolean values with| quoteproduces"true"/"false", which aligns with most cloud LBs. Double-check that OpenStack and Hetzner accept lowercase strings; if they require uppercase, you may need atplorupperfunction.
52-55: ConditionalloadBalancerIPinjection is correct
WrappingloadBalancerIPunderif .Values.ingress.IPand quoting it ensures Kubernetes accepts the IP as a string.
61-65: Consistent metrics labels via common helper
Great use ofcommon.tplvalues.renderforserviceMonitor.additionalLabels—this keeps labels consistent across charts.
66-71: Providers configuration is aligned with Gateway API
Disabling the Kubernetes CRD provider and enabling the Gateway provider withexperimentalChannel: truecorrectly reflects your multi-provider strategy.
79-88: Conditional tracing block is well implemented
Extracting telemetry config viainclude "common.telemetry.conf"andfromYamlthen gating onenabledis robust. This pattern cleanly avoids rendering empty blocks.
89-90: Proper closure of theifblock
The{{- end -}}correctly closes the rendering guard.
🤖 I have created a release *beep* *boop* --- ## [8.0.0](base-cluster-v7.2.1...base-cluster-v8.0.0) (2025-05-27) ### ⚠ BREAKING CHANGES * **base-cluster/ingress:** add option traefik for ingress controller and make it default ([#1420](#1420)) * **base-cluster/monitoring:** migrate promtail to alloy ([#1347](#1347)) ### Features * **base-cluster/ingress-nginx:** use risk-level Critical when annotations are enabled ([#1417](#1417)) ([a9d8ef2](a9d8ef2)) * **base-cluster/ingress:** add option traefik for ingress controller and make it default ([#1420](#1420)) ([f62b197](f62b197)) * **base-cluster/ingress:** rename oauth-proxies to have a clean name ([#1434](#1434)) ([27a28d5](27a28d5)) * **base-cluster/monitoring:** migrate promtail to alloy ([#1347](#1347)) ([24db445](24db445)) * **base-cluster/monitoring:** rename alloy to be a generic name ([#1433](#1433)) ([3f5826a](3f5826a)) ### Bug Fixes * **base-cluster/cert-manager:** metrics collection ([#1397](#1397)) ([71e1189](71e1189)) * **base-cluster/rbac:** *RoleBindings should always be prefixed to avoid collision ([#1484](#1484)) ([75de246](75de246)) ### Miscellaneous Chores * **base-cluster/monitoring:** remove deprecated plugin ([#1478](#1478)) ([ee22df5](ee22df5)) * **base-cluster:** formatting ([#1424](#1424)) ([853f146](853f146)) * **base-cluster:** pin all versions ([#1447](#1447)) ([ec8a430](ec8a430)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for the Traefik ingress controller, now recommended and available as an alternative to NGINX. - Updated ingress configuration to use a new provider option, allowing selection between "nginx", "traefik", or "none". - Expanded documentation with detailed migration guides and ingress controller comparisons. - **Breaking Changes** - Traefik is now the default ingress controller; configuration for disabling ingress has changed. - Monitoring stack migrated from promtail to alloy; Loki deployment updated and namespace changed. - Deprecated properties and configuration options removed or replaced. - **Bug Fixes** - Improved metrics collection in cert-manager. - Resolved RBAC role binding name collisions. - **Chores** - Updated container image versions and removed deprecated plugins. - Reformatted and reorganized chart metadata and documentation for clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
3 participants
Summary by CodeRabbit