feat: secure kepler's /metrics endpoint with kube rbac proxy by KaiyiLiu1234 · Pull Request #554 · sustainable-computing-io/kepler-operator

KaiyiLiu1234 · 2025-07-03T04:36:17Z

Power Monitor CR has been updated with a Security Mode to protect kepler's /metrics endpoint. When Mode is switched to 'rbac', the power monitor daemonset will be updated with kube rbac proxy sidecar which will enforce which service accounts can access /metrics based on static configuration and clusterrole/role rules. TLS is also enabled using kube rbac proxy and certs produced by openshift. Power Monitor API has been updated to include 'allowedSANames' which will allow users to request certain service accounts to have access.

codecov · 2025-07-03T04:38:32Z

Codecov Report

Attention: Patch coverage is 18.78558% with 856 lines in your changes missing coverage. Please review.

Project coverage is 28.35%. Comparing base (664edad) to head (44a6e70).
Report is 4 commits behind head on v1alpha1.

Files with missing lines	Patch %	Lines
pkg/utils/test/framework.go	0.39%	509 Missing ⚠️
pkg/reconciler/security.go	0.00%	261 Missing ⚠️
pkg/components/power-monitor/deployment.go	78.75%	47 Missing and 4 partials ⚠️
pkg/utils/k8s/k8s.go	29.16%	17 Missing ⚠️
pkg/utils/test/power_monitor_internal_builder.go	0.00%	9 Missing ⚠️
pkg/reconciler/service_monitor.go	0.00%	6 Missing ⚠️
pkg/reconciler/power-monitor.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           v1alpha1     #554      +/-   ##
============================================
- Coverage     29.80%   28.35%   -1.46%     
============================================
  Files            23       25       +2     
  Lines          2124     3164    +1040     
============================================
+ Hits            633      897     +264     
- Misses         1478     2248     +770     
- Partials         13       19       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

KaiyiLiu1234 · 2025-07-03T04:50:37Z

@sthaha Unit tests have been added in deployment_test.go. e2e tests have been added. Only change left is moving kube-rbac-proxy-image to the csv which will be added tomorrow asap.

vimalk78

quick 2 comments

vimalk78 · 2025-07-03T11:50:08Z

bundle/manifests/kepler-operator.clusterserviceversion.yaml

  - image: quay.io/sustainable_computing_io/kepler-reboot:v0.0.10
    name: kepler-reboot
-  replaces: kepler-operator.v0.17.0
+  replaces: kepler-operator.v0.0.3-refactorsecurity


dont add this change

vimalk78 · 2025-07-03T12:39:12Z

pkg/components/power-monitor/deployment.go

+func newKubeRBACProxyContainer(pmi *v1alpha1.PowerMonitorInternal) corev1.Container {
+	return corev1.Container{
+		Name:  KubeRBACProxyContainerName,
+		Image: "quay.io/brancz/kube-rbac-proxy:v0.19.0",


dont hard code image

fixed. Image can be added via csv just like the kepler-reboot image

vprashar2929 · 2025-07-06T06:24:17Z

api/v1alpha1/power_monitor_internal_types.go

 	Image string `json:"image"`

+	// +kubebuilder:validation:MinLength=3
+	// +kubebuilder:default="quay.io/brancz/kube-rbac-proxy:v0.19.0"


We can remove the default for here?

cmd/main.go

api/v1alpha1/power_monitor_types.go

cmd/main.go

internal/controller/power_monitor_internal.go

vprashar2929 · 2025-07-06T07:02:27Z

internal/controller/power_monitor_internal.go

+	}
+
+	pmis := &v1alpha1.PowerMonitorInternalList{}
+	err := r.List(ctx, pmis)


err:= r.Client.List(ctx,pmis)?

they both work the same but the precommit checks fail if using r.Client instead of r.List. I believe it's because r.List is more idiomatic than go given the fact client.Client is embedded field. So I am forced by precommit to use r.List.

It's not ideal ... and consider these comments as nit

internal/controller/power_monitor_internal.go

vprashar2929 · 2025-07-06T07:35:54Z

internal/controller/power_monitor_internal.go

+	}
+
+	pmis := &v1alpha1.PowerMonitorInternalList{}
+	err := r.List(ctx, pmis)


err := r.Client.List(ctx,pmis)

I agree with r.Client.List() but like @KaiyiLiu1234 said, lets fix the pre-commit ( in a totally separate pr) :)

It's golangci-lint which is expects r.List instead of r.Client.List

Makefile

go.mod

internal/controller/power_monitor_internal.go

sthaha · 2025-07-07T04:54:42Z

internal/controller/power_monitor_internal.go

+		c = c.Watches(&corev1.Secret{},
+			handler.EnqueueRequestsFromMapFunc(r.mapSecretToPowerMonitorRequests),
+			builder.WithPredicates(
+				predicate.GenerationChangedPredicate{},


should we have GenerationChanged ?

As in is the preference to switch to ResourceVersionChanged (which should cover all changes)?

Wait didn't we want to use Generationchanged so we did not have to use Resourceversionchanged which may result in many reconciliations for being to sensitive?

No, lets keep it this way .. I didn't realize secrets (certs) may be updated externally. Could you please add a comment why GenerationChanged is needed despite of having AnnotationChanged ?

Added the comment

sthaha · 2025-07-07T04:59:56Z

pkg/components/power-monitor/deployment.go

+		}
+		config.Authorization.StaticRules = append(config.Authorization.StaticRules, newRule)
+	}
+	yamlData, _ := yaml.Marshal(&config)


