From b97fd65f6344b6bfd2df78eddba369cb19ad2b5f 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 1aa5ffaaefe..2acc4130672 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -3572,9 +3572,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

@@ -4665,9 +4663,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 a15655521f7..b7da45e0b57 100644 --- a/pkg/apis/pipeline/v1/container_types.go +++ b/pkg/apis/pipeline/v1/container_types.go @@ -249,8 +249,6 @@ func (s *Step) GetVarSubstitutionExpressions() []string { 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. @@ -369,8 +367,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 0fb74d29b15..fe5d925166f 100644 --- a/pkg/apis/pipeline/v1/openapi_generated.go +++ b/pkg/apis/pipeline/v1/openapi_generated.go @@ -2364,7 +2364,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: "", }, @@ -3160,7 +3160,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 d99469bffd7..8093c7e8389 100644 --- a/pkg/apis/pipeline/v1/swagger.json +++ b/pkg/apis/pipeline/v1/swagger.json @@ -1232,7 +1232,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": { @@ -1664,7 +1664,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 d9686b67351..86c277ca968 100644 --- a/pkg/apis/pipeline/v1/task_validation.go +++ b/pkg/apis/pipeline/v1/task_validation.go @@ -89,7 +89,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)) @@ -149,18 +149,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 08e7657873a..6f38a2d38b0 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1617,6 +1617,55 @@ func TestTaskSpecValidateErrorWithStepActionRef(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 @@ -1630,7 +1679,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: {}