From 302bfbaaac98fd889aefbabe0573946fe2b10b44 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Mon, 15 Jun 2026 12:16:23 -0700 Subject: [PATCH 1/3] Add design for server default restore resource modifier Introduces a --default-resource-modifier-configmap server flag that references a ConfigMap with resource modifier rules applied automatically to all restores. This eliminates per-restore configuration for common transformations like stripping stale CNI annotations (OVN-Kubernetes, Multus) that can break workloads after restore. Key design decisions: - Exclusive precedence: per-restore modifiers fully replace the default - Non-fatal default errors: misconfigured default does not break restores - Opt-out via SkipDefaultResourceModifier field in RestoreSpec - Ships with a curated example ConfigMap for CNI annotation stripping Signed-off-by: Shubham Pampattiwar --- design/default-resource-modifier_design.md | 400 +++++++++++++++++++++ 1 file changed, 400 insertions(+) create mode 100644 design/default-resource-modifier_design.md diff --git a/design/default-resource-modifier_design.md b/design/default-resource-modifier_design.md new file mode 100644 index 000000000..0a7bffef7 --- /dev/null +++ b/design/default-resource-modifier_design.md @@ -0,0 +1,400 @@ +# Server Default Restore Resource Modifier + +- [Server Default Restore Resource Modifier](#server-default-restore-resource-modifier) + - [Abstract](#abstract) + - [Background](#background) + - [Goals](#goals) + - [Non Goals](#non-goals) + - [High-Level Design](#high-level-design) + - [Detailed Design](#detailed-design) + - [Server Configuration](#server-configuration) + - [Restore API Change](#restore-api-change) + - [Controller Logic](#controller-logic) + - [Restore CLI](#restore-cli) + - [Install Path](#install-path) + - [Curated Default ConfigMap Example](#curated-default-configmap-example) + - [Alternatives Considered](#alternatives-considered) + - [Security Considerations](#security-considerations) + - [Compatibility](#compatibility) + - [Implementation](#implementation) + - [Open Issues](#open-issues) + +## Abstract + +This proposal introduces a server-level default restore resource modifier for Velero. +A new `--default-resource-modifier-configmap` flag on the Velero server references a ConfigMap containing resource modifier rules that apply automatically to every restore, eliminating the need for per-restore configuration for common transformations like stripping stale CNI annotations. + +## Background + +When pods are backed up, CNI-managed annotations may be present that carry pod-specific networking state such as IP addresses, MAC addresses, and routes. +Restoring these stale values can cause networking failures because the CNI expects to inject fresh values and the restored annotations may conflict with the new cluster's network state. + +The following annotations are commonly affected: + +| Annotation | CNI | +|---|---| +| `k8s.ovn.org/pod-networks` | OVN-Kubernetes | +| `k8s.v1.cni.cncf.io/network-status` | Multus | +| `k8s.v1.cni.cncf.io/networks-status` | Multus | + +Today, users can strip these annotations using [Resource Modifiers](https://velero.io/docs/main/restore-resource-modifiers/), but this requires authoring a ConfigMap and referencing it on every restore via `--resource-modifier-configmap`. +This is not discoverable for users unfamiliar with the feature and adds friction for a problem that affects most OpenShift and multi-CNI deployments. + +Velero already strips certain annotations during restore as built-in behavior (e.g., `volume.kubernetes.io/selected-node` from PVCs). +This proposal extends that concept by allowing administrators to configure a default set of resource modifier rules at the server level. + +## Goals + +- Allow Velero administrators to configure a default resource modifier ConfigMap that applies to all restores without per-restore configuration. +- Provide a mechanism for individual restores to opt out of the default modifier. +- Ship a documented example ConfigMap that strips well-known CNI annotations. + +## Non Goals + +- Auto-creating a default ConfigMap during `velero install`. The mechanism is opt-in; administrators create and configure the ConfigMap. +- Merging default and per-restore resource modifier rules. When a per-restore modifier is specified, it takes exclusive precedence over the default. +- Supporting non-ConfigMap sources for default modifiers (e.g., CRDs, inline rules). +- Stripping CNI annotations via a built-in RestoreItemAction plugin. The resource modifier mechanism is the right abstraction for this. + +## High-Level Design + +A new `--default-resource-modifier-configmap` server flag references a ConfigMap name in the Velero namespace. +During restore, if no per-restore resource modifier is specified, the server loads and applies the default ConfigMap's rules. +When a per-restore modifier is specified via `--resource-modifier-configmap`, it takes exclusive precedence and the default is not applied. +A new `--skip-default-resource-modifier` flag on `velero restore create` allows opting out of the default per-restore. + +This follows the existing pattern used by `--backup-repository-configmap` and `--repo-maintenance-job-configmap`. + +## Detailed Design + +### Server Configuration + +Add a new field to the server `Config` struct and bind it as a CLI flag. + +In `pkg/cmd/server/config/config.go`: + +```go +type Config struct { + // ... existing fields ... + DefaultResourceModifierConfigMap string +} +``` + +```go +func (c *Config) BindFlags(flags *pflag.FlagSet) { + // ... existing flags ... + flags.StringVar( + &c.DefaultResourceModifierConfigMap, + "default-resource-modifier-configmap", + c.DefaultResourceModifierConfigMap, + "The name of a ConfigMap in the Velero namespace containing default resource modifier rules applied to all restores. "+ + "Ignored when a per-restore resource modifier is specified.", + ) +} +``` + +The default value is an empty string, meaning no default modifier is configured. +No change to `GetDefaultConfig()` is needed. + +### Restore API Change + +Add a new field to `RestoreSpec` for opting out of the default modifier. + +In `pkg/apis/velero/v1/restore_types.go`: + +```go +type RestoreSpec struct { + // ... existing fields ... + + // SkipDefaultResourceModifier controls whether the server-configured default + // resource modifier is applied to this restore. + // When true, the default modifier is skipped even if configured on the server. + // Has no effect when a per-restore ResourceModifier is specified. + // +optional + SkipDefaultResourceModifier bool `json:"skipDefaultResourceModifier,omitempty"` +} +``` + + +### Controller Logic + +Thread the new config value through to the restore controller and implement the precedence logic. + +In `pkg/controller/restore_controller.go`, add a field to `restoreReconciler`: + +```go +type restoreReconciler struct { + // ... existing fields ... + defaultResourceModifierConfigMap string +} +``` + +Update `NewRestoreReconciler` to accept and store the new parameter. + +In `pkg/cmd/server/server.go`, pass `s.config.DefaultResourceModifierConfigMap` to `NewRestoreReconciler`. + +Refactor `validateAndComplete` to use a shared helper for ConfigMap loading and implement the precedence logic: + +```go +func (r *restoreReconciler) validateAndComplete(restore *api.Restore) (backupInfo, *resourcemodifiers.ResourceModifiers) { + // ... existing validation logic (unchanged) ... + + // Resource modifier resolution: per-restore takes exclusive precedence over default. + var resourceModifiers *resourcemodifiers.ResourceModifiers + + if restore.Spec.ResourceModifier != nil && + strings.EqualFold(restore.Spec.ResourceModifier.Kind, resourcemodifiers.ConfigmapRefType) { + // Per-restore modifier specified: use it exclusively, ignore default. + resourceModifiers = r.loadResourceModifierConfigMap( + restore, restore.Spec.ResourceModifier.Name, false, + ) + } else if r.defaultResourceModifierConfigMap != "" && !restore.Spec.SkipDefaultResourceModifier { + // No per-restore modifier: apply server default if configured and not skipped. + resourceModifiers = r.loadResourceModifierConfigMap( + restore, r.defaultResourceModifierConfigMap, true, + ) + } + + return info, resourceModifiers +} +``` + +Extract the ConfigMap loading into a helper to avoid code duplication: + +```go +// loadResourceModifierConfigMap loads and validates a resource modifier ConfigMap. +// When isDefault is true, errors are non-fatal (logged as warnings, returns nil). +// When isDefault is false, errors are added to restore.Status.ValidationErrors. +func (r *restoreReconciler) loadResourceModifierConfigMap( + restore *api.Restore, cmName string, isDefault bool, +) *resourcemodifiers.ResourceModifiers { + cm := &corev1api.ConfigMap{} + if err := r.kbClient.Get( + context.Background(), + client.ObjectKey{Namespace: restore.Namespace, Name: cmName}, + cm, + ); err != nil { + if isDefault { + r.logger.WithError(err).Warnf( + "Default resource modifier configmap %s/%s not found, skipping", + restore.Namespace, cmName, + ) + return nil + } + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, + fmt.Sprintf("failed to get resource modifiers configmap %s/%s", restore.Namespace, cmName)) + return nil + } + + modifiers, err := resourcemodifiers.GetResourceModifiersFromConfig(cm) + if err != nil { + if isDefault { + r.logger.WithError(err).Warnf( + "Error parsing default resource modifier configmap %s/%s, skipping", + restore.Namespace, cmName, + ) + return nil + } + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, + errors.Wrapf(err, "Error in parsing resource modifiers provided in configmap %s/%s", + restore.Namespace, cmName).Error()) + return nil + } + + if err = modifiers.Validate(); err != nil { + if isDefault { + r.logger.WithError(err).Warnf( + "Validation error in default resource modifier configmap %s/%s, skipping", + restore.Namespace, cmName, + ) + return nil + } + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, + errors.Wrapf(err, "Validation error in resource modifiers provided in configmap %s/%s", + restore.Namespace, cmName).Error()) + return nil + } + + source := "per-restore" + if isDefault { + source = "default" + } + r.logger.Infof("Retrieved %s resource modifiers from configmap %s/%s", source, restore.Namespace, cmName) + return modifiers +} +``` + +Key design decisions in this logic: + +1. **Exclusive precedence**: When a per-restore modifier is specified, the default is not applied at all. +This is the simplest mental model and avoids complex merge semantics. +Users who want both default and custom rules can copy the default rules into their per-restore ConfigMap. + +2. **Non-fatal default errors**: If the default ConfigMap is missing or invalid, log a warning and proceed without it. +A misconfigured default should not break all restores cluster-wide. +Per-restore modifier errors remain fatal (validation errors), preserving current behavior. + +3. **SkipDefaultResourceModifier**: Allows opting out per-restore without specifying a per-restore modifier. +Has no effect when a per-restore modifier is specified (it already takes precedence). + +### Restore CLI + +Add a `--skip-default-resource-modifier` flag to `velero restore create`. + +In `pkg/cmd/cli/restore/create.go`: + +```go +type CreateOptions struct { + // ... existing fields ... + SkipDefaultResourceModifier bool +} +``` + +```go +func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { + // ... existing flags ... + flags.BoolVar(&o.SkipDefaultResourceModifier, "skip-default-resource-modifier", false, + "Skip applying the server-configured default resource modifier for this restore") +} +``` + +Set the field on the RestoreSpec when building the Restore object: + +```go +Spec: api.RestoreSpec{ + // ... existing fields ... + SkipDefaultResourceModifier: o.SkipDefaultResourceModifier, +} +``` + +Update the restore describer in `pkg/cmd/util/output/restore_describer.go` to display the field when set. + +### Install Path + +Add the flag to the install CLI and deployment builder so administrators can configure it during installation. + +In `pkg/install/deployment.go`, add a `defaultResourceModifierConfigMap` field to `podTemplateConfig` with an option function: + +```go +func WithDefaultResourceModifierConfigMap(name string) podTemplateOption { + return func(c *podTemplateConfig) { + c.defaultResourceModifierConfigMap = name + } +} +``` + +In the `Deployment()` function, append the CLI arg: + +```go +if len(c.defaultResourceModifierConfigMap) > 0 { + args = append(args, fmt.Sprintf("--default-resource-modifier-configmap=%s", + c.defaultResourceModifierConfigMap)) +} +``` + +Wire it through `VeleroOptions` in `pkg/install/resources.go` and the install CLI in `pkg/cmd/cli/install/install.go`. + +Add a builder method to `pkg/builder/restore_builder.go`: + +```go +func (b *RestoreBuilder) SkipDefaultResourceModifier(val bool) *RestoreBuilder { + b.object.Spec.SkipDefaultResourceModifier = val + return b +} +``` + +### Curated Default ConfigMap Example + +Provide a ready-to-use ConfigMap in `examples/default-resource-modifier-cni.yaml` that strips well-known CNI annotations: + +```yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: default-restore-resource-modifiers + namespace: velero +data: + resource-modifiers.yaml: | + version: v1 + resourceModifierRules: + - conditions: + groupResource: pods + mergePatches: + - patchData: | + metadata: + annotations: + k8s.ovn.org/pod-networks: null + k8s.v1.cni.cncf.io/network-status: null + k8s.v1.cni.cncf.io/networks-status: null +``` + +This uses JSON Merge Patch to remove annotations by setting them to `null`. +Administrators can extend this ConfigMap with additional CNI-specific annotations (Calico, Cilium, etc.) or other stale metadata as needed. + +Usage: +```bash +# Create the ConfigMap +kubectl apply -f examples/default-resource-modifier-cni.yaml + +# Configure the Velero server to use it +# Option 1: During install +velero install --default-resource-modifier-configmap=default-restore-resource-modifiers ... + +# Option 2: Edit existing deployment +kubectl -n velero edit deploy velero +# Add: --default-resource-modifier-configmap=default-restore-resource-modifiers +``` + +## Alternatives Considered + +**Merge default and per-restore rules**: Instead of exclusive precedence, concatenate default and per-restore rules so both apply. +This avoids users having to copy default rules when specifying per-restore modifiers. +However, it introduces complexity around rule ordering and makes it harder to reason about what transformations will be applied. +It also makes it impossible to fully override the default for a specific restore without the `SkipDefaultResourceModifier` flag. +Exclusive precedence was chosen for simplicity. +Merge semantics can be revisited in a future enhancement if user demand warrants it. + +**Built-in RestoreItemAction plugin**: Implement CNI annotation stripping as a built-in RIA plugin rather than using the resource modifier mechanism. +This would hard-code the logic and make it less configurable. +The resource modifier mechanism already supports this use case and is more flexible. + +**Validate default ConfigMap at server startup**: Validate the ConfigMap when the server starts rather than at restore time. +Rejected because the ConfigMap may be created after the server starts and should not require a server restart to take effect. + +**Auto-create default ConfigMap during install**: Have `velero install` automatically create the CNI-stripping ConfigMap. +Rejected for the initial release to minimize the change surface and let administrators opt in. +Can be added later as a default behavior or install flag. + +## Security Considerations + +No new security surface. +The default ConfigMap resides in the Velero namespace and is subject to the same RBAC controls as existing resource modifier ConfigMaps. +Only users with access to create/edit ConfigMaps in the Velero namespace can modify the default modifier rules. + +## Compatibility + +Fully backward compatible. +When `--default-resource-modifier-configmap` is not set (the default), behavior is identical to current Velero. +No changes to existing per-restore resource modifier behavior. +The new `SkipDefaultResourceModifier` field in RestoreSpec defaults to `false` and has no effect when no default modifier is configured. + +## Implementation + +1. Add `DefaultResourceModifierConfigMap` to `Config` struct and bind the CLI flag. +2. Add `SkipDefaultResourceModifier` to `RestoreSpec` and regenerate deepcopy/CRD. +3. Thread the config to `restoreReconciler` via `NewRestoreReconciler`. +4. Refactor `validateAndComplete` with `loadResourceModifierConfigMap` helper. +5. Add `--skip-default-resource-modifier` to the restore CLI. +6. Wire through the install path (deployment builder, install CLI). +7. Add unit tests for all precedence and error scenarios. +8. Create the example ConfigMap. +9. Update user documentation. +10. Add E2E test for default resource modifier. + + +## Open Issues + +- Should additional CNI annotations (Calico, Cilium) be included in the curated example ConfigMap? +Feedback from the community on which annotations are commonly problematic would be helpful. +- Should `velero restore describe` show which resource modifier was used (default vs per-restore)? +This would improve observability but is a minor enhancement that can be added separately. From 89dc9b06b26b9bb62e703963c2d9177ddff98f4a Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Mon, 15 Jun 2026 12:20:13 -0700 Subject: [PATCH 2/3] Add changelog for PR #9921 Signed-off-by: Shubham Pampattiwar --- changelogs/unreleased/9921-shubham-pampattiwar | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/9921-shubham-pampattiwar diff --git a/changelogs/unreleased/9921-shubham-pampattiwar b/changelogs/unreleased/9921-shubham-pampattiwar new file mode 100644 index 000000000..6475cfe58 --- /dev/null +++ b/changelogs/unreleased/9921-shubham-pampattiwar @@ -0,0 +1 @@ +Design: Server default restore resource modifier From 3deb7d14042eaf7ad88c48e2d5eebd1d257a4370 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Thu, 25 Jun 2026 15:24:44 -0700 Subject: [PATCH 3/3] Address review comments on design doc - Use *bool for SkipDefaultResourceModifier to match RestoreSpec conventions - Use boolptr.IsSetToTrue for nil-safe bool check in controller logic - Fix warning message to say "failed to retrieve" instead of "not found" - Add Restore Describe Output subsection covering all resolved states - Only set SkipDefaultResourceModifier from CLI when flag is true Signed-off-by: Shubham Pampattiwar --- design/default-resource-modifier_design.md | 39 ++++++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/design/default-resource-modifier_design.md b/design/default-resource-modifier_design.md index 0a7bffef7..5f10b3b6b 100644 --- a/design/default-resource-modifier_design.md +++ b/design/default-resource-modifier_design.md @@ -11,6 +11,7 @@ - [Restore API Change](#restore-api-change) - [Controller Logic](#controller-logic) - [Restore CLI](#restore-cli) + - [Restore Describe Output](#restore-describe-output) - [Install Path](#install-path) - [Curated Default ConfigMap Example](#curated-default-configmap-example) - [Alternatives Considered](#alternatives-considered) @@ -111,10 +112,14 @@ type RestoreSpec struct { // When true, the default modifier is skipped even if configured on the server. // Has no effect when a per-restore ResourceModifier is specified. // +optional - SkipDefaultResourceModifier bool `json:"skipDefaultResourceModifier,omitempty"` + // +nullable + SkipDefaultResourceModifier *bool `json:"skipDefaultResourceModifier,omitempty"` } ``` +This follows the existing RestoreSpec convention where optional booleans use `*bool` with `+nullable` (e.g., `RestorePVs`, `PreserveNodePorts`, `IncludeClusterResources`). +This preserves the ability to distinguish "unset" from "explicit false" if needed in the future. + ### Controller Logic @@ -148,7 +153,7 @@ func (r *restoreReconciler) validateAndComplete(restore *api.Restore) (backupInf resourceModifiers = r.loadResourceModifierConfigMap( restore, restore.Spec.ResourceModifier.Name, false, ) - } else if r.defaultResourceModifierConfigMap != "" && !restore.Spec.SkipDefaultResourceModifier { + } else if r.defaultResourceModifierConfigMap != "" && !boolptr.IsSetToTrue(restore.Spec.SkipDefaultResourceModifier) { // No per-restore modifier: apply server default if configured and not skipped. resourceModifiers = r.loadResourceModifierConfigMap( restore, r.defaultResourceModifierConfigMap, true, @@ -176,7 +181,7 @@ func (r *restoreReconciler) loadResourceModifierConfigMap( ); err != nil { if isDefault { r.logger.WithError(err).Warnf( - "Default resource modifier configmap %s/%s not found, skipping", + "Failed to retrieve default resource modifier configmap %s/%s, skipping", restore.Namespace, cmName, ) return nil @@ -258,16 +263,30 @@ func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { } ``` -Set the field on the RestoreSpec when building the Restore object: +Set the field on the RestoreSpec when building the Restore object. +Only set it when the flag is true (using `boolptr.True()`) to leave it nil otherwise, consistent with how other `*bool` fields are handled: ```go -Spec: api.RestoreSpec{ - // ... existing fields ... - SkipDefaultResourceModifier: o.SkipDefaultResourceModifier, +if o.SkipDefaultResourceModifier { + restore.Spec.SkipDefaultResourceModifier = boolptr.True() } ``` -Update the restore describer in `pkg/cmd/util/output/restore_describer.go` to display the field when set. +### Restore Describe Output + +Update the restore describer in `pkg/cmd/util/output/restore_describer.go` to show which resource modifier was applied and its source. +The describe output should reflect the resolved state: + +- When the default resource modifier was applied, display its ConfigMap name and source: + ``` + Default Resource Modifier: default-restore-resource-modifiers + ``` +- When the default was skipped because `SkipDefaultResourceModifier` is true: + ``` + Default Resource Modifier: skipped (SkipDefaultResourceModifier=true) + ``` +- When the default was skipped because a per-restore modifier was specified, no extra output is needed since the per-restore modifier is already displayed under the existing `Resource Modifier` field. +- When the default was ignored due to a validation or retrieval error, the warning is already logged to the restore log. The describe output should not surface transient errors. ### Install Path @@ -298,7 +317,7 @@ Add a builder method to `pkg/builder/restore_builder.go`: ```go func (b *RestoreBuilder) SkipDefaultResourceModifier(val bool) *RestoreBuilder { - b.object.Spec.SkipDefaultResourceModifier = val + b.object.Spec.SkipDefaultResourceModifier = &val return b } ``` @@ -396,5 +415,3 @@ The new `SkipDefaultResourceModifier` field in RestoreSpec defaults to `false` a - Should additional CNI annotations (Calico, Cilium) be included in the curated example ConfigMap? Feedback from the community on which annotations are commonly problematic would be helpful. -- Should `velero restore describe` show which resource modifier was used (default vs per-restore)? -This would improve observability but is a minor enhancement that can be added separately.