Skip to content
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

[kube-prometheus-stack] Use namespaceOverrides when set #1343

Merged
merged 5 commits into from
Sep 28, 2021

Conversation

sathieu
Copy link
Contributor

@sathieu sathieu commented Sep 14, 2021

What this PR does / why we need it:

Uses grafana.namespaceOverride, kube-state-metrics.namespaceOverride and prometheus-node-exporter.namespaceOverride when set, for created resource.

(includes #1342).

Checklist

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

@sathieu sathieu force-pushed the namespace_overrides branch 2 times, most recently from 74ab1f0 to c486c90 Compare September 16, 2021 10:21
Copy link
Contributor

@vsliouniaev vsliouniaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, noticed another potential issue with namespace selectors in serivcemonitors:

https://github.com/prometheus-community/helm-charts/pull/1343/files#diff-ef9695f03f05e41615990a96c6a839fd0858697075287a8be909cc1acc18509aR16-R20

Could you try testing this end-to-end with all possible namespace overrides set to different values and see if everything remains fully operational?

@sathieu
Copy link
Contributor Author

sathieu commented Sep 16, 2021

@vsliouniaev Actually, I have a problem when doing Helm template (from ArgoCD):

`helm template . [...]` failed exit status 1: Error: template: prometheus-stack/charts/kube-prometheus-stack/templates/_helpers.tpl:103:16: executing "grafana.namespace" at <.Values.grafana.namespaceOverride>: nil pointer evaluating interface {}.namespaceOverride Use --debug flag to render out invalid YAML

The CI has the same message.

Also, your link is not pointing to a specific file. Could you update it?

@sathieu sathieu force-pushed the namespace_overrides branch from c486c90 to 3a95578 Compare September 16, 2021 15:52
@sathieu
Copy link
Contributor Author

sathieu commented Sep 16, 2021

@vsliouniaev I found the cause: from the doc:

Recall how the labels on templates are globally shared. Thus, the labels chart can be included from any other chart.

... and defining a template twice leads to mixed scope.

To fix this, I removed the templates from kube-prometheus-stack, so that subchart's templates are used.

@sathieu sathieu force-pushed the namespace_overrides branch from 3a95578 to c41c7d4 Compare September 16, 2021 17:02
@sathieu
Copy link
Contributor Author

sathieu commented Sep 16, 2021

In can confirm that it works:

$ helm template  --set grafana.namespaceOverride=grafana --set kube-state-metrics.namespaceOverride=kube-state-metrics --set prometheus-node-exporter.namespaceOverride=prometheus-node-exporter .  | grep -A 5 'kind: ServiceMonitor'
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-alertmanager
  namespace: default
  labels:
    app: kube-prometheus-stack-alertmanager
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-coredns
  namespace: default
  labels:
    app: kube-prometheus-stack-coredns
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-apiserver
  namespace: default
  labels:
    app: kube-prometheus-stack-apiserver
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-kube-controller-manager
  namespace: default
  labels:
    app: kube-prometheus-stack-kube-controller-manager
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-kube-etcd
  namespace: default
  labels:
    app: kube-prometheus-stack-kube-etcd
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-kube-proxy
  namespace: default
  labels:
    app: kube-prometheus-stack-kube-proxy
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-kube-scheduler
  namespace: default
  labels:
    app: kube-prometheus-stack-kube-scheduler
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-kube-state-metrics
  namespace: kube-state-metrics
  labels:
    app: kube-prometheus-stack-kube-state-metrics
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-kubelet
  namespace: default
  labels:
    app: kube-prometheus-stack-kubelet
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-node-exporter
  namespace: prometheus-node-exporter
  labels:
    app: kube-prometheus-stack-node-exporter
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-grafana
  namespace: grafana
  labels:
    app: kube-prometheus-stack-grafana
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-operator
  namespace: default
  labels:
    app: kube-prometheus-stack-operator
--
kind: ServiceMonitor
metadata:
  name: RELEASE-NAME-kube-promethe-prometheus
  namespace: default
  labels:
    app: kube-prometheus-stack-prometheus

@vsliouniaev
Copy link
Contributor

@sathieu
Copy link
Contributor Author

sathieu commented Sep 17, 2021

thanks @vsliouniaev I've removed kubeStateMetrics.serviceMonitor.namespaceOverride and bumped major for this.

I've also fixed the prometheus Datasource.

Currently testing ...

@sathieu
Copy link
Contributor Author

sathieu commented Sep 17, 2021

At 4f92516, with grafana.namespaceOverride, kube-state-metrics.namespaceOverride and prometheus-node-exporter.namespaceOverride, I have tested:

  • ✔️ Prometheus Datasource is set-up and working
  • ✔️ Dashboards are created
  • ✔️ All tested dashboards have filled graphs, including
    • ✔️ Node Exporter / Nodes (prometheus-node-exporter.namespaceOverride)
  • However, I have the following dashboards empty:
    • 🔴 Alertmanager / Overview (serviceMonitor is unhealthy server returned HTTP status 404 Not Found, maybe because of istio)
    • Kubernetes / Proxy (serviceMonitor is unhealthy server returned HTTP status 503 Service Unavailable) fixed (this was a Kubespray config)
    • 🟠 etcd (expected, my etcd install is not using containers)

@sathieu
Copy link
Contributor Author

sathieu commented Sep 21, 2021

@vsliouniaev Please review again.

NB: I've rebased to resolve a merge conflict.

@vsliouniaev
Copy link
Contributor

From the code, I can see there is an inconsistency in the namespaces in charts/kube-prometheus-stack/templates/exporters/node-exporter/servicemonitor.yaml.

@sathieu sathieu force-pushed the namespace_overrides branch from 6077313 to 767b6a5 Compare September 23, 2021 08:54
@sathieu
Copy link
Contributor Author

sathieu commented Sep 23, 2021

@vsliouniaev Thanks. Fixed. back to you 🏓 !

@vsliouniaev
Copy link
Contributor

This generally looks like it's working. Not sure what the use-case is for such an added complication to the chart.

Signed-off-by: Mathieu Parent <math.parent@gmail.com>
… set

Signed-off-by: Mathieu Parent <math.parent@gmail.com>
…e when set

Signed-off-by: Mathieu Parent <math.parent@gmail.com>
Signed-off-by: Mathieu Parent <math.parent@gmail.com>
…aceOverride

Signed-off-by: Mathieu Parent <math.parent@gmail.com>
@sathieu sathieu force-pushed the namespace_overrides branch from 767b6a5 to be48f7f Compare September 28, 2021 09:32
@sathieu
Copy link
Contributor Author

sathieu commented Sep 28, 2021

@vsliouniaev I think it's a good idea to separate the components in different namespaces. Namespaces are security boundaries in k8s, and usage of network policies or roles is easier. Monitoring is also easier.

Also, I'm not the only one using namespace overrides (See older bugs #984, #820), this PR fixes all remaining problems about nsOverrides.

Note: I've resolved conflict and force-pushed.

@vsliouniaev vsliouniaev merged commit 12699d2 into prometheus-community:main Sep 28, 2021
@sathieu sathieu deleted the namespace_overrides branch September 28, 2021 11:57
@sathieu
Copy link
Contributor Author

sathieu commented Sep 28, 2021

Thanks @vsliouniaev !

@james-callahan
Copy link

james-callahan commented Sep 29, 2021

This breaks namespace independence.
namespaceSelector shouldn't be set unless it's pointing at a different namespace

Created #1382

QuentinBisson pushed a commit to giantswarm/prometheus-community-helm-charts-upstream that referenced this pull request Oct 5, 2021
…ommunity#1343)

* [kube-prometheus-stack] Use grafana.namespaceOverride when set

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

* [kube-prometheus-stack] Use kube-state-metrics.namespaceOverride when set

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

* [kube-prometheus-stack] Use prometheus-node-exporter.namespaceOverride when set

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

* [kube-prometheus-stack] Specify namespace for Prometheus datasource

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

* [kube-prometheus-stack] Remove kubeStateMetrics.serviceMonitor.namespaceOverride

Signed-off-by: Mathieu Parent <math.parent@gmail.com>
Signed-off-by: QuentinBisson <quentin@giantswarm.io>
stamzid pushed a commit to Unstructured-IO/prometheus-community-helm-charts that referenced this pull request Mar 3, 2023
…ommunity#1343)

* [kube-prometheus-stack] Use grafana.namespaceOverride when set

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

* [kube-prometheus-stack] Use kube-state-metrics.namespaceOverride when set

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

* [kube-prometheus-stack] Use prometheus-node-exporter.namespaceOverride when set

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

* [kube-prometheus-stack] Specify namespace for Prometheus datasource

Signed-off-by: Mathieu Parent <math.parent@gmail.com>

* [kube-prometheus-stack] Remove kubeStateMetrics.serviceMonitor.namespaceOverride

Signed-off-by: Mathieu Parent <math.parent@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants