Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[issue#306] Add missing ClusterRoles #465

Merged
merged 8 commits into from
Sep 21, 2023
Merged

[issue#306] Add missing ClusterRoles #465

merged 8 commits into from
Sep 21, 2023

Conversation

elfiesmelfie
Copy link
Collaborator

The cluster-monitoring-operator is required for STF to install. It creates the required alertmanager-main and prometheus-k8s. ClusterRoles, and STF relies on these being present. These are not present when using CRC, so ClusterRoles need to be explicitly created.

The names of the ClusterRoles have been updated, in case there is some conflict when cluster-monitoring-operator is installed after STF.

This is a workaround for not having cluster-monitoring-operator installed: #306

resolves #306

@elfiesmelfie elfiesmelfie requested review from csibbitt, leifmadsen and vkmc and removed request for csibbitt September 14, 2023 15:52
@leifmadsen leifmadsen enabled auto-merge (squash) September 14, 2023 17:02
@elfiesmelfie
Copy link
Collaborator Author

The Jenkins job timed out 😞

@leifmadsen
Copy link
Member

The Jenkins job timed out 😞

There is a bug. It times out on the validation because all components are not operational. Here is the first error I found in the STO logs:

 TASK [Bind the local prometheus SA to prometheus cluster role (for oauth perms)] ******************************** 
fatal: [localhost]: FAILED! => {"changed": false, "error": 422, "msg": "Failed to patch object: b'{\"kind\":\"Status\",\"apiVersion\":\"v1\",\"metadata\":{},\"status\":\"Failure\",\"message\":\"ClusterRoleBinding.rbac.authorization.k8s.io \\\\\"prometheus-k8s-service-telemetry\\\\\" is invalid: roleRef: Invalid value: rbac.RoleRef{APIGroup:\\\\\"rbac.authorization.k8s.io\\\\\", Kind:\\\\\"ClusterRole\\\\\", Name:\\\\\"prometheus-stf\\\\\"}: cannot change roleRef\",\"reason\":\"Invalid\",\"details\":{\"name\":\"prometheus-k8s-service-telemetry\",\"group\":\"rbac.authorization.k8s.io\",\"kind\":\"ClusterRoleBinding\",\"causes\":[{\"reason\":\"FieldValueInvalid\",\"message\":\"Invalid value: rbac.RoleRef{APIGroup:\\\\\"rbac.authorization.k8s.io\\\\\", Kind:\\\\\"ClusterRole\\\\\", Name:\\\\\"prometheus-stf\\\\\"}: cannot change roleRef\",\"field\":\"roleRef\"}]},\"code\":422}\\n'", "reason": "Unprocessable Entity", "status": 422}

@leifmadsen leifmadsen self-requested a review September 15, 2023 14:30
@leifmadsen leifmadsen self-requested a review September 15, 2023 14:56
csibbitt
csibbitt reviewed Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@csibbitt csibbitt left a comment

Choose a reason for hiding this comment

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

Since we're mucking about in here, I'd like to take the opportunity to try to narrow this scope as much as possible. It looks pretty good right now, but I think there are still a few things than can move from the cluster scope to the namespaced scope.

deploy/role.yaml
@@ -22,6 +22,7 @@ rules:
- watch
- update
- patch
- delete
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noting that this should be transitional only and could be removed in a later version.

Copy link
Member

@leifmadsen leifmadsen Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was going to file an issue about this, but then I realized that if someone was somehow running STF 1.5.2 and moved to STF 1.5.4 they would need this.

Additionally, the latest changes I landed in 5d0210d will continually require the delete RBAC.

We could refine this though and break out the clusterroles and clusterrolebindings though to have the delete resource only on the clusterrolebindings. I'm also curious if we could further lock this down to just resources we manage and not all ClusterRoles and ClusterRoleBindings.

I am not going to do any of this at this point though, but I'm certainly interested about doing another RBAC focused review, and if we can make the scope even more restrictive and controlled for only objects we actually need to adjust and own.

roles/servicetelemetry/tasks/component_prometheus.yml Show resolved Hide resolved
roles/servicetelemetry/tasks/component_prometheus.yml Outdated Show resolved Hide resolved
roles/servicetelemetry/tasks/component_alertmanager.yml Outdated Show resolved Hide resolved
roles/servicetelemetry/tasks/component_alertmanager.yml Outdated Show resolved Hide resolved
@leifmadsen
Copy link
Member

Requesting final review prior to merge!

@leifmadsen
Copy link
Member

CI timed out again. I think the Jenkins-based CI systems are just overwhelmed.

@leifmadsen
Copy link
Member

I'm skipping out on approving this since I landed a fair number of changes here, and it'd effectively be self-approving.

Deferring to Chris and Victoria on this one, but I think we've all probably looked at this enough that if CI passes, landing it isn't going to be a problem for anyone. Thanks everyone for the patience and quick turn around on the reviews!

elfiesmelfie and others added 7 commits September 21, 2023 08:38
@elfiesmelfie @leifmadsen
[issue#306] Add missing ClusterRoles
d537327 
The cluster-monitoring-operator is required for STF to install. It
creates the required alertmanager-main and prometheus-k8s.
ClusterRoles, and STF relies on these being present.
These are not present when using CRC, so ClusterRoles need to be
explicitly created.

The names of the ClusterRoles have been updated, in case there is some
conflict when cluster-monitoring-operator is installed after STF.

This is a workaround for not having cluster-monitoring-operator
installed: #306

resolves #306
@leifmadsen
Fix up the RBAC setup for prometheus-stf (#467)
a74cffb 
Fix up the RBAC changes to fully get prometheus-stf working and
decoupled from prometheus-k8s. Changes to using a separate
prometheus-stf ClusterRole, ClusterRoleBinding, and ServiceAccount,
along with a Role and RoleBinding, all using prometheus-stf as the
ServiceAccount. Also updates the Alertmanager configuration to use
alertmanager-stf instead of alertmanager-main.
@leifmadsen
Fix smoketest to use prometheus-stf for token retrieval
e3306bd
@leifmadsen
Refactor smoketest script (#468)
a63f79c 
* Refactor smoketest script

Perform a bit of smoketest refactoring and fix up a few bugs.

* Update alert trigger to use startsAt in order to potentially speed up
  delivery of the alerts. Failures in the SNMP_WEBHOOK_STATUS seems to
  be primarily to delayed alert notification through
  prometheus-snmp-webhook.
* Add an alert clean up task as part of the clean up logic at the end.
* Update openssl x509 to not use the -in flag which seems unnecessary
  and on some systems causes a failure.
* Add new SMOKETEST_VERBOSE boolean so local testing can skip massive
  amounts of information dumped to stdout.
* Remove curl pod using label selector for slightly cleaner output.
* Update failure check to combine RET and SNMP_WEBHOOK_STATUS since
  testing seems to show changes are slightly more reliable.

* Show logs from curl
@leifmadsen
Remove nodes/metrics permission from ClusterRole
557d75f 
As part of least priviledge work, remove the nodes/metrics permission as
we're not scraping nodes for information. Everything appears to continue
working in STF without this permission.
@leifmadsen
Move SCC RBAC from ClusterRole to Role
a4787a3 
Working on simplifying and reducing our access scope as much as
possible. It appears moving SCC RBAC from ClusterRole to Role allows
things to continue to work with Prometheus. It's possible further
testing may reveal this will need to reverted.
@leifmadsen
Convert alertmanager-stf Role to ClusterRole (#473)
c525fb0 
Convert alertmanager-stf Role to ClusterRole as the tokenreviews and
subjectaccessreviews resources need to be accessable at the cluster
scope.
@leifmadsen
Create ClusterRoleBinding and Role for alertmanager (#475)
abe966b 
* Create ClusterRoleBinding and Role for alertmanager

Create appropriate ClusterRoleBinding and Role for alertmanager-stf,
breaking out SCC into a Role vs ClusterRole to keep things in alignment
to prometheus-stf RBAC setup.

* Adjust smoketest.sh for SNMP webhook test failures

Adjust the smoketest script to also fail when the SNMP webhook test has
failed. Add a wait condition for the curl pod to complete so logs can be
retrieved.

* Add *RoleBinding rescue capabilities

If changes happen to the ClusterRoleBinding or RoleBinding then
generally the system is not going to allow you to patch the object. Adds
block/rescue logic to remove the existing ClusterRoleBinding or
RoleBinding before creating it when patching the object fails.
@leifmadsen
Copy link
Member

LET'S GO!!!!!!!

@leifmadsen leifmadsen merged commit 805ada4 into master Sep 21, 2023
11 checks passed
@leifmadsen leifmadsen deleted the issue/306 branch September 21, 2023 14:54
@leifmadsen leifmadsen mentioned this pull request Oct 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csibbitt csibbitt csibbitt approved these changes

@vkmc vkmc Awaiting requested review from vkmc

@leifmadsen leifmadsen Awaiting requested review from leifmadsen

@ayefimov-1 ayefimov-1 Awaiting requested review from ayefimov-1

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

STF won't install without cluster-monitoring-operator installed (workaround within)
3 participants
@elfiesmelfie @leifmadsen @csibbitt