SANDBOX-1067 | feature: namespace deletion member service account#1250
Conversation
WalkthroughThe change updates Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MikelAlejoBR
force-pushed
the
SANDBOX-1067-workspace-reset-mo
branch
from
a323513 to
164c2a4
Compare
|
/retest |
The registration service uses the member service account to be able to interact with the member clusters. In order to be able to delete the user namespaces to trigger a reconciliation from the NSTemplateSet controller, we need the service account to have the "delete" permission too. SANDBOX-1067
MikelAlejoBR
force-pushed
the
SANDBOX-1067-workspace-reset-mo
branch
from
164c2a4 to
8947756
Compare
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Looks good!
Are you planning to add e2e tests in another PR and pair it with codeready-toolchain/registration-service#573 ?
Yes, that was the idea! |
|
/retest FYI- here if the paired member operator PR is not have the latest changes , the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fbm3307, mfrancisc, MikelAlejoBR, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsupport/wait/member.go`:
- Around line 2658-2660: The ClusterRole binding grants cluster-wide namespace
deletion by listing "namespaces" in the Resources slice with the "delete" verb
for the toolchaincluster-member service account; remove the "delete" verb from
that Resources entry (or drop "namespaces" entirely) and instead implement a
separate, narrower ClusterRole/ServiceAccount reserved for namespace deletion
that validates ownership before deleting (create a dedicated controller and role
for that capability rather than allowing toolchaincluster-member to delete
cluster-scoped "namespaces").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee2e8349-fc85-4056-b4a0-9f6259095545
📒 Files selected for processing (1)
testsupport/wait/member.go
| APIGroups: []string{""}, | ||
| Resources: []string{"namespaces"}, | ||
| Verbs: []string{"get", "list", "watch"}, | ||
| Verbs: []string{"delete", "get", "list", "watch"}, |
There was a problem hiding this comment.
Avoid granting cluster-wide namespace deletion to the shared member service account.
namespaces is cluster-scoped, so adding delete here lets toolchaincluster-member delete any namespace on the member cluster, not just user namespaces. That is a much larger blast radius than the PR objective describes. Prefer a narrower trigger path, or isolate this capability behind a dedicated controller/service account that validates ownership before deleting.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testsupport/wait/member.go` around lines 2658 - 2660, The ClusterRole binding
grants cluster-wide namespace deletion by listing "namespaces" in the Resources
slice with the "delete" verb for the toolchaincluster-member service account;
remove the "delete" verb from that Resources entry (or drop "namespaces"
entirely) and instead implement a separate, narrower ClusterRole/ServiceAccount
reserved for namespace deletion that validates ownership before deleting (create
a dedicated controller and role for that capability rather than allowing
toolchaincluster-member to delete cluster-scoped "namespaces").
|
|
/retest |
|
/retest quay issue |
MikelAlejoBR
merged commit ea125f1
into
codeready-toolchain:master
4 participants
The registration service uses the member service account to be able to
interact with the member clusters. In order to be able to delete the
user namespaces to trigger a reconciliation from the NSTemplateSet
controller, we need the service account to have the "delete" permission
too.
Jira ticket
[SANDBOX-1067]
Summary by CodeRabbit