CNF-11234: Enable RTE metrics to be scraped securely by Prometheus #1107

rbaturov · 2024-12-10T21:42:27Z

This is a follow-up PR for Enable NROP metrics to be to scraped securely by Prometheus .

This PR encompasses and integrates all the needed infrastructure to enable secured communication for scraping metrics by Prometheus, now for RTE.

A follow-up PR will be posted for e2e tests later.

To validate that this PR is functioning correctly, please follow these steps:

build image of the operator (make docker-build docker-push)
run: make deploy
Attach to one of the prometheus pods oc exec -it prometheus-k8s-0 -n openshift-monitoring /bin/bash
run:

curl -v \
--cacert /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt \
-H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \
https://numaresources-rte-metrics-service.numaresources.svc:2112/metrics

openshift-ci · 2024-12-10T21:42:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ffromani · 2024-12-11T07:48:55Z

/retest

ffromani

I'm not sure we need a separate manifest set. I'. mot saying I'm against it but I'd like to see if/how we can merge it with the existing manifest set

ffromani · 2024-12-11T07:51:15Z

config/rbac/role.yaml

+- apiGroups:
+  - ""
+  resources:
+  - serviceaccounts
+  verbs:
+  - '*'
+- apiGroups:
+  - ""
+  resources:
+  - services
+  verbs:
+  - '*'


ok, this is a role, hence it is ONLY granted in the NROP namespace, right? Stll this is very broad, we need to narrow down AND document in the commit message why do we need these permissions

I agree, you are right. we want in only in the NROP namespace.

@ffromani
I've tried to create a separate role (not clusterRole) and a RoleBinding.
But adding the label
//+kubebuilder:rbac:groups="",resources=services,verbs=*
followed by make manifests appends its in the clusterRole.
Not sure how we can avoide this and append this in the dedicated role.

ffromani · 2024-12-11T07:51:40Z

controllers/numaresourcesoperator_controller.go

+
+	for _, obj := range r.RTEMetricsManifests.ToObjects() {
+		// Check if the object already exists
+		existingObj := obj.DeepCopyObject().(client.Object)


why do we need the cast here?

If I'll remove the casting:
I'll get this error when using r.Client.Get:
cannot use existingObj (variable of type runtime.Object) as client.Object value in argument to r.Client.Get: runtime.Object does not implement client.Object (missing method GetAnnotations)

and why do we use DeepCopyObject() and not DeepCopy() ?

@ffromani This because the client.Object implements DeepCopyObject() and not DeepCopy()

controllers/numaresourcesoperator_controller_test.go

rbaturov · 2024-12-11T08:53:12Z

I'm not sure we need a separate manifest set. I'. mot saying I'm against it but I'd like to see if/how we can merge it with the existing manifest set

I agree on this one.
But I coudln't figure out a more appropriate place to put this manifest.

ffromani

/approve
/lgtm

good starting point for 4.19 and onwards

openshift-ci · 2024-12-16T08:48:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, rbaturov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ffromani]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ffromani · 2024-12-16T08:49:49Z

/hold

need to check RBAC rules again

ffromani · 2024-12-16T08:52:00Z

/lgtm cancel

rbaturov · 2024-12-16T14:13:02Z

/hold

need to check RBAC rules again

Regarding this, it is correct that now I have granted permission to the operator to manage service objects across the entire cluster and not narrowed it down to NROP namespace.
As I said before, I'm not sure how we can create a role not a clusterrole for this matter.

rbaturov · 2024-12-19T09:11:42Z

/retest-required

rbaturov · 2024-12-19T10:18:05Z

/retest-required

rbaturov · 2024-12-19T11:43:30Z

/retest-required

ffromani · 2024-12-19T11:46:50Z

/retest-required

the first retest it's a freebie, but past that we need to check why tests are failing and only when we are confident it's actually an infra issue we should retest

rbaturov · 2024-12-19T12:00:13Z

/retest-required

the first retest it's a freebie, but past that we need to check why tests are failing and only when we are confident it's actually an infra issue we should retest