lets not ignore (any) error .. can we propagate it or log it some how if it isn't easy, lets file an issue and the address this in a generic way.

pkg/components/power-monitor/deployment.go

sthaha

LGTM! Good job @KaiyiLiu1234 !!

Note: minor comments to be be fixed in follow up PRs ..
@KaiyiLiu1234 could you please create tracking issue for all unresolved comments?

pkg/utils/test/framework.go

vprashar2929 · 2025-07-08T12:22:41Z

pkg/components/power-monitor/deployment.go

+		Args: []string{
+			fmt.Sprintf("--secure-listen-address=0.0.0.0:%d", SecurePort),
+			fmt.Sprintf("--upstream=http://127.0.0.1:%d", PowerMonitorDSPort),
+			fmt.Sprintf("--auth-token-audiences=%s.%s.svc", pmi.Name, pmi.Namespace()),


Do we need auth-token-audiences? With this kuberbac expects user token's with audience power-monitor.power-monitor.svc to be present in order to access

Yes this is important to have. The purpose of the audience is to deliberately limit the token to the intended service (in this case our power-monitor service). We do not want the token to be used for any other purposes.

Doesn't it make sense to have this if we had more than one service? And we want SA to be for a specific service

Users can provide many service accounts they want kepler to allow access to. Each service account will need to pass a token (for authorization). Users can have many service accounts for many different target services (not just power-monitor service). This can help with accidental use of a token or with stolen tokens on the user side.

Power Monitor CR has been updated with a Security Mode to protect kepler's /metrics endpoint. When Mode is switched to 'rbac', the power monitor daemonset will be updated with kube rbac proxy sidecar which will enforce which service accounts can access /metrics based on static configuration and clusterrole/role rules. TLS is also enabled using kube rbac proxy and certs produced by openshift. Power Monitor API has been updated to include 'allowedSANames' which will allow users to request certain service accounts to have access. Signed-off-by: Kaiyi Liu <kaliu@redhat.com>

KaiyiLiu1234 requested review from sthaha and vprashar2929 July 3, 2025 04:36

github-actions bot added the feat A new feature or enhancement label Jul 3, 2025

KaiyiLiu1234 force-pushed the kube-rbac-proxy branch from 9bd8aad to 521f0d9 Compare July 3, 2025 04:43

KaiyiLiu1234 force-pushed the kube-rbac-proxy branch 13 times, most recently from e75df84 to 45110c4 Compare July 3, 2025 14:39

vimalk78 reviewed Jul 3, 2025

View reviewed changes

KaiyiLiu1234 force-pushed the kube-rbac-proxy branch 7 times, most recently from 608d711 to a402e36 Compare July 5, 2025 00:36

vprashar2929 reviewed Jul 6, 2025

View reviewed changes

KaiyiLiu1234 force-pushed the kube-rbac-proxy branch from a402e36 to 5dbf2f9 Compare July 6, 2025 23:25

KaiyiLiu1234 force-pushed the kube-rbac-proxy branch from 5dbf2f9 to cd35db5 Compare July 7, 2025 01:56

sthaha reviewed Jul 7, 2025

View reviewed changes

Makefile Show resolved Hide resolved

sthaha reviewed Jul 7, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

sthaha reviewed Jul 7, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

sthaha reviewed Jul 7, 2025

View reviewed changes

internal/controller/power_monitor_internal.go Show resolved Hide resolved

sthaha reviewed Jul 7, 2025

View reviewed changes

vprashar2929 reviewed Jul 7, 2025

View reviewed changes

pkg/components/power-monitor/deployment.go Outdated Show resolved Hide resolved

KaiyiLiu1234 mentioned this pull request Jul 7, 2025

Errors in deployment.go should not be ignored and should be propogated #558

Closed

sthaha reviewed Jul 8, 2025

View reviewed changes

vprashar2929 reviewed Jul 8, 2025

View reviewed changes

pkg/utils/test/framework.go Show resolved Hide resolved

vprashar2929 reviewed Jul 8, 2025

View reviewed changes

pkg/utils/test/framework.go Outdated Show resolved Hide resolved

vprashar2929 reviewed Jul 8, 2025

View reviewed changes

KaiyiLiu1234 force-pushed the kube-rbac-proxy branch from cd35db5 to 44a6e70 Compare July 8, 2025 18:02

sthaha approved these changes Jul 9, 2025

View reviewed changes

sthaha merged commit 718e00f into sustainable-computing-io:v1alpha1 Jul 9, 2025
17 of 21 checks passed

vprashar2929 mentioned this pull request Jul 9, 2025

Document the need of audience in token when connecting to secure metric endpoint #561

Open

Conversation

KaiyiLiu1234 commented Jul 3, 2025

Uh oh!

codecov bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

KaiyiLiu1234 commented Jul 3, 2025

Uh oh!

vimalk78 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sthaha left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov bot commented Jul 3, 2025 •

edited

Loading

sthaha left a comment •

edited

Loading