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: tektoncd#7437

docs/pipeline-api.md

@@ -3572,9 +3572,7 @@ string
 <td>
 <em>(Optional)</em>
 <p>Image reference name.
-More info: <a href="https://kubernetes.io/docs/concepts/containers/images">https://kubernetes.io/docs/concepts/containers/images</a>
-This field is optional to allow higher level config management to default or override
-container images in workload controllers like Deployments and StatefulSets.</p>
+More info: <a href="https://kubernetes.io/docs/concepts/containers/images">https://kubernetes.io/docs/concepts/containers/images</a></p>
 </td>
 </tr>
 <tr>
@@ -4665,9 +4663,7 @@ string
 <td>
 <em>(Optional)</em>
 <p>Image reference name.
-More info: <a href="https://kubernetes.io/docs/concepts/containers/images">https://kubernetes.io/docs/concepts/containers/images</a>
-This field is optional to allow higher level config management to default or override
-container images in workload controllers like Deployments and StatefulSets.</p>
+More info: <a href="https://kubernetes.io/docs/concepts/containers/images">https://kubernetes.io/docs/concepts/containers/images</a></p>
 </td>
 </tr>
 <tr>

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/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": {

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 {

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: {}