Incorporate review comments

Makefile

@@ -1,4 +1,4 @@
-# # /bin/sh does not support source command needed in make test-all
+# # /bin/sh does not support source command needed in make all-test
 #SHELL := /bin/bash
 
 ROOT_DIR=$(shell git rev-parse --show-toplevel)

api/v1beta1/aerospikebackup_types.go

@@ -21,15 +21,15 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
-// AerospikeBackupSpec defines the desired state of AerospikeBackup
+// AerospikeBackupSpec defines the desired state of AerospikeBackup for a given AerospikeCluster
 // +k8s:openapi-gen=true
 type AerospikeBackupSpec struct {
 	// BackupService is the backup service reference i.e. name and namespace.
 	// It is used to communicate to the backup service to trigger backups. This field is immutable
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Backup Service"
-	BackupService *BackupService `json:"backupService"`
+	BackupService BackupService `json:"backupService"`
 
-	// Config is the configuration for the backup in YAML format.
+	// Config is the free form configuration for the backup in YAML format.
 	// This config is used to trigger backups. It includes: aerospike-cluster, backup-routines.
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Backup Config"
 	Config runtime.RawExtension `json:"config"`
@@ -63,7 +63,7 @@ type OnDemandSpec struct {
 // AerospikeBackupStatus defines the observed state of AerospikeBackup
 type AerospikeBackupStatus struct {
 	// BackupService is the backup service reference i.e. name and namespace.
-	BackupService *BackupService `json:"backupService"`
+	BackupService BackupService `json:"backupService"`
 
 	// Config is the configuration for the backup in YAML format.
 	// This config is used to trigger backups. It includes: aerospike-cluster, backup-routines.

api/v1beta1/aerospikebackup_webhook.go

@@ -38,7 +38,7 @@ import (
 )
 
 // log is for logging in this package.
-var aerospikebackuplog = logf.Log.WithName("aerospikebackup-resource")
+var aerospikeBackupLog = logf.Log.WithName("aerospikebackup-resource")
 
 func (r *AerospikeBackup) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
@@ -50,7 +50,7 @@ var _ webhook.Defaulter = &AerospikeBackup{}
 
 // Default implements webhook.Defaulter so a webhook will be registered for the type
 func (r *AerospikeBackup) Default() {
-	aerospikebackuplog.Info("default", "name", r.Name)
+	aerospikeBackupLog.Info("default", "name", r.Name)
 }
 
 //nolint:lll // for readability
@@ -60,7 +60,7 @@ var _ webhook.Validator = &AerospikeBackup{}
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (r *AerospikeBackup) ValidateCreate() (admission.Warnings, error) {
-	aerospikebackuplog.Info("validate create", "name", r.Name)
+	aerospikeBackupLog.Info("validate create", "name", r.Name)
 
 	if len(r.Spec.OnDemand) != 0 && r.Spec.Config.Raw != nil {
 		return nil, fmt.Errorf("onDemand and backup config cannot be specified together while creating backup")
@@ -75,7 +75,7 @@ func (r *AerospikeBackup) ValidateCreate() (admission.Warnings, error) {
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
 func (r *AerospikeBackup) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
-	aerospikebackuplog.Info("validate update", "name", r.Name)
+	aerospikeBackupLog.Info("validate update", "name", r.Name)
 
 	oldObj := old.(*AerospikeBackup)
 
@@ -107,7 +107,7 @@ func (r *AerospikeBackup) ValidateUpdate(old runtime.Object) (admission.Warnings
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (r *AerospikeBackup) ValidateDelete() (admission.Warnings, error) {
-	aerospikebackuplog.Info("validate delete", "name", r.Name)
+	aerospikeBackupLog.Info("validate delete", "name", r.Name)
 
 	// TODO(user): fill in your validation logic upon object deletion.
 	return nil, nil
@@ -127,106 +127,57 @@ func (r *AerospikeBackup) validateBackupConfig() error {
 		return err
 	}
 
-	var config model.Config
+	var backupSvcConfig model.Config
 
-	if err := yaml.Unmarshal(backupSvc.Spec.Config.Raw, &config); err != nil {
+	if err := yaml.Unmarshal(backupSvc.Spec.Config.Raw, &backupSvcConfig); err != nil {
 		return err
 	}
 
-	configMap := make(map[string]interface{})
+	backupSvcConfigMap := make(map[string]interface{})
 
-	if err := yaml.Unmarshal(r.Spec.Config.Raw, &configMap); err != nil {
+	if err := yaml.Unmarshal(r.Spec.Config.Raw, &backupSvcConfigMap); err != nil {
 		return err
 	}
 
-	if _, ok := configMap[common.AerospikeClusterKey]; !ok {
-		return fmt.Errorf("aerospike-cluster field is required in config")
-	}
-
-	cluster, ok := configMap[common.AerospikeClusterKey].(map[string]interface{})
-	if !ok {
-		return fmt.Errorf("aerospike-cluster field is not in the right format")
-	}
-
-	clusterBytes, cErr := yaml.Marshal(cluster)
-	if cErr != nil {
-		return cErr
-	}
-
-	aeroClusters := make(map[string]*model.AerospikeCluster)
-
-	if err := yaml.Unmarshal(clusterBytes, &aeroClusters); err != nil {
+	aeroClusters, err := validateAerospikeCluster(&backupSvcConfig, backupSvcConfigMap)
+	if err != nil {
 		return err
 	}
 
-	if len(aeroClusters) != 1 {
-		return fmt.Errorf("only one aerospike cluster is allowed in backup config")
-	}
-
-	if len(config.AerospikeClusters) == 0 {
-		config.AerospikeClusters = make(map[string]*model.AerospikeCluster)
-	}
-
-	for name, aeroCluster := range aeroClusters {
-		config.AerospikeClusters[name] = aeroCluster
-	}
-
-	if _, ok = configMap[common.BackupRoutinesKey]; !ok {
-		return fmt.Errorf("backup-routines field is required in config")
-	}
-
-	routines, ok := configMap[common.BackupRoutinesKey].(map[string]interface{})
-	if !ok {
-		return fmt.Errorf("backup-routines field is not in the right format")
-	}
-
-	routineBytes, rErr := yaml.Marshal(routines)
-	if rErr != nil {
-		return rErr
-	}
-
-	backupRoutines := make(map[string]*model.BackupRoutine)
-
-	if err := yaml.Unmarshal(routineBytes, &backupRoutines); err != nil {
+	backupRoutines, err := validateBackupRoutines(&backupSvcConfig, backupSvcConfigMap)
+	if err != nil {
 		return err
 	}
 
-	if len(config.BackupRoutines) == 0 {
-		config.BackupRoutines = make(map[string]*model.BackupRoutine)
-	}
-
-	for name, routine := range backupRoutines {
-		config.BackupRoutines[name] = routine
-	}
-
 	// validate if only correct aerospike cluster (the one referred in Backup CR) is used in backup routines
-	for _, routine := range config.BackupRoutines {
-		if _, ok = aeroClusters[routine.SourceCluster]; !ok {
-			return fmt.Errorf("cluster %s not found in backup config", routine.SourceCluster)
+	for _, routine := range backupRoutines {
+		if _, ok := aeroClusters[routine.SourceCluster]; !ok {
+			return fmt.Errorf("cluster %s not found in backup backupSvcConfig", routine.SourceCluster)
 		}
 	}
 
-	// Add empty placeholders for missing config sections. This is required for validation to work.
-	if config.ServiceConfig == nil {
-		config.ServiceConfig = &model.BackupServiceConfig{}
+	// Add empty placeholders for missing backupSvcConfig sections. This is required for validation to work.
+	if backupSvcConfig.ServiceConfig == nil {
+		backupSvcConfig.ServiceConfig = &model.BackupServiceConfig{}
 	}
 
-	if config.ServiceConfig.HTTPServer == nil {
-		config.ServiceConfig.HTTPServer = &model.HTTPServerConfig{}
+	if backupSvcConfig.ServiceConfig.HTTPServer == nil {
+		backupSvcConfig.ServiceConfig.HTTPServer = &model.HTTPServerConfig{}
 	}
 
-	if config.ServiceConfig.Logger == nil {
-		config.ServiceConfig.Logger = &model.LoggerConfig{}
+	if backupSvcConfig.ServiceConfig.Logger == nil {
+		backupSvcConfig.ServiceConfig.Logger = &model.LoggerConfig{}
 	}
 
-	if err := config.Validate(); err != nil {
+	if err := backupSvcConfig.Validate(); err != nil {
 		return err
 	}
 
 	// Validate on-demand backup
 	if len(r.Spec.OnDemand) > 0 {
-		if _, ok = config.BackupRoutines[r.Spec.OnDemand[0].RoutineName]; !ok {
-			return fmt.Errorf("backup routine %s not found", r.Spec.OnDemand[0].RoutineName)
+		if _, ok := backupSvcConfig.BackupRoutines[r.Spec.OnDemand[0].RoutineName]; !ok {
+			return fmt.Errorf("invalid onDemand config, backup routine %s not found",
+				r.Spec.OnDemand[0].RoutineName)
 		}
 	}
 
@@ -251,3 +202,73 @@ func getK8sClient() (client.Client, error) {
 
 	return cl, nil
 }
+
+func validateAerospikeCluster(backupSvcConfig *model.Config, backupSvcConfigMap map[string]interface{},
+) (map[string]*model.AerospikeCluster, error) {
+	if _, ok := backupSvcConfigMap[common.AerospikeClusterKey]; !ok {
+		return nil, fmt.Errorf("aerospike-cluster field is required in backupSvcConfig")
+	}
+
+	cluster, ok := backupSvcConfigMap[common.AerospikeClusterKey].(map[string]interface{})
+	if !ok {
+		return nil, fmt.Errorf("aerospike-cluster field is not in the right format")
+	}
+
+	clusterBytes, cErr := yaml.Marshal(cluster)
+	if cErr != nil {
+		return nil, cErr
+	}
+
+	aeroClusters := make(map[string]*model.AerospikeCluster)
+
+	if err := yaml.Unmarshal(clusterBytes, &aeroClusters); err != nil {
+		return nil, err
+	}
+
+	if len(aeroClusters) != 1 {
+		return aeroClusters, fmt.Errorf("only one aerospike cluster is allowed in backup backupSvcConfig")
+	}
+
+	if len(backupSvcConfig.AerospikeClusters) == 0 {
+		backupSvcConfig.AerospikeClusters = make(map[string]*model.AerospikeCluster)
+	}
+
+	for name, aeroCluster := range aeroClusters {
+		backupSvcConfig.AerospikeClusters[name] = aeroCluster
+	}
+
+	return aeroClusters, nil
+}
+
+func validateBackupRoutines(backupSvcConfig *model.Config, backupSvcConfigMap map[string]interface{},
+) (map[string]*model.BackupRoutine, error) {
+	if _, ok := backupSvcConfigMap[common.BackupRoutinesKey]; !ok {
+		return nil, fmt.Errorf("backup-routines field is required in backupSvcConfig")
+	}
+
+	routines, ok := backupSvcConfigMap[common.BackupRoutinesKey].(map[string]interface{})
+	if !ok {
+		return nil, fmt.Errorf("backup-routines field is not in the right format")
+	}
+
+	routineBytes, rErr := yaml.Marshal(routines)
+	if rErr != nil {
+		return nil, rErr
+	}
+
+	backupRoutines := make(map[string]*model.BackupRoutine)
+
+	if err := yaml.Unmarshal(routineBytes, &backupRoutines); err != nil {
+		return nil, err
+	}
+
+	if len(backupSvcConfig.BackupRoutines) == 0 {
+		backupSvcConfig.BackupRoutines = make(map[string]*model.BackupRoutine)
+	}
+
+	for name, routine := range backupRoutines {
+		backupSvcConfig.BackupRoutines[name] = routine
+	}
+
+	return backupRoutines, nil
+}

api/v1beta1/aerospikebackupservice_types.go

@@ -48,7 +48,7 @@ type AerospikeBackupServiceSpec struct {
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Backup Service Image"
 	Image string `json:"image"`
 
-	// Config is the configuration for the backup service in YAML format.
+	// Config is the free form configuration for the backup service in YAML format.
 	// This config is used to start the backup service. The config is passed as a file to the backup service.
 	// It includes: service, backup-policies, storage, secret-agent.
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Backup Service Config"
@@ -57,7 +57,7 @@ type AerospikeBackupServiceSpec struct {
 	// Resources defines the requests and limits for the backup service container.
 	// Resources.Limits should be more than Resources.Requests.
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Resources"
-	Resources corev1.ResourceRequirements `json:"resources,omitempty"`
+	Resources *corev1.ResourceRequirements `json:"resources,omitempty"`
 
 	// SecretMounts is the list of secret to be mounted in the backup service.
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Backup Service Volume"
@@ -70,18 +70,36 @@ type AerospikeBackupServiceSpec struct {
 }
 
 // AerospikeBackupServiceStatus defines the observed state of AerospikeBackupService
+//
+//nolint:govet // for readbility
 type AerospikeBackupServiceStatus struct {
-	// ContextPath is the backup service API context path
-	ContextPath string `json:"contextPath"`
+	// Image is the image for the backup service.
+	Image string `json:"image,omitempty"`
+
+	// Config is the free form configuration for the backup service in YAML format.
+	// This config is used to start the backup service. The config is passed as a file to the backup service.
+	// It includes: service, backup-policies, storage, secret-agent.
+	Config runtime.RawExtension `json:"config,omitempty"`
+
+	// Resources defines the requests and limits for the backup service container.
+	// Resources.Limits should be more than Resources.Requests.
+	Resources *corev1.ResourceRequirements `json:"resources,omitempty"`
 
-	// ConfigHash is the hash string of backup service config
-	ConfigHash string `json:"configHash"`
+	// SecretMounts is the list of secret to be mounted in the backup service.
+	SecretMounts []SecretMount `json:"secrets,omitempty"`
+
+	// Service defines the Kubernetes service configuration for the backup service.
+	// It is used to expose the backup service deployment. By default, the service type is ClusterIP.
+	Service *Service `json:"service,omitempty"`
+
+	// ContextPath is the backup service API context path
+	ContextPath string `json:"contextPath,omitempty"`
 
 	// Phase denotes Backup service phase
-	Phase AerospikeBackupServicePhase `json:"phase,omitempty"`
+	Phase AerospikeBackupServicePhase `json:"phase"`
 
 	// Port is the listening port of backup service
-	Port int32 `json:"port"`
+	Port int32 `json:"port,omitempty"`
 }
 
 // +kubebuilder:object:root=true

api/v1beta1/aerospikebackupservice_webhook.go

@@ -28,7 +28,7 @@ import (
 )
 
 // log is for logging in this package.
-var aerospikebackupservicelog = logf.Log.WithName("aerospikebackupservice-resource")
+var aerospikeBackupServiceLog = logf.Log.WithName("aerospikebackupservice-resource")
 
 func (r *AerospikeBackupService) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
@@ -40,7 +40,7 @@ var _ webhook.Defaulter = &AerospikeBackupService{}
 
 // Default implements webhook.Defaulter so a webhook will be registered for the type
 func (r *AerospikeBackupService) Default() {
-	aerospikebackupservicelog.Info("default", "name", r.Name)
+	aerospikeBackupServiceLog.Info("default", "name", r.Name)
 }
 
 //nolint:lll // for readability
@@ -50,7 +50,7 @@ var _ webhook.Validator = &AerospikeBackupService{}
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (r *AerospikeBackupService) ValidateCreate() (admission.Warnings, error) {
-	aerospikebackupservicelog.Info("validate create", "name", r.Name)
+	aerospikeBackupServiceLog.Info("validate create", "name", r.Name)
 
 	if err := r.validateBackupServiceConfig(); err != nil {
 		return nil, err
@@ -61,7 +61,7 @@ func (r *AerospikeBackupService) ValidateCreate() (admission.Warnings, error) {
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
 func (r *AerospikeBackupService) ValidateUpdate(_ runtime.Object) (admission.Warnings, error) {
-	aerospikebackupservicelog.Info("validate update", "name", r.Name)
+	aerospikeBackupServiceLog.Info("validate update", "name", r.Name)
 
 	if err := r.validateBackupServiceConfig(); err != nil {
 		return nil, err
@@ -72,7 +72,7 @@ func (r *AerospikeBackupService) ValidateUpdate(_ runtime.Object) (admission.War
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (r *AerospikeBackupService) ValidateDelete() (admission.Warnings, error) {
-	aerospikebackupservicelog.Info("validate delete", "name", r.Name)
+	aerospikeBackupServiceLog.Info("validate delete", "name", r.Name)
 
 	// TODO(user): fill in your validation logic upon object deletion.
 	return nil, nil