fix: Use roles instead of cluster roles for SBJ

[Gregory-Pereira]

/cc @securesign/qe @lance

[openshift-ci]

@Gregory-Pereira: GitHub didn't allow me to request PR reviews from the following users: securesign/qe.

Note that only securesign members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @securesign/qe @lance

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 kubernetes/test-infra repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gregory-Pereira

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 [Gregory-Pereira]

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

[lance]

@Gregory-Pereira I am seeing this in the e2e test logs

error: the namespace from the provided object "rhtas-operator" does not match the namespace "securesign". You must pass '--namespace=rhtas-operator' to perform this operation.

[lance]

The konflux build failed on build-source-image with this error, which I suspect may be transient.

Copying blob sha256:5038e19ffe9670630d6f85c37de580326cf0a715f5bac4dfbceee8ccb3236c12
time="2024-04-02T20:12:53Z" level=fatal msg="reading blob sha256:aea816dcd21789d1ec292c9318fb768591fd6e5a00baa492582a39d332b306c2: fetching blob: received unexpected HTTP status: 502 Bad Gateway"
2024-04-02 20:12:53,178:source-build:ERROR:failed to build source image

[Gregory-Pereira]

These errors I think are due to the wrong default toggle in the api/v1/common/monitoring.go. Fixing

[Gregory-Pereira]

Owner references have to be dropped:

ERROR ensure RBAC for segment job error during action execution {"controller": "securesign", "controllerGroup": "rhtas.redhat.com", "controllerKind": "Securesign", "Securesign": {"name":"securesign-sample-testing","namespace":"testing"}, "namespace": "testing", "name": "securesign-sample-testing", "reconcileID": "d4855b74-a45c-460e-9650-bab52d833aa5", "error": "could not set controll reference for role: cross-namespace owner references are disallowed, owner's namespace testing, obj's namespace openshift-monitoring"}

For more information see the Note: section of: https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/#owner-references-in-object-specifications

[bouskaJ]

You can remove resources without ownership in the finalizer section (see https://github.com/securesign/secure-sign-operator/blob/main/controllers/securesign/securesign_controller.go#L97)

[JasonPowr]

@Gregory-Pereira Just a thought feel free to ignore if I'm wrong (I probably am), Is there a reason we need to check these config maps to see if telemetry is enabled, Can we just add a field to the API, maybe securesign.telemetry: bool can also do a check to see if monitoring is enabled, if it is then make the calls to segment? It would at least remove the calls to cluster-monitoring-config and the console resource, for enable_user_workload_monitoring_if_does_not_exist it could just be a INFO:msg mentioning that User workload monitoring is not enabled, please enable it, with a link to docs?

[Gregory-Pereira]

@Gregory-Pereira Just a thought feel free to ignore if I'm wrong (I probably am), Is there a reason we need to check these config maps to see if telemetry is enabled, Can we just add a field to the API, maybe securesign.telemetry: bool can also do a check to see if monitoring is enabled, if it is then make the calls to segment? It would at least remove the calls to cluster-monitoring-config and the console resource, for enable_user_workload_monitoring_if_does_not_exist it could just be a INFO:msg mentioning that User workload monitoring is not enabled, please enable it, with a link to docs?

I am not sure that would work, these checks to see how / if monitoring were disabled were explicitly shared with me at product management (see jira: https://issues.redhat.com/browse/CONSOLE-3944 ). I think we are required to check in these pre-established ways to see if monitoring is disabled.

[JasonPowr]

@Gregory-Pereira Just a thought feel free to ignore if I'm wrong (I probably am), Is there a reason we need to check these config maps to see if telemetry is enabled, Can we just add a field to the API, maybe securesign.telemetry: bool can also do a check to see if monitoring is enabled, if it is then make the calls to segment? It would at least remove the calls to cluster-monitoring-config and the console resource, for enable_user_workload_monitoring_if_does_not_exist it could just be a INFO:msg mentioning that User workload monitoring is not enabled, please enable it, with a link to docs?

I am not sure that would work, these checks to see how / if monitoring were disabled were explicitly shared with me at product management (see jira: https://issues.redhat.com/browse/CONSOLE-3944 ). I think we are required to check in these pre-established ways to see if monitoring is disabled.

Ahhhhhhh Ok yea, I figured something like this applied, just wanted to make sure, thanks.

[kahboom]

/lgtm

[osmman]

I think that we should use only one method to decide if analytics should be enabled. Right now CanHandle and Handle method uses different method (annotation/spec api).

I do not know what was the reason to introduce spec.Analytics into API, if original purpose was to disable analytics by default than it could be removed and we can depend only on annotation.

[JasonPowr]

@osmman I've removed the patch from the config map, will update the image with the proper one on monday, if you want to test you can use this one: quay.io/redhat-user-workloads/rhtas-tenant/segment-backup-job/segment-backup-job:on-pr-5df9ba8ea1c4a42cb85ff12afc4102ff83825e17

[JasonPowr]

/hold

[osmman]

I tested it and it works without problem for me

[JasonPowr]

@osmman Sorry, when you say it worked, are you referring to the pr as a whole? I'm just not sure if you are referring to #301 (comment) or not?

[osmman]

@osmman Sorry, when you say it worked, are you referring to the pr as a whole? I'm just not sure if you are referring to #301 (comment) or not?

As a whole PR

[JasonPowr]

/unhold

[osmman]

/lgtm