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 `stepTemplate.image` and
`sidecar.image` are optional. BUT the labels of these two fields are
kept for backwards compatibility purposes.

/kind bug
fixes: tektoncd#7437

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.

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))

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 {