Add tier for -claw namespace#1265
Conversation
WalkthroughThis PR introduces a new "claw" namespace template tier for cluster resource management alongside updates to existing base tiers. Spacerequest ResourceQuota constraints are added to dev and stage namespaces in base and base1ns tiers. The new claw tier includes namespace isolation with RBAC and network policies, cluster-wide resource quotas, admin role bindings for claw resources, and test expectations updated to reflect the new tier. ChangesNamespace Template Tiers: Spacerequest Quotas and Claw Tier
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
feature
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/templates/nstemplatetiers/claw/ns_claw.yaml`:
- Around line 24-34: The Role exec-pods currently grants overly broad verbs for
the pods/exec resource; update the Role rule in exec-pods to only include the
minimal verbs required for exec functionality (keep "create" and remove "list",
"watch", "delete", "update", and "get" if not required by your client—most exec
clients only need "create"); apply the same tightening to the equivalent
pods/exec rules in the other templates that mirror this Role (the entries in the
base and base1ns templates) so all namespaces use the least-privilege set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d3bd19ed-614a-4cde-b97c-4d8acdc46013
📒 Files selected for processing (7)
deploy/templates/nstemplatetiers/base1ns/ns_dev.yamldeploy/templates/nstemplatetiers/base1ns/spacerole_admin.yamldeploy/templates/nstemplatetiers/claw/cluster.yamldeploy/templates/nstemplatetiers/claw/ns_claw.yamldeploy/templates/nstemplatetiers/claw/spacerole_admin.yamldeploy/templates/nstemplatetiers/claw/tier.yamlpkg/templates/nstemplatetiers/nstemplatetier_generator_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: govulncheck
- GitHub Check: GolangCI Lint
- GitHub Check: test
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
deploy/templates/nstemplatetiers/claw/cluster.yamldeploy/templates/nstemplatetiers/base1ns/spacerole_admin.yamldeploy/templates/nstemplatetiers/claw/tier.yamlpkg/templates/nstemplatetiers/nstemplatetier_generator_test.godeploy/templates/nstemplatetiers/claw/spacerole_admin.yamldeploy/templates/nstemplatetiers/claw/ns_claw.yamldeploy/templates/nstemplatetiers/base1ns/ns_dev.yaml
🔀 Multi-repo context codeready-toolchain/member-operator, codeready-toolchain/api, codeready-toolchain/toolchain-e2e, codeready-toolchain/registration-service, codeready-toolchain/toolchain-common
codeready-toolchain/member-operator
- Role/CRD references for spacerequests:
- deploy/templates/toolchaincluster/member-sa.yaml: lines showing RBAC resources include "spacerequests", "spacerequests/finalizers", "spacerequests/status" — implies member-operator service account permissions include spacerequests. [::codeready-toolchain/member-operator::deploy/templates/toolchaincluster/member-sa.yaml:42,48,54]
- config/crd/bases/toolchain.dev.openshift.com_spacerequests.yaml: CRD for spacerequests present. [::codeready-toolchain/member-operator::config/crd/bases/toolchain.dev.openshift.com_spacerequests.yaml:7]
- config/manifests/bases/member-operator.clusterserviceversion.yaml: lists spacerequests CRD in CSV. [::codeready-toolchain/member-operator::config/manifests/bases/member-operator.clusterserviceversion.yaml:52]
codeready-toolchain/api
- spacerequest API types and docs:
- api/v1alpha1/spacerequest_types.go: SpaceRequestSpec/Status type definitions. [::codeready-toolchain/api::api/v1alpha1/spacerequest_types.go:19,42,93]
- api/v1alpha1/zz_generated.deepcopy.go and zz_generated.openapi.go: generated deepcopy and OpenAPI defs for SpaceRequest types. [::codeready-toolchain/api::api/v1alpha1/zz_generated.deepcopy.go:2788,2797; ::api/v1alpha1/zz_generated.openapi.go:110,3889]
- make/generate.mk: spacerequests included in HOST/MEMBER CRD lists. [::codeready-toolchain/api::make/generate.mk:7-8]
- docs apiref includes SpaceRequest entries. [::codeready-toolchain/api::api/v1alpha1/docs/apiref.adoc:2950-3037]
codeready-toolchain/toolchain-e2e
- Tests and helpers touching spacerequests and the new quota key:
- testsupport/wait/member.go: expects resources "spacerequests", "spacerequests/finalizers", "spacerequests/status" in wait helpers. [::codeready-toolchain/toolchain-e2e::testsupport/wait/member.go:2639,2644,2649]
- testsupport/tiers/checks.go: parses ResourceQuota hard value for "count/spacerequests.toolchain.dev.openshift.com" (line showing parse of spaceRequestLimit). Tests will validate quota value. [::codeready-toolchain/toolchain-e2e::testsupport/tiers/checks.go:737]
- deploy/nstemplatetiers/appstudio/ns_tenant.yaml: example quota already sets count/spacerequests.toolchain.dev.openshift.com: "32" — shows existing usage pattern and expectations. [::codeready-toolchain/toolchain-e2e::deploy/nstemplatetiers/appstudio/ns_tenant.yaml:77]
- make/clean.mk: deletes spacerequests across namespaces during cleanup. [::codeready-toolchain/toolchain-e2e::make/clean.mk:24]
codeready-toolchain/registration-service and codeready-toolchain/toolchain-common
- No matches for "spacerequests" or "claw" found in quick search outputs. [::codeready-toolchain/registration-service::], [::codeready-toolchain/toolchain-common::]
Implications (observed from code references)
- The added ResourceQuota key count/spacerequests.toolchain.dev.openshift.com will be validated/parsed by toolchain-e2e tests (testsupport/tiers/checks.go) and conflicts with existing quotas (e.g., appstudio sets "32"); adding a quota of "1" in a new template may require updating tests or expectations where tiers are asserted.
- RBAC/CRD presence indicates spacerequests is an established API; adding Role granting CRUD on spacerequests (in templates) aligns with existing uses, but ensure member-operator and registration-service RBAC/CSV remain compatible.
🔇 Additional comments (7)
deploy/templates/nstemplatetiers/base1ns/ns_dev.yaml (1)
123-130: LGTM!deploy/templates/nstemplatetiers/base1ns/spacerole_admin.yaml (1)
53-82: LGTM!deploy/templates/nstemplatetiers/claw/spacerole_admin.yaml (1)
7-83: LGTM!deploy/templates/nstemplatetiers/claw/tier.yaml (1)
1-24: LGTM!pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go (1)
29-35: LGTM!Also applies to: 37-43
deploy/templates/nstemplatetiers/claw/cluster.yaml (1)
29-29: ⚡ Quick winRequest the missing
<review_comment>text — the message doesn’t include the original review comment to rewrite. Paste the full<review_comment>...</review_comment>content so I can update it based on code verification.deploy/templates/nstemplatetiers/claw/ns_claw.yaml (1)
141-154: ⚡ Quick winProvide the original review comment text (the contents inside the
<review_comment>tags) so I can rewrite it correctly.
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - secrets | ||
| verbs: | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete |
There was a problem hiding this comment.
isn't it missing read permissions?
There was a problem hiding this comment.
No, it's by design. OpenClaw will have user's permissions in the -dev namespace. So we don't want it to indirectly be able to read the secrets from -claw namespace.
There was a problem hiding this comment.
how would it be able to do that? this is Role granted to user, it's not visible anywhere in the -dev namespace.
There was a problem hiding this comment.
Yeah.. you are probably actually right. I'm still thinking about that space manager SA in the -dev namespace which we don't have. Let me think about it more carefully.
There was a problem hiding this comment.
Addressed here: e75981c
But we will also need to create a SA for the claw in the -dev namespace: claw-workspace SA. This SA will be used by OpenClaw to access -dev namespace. See https://github.com/codeready-toolchain/sandbox-claw-operator/blob/master/docs/proposals/namespace-isolation-design.md#claw-workspace-serviceaccount
I think we can stick with the option of creating this SA by Dashboard when provisioning the claw (vs. adding it to the namespace template). At least for now.
There was a problem hiding this comment.
yup, SA in the -dev namespace is a necessary piece of the puzzle, but it doesn't open any risks in terms of accessing secrets in -claw namespace, because SR shouldn't create the namespace-manager namespace in the -dev namespace when its name is not defined in the tier
host-operator/controllers/spacerequest/spacerequest_controller.go
Lines 113 to 121 in f9148fc
and
host-operator/controllers/spacerequest/spacerequest_controller.go
Lines 438 to 444 in f9148fc
but it's worth double-checking it in stage cluster
|
coderabbitai
Bot
removed
feature
There was a problem hiding this comment.
♻️ Duplicate comments (1)
deploy/templates/nstemplatetiers/claw/spacerole_admin.yaml (1)
50-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not grant end users broad
secretsCRUD in the-clawnamespace.This allows
${USERNAME}to read/modify/delete all namespace secrets, which is a high-impact credential exposure risk and was a previously raised concern in this PR discussion. Prefer removing this rule (or narrowing it to the minimum strictly required, if any).Proposed minimal hardening
- apiGroups: - "" resources: - - secrets - verbs: - - get - - list - - watch - - create - - update - - patch - - delete + - secrets + verbs: []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/templates/nstemplatetiers/claw/spacerole_admin.yaml` around lines 50 - 58, The role currently grants full CRUD on the resource "secrets" (the verbs block under "secrets") which exposes all namespace secrets to end users; remove the "secrets" resource entry entirely from the Role in spacerole_admin.yaml, or if truly required, replace it with a tightly scoped rule that uses "resourceNames" for only the specific secret names and minimal verbs (e.g., read-only "get"/"watch" if needed) instead of create/update/patch/delete. Locate the Role definition (the entry containing the "secrets" resource and its verbs) and either delete that resource block or change it to a minimal, explicit set of resourceNames and verbs to eliminate broad CRUD access.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@deploy/templates/nstemplatetiers/claw/spacerole_admin.yaml`:
- Around line 50-58: The role currently grants full CRUD on the resource
"secrets" (the verbs block under "secrets") which exposes all namespace secrets
to end users; remove the "secrets" resource entry entirely from the Role in
spacerole_admin.yaml, or if truly required, replace it with a tightly scoped
rule that uses "resourceNames" for only the specific secret names and minimal
verbs (e.g., read-only "get"/"watch" if needed) instead of
create/update/patch/delete. Locate the Role definition (the entry containing the
"secrets" resource and its verbs) and either delete that resource block or
change it to a minimal, explicit set of resourceNames and verbs to eliminate
broad CRUD access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ebaac59b-fc7d-4119-b4cb-788fc8cd684b
📒 Files selected for processing (3)
deploy/templates/nstemplatetiers/base/ns_dev.yamldeploy/templates/nstemplatetiers/base/ns_stage.yamldeploy/templates/nstemplatetiers/claw/spacerole_admin.yaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: GolangCI Lint
- GitHub Check: test
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
deploy/templates/nstemplatetiers/base/ns_dev.yamldeploy/templates/nstemplatetiers/claw/spacerole_admin.yamldeploy/templates/nstemplatetiers/base/ns_stage.yaml
🔀 Multi-repo context codeready-toolchain/member-operator, codeready-toolchain/api, codeready-toolchain/toolchain-e2e, codeready-toolchain/registration-service, codeready-toolchain/toolchain-common
codeready-toolchain/member-operator
- RBAC grants for spacerequests in member SA template: deploy/templates/toolchaincluster/member-sa.yaml (lines showing "spacerequests", "spacerequests/finalizers", "spacerequests/status"). [::codeready-toolchain/member-operator::deploy/templates/toolchaincluster/member-sa.yaml:42,48,54]
- CRD present for spacerequests: config/crd/bases/toolchain.dev.openshift.com_spacerequests.yaml. [::codeready-toolchain/member-operator::config/crd/bases/toolchain.dev.openshift.com_spacerequests.yaml:7]
- CSV references spacerequests CRD: config/manifests/bases/member-operator.clusterserviceversion.yaml. [::codeready-toolchain/member-operator::config/manifests/bases/member-operator.clusterserviceversion.yaml:52]
codeready-toolchain/api
- SpaceRequest API/types/docs exist; spacerequest types and apiref are present (api/v1alpha1 docs and generation makefile entries). Relevant files: api/v1alpha1/docs/apiref.adoc and make/generate.mk. [::codeready-toolchain/api::api/v1alpha1/docs/apiref.adoc:2950-3037][::codeready-toolchain/api::make/generate.mk:7-8]
codeready-toolchain/toolchain-e2e
- Tests parse/validate the ResourceQuota key added by this PR:
- testsupport/tiers/checks.go reads spec.Hard["count/spacerequests.toolchain.dev.openshift.com"] and parses quantity (implication: tests will validate the quota value). [::codeready-toolchain/toolchain-e2e::testsupport/tiers/checks.go:737]
- Test helpers expect spacerequests resources in waits: testsupport/wait/member.go (resources: "spacerequests", "spacerequests/finalizers", "spacerequests/status"). [::codeready-toolchain/toolchain-e2e::testsupport/wait/member.go:2639,2644,2649]
- Existing tier template sets a different spacerequests quota: deploy/nstemplatetiers/appstudio/ns_tenant.yaml sets count/spacerequests.toolchain.dev.openshift.com: "32" (shows precedent and potential test expectations). [::codeready-toolchain/toolchain-e2e::deploy/nstemplatetiers/appstudio/ns_tenant.yaml:77]
- Cleanup target deletes spacerequests across namespaces: make/clean.mk. [::codeready-toolchain/toolchain-e2e::make/clean.mk:24]
codeready-toolchain/registration-service
- No matches for "spacerequests" or "claw" found in quick search results. [::codeready-toolchain/registration-service::]
codeready-toolchain/toolchain-common
- No matches for "spacerequests" or "claw" found in quick search results. [::codeready-toolchain/toolchain-common::]
Implications (concise)
- Member-operator and API already define and handle spacerequests CRD and RBAC — adding quota key aligns with existing API surface.
- toolchain-e2e contains parsing and assertions for the exact ResourceQuota key; adding a quota of "1" for the new claw tier may cause tests that assert specific quota values (or rely on different defaults like "32") to fail or require updates.
🔇 Additional comments (3)
deploy/templates/nstemplatetiers/base/ns_dev.yaml (1)
79-87: LGTM!deploy/templates/nstemplatetiers/base/ns_stage.yaml (1)
79-87: LGTM!deploy/templates/nstemplatetiers/claw/spacerole_admin.yaml (1)
7-21: LGTM!Also applies to: 43-47
|
/retest |
| - apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy |
There was a problem hiding this comment.
I'm wondering if we would need also a NetworkPolicy for communication between -claw and -dev namespace, but probably not. WDYT?
There was a problem hiding this comment.
It might be useful so the claw could access services running in -dev. But we would also need to whitelist internal cluster traffic in the proxy to make it work.
PR for the proxy side: codeready-toolchain/claw-operator#139
I also added the NP creation by Dashboard to the design: https://github.com/codeready-toolchain/sandbox-claw-operator/blob/master/docs/proposals/namespace-isolation-checklist.md#8-dashboard-flow
I would probably avoid adding this NP to the -dev template so we don't introduce another NP for all users even if they don't deploy any Claws.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, xcoulon 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 |
alexeykazakov
merged commit d757b66
into
codeready-toolchain:master
3 participants
Related to https://github.com/codeready-toolchain/sandbox-sre/pull/3177
Paired with codeready-toolchain/toolchain-e2e#1282
Summary by CodeRabbit
New Features
Configuration
Tests