Detect SCC UID/GID-range mismatch on namespace restore#449
Open
kaovilai wants to merge 1 commit into
Open
Conversation
openshift-ci
Bot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai 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 |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds namespace SCC annotation bookkeeping during ServiceAccount backup and verifies restored namespace annotations for mismatches. New backup and restore item actions are registered with caching, warning logs, cleanup, and unit tests. ChangesNamespace SCC range handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceAccount
participant BackupPlugin
participant KubernetesAPI
ServiceAccount->>BackupPlugin: backup service account
BackupPlugin->>KubernetesAPI: fetch namespace SCC annotations
KubernetesAPI-->>BackupPlugin: return annotation values
BackupPlugin-->>ServiceAccount: add bookkeeping annotations
sequenceDiagram
participant RestoreItemAction
participant RestorePlugin
participant KubernetesAPI
participant RestoreLog
RestoreItemAction->>RestorePlugin: restore ServiceAccount
RestorePlugin->>KubernetesAPI: fetch target namespace annotations
KubernetesAPI-->>RestorePlugin: return current values
RestorePlugin->>RestoreLog: log SCC mismatch warnings
RestorePlugin-->>RestoreItemAction: return cleaned ServiceAccount
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@velero-plugins/namespacescc/backup.go`:
- Line 99: Replace the uncancellable context used by the Namespaces().Get
lookups with a bounded-timeout context in both
velero-plugins/namespacescc/backup.go:99 and
velero-plugins/namespacescc/restore.go:104. Update the surrounding backup and
restore lookup flows to create and cancel the timeout context appropriately
while preserving the existing namespace retrieval behavior.
- Around line 59-60: Propagate all JSON conversion errors instead of ignoring
them, preventing partially populated ServiceAccount values or nil Objects from
being treated as successful. Update the conversion sites in
velero-plugins/namespacescc/backup.go at lines 59-60 and 73-75, and
velero-plugins/namespacescc/restore.go at lines 51-52, 56-58, and 78-80; either
return each Marshal/Unmarshal error through the surrounding backup and restore
flows or replace the round-trips with runtime.DefaultUnstructuredConverter.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69aa19f5-2bca-44fb-b8cc-45381b7806cd
📒 Files selected for processing (6)
velero-plugins/common/types.govelero-plugins/main.govelero-plugins/namespacescc/backup.govelero-plugins/namespacescc/backup_test.govelero-plugins/namespacescc/restore.govelero-plugins/namespacescc/restore_test.go
Comment on lines
+59
to
+60
| itemMarshal, _ := json.Marshal(item) | ||
| json.Unmarshal(itemMarshal, &serviceAccount) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e
printf '=== outline backup.go ===\n'
ast-grep outline velero-plugins/namespacescc/backup.go --view expanded || true
printf '\n=== outline restore.go ===\n'
ast-grep outline velero-plugins/namespacescc/restore.go --view expanded || true
printf '\n=== backup.go excerpt ===\n'
cat -n velero-plugins/namespacescc/backup.go | sed -n '1,220p'
printf '\n=== restore.go excerpt ===\n'
cat -n velero-plugins/namespacescc/restore.go | sed -n '1,220p'Repository: openshift/openshift-velero-plugin
Length of output: 14216
🏁 Script executed:
set -e
printf '=== json conversion usages in namespacescc ===\n'
rg -n 'json\.(Marshal|Unmarshal)|DefaultUnstructuredConverter|UnstructuredConverter' velero-plugins/namespacescc || true
printf '\n=== related tests or helpers ===\n'
rg -n 'namespacescc|ServiceAccount|unstructured' velero-plugins -g '*_test.go' || trueRepository: openshift/openshift-velero-plugin
Length of output: 1786
🏁 Script executed:
set -e
printf '=== backup_test.go excerpt ===\n'
cat -n velero-plugins/namespacescc/backup_test.go | sed -n '1,220p'
printf '\n=== restore_test.go excerpt ===\n'
cat -n velero-plugins/namespacescc/restore_test.go | sed -n '1,240p'Repository: openshift/openshift-velero-plugin
Length of output: 8537
🏁 Script executed:
set -e
cat >/tmp/namespacescc_json_verify.go <<'GO'
package main
import (
"encoding/json"
"fmt"
)
type ServiceAccount struct {
Namespace string `json:"namespace"`
Annotations map[string]string `json:"annotations"`
}
func main() {
bad := []byte(`{"namespace": 123, "annotations": {"a": "b"}}`)
var sa ServiceAccount
err := json.Unmarshal(bad, &sa)
fmt.Printf("unmarshal err: %v\n", err)
fmt.Printf("result: %#v\n", sa)
var out map[string]interface{}
marshalBad := []byte(`{"namespace":"ns","annotations":` + "`" + `oops` + "`" + `}`)
err = json.Unmarshal(marshalBad, &out)
fmt.Printf("unmarshal-to-map err: %v\n", err)
fmt.Printf("map is nil: %v\n", out == nil)
}
GO
go run /tmp/namespacescc_json_verify.goRepository: openshift/openshift-velero-plugin
Length of output: 451
Propagate these conversion errors.
These JSON round-trips are unchecked in both backup and restore. A failed decode can leave a partially populated ServiceAccount, and a failed encode/decode can return a nil Object while still reporting success. Return the error (or use runtime.DefaultUnstructuredConverter) at:
velero-plugins/namespacescc/backup.go#L59-L60,#L73-L75velero-plugins/namespacescc/restore.go#L51-L52,#L56-L58,#L78-L80
📍 Affects 2 files
velero-plugins/namespacescc/backup.go#L59-L60(this comment)velero-plugins/namespacescc/backup.go#L73-L75velero-plugins/namespacescc/restore.go#L51-L52velero-plugins/namespacescc/restore.go#L56-L58velero-plugins/namespacescc/restore.go#L78-L80
🤖 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 `@velero-plugins/namespacescc/backup.go` around lines 59 - 60, Propagate all
JSON conversion errors instead of ignoring them, preventing partially populated
ServiceAccount values or nil Objects from being treated as successful. Update
the conversion sites in velero-plugins/namespacescc/backup.go at lines 59-60 and
73-75, and velero-plugins/namespacescc/restore.go at lines 51-52, 56-58, and
78-80; either return each Marshal/Unmarshal error through the surrounding backup
and restore flows or replace the round-trips with
runtime.DefaultUnstructuredConverter.
Source: Path instructions
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ns, err := client.Namespaces().Get(context.Background(), namespace, metav1.GetOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n## backup.go\n'
cat -n velero-plugins/namespacescc/backup.go | sed -n '1,220p'
printf '\n## restore.go\n'
cat -n velero-plugins/namespacescc/restore.go | sed -n '1,220p'
printf '\n## search for mutex/context usage\n'
rg -n "WithTimeout|context\\.Background|mutex|Lock\\(|Unlock\\(|ServiceAccount|Namespaces\\(\\)\\.Get" velero-plugins/namespacescc -SRepository: openshift/openshift-velero-plugin
Length of output: 14037
Bound the namespace lookups with a timeout.
context.Background() leaves both Namespaces().Get(...) calls uncancellable, and each one runs while mu is held, so a slow apiserver can stall every ServiceAccount processed by the same plugin instance.
velero-plugins/namespacescc/backup.go#L99velero-plugins/namespacescc/restore.go#L104
📍 Affects 2 files
velero-plugins/namespacescc/backup.go#L99-L99(this comment)velero-plugins/namespacescc/restore.go#L104-L104
🤖 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 `@velero-plugins/namespacescc/backup.go` at line 99, Replace the uncancellable
context used by the Namespaces().Get lookups with a bounded-timeout context in
both velero-plugins/namespacescc/backup.go:99 and
velero-plugins/namespacescc/restore.go:104. Update the surrounding backup and
restore lookup flows to create and cancel the timeout context appropriately
while preserving the existing namespace retrieval behavior.
Source: Path instructions
Namespace objects never pass through RestoreItemAction plugins in the pinned Velero version (restore.go unconditionally skips them), so a namespaces-scoped plugin as literally proposed in openshift#448 can never fire. Instead, stash the namespace's SCC UID/GID-range annotations (sa.scc.uid-range, sa.scc.supplemental-groups, sa.scc.mcs) onto each backed-up ServiceAccount, and compare them against the actual restored namespace's annotations at restore time, logging a warning on mismatch. Fixes openshift#448 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai
force-pushed
the
issue-448-scc-uid-gid-mismatch
branch
from
July 21, 2026 01:05
a4117e5 to
d597b27
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Responses generated with Claude
Summary
Restoring into a namespace whose OpenShift-assigned UID/GID range differs from the one recorded at backup time silently breaks file ownership for workloads that
chown'd data to the backup-time UID (e.g. non-root DB containers). Restore reportsCompleted; the failure only surfaces later as an app-level permission error. This is a documented OADP limitation, never implemented as code (see #448).Why not a
RestoreItemActiononnamespaces(as originally proposed in #448)Traced through the pinned
github.com/openshift/velerofork's actual source:pkg/restore/restore.gounconditionally skipsNamespaceobjects from ever reaching theRestoreItemActionpipeline —Namespace creation/existence is instead handled by a hardcoded
kube.EnsureNamespaceExistsAndIsReady()call that reads the Namespace straight from the backup archive, with no plugin hook. ARestoreItemAction{AppliesTo: {"namespaces"}}would simply never fire on this Velero version.Also confirmed:
RestoreItemActionExecuteOutputhas noWarningfield in this API version — a plugin can only fail an item (blocking) or succeed silently, there's no "log a warning but still restore" return path forRestoreItemAction.Approach
New package
velero-plugins/namespacescc, piggybacking onserviceaccounts(always present in every namespace, already routes through the restore action pipeline):backup.go: stashes the namespace'sopenshift.io/sa.scc.uid-range,sa.scc.supplemental-groups, andsa.scc.mcsannotations onto each backed-up ServiceAccount, under bookkeeping annotation keys (oadp.openshift.io/backup-ns-scc-*).restore.go: reads those stashed values back from the pristineItemFromBackup, resolves the actual target namespace (honoringRestore.Spec.NamespaceMapping), fetches its live annotations, andLog.Warnfs on any mismatch — the closest available signal given the API constraint above. Strips the bookkeeping annotations before the ServiceAccount is persisted, so they don't leak onto the restored object.Namespaces().Get()call.Explicitly out of scope: auto-reapplying the original range on mismatch (issue's stretch goal) — re-assigning a UID/GID range that OpenShift's allocator already gave to a different namespace risks a collision; needs its own design.
Real-world validation
Traced OpenShift's actual
NamespaceSCCAllocationController(openshift/cluster-policy-controller): it only allocates a fresh range when theuid-rangeannotation is absent on the namespace — if present (e.g. because it pre-existed with its own, different range before Velero'sEnsureNamespaceExistsAndIsReadyno-op'd on it), the controller never re-validates or reconciles it. This matches the customer scenario that motivated #448 (restored PostgreSQL:chmod: ... Operation not permitted) — most likely triggered by restoring into a namespace pre-provisioned by cluster automation before the restore ran.Fixes #448
Test plan
go build ./...go vet ./velero-plugins/namespacescc/...go test ./velero-plugins/namespacescc/...— new table-driven unit tests for the pure annotation stash/compare/strip helperssa.scc.uid-rangethan at backup time, confirm the plugin logs the mismatchSummary by CodeRabbit
New Features
Tests