From 156548655ec83dd79503c44b130dae4eadcc6b30 Mon Sep 17 00:00:00 2001 From: JeromeJu Date: Thu, 30 Nov 2023 18:53:12 +0000 Subject: [PATCH] Fix validations for Sidecars to be consistent This commit fixes the inconsistent validations for Sidecars by adding webhook validations. It previously only validate Sidecar at runtime instead of what's validated for Step at webhook. It also deletes the docString showing and are optional. BUT the labels of these two fields are kept for backwards compatibility purposes. /kind bug fixes: #7437 --- docs/pipeline-api.md | 8 +-- pkg/apis/pipeline/v1/container_types.go | 4 -- pkg/apis/pipeline/v1/openapi_generated.go | 4 +- pkg/apis/pipeline/v1/swagger.json | 4 +- pkg/apis/pipeline/v1/task_validation.go | 32 +++++++++--- pkg/apis/pipeline/v1/task_validation_test.go | 51 +++++++++++++++++++- test/conversion_test.go | 2 - 7 files changed, 81 insertions(+), 24 deletions(-) diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index a1908601521..3b3df81090d 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -3364,9 +3364,7 @@ string (Optional)

Image reference name. -More info: https://kubernetes.io/docs/concepts/containers/images -This field is optional to allow higher level config management to default or override -container images in workload controllers like Deployments and StatefulSets.

+More info: https://kubernetes.io/docs/concepts/containers/images

@@ -4329,9 +4327,7 @@ string (Optional)

Image reference name. -More info: https://kubernetes.io/docs/concepts/containers/images -This field is optional to allow higher level config management to default or override -container images in workload controllers like Deployments and StatefulSets.

+More info: https://kubernetes.io/docs/concepts/containers/images

