External-dns and new clusterissuer dns01 Cloudflare - #374

packages/system/external-dns/.helmignore (1)

1-3: LGTM! Consider adding more common ignore patterns.

The current entries in the .helmignore file are appropriate and will help keep the Helm chart clean. Good job on including images, hack, and .gitkeep.

Consider adding these common ignore patterns to make the .helmignore file more comprehensive:

 images
 hack
 .gitkeep
+# Common VCS dirs
+.git/
+.gitignore
+.bzr/
+.bzrignore
+.hg/
+.hgignore
+.svn/
+# Common backup files
+*.swp
+*.bak
+*.tmp
+*.orig
+*~
+# Various IDEs
+.project
+.idea/
+*.tmproj
+.vscode/

These additional patterns will help exclude version control system files, backup files, and IDE-specific files, which are typically not needed in a Helm chart.

packages/system/external-dns/charts/external-dns/ci/ci-values.yaml (1)

1-2: LGTM! Consider adding a comment for clarity.

The configuration looks good and is appropriate for CI purposes. The use of the "inmemory" provider is perfect for testing the external-dns chart without affecting real DNS records.

Consider adding a comment at the top of the file to explain its purpose, like this:

# CI configuration for external-dns chart
# Uses in-memory provider for testing without affecting real DNS records
provider:
  name: inmemory

This addition would improve clarity for other developers who might work on this file in the future.

packages/system/external-dns/Chart.yaml (1)

1-3: LGTM! Consider adding more metadata.

The basic structure of the Chart.yaml file is correct and follows Helm 3 conventions. The use of a placeholder version (0.0.0) is a good practice for automated versioning during the build process.

To enhance the chart's documentation and maintainability, consider adding more metadata fields such as description, type, appVersion, and maintainers. Here's an example of how you could expand the file:

apiVersion: v2
name: cozy-external-dns
version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process
description: A Helm chart for deploying ExternalDNS in the Cozystack environment
type: application
appVersion: "0.13.1"  # Replace with the actual ExternalDNS version being used
maintainers:
  - name: Cozystack Team
    email: team@cozystack.io

These additional fields provide more context about the chart and make it easier for other developers to understand its purpose and maintenance status.

packages/system/external-dns/Makefile (2)

6-10: LGTM: Well-structured 'update' target with a minor suggestion.

The 'update' target is well-structured and follows a logical sequence of operations. It correctly manages the external-dns chart using Helm commands.

Consider adding a -p flag to the rm command for safer execution:

 update:
-	rm -rf charts
+	rm -rf -p charts
 	helm repo add external-dns https://kubernetes-sigs.github.io/external-dns/
 	helm repo update external-dns
 	helm pull external-dns/external-dns --untar --untardir charts

This change ensures that rm doesn't throw an error if the 'charts' directory doesn't exist.

---

1-10: Overall, excellent Makefile for managing external-dns chart.

This Makefile is well-structured and aligns perfectly with the PR objectives. It provides a clear and maintainable way to manage the external-dns chart, which is crucial for the integration of External-DNS into the deployment bundles. The use of variables, inclusion of a parent Makefile, and the well-defined 'update' target all contribute to a robust and flexible configuration.

As the project grows, consider creating a common.mk file for shared variables like NAMESPACE prefix, if not already present. This would enhance consistency across different package Makefiles.

packages/system/external-dns/charts/external-dns/.helmignore (2)

4-4: LGTM: Ignoring system-specific files

Including .DS_Store is a good practice, especially for cross-platform development.

Consider adding Thumbs.db to ignore Windows-specific thumbnail cache files:

 .DS_Store
+Thumbs.db

---

19-23: LGTM: Good coverage of IDE-specific files

The list covers some of the most popular IDEs, which is good for maintaining a clean package.

Consider adding patterns for a few more IDEs to make it more comprehensive:

 .project
 .idea/
 *.tmproj
 .vscode/
+*.sublime-project
+*.sublime-workspace
+.atom/
+.vs/
+*.code-workspace

These additions would cover Sublime Text, Atom, Visual Studio, and an alternative Visual Studio Code workspace file.

packages/system/external-dns/charts/external-dns/templates/serviceaccount.yaml (1)

1-17: Well-structured ServiceAccount template with good practices.

The template for creating a ServiceAccount is well-designed and follows Kubernetes best practices. Here are some positive aspects:

1. Conditional creation based on .Values.serviceAccount.create.
2. Proper use of API version and kind for ServiceAccount.
3. Utilization of the include function for name and labels, promoting consistency.
4. Support for custom labels and annotations.
5. Configurable automountServiceAccountToken for security considerations.

For improved readability, consider adding a blank line before line 16 to separate the automountServiceAccountToken field from the metadata section:

  {{- end }}
+
automountServiceAccountToken: {{ .Values.serviceAccount.automountServiceAccountToken }}

🧰 Tools

🪛 yamllint

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

(syntax)

packages/system/external-dns/charts/external-dns/templates/clusterrolebinding.yaml (2)

3-7: LGTM: Dynamic resource kind and metadata, with a suggestion.

The dynamic resource kind and metadata section are well-implemented, allowing for flexible deployment options. The use of templating for name and labels ensures consistency across resources.

Consider adding a comment explaining the purpose of the "-viewer" suffix in the resource name. This will help clarify its intended use for future maintainers.

---

1-16: Overall: Well-implemented RBAC configuration for external-dns.

This file successfully implements a flexible RBAC configuration for external-dns, aligning with the PR objectives. Key points:

