-
Notifications
You must be signed in to change notification settings - Fork 314
chore(KFLUXSPRT-3001): konflux-support read access in all namespaces #8750
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.
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
base: main
Are you sure you want to change the base?
Conversation
🤖 Gemini AI Assistant AvailableHi @Omeramsc! I'm here to help with your pull request. You can interact with me using the following commands: Available Commands
How to Use
PermissionsOnly OWNER, MEMBER, or COLLABORATOR users can trigger my responses. This ensures secure and appropriate usage. This message was automatically added to help you get started with the Gemini AI assistant. Feel free to delete this comment if you don't need assistance. |
|
🤖 Hi @Omeramsc, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
As a heads up, looks like yamllint throws a few warnings from your changes. |
...ux-rbac/konflux-support-viewer-access/generate-support-viewer-rolebinding-clusterpolicy.yaml
Outdated
Show resolved
Hide resolved
...ux-rbac/konflux-support-viewer-access/generate-support-viewer-rolebinding-clusterpolicy.yaml
Outdated
Show resolved
Hide resolved
f7f9922 to
8eb6cca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work! 👍🏾
This PR is updating dev/staging/production at the same time. We should target dev/stage first, then production. Can you split them?
These kind of policies (synchronize, generateExisting) can put a lot of pressure on kyverno and on the cluster, we should check kyverno performance and -in case- increase resource requests/limits.
components/policies/development/konflux-rbac/konflux-support-viewer-access/OWNERS
Show resolved
Hide resolved
...ux-rbac/konflux-support-viewer-access/generate-support-viewer-rolebinding-clusterpolicy.yaml
Outdated
Show resolved
Hide resolved
...ux-rbac/konflux-support-viewer-access/generate-support-viewer-rolebinding-clusterpolicy.yaml
Show resolved
Hide resolved
...ux-rbac/konflux-support-viewer-access/generate-support-viewer-rolebinding-clusterpolicy.yaml
Outdated
Show resolved
Hide resolved
...ux-rbac/konflux-support-viewer-access/generate-support-viewer-rolebinding-clusterpolicy.yaml
Outdated
Show resolved
Hide resolved
introduces a ClusterPolicy to automatically generate the konflux-read-only-binding RoleBinding in all application namespaces, granting konflux-sre and ai-konflux-user-support view access via the konflux-viewer-user-actions ClusterRole. - The policy explicitly uses synchronize: true and background: true, overriding general Kyverno performance best practices. This is intentional to ensure non-negotiable support access: - background: true: Required for immediate retroactive application to all existing Konflux tenant namespaces. - synchronize: true: Required to make the RoleBinding self-healing. If an application user or process deletes the binding, Kyverno automatically reinstates it, guaranteeing persistent visibility for SRE/Support teams. Assisted-by: Cursor Signed-off-by: Omer Turner <[email protected]>
8eb6cca to
08fdfd2
Compare
|
added #8841 with changes to production |
|
we already have [1] https://github.com/redhat-appstudio/infra-deployments/blob/main/components/authentication/base/everyone-can-view.yaml why do we need to use a policy for generating the role bindings? is it going to replace [1]? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: filariow, Omeramsc 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:
Approvers can indicate their approval by writing |
|
/test appstudio-upgrade-tests /hold |
|
@gbenhaim I wasn't aware of that policy, but it seems like the two serve similar, but different purposes. the new policy also is specific to the two groups, konflux-sre and ai-konflux-user-support. although as mention before we may make it even more, user-specific, to have access via the UI as well. |
Everyone-can-view clusterrole that we grant to all Konflux dev is different that this policy. everyone-can-view basically grant view access to a lot of resources in the OCP cluster and having access to those can help troubleshooting things but it does not grant access to the tenants NS from Konflux UI, which this PR is addressing. everyone-can view cover the use case of Konflux dev accessing tenants ns using back door but also when they need to access Konflux control plane resources, pods, logs,... of the Konflux services. We can always get rid of it but we will need to beef up the permissions granted for each konflux components before removing it otherwise we cannot troubleshoot Konflux issues |
|
| but it does not grant access to the tenants NS from Konflux UI, which this PR is addressing. This pr is one step in this way, but not a full implementation since it gives the permissions to groups which the ui currently not support. |
|
/lgtm |
|
@gbenhaim I think we should start with this approach, once this is merged I'll see how can we set all the required user, and then proceed with the same approach on prod. |
|
New changes are detected. LGTM label has been removed. |
|
@Omeramsc: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
introduces a ClusterPolicy to automatically generate the konflux-read-only-binding RoleBinding in all application namespaces, granting konflux-sre and ai-konflux-user-support view access via the konflux-viewer-user-actions ClusterRole.
The policy explicitly uses synchronize: true and background: true, overriding general Kyverno performance best practices. This is intentional to ensure non-negotiable support access:
background: true: Required for immediate retroactive application to all existing Konflux tenant namespaces.
synchronize: true: Required to make the RoleBinding self-healing. If an application user or process deletes the binding, Kyverno automatically reinstates it, guaranteeing persistent visibility for SRE/Support teams.
Assisted-by: Cursor