-
Notifications
You must be signed in to change notification settings - Fork 4
Sync kubex charts from automation-controller main @ f2541e4 #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,12 +119,22 @@ Controller manager container args | |
| - --health-probe-bind-address={{ .Values.controllerManager.healthProbeBindAddress }} | ||
| {{- if .Values.metrics.enabled }} | ||
| - --metrics-bind-address={{ .Values.controllerManager.metricsBindAddress }} | ||
| {{- include "kubex-automation-engine.metricsSecureArg" . }} | ||
| {{- end }} | ||
| {{- range .Values.controllerManager.extraArgs }} | ||
| - {{ . }} | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{/* | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CONTENT OF THIS REVIEW IS AI GENERATED [Severity: Major] [Confidence: High] Location: Issue: Why it matters: The security posture of the metrics endpoint is silently degraded to unauthenticated HTTP any time Suggested fix: Introduce a dedicated, explicit -{{- if and .Values.metrics.enabled (eq .Values.metrics.serviceMonitor.scheme "http") }}
+{{- if and .Values.metrics.enabled .Values.metrics.serviceMonitor.enabled (eq .Values.metrics.serviceMonitor.scheme "http") }} |
||
| Controller metrics security flag. | ||
| */}} | ||
| {{- define "kubex-automation-engine.metricsSecureArg" -}} | ||
| {{- if and .Values.metrics.enabled (eq .Values.metrics.serviceMonitor.scheme "http") }} | ||
| - --metrics-secure=false | ||
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{/* | ||
| Generate or retrieve self-signed certificates for webhook | ||
| Returns a dict with ca, cert, and key | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| {{- if and .Values.metrics.enabled .Values.metrics.serviceMonitor.enabled -}} | ||
| apiVersion: monitoring.coreos.com/v1 | ||
| kind: ServiceMonitor | ||
| metadata: | ||
| name: {{ include "kubex-automation-engine.fullname" . }}-metrics | ||
| namespace: {{ include "kubex-automation-engine.namespace" . }} | ||
| labels: | ||
| {{- include "kubex-automation-engine.labels" . | nindent 4 }} | ||
| {{- with .Values.metrics.serviceMonitor.additionalLabels }} | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| spec: | ||
| {{- with .Values.metrics.serviceMonitor.namespaceSelector }} | ||
| namespaceSelector: | ||
| matchNames: | ||
| {{- toYaml . | nindent 6 }} | ||
| {{- else }} | ||
| namespaceSelector: | ||
| any: true | ||
| {{- end }} | ||
| selector: | ||
| matchLabels: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CONTENT OF THIS REVIEW IS AI GENERATED [Severity: Minor] [Confidence: High] Location: Issue: The Why it matters: Pod selector labels (used for workload matching) are appropriate for Deployments, but ServiceMonitor selectors should typically only match stable, non-version labels. Version churn during upgrades silently breaks scraping until the reconciler catches up. Suggested fix: Consider using a subset of labels that excludes |
||
| {{- include "kubex-automation-engine.selectorLabels" . | nindent 6 }} | ||
| endpoints: | ||
| - port: metrics | ||
| path: {{ .Values.metrics.serviceMonitor.path }} | ||
| scheme: {{ .Values.metrics.serviceMonitor.scheme }} | ||
| # tlsConfig: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CONTENT OF THIS REVIEW IS AI GENERATED [Severity: Minor] [Confidence: High] Location: Issue: Commented-out TLS config blocks ( Why it matters: If TLS scraping is a future-planned feature, this is better tracked as a GitHub issue or a Suggested fix: Remove the commented-out block entirely. If HTTPS scraping is intended to be supported, add a |
||
| # caFile: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt | ||
| # insecureSkipVerify: true | ||
| # bearerTokenFile: /var/run/secrets/kubernetes.io/serviceaccount/token | ||
| {{- with .Values.metrics.serviceMonitor.interval }} | ||
| interval: {{ . }} | ||
| {{- end }} | ||
| {{- with .Values.metrics.serviceMonitor.scrapeTimeout }} | ||
| scrapeTimeout: {{ . }} | ||
| {{- end }} | ||
| {{- end }} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,6 +104,53 @@ | |
| } | ||
| } | ||
| }, | ||
| "metrics": { | ||
| "type": "object", | ||
| "properties": { | ||
| "enabled": { | ||
| "type": "boolean" | ||
| }, | ||
| "type": { | ||
| "type": "string" | ||
| }, | ||
| "port": { | ||
| "type": "integer" | ||
| }, | ||
| "annotations": { | ||
| "type": "object" | ||
| }, | ||
| "serviceMonitor": { | ||
| "type": "object", | ||
| "properties": { | ||
| "enabled": { | ||
| "type": "boolean" | ||
| }, | ||
| "additionalLabels": { | ||
| "type": "object" | ||
| }, | ||
| "namespaceSelector": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "scheme": { | ||
| "type": "string", | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CONTENT OF THIS REVIEW IS AI GENERATED [Severity: Minor] [Confidence: High] Location: Issue: The JSON schema Why it matters: Schema validation is the last line of defence against silent misconfiguration. Without Suggested fix: Add |
||
| "enum": ["http", "https"] | ||
| }, | ||
| "path": { | ||
| "type": "string" | ||
| }, | ||
| "interval": { | ||
| "type": "string" | ||
| }, | ||
| "scrapeTimeout": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "openshift": { | ||
| "type": "object", | ||
| "properties": { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,7 +126,7 @@ controllerManager: | |
| healthProbeBindAddress: ":8081" | ||
|
|
||
| # -- Metrics bind address | ||
| metricsBindAddress: ":8443" | ||
| metricsBindAddress: ":8080" | ||
|
|
||
| # -- Additional arguments for the controller manager | ||
| extraArgs: | ||
|
|
@@ -180,9 +180,30 @@ metrics: | |
| # -- Service type for metrics | ||
| type: ClusterIP | ||
| # -- Service port for metrics | ||
| port: 8443 | ||
| port: 8080 | ||
| # -- Service annotations | ||
| annotations: {} | ||
| annotations: | ||
| # Controller metrics are served over plain HTTP without authentication in this chart; the manager args set --metrics-secure=false when this scheme is used. | ||
| prometheus.io/scrape: "true" | ||
| prometheus.io/port: '{{ .Values.metrics.port }}' | ||
| prometheus.io/path: '{{ .Values.metrics.serviceMonitor.path }}' | ||
| prometheus.io/scheme: '{{ .Values.metrics.serviceMonitor.scheme }}' | ||
| # -- Optional ServiceMonitor configuration for Prometheus Operator | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CONTENT OF THIS REVIEW IS AI GENERATED [Severity: Major] [Confidence: High] Location: Issue: The default Why it matters: Using Suggested fix: If templated annotations are really required, document this contract clearly in the values comments, and consider using a named helper with a specific prefix convention instead of templating the entire annotations block. Alternatively, move the templated annotations out of the default |
||
| serviceMonitor: | ||
| # -- Create a ServiceMonitor for the metrics Service | ||
| enabled: false | ||
| # -- Additional labels applied to the ServiceMonitor | ||
| additionalLabels: {} | ||
| # -- Namespace names to discover for the ServiceMonitor. Leave empty to watch all namespaces. | ||
| namespaceSelector: [] | ||
| # -- Scrape scheme used by Prometheus | ||
| scheme: http | ||
| # -- Metrics endpoint path | ||
| path: /metrics | ||
| # -- Optional scrape interval | ||
| interval: "" | ||
| # -- Optional scrape timeout. Must be <= interval when interval is set. | ||
| scrapeTimeout: "" | ||
|
|
||
| # Webhook configuration | ||
| webhook: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONTENT OF THIS REVIEW IS AI GENERATED
[Severity: Minor] [Confidence: Medium]
Location:
charts/kubex-automation-engine/docs/Configuration-Reference.md:119Issue: The documentation table documents several new
metrics.serviceMonitor.*fields but is missingmetrics.serviceMonitor.interval,metrics.serviceMonitor.additionalLabels,metrics.serviceMonitor.path, andmetrics.serviceMonitor.enabledis only listed once without the co-related guard condition (metrics.enabledmust also be true).Why it matters: Incomplete documentation is a usability issue — operators scanning the reference table cannot discover all available knobs, particularly for the
intervalandpathvalues that affect scrape behaviour.Suggested fix: Add rows for
metrics.serviceMonitor.interval("") andmetrics.serviceMonitor.additionalLabels({}),metrics.serviceMonitor.path(/metrics), and update themetrics.serviceMonitor.enabledrow description to note thatmetrics.enabledmust also betrue.