Merge pull request #8 from bennsimon/add-workloadschedule-schedules-v…

…alidation

add validation to workloadschedule schedules

handler/scheduleHandler/handler.go

@@ -12,7 +12,7 @@ import (
 )
 
 type IScheduleHandler interface {
-	getScheduleByName(schedule string, r client.Reader, ctx context.Context) (*workloadschedulerv1.Schedule, error)
+	GetScheduleByName(schedule string, r client.Reader, ctx context.Context) (*workloadschedulerv1.Schedule, error)
 	FetchWorkloadSchedules(schedules []workloadschedulerv1.WorkloadScheduleUnit, r client.Reader, ctx context.Context) ([]workloadschedulerv1.Schedule, error)
 	IsThisDayIncluded(days []string, now time.Time) bool
 	ValidateSchedule(schedule *workloadschedulerv1.Schedule) error
@@ -74,7 +74,7 @@ func (s *ScheduleHandler) IsThisDayIncluded(days []string, now time.Time) bool {
 func (s *ScheduleHandler) FetchWorkloadSchedules(_schedules []workloadschedulerv1.WorkloadScheduleUnit, r client.Reader, ctx context.Context) ([]workloadschedulerv1.Schedule, error) {
 	var schedules []workloadschedulerv1.Schedule
 	for _, _schedule := range _schedules {
-		schedule, err := s.IScheduleHandler.getScheduleByName(_schedule.Schedule, r, ctx)
+		schedule, err := s.IScheduleHandler.GetScheduleByName(_schedule.Schedule, r, ctx)
 		if err != nil {
 			return nil, fmt.Errorf(fmt.Sprintf("error when fetching: %v schedule", _schedule), err)
 		} else {
@@ -84,7 +84,7 @@ func (s *ScheduleHandler) FetchWorkloadSchedules(_schedules []workloadschedulerv
 	return schedules, nil
 }
 
-func (s *ScheduleHandler) getScheduleByName(_schedule string, r client.Reader, ctx context.Context) (*workloadschedulerv1.Schedule, error) {
+func (s *ScheduleHandler) GetScheduleByName(_schedule string, r client.Reader, ctx context.Context) (*workloadschedulerv1.Schedule, error) {
 	schedule := &workloadschedulerv1.Schedule{}
 	err := r.Get(ctx, client.ObjectKey{Name: _schedule}, schedule)
 	if err != nil {

handler/scheduleHandler/handler_test.go

@@ -84,13 +84,13 @@ func TestScheduleHandler_getSchedulesByName(t *testing.T) {
 			s := &ScheduleHandler{}
 			tt.setupMocks()
 			defer tt.verifyMocks()
-			got, err := s.getScheduleByName(tt.args.schedule, testreader, tt.args.ctx)
+			got, err := s.GetScheduleByName(tt.args.schedule, testreader, tt.args.ctx)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("getScheduleByName() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("GetScheduleByName() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
 			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("getScheduleByName() got = %v, want %v", got, tt.want)
+				t.Errorf("GetScheduleByName() got = %v, want %v", got, tt.want)
 			}
 		})
 	}
@@ -101,7 +101,7 @@ type testScheduleHandler struct {
 	IScheduleHandler
 }
 
-func (s *testScheduleHandler) getScheduleByName(schedule string, r client.Reader, ctx context.Context) (*v1.Schedule, error) {
+func (s *testScheduleHandler) GetScheduleByName(schedule string, r client.Reader, ctx context.Context) (*v1.Schedule, error) {
 	args := s.Called(schedule, r, ctx)
 	if args.Get(0) == nil {
 		return nil, args.Error(1)
@@ -128,7 +128,7 @@ func TestScheduleHandler_CheckIfSchedulesExist(t *testing.T) {
 	}{
 		{name: "should return nil schedules when error occurs when getting schedule.", setupMocks: func() {
 			testschedulehandler = &testScheduleHandler{}
-			testschedulehandler.On("getScheduleByName", mock.IsType("weekday"), mock.IsType(&testReader{}), mock.IsType(context.TODO())).Return(nil, fmt.Errorf("some error"))
+			testschedulehandler.On("GetScheduleByName", mock.IsType("weekday"), mock.IsType(&testReader{}), mock.IsType(context.TODO())).Return(nil, fmt.Errorf("some error"))
 			s.IScheduleHandler = testschedulehandler
 		}, verifyMocks: func() {
 			testschedulehandler.AssertExpectations(t)
@@ -141,7 +141,7 @@ func TestScheduleHandler_CheckIfSchedulesExist(t *testing.T) {
 			want: nil, wantErr: true},
 		{name: "should return schedules when schedule retrieved successfully.", setupMocks: func() {
 			testschedulehandler = &testScheduleHandler{}
-			testschedulehandler.On("getScheduleByName", mock.IsType("weekday"), mock.IsType(&testReader{}), mock.IsType(context.TODO())).Return(&v1.Schedule{}, nil)
+			testschedulehandler.On("GetScheduleByName", mock.IsType("weekday"), mock.IsType(&testReader{}), mock.IsType(context.TODO())).Return(&v1.Schedule{}, nil)
 			s.IScheduleHandler = testschedulehandler
 		}, verifyMocks: func() {
 			testschedulehandler.AssertExpectations(t)

handler/workloadScheduleHandler/handler.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"github.com/alistanis/cartesian"
 	apps "k8s.io/api/apps/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sort"
@@ -28,8 +29,9 @@ type WorkloadHandler interface {
 type IWorkloadScheduleHandler interface {
 	EvaluateWorkloadSchedulers(schedulers *workloadschedulerv1.WorkloadScheduleList, r client.Reader, ctx context.Context) (map[string][]workloadschedulerv1.Schedule, map[string]workloadschedulerv1.WorkloadSchedule)
 	ProcessWorkloadSchedules(schedules map[string][]workloadschedulerv1.Schedule, schedulerMap map[string]workloadschedulerv1.WorkloadSchedule, c client.Client, ctx context.Context) error
-	ValidateWorkloadSchedule(schedule *workloadschedulerv1.WorkloadSchedule) error
+	ValidateWorkloadSchedule(schedule *workloadschedulerv1.WorkloadSchedule, r client.Reader) error
 }
+
 type DeploymentHandler struct {
 	config.Config
 	apps.Deployment
@@ -52,9 +54,21 @@ func New() *WorkloadScheduleHandler {
 	return &WorkloadScheduleHandler{ScheduleHandler: scheduleHandler.New(), Config: *config.New()}
 }
 
-func (w *WorkloadScheduleHandler) ValidateWorkloadSchedule(workloadSchedule *workloadschedulerv1.WorkloadSchedule) error {
+func (w *WorkloadScheduleHandler) ValidateWorkloadSchedule(workloadSchedule *workloadschedulerv1.WorkloadSchedule, r client.Reader) error {
 	if workloadSchedule.Spec.Schedules == nil || len(workloadSchedule.Spec.Schedules) == 0 {
 		return fmt.Errorf("schedules need to be defined")
+
+	} else {
+		for _, schedule := range workloadSchedule.Spec.Schedules {
+			if errs := validation.IsDNS1123Label(schedule.Schedule); errs != nil {
+				return fmt.Errorf("schedule: %s is not valid. %v", schedule.Schedule, errs)
+			}
+
+			_, err := w.ScheduleHandler.GetScheduleByName(schedule.Schedule, r, context.Background())
+			if err != nil {
+				return err
+			}
+		}
 	}
 	return nil
 }

handler/workloadScheduleHandler/handler_test.go

@@ -3,6 +3,7 @@ package workloadScheduleHandler
 import (
 	v1 "bennsimon.github.io/workload-scheduler-operator/api/v1"
 	"bennsimon.github.io/workload-scheduler-operator/handler/scheduleHandler"
+	"bennsimon.github.io/workload-scheduler-operator/util/config"
 	"context"
 	"fmt"
 	"github.com/stretchr/testify/mock"
@@ -258,3 +259,47 @@ func TestWorkloadScheduleHandler_BuildSpecMap(t *testing.T) {
 		})
 	}
 }
+
+type NopScheduleHandler struct {
+	scheduleHandler.IScheduleHandler
+}
+
+func (s *NopScheduleHandler) GetScheduleByName(_schedule string, r client.Reader, ctx context.Context) (*v1.Schedule, error) {
+	return nil, nil
+}
+
+func TestWorkloadScheduleHandler_ValidateWorkloadSchedule(t *testing.T) {
+	type fields struct {
+		ScheduleHandler scheduleHandler.IScheduleHandler
+		Config          config.Config
+	}
+	type args struct {
+		workloadSchedule *v1.WorkloadSchedule
+		r                client.Reader
+	}
+	tests := []struct {
+		name    string
+		fields  fields
+		args    args
+		wantErr bool
+	}{
+		{name: "should return error if schedules is empty", fields: fields{ScheduleHandler: &NopScheduleHandler{}}, args: args{workloadSchedule: &v1.WorkloadSchedule{Spec: v1.WorkloadScheduleSpec{Schedules: []v1.WorkloadScheduleUnit{}}}}, wantErr: true},
+		{name: "should return error if schedules is nil", fields: fields{ScheduleHandler: &NopScheduleHandler{}}, args: args{workloadSchedule: &v1.WorkloadSchedule{Spec: v1.WorkloadScheduleSpec{Schedules: nil}}}, wantErr: true},
+		{name: "should return error if schedule name is invalid", fields: fields{ScheduleHandler: &NopScheduleHandler{}}, args: args{workloadSchedule: &v1.WorkloadSchedule{Spec: v1.WorkloadScheduleSpec{Schedules: []v1.WorkloadScheduleUnit{{Schedule: "week day"}}}}}, wantErr: true},
+		{name: "should return error if schedule name is invalid", fields: fields{ScheduleHandler: &NopScheduleHandler{}}, args: args{workloadSchedule: &v1.WorkloadSchedule{Spec: v1.WorkloadScheduleSpec{Schedules: []v1.WorkloadScheduleUnit{{Schedule: "Weekday"}}}}}, wantErr: true},
+		{name: "should return error if schedule name is invalid", fields: fields{ScheduleHandler: &NopScheduleHandler{}}, args: args{workloadSchedule: &v1.WorkloadSchedule{Spec: v1.WorkloadScheduleSpec{Schedules: []v1.WorkloadScheduleUnit{{Schedule: ".weekday"}}}}}, wantErr: true},
+		{name: "should return error if schedule name is invalid", fields: fields{ScheduleHandler: &NopScheduleHandler{}}, args: args{workloadSchedule: &v1.WorkloadSchedule{Spec: v1.WorkloadScheduleSpec{Schedules: []v1.WorkloadScheduleUnit{{Schedule: ""}}}}}, wantErr: true},
+		{name: "should not return error if schedule name is valid", fields: fields{ScheduleHandler: &NopScheduleHandler{}}, args: args{workloadSchedule: &v1.WorkloadSchedule{Spec: v1.WorkloadScheduleSpec{Schedules: []v1.WorkloadScheduleUnit{{Schedule: "weekday"}}}}}, wantErr: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			w := &WorkloadScheduleHandler{
+				ScheduleHandler: tt.fields.ScheduleHandler,
+				Config:          tt.fields.Config,
+			}
+			if err := w.ValidateWorkloadSchedule(tt.args.workloadSchedule, tt.args.r); (err != nil) != tt.wantErr {
+				t.Errorf("ValidateWorkloadSchedule() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

internal/controller/workloadschedule_controller.go

@@ -46,7 +46,7 @@ func (r *WorkloadScheduleReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	err = r.IWorkloadScheduleHandler.ValidateWorkloadSchedule(workloadSchedule)
+	err = r.IWorkloadScheduleHandler.ValidateWorkloadSchedule(workloadSchedule, r)
 	if err != nil {
 		return ctrl.Result{}, err
 	}