OADP-6540: Skip restore of k8s RBAC system rolebindings#402
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@kaovilai: This pull request references OADP-6540 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a Kubernetes RBAC rolebindings restore plugin that reuses an exported OpenShift system-rolebinding allowlist, registers the plugin in main.go, adds unit tests, splits README docs for OpenShift vs RBAC rolebindings, and modernizes the Makefile to use golang:1.25 with simplified Docker mounts. ChangesKubernetes RBAC Role Bindings Restore Plugin
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/cherry-pick oadp-1.6 |
PR openshift#311 added skip logic for system rolebindings (system:image-pullers, system:image-builders, system:deployers) but only for the authorization.openshift.io API group. OpenShift stores these as rbac.authorization.k8s.io objects and exposes them via both API groups. Velero backs up both variants since rolebindings are not in Velero's cohabitating resources dedup list. During restore with namespace mapping, the k8s RBAC variants were restored with stale subject references (old namespace), overwriting the correct auto-created rolebindings and causing ErrImagePull. Add a new RestoreItemAction for rolebindings (rbac.authorization.k8s.io) that skips the same system rolebindings, letting OpenShift create them with correct namespace references. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions 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. |
|
/cherry-pick oadp-1.5 |
kaovilai
force-pushed
the
OADP-6540-skip-rbac-system-rolebindings
branch
from
3918c36 to
999496b
Compare
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions 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. |
|
/cherry-pick oadp-1.4 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions 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. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
velero-plugins/rolebindings/restore.go (1)
40-41:⚠️ Potential issue | 🟠 MajorHandle JSON marshal/unmarshal errors in rolebinding restore flow.
velero-plugins/rolebindings/ocp_restore.goignoresjson.Marshal/json.Unmarshalerrors at lines 40-41 and 65-66, risking incorrectSystemRoleBindingsskip decisions and returning malformed restore objects.velero-plugins/rolebindings/k8s_restore.goalso ignoresjson.Marshal/json.Unmarshalerrors at lines 28-29 before evaluatingSystemRoleBindings, so failed decoding can cause system rolebindings to be restored when they should be skipped.Proposed fix
import ( "encoding/json" + "fmt" "strings" @@ roleBinding := apiauthorization.RoleBinding{} - itemMarshal, _ := json.Marshal(input.Item) - json.Unmarshal(itemMarshal, &roleBinding) + itemMarshal, err := json.Marshal(input.Item) + if err != nil { + return nil, fmt.Errorf("marshal restore item: %w", err) + } + if err := json.Unmarshal(itemMarshal, &roleBinding); err != nil { + return nil, fmt.Errorf("unmarshal rolebinding: %w", err) + } @@ var out map[string]interface{} - objrec, _ := json.Marshal(roleBinding) - json.Unmarshal(objrec, &out) + objrec, err := json.Marshal(roleBinding) + if err != nil { + return nil, fmt.Errorf("marshal rolebinding output: %w", err) + } + if err := json.Unmarshal(objrec, &out); err != nil { + return nil, fmt.Errorf("unmarshal output object: %w", err) + }🤖 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/rolebindings/restore.go` around lines 40 - 41, The code is ignoring errors from json.Marshal/json.Unmarshal when decoding input.Item into roleBinding (used to check SystemRoleBindings), which can lead to wrong skip decisions or malformed restores; update the restore logic in the functions that decode input.Item (the blocks using itemMarshal := json.Marshal(input.Item) and json.Unmarshal(itemMarshal, &roleBinding) in ocp_restore.go and k8s_restore.go) to capture and handle both marshal and unmarshal errors: check the error returned by json.Marshal and json.Unmarshal, and on error either return the error up the Restore flow or log and safely skip/abort the restore as appropriate for SystemRoleBindings checking; ensure the variable roleBinding and the SystemRoleBindings evaluation only proceed when decoding succeeded.Source: Coding guidelines
🤖 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/rbacrolebindings/restore.go`:
- Around line 29-30: The code currently ignores json.Marshal/json.Unmarshal
errors when converting input.Item to roleBinding/out in the Execute functions,
which can produce zero-value structs and incorrect skip/restore behavior; update
the Execute implementations (references: Execute, variable roleBinding in
velero-plugins/rolebindings/k8s_restore.go and the out variable in
velero-plugins/rolebindings/ocp_restore.go) to check the error returned by
json.Marshal and json.Unmarshal, and handle failures by returning or logging the
error and aborting the restore flow for that item rather than proceeding with a
zero-value object.
---
Outside diff comments:
In `@velero-plugins/rolebindings/restore.go`:
- Around line 40-41: The code is ignoring errors from
json.Marshal/json.Unmarshal when decoding input.Item into roleBinding (used to
check SystemRoleBindings), which can lead to wrong skip decisions or malformed
restores; update the restore logic in the functions that decode input.Item (the
blocks using itemMarshal := json.Marshal(input.Item) and
json.Unmarshal(itemMarshal, &roleBinding) in ocp_restore.go and k8s_restore.go)
to capture and handle both marshal and unmarshal errors: check the error
returned by json.Marshal and json.Unmarshal, and on error either return the
error up the Restore flow or log and safely skip/abort the restore as
appropriate for SystemRoleBindings checking; ensure the variable roleBinding and
the SystemRoleBindings evaluation only proceed when decoding succeeded.
🪄 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: ac42bc5a-7ac1-4775-a38f-7259e96278b3
📒 Files selected for processing (5)
README.mdvelero-plugins/main.govelero-plugins/rbacrolebindings/restore.govelero-plugins/rbacrolebindings/restore_test.govelero-plugins/rolebindings/restore.go
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/rolebindings/k8s_restore.go`:
- Around line 28-29: The code ignores errors from json.Marshal and
json.Unmarshal when converting input.Item into roleBinding, which can leave
roleBinding zero-valued and cause incorrect logic or panics; update the block
around json.Marshal and json.Unmarshal (referencing input.Item and roleBinding)
to check both error returns, handle them appropriately (e.g., log the error via
the existing logger or return/continue with an error), and avoid proceeding with
a malformed roleBinding—ensure you propagate or handle the error path instead of
discarding it.
- Around line 24-41: The Execute method in K8sRestorePlugin currently ignores
input.Restore.Spec.NamespaceMapping and simply returns the RoleBinding unchanged
(besides skipping SystemRoleBindings), which can leave subject namespaces stale;
update K8sRestorePlugin.Execute to detect input.Restore.Spec.NamespaceMapping,
iterate over the deserialized rbacv1.RoleBinding.Subjects and rewrite any
Subject.Namespace (and serviceaccount-style Subject.Name portions like
"system:serviceaccount:<ns>:<name>" if present) using the mapping, then marshal
the modified roleBinding back into the output item returned by
velero.NewRestoreItemActionExecuteOutput; keep the existing SystemRoleBindings
name check but do not skip non-system rolebindings based solely on name.
🪄 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: 7163bba9-446d-4553-9672-5140175f2946
📒 Files selected for processing (7)
MakefileREADME.mdvelero-plugins/main.govelero-plugins/rolebindings/k8s_restore.govelero-plugins/rolebindings/k8s_restore_test.govelero-plugins/rolebindings/ocp_restore.govelero-plugins/rolebindings/ocp_restore_test.go
✅ Files skipped from review due to trivial changes (1)
- README.md
| func (p *K8sRestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { | ||
| p.Log.Info("[rbac-rolebinding-restore] Entering RBAC Role Bindings restore plugin") | ||
|
|
||
| roleBinding := rbacv1.RoleBinding{} | ||
| itemMarshal, _ := json.Marshal(input.Item) | ||
| json.Unmarshal(itemMarshal, &roleBinding) | ||
|
|
||
| p.Log.Infof("[rbac-rolebinding-restore] role binding - %s, API version %s", roleBinding.Name, roleBinding.APIVersion) | ||
|
|
||
| if SystemRoleBindings[roleBinding.Name] { | ||
| p.Log.Infof("[rbac-rolebinding-restore] Skipping system rolebinding %s as it will be automatically created", roleBinding.Name) | ||
| return &velero.RestoreItemActionExecuteOutput{ | ||
| SkipRestore: true, | ||
| }, nil | ||
| } | ||
|
|
||
| return velero.NewRestoreItemActionExecuteOutput(input.Item), nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for namespace mapping patterns in rolebinding restore plugins
echo "=== Checking if authorization.openshift.io rolebindings are also backed up as rbac.authorization.k8s.io ==="
rg -n -C3 'authorization\.openshift\.io' velero-plugins/rolebindings/
echo -e "\n=== Checking NamespaceMapping usage in restore plugins ==="
rg -n 'NamespaceMapping' velero-plugins/ --type go -A3 -B3
echo -e "\n=== Checking AppliesTo resource selectors for rolebindings ==="
ast-grep --pattern $'func ($) AppliesTo() $_ {
$$$
IncludedResources: $$$
$$$
}'Repository: openshift/openshift-velero-plugin
Length of output: 15391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rolebindings directory ==="
ls -la velero-plugins/rolebindings
echo -e "\n=== AppliesTo implementations in rolebindings plugins ==="
rg -n "func \\(p \\*.*\\) AppliesTo\\(" velero-plugins/rolebindings/*.go
rg -n "IncludedResources" velero-plugins/rolebindings/*.go
echo -e "\n=== NamespaceMapping usage in rolebindings plugins ==="
rg -n "NamespaceMapping" velero-plugins/rolebindings/*.go
echo -e "\n=== k8s_restore.go Execute() details around unmarshalling/skip logic ==="
sed -n '1,120p' velero-plugins/rolebindings/k8s_restore.go
echo -e "\n=== ocp_restore.go Execute() details around namespace mapping ==="
sed -n '1,160p' velero-plugins/rolebindings/ocp_restore.go
echo -e "\n=== k8s_restore_test.go (look for NamespaceMapping / subject mapping behavior) ==="
sed -n '1,220p' velero-plugins/rolebindings/k8s_restore_test.goRepository: openshift/openshift-velero-plugin
Length of output: 10563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== clusterrolebindings/restore.go (namespace mapping logic) ==="
sed -n '1,220p' velero-plugins/clusterrolebindings/restore.goRepository: openshift/openshift-velero-plugin
Length of output: 2255
Add NamespaceMapping handling for Kubernetes rolebindings restore
velero-plugins/rolebindings/k8s_restore.goignoresinput.Restore.Spec.NamespaceMappingand restores customrbacv1.RoleBindingunchanged (only skipsSystemRoleBindingsby name), so subject namespace references can remain stale.velero-plugins/rolebindings/ocp_restore.goandvelero-plugins/clusterrolebindings/restore.goboth applyNamespaceMappingby rewriting subject/identity namespace fields;k8s_restore.goshould implement the equivalent namespace swapping forrbacv1.RoleBinding.Subjects(and any subject.Name serviceaccount-group namespace portions) whenNamespaceMappingis set.- Skipping custom K8s rolebindings isn’t necessary:
k8s_restore.gotargetsrolebindings, whileocp_restore.gotargetsrolebinding.authorization.openshift.io(different resource selectors).
🤖 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/rolebindings/k8s_restore.go` around lines 24 - 41, The Execute
method in K8sRestorePlugin currently ignores input.Restore.Spec.NamespaceMapping
and simply returns the RoleBinding unchanged (besides skipping
SystemRoleBindings), which can leave subject namespaces stale; update
K8sRestorePlugin.Execute to detect input.Restore.Spec.NamespaceMapping, iterate
over the deserialized rbacv1.RoleBinding.Subjects and rewrite any
Subject.Namespace (and serviceaccount-style Subject.Name portions like
"system:serviceaccount:<ns>:<name>" if present) using the mapping, then marshal
the modified roleBinding back into the output item returned by
velero.NewRestoreItemActionExecuteOutput; keep the existing SystemRoleBindings
name check but do not skip non-system rolebindings based solely on name.
There was a problem hiding this comment.
Not needed — Velero's built-in RoleBindingAction already handles Subject.Namespace remapping for k8s RBAC rolebindings. Multiple RestoreItemActions chain — our plugin runs first (skip system ones), then Velero's built-in runs on non-system ones and handles namespace mapping.
For system rolebindings, we skip them entirely (SkipRestore: true) so OpenShift creates fresh ones with correct subjects — no namespace remapping needed.
The embedded namespace in Subject.Name (e.g. system:serviceaccounts:<ns>) is an OpenShift-specific pattern that only appears in the system rolebindings we're skipping. Custom k8s RBAC rolebindings use Subject.Namespace field which Velero's built-in handles.
Note
Responses generated with Claude
There was a problem hiding this comment.
@kaovilai, thanks for the detailed explanation — that makes sense. Velero's built-in RoleBindingAction chaining after this plugin, handling Subject.Namespace remapping for non-system rolebindings, fully covers the namespace mapping concern. And since system rolebindings are skipped entirely, there's nothing to remap for them. I'll withdraw the comment.
✏️ Learnings added
Learnt from: kaovilai
Repo: openshift/openshift-velero-plugin PR: 402
File: velero-plugins/rolebindings/k8s_restore.go:24-46
Timestamp: 2026-06-08T16:56:41.949Z
Learning: In `openshift/openshift-velero-plugin`, the `K8sRestorePlugin` in `velero-plugins/rolebindings/k8s_restore.go` does NOT need to handle `NamespaceMapping` / `Subject.Namespace` remapping for k8s RBAC rolebindings (`rbac.authorization.k8s.io/v1/RoleBinding`). Velero's built-in `RoleBindingAction` (at `pkg/restore/actions/rolebinding_action.go`) already handles `Subject.Namespace` remapping. Multiple RestoreItemActions chain: the OpenShift plugin runs first and skips system rolebindings (`system:image-pullers`, `system:image-builders`, `system:deployers`) with `SkipRestore: true`, then Velero's built-in action runs on the remaining non-system ones and applies namespace mapping. System rolebindings are skipped entirely so namespace remapping is irrelevant for them. The `system:serviceaccounts:<ns>` embedded namespace pattern in `Subject.Name` is OpenShift-specific and only appears in the system rolebindings that are being skipped.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, shubham-pampattiwar, sseago 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 |
|
@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. |
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 `@Makefile`:
- Line 19: The build recipe producing _output/$(BIN) currently ignores ARCH
because docker run uses BUILD_IMAGE without specifying platform or Go envs;
update the Makefile build recipe so it passes the intended architecture into the
container: add --platform=linux/$(ARCH) to the docker run invocation (or set
GOOS/GOARCH env vars inside the container) and export GOARCH from ARCH (e.g.,
set GOARCH=$(ARCH) via -e GOARCH=$(ARCH)) so go build inside the container
honors the requested architecture; ensure this change is applied to the rule
that references BUILD_IMAGE and creates _output/$(BIN).
🪄 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: a877713b-1bcb-436d-a44c-99abb04be286
📒 Files selected for processing (7)
MakefileREADME.mdvelero-plugins/main.govelero-plugins/rolebindings/k8s_restore.govelero-plugins/rolebindings/k8s_restore_test.govelero-plugins/rolebindings/ocp_restore.govelero-plugins/rolebindings/ocp_restore_test.go
| REPO ?= github.com/konveyor/openshift-velero-plugin | ||
|
|
||
| BUILD_IMAGE ?= openshift/origin-release:golang-1.14 | ||
| BUILD_IMAGE ?= golang:1.25 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the official golang:1.25Docker image publish multi-architecture manifests, and whenGOOS/GOARCHare unset doesgo build default to the container's OS/architecture?
💡 Result:
The official golang:1.25 Docker image publishes multi-architecture manifests [1][2]. These manifests allow the Docker engine to automatically pull the appropriate image for the host's architecture (such as amd64, arm64v8, or others) [1][3]. When GOOS and GOARCH are unset, the go build command defaults to the OS and architecture of the environment where the command is executed [4][5]. In the context of a running Docker container, this corresponds to the OS and architecture of that container [3][5]. The Go toolchain determines these default values based on its own build configuration and the system's runtime environment [4][6][7].
Citations:
- 1: https://github.com/docker-library/official-images/blob/master/library/golang
- 2: https://hub.docker.com/_/golang
- 3: https://github.com/docker-library/docs/tree/master/golang
- 4: https://pkg.go.dev/go/build
- 5: https://www.digitalocean.com/community/tutorials/building-go-applications-for-different-operating-systems-and-architectures
- 6: https://github.com/golang/go/blob/996b9850/src/go/build/build.go
- 7: https://github.com/golang/go/blob/996b9850/src/internal/buildcfg/cfg.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
if [ ! -f Makefile ]; then echo "Makefile not found"; exit 1; fi
nl -ba Makefile | sed -n '1,120p'Repository: openshift/openshift-velero-plugin
Length of output: 1062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Makefile: lines 1-120 with numbers (awk) ==="
awk '{printf "%d:%s\n", NR, $0}' Makefile | sed -n '1,120p'
echo "=== Makefile: relevant vars and recipe parts ==="
rg -n "BUILD_IMAGE|ARCH\s*\?=|DOCKER_BUILD_ARGS|docker run|go build" Makefile || trueRepository: openshift/openshift-velero-plugin
Length of output: 3840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "_output/\$\(\s*BIN\s*\)|_output/" -S . || true
rg -n "ARCH\s*\?=|DOCKER_BUILD_ARGS|CLUSTER_OS|CLUSTER_ARCH|BUILD_IMAGE" -S Makefile Dockerfile velero-plugins -g'!*/vendor/*' || trueRepository: openshift/openshift-velero-plugin
Length of output: 1302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Dockerfile ==="
nl -ba Dockerfile | sed -n '1,120p'
echo "=== konflux.Dockerfile ==="
nl -ba konflux.Dockerfile | sed -n '1,120p'Repository: openshift/openshift-velero-plugin
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Dockerfile: numbered head ==="
awk '{printf "%d:%s\n", NR, $0}' Dockerfile | sed -n '1,140p'
echo "=== konflux.Dockerfile: numbered head ==="
awk '{printf "%d:%s\n", NR, $0}' konflux.Dockerfile | sed -n '1,220p'Repository: openshift/openshift-velero-plugin
Length of output: 3171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "make (build|docker|container)|_output/\$\(BIN\)|_output/velero-plugins|_output/" . -g'!.git/*' -SRepository: openshift/openshift-velero-plugin
Length of output: 825
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\bARCH\b|_output/\$\(\s*BIN\s*\)" . -S -g'!vendor/*' -g'!**/node_modules/*'Repository: openshift/openshift-velero-plugin
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Makefile (full) ==="
cat Makefile
echo "=== README: lines around make build/container ==="
rg -n "\$ make (build|container)|make build|make container" README.md
sed -n '1,120p' README.md
sed -n '50,120p' README.md
sed -n '120,220p' README.mdRepository: openshift/openshift-velero-plugin
Length of output: 20451
Make make build honor $(ARCH) when using golang:1.25
golang:1.25 is multi-arch, and the docker run used for _output/$(BIN) doesn’t set --platform, GOOS, or GOARCH, so go build defaults to the container/host architecture; meanwhile ARCH ?= amd64 is unused in that recipe. This can produce _output/$(BIN) for an architecture different from what you intend.
Suggested fix
BUILD_IMAGE ?= golang:1.25
@@
docker run \
+ --platform=linux/$(ARCH) \
--rm \
-v $$(pwd):/workspace:z \
-v $$(pwd)/.go/.cache:/go/.cache:z \
-w /workspace \
+ -e GOOS=linux \
+ -e GOARCH=$(ARCH) \
-e GOCACHE=/go/.cache \
-e GOFLAGS="-mod=mod -buildvcs=false" \
$(BUILD_IMAGE) \
go build -tags $(BUILDTAGS) -v -o _output/$(BIN) ./$(BIN)🤖 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 `@Makefile` at line 19, The build recipe producing _output/$(BIN) currently
ignores ARCH because docker run uses BUILD_IMAGE without specifying platform or
Go envs; update the Makefile build recipe so it passes the intended architecture
into the container: add --platform=linux/$(ARCH) to the docker run invocation
(or set GOOS/GOARCH env vars inside the container) and export GOARCH from ARCH
(e.g., set GOARCH=$(ARCH) via -e GOARCH=$(ARCH)) so go build inside the
container honors the requested architecture; ensure this change is applied to
the rule that references BUILD_IMAGE and creates _output/$(BIN).
|
/lgtm |
|
@kaovilai: new pull request created: #403 DetailsIn response to this:
Instructions 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. |
|
@kaovilai: #402 failed to apply on top of branch "oadp-1.5": DetailsIn response to this:
Instructions 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. |
|
@kaovilai: #402 failed to apply on top of branch "oadp-1.4": DetailsIn response to this:
Instructions 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. |
VerificationTested with OADP 1.6.0 downstream build on OCP 5.0 (arm64):
Backup of Both API groups now skip system rolebindings: Subjects in target namespace point to correct namespace: [{"apiGroup":"rbac.authorization.k8s.io","kind":"Group","name":"system:serviceaccounts:test-rb-target"}]Note Responses generated with Claude |
5 participants
Summary
K8sRestorePluginin therolebindingspackage forrolebindings(rbac.authorization.k8s.io) that skips system rolebindings (system:image-pullers,system:image-builders,system:deployers)SystemRoleBindingsmap for reuse across both OCP and k8s restore pluginsocp_restore.go/ocp_restore_test.gofor claritygolang:1.25) and flagsProblem
When restoring with namespace mapping (e.g.
ns-a→ns-b), pods in the target namespace fail withErrImagePull/ImagePullBackOffbecause they can't pull from the internal registry.The root cause is that OpenShift auto-creates
system:image-pullerswhen a namespace is created, with subjects pointing to the correct target namespace (system:serviceaccounts:ns-b). But Velero also restores the backed-uprbac.authorization.k8s.io/v1/RoleBindingnamedsystem:image-pullers, which overwrites the auto-created one with subjects still pointing to the source namespace (system:serviceaccounts:ns-a). Service accounts inns-bthen lack pull permissions forns-b's image registry.PR #311 fixed this for
authorization.openshift.iorolebindings by skipping the three system rolebindings. But OpenShift stores rolebindings asrbac.authorization.k8s.ioin etcd — theauthorization.openshift.ioAPI is a proxy/compatibility layer, not separate storage. Velero backs up both API groups becauserolebindingsis not in Velero'scohabitatingResources()dedup list. So therbac.authorization.k8s.iovariants were still being restored and overwriting the correct auto-created ones.Fix
Skip the same three system rolebindings (
system:image-pullers,system:image-builders,system:deployers) for therbac.authorization.k8s.ioAPI group. OpenShift creates these automatically with correct subject namespace references when a project is created, so they should never be restored from backup.Test plan
K8sRestorePlugin(skip system, pass-through custom)rolebindingstests pass with exportedSystemRoleBindingsgo build ./velero-plugins/...compiles cleanrbac.authorization.k8s.io/v1/RoleBindingsystem:image-pullers shows as(skipped)in velero logssystem:image-pullerssubjects in target namespace correctly referencesystem:serviceaccounts:test-rb-target(not source namespace)Fixes: https://issues.redhat.com/browse/OADP-6540
🤖 Generated with Claude Code