Merge pull request #9132 from mjnagel/crd-upgrade

feat: add apply flag to install command

changelogs/unreleased/9132-mjnagel

@@ -0,0 +1 @@
+Add `--apply` flag to `install` command, allowing usage of Kubernetes apply to make changes to existing installs

design/Implemented/apply-flag.md

@@ -0,0 +1,70 @@
+# Apply flag for install command
+
+## Abstract
+Add an `--apply` flag to the install command that enables applying existing resources rather than creating them. This can be useful as part of the upgrade process for existing installations.
+
+## Background
+The current Velero install command creates resources but doesn't provide a direct way to apply updates to an existing installation.
+Users attempting to run the install command on an existing installation receive "already exists" messages.
+Upgrade steps for existing installs typically involve a three (or more) step process to apply updated CRDs (using `--dry-run` and piping to `kubectl apply`) and then updating/setting images on the Velero deployment and node-agent.
+
+## Goals
+- Provide a simple flag to enable applying resources on an existing Velero installation.
+- Use server-side apply to update existing resources rather than attempting to create them.
+- Maintain consistency with the regular install flow.
+
+## Non Goals
+- Implement special logic for specific version-to-version upgrades (i.e. resource deletion, etc).
+- Add complex upgrade validation or pre/post-upgrade hooks.
+- Provide rollback capabilities.
+
+## High-Level Design
+The `--apply` flag will be added to the Velero install command.
+When this flag is set, the installation process will use server-side apply to update existing resources instead of using create on new resources.
+This flag can be used as _part_ of the upgrade process, but will not always fully handle an upgrade.
+
+## Detailed Design
+The implementation adds a new boolean flag `--apply` to the install command.
+This flag will be passed through to the underlying install functions where the resource creation logic resides.
+
+When the flag is set to true:
+- The `createOrApplyResource` function will use server-side apply with field manager "velero-cli" and `force=true` to update resources.
+- Resources will be applied in the same order as they would be created during installation.
+- Custom Resource Definitions will still be processed first, and the system will wait for them to be established before continuing.
+
+The server-side apply approach with `force=true` ensures that resources are updated even if there are conflicts with the last applied state.
+This provides a best-effort mechanism to apply resources that follows the same flow as installation but updates resources instead of creating them.
+
+No special handling is added for specific versions or resource structures, making this a general-purpose mechanism for applying resources.
+
+## Alternatives Considered
+1. Creating a separate `upgrade` command that would duplicate much of the install command logic.
+   - Rejected due to code duplication and maintenance overhead.
+
+2. Implementing version-specific upgrade logic to handle breaking changes between versions.
+   - Rejected as overly complex and difficult to maintain across multiple version paths.
+   - This could be considered again in the future, but is not in the scope of the current design.
+
+3. Adding automatic detection of existing resources and switching to apply mode.
+   - Rejected as it could lead to unexpected behavior and confusion if users unintentionally apply changes to existing resources.
+
+## Security Considerations
+The apply flag maintains the same security profile as the install command.
+No additional permissions are required beyond what is needed for resource creation.
+The use of `force=true` with server-side apply could potentially override manual changes made to resources, but this is a necessary trade-off to ensure apply is successful.
+
+## Compatibility
+This enhancement is compatible with all existing Velero installations as it is a new opt-in flag.
+It does not change any resource formats or API contracts.
+The apply process is best-effort and does not guarantee compatibility between arbitrary versions of Velero.
+Users should still consult release notes for any breaking changes that may require manual intervention.
+This flag could be adopted by the helm chart, specifically for CRD updates, to simplify the CRD update job.
+
+## Implementation
+The implementation involves:
+1. Adding support for `Apply` to the existing Kubernetes client code.
+1. Adding the `--apply` flag to the install command options.
+1. Changing `createResource` to `createOrApplyResource` and updating it to use server-side apply when the `apply` boolean is set.
+
+The implementation is straightforward and follows existing code patterns.
+No migration of state or special handling of specific resources is required.

pkg/client/dynamic.go

@@ -102,6 +102,11 @@ type StatusUpdater interface {
 	UpdateStatus(obj *unstructured.Unstructured, opts metav1.UpdateOptions) (*unstructured.Unstructured, error)
 }
 
