From a3c66dd0ab6b055c9503ef05cc7cb40106fe1695 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Fri, 4 Feb 2022 14:43:50 +0000 Subject: [PATCH] add taskspec resources validation --- pkg/reconciler/taskrun/taskrun.go | 6 + pkg/reconciler/taskrun/taskrun_test.go | 216 +++++++++++++++++++ pkg/reconciler/taskrun/validate_resources.go | 26 +++ 3 files changed, 248 insertions(+) diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 762ac004cac..e7e837404df 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -320,6 +320,12 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 return nil, nil, controller.NewPermanentError(err) } + if err := validateTaskSpecRequestResources(ctx, taskSpec); err != nil { + logger.Errorf("TaskRun %s taskSpec request resources are invalid: %v", tr.Name, err) + tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) + return nil, nil, controller.NewPermanentError(err) + } + if err := ValidateResolvedTaskResources(ctx, tr.Spec.Params, rtr); err != nil { logger.Errorf("TaskRun %q resources are invalid: %v", tr.Name, err) tr.Status.MarkResourceFailed(podconvert.ReasonFailedValidation, err) diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index bed38f0636f..4f92b5d47f1 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -3379,6 +3379,50 @@ func TestReconcileTaskResourceResolutionAndValidation(t *testing.T) { "Warning Failed", // Event about the TaskRun state changed "Warning InternalError", // Event about the error (generated by the genreconciler) }, + }, { + desc: "Fail ValidateTaskSpecRequestResources", + d: test.Data{ + Tasks: []*v1beta1.Task{{ + ObjectMeta: objectMeta("test-task-invalid-taskspec-resource", "foo"), + Spec: v1beta1.TaskSpec{ + Workspaces: []v1beta1.WorkspaceDeclaration{{ + Name: "ws1", + Description: "a test task workspace", + ReadOnly: true, + }}, + Steps: []v1beta1.Step{{Container: corev1.Container{ + Image: "image", + Command: []string{"cmd"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }}}}, + }}, + TaskRuns: []*v1beta1.TaskRun{{ + ObjectMeta: objectMeta("test-taskrun-invalid-taskspec-resource", "foo"), + Spec: v1beta1.TaskRunSpec{ + TaskRef: &v1beta1.TaskRef{ + Name: "test-task-invalid-taskspec-resource", + APIVersion: "a1", + }, + }, + }}, + ClusterTasks: nil, + PipelineResources: nil, + }, + wantFailedReason: podconvert.ReasonFailedValidation, + wantEvents: []string{ + "Normal Started ", + "Warning Failed", // Event about the TaskRun state changed + "Warning InternalError", // Event about the error (generated by the genreconciler) + }, }} { t.Run(tt.desc, func(t *testing.T) { testAssets, cancel := getTaskRunController(t, tt.d) @@ -4371,6 +4415,178 @@ func TestStopSidecars_NoClientGetPodForTaskSpecWithoutRunningSidecars(t *testing } } +func Test_validateTaskSpecRequestResources_ValidResources(t *testing.T) { + ctx := context.Background() + + tcs := []struct { + name string + taskSpec *v1beta1.TaskSpec + }{{ + name: "no requested resources", + taskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{ + { + Container: corev1.Container{ + Image: "image", + Command: []string{"cmd"}, + }, + }}, + StepTemplate: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + }, + }, + }, + }, { + name: "no limit configured", + taskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Container: corev1.Container{ + Image: "image", + Command: []string{"cmd"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }}}, + }, + }, { + name: "request less or equal than step limit but larger than steptemplate limit", + taskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Container: corev1.Container{ + Image: "image", + Command: []string{"cmd"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + }, + }}}, + StepTemplate: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("4"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, + }, { + name: "request less or equal than step limit", + taskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Container: corev1.Container{ + Image: "image", + Command: []string{"cmd"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + }, + }}}, + }, + }, { + name: "request less or equal than steptemplate limit", + taskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Container: corev1.Container{ + Image: "image", + Command: []string{"cmd"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }}}, + StepTemplate: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + }, + }, + }, + }} + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + if err := validateTaskSpecRequestResources(ctx, tc.taskSpec); err != nil { + t.Fatalf("Expected to see error when validating invalid TaskSpec resources but saw none") + } + }) + } + +} + +func Test_validateTaskSpecRequestResources_InvalidResources(t *testing.T) { + ctx := context.Background() + tcs := []struct { + name string + taskSpec *v1beta1.TaskSpec + }{{ + name: "step request larger than step limit", + taskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Container: corev1.Container{ + Image: "image", + Command: []string{"cmd"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }}}}, + }, { + name: "step request larger than steptemplate limit", + taskSpec: &v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{Container: corev1.Container{ + Image: "image", + Command: []string{"cmd"}, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("8Gi"), + }, + }, + }}}, + StepTemplate: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + }, + }, + }, + }, + }} + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + if err := validateTaskSpecRequestResources(ctx, tc.taskSpec); err == nil { + t.Fatalf("Expected to see error when validating invalid TaskSpec resources but saw none") + } + }) + } +} + func podVolumeMounts(idx, totalSteps int) []corev1.VolumeMount { var mnts []corev1.VolumeMount mnts = append(mnts, corev1.VolumeMount{ diff --git a/pkg/reconciler/taskrun/validate_resources.go b/pkg/reconciler/taskrun/validate_resources.go index d7b38472971..213b5763a9f 100644 --- a/pkg/reconciler/taskrun/validate_resources.go +++ b/pkg/reconciler/taskrun/validate_resources.go @@ -142,3 +142,29 @@ func ValidateResolvedTaskResources(ctx context.Context, params []v1beta1.Param, return nil } + +func validateTaskSpecRequestResources(ctx context.Context, taskSpec *v1beta1.TaskSpec) error { + if taskSpec != nil { + for _, step := range taskSpec.Steps { + for k, request := range step.Resources.Requests { + // First validate the limit in step + if limit, ok := step.Resources.Limits[k]; ok { + if (&limit).Cmp(request) == -1 { + return fmt.Errorf("Invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) + } + } else if taskSpec.StepTemplate != nil { + // If step doesn't configure the limit, validate the limit in stepTemplate + if limit, ok := taskSpec.StepTemplate.Resources.Limits[k]; ok { + if (&limit).Cmp(request) == -1 { + return fmt.Errorf("Invalid request resource value: %v must be less or equal to limit %v", request.String(), limit.String()) + } + } + + } + + } + } + } + + return nil +}