1. Conditional creation of RoleBinding or ClusterRoleBinding based on the namespaced value.
2. Dynamic resource naming and labeling using Helm templating.
3. Proper reference to the associated Role/ClusterRole and ServiceAccount.

The implementation allows for easy configuration changes and supports both namespaced and cluster-wide deployments, which is crucial for the integration of External-DNS into various deployment scenarios.

Consider documenting the expected values for .Values.rbac.create and .Values.namespaced in the chart's README or values.yaml file to guide users in properly configuring these RBAC resources.

🧰 Tools

🪛 yamllint

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

(syntax)

packages/system/external-dns/charts/external-dns/RELEASE.md (1)

1-3: Update the pull request number in the release notes.

The update to ExternalDNS v0.15.0 is good, but the pull request number is missing. Please replace "xxxx" with the actual pull request number for proper documentation and traceability.

packages/system/external-dns/charts/external-dns/templates/service.yaml (2)

1-12: LGTM! Consider adding a comment for clarity.

The metadata section is well-structured and follows Helm chart best practices. The use of template functions and conditional inclusion of annotations is correct.

Consider adding a brief comment before line 1 to explain the purpose of the $providerName variable, as it's used later in the template. For example:

{{- /* Define provider name for conditional logic later in the template */ -}}
{{- $providerName := include "external-dns.providerName" . }}

🧰 Tools

🪛 yamllint

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

(syntax)

---

13-36: LGTM! Consider simplifying the conditional webhook port logic.

The service specification is well-structured and correctly implements the required functionality. The use of conditional logic for IP families and ports is appropriate.

Consider simplifying the conditional logic for the webhook port by moving the with statement inside the if block. This can reduce nesting and improve readability:

  ports:
    - name: http
      port: {{ .Values.service.port }}
      targetPort: http
      protocol: TCP
    {{- if eq $providerName "webhook" }}
    {{- with .Values.provider.webhook.service }}
    - name: http-webhook
      port: {{ .port }}
      targetPort: http-webhook
      protocol: TCP
    {{- end }}
    {{- end }}

This change maintains the same functionality while slightly improving the template's readability.

packages/extra/ingress/templates/dashboard.yaml (3)

1-2: Approve the dynamic configuration lookup with a suggestion.

The addition of the ConfigMap lookup for the cluster issuer type is a good improvement, allowing for dynamic configuration. This change aligns well with the PR objectives of enhancing flexibility in configuration.

Consider adding a comment explaining the purpose of this lookup and the possible values for clusterissuer. This would improve code readability and maintainability. For example:

{{- /* Lookup the cluster issuer type from the cozystack ConfigMap. Possible values: "http01", "cloudflare" */ -}}
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
{{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }}

🧰 Tools

🪛 yamllint

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

(syntax)

---

[error] 2-2: trailing spaces

(trailing-spaces)

---

13-16: Approve the conditional annotation with a suggestion for clarity.

The addition of the conditional block for setting the acme.cert-manager.io/http01-ingress-class annotation based on the issuer type is correct and aligns with the PR objectives. This allows for proper configuration when using different issuers, such as Cloudflare for DNS challenges.

To improve code clarity and maintainability, consider restructuring the conditional block as follows:

{{- if ne $issuerType "cloudflare" }}
    acme.cert-manager.io/http01-ingress-class: tenant-root
{{- end }}

This change eliminates the empty else block and makes the condition more explicit, improving readability.

🧰 Tools

🪛 yamllint

[error] 13-13: trailing spaces

(trailing-spaces)

---

[error] 14-14: trailing spaces

(trailing-spaces)

---

[error] 15-15: trailing spaces

(trailing-spaces)

---

[error] 16-16: trailing spaces

(trailing-spaces)

---

1-35: Address trailing spaces throughout the file.

The static analysis tool has reported multiple instances of trailing spaces in the file. While these don't affect functionality, removing them can improve code cleanliness and prevent unnecessary diffs in version control.

Consider running a linter or using an editor feature to remove trailing spaces throughout the file. This will improve code consistency and adhere to common YAML style guidelines.

Note: The syntax error reported on line 1 appears to be a false positive due to the Go templating syntax and can be safely ignored.

🧰 Tools

🪛 yamllint

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

(syntax)

---

[error] 2-2: trailing spaces

(trailing-spaces)

---

[error] 4-4: trailing spaces

(trailing-spaces)

---

[error] 5-5: trailing spaces

(trailing-spaces)

---

[error] 7-7: trailing spaces

(trailing-spaces)

---

[error] 8-8: trailing spaces

(trailing-spaces)

---

[error] 9-9: trailing spaces

(trailing-spaces)

---

[error] 10-10: trailing spaces

(trailing-spaces)

---

[error] 11-11: trailing spaces

(trailing-spaces)

---

[error] 12-12: trailing spaces

(trailing-spaces)

---

[error] 13-13: trailing spaces

(trailing-spaces)

---

[error] 14-14: trailing spaces

(trailing-spaces)

---

[error] 15-15: trailing spaces

(trailing-spaces)

---

[error] 16-16: trailing spaces

(trailing-spaces)

---

[error] 17-17: trailing spaces

(trailing-spaces)

---

[error] 18-18: trailing spaces

(trailing-spaces)

---

[error] 19-19: trailing spaces

(trailing-spaces)

---

[error] 20-20: trailing spaces

(trailing-spaces)

---

[error] 21-21: trailing spaces

(trailing-spaces)

---

[error] 22-22: trailing spaces

(trailing-spaces)

---

[error] 23-23: trailing spaces

(trailing-spaces)

---

[error] 24-24: trailing spaces

(trailing-spaces)

