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

Update required resource permission reference #528

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Update required resource permission reference #528

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a130c14
Update required resource permission reference
leifmadsen Feb 6, 2024
d80454f
Merge branch 'master' into fixup/fix-grafana-resource-ref
leifmadsen Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc-Service-Telemetry-Framework/modules/proc_accessing-uis-for-stf-components.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ You need the following permissions to access the corresponding component UI's:

[source,json,options="nowrap"]
----
{"namespace":"service-telemetry", "resource":"grafana", "group":"integreatly.org", "verb":"get"}
{"namespace":"service-telemetry", "resource":"grafana", "group":"grafana.integreatly.org", "verb":"get"}
Copy link
Contributor

Choose a reason for hiding this comment

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

❌ Unfortunately this stuff is not automatic. The permission required to access grafana is controlled here[1], and it looks like it has not been updated.

[1] https://github.com/infrawatch/service-telemetry-operator/blob/164f0ca5d6a2b262efcc0ea2265ffc2ca1cd96e1/roles/servicetelemetry/templates/manifest_grafana.j2#L45

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Chris, I'm glad I didn't just self-merge this :)

That's the Grafana v4 deployment manifest. I ended up creating another one here: https://github.com/infrawatch/service-telemetry-operator/blob/master/roles/servicetelemetry/templates/manifest_grafana_v5.j2#L64

The permission is correct there though I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh whoops, yes, it matches there, sorry. I overlooked that we have two now - it seems that they are chosen based on which API is available in the cluster? It might simplify documentation and support if they could be the same perms in both cases, but I'm unsure if you can add a sar for "group":"grafana.integreatly.org" if the new API isn't installed. Could also just standardize on the older version though it does seem less precisely targeted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I hear you. I'm not sure what would happen if someone didn't have the Grafana Operator v4 installed (I suspect some sort of failure?). It might not be a problem with someone who migrated, but in a greenfield installation (or if they cleaned out the old CRD) I'm also not sure what would happen.

I did end up testing both v4 and v5 operators, and they both worked. Unless there is a reason to flatten this down, I'd rather avoid having to stand everything up and test both operator versions again.

Copy link
Member Author

@leifmadsen leifmadsen Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also to answer your other question, yes the logic depends on which API interface is available. It looks for grafana.integreatly.org first (signals G-O v5) and then if that doesn't exist, looks for integreatly.org (G-O v4). If it finds the old API (after not finding the new API) then it will do what it did before.

https://github.com/infrawatch/service-telemetry-operator/blob/master/roles/servicetelemetry/tasks/component_grafana.yml#L1-L2

Copy link
Contributor

@csibbitt csibbitt Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

"I did end up testing both v4 and v5 operators"

Do you mean that you tested an account with the "group":"grafana.integreatly.org" permission against both operators? I'd not considered that the less specific "group" in the v4 SAR might still match when a user is given the more specific permission. If that's the case then this is totally fine by me. If the same perms work for both cases, lets just use that!

If the perms have to be different for both cases, either we should fix that or document both cases. (or, you know, since Grafana operator is "not supported", just document the latest case and let users figure it out if they are behind the times?)

Copy link
Member Author

Choose a reason for hiding this comment

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

So to clarify:

  • I installed Grafana Operator v4, and enabled dashboarding, which used the old template, and the old SAR: worked
  • I removed Grafana Operator v4 and CRDs, and installed Grafana Operator v5, enabled dashboarding, which used the new template, and the new SAR configuration: worked

I then updated the documentation everywhere to just reference Grafana Operator v5, which is expected to be used in deployments now, with an expectation everyone needs to get on board. I have a backlog item to write a KBA to help migrate (remove then reinstall) to v5 Operator.

{"namespace":"service-telemetry", "resource":"prometheus", "group":"monitoring.rhobs", "verb":"get"}
{"namespace":"service-telemetry", "resource":"alertmanager", "group":"monitoring.rhobs", "verb":"get"}
----
Expand Down
Loading