helm-charts icon indicating copy to clipboard operation
helm-charts copied to clipboard

[kube-prometheus-stack] Default port on which ETCD exposes its metrics

Open xogoodnow opened this issue 2 years ago • 14 comments

By default ETCD exposes its metrics on port 2379 and not 2381

What this PR does / why we need it

By default ETCD exposes its metrics on port 2379 and not 2381

Which issue this PR fixes

No service (by default) is listening on port 2381 on the cluster, so after installing the chart, the metrics for etcd would not be monitored unless the port in values.yaml (or ETCD configuration) is changed. (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes # Changing eh default port for etcd metrics from 2381 to 2379 in the following path kubeEtcd.service. port AND kubeEtcd.service. targetPort

Special notes for your reviewer

Checklist

  • [x] DCO signed
  • [x] Chart Version bumped
  • [x] Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

xogoodnow avatar Aug 25 '23 13:08 xogoodnow

Hi @xogoodnow, thank you for the contribution. Please check https://github.com/prometheus-community/helm-charts/pull/2264 regarding the ETCD ports.

TLDR: https://github.com/kubernetes/kubernetes/blob/8b132ea40a250e9acf08ab2fadb9a8c2fd811d74/cmd/kubeadm/app/constants/constants.go#L95-L96

GMartinez-Sisti avatar Aug 25 '23 13:08 GMartinez-Sisti

Hi @QuentinBisson, The version of my cluster at the moment is 1.28 which is the latest. as for the security concern, by default etcd exposes its metrics on 2379 and in the official docs of etcd itself there are no mentions of any security risk for doing this. https://etcd.io/docs/v3.1/op-guide/monitoring/ There is no mention of security risk of doing this anywhere (since it uses the certificates) Also I believe the default value must be compatible with default configuration of other services

xogoodnow avatar Aug 25 '23 13:08 xogoodnow

Hi @GMartinez-Sisti , Just deploy the latest version of the cluster and install the chart, it does not work. I just tested it on our stage and it did not work. (if you have the time I recommend you just test this with the latest version of k8s 1.28 and see if it works by default) cheers

xogoodnow avatar Aug 25 '23 13:08 xogoodnow

My impression is that introducing 2381 as an Etcd metrics port in the chart was based on a specific internal change in kubeadm, judging from the link to its constants above and in the original PR. From Etcd metrics-endpoint

Each etcd server exports metrics under the /metrics path on its client port and optionally on locations given by --listen-metrics-urls.

The client port is indeed 2379/tcp and supposed to always be protected by TLS.

kubeadm introduced port 2381 in a change to access the Etcd's /health endpoint for health checks/liveness probes without TLS replacing thus the previous health check implementation. It makes use of the Etcd flag above that can enable additional URLs serving /metrics and /health. Furthermore, as the health check is insecure, it opens the port at localhost - it is not supposed to be accessed from outside.

The changes in kubeadm have no effect on how Etcd primarily serves its metrics: through the client port.

zeritti avatar Aug 30 '23 21:08 zeritti

IMHO if we decide to go ahead and change the port, it should be considered a breaking change to ensure we don't break existing installations.

GMartinez-Sisti avatar Aug 31 '23 08:08 GMartinez-Sisti

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

stale[bot] avatar Oct 15 '23 08:10 stale[bot]

Hi, considering that this would be a breaking change, do we have any estimation on how and when to make these changes on the chart?

xogoodnow avatar Oct 16 '23 07:10 xogoodnow

Bump the helm major chart version + upgrade notes on the README.md is the way to go for an breaking change and needs to be part of the PR itself.

jkroepke avatar Oct 16 '23 16:10 jkroepke

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

stale[bot] avatar Mar 13 '24 10:03 stale[bot]

Hi Should something be done on my end?

xogoodnow avatar Mar 16 '24 09:03 xogoodnow

Hi Should something be done on my end?

Yes, please take note of the review comment from @GMartinez-Sisti + resolve MR conflicts

jkroepke avatar Mar 16 '24 14:03 jkroepke

hi @jkroepke, the changes have been made Please check and if there is something that I must do , let me know

xogoodnow avatar Mar 17 '24 19:03 xogoodnow

Hi @xogoodnow

currently, I'm missing everything what is requested.

jkroepke avatar Mar 17 '24 20:03 jkroepke

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

stale[bot] avatar Apr 22 '24 09:04 stale[bot]