Skip to content

added metrics auth rbac#198

Open
Bloodraven21 wants to merge 3 commits into
valkey-io:mainfrom
Bloodraven21:feat/metrics-auth-rbac
Open

added metrics auth rbac#198
Bloodraven21 wants to merge 3 commits into
valkey-io:mainfrom
Bloodraven21:feat/metrics-auth-rbac

Conversation

@Bloodraven21

Copy link
Copy Markdown
Collaborator

#192

Operator: secure metrics with auth RBAC (metrics-auth + metrics-reader)

Closes #192

Summary

The operator served its controller-runtime metrics over plain HTTP because the deployment hardcoded --metrics-secure=false, and the chart shipped none of the RBAC the kubebuilder secure-metrics convention requires. This PR adds that RBAC, makes secure
mode toggleable, and documents and tests the flow.

Changes

  • metrics-auth ClusterRole + ClusterRoleBinding (templates/metrics-rbac.yaml): grants the operator ServiceAccount create on tokenreviews.authentication.k8s.io and subjectaccessreviews.authorization.k8s.io so it can authenticate and authorize incoming
    scrape requests.
  • metrics-reader ClusterRole: grants get on the /metrics non-resource URL.
  • Optional reader binding: metrics.reader.binding.{create,serviceAccountName,namespace} lets the chart bind a scraper ServiceAccount to metrics-reader. The subject name and namespace support templating (tpl), e.g. {{ .Release.Name }}-prometheus.
    Disabled by default, so the admin binds the scraper manually unless they opt in.
  • Secure toggle (templates/deployment.yaml): --metrics-secure={{ .Values.metrics.secure }}, new value metrics.secure defaulting to false.
  • Metrics Service (templates/metrics-service.yaml): port name renders https when secure, http otherwise.
  • All new RBAC is gated behind the existing rbac.create, consistent with the manager ClusterRole/Binding.
  • Docs: new "Metrics and RBAC" section and parameter rows in README.md.
  • Chart version bumped to 0.2.3.

Backward compatibility

metrics.secure defaults to false and metrics.reader.binding.create defaults to false, so existing installs are unchanged.

Testing

  • helm lint clean; helm unittest passes (29 tests across 4 suites) covering: roles render with rbac.create: true and are omitted when false, correct rules and binding subject, the secure flag toggling deployment args, the service port-name switch, and
    the optional reader binding including its tpl behavior.
  • Validated live on a kind cluster with metrics.secure=true:
    • --metrics-secure=true present in deployment args; metrics served over HTTPS.
    • No-token request → 401 (authn enforced).
    • Operator SA token → authenticated but authz-denied (it has metrics-auth, not metrics-reader), proving the SubjectAccessReview path.
    • A ServiceAccount bound to metrics-reader → 200 with real metrics.

Values

metrics:
secure: false
reader:
binding:
create: false
serviceAccountName: ""
namespace: ""

Signed-off-by: Ishan Jain <ishanij10115@gmail.com>
@Bloodraven21 Bloodraven21 added the chart:valkey-operator Related to the valkey-operator chart label Jun 24, 2026
@@ -0,0 +1,65 @@
{{- if .Values.rbac.create -}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This RBAC renders independent of metrics.enabled/metrics.secure. Setting metrics.enabled: false still creates these ClusterRoles/Bindings, leaving orphaned cluster-scoped RBAC. Consider gating on metrics.secure as controller-runtime only makes tokenreview and subjectaccessreview API calls when it's doing delegated authn/authz on an incoming scrape and not on insecure metrics.

Consider below suggestions:

Suggested change
{{- if .Values.rbac.create -}}
{{- if and .Values.rbac.create .Values.metrics.enabled -}}

or

Suggested change
{{- if .Values.rbac.create -}}
{{- if and .Values.rbac.create .Values.metrics.enabled .Values.metrics.secure -}}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

thanks will check this

Comment thread valkey-operator/CHANGELOG.md Outdated

## Unreleased
### Added
-Add metrics auth RBAC (metrics-auth-role, metrics-reader-role) support to the valkey-operator chart

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: you may want to add space after -

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

thanks done

Signed-off-by: Ishan Jain <ishanij10115@gmail.com>
Signed-off-by: Ishan Jain <ishanij10115@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chart:valkey-operator Related to the valkey-operator chart

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Operator: Add metrics auth RBAC (metrics-auth-role, metrics-reader-role) support to the valkey-operator chart

2 participants