+// Applier applies changes to an object using server-side apply
+type Applier interface {
+	Apply(name string, obj *unstructured.Unstructured, opts metav1.ApplyOptions) (*unstructured.Unstructured, error)
+}
+
 // Dynamic contains client methods that Velero needs for backing up and restoring resources.
 type Dynamic interface {
 	Creator
@@ -111,6 +116,7 @@ type Dynamic interface {
 	Patcher
 	Deletor
 	StatusUpdater
+	Applier
 }
 
 // dynamicResourceClient implements Dynamic.
@@ -136,6 +142,10 @@ func (d *dynamicResourceClient) Get(name string, opts metav1.GetOptions) (*unstr
 	return d.resourceClient.Get(context.TODO(), name, opts)
 }
 
+func (d *dynamicResourceClient) Apply(name string, obj *unstructured.Unstructured, opts metav1.ApplyOptions) (*unstructured.Unstructured, error) {
+	return d.resourceClient.Apply(context.TODO(), name, obj, opts)
+}
+
 func (d *dynamicResourceClient) Patch(name string, data []byte) (*unstructured.Unstructured, error) {
 	return d.resourceClient.Patch(context.TODO(), name, types.MergePatchType, data, metav1.PatchOptions{})
 }

pkg/cmd/cli/install/install.go

@@ -92,6 +92,7 @@ type Options struct {
 	ConcurrentBackups               int
 	NodeAgentDisableHostPath        bool
 	kubeletRootDir                  string
+	Apply                           bool
 	ServerPriorityClassName         string
 	NodeAgentPriorityClassName      string
 }
@@ -102,6 +103,7 @@ func (o *Options) BindFlags(flags *pflag.FlagSet) {
 	flags.StringVar(&o.BucketName, "bucket", o.BucketName, "Name of the object storage bucket where backups should be stored")
 	flags.StringVar(&o.SecretFile, "secret-file", o.SecretFile, "File containing credentials for backup and volume provider. If not specified, --no-secret must be used for confirmation. Optional.")
 	flags.BoolVar(&o.NoSecret, "no-secret", o.NoSecret, "Flag indicating if a secret should be created. Must be used as confirmation if --secret-file is not provided. Optional.")
+	flags.BoolVar(&o.Apply, "apply", o.Apply, "Flag indicating if resources should be applied instead of created. This can be used for updating existing resources.")
 	flags.BoolVar(&o.NoDefaultBackupLocation, "no-default-backup-location", o.NoDefaultBackupLocation, "Flag indicating if a default backup location should be created. Must be used as confirmation if --bucket or --provider are not provided. Optional.")
 	flags.StringVar(&o.Image, "image", o.Image, "Image to use for the Velero and node agent pods. Optional.")
 	flags.StringVar(&o.Prefix, "prefix", o.Prefix, "Prefix under which all Velero data should be stored within the bucket. Optional.")
@@ -416,7 +418,7 @@ func (o *Options) Run(c *cobra.Command, f client.Factory) error {
 
 	errorMsg := fmt.Sprintf("\n\nError installing Velero. Use `kubectl logs deploy/velero -n %s` to check the deploy logs", o.Namespace)
 
-	err = install.Install(dynamicFactory, kbClient, resources, os.Stdout)
+	err = install.Install(dynamicFactory, kbClient, resources, os.Stdout, o.Apply)
 	if err != nil {
 		return errors.Wrap(err, errorMsg)
 	}

pkg/install/install.go

@@ -278,30 +278,45 @@ func GroupResources(resources *unstructured.UnstructuredList) *ResourceGroup {
 	return rg
 }
 
-// createResource attempts to create a resource in the cluster.
+// createOrApplyResource attempts to create or apply a resource in the cluster.
-// If the resource already exists in the cluster, it's merely logged.
+// If apply is true, it uses server-side apply to update existing resources.
-func createResource(r *unstructured.Unstructured, factory client.DynamicFactory, w io.Writer) error {
+// If apply is false and the resource already exists in the cluster, it's merely logged.
+func createOrApplyResource(r *unstructured.Unstructured, factory client.DynamicFactory, w io.Writer, apply bool) error {
 	id := fmt.Sprintf("%s/%s", r.GetKind(), r.GetName())
 
 	// Helper to reduce boilerplate message about the same object
-	log := func(f string, a ...any) {
+	log := func(f string) {
-		format := strings.Join([]string{id, ": ", f, "\n"}, "")
+		fmt.Fprintf(w, "%s: %s\n", id, f)
-		fmt.Fprintf(w, format, a...)
 	}
-	log("attempting to create resource")
 
 	c, err := CreateClient(r, factory, w)
 	if err != nil {
 		return err
 	}
 
-	if _, err := c.Create(r); apierrors.IsAlreadyExists(err) {
+	if apply {
-		log("already exists, proceeding")
+		log("attempting to apply resource")
-	} else if err != nil {
+		// Set field manager for server-side apply and force to override conflicts
-		return errors.Wrapf(err, "Error creating resource %s", id)
+		applyOpts := metav1.ApplyOptions{
+			FieldManager: "velero-cli",
+			Force:        true,
+		}
+
+		if _, err := c.Apply(r.GetName(), r, applyOpts); err != nil {
+			return errors.Wrapf(err, "Error applying resource %s", id)
+		}
+		log("applied")
+	} else {
+		log("attempting to create resource")
+		if _, err := c.Create(r); apierrors.IsAlreadyExists(err) {
+			log("already exists, proceeding")
+		} else if err != nil {
+			return errors.Wrapf(err, "Error creating resource %s", id)
+		} else {
+			log("created")
+		}
 	}
 
-	log("created")
 	return nil
 }
 
@@ -335,13 +350,14 @@ func CreateClient(r *unstructured.Unstructured, factory client.DynamicFactory, w
 // An unstructured list of resources is sent, one at a time, to the server. These are assumed to be in the preferred order already.
 // Resources will be sorted into CustomResourceDefinitions and any other resource type, and the function will wait up to 1 minute
 // for CRDs to be ready before proceeding.
+// If apply is true, it uses server-side apply to update existing resources.
 // An io.Writer can be used to output to a log or the console.
-func Install(dynamicFactory client.DynamicFactory, kbClient kbclient.Client, resources *unstructured.UnstructuredList, w io.Writer) error {
+func Install(dynamicFactory client.DynamicFactory, kbClient kbclient.Client, resources *unstructured.UnstructuredList, w io.Writer, apply bool) error {
 	rg := GroupResources(resources)
 
 	//Install CRDs first
 	for _, r := range rg.CRDResources {
-		if err := createResource(r, dynamicFactory, w); err != nil {
+		if err := createOrApplyResource(r, dynamicFactory, w, apply); err != nil {
 			return err
 		}
 	}
@@ -357,7 +373,7 @@ func Install(dynamicFactory client.DynamicFactory, kbClient kbclient.Client, res
 
 	// Install all other resources
 	for _, r := range rg.OtherResources {
-		if err = createResource(r, dynamicFactory, w); err != nil {
+		if err = createOrApplyResource(r, dynamicFactory, w, apply); err != nil {
 			return err
 		}
 	}

pkg/install/install_test.go

@@ -1,6 +1,8 @@
 package install
 
 import (
+	"bytes"
+	"errors"
 	"os"
 	"testing"
 	"time"
@@ -12,9 +14,11 @@ import (
 	corev1api "k8s.io/api/core/v1"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	v1crds "github.com/vmware-tanzu/velero/config/crd/v1/crds"
@@ -53,7 +57,7 @@ func TestInstall(t *testing.T) {
 	require.NoError(t, appendUnstructured(resources, v1crds.CRDs[0]))
 	require.NoError(t, appendUnstructured(resources, Namespace("velero")))
 
-	assert.NoError(t, Install(factory, c, resources, os.Stdout))
+	assert.NoError(t, Install(factory, c, resources, os.Stdout, false))
 }
 
 func Test_crdsAreReady(t *testing.T) {
@@ -168,3 +172,233 @@ func TestNodeAgentWindowsIsReady(t *testing.T) {
 	require.NoError(t, err)
 	assert.True(t, ready)
 }
+
+func TestCreateOrApplyResourceError(t *testing.T) {
+	r := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]any{
+				"name":      "test-configmap",
+				"namespace": "velero",
+			},
+		},
+	}
+
+	dc := &test.FakeDynamicClient{}
+	expectedErr := errors.New("create error")
+	dc.On("Create", mock.Anything).Return(&unstructured.Unstructured{}, expectedErr)
+
+	factory := &test.FakeDynamicFactory{}
+	factory.On("ClientForGroupVersionResource", mock.Anything, mock.Anything, mock.Anything).Return(dc, nil)
+
+	var buf bytes.Buffer
+	err := createOrApplyResource(r, factory, &buf, false)
+
+	require.Error(t, err)
+	require.Contains(t, err.Error(), expectedErr.Error())
+}
+
+func TestCreateOrApplyResourceAlreadyExists(t *testing.T) {
+	r := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]any{
+				"name":      "test-configmap",
+				"namespace": "velero",
+			},
+		},
+	}
+
+	dc := &test.FakeDynamicClient{}
+	alreadyExistsErr := apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, "test-configmap")
+	// We need to return a non-nil unstructured object even though it's not used
+	dc.On("Create", mock.Anything).Return(&unstructured.Unstructured{}, alreadyExistsErr)
+
+	factory := &test.FakeDynamicFactory{}
+	factory.On("ClientForGroupVersionResource", mock.Anything, mock.Anything, mock.Anything).Return(dc, nil)
+
+	var buf bytes.Buffer
+	err := createOrApplyResource(r, factory, &buf, false)
+
+	require.NoError(t, err)
+}
+
+func TestCreateOrApplyResourceClientError(t *testing.T) {
+	r := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]any{
+				"name":      "test-configmap",
+				"namespace": "velero",
+			},
+		},
+	}
+
+	factory := &test.FakeDynamicFactory{}
+	expectedErr := errors.New("client creation error")
+	// Return error from ClientForGroupVersionResource
+	factory.On("ClientForGroupVersionResource", mock.Anything, mock.Anything, mock.Anything).Return(&test.FakeDynamicClient{}, expectedErr)
+
+	var buf bytes.Buffer
+	err := createOrApplyResource(r, factory, &buf, false)
+
+	require.Error(t, err)
+	require.Contains(t, err.Error(), expectedErr.Error())
+}
+
+func TestCreateOrApplyResourceApplyError(t *testing.T) {
+	r := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]any{
+				"name":      "test-configmap",
+				"namespace": "velero",
+			},
+		},
+	}
+
+	dc := &test.FakeDynamicClient{}
+	expectedErr := errors.New("apply error")
+	// Mock Apply to return an error
+	dc.On("Apply", mock.Anything, mock.Anything, mock.Anything).Return(&unstructured.Unstructured{}, expectedErr)
+
+	factory := &test.FakeDynamicFactory{}
+	factory.On("ClientForGroupVersionResource", mock.Anything, mock.Anything, mock.Anything).Return(dc, nil)
+
+	var buf bytes.Buffer
+	err := createOrApplyResource(r, factory, &buf, true) // true for apply flag to use Apply
+
+	require.Error(t, err)
+	require.Contains(t, err.Error(), expectedErr.Error())
+}
+
+func TestInstallErrorAfterCreateClient(t *testing.T) {
+	// Create a test non-CRD resource
+	nonCRDResource := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]any{
+				"name": "test-configmap",
+			},
+		},
+	}
+
+	resources := &unstructured.UnstructuredList{
+		Items: []unstructured.Unstructured{*nonCRDResource},
+	}
+
+	// Mock the factory to return a client that will succeed on ClientForGroupVersionResource
+	// but fail on Create
+	dc := &test.FakeDynamicClient{}
+	expectedErr := errors.New("create error after successful client creation")
+	dc.On("Create", mock.Anything).Return(&unstructured.Unstructured{}, expectedErr)
+
+	factory := &test.FakeDynamicFactory{}
+	factory.On("ClientForGroupVersionResource", mock.Anything, mock.Anything, mock.Anything).Return(dc, nil)
+
+	c := fake.NewClientBuilder().Build()
+
+	var buf bytes.Buffer
+	err := Install(factory, c, resources, &buf, false)
+
+	require.Error(t, err)
+	require.Contains(t, err.Error(), expectedErr.Error())
+}
+
+func TestInstallErrorOnCRDResource(t *testing.T) {
+	crdResource := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "apiextensions.k8s.io/v1",
+			"kind":       "CustomResourceDefinition",
+			"metadata": map[string]any{
+				"name": "test-crd",
+			},
+		},
+	}
+
+	resources := &unstructured.UnstructuredList{
+		Items: []unstructured.Unstructured{*crdResource},
+	}
+
+	dc := &test.FakeDynamicClient{}
+	expectedErr := errors.New("error creating CRD resource")
+	// We need to return a non-nil unstructured object even though it's not used
+	dc.On("Create", mock.Anything).Return(&unstructured.Unstructured{}, expectedErr)
+
+	factory := &test.FakeDynamicFactory{}
+	factory.On("ClientForGroupVersionResource", mock.Anything, mock.Anything, mock.Anything).Return(dc, nil)
+
+	c := fake.NewClientBuilder().Build()
+
+	var buf bytes.Buffer
+	err := Install(factory, c, resources, &buf, false)
+
+	require.Error(t, err)
+	require.Contains(t, err.Error(), expectedErr.Error())
+}
+
+func TestInstallWithApplyFlag(t *testing.T) {
+	// Create a test resource
+	testResource := &unstructured.Unstructured{
+		Object: map[string]any{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]any{
+				"name":      "test-configmap",
+				"namespace": "velero",
+			},
+			"data": map[string]any{
+				"key1": "value1",
+			},
+		},
+	}
+
+	resources := &unstructured.UnstructuredList{
+		Items: []unstructured.Unstructured{*testResource},
+	}
+
+	// Test case 1: Without apply flag (create)
+	{
+		dc := &test.FakeDynamicClient{}
+		// Expect Create to be called
+		dc.On("Create", mock.Anything).Return(testResource, nil)
+		// Apply should not be called
+
+		factory := &test.FakeDynamicFactory{}
+		factory.On("ClientForGroupVersionResource", mock.Anything, mock.Anything, mock.Anything).Return(dc, nil)
+
+		c := fake.NewClientBuilder().Build()
+
+		err := Install(factory, c, resources, os.Stdout, false)
+		require.NoError(t, err)
+
+		// Verify that Create was called and Apply was not
+		dc.AssertCalled(t, "Create", mock.Anything)
+		dc.AssertNotCalled(t, "Apply", mock.Anything, mock.Anything, mock.Anything)
+	}
+
+	// Test case 2: With apply flag
+	{
+		dc := &test.FakeDynamicClient{}
+		// Create should not be called
+		// Expect Apply to be called
+		dc.On("Apply", mock.Anything, mock.Anything, mock.Anything).Return(testResource, nil)
+
+		factory := &test.FakeDynamicFactory{}
+		factory.On("ClientForGroupVersionResource", mock.Anything, mock.Anything, mock.Anything).Return(dc, nil)
+
+		c := fake.NewClientBuilder().Build()
+
+		err := Install(factory, c, resources, os.Stdout, true)
+		require.NoError(t, err)
+
+		// Verify that Apply was called and Create was not
+		dc.AssertCalled(t, "Apply", mock.Anything, mock.Anything, mock.Anything)
+		dc.AssertNotCalled(t, "Create", mock.Anything)
+	}
+}

