zalando-stups / stups-etcd-cluster

Etcd cluster appliance for the STUPS (AWS) environment
Other
30 stars 9 forks source link

Serve metrics under a dedicated port #48

Closed linki closed 6 years ago

linki commented 6 years ago

etcd v3.3 introduced a new flag to allow serving /metrics and /health under a different port than e.g. /v2/keys. This allows us to protect etcd's data via firewall rules but still let monitoring tools to access the monitoring information.

See feature request in etcd repo: https://github.com/coreos/etcd/issues/8060. The implementation landed in v3.3: https://github.com/coreos/etcd/pull/8242

This PR instructs etcd to serve metrics and health under the additonal port 2381 unconditionally when the used etcd binary is >=v3.3.x. However, if not explicitely set in the senza.yaml this port won't be mapped to the outside and therefore isn't accessible. It doesn't expose more information than anything under 2379 already does. See our intended usage here: https://github.com/zalando-incubator/kubernetes-on-aws/pull/879.

Note: This fails when bundled with etcd lower than v3.3. Furthermore, serving this additional endpoint unconditionally should be safe but might not be desired. It keeps the implementation very simple, though. LMKWYT

/cc @CyberDem0n @aermakov-zalando @mikkeloscar

mikkeloscar commented 6 years ago

lgtm

linki commented 6 years ago

@CyberDem0n Thanks.

It now looks at the ETCDVERSION env variable and only adds the -listen-metrics-urls flag when the version is greater than or equal to v3.3.x

szuecs commented 6 years ago

@CyberDem0n can we get this merged?

szuecs commented 6 years ago

:+1:

CyberDem0n commented 6 years ago

:+1:

linki commented 6 years ago

@CyberDem0n let me quickly test on our cluster that it still works as expected before merging.

linki commented 6 years ago

@CyberDem0n ok, works with 3.3.

CyberDem0n commented 6 years ago

I need a second :+1:

linki commented 6 years ago

@CyberDem0n Did it work for you as well?

CyberDem0n commented 6 years ago

I didn't tried. In order to build image it needs to be merged and we need a second approval for that

mikkeloscar commented 6 years ago

:+1: