Skip to content

Add SA to each namespace for running cron jobs #7032

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+64 −0
Open

Add SA to each namespace for running cron jobs #7032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 17 additions & 0 deletions components/konflux-rbac/base/konflux-cron-sa-actions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: konflux-cron-sa-actions
labels:
konflux-cluster-role: "true"
rules:
- verbs:
- get
- list
- watch
- patch
apiGroups:
- appstudio.redhat.com
resources:
- snapshots
1 change: 1 addition & 0 deletions components/konflux-rbac/base/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ kind: Kustomization
resources:
- appstudio-pipelines-runner.yaml
- konflux-integration-runner.yaml
- konflux-cron-sa-actions.yaml
46 changes: 46 additions & 0 deletions ...onflux-rbac/bootstrap-tenant-namespace/bootstrap-tenant-namespace-np-konflux-cron-sa.yaml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
name: init-ns-cron-sa
spec:
generateExisting: true
rules:
- name: generate-serviceaccount
match:
any:
- resources:
kinds:
- Namespace
selector:
matchLabels:
konflux-ci.dev/type: tenant
generate:
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

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

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:

kind: ServiceAccount
apiVersion: v1
name: konflux-cron-sa
namespace: '{{request.object.metadata.name}}'
synchronize: true
- name: generate-snapshot-rolebinding
match:
Comment on lines +8 to +24
Copy link
Member

Choose a reason for hiding this comment

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

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:

Copy link
Member

Choose a reason for hiding this comment

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

#7348 will mitigate this problem though

any:
- resources:
kinds:
- Namespace
selector:
matchLabels:
konflux-ci.dev/type: tenant
generate:
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

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:

kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
name: snapshot-access-binding
namespace: '{{request.object.metadata.name}}'
synchronize: true
data:
roleRef:
kind: ClusterRole
name: konflux-cron-sa-actions
apiGroup: rbac.authorization.k8s.io
subjects:
- kind: ServiceAccount
name: konflux-cron-sa
namespace: '{{request.object.metadata.name}}'