I saw there was a timeout. I'm not sure this is the infra indeed, I wanted to give it a try before deep diving into that
Edit: actually I forgot this is the second retest. will take a look now

openshift-ci-robot · 2024-12-19T12:37:17Z

@rbaturov: This pull request references CNF-11234 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

This is a follow-up PR for Enable NROP metrics to be to scraped securely by Prometheus .

This PR encompasses and integrates all the needed infrastructure to enable secured communication for scraping metrics by Prometheus, now for RTE.

A follow-up PR will be posted for e2e tests later.

To validate that this PR is functioning correctly, please follow these steps:

build image of the operator (make docker-build docker-push)

run: make deploy

Attach to one of the prometheus pods oc exec -it prometheus-k8s-0 -n openshift-monitoring /bin/bash

run:
curl -v \
--cacert /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt \
-H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \
https://numaresources-rte-metrics-service.numaresources.svc:2112/metrics

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

ffromani

/lgtm

we can narrow down permissions later on

ffromani · 2024-12-19T14:09:14Z

/hold cancel

controllers/numaresourcesoperator_controller.go

* Enabled metrics by default by setting --metrics-mode=httptls. * Added an RTE metrics package and associated manifests. Currently, this includes only the Service manifest. * The Service manifest includes a special annotation monitored by the Service CA operator. This operator generates the tls.key and tls.crt files in the rte-metrics-service-cert secret, which is consumed by the RTE worker container. * Updated the DaemonSet configuration to include the required volume and volumeMount for accessing the metrics secret. * Deployment of this Service is required for metrics functionality. Signed-off-by: Ronny Baturov <[email protected]>

As part of enabling metrics for RTE, a Service resource is created during the deployment of the RTE metrics manifests by the operator. This commit grants the operator pod the necessary permissions to deploy the Service CR. Signed-off-by: Ronny Baturov <[email protected]>

* Integrating RTE metrics manifests to be deployed by the operator * This adds unit test for metrics components creation Signed-off-by: Ronny Baturov <[email protected]>

Signed-off-by: Ronny Baturov <[email protected]>

Tal-or · 2024-12-22T12:22:49Z

/lgtm
Good enough for now. Thanks!

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 10, 2024

rbaturov marked this pull request as ready for review December 11, 2024 06:22

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2024

openshift-ci bot requested review from ffromani and Tal-or December 11, 2024 06:22

rbaturov mentioned this pull request Dec 11, 2024

CNF-14833: E2E: metrics exposed securely #1037

Merged

ffromani reviewed Dec 11, 2024

View reviewed changes

rbaturov force-pushed the expose-metrics-securely-rte branch 3 times, most recently from 1efaad4 to b827ccd Compare December 15, 2024 10:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2024

rbaturov force-pushed the expose-metrics-securely-rte branch from b827ccd to 7b5b369 Compare December 15, 2024 10:20

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2024

rbaturov force-pushed the expose-metrics-securely-rte branch from 7b5b369 to 15b513e Compare December 15, 2024 11:23

ffromani reviewed Dec 16, 2024

View reviewed changes

openshift-ci bot assigned ffromani Dec 16, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2024

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2024

rbaturov force-pushed the expose-metrics-securely-rte branch from 15b513e to 3ddbf0e Compare December 19, 2024 08:33

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2024

rbaturov force-pushed the expose-metrics-securely-rte branch from 3ddbf0e to 63eef4a Compare December 19, 2024 08:33

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2024

rbaturov changed the title ~~Enable RTE metrics to be scraped securely by Prometheus~~ CNF-11234: Enable RTE metrics to be scraped securely by Prometheus Dec 19, 2024

rbaturov force-pushed the expose-metrics-securely-rte branch from 63eef4a to 91efaab Compare December 19, 2024 13:01

ffromani reviewed Dec 19, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 19, 2024

rbaturov force-pushed the expose-metrics-securely-rte branch from 91efaab to 2edf895 Compare December 22, 2024 07:29

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2024

