Add SA to each namespace for running cron jobs #7032

brianwcook · 2025-07-07T14:38:40Z

This PR supports adding a UI for https://konflux-ci.dev/docs/testing/integration/periodic-integration-tests/ In order for the UI to work an SA with correct permissions to mutate snapshots must exist in the namespace with a predictable name.

openshift-ci · 2025-07-07T14:38:51Z

Hi @brianwcook. Thanks for your PR.

I'm waiting for a redhat-appstudio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

gbenhaim · 2025-07-08T12:05:24Z

with this permission users can change the snapshot content. are we ok with it?

brianwcook · 2025-07-08T12:23:19Z

Users (maintainers and admins) can already change snapshot content. This gives us a service account that can do it so that we can run ITS jobs with cron.

arewm · 2025-07-08T18:02:58Z

Users cannot change Snapshots. If they want to change the content, they need to create a new one. The intention for Snapshots is to have some immutable reference which can be used for testing and releasing. If we expose the ability to modify them (even accidentally), we introduce the possibility for inconsistencies.

Why do we need to mutate the snapshots?

arewm · 2025-07-08T18:07:01Z

but the ClusterRole for admins has:

infra-deployments/components/konflux-rbac/staging/base/konflux-admin-user-actions.yaml

Lines 24 to 35 in 4e205ca

    
           - verbs: 
        
               - get 
        
               - list 
        
               - watch 
        
               - create 
        
               - update 
        
               - patch 
        
               - delete 
        
             apiGroups: 
        
               - appstudio.redhat.com 
        
             resources: 
        
               - snapshots

@gbenhaim , how do we protect against changing Snapshots today?

brianwcook · 2025-07-08T18:18:55Z

the issue is that we need to be able to add a label to an existing snapshot as described here in order to kick off the ITS. I am following the instructions given on creating an SA and what permissions to assign to it. I think that if we are worried about this PR, then we should also be worried about the advice given in those docs.

arewm · 2025-07-09T04:16:04Z

This also appears to be consistent with https://github.com/konflux-ci/architecture/blob/main/ADR/0011-roles-and-permissions.md. It seems like we might need additional protections in place to protect against mutating the components.

brianwcook · 2025-07-09T14:41:47Z

@gbenhaim we decided to add a new webhook to make snapshot components immutable.
konflux-ci/integration-service#1212

gbenhaim · 2025-07-11T10:27:19Z

...e/konflux-rbac/bootstrap-tenant-namespace/bootstrap-tenant-namespace-np-konflux-cron-sa.yaml

+              matchLabels:
+                konflux-ci.dev/type: tenant
+      generate:
+        kind: Role


I think it would be better to create a single cluster role in this application https://github.com/redhat-appstudio/infra-deployments/tree/main/components/konflux-rbac that will be referenced by the role in each tenant namespace.

Also, the group name in apiGroups should be changed

ok, I made those changes and rebase. Also, the validating webhook to block mutation of snapshots has been merged.

This PR supports adding a UI for https://konflux-ci.dev/docs/testing/integration/periodic-integration-tests/ In order for the UI to work an SA with correct permissions to mutate snapshots must exist in the namespace with a predictable name. Signed-off-by: Brian Cook <[email protected]>

openshift-ci · 2025-07-17T13:57:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: brianwcook
Once this PR has been reviewed and has the lgtm label, please assign filariow for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

components/konflux-rbac/OWNERS
components/policies/OWNERS

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

brianwcook · 2025-07-25T18:00:19Z

Can this proceed now?

filariow · 2025-07-28T08:22:12Z

...e/konflux-rbac/bootstrap-tenant-namespace/bootstrap-tenant-namespace-np-konflux-cron-sa.yaml

+    - name: generate-serviceaccount
+      match:
+        any:
+        - resources:
+            kinds:
+            - Namespace
+            selector:
+              matchLabels:
+                konflux-ci.dev/type: tenant
+      generate:
+        kind: ServiceAccount
+        apiVersion: v1
+        name: konflux-cron-sa
+        namespace: '{{request.object.metadata.name}}'
+        synchronize: true
+    - name: generate-snapshot-rolebinding
+      match:


the skipBackgroundRequests field needs to be explicitly set otherwise ArgoCD will always consider the ClusterPolicy OutOfSync

Suggested change

- name: generate-serviceaccount

match:

any:

- resources:

kinds:

- Namespace

selector:

matchLabels:

konflux-ci.dev/type: tenant

generate:

kind: ServiceAccount

apiVersion: v1

name: konflux-cron-sa

namespace: '{{request.object.metadata.name}}'

synchronize: true

- name: generate-snapshot-rolebinding

