Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix validations for Sidecars to be consistent #7443

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Fix validations for Sidecars to be consistent #7443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions docs/pipeline-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
4 changes: 0 additions & 4 deletions pkg/apis/pipeline/v1/container_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down Expand Up @@ -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": {
Expand Down
32 changes: 25 additions & 7 deletions pkg/apis/pipeline/v1/task_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down
51 changes: 50 additions & 1 deletion pkg/apis/pipeline/v1/task_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
2 changes: 0 additions & 2 deletions test/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ spec:
volumeMounts:
- name: messages
mountPath: /messages
script: echo test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously invalid syntax which aimed to test as many fields as possible in Task but not tested for sidecar.

volumes:
- name: messages
emptyDir: {}
Expand Down Expand Up @@ -189,7 +188,6 @@ spec:
mountPath: /messages
securityContext:
runAsNonRoot: true
script: echo test
volumes:
- name: messages
emptyDir: {}
Expand Down
Loading