rbaturov force-pushed the expose-metrics-securely-rte branch from 2edf895 to a063be5 Compare December 22, 2024 07:36

Tal-or reviewed Dec 22, 2024

View reviewed changes

controllers/numaresourcesoperator_controller.go Show resolved Hide resolved

rbaturov force-pushed the expose-metrics-securely-rte branch from a063be5 to fbeca08 Compare December 22, 2024 10:46

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2024

rbaturov added 4 commits December 22, 2024 12:51

RTE metrics enablement

f8f3922

* Integrating RTE metrics manifests to be deployed by the operator * This adds unit test for metrics components creation Signed-off-by: Ronny Baturov <[email protected]>

make generate bundle manifests

ae455f8

Signed-off-by: Ronny Baturov <[email protected]>

rbaturov force-pushed the expose-metrics-securely-rte branch from fbeca08 to ae455f8 Compare December 22, 2024 10:52

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2024

openshift-ci bot assigned Tal-or Dec 22, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2024

openshift-merge-bot bot merged commit 1dd2f17 into openshift-kni:main Dec 22, 2024
15 checks passed

rbaturov mentioned this pull request Jan 7, 2025

[release-4.18] Enable RTE metrics to be scraped securely by Prometheus #1141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNF-11234: Enable RTE metrics to be scraped securely by Prometheus #1107

CNF-11234: Enable RTE metrics to be scraped securely by Prometheus #1107

rbaturov commented Dec 10, 2024

openshift-ci bot commented Dec 10, 2024

ffromani commented Dec 11, 2024

ffromani left a comment

ffromani Dec 11, 2024

rbaturov Dec 11, 2024

rbaturov Dec 15, 2024

ffromani Dec 11, 2024

rbaturov Dec 11, 2024

ffromani Dec 16, 2024

rbaturov Dec 16, 2024

rbaturov commented Dec 11, 2024

ffromani left a comment

openshift-ci bot commented Dec 16, 2024

ffromani commented Dec 16, 2024

ffromani commented Dec 16, 2024

rbaturov commented Dec 16, 2024

rbaturov commented Dec 19, 2024

rbaturov commented Dec 19, 2024

rbaturov commented Dec 19, 2024

ffromani commented Dec 19, 2024

rbaturov commented Dec 19, 2024 •

edited

Loading

openshift-ci-robot commented Dec 19, 2024

ffromani left a comment

ffromani commented Dec 19, 2024

Tal-or commented Dec 22, 2024

CNF-11234: Enable RTE metrics to be scraped securely by Prometheus #1107

CNF-11234: Enable RTE metrics to be scraped securely by Prometheus #1107

Conversation

rbaturov commented Dec 10, 2024

openshift-ci bot commented Dec 10, 2024

ffromani commented Dec 11, 2024

ffromani left a comment

Choose a reason for hiding this comment

ffromani Dec 11, 2024

Choose a reason for hiding this comment

rbaturov Dec 11, 2024

Choose a reason for hiding this comment

rbaturov Dec 15, 2024

Choose a reason for hiding this comment

ffromani Dec 11, 2024

Choose a reason for hiding this comment

rbaturov Dec 11, 2024

Choose a reason for hiding this comment

ffromani Dec 16, 2024

Choose a reason for hiding this comment

rbaturov Dec 16, 2024

Choose a reason for hiding this comment

rbaturov commented Dec 11, 2024

ffromani left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 16, 2024

ffromani commented Dec 16, 2024

ffromani commented Dec 16, 2024

rbaturov commented Dec 16, 2024

rbaturov commented Dec 19, 2024

rbaturov commented Dec 19, 2024

rbaturov commented Dec 19, 2024

ffromani commented Dec 19, 2024

rbaturov commented Dec 19, 2024 • edited Loading

openshift-ci-robot commented Dec 19, 2024

ffromani left a comment

Choose a reason for hiding this comment

ffromani commented Dec 19, 2024

Tal-or commented Dec 22, 2024

rbaturov commented Dec 19, 2024 •

edited

Loading