match:

- name: generate-serviceaccount

skipBackgroundRequests: true

match:

any:

- resources:

kinds:

- Namespace

selector:

matchLabels:

konflux-ci.dev/type: tenant

generate:

kind: ServiceAccount

apiVersion: v1

name: konflux-cron-sa

namespace: '{{request.object.metadata.name}}'

synchronize: true

- name: generate-snapshot-rolebinding

skipBackgroundRequests: true

match:

#7348 will mitigate this problem though

filariow · 2025-07-28T08:25:26Z

...e/konflux-rbac/bootstrap-tenant-namespace/bootstrap-tenant-namespace-np-konflux-cron-sa.yaml

we need to add this ClusterPolicy to the kustomization.yaml file, otherwise it won't be deployed.

filariow · 2025-07-28T08:27:54Z

...e/konflux-rbac/bootstrap-tenant-namespace/bootstrap-tenant-namespace-np-konflux-cron-sa.yaml

we should update the tests to check the resource is correctly created. Something like

infra-deployments/components/policies/staging/base/konflux-rbac/bootstrap-tenant-namespace/.chainsaw-test/chainsaw-test.yaml

Lines 50 to 54 in de5e61a

- name: then-resources-are-created

try:

- assert:

file: resources/expected-resources.yaml

template: true

filariow · 2025-07-28T08:30:20Z

...e/konflux-rbac/bootstrap-tenant-namespace/bootstrap-tenant-namespace-np-konflux-cron-sa.yaml

+                konflux-ci.dev/type: tenant
+      generate:


for how the ClusterPolicy is now, it will react to any Update events. We are only interested in Update events that add the tenant label. We can restrict this way:

Suggested change

konflux-ci.dev/type: tenant

generate:

konflux-ci.dev/type: tenant

celPreconditions:

- name: "on update, oldObject had no konflux-ci.dev/type=tenant label"

expression: "request.operation != UPDATE || ! (has(oldObject.metadata.labels) && 'konflux-ci.dev/type' in oldObject.metadata.labels && oldObject.metadata.labels['konflux-ci.dev/type] == 'tenant')"

generate:

filariow · 2025-07-28T08:30:42Z

...e/konflux-rbac/bootstrap-tenant-namespace/bootstrap-tenant-namespace-np-konflux-cron-sa.yaml

+                konflux-ci.dev/type: tenant
+      generate:


for how the ClusterPolicy is now, it will react to any Update events. We are only interested in Update events that add the tenant label. We can restrict this way:

Suggested change

konflux-ci.dev/type: tenant

generate:

konflux-ci.dev/type: tenant

celPreconditions:

- name: "on update, oldObject had no konflux-ci.dev/type=tenant label"

expression: "request.operation != UPDATE || ! (has(oldObject.metadata.labels) && 'konflux-ci.dev/type' in oldObject.metadata.labels && oldObject.metadata.labels['konflux-ci.dev/type] == 'tenant')"

generate:

brianwcook · 2025-07-29T22:00:49Z

Just checking do you want me to proceed and make the changes for this PR, or would you rather us just use the default SA (assuming it has or can be given the proper privs)?

openshift-ci bot requested review from filariow and gbenhaim July 7, 2025 14:38

openshift-ci bot added the needs-ok-to-test label Jul 7, 2025

brianwcook mentioned this pull request Jul 7, 2025

feat: scheduled integration tests feature konflux-ci/konflux-ui#292

Open

10 tasks

gbenhaim requested changes Jul 11, 2025

View reviewed changes

brianwcook force-pushed the add-konflux-cron-sa branch from d0ff364 to 1b1cf87 Compare July 17, 2025 13:56

filariow reviewed Jul 28, 2025

View reviewed changes

	- name: then-resources-are-created
	try:
	- assert:
	file: resources/expected-resources.yaml
	template: true

Add SA to each namespace for running cron jobs #7032

Are you sure you want to change the base?

Add SA to each namespace for running cron jobs #7032

Conversation

brianwcook commented Jul 7, 2025

Uh oh!

openshift-ci bot commented Jul 7, 2025

Uh oh!

gbenhaim commented Jul 8, 2025

Uh oh!

brianwcook commented Jul 8, 2025

Uh oh!

arewm commented Jul 8, 2025

Uh oh!

arewm commented Jul 8, 2025

Uh oh!

brianwcook commented Jul 8, 2025

Uh oh!

arewm commented Jul 9, 2025

Uh oh!

brianwcook commented Jul 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jul 17, 2025

Uh oh!

brianwcook commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianwcook commented Jul 29, 2025

Uh oh!

Uh oh!