From 71ac7d2a7353d8ffef91ba0f18d9d249067ca069 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 8 Jun 2026 09:53:33 -0400 Subject: [PATCH 1/3] OADP-6540: Skip restore of k8s RBAC system rolebindings PR #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 Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- Makefile | 17 ++- README.md | 11 +- velero-plugins/main.go | 5 + velero-plugins/rolebindings/k8s_restore.go | 41 +++++++ .../rolebindings/k8s_restore_test.go | 101 ++++++++++++++++++ .../{restore.go => ocp_restore.go} | 8 +- .../{restore_test.go => ocp_restore_test.go} | 0 7 files changed, 167 insertions(+), 16 deletions(-) create mode 100644 velero-plugins/rolebindings/k8s_restore.go create mode 100644 velero-plugins/rolebindings/k8s_restore_test.go rename velero-plugins/rolebindings/{restore.go => ocp_restore.go} (95%) rename velero-plugins/rolebindings/{restore_test.go => ocp_restore_test.go} (100%) diff --git a/Makefile b/Makefile index 78322603..9cdf0d42 100644 --- a/Makefile +++ b/Makefile @@ -16,7 +16,7 @@ BINS = $(wildcard velero-*) REPO ?= github.com/konveyor/openshift-velero-plugin -BUILD_IMAGE ?= openshift/origin-release:golang-1.14 +BUILD_IMAGE ?= golang:1.25 IMAGE ?= docker.io/konveyor/openshift-velero-plugin @@ -34,19 +34,16 @@ build-%: build: _output/$(BIN) _output/$(BIN): $(BIN)/*.go - mkdir -p .go/src/$(REPO) .go/pkg .go/.cache .go/std/$(ARCH) _output - cp -rp * .go/src/$(REPO) + mkdir -p .go/.cache _output docker run \ --rm \ - -v $$(pwd)/.go/pkg:/go/pkg:z \ - -v $$(pwd)/.go/src:/go/src:z \ - -v $$(pwd)/.go/std:/go/std:z \ + -v $$(pwd):/workspace:z \ -v $$(pwd)/.go/.cache:/go/.cache:z \ - -v $$(pwd)/_output:/go/src/$(REPO)/_output:z \ - -v $$(pwd)/.go/std/$(ARCH):/usr/local/go/pkg/linux_$(ARCH)_static:z \ - -w /go/src/$(REPO) \ + -w /workspace \ + -e GOCACHE=/go/.cache \ + -e GOFLAGS="-mod=mod -buildvcs=false" \ $(BUILD_IMAGE) \ - go build -installsuffix "static" -tags $(BUILDTAGS) -i -v -o _output/$(BIN) ./$(BIN) + go build -tags $(BUILDTAGS) -v -o _output/$(BIN) ./$(BIN) DOCKER_BUILD_ARGS ?= --platform=linux/amd64 ifneq ($(CLUSTER_OS),) diff --git a/README.md b/README.md index 190e884a..51c02c91 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ Velero currently supports the following kinds of plugins: ## Resources Included in Plugin -- Build, Build Config, Cluster Role Binding, Cron Job, Daemonset, Deployment, Deployment Config, Image Stream, Image Stream Tag, Image Tag, Persistent Volume, Persistent Volume Claim, Pod, Replica Set, Replication Controller, Role Binding, Route, SCC, Service, Service Account, and Stateful Set +- Build, Build Config, Cluster Role Binding, Cron Job, Daemonset, Deployment, Deployment Config, Image Stream, Image Stream Tag, Image Tag, Persistent Volume, Persistent Volume Claim, Pod, Replica Set, Replication Controller, Role Binding (authorization.openshift.io), Role Binding (rbac.authorization.k8s.io), Route, SCC, Service, Service Account, and Stateful Set ## Enabling and Disabling the Plugin for Individual Resources @@ -303,10 +303,17 @@ time="2020-07-29T18:51:04Z" level=info msg="[pvc-restore] Returning pvc object a - If the Replication Controller is owned by Deployment Config, set SkipRestore to true, so that the resource is not restored by Replication Controller ### Role Binding -#### Restore Plugin +#### Restore Plugin (OpenShift authorization.openshift.io) - Skips restore of system rolebindings ("system:image-pullers", "system:image-builders", "system:deployers") as these are automatically created by OpenShift - If restore namespace mapping is enabled, then the namespaces in RoleRef.Namespace, usernames, groupnames, and subjects are swapped accordingly +#### Restore Plugin (Kubernetes RBAC) + +- **Resources**: rolebindings (rbac.authorization.k8s.io) +- **Actions**: + - Skips restore of the same system rolebindings ("system:image-pullers", "system:image-builders", "system:deployers") for the Kubernetes RBAC API group + - OpenShift stores rolebindings in rbac.authorization.k8s.io format and exposes them via both API groups; both must be skipped during restore to avoid conflicts with auto-created rolebindings + ### Route #### Restore Plugin - If the host generated annotation is set to true, then strip the source cluster host from the Route diff --git a/velero-plugins/main.go b/velero-plugins/main.go index 743e0e6b..b0989ee9 100644 --- a/velero-plugins/main.go +++ b/velero-plugins/main.go @@ -69,6 +69,7 @@ func main() { RegisterRestoreItemAction("openshift.io/24-horizontalpodautoscaler-restore-plugin", newHorizontalPodAutoscalerRestorePlugin). RegisterBackupItemAction("openshift.io/25-configmap-backup-plugin", newConfigMapBackupPlugin). RegisterRestoreItemAction("openshift.io/25-configmap-restore-plugin", newConfigMapRestorePlugin). + RegisterRestoreItemAction("openshift.io/27-rbac-role-bindings-restore-plugin", newRBACRoleBindingRestorePlugin). Serve() } @@ -215,3 +216,7 @@ func newConfigMapBackupPlugin(logger logrus.FieldLogger) (interface{}, error) { func newConfigMapRestorePlugin(logger logrus.FieldLogger) (interface{}, error) { return &configmap.RestorePlugin{Log: logger}, nil } + +func newRBACRoleBindingRestorePlugin(logger logrus.FieldLogger) (interface{}, error) { + return &rolebindings.K8sRestorePlugin{Log: logger}, nil +} diff --git a/velero-plugins/rolebindings/k8s_restore.go b/velero-plugins/rolebindings/k8s_restore.go new file mode 100644 index 00000000..138bb452 --- /dev/null +++ b/velero-plugins/rolebindings/k8s_restore.go @@ -0,0 +1,41 @@ +package rolebindings + +import ( + "encoding/json" + + "github.com/sirupsen/logrus" + "github.com/vmware-tanzu/velero/pkg/plugin/velero" + rbacv1 "k8s.io/api/rbac/v1" +) + +// K8sRestorePlugin is a restore item action plugin for k8s RBAC rolebindings +type K8sRestorePlugin struct { + Log logrus.FieldLogger +} + +// AppliesTo returns a velero.ResourceSelector that applies to k8s RBAC rolebindings +func (p *K8sRestorePlugin) AppliesTo() (velero.ResourceSelector, error) { + return velero.ResourceSelector{ + IncludedResources: []string{"rolebindings"}, + }, nil +} + +// Execute skips system rolebindings that OpenShift auto-creates for new namespaces +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 +} diff --git a/velero-plugins/rolebindings/k8s_restore_test.go b/velero-plugins/rolebindings/k8s_restore_test.go new file mode 100644 index 00000000..cca27100 --- /dev/null +++ b/velero-plugins/rolebindings/k8s_restore_test.go @@ -0,0 +1,101 @@ +package rolebindings + +import ( + "testing" + + "github.com/konveyor/openshift-velero-plugin/velero-plugins/util/test" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/vmware-tanzu/velero/pkg/plugin/velero" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +func TestK8sRestorePluginAppliesTo(t *testing.T) { + restorePlugin := &K8sRestorePlugin{Log: test.NewLogger()} + + expectedResources := []string{"rolebindings"} + + selectedResources, err := restorePlugin.AppliesTo() + require.NoError(t, err) + + assert.Equal(t, expectedResources, selectedResources.IncludedResources) +} + +func TestK8sExecuteSystemRoleBindings(t *testing.T) { + restorePlugin := &K8sRestorePlugin{Log: logrus.New()} + + tests := []struct { + name string + rbName string + shouldSkip bool + }{ + { + name: "Skip system:image-pullers", + rbName: "system:image-pullers", + shouldSkip: true, + }, + { + name: "Skip system:image-builders", + rbName: "system:image-builders", + shouldSkip: true, + }, + { + name: "Skip system:deployers", + rbName: "system:deployers", + shouldSkip: true, + }, + { + name: "Don't skip regular rolebinding", + rbName: "my-custom-rolebinding", + shouldSkip: false, + }, + { + name: "Don't skip rolebinding with system: prefix but not in list", + rbName: "system:custom-role", + shouldSkip: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + roleBinding := rbacv1.RoleBinding{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "rbac.authorization.k8s.io/v1", + Kind: "RoleBinding", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: tt.rbName, + Namespace: "test-namespace", + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "ClusterRole", + Name: "test-role", + }, + } + + unstructuredObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&roleBinding) + require.NoError(t, err) + + item := &unstructured.Unstructured{Object: unstructuredObj} + + input := &velero.RestoreItemActionExecuteInput{ + Item: item, + Restore: &velerov1.Restore{ + Spec: velerov1.RestoreSpec{ + NamespaceMapping: map[string]string{}, + }, + }, + } + + output, err := restorePlugin.Execute(input) + require.NoError(t, err) + assert.Equal(t, tt.shouldSkip, output.SkipRestore) + }) + } +} diff --git a/velero-plugins/rolebindings/restore.go b/velero-plugins/rolebindings/ocp_restore.go similarity index 95% rename from velero-plugins/rolebindings/restore.go rename to velero-plugins/rolebindings/ocp_restore.go index 9d483029..1ab94896 100644 --- a/velero-plugins/rolebindings/restore.go +++ b/velero-plugins/rolebindings/ocp_restore.go @@ -11,10 +11,10 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -// systemRoleBindings contains rolebindings that are automatically created by OpenShift -// These rolebindings are expected to be created by the system and don't need restoring +// SystemRoleBindings contains rolebindings that are automatically created by OpenShift +// when a new project/namespace is created. These don't need restoring. // Reference: https://github.com/openshift/openshift-apiserver/blob/eefb161cffdc97a949d6e9cc81aa900005912a97/pkg/project/apiserver/registry/projectrequest/delegated/delegated.go#L111 -var systemRoleBindings = map[string]bool{ +var SystemRoleBindings = map[string]bool{ "system:image-pullers": true, "system:image-builders": true, "system:deployers": true, @@ -42,7 +42,7 @@ func (p *RestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput) (*v p.Log.Infof("[rolebinding-restore] role binding - %s, API version", roleBinding.Name, roleBinding.APIVersion) - if systemRoleBindings[roleBinding.Name] { + if SystemRoleBindings[roleBinding.Name] { p.Log.Infof("[rolebinding-restore] Skipping system rolebinding %s as it will be automatically created", roleBinding.Name) return &velero.RestoreItemActionExecuteOutput{ SkipRestore: true, diff --git a/velero-plugins/rolebindings/restore_test.go b/velero-plugins/rolebindings/ocp_restore_test.go similarity index 100% rename from velero-plugins/rolebindings/restore_test.go rename to velero-plugins/rolebindings/ocp_restore_test.go From b464d600d6687ca46862fb0205767171610a78d2 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 8 Jun 2026 12:55:43 -0400 Subject: [PATCH 2/3] fix: handle json.Marshal/Unmarshal errors in k8s restore plugin Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- velero-plugins/rolebindings/k8s_restore.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/velero-plugins/rolebindings/k8s_restore.go b/velero-plugins/rolebindings/k8s_restore.go index 138bb452..a7c128f2 100644 --- a/velero-plugins/rolebindings/k8s_restore.go +++ b/velero-plugins/rolebindings/k8s_restore.go @@ -25,8 +25,13 @@ func (p *K8sRestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput) p.Log.Info("[rbac-rolebinding-restore] Entering RBAC Role Bindings restore plugin") roleBinding := rbacv1.RoleBinding{} - itemMarshal, _ := json.Marshal(input.Item) - json.Unmarshal(itemMarshal, &roleBinding) + itemMarshal, err := json.Marshal(input.Item) + if err != nil { + return nil, err + } + if err = json.Unmarshal(itemMarshal, &roleBinding); err != nil { + return nil, err + } p.Log.Infof("[rbac-rolebinding-restore] role binding - %s, API version %s", roleBinding.Name, roleBinding.APIVersion) From 56f52aac4707d4c2c82799acb2d0248f757dec28 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 8 Jun 2026 13:56:22 -0400 Subject: [PATCH 3/3] fix: mock GetBucketRegion in shared_test.go TestGetRegistryEnvsForLocation was hitting real S3 to determine bucket region, causing CI failures when no AWS credentials are available. Mock GetBucketRegionFunc like s3_test.go and registry_test.go already do. Also fix wantErrString to match the full wrapped error from GetRegistryEnvsForLocation. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- velero-plugins/imagestream/shared_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/velero-plugins/imagestream/shared_test.go b/velero-plugins/imagestream/shared_test.go index 8685fe42..daf87ac8 100644 --- a/velero-plugins/imagestream/shared_test.go +++ b/velero-plugins/imagestream/shared_test.go @@ -167,6 +167,13 @@ func TestGetRegistryEnvsForLocation(t *testing.T) { t.Fatal(err) } defer testEnv.Stop() + + originalGetBucketRegionFunc := GetBucketRegionFunc + GetBucketRegionFunc = func(bucket string) (string, error) { + return "us-east-2", nil + } + defer func() { GetBucketRegionFunc = originalGetBucketRegionFunc }() + clients.SetInClusterConfig(cfg) client, err := dynamic.NewForConfig(cfg) if err != nil {