---

[error] 25-25: trailing spaces

(trailing-spaces)

---

[error] 26-26: trailing spaces

(trailing-spaces)

---

[error] 27-27: trailing spaces

(trailing-spaces)

---

[error] 28-28: trailing spaces

(trailing-spaces)

---

[error] 29-29: trailing spaces

(trailing-spaces)

---

[error] 30-30: trailing spaces

(trailing-spaces)

---

[error] 31-31: trailing spaces

(trailing-spaces)

---

[error] 32-32: trailing spaces

(trailing-spaces)

---

[error] 33-33: trailing spaces

(trailing-spaces)

---

[error] 34-34: trailing spaces

(trailing-spaces)

---

[error] 35-35: trailing spaces

(trailing-spaces)

packages/system/external-dns/charts/external-dns/Chart.yaml (1)

13-33: Comprehensive chart metadata with room for minor improvement.

The chart metadata is well-structured and provides essential information for users. The appVersion and version are consistent with the changes mentioned in the annotations.

Consider adding a brief note in the description or as a separate annotation to highlight the new features related to Cloudflare DNS01 ClusterIssuer support, as mentioned in the PR objectives. This would provide users with immediate visibility into this significant enhancement.

packages/system/cert-manager-issuers/templates/cluster-issuers.yaml (4)

1-2: LGTM! Consider enhancing readability.

The dynamic configuration approach using ConfigMap lookup is well-implemented and aligns with the PR objectives. It provides flexibility in configuration as mentioned in the PR summary.

Consider adding a brief comment explaining the purpose of these variables for better readability:

{{- /* Lookup ConfigMap for dynamic configuration */ -}}
{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}  
{{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }}

🧰 Tools

🪛 yamllint

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

(syntax)

---

[error] 2-2: trailing spaces

(trailing-spaces)

---

4-24: LGTM! Fix indentation for better readability.

The dynamic solver configuration for the letsencrypt-prod ClusterIssuer is well-implemented and aligns with the PR objectives. It correctly supports both dns01 (Cloudflare) and http01 (NGINX ingress) challenge types based on the $issuerType variable.

Please fix the indentation issues to improve readability and consistency:

apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  name: letsencrypt-prod
spec:
  acme:
    privateKeySecretRef:
      name: letsencrypt-prod
    server: https://acme-v02.api.letsencrypt.org/directory
    solvers:
    - {{- if eq $issuerType "cloudflare" }}
      dns01:
        cloudflare:
          apiTokenSecretRef:
            name: cloudflare-api-token-secret
            key: api-token
    {{- else }}
      http01:
        ingress:
          class: nginx
    {{- end }}

🧰 Tools

🪛 yamllint

[error] 4-4: trailing spaces

(trailing-spaces)

---

[error] 5-5: trailing spaces

(trailing-spaces)

---

[error] 6-6: trailing spaces

(trailing-spaces)

---

[error] 7-7: trailing spaces

(trailing-spaces)

---

[error] 8-8: trailing spaces

(trailing-spaces)

---

[error] 9-9: trailing spaces

(trailing-spaces)

---

[error] 10-10: trailing spaces

(trailing-spaces)

---

[error] 11-11: trailing spaces

(trailing-spaces)

---

[error] 12-12: trailing spaces

(trailing-spaces)

---

[error] 13-13: trailing spaces

(trailing-spaces)

---

[error] 14-14: trailing spaces

(trailing-spaces)

---

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

(indentation)

---

[error] 15-15: trailing spaces

(trailing-spaces)

---

[error] 16-16: trailing spaces

(trailing-spaces)

---

[error] 17-17: trailing spaces

(trailing-spaces)

---

[error] 18-18: trailing spaces

(trailing-spaces)

---

[error] 19-19: trailing spaces

(trailing-spaces)

---

[warning] 20-20: wrong indentation: expected 4 but found 6

(indentation)

---

[error] 20-20: trailing spaces

(trailing-spaces)

---

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

(indentation)

---

[error] 21-21: trailing spaces

(trailing-spaces)

---

[error] 22-22: trailing spaces

(trailing-spaces)

---

[error] 23-23: trailing spaces

(trailing-spaces)

---

[warning] 24-24: wrong indentation: expected 4 but found 6

(indentation)

---

[error] 24-24: trailing spaces

(trailing-spaces)

---

51-56: LGTM! Add newline at end of file.

The selfsigned-cluster-issuer configuration is correct and doesn't require any changes.

Add a newline at the end of the file to comply with YAML best practices:

apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  name: selfsigned-cluster-issuer
spec:
  selfSigned: {}

🧰 Tools

🪛 yamllint

[error] 51-51: trailing spaces

(trailing-spaces)

---

[error] 52-52: trailing spaces

(trailing-spaces)

---

[error] 53-53: trailing spaces

(trailing-spaces)

---

[error] 54-54: trailing spaces

(trailing-spaces)

---

[error] 55-55: trailing spaces

(trailing-spaces)

---

[error] 56-56: no new line character at the end of file

(new-line-at-end-of-file)

---

1-56: Overall, great implementation with minor formatting issues.

The changes successfully implement dynamic configuration for ClusterIssuers, supporting both Cloudflare DNS01 and HTTP01 challenge types as outlined in the PR objectives. The implementation allows for easy switching between different issuers by updating the ConfigMap, which enhances flexibility and maintainability.

Consider running a global formatting pass on the file to address the following issues:

1. Remove trailing spaces at the end of lines.
2. Ensure consistent indentation throughout the file (2 spaces for YAML is standard).
3. Add a newline at the end of the file.

