Use CRD defaults

[mateusoliveira43]

Problem

OADP does not use default value markers for generating its CRDs

// +kubebuilder:default=<value>

The adoption of this would be better for:

• developers

  Less code to write. Functions like this would not be necessary, as API would ensure a value is present on the field

  oadp-operator/api/v1alpha1/oadp_types.go

  Lines 614 to 617 in fb2411e

  	// Default BackupImages behavior when nil to true
  	func (dpa *DataProtectionApplication) BackupImages() bool {
  	return dpa.Spec.BackupImages == nil || *dpa.Spec.BackupImages
  	}

• users

  Default values would be more visible to users. Easier to see when default values are updated

Examples

If user creates a DPA with this spec

spec:
  configuration:
    velero:
      logLevel: debug

it would have this spec in the cluster

spec:
  configuration:
    velero:
      client-burst: 100
      client-qps: 100
      defaultItemOperationTimeout: 1h
      disableInformerCache: false
      itemOperationSyncFrequency: 2m
      logLevel: debug
      noDefaultBackupLocation: false
      resourceTimeout: 10m

Note: default disallows user to remove field from object. If user try to remove a field with default from spec, API just adds it back with the default value.

Upgrades would work with these changes. A sample DPA prior to upgrade

apiVersion: oadp.openshift.io/v1alpha1
kind: DataProtectionApplication
metadata:
  creationTimestamp: '2024-10-14T14:47:20Z'
  generation: 3
  managedFields:
    ...
  name: velero-sample
  namespace: test-oadp-operator
  resourceVersion: '566563332'
  uid: 9f8ff5f5-041b-40bd-ba2e-6f7e658c7515
spec:
  backupImages: false
  configuration:
    velero:
      logLevel: debug
      noDefaultBackupLocation: true

The same DPA after upgarde

apiVersion: oadp.openshift.io/v1alpha1
kind: DataProtectionApplication
metadata:
  creationTimestamp: '2024-10-14T14:47:20Z'
  generation: 3
  managedFields:
    ...
  name: velero-sample
  namespace: test-oadp-operator
  resourceVersion: '566563332'
  uid: 9f8ff5f5-041b-40bd-ba2e-6f7e658c7515
spec:
  backupImages: false
  configuration:
    velero:
      client-burst: 100
      client-qps: 100
      defaultItemOperationTimeout: 1h
      disableInformerCache: false
      itemOperationSyncFrequency: 2m
      logLevel: debug
      noDefaultBackupLocation: true
      resourceTimeout: 10m

Note: even though the object changed, note that metadata.generation is the same on both objects.

References

https://book.kubebuilder.io/reference/markers/crd-validation
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#defaulting

Note: some default values are not static, so they may be added with other strategies.

[kaovilai]

 // Default BackupImages behavior when nil to true 
 func (dpa *DataProtectionApplication) BackupImages() bool { 
 	return dpa.Spec.BackupImages == nil || *dpa.Spec.BackupImages 
 } 

These were added because at the time we want to add configurability while hiding that these are defaulted to true since all users essentially had this on but we want to give the option to turn it off.

We didn't want to require DPA update to restore prior behavior.

If we had used CRD marker defaults, it would've added a new line to everyone's dpa, which while clearer was seen as "changing user's dpa", an undesired behavior.

[kaovilai]

Essentially, What You oc create -f is what you oc get dpa <name> -oyaml.

There are enough examples of controllers / CRDs displaying defaults to users' created CR, so the prior behavior was not a hard requirement.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[kaovilai]

/lifecycle frozen