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

[xogoodnow]

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

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

[QuentinBisson]

The helm chart prefers to enable secure settings per default and port 2381 is etcd TLS port that is used to fetch metrics. It is possible that your clusters are either old or not providing a secure access to etcd in which case you need to configure the port to 2379 but that should only be the case for your clusters.

[GMartinez-Sisti]

Hi @xogoodnow, thank you for the contribution. Please check #2264 regarding the ETCD ports.

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

[xogoodnow]

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]

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

[zeritti]

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.

[GMartinez-Sisti]

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.

[stale]

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.

[xogoodnow]

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?

[jkroepke]

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.

[GMartinez-Sisti]

These shouldn't be here. Can you please remove them?

[GMartinez-Sisti]

As @jkroepke said, please do:

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.

[stale]

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.

[xogoodnow]

Hi
Should something be done on my end?

[jkroepke]

Hi Should something be done on my end?

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

[xogoodnow]

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

[jkroepke]

Hi @xogoodnow

currently, I'm missing everything what is requested.

[stale]

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.