fix/rbac: Fix Prometheus' defaults, privileged: false, and scope discovery down to the same namespace

charts/sourcegraph/CHANGELOG.md

@@ -8,6 +8,9 @@ Use `**BREAKING**:` to denote a breaking change
 
 ## Unreleased
 
+- `prometheus.privileged` now defaults to `false`. A default install creates one RoleBinding from the `prometheus` ServiceAccount to the built-in `view` ClusterRole in the release namespace, instead of a ClusterRole and ClusterRoleBinding that grant cluster-wide permissions, including `nodes/proxy`. Prometheus no longer runs the `kubernetes-apiservers` and `kubernetes-nodes` scrape jobs, whose metrics no Sourcegraph dashboard or alert uses. Upgrading a release that does not set `prometheus.privileged` deletes the `prometheus` ClusterRole and ClusterRoleBinding and creates the RoleBinding; set `prometheus.privileged: true` to keep the previous RBAC resources and scrape jobs.
+- The `kubernetes-pods` Prometheus scrape job now discovers targets only in the release namespace, as `kubernetes-service-endpoints` already did, including when `prometheus.privileged` is `true`. Pods annotated `sourcegraph.prometheus/scrape: "true"` in other namespaces are no longer scraped. With `prometheus.privileged: false`, this job previously failed to list pods cluster-wide, so pod-annotated targets such as `cadvisor` were not scraped; they are now. The `prometheus.privileged` warning is removed from the install notes.
+- The `prometheus` pod now restarts when an upgrade changes the chart's `prometheus.yml`, through a `checksum/prometheus-config` pod annotation. Prometheus does not reload a changed ConfigMap by itself, so such a change took effect only at the pod's next restart. The annotation is not set with `prometheus.existingConfig`.
 - The `correct-data-dir-permissions` init containers of `pgsql`, `codeintel-db`, and `codeinsights-db` now run the database's own image instead of `sourcegraph/alpine-3.14`, which was based on an end-of-life Alpine release. The chart no longer pulls `alpine-3.14`, so private registries no longer need to mirror it. `alpine.image` has been removed and is ignored if set; `alpine.containerSecurityContext`, `alpine.resources`, and `codeInsightsDB.init.containerSecurityContext` still apply.
 - Added `sourcegraph.selectorLabelsOverride`, `<serviceName>.extraSelectorLabels`, and `storageLabels` on `gitserver`, `grafana`, `indexedSearch`, and `searcher`, so the chart can render the selectors and volume claim templates of workloads created outside it, such as with Kustomize, and a Helm release can take them over without deleting them. All default to empty, so existing releases render the same selectors. Never change them on an installed release: Kubernetes does not allow changing these fields. See [Adopting existing workloads](README.md#adopting-existing-workloads).
 - Fixed `frontend.command` and `migrator.command` rendering a list as a single element, e.g. `["sh", "-c", "<script>"]` became `[sh -c <script>]`, so the container failed to start. Both are now rendered as YAML lists and documented in `values.yaml`. A string holding a YAML list, the only form that worked before, still renders unchanged.

charts/sourcegraph/README.md

@@ -395,14 +395,14 @@ In addition to the documented values, all services also support the following va
 | preciseCodeIntel.storageType | string | `"emptyDir"` | Backing store for the `/tmp` scratch volume, used for large SCIP upload processing. One of: emptyDir, pvc. emptyDir: node ephemeral storage, i.e. the node boot disk. pvc: per-pod PVC, discarded with the pod. The pod `fsGroup` defaults to the container `runAsGroup` so the non-root worker can write to the volume; set `podSecurityContext.fsGroup` to override it. |
 | priorityClasses | list | `[]` | Additional priorityClasses minimize re-scheduling downtime for StatefulSets. Each StatefulSets might use different priority class. learn more from the [Kubernetes documentation](https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#priorityclass) Sample class definition: - name: gitserver-class   value: 100   preemptionPolicy: Never   description: "gitserver priority class" |
 | prometheus.containerSecurityContext | object | `{"allowPrivilegeEscalation":false,"readOnlyRootFilesystem":false,"runAsGroup":100,"runAsUser":100}` | Security context for the `prometheus` container, learn more from the [Kubernetes documentation](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-container) |
-| prometheus.createRoleBinding | bool | `true` | Disable the creation of a RoleBinding object, for customers who block all RBAC resource creation |
+| prometheus.createRoleBinding | bool | `true` | Create the binding for the `prometheus` ServiceAccount: a RoleBinding, or a ClusterRoleBinding if `privileged` is `true`. Set to `false` with `privileged: false` to create no RBAC resources for `prometheus`, for clusters that block RBAC resource creation; then bind the ServiceAccount to a Role that can get, list, and watch `pods`, `services`, and `endpoints` in the release namespace |
 | prometheus.enabled | bool | `true` | Enable `prometheus` (recommended) |
-| prometheus.existingConfig | string | `""` | Name of existing ConfigMap for `pgsql`. It must contain a `prometheus.yml` key |
+| prometheus.existingConfig | string | `""` | Name of existing ConfigMap for `prometheus`. It must contain a `prometheus.yml` key. Set `privileged: true` if its config discovers targets outside the release namespace, or scrapes nodes or API servers |
 | prometheus.image.defaultTag | string | `<appVersion>@sha256:<digest>`, pinned by each release | Docker image tag for the `prometheus` image |
 | prometheus.image.name | string | `"prometheus"` | Docker image name for the `prometheus` image |
 | prometheus.name | string | `"prometheus"` | Name used by resources. Does not affect service names or PVCs. |
 | prometheus.podSecurityContext | object | `{"fsGroup":100,"fsGroupChangePolicy":"OnRootMismatch"}` | Security context for the `prometheus` pod, learn more from the [Kubernetes documentation](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-pod) |
-| prometheus.privileged | bool | `true` | Enable RBAC for `prometheus` |
+| prometheus.privileged | bool | `false` | Grant `prometheus` cluster-wide permissions and scrape the Kubernetes API servers and nodes. `true` creates a ClusterRole, binds it with a ClusterRoleBinding, and adds the `kubernetes-apiservers` and `kubernetes-nodes` scrape jobs. `false` creates only a RoleBinding to the built-in `view` ClusterRole in the release namespace, which is all `prometheus` needs to discover and scrape Sourcegraph's own pods and services. Executor metrics do not need `true`, wherever the executors run: executors send their metrics to `frontend` with each heartbeat, and `prometheus` scrapes them from the `worker-executors` Service in the release namespace |
 | prometheus.resources | object | `{"limits":{"cpu":"2","memory":"6G"},"requests":{"cpu":"500m","memory":"6G"}}` | Resource requests & limits for the `prometheus` container, learn more from the [Kubernetes documentation](https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/) Prometheus is relied upon to monitor services for sending alerts to site admins when something is wrong with Sourcegraph, thus its memory requests and limits are the same to guarantee it has enough memory to perform its job reliably and prevent conflicts with other pods on the same host node.  The limit chosen here is based on what works reliably on Sourcegraph.com with lots of traffic. |
 | prometheus.serviceAccount.create | bool | `true` | Enable creation of ServiceAccount |
 | prometheus.serviceAccount.name | string | `"prometheus"` | Name of the ServiceAccount to be created or an existing ServiceAccount |

charts/sourcegraph/examples/openshift/override.yaml

@@ -83,18 +83,22 @@ x-serviceAccount: &serviceAccount
 #            Only use "false" if cluster policy blocks creation of custom Roles
 #            Sourcegraph functionality is the same in either case
 #
-#   prometheus.privileged (default: true)
+#   prometheus.privileged (default: false)
 #
 #     Sourcegraph Helm values config to control how RBAC is handled for the Prometheus service account
 #
 #     true:  Creates a ClusterRole + ClusterRoleBinding granting cluster-wide read access to
 #            endpoints, nodes, pods, services, and /metrics.
 #            Enables kubernetes-apiservers and kubernetes-nodes scrape jobs in the Prometheus config
-#            If Executors are deployed in a separate namespace (recommended), then this allows
-#            Prometheus to scrape metrics from them
 #
 #     false: Binds its ServiceAccount to the built-in "view" ClusterRole (namespace-scoped)
-#            Only use "false" if cluster policy blocks creation of custom, cluster-wide Roles
+#            Prometheus discovers and scrapes Sourcegraph's pods and services in the release
+#            namespace in either case
+#
+#     Executor metrics do not depend on this setting, even if Executors are deployed in a
+#     separate namespace (recommended): Executors send their metrics to frontend with each
+#     heartbeat, and Prometheus scrapes them from the worker-executors Service in the
+#     release namespace
 
 # Creation of RBAC resources in sourcegraph-executor/k8s Helm chart
 #

charts/sourcegraph/examples/prometheus/README.md

@@ -14,6 +14,8 @@
 ```yaml
 prometheus:
   existingConfig: prometheus-override-k3s
+  # This config discovers targets in every namespace, and scrapes the nodes and API servers
+  privileged: true
 ```
 - Re-apply your Helm values override file, which may restart the Prometheus pod, but should not restart other services
 

charts/sourcegraph/templates/NOTES.txt

@@ -8,17 +8,6 @@ To learn more about supported configuration, check out https://sourcegraph.com/d
 
 {{- "\n" }}
 
-{{- if not .Values.prometheus.privileged }}
-🚧 Warning 🚧
-
-You have set 'prometheus.privileged' to 'false' and Prometheus will run with reduced privileges.
-Our observability stack requires some cluster-wide privileges to display provisioning metrics.
-These metrics provide critical information to help you scale the Sourcegraph deployment.
-Provisioning metrics can be restored by assigning elevated permissions to Service Account '{{ include "sourcegraph.serviceAccountName" (list . "prometheus") }}'.
-Consult https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/charts/sourcegraph/templates/prometheus/prometheus.ClusterRole.yaml for the necessary permissions.
-
-{{- end }}
-
 {{- if not .Values.cadvisor.containerSecurityContext.privileged }}
 🚧 Warning 🚧
 

charts/sourcegraph/templates/prometheus/prometheus.ConfigMap.yaml

@@ -139,7 +139,7 @@ data:
       - role: endpoints
         namespaces:
           names:
-           - {{ .Release.Namespace }}
+           - {{ .Release.Namespace | quote }}
 
       relabel_configs:
       - source_labels: [__meta_kubernetes_service_annotation_sourcegraph_prometheus_scrape]
@@ -187,40 +187,6 @@ data:
         regex: ^$
         action: drop
 
-    # Example scrape config for probing services via the Blackbox Exporter.
-    #
-    # The relabeling allows the actual service scrape endpoint to be configured
-    # via the following annotations:
-    #
-    # * `prometheus.io/probe`: Only probe services that have a value of `true`
-    - job_name: 'kubernetes-services'
-
-      metrics_path: /probe
-      params:
-        module: [http_2xx]
-
-      kubernetes_sd_configs:
-      - role: service
-
-      relabel_configs:
-      - source_labels: [__meta_kubernetes_service_annotation_prometheus_io_probe]
-        action: keep
-        regex: true
-      - source_labels: [__address__]
-        target_label: __param_target
-      - target_label: __address__
-        replacement: blackbox
-      - source_labels: [__param_target]
-        target_label: instance
-      - action: labelmap
-        regex: __meta_kubernetes_service_label_(.+)
-      - source_labels: [__meta_kubernetes_service_namespace]
-        # Sourcegraph specific customization. We want a more convenient to type label.
-        # target_label: kubernetes_namespace
-        target_label: ns
-      - source_labels: [__meta_kubernetes_service_name]
-        target_label: kubernetes_name
-
     # Example scrape config for pods
     #
     # The relabeling allows the actual pod scrape endpoint to be configured via the
@@ -233,6 +199,9 @@ data:
 
       kubernetes_sd_configs:
       - role: pod
+        namespaces:
+          names:
+           - {{ .Release.Namespace | quote }}
 
       relabel_configs:
       - source_labels: [__meta_kubernetes_pod_annotation_sourcegraph_prometheus_scrape]

charts/sourcegraph/templates/prometheus/prometheus.Deployment.yaml

@@ -26,6 +26,10 @@ spec:
     metadata:
       annotations:
         kubectl.kubernetes.io/default-container: prometheus
+        {{- if not .Values.prometheus.existingConfig }}
+        {{- /* Prometheus does not reload a changed ConfigMap by itself. The ConfigMap's labels are left out of the hash. */}}
+        checksum/prometheus-config: {{ (include (print $.Template.BasePath "/prometheus/prometheus.ConfigMap.yaml") . | fromYaml).data | toJson | sha256sum }}
+        {{- end }}
       {{- if .Values.sourcegraph.podAnnotations }}
       {{- toYaml .Values.sourcegraph.podAnnotations | nindent 8 }}
       {{- end }}

charts/sourcegraph/templates/prometheus/prometheus.RoleBinding.yaml

@@ -8,7 +8,7 @@ metadata:
     app.kubernetes.io/component: prometheus
   name: {{ .Values.prometheus.name }}
 roleRef:
-  apiGroup: ""
+  apiGroup: "rbac.authorization.k8s.io"
   kind: ClusterRole
   name: view
 subjects:

charts/sourcegraph/tests/prometheusConfigChecksum_test.yaml

@@ -0,0 +1,26 @@
+---
+suite: prometheusConfigChecksum
+# The ConfigMap template is listed so the Deployment's checksum annotation can
+# include it.
+templates:
+- prometheus/prometheus.Deployment.yaml
+- prometheus/prometheus.ConfigMap.yaml
+tests:
+- it: should restart prometheus when the chart's configuration changes
+  template: prometheus/prometheus.Deployment.yaml
+  asserts:
+  - matchRegex:
+      path: spec.template.metadata.annotations["checksum/prometheus-config"]
+      pattern: ^[0-9a-f]{64}$
+  # The hash of `null`, which is what an unreadable ConfigMap would hash to
+  - notEqual:
+      path: spec.template.metadata.annotations["checksum/prometheus-config"]
+      value: 74234e98afe7498fb5daf1f36ac2d78acc339464f950703b8c019892f982b90b
+- it: should drop the checksum annotation with an existing ConfigMap
+  template: prometheus/prometheus.Deployment.yaml
+  set:
+    prometheus:
+      existingConfig: my-prometheus-config
+  asserts:
+  - notExists:
+      path: spec.template.metadata.annotations["checksum/prometheus-config"]

charts/sourcegraph/tests/prometheusPrivileged_test.yaml

@@ -1,16 +1,131 @@
 suite: prometheusPrivileged
+release:
+  name: sourcegraph
+  namespace: sourcegraph-test
 templates:
-  - NOTES.txt
+  - prometheus/prometheus.ClusterRole.yaml
+  - prometheus/prometheus.ClusterRoleBinding.yaml
+  - prometheus/prometheus.RoleBinding.yaml
+  - prometheus/prometheus.ConfigMap.yaml
 tests:
-  - it: should not have the warning text when prometheus.privileged=true
+  - it: should create only a RoleBinding to the view ClusterRole by default
+    asserts:
+      - hasDocuments:
+          count: 0
+        template: prometheus/prometheus.ClusterRole.yaml
+      - hasDocuments:
+          count: 0
+        template: prometheus/prometheus.ClusterRoleBinding.yaml
+      - containsDocument:
+          kind: RoleBinding
+          apiVersion: rbac.authorization.k8s.io/v1
+          name: prometheus
+        template: prometheus/prometheus.RoleBinding.yaml
+      - equal:
+          path: roleRef
+          value:
+            apiGroup: rbac.authorization.k8s.io
+            kind: ClusterRole
+            name: view
+        template: prometheus/prometheus.RoleBinding.yaml
+      - equal:
+          path: subjects
+          value:
+            - kind: ServiceAccount
+              name: prometheus
+              namespace: sourcegraph-test
+        template: prometheus/prometheus.RoleBinding.yaml
+
+  - it: should create a ClusterRole and ClusterRoleBinding instead of a RoleBinding when prometheus.privileged=true
+    set:
+      prometheus.privileged: true
+    asserts:
+      - containsDocument:
+          kind: ClusterRole
+          apiVersion: rbac.authorization.k8s.io/v1
+          name: prometheus
+        template: prometheus/prometheus.ClusterRole.yaml
+      - containsDocument:
+          kind: ClusterRoleBinding
+          apiVersion: rbac.authorization.k8s.io/v1
+          name: prometheus
+        template: prometheus/prometheus.ClusterRoleBinding.yaml
+      - equal:
+          path: roleRef
+          value:
+            apiGroup: rbac.authorization.k8s.io
+            kind: ClusterRole
+            name: prometheus
+        template: prometheus/prometheus.ClusterRoleBinding.yaml
+      - equal:
+          path: subjects
+          value:
+            - kind: ServiceAccount
+              name: prometheus
+              namespace: sourcegraph-test
+        template: prometheus/prometheus.ClusterRoleBinding.yaml
+      - hasDocuments:
+          count: 0
+        template: prometheus/prometheus.RoleBinding.yaml
+
+  - it: should create no RBAC resources when prometheus.createRoleBinding=false
+    set:
+      prometheus.createRoleBinding: false
+    asserts:
+      - hasDocuments:
+          count: 0
+    templates:
+      - prometheus/prometheus.ClusterRole.yaml
+      - prometheus/prometheus.ClusterRoleBinding.yaml
+      - prometheus/prometheus.RoleBinding.yaml
+
+  - it: should create only the ClusterRole when prometheus.privileged=true and prometheus.createRoleBinding=false
     set:
       prometheus.privileged: true
+      prometheus.createRoleBinding: false
     asserts:
-      - notMatchRegexRaw:
-          pattern: You have set 'prometheus.privileged' to 'false'
-  - it: should have the warning text when prometheus.privileged=false
+      - hasDocuments:
+          count: 1
+        template: prometheus/prometheus.ClusterRole.yaml
+      - hasDocuments:
+          count: 0
+        template: prometheus/prometheus.ClusterRoleBinding.yaml
+      - hasDocuments:
+          count: 0
+        template: prometheus/prometheus.RoleBinding.yaml
+
+  # The patterns below spell out the indentation: `namespaces` at any other level
+  # is not part of the discovery config, and Prometheus rejects the file
+  - it: should discover pods and endpoints only in the release namespace by default
+    template: prometheus/prometheus.ConfigMap.yaml
+    asserts:
+      - matchRegex:
+          path: data["prometheus.yml"]
+          pattern: "- job_name: 'kubernetes-pods'\n\n  kubernetes_sd_configs:\n  - role: pod\n    namespaces:\n      names:\n       - \"sourcegraph-test\"\n"
+      - matchRegex:
+          path: data["prometheus.yml"]
+          pattern: "- job_name: 'kubernetes-service-endpoints'\n\n  kubernetes_sd_configs:\n  - role: endpoints\n    namespaces:\n      names:\n       - \"sourcegraph-test\"\n"
+      # No service discovery without a namespace list: the view RoleBinding cannot list other namespaces
+      - notMatchRegex:
+          path: data["prometheus.yml"]
+          pattern: "- role: \\w+\n *\n"
+      - notMatchRegex:
+          path: data["prometheus.yml"]
+          pattern: "job_name: 'kubernetes-(apiservers|nodes)'"
+
+  - it: should keep pods scoped to the release namespace when prometheus.privileged=true
     set:
-      prometheus.privileged: false
+      prometheus.privileged: true
+    template: prometheus/prometheus.ConfigMap.yaml
     asserts:
-      - matchRegexRaw:
-          pattern: You have set 'prometheus.privileged' to 'false'
+      - matchRegex:
+          path: data["prometheus.yml"]
+          pattern: "- job_name: 'kubernetes-pods'\n\n  kubernetes_sd_configs:\n  - role: pod\n    namespaces:\n      names:\n       - \"sourcegraph-test\"\n"
+      # The API servers are endpoints of the `kubernetes` Service in the `default` namespace,
+      # so this job must not be scoped to the release namespace
+      - matchRegex:
+          path: data["prometheus.yml"]
+          pattern: "- job_name: 'kubernetes-apiservers'\n\n  kubernetes_sd_configs:\n  - role: endpoints\n\n"
+      - matchRegex:
+          path: data["prometheus.yml"]
+          pattern: "job_name: 'kubernetes-nodes'"