These formatting improvements will enhance readability and align with YAML best practices.

🧰 Tools

🪛 yamllint

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

(syntax)

---

[error] 2-2: trailing spaces

(trailing-spaces)

---

[error] 4-4: trailing spaces

(trailing-spaces)

---

[error] 5-5: trailing spaces

(trailing-spaces)

---

[error] 6-6: trailing spaces

(trailing-spaces)

---

[error] 7-7: trailing spaces

(trailing-spaces)

---

[error] 8-8: trailing spaces

(trailing-spaces)

---

[error] 9-9: trailing spaces

(trailing-spaces)

---

[error] 10-10: trailing spaces

(trailing-spaces)

---

[error] 11-11: trailing spaces

(trailing-spaces)

---

[error] 12-12: trailing spaces

(trailing-spaces)

---

[error] 13-13: trailing spaces

(trailing-spaces)

---

[error] 14-14: trailing spaces

(trailing-spaces)

---

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

(indentation)

---

[error] 15-15: trailing spaces

(trailing-spaces)

---

[error] 16-16: trailing spaces

(trailing-spaces)

---

[error] 17-17: trailing spaces

(trailing-spaces)

---

[error] 18-18: trailing spaces

(trailing-spaces)

---

[error] 19-19: trailing spaces

(trailing-spaces)

---

[warning] 20-20: wrong indentation: expected 4 but found 6

(indentation)

---

[error] 20-20: trailing spaces

(trailing-spaces)

---

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

(indentation)

---

[error] 21-21: trailing spaces

(trailing-spaces)

---

[error] 22-22: trailing spaces

(trailing-spaces)

---

[error] 23-23: trailing spaces

(trailing-spaces)

---

[warning] 24-24: wrong indentation: expected 4 but found 6

(indentation)

---

[error] 24-24: trailing spaces

(trailing-spaces)

---

[error] 26-26: trailing spaces

(trailing-spaces)

---

[error] 28-28: trailing spaces

(trailing-spaces)

---

[error] 29-29: trailing spaces

(trailing-spaces)

---

[error] 30-30: trailing spaces

(trailing-spaces)

---

[error] 31-31: trailing spaces

(trailing-spaces)

---

[error] 32-32: trailing spaces

(trailing-spaces)

---

[error] 33-33: trailing spaces

(trailing-spaces)

---

[error] 35-35: trailing spaces

(trailing-spaces)

---

[error] 36-36: trailing spaces

(trailing-spaces)

---

[error] 37-37: trailing spaces

(trailing-spaces)

---

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

(indentation)

---

[error] 38-38: trailing spaces

(trailing-spaces)

---

[error] 39-39: trailing spaces

(trailing-spaces)

---

[error] 40-40: trailing spaces

(trailing-spaces)

---

[error] 41-41: trailing spaces

(trailing-spaces)

---

[error] 42-42: trailing spaces

(trailing-spaces)

---

[warning] 43-43: wrong indentation: expected 4 but found 6

(indentation)

---

[error] 43-43: trailing spaces

(trailing-spaces)

---

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

(indentation)

---

[error] 44-44: trailing spaces

(trailing-spaces)

---

[error] 45-45: trailing spaces

(trailing-spaces)

---

[error] 46-46: trailing spaces

(trailing-spaces)

---

[warning] 47-47: wrong indentation: expected 4 but found 6

(indentation)

---

[error] 47-47: trailing spaces

(trailing-spaces)

---

[error] 49-49: trailing spaces

(trailing-spaces)

---

[error] 51-51: trailing spaces

(trailing-spaces)

---

[error] 52-52: trailing spaces

(trailing-spaces)

---

[error] 53-53: trailing spaces

(trailing-spaces)

---

[error] 54-54: trailing spaces

(trailing-spaces)

---

[error] 55-55: trailing spaces

(trailing-spaces)

---

[error] 56-56: no new line character at the end of file

(new-line-at-end-of-file)

packages/system/external-dns/charts/external-dns/values.schema.json (1)

55-91: LGTM: Service property is well-structured. Consider adding descriptions.

The service property is correctly implemented with appropriate sub-properties for annotations, IP families, IP family policy, and port. The use of enums for IP families and IP family policy ensures valid inputs, and the port is correctly defined as an integer.

To improve clarity and ease of use, consider adding description fields to each property. For example:

 "service": {
   "type": "object",
+  "description": "Configuration for the external-dns service",
   "properties": {
     "annotations": {
-      "type": "object"
+      "type": "object",
+      "description": "Annotations to add to the service"
     },
     "ipFamilies": {
+      "description": "List of IP families to use for the service",
       "type": "array",
       // ... rest of the ipFamilies definition
     },
     // ... add descriptions for ipFamilyPolicy and port
   }
 }

This addition would make the schema more self-documenting and easier for users to understand without referring to external documentation.

packages/system/external-dns/charts/external-dns/templates/servicemonitor.yaml (4)

8-16: LGTM: Flexible metadata configuration with a minor suggestion

The metadata section is well-structured:

• Conditional inclusion of annotations from .Values.serviceMonitor.annotations allows for customization.
• Labels are consistently applied using the include function.
• Additional labels can be added, providing extensibility.

These practices offer good flexibility in resource identification and selection.

Consider adding a comment before line 14 to clarify the purpose of the additional labels, e.g.:

  labels:
    {{- include "external-dns.labels" . | nindent 4 }}
    # Additional custom labels
  {{- with .Values.serviceMonitor.additionalLabels }}
    {{- toYaml . | nindent 4 }}
  {{- end }}

