Repository navigation
fix/rbac: Fix Prometheus' defaults, privileged: false, and scope discovery down to the same namespace - #1015
Draft
marcleblanc2 wants to merge 7 commits into
Draft
marcleblanc2 wants to merge 7 commits into
marcleblanc2 wants to merge 7 commits into
Conversation
This was referenced Nov 13, 2026
…overy to the release namespace A default install granted the prometheus ServiceAccount cluster-wide read access, including nodes/proxy, to run two scrape jobs whose metrics no Sourcegraph dashboard or alert uses. It now creates only a RoleBinding to the built-in view ClusterRole in the release namespace. The kubernetes-pods and kubernetes-services jobs listed pods and services in every namespace, which the view RoleBinding does not allow, so with privileged=false they failed and pod-annotated targets such as cadvisor were never scraped. They now list only the release namespace, as kubernetes-service-endpoints already did. Amp-Thread-ID: https://ampcode.com/threads/T-01a1d140-e6de-7173-8b4f-187543b7833c Co-authored-by: Amp <amp@ampcode.com>
Neither change needs an admin to act during an upgrade: Helm swaps the RBAC resources itself, and the chart's own scrape targets are all in the release namespace.
Executors send their metrics to frontend with each heartbeat, and prometheus scrapes them from the worker-executors Service in the release namespace. The OpenShift example said prometheus.privileged: true was needed to scrape executors in another namespace; correct it, and say so next to prometheus.privileged in values.yaml. Amp-Thread-ID: https://ampcode.com/threads/T-01a1d140-e6de-7173-8b4f-187543b7833c Co-authored-by: Amp <amp@ampcode.com>
Prometheus does not reload a changed ConfigMap by itself, so an upgrade that keeps the image tag swapped the RBAC resources while the pod kept running the old scrape configuration. The annotation, its test, and the changelog entry are the same as in #1008.
…ten the tests An unquoted namespace named `null` rendered as a YAML null in the discovery configuration. The tests now assert the bindings' subjects and roleRef, the exact indentation of the namespace lists, and that kubernetes-apiservers stays cluster-wide, so these broken charts no longer pass: `namespaces` at the wrong indentation, a subject in another namespace, roleRef.kind: Role, and kubernetes-apiservers scoped to the release namespace.
…alling nodes/proxy read access The k3s example's config discovers targets in every namespace, so it needs prometheus.privileged: true now that the default is false. Also fixes the existingConfig description, which named pgsql.
marcleblanc2
force-pushed
the
marc/prometheus-namespace-scope
branch
from
November 13, 2026 06:16
4bc85aa to
77776fc
Compare
This was referenced Nov 13, 2026
marcleblanc2
marked this pull request as draft
November 13, 2026 10:13
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Open issues
Found in review on 2026-11-13 (review thread), at
77776fc. The change works as described; this PR is a draft until the changelog covers the first two.prometheus.existingConfigset andprivilegedunset: reproduced on EKS with the repo's k3s example config. After the upgradeprometheushad 0 Kubernetes targets, its log had noforbiddenlines, andhelm upgradeprinted nothing. Addingprometheus.privileged: truebrought the targets backprometheus.createRoleBinding: falseandprivilegedunset, with the chart's ClusterRole bound by hand: the upgrade deletes the ClusterRole. Seen inhelm templateonlyprometheus.privileged: truebefore upgrading. TheBREAKINGlabel was removed on purpose inddfa59a, becauseprometheus.existingConfigis rarely used, and stays offkubernetes-servicesremoval has no changelog entry here, and the k3s example still contains the job. Only fix/resources: Drop unused ports, Services, Role verbs, and settings #1016 has both. Fix: merge fix/resources: Drop unused ports, Services, Role verbs, and settings #1016 first, or copy its changelog line and example changeexamples/openshift/override.yamlstill describes the ClusterRole withoutnodes/proxyTested in this review on another EKS test instance, by upgrading from
mainto this branch merged ontomain, both with chart-defaultprometheus.privileged:view, and theprometheuspod was replaced oncekubernetes-apiserversandkubernetes-nodeswent away, the service endpoint targets stayed at 23, and there were 0 discovery failureskubectl auth can-ias the ServiceAccount matched "Test results" below, and also says no toget nodes/proxyProblems
Our current default Prometheus config:
kubernetes-apiserversandkubernetes-nodesjobs were 74% of all series (157,910 of 212,150)0936a4f)nodes,nodes/proxy,nodes/metrics,pods,services,endpoints, andnamespaces, cluster-widegetonnodes/proxyalso authorizes running commands in any container on the nodeprometheus.privileged: falsewas broken for pod targets:kubernetes-podsjob lists pods in all namespaces, which the RoleBinding toviewdoesn't allow, socadvisorwas never scrapedSolution
prometheus.privilegeddefaults tofalse: no cluster-scoped RBAC resource, and nokubernetes-apiserversorkubernetes-nodesjobkubernetes-podsjob discovers pods only in the release namespace, withprivileged: truetookubernetes-servicesjob is removed, as in fix/resources: Drop unused ports, Services, Role verbs, and settings #1016: it probes through ablackboxexporter the chart doesn't shipprometheusrestarts when an upgrade changes the chart'sprometheus.yml, as in Render sourcegraph.labels on every object, and add storage and ServiceAccount labels #1008, instead of keeping the old config under the new RBACprometheus.privilegedinstall warning is removed, and the docs now say whatprivilegedandcreateRoleBindingdo, and that executor metrics don't needprivileged: trueRBAC resources rendered for
prometheus:viewprivileged: trueprivileged: falseviewcreateRoleBinding: falseprivileged: true,createRoleBinding: falseOn upgrade:
prometheus.privileged: trueto keep the ClusterRole, the ClusterRoleBinding, and the two scrape jobs. Sourcegraph's dashboards and alerts don't need it; your own do if they use kubelet or API server metrics, such askubelet_volume_stats_available_bytessourcegraph.prometheus/scrape: "true"are no longer scraped, withprivileged: truetoo. The chart has no value that adds namespaces; scraping them takes your ownprometheus.yml, throughprometheus.existingConfigcadvisorandnode-exporterrun in the release namespace and are still scrapedprometheus.existingConfigthat discovers targets outside the release namespace, or scrapes nodes or API servers, needsprometheus.privileged: trueTest results
helm lintandhelm unittestpass (267 tests), andhelm templateonmainand on this branch renders the table aboveOn an EKS test instance,
main(643f7c0) and this branch (77776fc) were installed in turn withhelm upgrade, with the same values and images,cadvisorandjaegerenabled, and one annotated pod in a second namespace.Targets found by a new
prometheuspod:cadvisorjaegermainmainprivileged: falseprivileged: falseprivileged: trueprivileged: true,createRoleBinding: falsecreateRoleBinding: falsecadvisorruns one pod per nodejaegeron both charts: its annotated port 16686 returns 404 for/metrics, and fix/resources: Drop unused ports, Services, Role verbs, and settings #1016 removes the annotationcreateRoleBinding: falserows, binding the ServiceAccount by hand brought the targets back without a restart: a ClusterRoleBinding to the chart's ClusterRole gave theprivileged: truerow's targets, and a RoleBinding toviewgave the default row'skubectl auth can-ias the ServiceAccount, with this PR's default:view, getpods/logand list configmapsnodes/proxy,nodes/metrics, namespaces, and/metricsprivileged: trueit answers as it does onmain's default: yes to each of those cluster-wide requests, and no to secrets,pods/log, and list configmapsUpgrading from
mainto this PR, both with default values:helm difflisted theprometheusConfigMap and Deployment as changed, the ClusterRole and ClusterRoleBinding as removed, the RoleBinding as added, and nothing elseprometheus, and it loaded the new config 24 seconds afterhelm upgradestartedprometheusstartedkubernetes-apiservers(134,489) andkubernetes-nodes(23,421) were removedbuiltin-alertmanager, and the 383 series without ajoblabel kept the same set of seriesbuiltin-prometheuslost 90 series, about the removed jobs and their discovery, and gained 46, about API requests the test madekubernetes-podslost the 528 series of the pod in the second namespacecadvisorseries about things that changed on the nodes during the test: the replacedprometheuspod, host network interfaces that came and went (22 of the 23 new ones belong to pods another release started during the test), and 4 filesystem series for one disk, whose cause was not provenmain. On this PR none gained gaps, and 21 returned a different set of series:List, which Zoekt first reported at the end of themainmeasurementtopkpanel also differed between twoprometheuspods with the samemainconfigmain's data without the two removed jobs changed those 2 panels and no othersOther upgrades:
privileged: falsegave an emptyhelm diffand kept the pod; settingprivileged: truereplaced itcreateRoleBinding: falsetoprivileged: trueremoved only the ClusterRoleBinding and kept the pod. Its API server and node scrapes then returned 403, and itscadvisorand service endpoint targets stayed upmainwithprivileged: falseto this PR withprivileged: false, the pod was replaced, andcadvisorwent from 0 to 9 targetsmain, changingprivilegedchanged the ConfigMap and the RBAC resources but not the Deployment, soprometheuskept running the old configAlso seen on
main: at one moment before these measurements, its default config was scraping the 10cadvisorpods of a second Sourcegraph release in the same cluster, 17,318 more series. Leaving those series out changed 17 expressions; for example, the gitserver CPU panel read 2.0 times as high with them. This PR found no pod outside the release namespace in any row.Executor metrics, checked on two other test instances, where
prometheushas only the RoleBinding toviewand itskubernetes-service-endpointsjob lists only the release namespace:prometheushas no target at its addressworker-executorsService in the release namespace, and so do those of an executor on a machine outside the clusterNot tested
node-exporter, which was disabled on the test instancemainwithcreateRoleBinding: falseprometheus.existingConfig, in this test; "Open issues" has the review's resultLinked PRs
prometheus.privileged: truein the two tests that expect theprometheusClusterRole and ClusterRoleBinding in the default render, inallObjectLabels_test.yamlandobjectLabels_test.yamlAmp threads
Changelog
prometheus.privilegednow defaults tofalsekubernetes-podsscrape job now discovers pods only in the release namespaceprometheuspod now restarts when an upgrade changes the chart'sprometheus.yml