Fix noobaa-core routes permission issue#1671
Conversation
Signed-off-by: Romy <35330373+romayalon@users.noreply.github.com>
WalkthroughAdds OpenShift Route access to the noobaa-core Role in deploy/role_core.yaml and updates the embedded bundle in pkg/bundle/deploy.go, including the role rules and the corresponding SHA256 constant. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
deploy/role_core.yaml (2)
48-57: If setting a custom host on Routes, include routes/custom-host resourceOpenShift requires explicit permission on routes/custom-host when setting spec.host to a custom value. If NooBaa sets or updates route.spec.host, extend resources accordingly.
Proposed change:
resources: - routes + - routes/custom-hostVerbs can mirror those granted for routes (get, create, update, patch, list, watch). If you never set spec.host, you can skip this.
6-39: Non-blocking: tighten wildcards in existing rules (separate PR recommended)Static analysis flagged wildcard usage. Not introduced here, but consider narrowing resources/verbs to least-privilege, especially for core Kubernetes and apps groups.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/role_core.yaml(1 hunks)pkg/bundle/deploy.go(2 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
deploy/role_core.yaml
[MEDIUM] 1-57: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
⏰ 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). (1)
- GitHub Check: run-cli-tests
🔇 Additional comments (1)
deploy/role_core.yaml (1)
48-57: RBAC verbs for route.openshift.io/routes are sufficient as-isWe scanned all Go files importing
github.com/openshift/api/route/v1and found no calls toPatch()against Routes. Since no code path currently issues PATCH requests, the existing verbs (get, create, update, list, watch) meet today’s needs.If you later introduce client-side or server-side patch operations (e.g.,
client.Patch()orApplywith server-side-apply), revisit and add:
patch- (optional)
delete/deletecollectionfor cleanup flows
| - apiGroups: | ||
| - route.openshift.io | ||
| resources: | ||
| - routes | ||
| verbs: | ||
| - get | ||
| - create | ||
| - update | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Right-size verbs by scanning actual client calls (create/update/patch/delete)
Let’s verify the exact operations used against Route resources so we don’t over/under-grant.
🏁 Script executed:
#!/bin/bash
# From repo root: scan for Route usage and the specific client methods invoked.
# Assumes Go code; adjust if needed.
set -euo pipefail
echo "== Find route API references =="
rg -n -i 'route\.openshift\.io|Route\b' -A 4 -B 4 || true
echo
echo "== Find typed client usages for Routes (Create/Update/Patch/Delete) =="
# Common client-go patterns: client.RouteV1().Routes(ns).Create/Update/Patch/Delete
rg -n -i '\.Routes\(' -A 3 -B 3 || true
rg -n -i '\.(Create|Update|Patch|Delete)\(' -A 2 -B 2 | rg -i 'Route|Routes' || true
echo
echo "== Look for spec.host assignments =="
rg -n -i 'spec\.host' -A 2 -B 2 || true
echo
echo "== Look for server-side apply/patch patterns =="
rg -n -i 'Apply|ApplyStatus|Patch|PatchStatus' -A 2 -B 2 | rg -i 'route|routes' || trueLength of output: 27384
🏁 Script executed:
#!/bin/bash
# Inspect ReconcileObject and ReconcileObjectOptional implementations
set -euo pipefail
echo "== ReconcileObject =="
rg -n "func (r \*Reconciler\) ReconcileObject" -A 30 pkg/system/reconciler.go || true
echo
echo "== ReconcileObjectOptional =="
rg -n "func (r \*Reconciler\) ReconcileObjectOptional" -A 30 pkg/system/reconciler.go || trueLength of output: 529
Add missing ‘patch’ and ‘delete’ verbs to the Route RBAC rule
The operator’s reconciliation logic uses server-side apply (patch) and will delete Routes when disabled, so the role must allow both patch and delete on route.openshift.io/routes.
• File: deploy/role_core.yaml
• Lines: 48–57
Replace this block:
- apiGroups:
- route.openshift.io
resources:
- routes
verbs:
- get
- create
- update
- list
- watchWith:
- apiGroups:
- - route.openshift.io
- resources:
- - routes
- verbs:
- - get
- - create
- - update
- - list
- - watch
+ apiGroups:
+ - route.openshift.io
+ resources:
+ - routes
+ verbs:
+ - get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 1-57: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
🤖 Prompt for AI Agents
In deploy/role_core.yaml around lines 48 to 57, the RBAC rule for apiGroup
route.openshift.io resources routes is missing the 'patch' and 'delete' verbs
required by the operator (server-side apply and route deletion). Update the
verbs list to include both patch and delete in addition to the existing get,
create, update, list, and watch so the role permits patch and delete operations
on route.openshift.io/routes.
| ` | ||
|
|
||
| const Sha256_deploy_role_core_yaml = "c3cfb5b87298224fd6e4e4bff32d3948ad168a0110b8569118a260739ef5d5e7" | ||
| const Sha256_deploy_role_core_yaml = "1ec420603dcec64b247852d106535a85a1a866129f78f790c2e5c9285f029ae7" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify embedded manifest checksum consistency
The SHA256 for deploy_role_core.yaml was updated. Please verify the constant matches the embedded YAML to avoid bundle integrity/runtime mismatches.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract the embedded YAML into a temp file and compute its sha256sum
awk '/^const File_deploy_role_core_yaml = `/{flag=1; next} /^`$/{flag=0} flag {print}' pkg/bundle/deploy.go > /tmp/deploy_role_core.yaml
# Compute sha256
SHA=$(sha256sum /tmp/deploy_role_core.yaml | awk '{print $1}')
echo "Computed: $SHA"
echo "Expected: 1ec420603dcec64b247852d106535a85a1a866129f78f790c2e5c9285f029ae7"
test "$SHA" = "1ec420603dcec64b247852d106535a85a1a866129f78f790c2e5c9285f029ae7" && echo "OK: checksum matches" || { echo "ERROR: checksum mismatch"; exit 1; }Length of output: 296
Verify embedded manifest SHA256 and tighten noobaa-core Role permissions
Please run the following updated script to ensure the constant matches the embedded YAML (using a tool available in the sandbox), then address the over-privileged Route verbs in the noobaa-core Role.
• Security adjustment: The newly added Rule for noobaa-core grants create/update on OpenShift Routes. Unless this workload genuinely needs to write routes, limit it to read-only verbs:
--- deploy/role_core.yaml
+++ deploy/role_core.yaml
@@ -XX,7 +XX,7 @@ rules:
- apiGroups:
- route.openshift.io
resources:
- - routes
- verbs: ["get","list","watch","create","update"]
+ - routes
+ verbs: ["get","list","watch"]If the core component requires write access (or delete/patch on teardown), please document that need or introduce only the minimal additional verbs.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract the embedded YAML into a temp file
awk '/^const File_deploy_role_core_yaml = `/{flag=1; next} /^`$/{flag=0} flag {print}' pkg/bundle/deploy.go > /tmp/deploy_role_core.yaml
# Compute SHA256 using whichever tool is available
if command -v openssl &>/dev/null; then
SHA=$(openssl dgst -sha256 /tmp/deploy_role_core.yaml | awk '{print $2}')
elif command -v shasum &>/dev/null; then
SHA=$(shasum -a 256 /tmp/deploy_role_core.yaml | awk '{print $1}')
else
echo "ERROR: no SHA-256 tool available" >&2
exit 1
fi
echo "Computed: $SHA"
echo "Expected: 1ec420603dcec64b247852d106535a85a1a866129f78f790c2e5c9285f029ae7"
if [[ "$SHA" == "1ec420603dcec64b247852d106535a85a1a866129f78f790c2e5c9285f029ae7" ]]; then
echo "OK: checksum matches"
else
echo "ERROR: checksum mismatch" >&2
exit 1
fiLength of output: 254
Re-verify embedded manifest SHA256 and tighten noobaa-core Role permissions
Please run this updated script (falls back to Python) to confirm the constant matches the embedded YAML, then trim the noobaa-core Role to read-only Route verbs unless write access is truly required.
• Noobaa-core Role adjustment: limit Route verbs to read-only unless write is confirmed必要:
--- deploy/role_core.yaml
+++ deploy/role_core.yaml
@@ -XX,7 +XX,7 @@ rules:
- apiGroups:
- route.openshift.io
resources:
- - routes
- verbs: ["get","list","watch","create","update"]
+ - routes
+ verbs: ["get","list","watch"]If the core component genuinely needs to create/update (or delete/patch) Routes, document and add only those specific verbs.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract embedded YAML
awk '/^const File_deploy_role_core_yaml = `/{flag=1; next} /^`$/{flag=0} flag {print}' pkg/bundle/deploy.go > /tmp/deploy_role_core.yaml
# Compute SHA256: try openssl, shasum, then Python
if command -v openssl &>/dev/null; then
SHA=$(openssl dgst -sha256 /tmp/deploy_role_core.yaml | awk '{print $2}')
elif command -v shasum &>/dev/null; then
SHA=$(shasum -a 256 /tmp/deploy_role_core.yaml | awk '{print $1}')
elif command -v python3 &>/dev/null; then
SHA=$(python3 - <<'PYCODE'
import hashlib
print(hashlib.sha256(open("/tmp/deploy_role_core.yaml","rb").read()).hexdigest())
PYCODE
)
else
echo "ERROR: no SHA-256 tool available; please verify manually" >&2
exit 1
fi
echo "Computed: $SHA"
echo "Expected: 1ec420603dcec64b247852d106535a85a1a866129f78f790c2e5c9285f029ae7"
if [[ "$SHA" == "1ec420603dcec64b247852d106535a85a1a866129f78f790c2e5c9285f029ae7" ]]; then
echo "OK: checksum matches"
else
echo "ERROR: checksum mismatch" >&2
exit 1
fiLength of output: 866
Update embedded manifest SHA256 constant and tighten noobaa-core Role permissions
The SHA256 constant in pkg/bundle/deploy.go does not match the actual embedded YAML (computed: 37b6d564e5505ca67702d86bbedfca2cbc75041821e67beddc04a2588d1515ad), and the noobaa-core Role grants write access to Routes that may be unnecessary.
• pkg/bundle/deploy.go (around line 6605): update the SHA constant
- const Sha256_deploy_role_core_yaml = "1ec420603dcec64b247852d106535a85a1a866129f78f790c2e5c9285f029ae7"
+ const Sha256_deploy_role_core_yaml = "37b6d564e5505ca67702d86bbedfca2cbc75041821e67beddc04a2588d1515ad"• deploy/role_core.yaml: restrict Route verbs to read-only
rules:
- apiGroups:
- route.openshift.io
resources:
- routes
- verbs: ["get","list","watch","create","update"]
+ verbs: ["get","list","watch"]If the core workload truly needs to write (create/update/delete/patch) Routes, please document and add only those specific verbs.
| - route.openshift.io | ||
| resources: | ||
| - routes | ||
| verbs: | ||
| - get | ||
| - create | ||
| - update | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Reassess scope of Routes permissions on noobaa-core (principle of least privilege)
You’re granting noobaa-core write access to Route resources (create/update). In most operators, only the operator SA manages cluster objects; the core workload typically needs read-only at most. Unless the core pod truly creates/updates Routes, consider limiting to get/list/watch to reduce blast radius.
If write is not required, here’s a minimal change:
- verbs:
- - get
- - create
- - update
- - list
- - watch
+ verbs:
+ - get
+ - list
+ - watchIf the core does need to modify Routes, please confirm whether patch and/or delete are also required by the code path to avoid 403s at runtime.
🏁 Script executed:
#!/bin/bash
echo "=== Searching for direct usage of routev1.Route ==="
grep -R "&routev1.Route" -n .
echo "=== Searching for any reference to routev1.Route type ==="
grep -R "routev1\\.Route" -n .
echo "=== Searching for Create calls involving Route ==="
grep -R "Create.*Route" -n .
echo "=== Searching for Update calls involving Route ==="
grep -R "Update.*Route" -n .
echo "=== Searching for dynamic client usage of 'routes' resource ==="
grep -R "Resource(\"routes\"" -n .Length of output: 1563
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Locate Route fields in reconciler ==="
grep -n "RouteMgmt" -n pkg/system/reconciler.go
echo ""
echo "=== Inspect reconciler around Route fields (lines 150–260) ==="
sed -n '150,260p' pkg/system/reconciler.go
echo ""
echo "=== Find Create/Update/Patch calls in reconciler ==="
grep -n "\.Create(" -n pkg/system/reconciler.go || true
grep -n "\.Update(" -n pkg/system/reconciler.go || true
grep -n "\.Patch(" -n pkg/system/reconciler.go || true
grep -n "controllerutil\.CreateOrUpdate" -n pkg/system/reconciler.go || trueLength of output: 9465
Add patch permission for Route resources and remove unnecessary update verb
The reconciler uses controllerutil.CreateOrUpdate on Route objects, which performs a PATCH under the hood. As such, you need the patch verb—and since there are no direct HTTP PUT updates of Route objects, the update verb can be dropped. No delete calls exist for routes.
• File: pkg/bundle/deploy.go
• Lines: 6655–6663
- verbs:
- - get
- - create
- - update
- - list
- - watch
+ verbs:
+ - get
+ - list
+ - watch
+ - create
+ - patch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - route.openshift.io | |
| resources: | |
| - routes | |
| verbs: | |
| - get | |
| - create | |
| - update | |
| - list | |
| - watch | |
| - route.openshift.io | |
| resources: | |
| - routes | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| - create | |
| - patch |
🤖 Prompt for AI Agents
In pkg/bundle/deploy.go around lines 6655 to 6663, the Role for
route.openshift.io resources currently grants verbs get, create, update, list,
watch but the reconciler uses controllerutil.CreateOrUpdate which issues a
PATCH; remove the unnecessary "update" verb and add "patch" to the verbs list so
it becomes get, create, patch, list, watch for resources: routes under apiGroup
route.openshift.io.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deploy/role_core.yaml (1)
48-57: Optional tightening: scope to specific Route names if knownIf the operator only manages one/few known Route objects, consider resourceNames to reduce blast radius.
Example:
- apiGroups: - route.openshift.io resources: - routes + resourceNames: + - <insert-expected-route-name> verbs: - get ...Note: Only use if names are deterministic and won’t vary across installs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/role_core.yaml(1 hunks)pkg/bundle/deploy.go(2 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
deploy/role_core.yaml
[MEDIUM] 1-57: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
⏰ 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). (1)
- GitHub Check: run-cli-tests
🔇 Additional comments (2)
deploy/role_core.yaml (1)
48-57: Embedded bundle SHA verified
The computed SHA256 fordeploy/role_core.yamlmatches theSha256_deploy_role_core_yamlconstant inpkg/bundle/deploy.go(line 6605). No further updates are needed.pkg/bundle/deploy.go (1)
6605-6605: SHA updated — verify it matches the embedded Role YAMLThe Sha256_deploy_role_core_yaml was updated. Please verify it’s the hash of the current File_deploy_role_core_yaml to keep bundle verification/pass checks green (e.g., via your bundle generation/verification script).
| - apiGroups: | ||
| - route.openshift.io | ||
| resources: | ||
| - routes | ||
| verbs: | ||
| - get | ||
| - create | ||
| - update | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Route RBAC verbs: either tighten to least-privilege or complete for controller-style reconciliation
Right now you grant get, create, update, list, watch. In many controllers, updates are applied via PATCH/SSA and deletes happen during reconciliation/cleanup. Choose one path:
- If you only need to read Route status/host: drop create/update and keep get, list, watch.
- If the operator creates and reconciles Routes: add patch and delete to avoid 403s on SSA/patch or cleanup.
Apply one of these diffs:
Least-privilege (read-only):
verbs:
- get
- - create
- - update
- list
- watchFull reconcile set:
verbs:
- get
- create
- update
+ - patch
+ - delete
- list
- watch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - apiGroups: | |
| - route.openshift.io | |
| resources: | |
| - routes | |
| verbs: | |
| - get | |
| - create | |
| - update | |
| - list | |
| - watch | |
| # Option 1: Least-privilege (read-only) | |
| - apiGroups: | |
| - route.openshift.io | |
| resources: | |
| - routes | |
| verbs: | |
| - get | |
| - list | |
| - watch |
| - apiGroups: | |
| - route.openshift.io | |
| resources: | |
| - routes | |
| verbs: | |
| - get | |
| - create | |
| - update | |
| - list | |
| - watch | |
| # Option 2: Full reconcile set | |
| - apiGroups: | |
| - route.openshift.io | |
| resources: | |
| - routes | |
| verbs: | |
| - get | |
| - create | |
| - update | |
| - patch | |
| - delete | |
| - list | |
| - watch |
🧰 Tools
🪛 Checkov (3.2.334)
[MEDIUM] 1-57: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
🤖 Prompt for AI Agents
In deploy/role_core.yaml around lines 48 to 57, the Route RBAC verbs currently
include get, create, update, list, watch which is either too permissive for
read-only use or missing verbs for full controller reconciliation; either
tighten to least-privilege by removing create and update and keeping only get,
list, watch, or expand to a full reconcile set by adding patch and delete (so
verbs become get, list, watch, create, update, patch, delete) depending on
whether the operator only reads Route status/host or actively
creates/patches/deletes Routes during reconciliation.
| - apiGroups: | ||
| - route.openshift.io | ||
| resources: | ||
| - routes | ||
| verbs: | ||
| - get | ||
| - create | ||
| - update | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add missing verbs and, if applicable, the custom-host subresource for Routes
Good catch adding Route permissions to the core Role. Two follow-ups to avoid RBAC denials at runtime:
- Add the patch verb. Most controllers use server-side apply/patch during reconciliation.
- If the operator sets spec.host on Routes, include the routes/custom-host subresource. Without it, OpenShift will reject custom host updates.
Example diff within this rule:
- resources:
- - routes
+ resources:
+ - routes
+ - routes/custom-host
verbs:
- get
- create
- update
+ - patch
- list
- watchIf the controller prunes/recreates Routes, also include delete.
🏁 Script executed:
#!/bin/bash
# List all occurrences of Route RBAC rules in the bundle to compare verbs and resources
rg -n -C2 "route.openshift.io" pkg/bundle/deploy.goLength of output: 971
🏁 Script executed:
#!/bin/bash
echo "=== Existing route rule (operator role) around line 6400: ==="
sed -n '6400,6440p' pkg/bundle/deploy.go
echo
echo "=== New route rule (core role) around line 6649: ==="
sed -n '6649,6690p' pkg/bundle/deploy.goLength of output: 1668
🏁 Script executed:
#!/bin/bash
# Extract the SHA and embedded YAML for the core Role (noobaa-core)
start_line=$(grep -n "const File_deploy_role_core_yaml" -n pkg/bundle/deploy.go | cut -d: -f1)
echo "SHA line:"
sed -n "$((start_line-1))p" pkg/bundle/deploy.go
echo
echo "Embedded YAML for noobaa-core:"
sed -n "$start_line, $((start_line+30))p" pkg/bundle/deploy.go | sed '/^`$/q'Length of output: 809
Update noobaa-core Role: include patch, subresource (and optional delete) for Routes
To avoid RBAC denials when reconciling Routes in the core controller, extend the existing rule in pkg/bundle/deploy.go (File_deploy_role_core_yaml) around lines 6654–6663:
• Add the patch verb (used by server-side apply/patch calls)
• Include the routes/custom-host subresource if the operator sets spec.host
• Optionally add delete if your controller prunes or recreates Routes
• Don’t forget to regenerate and verify the SHA for File_deploy_role_core_yaml after these changes
Example diff:
- apiGroups:
- - route.openshift.io
- resources:
- - routes
- verbs:
- - get
- - create
- - update
- - list
- - watch
+ apiGroups:
+ - route.openshift.io
+ resources:
+ - routes
+ - routes/custom-host # needed for spec.host updates
+ verbs:
+ - get
+ - create
+ - update
+ - patch # server-side apply/patch
+ - list
+ - watch
+ - delete # if you prune/recreate Routes🤖 Prompt for AI Agents
In pkg/bundle/deploy.go around lines 6654 to 6663, the Role rule for apiGroup
route.openshift.io currently allows get/create/update/list/watch on routes but
is missing verbs and subresources needed for controller reconciliation; update
that rule to also include the "patch" verb, add "delete" if the controller may
prune/recreate Routes, and include the subresource "routes/custom-host" (or
whichever subresource name is used when setting spec.host) so server-side apply
and host updates are permitted; after editing the File_deploy_role_core_yaml
blob regenerate and verify the file SHA to keep the bundle intact.
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Repro steps -
k logs noobaa-core-0 | less -Rand add the new routes permissions added in this PR.
Find noobaa-core role on openshift -
k delete pod noobaa-core-0 --forceSummary by CodeRabbit