diff --git a/pkg/apis/pipeline/v1/container_types.go b/pkg/apis/pipeline/v1/container_types.go index 79c9922f462..f18d4c185ae 100644 --- a/pkg/apis/pipeline/v1/container_types.go +++ b/pkg/apis/pipeline/v1/container_types.go @@ -192,8 +192,6 @@ func (s *Step) SetContainerFields(c corev1.Container) { type StepTemplate struct { // Image reference name. // More info: https://kubernetes.io/docs/concepts/containers/images - // This field is optional to allow higher level config management to default or override - // container images in workload controllers like Deployments and StatefulSets. // +optional Image string `json:"image,omitempty" protobuf:"bytes,2,opt,name=image"` // Entrypoint array. Not executed within a shell. @@ -312,8 +310,6 @@ type Sidecar struct { Name string `json:"name" protobuf:"bytes,1,opt,name=name"` // Image reference name. // More info: https://kubernetes.io/docs/concepts/containers/images - // This field is optional to allow higher level config management to default or override - // container images in workload controllers like Deployments and StatefulSets. // +optional Image string `json:"image,omitempty" protobuf:"bytes,2,opt,name=image"` // Entrypoint array. Not executed within a shell. diff --git a/pkg/apis/pipeline/v1/openapi_generated.go b/pkg/apis/pipeline/v1/openapi_generated.go index abb49a962e7..75bf7d298a5 100644 --- a/pkg/apis/pipeline/v1/openapi_generated.go +++ b/pkg/apis/pipeline/v1/openapi_generated.go @@ -2307,7 +2307,7 @@ func schema_pkg_apis_pipeline_v1_Sidecar(ref common.ReferenceCallback) common.Op }, "image": { SchemaProps: spec.SchemaProps{ - Description: "Image reference name. More info: https://kubernetes.io/docs/concepts/containers/images This field is optional to allow higher level config management to default or override container images in workload controllers like Deployments and StatefulSets.", + Description: "Image reference name. More info: https://kubernetes.io/docs/concepts/containers/images", Type: []string{"string"}, Format: "", }, @@ -2993,7 +2993,7 @@ func schema_pkg_apis_pipeline_v1_StepTemplate(ref common.ReferenceCallback) comm Properties: map[string]spec.Schema{ "image": { SchemaProps: spec.SchemaProps{ - Description: "Image reference name. More info: https://kubernetes.io/docs/concepts/containers/images This field is optional to allow higher level config management to default or override container images in workload controllers like Deployments and StatefulSets.", + Description: "Image reference name. More info: https://kubernetes.io/docs/concepts/containers/images", Type: []string{"string"}, Format: "", }, diff --git a/pkg/apis/pipeline/v1/swagger.json b/pkg/apis/pipeline/v1/swagger.json index 5a71f5f50c4..b842b3ee530 100644 --- a/pkg/apis/pipeline/v1/swagger.json +++ b/pkg/apis/pipeline/v1/swagger.json @@ -1202,7 +1202,7 @@ "x-kubernetes-list-type": "atomic" }, "image": { - "description": "Image reference name. More info: https://kubernetes.io/docs/concepts/containers/images This field is optional to allow higher level config management to default or override container images in workload controllers like Deployments and StatefulSets.", + "description": "Image reference name. More info: https://kubernetes.io/docs/concepts/containers/images", "type": "string" }, "imagePullPolicy": { @@ -1575,7 +1575,7 @@ "x-kubernetes-list-type": "atomic" }, "image": { - "description": "Image reference name. More info: https://kubernetes.io/docs/concepts/containers/images This field is optional to allow higher level config management to default or override container images in workload controllers like Deployments and StatefulSets.", + "description": "Image reference name. More info: https://kubernetes.io/docs/concepts/containers/images", "type": "string" }, "imagePullPolicy": { diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index 8a051618fe0..80f77685912 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -93,7 +93,7 @@ func (ts *TaskSpec) Validate(ctx context.Context) (errs *apis.FieldError) { } errs = errs.Also(validateSteps(ctx, mergedSteps).ViaField("steps")) - errs = errs.Also(validateSidecarNames(ts.Sidecars)) + errs = errs.Also(validateSidecars(ts.Sidecars).ViaField("sidecars")) errs = errs.Also(ValidateParameterTypes(ctx, ts.Params).ViaField("params")) errs = errs.Also(ValidateParameterVariables(ctx, ts.Steps, ts.Params)) errs = errs.Also(validateTaskContextVariables(ctx, ts.Steps)) @@ -153,18 +153,36 @@ func validateObjectParamsHaveProperties(ctx context.Context, params ParamSpecs) return errs } -func validateSidecarNames(sidecars []Sidecar) (errs *apis.FieldError) { +func validateSidecars(sidecars []Sidecar) (errs *apis.FieldError) { for _, sc := range sidecars { - if sc.Name == pipeline.ReservedResultsSidecarName { - errs = errs.Also(&apis.FieldError{ - Message: fmt.Sprintf("Invalid: cannot use reserved sidecar name %v ", sc.Name), - Paths: []string{"sidecars"}, - }) + errs = errs.Also(validateSidecarName(sc)) + + if sc.Image == "" { + errs = errs.Also(apis.ErrMissingField("image")) + } + + if sc.Script != "" { + if len(sc.Command) > 0 { + errs = errs.Also(&apis.FieldError{ + Message: "script cannot be used with command", + Paths: []string{"script"}, + }) + } } } return errs } +func validateSidecarName(sc Sidecar) (errs *apis.FieldError) { + if sc.Name == pipeline.ReservedResultsSidecarName { + errs = errs.Also(&apis.FieldError{ + Message: fmt.Sprintf("Invalid: cannot use reserved sidecar name %v ", sc.Name), + Paths: []string{"name"}, + }) + } + return errs +} + func validateResults(ctx context.Context, results []TaskResult) (errs *apis.FieldError) { for index, result := range results { errs = errs.Also(result.Validate(ctx).ViaIndex(index)) diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index b04d43f190e..2b51d346dfe 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1370,6 +1370,55 @@ func TestTaskSpecValidateError(t *testing.T) { } } +func TestTaskSpecValidateErrorSidecars(t *testing.T) { + tests := []struct { + name string + sidecars []v1.Sidecar + expectedError apis.FieldError + }{{ + name: "missing image", + sidecars: []v1.Sidecar{{ + Name: "tekton-invalid-side-car", + }}, + expectedError: apis.FieldError{ + Message: "missing field(s)", + Paths: []string{"sidecars.image"}, + }, + }, { + name: "invalid command with script", + sidecars: []v1.Sidecar{{ + Name: "tekton-invalid-side-car", + Image: "ubuntu", + Command: []string{"command foo"}, + Script: ` + #!/usr/bin/env bash + echo foo`, + }}, + expectedError: apis.FieldError{ + Message: "script cannot be used with command", + Paths: []string{"sidecars.script"}, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ts := &v1.TaskSpec{ + Steps: []v1.Step{{ + Name: "does-not-matter", + Image: "does-not-matter", + }}, + Sidecars: tt.sidecars, + } + err := ts.Validate(context.Background()) + if err == nil { + t.Fatalf("Expected an error, got nothing for %v", ts) + } + if d := cmp.Diff(tt.expectedError.Error(), err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" { + t.Errorf("TaskSpec.Validate() errors diff %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestTaskSpecValidateErrorSidecarName(t *testing.T) { tests := []struct { name string @@ -1383,7 +1432,7 @@ func TestTaskSpecValidateErrorSidecarName(t *testing.T) { }}, expectedError: apis.FieldError{ Message: fmt.Sprintf("Invalid: cannot use reserved sidecar name %v ", pipeline.ReservedResultsSidecarName), - Paths: []string{"sidecars"}, + Paths: []string{"sidecars.name"}, }, }} for _, tt := range tests { diff --git a/test/conversion_test.go b/test/conversion_test.go index 96896acd296..9977390de9e 100644 --- a/test/conversion_test.go +++ b/test/conversion_test.go @@ -115,7 +115,6 @@ spec: volumeMounts: - name: messages mountPath: /messages - script: echo test volumes: - name: messages emptyDir: {} @@ -189,7 +188,6 @@ spec: mountPath: /messages securityContext: runAsNonRoot: true - script: echo test volumes: - name: messages emptyDir: {}