pkg/test/fake_dynamic.go

@@ -83,3 +83,8 @@ func (c *FakeDynamicClient) UpdateStatus(obj *unstructured.Unstructured, opts me
 	args := c.Called(obj, opts)
 	return args.Get(0).(*unstructured.Unstructured), args.Error(1)
 }
+
+func (c *FakeDynamicClient) Apply(name string, obj *unstructured.Unstructured, opts metav1.ApplyOptions) (*unstructured.Unstructured, error) {
+	args := c.Called(name, obj, opts)
+	return args.Get(0).(*unstructured.Unstructured), args.Error(1)
+}

site/content/docs/main/customize-installation.md

@@ -42,6 +42,16 @@ By default, `velero install` does not enable the use of File System Backup (FSB)
 
 If you are planning to only use FSB for volume backups, you can run the `velero install` command with the `--default-volumes-to-fs-backup` flag. This will default all pod volumes backups to use FSB without having to apply annotations to pods. Note that when this flag is set during install, Velero will always try to use FSB to perform the backup, even want an individual backup to use volume snapshots, by setting the `--snapshot-volumes` flag in the `backup create` command. Alternatively, you can set the  `--default-volumes-to-fs-backup` on an individual backup to to make sure Velero uses FSB for each volume being backed up.
 
+## Update an existing installation
+
+By default, the `velero install` command creates new resources in your cluster. If you're updating an existing Velero installation, you can use the `--apply` flag to apply changes to existing resources instead of attempting to create new ones:
+
+```bash
+velero install --apply
+```
+
+When the `--apply` flag is specified, Velero uses server-side apply to update existing resources. This is particularly useful when updating Velero to a new version or when modifying your installation configuration. While this can be used as part of an upgrade process, please note that for version upgrades, additional steps may be required depending on the specific changes between versions. Also ensure when using this flag that you are setting any additional flags previously used for your existing configuration so that you don't introduce unexpected changes.
+
 ## Enable features
 
 New features in Velero will be released as beta features behind feature flags which are not enabled by default. A full listing of Velero feature flags can be found [here][11].