redpanda-data / redpanda-connect-helm-chart

Helm 3 repository for benthosdev/benthos
MIT License
36 stars 26 forks source link

ServiceMonitor not working when tls/auth is enabled #74

Closed tbnguyen1407 closed 9 months ago

tbnguyen1407 commented 9 months ago

Problem

When tls/auth is enabled, ServiceMonitor is not working due to missing scheme: https and basicAuth config for endpoint.

Reproduction

Deploy with custom values file

~ values.yaml ~
http:
  enabled: true
  cert_file: /etc/tls/benthos/cert.pem
  key_file: /etc/tls/benthos/key.pem
  basic_auth:
    username: <username>
    password_hash: <password_hash>

extraVolumes:
  - name: benthos-tls-vol
    secret:
      secretName: benthos-tls-secret

extraVolumeMounts:
  - name: benthos-tls-vol
    mountPath: /etc/tls/benthos

serviceMonitor:
  enabled: true

Notes

ServiceMonitor resource endpoint scheme and basicAuth should be configurable.

tbnguyen1407 commented 9 months ago

Hello, I checked latest chart. Support for tls/auth in ServiceMonitor still has problems:

BasicAuth

basicAuth should be set at endpoint level, not ServiceMonitor.spec. Latest chart .Values.serviceMonitor.basicAuth config is ignored as it is set at wrong level.

TLS

tlsConfig is also required for endpoint as most certs do not include IP in SANs and hence cert validation must be skipped.

Error from Prometheus:

Get "https://10.42.0.13:4195/metrics": tls: failed to verify certificate: x509: cannot validate certificate for 10.42.0.13 because it doesn't contain any IP SANs

Proposed fix

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
spec:
- {{- with .Values.serviceMonitor.basicAuth }}
- basicAuth:
-   {{- toYaml . | nindent 4 }}
- {{- end }}
  endpoints:
    - interval: {{ .Values.serviceMonitor.interval}}
      targetPort: http
      path: /metrics
      scheme: {{ .Values.serviceMonitor.scheme }}
+     {{- with .Values.serviceMonitor.basicAuth }}
+     basicAuth:
+       {{- toYaml . | nindent 8 }}
+     {{- end }}
+     {{- with .Values.serviceMonitor.tlsConfig }}
+     tlsConfig:
+       {{- toYaml . | nindent 8 }}
+     {{- end }}
charlie-haley commented 9 months ago

I've released a fix for this as part of 2.1.1, I've also raised an issue to add schema linting so this can be better caught in the future.