This would improve readability and make the purpose of this block more explicit.

---

17-24: LGTM: Well-configured spec with a suggestion for enhanced flexibility

The spec section is correctly configured:

• The jobLabel is appropriately set to "app.kubernetes.io/instance".
• The namespaceSelector correctly targets the release namespace.
• The selector uses labels from a template function, ensuring consistency.

This configuration ensures that the ServiceMonitor targets the correct resources.

Consider adding an option to allow multi-namespace monitoring. This could be achieved by making the namespaceSelector configurable:

  namespaceSelector:
  {{- if .Values.serviceMonitor.namespaceSelector }}
    {{- toYaml .Values.serviceMonitor.namespaceSelector | nindent 4 }}
  {{- else }}
    matchNames:
      - {{ .Release.Namespace }}
  {{- end }}

This would provide more flexibility for users who might want to monitor external-dns across multiple namespaces.

---

25-81: LGTM: Comprehensive and flexible endpoint configuration

The endpoints section is well-structured and highly configurable:

• The main endpoint is thoroughly configurable with various optional parameters.
• A conditional block for the "webhook" provider adds necessary specificity.
• Extensive use of .Values for configuration promotes reusability and ease of customization.

This implementation provides great flexibility for different monitoring scenarios.

To improve maintainability, consider extracting the common endpoint configuration into a named template. This would reduce duplication between the main endpoint and the webhook endpoint:

{{- define "external-dns.serviceMonitor.endpoint" -}}
path: /metrics
{{- with .interval }}
interval: {{ . }}
{{- end }}
# ... (other common configurations)
{{- end }}

endpoints:
  - port: http
    {{- include "external-dns.serviceMonitor.endpoint" .Values.serviceMonitor | nindent 4 }}
  {{- if eq $providerName "webhook" }}
  {{- with .Values.provider.webhook.serviceMonitor }}
  - port: http-webhook
    {{- include "external-dns.serviceMonitor.endpoint" . | nindent 4 }}
  {{- end }}
  {{- end }}

This approach would make the template more DRY and easier to maintain.

---

82-86: LGTM: Flexible targetLabels configuration

The targetLabels section is well-implemented:

• Conditional inclusion based on .Values.serviceMonitor.targetLabels keeps the configuration clean.
• Allows for additional label selection flexibility in the ServiceMonitor.

This implementation provides a good balance between functionality and simplicity.

Consider adding a brief comment explaining the purpose and impact of targetLabels:

  {{- with .Values.serviceMonitor.targetLabels }}
  # Additional labels to transfer from targets to metrics
  targetLabels:
    {{- toYaml . | nindent 4 }}
  {{- end }}

This would help users understand the significance of this optional configuration.

packages/core/platform/bundles/distro-full.yaml (2)

144-148: LGTM! Consider adding privileged field for consistency.

The new external-dns release entry is well-structured and aligns with the PR objectives. It follows the naming conventions and uses appropriate dependencies.

For consistency with other entries, consider adding the privileged field, even if set to false. This makes the configuration more explicit:

- name: external-dns
  releaseName: external-dns
  chart: cozy-external-dns
  namespace: cozy-external-dns
  privileged: false
  dependsOn: [cilium,cert-manager]

🧰 Tools

🪛 yamllint

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

(commas)

---

[error] 148-148: no new line character at the end of file

(new-line-at-end-of-file)

---

[error] 148-148: trailing spaces

(trailing-spaces)

---

148-148: Fix minor YAML formatting issues.

Please address the following minor formatting issues:

1. Add a space after each comma in the dependsOn list.
2. Ensure there's a newline at the end of the file.
3. Remove any trailing spaces.

Apply this diff to fix the formatting:

-  dependsOn: [cilium,cert-manager]  
+  dependsOn: [cilium, cert-manager]
+

🧰 Tools

🪛 yamllint

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

(commas)

---

[error] 148-148: no new line character at the end of file

(new-line-at-end-of-file)

---

[error] 148-148: trailing spaces

(trailing-spaces)

packages/extra/monitoring/templates/grafana/grafana.yaml (2)

1-2: LGTM! Minor formatting suggestion.

The new variables $cozyConfig and $issuerType are correctly implemented to retrieve the ConfigMap data and cluster issuer type. This aligns well with the PR objectives for flexible configuration.

Consider removing the trailing space at the end of line 2 to adhere to YAML best practices:

-{{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }} 
+{{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }}

🧰 Tools

🪛 yamllint

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

(syntax)

---

[error] 2-2: trailing spaces

(trailing-spaces)

---

96-98: LGTM! Indentation needs adjustment.

The conditional logic for setting the HTTP01 challenge annotation based on the issuer type is correctly implemented. This change supports the PR objective of flexible configuration for different issuers.

Please adjust the indentation to maintain consistency with the rest of the file:

 {{- if ne $issuerType "cloudflare" }}
-        acme.cert-manager.io/http01-ingress-class: "{{ $ingress }}"  
+      acme.cert-manager.io/http01-ingress-class: "{{ $ingress }}"
 {{- end }}

Also, remove the trailing spaces at the end of the line.

🧰 Tools

🪛 yamllint

[warning] 97-97: wrong indentation: expected 6 but found 8

(indentation)

---

[error] 97-97: trailing spaces

(trailing-spaces)

packages/system/external-dns/charts/external-dns/README.md.gotmpl (1)

28-50: Consider expanding the providers table or explaining its current state

The providers section is informative and well-structured. However, the table of providers with specific configuration support only includes 'webhook'. Consider either:

1. Expanding this table to include other providers that have specific configuration support, or
2. Explaining why only 'webhook' is listed (e.g., if it's the only provider with special configuration needs).

This addition would provide more clarity to users about provider configuration options.

packages/system/external-dns/charts/external-dns/crds/dnsendpoint.yaml (2)

1-16: LGTM! Consider future versioning strategy.

The CRD structure and metadata are well-defined and follow Kubernetes best practices. The use of v1alpha1 is appropriate for an initial release.

As the API matures, consider planning for future versions (e.g., v1beta1, v1) and how you'll handle API evolution and deprecation.

---

40-88: LGTM! Consider enhancing recordType definition.

The endpoints structure is well-defined and covers all necessary aspects of DNS record management. The inclusion of providerSpecific properties allows for great flexibility.

Consider enhancing the recordType property by providing an enum of allowed values (e.g., A, AAAA, CNAME, TXT, SRV) to improve validation and user guidance.

packages/system/external-dns/charts/external-dns/templates/clusterrole.yaml (5)

1-7: LGTM! Consider adding a comment for clarity.

The initial structure and metadata of the role are well-defined. The conditional creation and dynamic role type based on .Values.namespaced provide flexibility.

Consider adding a comment at the beginning of the file to briefly explain the purpose of this role and its conditional nature. For example:

{{- if .Values.rbac.create -}}
# This file defines a Role or ClusterRole for External DNS based on configuration.
# The role type and permissions are dynamically generated based on .Values settings.
apiVersion: rbac.authorization.k8s.io/v1
...

🧰 Tools

🪛 yamllint

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

(syntax)

---

9-23: LGTM! Consider standardizing permission blocks.

The permissions for node, pod, and service resources are well-structured and correctly conditioned on the relevant source types.

For consistency, consider standardizing the format of each permission block. For example:

{{- if or (has "pod" .Values.sources) (has "service" .Values.sources) (has "contour-httpproxy" .Values.sources) (has "gloo-proxy" .Values.sources) (has "openshift-route" .Values.sources) (has "skipper-routegroup" .Values.sources) }}
  - apiGroups: [""]
    resources: ["pods"]
    verbs: ["get", "watch", "list"]
{{- end }}

Note the consistent spacing and comma placement in the verbs array.

---

24-64: LGTM! Consider grouping related permissions.

The permissions for various ingress and gateway resources are well-defined and correctly conditioned on the relevant source types. The wide range of supported types demonstrates good compatibility.

To improve maintainability, consider grouping related permissions together and adding comments to separate major sections. For example:

# Ingress permissions
{{- if or (has "ingress" .Values.sources) (has "contour-httpproxy" .Values.sources) (has "openshift-route" .Values.sources) (has "skipper-routegroup" .Values.sources) }}
  - apiGroups: ["extensions", "networking.k8s.io"]
    resources: ["ingresses"]
    verbs: ["get", "watch", "list"]
{{- end }}

# Istio permissions
{{- if or (has "istio-gateway" .Values.sources) (has "istio-virtualservice" .Values.sources) }}
  - apiGroups: ["networking.istio.io"]
    resources: ["gateways"]
    verbs: ["get", "watch", "list"]
{{- end }}

This grouping will make it easier to maintain and update permissions for specific ecosystems in the future.

---

65-90: LGTM! Consider optimizing permission checks.

The permissions for Gateway API resources are well-defined and correctly conditioned on the relevant source types. The support for various Gateway API resources demonstrates good compatibility with the latest Kubernetes networking features.

To improve efficiency and reduce redundancy, consider combining the checks for Gateway API resources. For example:

{{- $gatewayAPIResources := list "gateway-httproute" "gateway-grpcroute" "gateway-tlsroute" "gateway-tcproute" "gateway-udproute" }}
{{- if or (has "gateway-httproute" .Values.sources) (has "gateway-grpcroute" .Values.sources) (has "gateway-tlsroute" .Values.sources) (has "gateway-tcproute" .Values.sources) (has "gateway-udproute" .Values.sources) }}
  - apiGroups: ["gateway.networking.k8s.io"]
    resources: ["gateways"]
    verbs: ["get", "watch", "list"]
  - apiGroups: [""]
    resources: ["namespaces"]
    verbs: ["get", "watch", "list"]
  {{- range $resource := $gatewayAPIResources }}
  {{- if has $resource $.Values.sources }}
  - apiGroups: ["gateway.networking.k8s.io"]
    resources: ["{{ $resource | trimPrefix "gateway-" }}s"]
    verbs: ["get", "watch", "list"]
  {{- end }}
  {{- end }}
{{- end }}

This approach reduces repetition and makes it easier to add or modify Gateway API resources in the future.

---

91-127: LGTM! Consider adding documentation for custom permissions.

The permissions for additional resources (Gloo, Kong, Traefik, OpenShift, etc.) are well-defined and correctly conditioned on the relevant source types. The inclusion of custom additional permissions via .Values.rbac.additionalPermissions provides excellent flexibility.

To improve usability, consider adding a comment explaining the usage of .Values.rbac.additionalPermissions. For example:

{{- with .Values.rbac.additionalPermissions }}
  # Additional custom permissions can be added here.
  # Example:
  # additionalPermissions:
  #   - apiGroups: ["custom.example.com"]
  #     resources: ["customresources"]
  #     verbs: ["get", "list", "watch"]
  {{- toYaml . | nindent 2 }}
{{- end }}

This comment will help users understand how to properly use the custom permissions feature.

packages/extra/monitoring/templates/alerta/alerta.yaml (1)

152-154: Approve changes with minor style improvements

The conditional annotation for the HTTP01 challenge is correctly implemented and aligns with the PR objectives. It provides the necessary flexibility to support different ClusterIssuers based on the configuration.

Consider removing the trailing spaces at the end of these lines for better code style:

-    {{- if ne $issuerType "cloudflare" }} 
-    acme.cert-manager.io/http01-ingress-class: {{ $ingress }}  
-    {{- end }} 
+    {{- if ne $issuerType "cloudflare" }}
+    acme.cert-manager.io/http01-ingress-class: {{ $ingress }}
+    {{- end }}

🧰 Tools

🪛 yamllint

[error] 152-152: trailing spaces

(trailing-spaces)

---

[error] 153-153: trailing spaces

(trailing-spaces)

---

[error] 154-154: trailing spaces

(trailing-spaces)

packages/core/platform/bundles/paas-full.yaml (1)

220-224: LGTM! Consider minor formatting adjustment.

The new external-dns release configuration is well-structured and aligns with the PR objectives. It's correctly placed in the release list and has appropriate dependencies.

For consistency with other entries, consider removing the extra space before the dependsOn key:

  namespace: cozy-external-dns
- dependsOn: [cilium,kubeovn,cert-manager]  
+ dependsOn: [cilium,kubeovn,cert-manager]

🧰 Tools

🪛 yamllint

[error] 224-224: no new line character at the end of file

(new-line-at-end-of-file)

---

[error] 224-224: trailing spaces

(trailing-spaces)

packages/system/external-dns/charts/external-dns/templates/deployment.yaml (1)

14-14: Consider using multiple replicas for high availability.

The deployment is currently set to use only one replica. For production environments, it's recommended to use multiple replicas to ensure high availability and fault tolerance of the external-dns service.

Consider updating the replicas count based on your specific requirements:

replicas: {{ .Values.replicaCount | default 2 }}

This change allows you to set the replica count through Helm values, with a default of 2 if not specified.

packages/system/external-dns/charts/external-dns/values.yaml (2)

25-52: Consider adding a security note for RBAC

The service account and RBAC configuration looks good and provides flexibility. However, it might be beneficial to add a comment about the security implications of RBAC, especially when rbac.create is set to true. This could help users understand the permissions being granted to the external-dns service.

Consider adding a comment like:

rbac:
  # -- If `true`, create a `ClusterRole` & `ClusterRoleBinding` with access to the Kubernetes API.
  # Note: Enabling this grants cluster-wide permissions. Ensure this aligns with your security policies.
  create: true

---

194-195: Consider adding a note about the update interval

The default update interval is set to 1 minute, which might be too frequent for some use cases and could potentially cause unnecessary API calls to the DNS provider.

Consider adding a comment to highlight this:

# -- Interval for DNS updates. Note: Too frequent updates may cause rate limiting with some providers.
interval: 1m

Users can then adjust this value based on their specific needs and provider limitations.

packages/system/external-dns/charts/external-dns/CHANGELOG.md (1)

19-20: Consider utilizing the UNRELEASED section

The "UNRELEASED" section is currently empty. Consider using this section to document changes that are planned for the next release but haven't been released yet. This practice can help users and contributors stay informed about upcoming changes and provide transparency about the project's development direction.

packages/system/external-dns/charts/external-dns/README.md (3)

19-31: Minor grammatical improvement suggested

The installation instructions are clear and helpful. However, there's a small grammatical improvement that can be made to enhance readability.

Consider adding a comma after "chart" in line 21:

-Before you can install the chart you will need to add the `external-dns` repo to [Helm](https://helm.sh/).
+Before you can install the chart, you will need to add the `external-dns` repo to [Helm](https://helm.sh/).

This small change improves the flow of the sentence and makes it easier to read.

🧰 Tools

🪛 LanguageTool

[uncategorized] ~21-~21: Possible missing comma found.
Context: ...g the Chart Before you can install the chart you will need to add the external-dns...

(AI_HYDRA_LEO_MISSING_COMMA)

---

[typographical] ~27-~27: Consider inserting a comma for improved readability.
Context: ...ns/ After you've installed the repo you can install the chart. shell helm u...

(INITIAL_ADVP_COMMA)

---

33-54: Excellent provider information with minor typo

The providers section is well-structured and informative, offering users clear guidance on configuring ExternalDNS with various DNS providers. The table of providers with specific configuration support and the links to setup instructions for different providers are particularly helpful.

There's a minor typo in line 35 that should be corrected:

-Configuring the _ExternalDNS_ provider should be done via the `provider.name` value with provider specific configuration being set via the `provider.<name>.<key>` values, where supported, and the `extraArgs` value.
+Configuring the _ExternalDNS_ provider should be done via the `provider.name` value with provider-specific configuration being set via the `provider.<name>.<key>` values, where supported, and the `extraArgs` value.

This change adds a hyphen to "provider-specific" to correct the compound adjective.

The overall structure and content of this section are excellent and will be very useful for users setting up ExternalDNS with different providers.

🧰 Tools

🪛 LanguageTool

[uncategorized] ~35-~35: When ‘provider-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...done via the provider.name value with provider specific configuration being set via the `provid...

(SPECIFIC_HYPHEN)

---

[uncategorized] ~35-~35: Possible missing comma found.
Context: ..., and the extraArgs value. For legacy support provider can be set to the name of th...

(AI_HYDRA_LEO_MISSING_COMMA)

🪛 Markdownlint

48-48: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

49-49: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

50-50: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

51-51: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

52-52: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

53-53: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

54-54: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

56-88: Informative namespaced scope section with minor grammatical issue

This section provides valuable information about running ExternalDNS in a namespaced scope. The support matrix for different sources is particularly helpful for users to understand which features are available in this configuration.

There's a minor grammatical issue in line 59 that should be corrected:

-If `namespaced=true` is defined, the helm chart will setup `Roles` and `RoleBindings` instead `ClusterRoles` and `ClusterRoleBindings`.
+If `namespaced=true` is defined, the helm chart will set up `Roles` and `RoleBindings` instead of `ClusterRoles` and `ClusterRoleBindings`.

This change corrects the verb "setup" to "set up" and adds "of" before "ClusterRoles" for better readability.

Overall, this section provides crucial information for users considering a namespaced installation of ExternalDNS. The clear explanation of limitations and the support matrix are excellent additions to the documentation.

🧰 Tools

🪛 LanguageTool

[grammar] ~59-~59: The word “setup” is a noun. The verb is spelled with a space.
Context: ...d=trueis defined, the helm chart will setupRolesandRoleBindingsinsteadClu...

(NOUN_VERB_CONFUSION)

Configuration used: CodeRabbit UI
Review profile: CHILL

packages/extra/ingress/templates/dashboard.yaml (1)

1-35: Address trailing spaces throughout the file

The static analysis tool has reported multiple instances of trailing spaces in the file. While these don't affect functionality, removing them improves code cleanliness and consistency.

Consider running a linter or using an editor feature to remove trailing spaces throughout the file. This will improve code consistency and prevent potential issues with version control systems that are sensitive to whitespace changes.

Note: The syntax error reported on line 1 appears to be a false positive due to the Go templating syntax and can be ignored.

🧰 Tools

🪛 yamllint

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

(syntax)

---

[error] 2-2: trailing spaces

(trailing-spaces)

---

[error] 4-4: trailing spaces

(trailing-spaces)

---

[error] 5-5: trailing spaces

(trailing-spaces)

---

[error] 7-7: trailing spaces

(trailing-spaces)

---

[error] 8-8: trailing spaces

(trailing-spaces)

---

[error] 9-9: trailing spaces

(trailing-spaces)

---

[error] 10-10: trailing spaces

(trailing-spaces)

---

[error] 11-11: trailing spaces

(trailing-spaces)

---

[error] 12-12: trailing spaces

(trailing-spaces)

---

[error] 13-13: trailing spaces

(trailing-spaces)

---

[error] 14-14: trailing spaces

(trailing-spaces)

---

[error] 15-15: trailing spaces

(trailing-spaces)

---

[error] 16-16: trailing spaces

(trailing-spaces)

---

[error] 17-17: trailing spaces

(trailing-spaces)

---

[error] 18-18: trailing spaces

(trailing-spaces)

---

[error] 19-19: trailing spaces

(trailing-spaces)

---

[error] 20-20: trailing spaces

(trailing-spaces)

---

[error] 21-21: trailing spaces

(trailing-spaces)

---

[error] 22-22: trailing spaces

(trailing-spaces)

---

[error] 23-23: trailing spaces

(trailing-spaces)

---

[error] 24-24: trailing spaces

(trailing-spaces)

---

[error] 25-25: trailing spaces

(trailing-spaces)

---

[error] 26-26: trailing spaces

(trailing-spaces)

---

[error] 27-27: trailing spaces

(trailing-spaces)

---

[error] 28-28: trailing spaces

(trailing-spaces)

---

[error] 29-29: trailing spaces

(trailing-spaces)

---

[error] 30-30: trailing spaces

(trailing-spaces)

---

[error] 31-31: trailing spaces

(trailing-spaces)

---

[error] 32-32: trailing spaces

(trailing-spaces)

---

[error] 33-33: trailing spaces

(trailing-spaces)

---

[error] 34-34: trailing spaces

(trailing-spaces)

---

[error] 35-35: trailing spaces

(trailing-spaces)

packages/extra/monitoring/templates/alerta/alerta.yaml (1)

152-154: Approve changes with minor formatting suggestions

The conditional annotations for the Ingress resource are well-implemented, allowing for flexibility in using either HTTP01 or DNS01 challenges based on the $issuerType. This aligns perfectly with the PR objectives.

Consider removing the trailing spaces at the end of lines 152-154 for better code formatting:

-    {{- if ne $issuerType "cloudflare" }} 
-    acme.cert-manager.io/http01-ingress-class: {{ $ingress }}  
-    {{- end }} 
+    {{- if ne $issuerType "cloudflare" }}
+    acme.cert-manager.io/http01-ingress-class: {{ $ingress }}
+    {{- end }}

🧰 Tools

🪛 yamllint

[error] 152-152: trailing spaces

(trailing-spaces)

---

[error] 153-153: trailing spaces

(trailing-spaces)

---

[error] 154-154: trailing spaces

(trailing-spaces)

Configuration used: CodeRabbit UI
Review profile: CHILL

packages/core/platform/bundles/paas-hosted.yaml (1)

102-106: LGTM! Minor formatting fix needed.

The addition of the external-dns release is well-structured and aligns with the PR objectives to integrate External-DNS into the deployment bundles. The dependencies on cilium and kubeovn are appropriate, ensuring that the necessary networking components are set up before external-dns.

However, there's a minor formatting issue in the dependsOn field.

Please add a space after the comma in the dependsOn field for consistency with YAML best practices:

- dependsOn: [cilium,kubeovn]
+ dependsOn: [cilium, kubeovn]

🧰 Tools

🪛 yamllint

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

(commas)

Configuration used: CodeRabbit UI
Review profile: CHILL