From d21e0871e48027d96dc2c81eea992193bb50c7fa Mon Sep 17 00:00:00 2001 From: Shivam Mukhade Date: Sun, 29 Aug 2021 21:44:34 +0530 Subject: [PATCH] Converts ResultType in PipelineResourceResult string to int This converts the ResultType in PipelineResourceResult from string to int enum to curtail the size of json object emittted by steps. ResultTypes were removed because they made JSON messages bigger, which in turn limited the amount of space in termination messages for task results. String support is maintained for backwards compatibility. Closes #4150 Signed-off-by: Shivam Mukhade --- .../pipeline/v1beta1/openapi_generated.go | 4 +- pkg/apis/pipeline/v1beta1/resource_types.go | 43 +++++++++++++- .../pipeline/v1beta1/resource_types_test.go | 35 ++++++++++++ pkg/apis/pipeline/v1beta1/swagger.json | 3 +- pkg/apis/pipeline/v1beta1/task_types.go | 8 +-- pkg/pod/status_test.go | 57 +++++++++++++++---- 6 files changed, 132 insertions(+), 18 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index ee46447442c..51c56971372 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -1211,8 +1211,8 @@ func schema_pkg_apis_pipeline_v1beta1_PipelineResourceResult(ref common.Referenc }, "type": { SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", + Type: []string{"integer"}, + Format: "int32", }, }, }, diff --git a/pkg/apis/pipeline/v1beta1/resource_types.go b/pkg/apis/pipeline/v1beta1/resource_types.go index 91192d3b4f0..9b9f298e793 100644 --- a/pkg/apis/pipeline/v1beta1/resource_types.go +++ b/pkg/apis/pipeline/v1beta1/resource_types.go @@ -17,9 +17,11 @@ limitations under the License. package v1beta1 import ( + "encoding/json" "fmt" "github.com/google/go-cmp/cmp" + "github.com/hashicorp/go-multierror" resource "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1" v1 "k8s.io/api/core/v1" ) @@ -130,7 +132,46 @@ type PipelineResourceResult struct { } // ResultType used to find out whether a PipelineResourceResult is from a task result or not -type ResultType string +type ResultType int + +// UnmarshalJSON unmarshals either an int or a string into a ResultType. String +// ResultTypes were removed because they made JSON messages bigger, which in +// turn limited the amount of space in termination messages for task results. String +// support is maintained for backwards compatibility - the Pipelines controller could +// be stopped midway through TaskRun execution, updated with support for int in place +// of string, and then fail the running TaskRun because it doesn't know how to interpret +// the string value that the TaskRun's entrypoint will emit when it completes. +func (r *ResultType) UnmarshalJSON(data []byte) error { + + var asInt int + var intErr error + + if err := json.Unmarshal(data, &asInt); err != nil { + intErr = err + } else { + *r = ResultType(asInt) + return nil + } + + var asString string + + if err := json.Unmarshal(data, &asString); err != nil { + return fmt.Errorf("unsupported value type, neither int nor string: %v", multierror.Append(intErr, err).ErrorOrNil()) + } + + switch asString { + case "TaskRunResult": + *r = TaskRunResultType + case "PipelineResourceResult": + *r = PipelineResourceResultType + case "InternalTektonResult": + *r = InternalTektonResultType + default: + *r = UnknownResultType + } + + return nil +} // PipelineResourceRef can be used to refer to a specific instance of a Resource type PipelineResourceRef struct { diff --git a/pkg/apis/pipeline/v1beta1/resource_types_test.go b/pkg/apis/pipeline/v1beta1/resource_types_test.go index ac232d7f5f2..ef481daa87d 100644 --- a/pkg/apis/pipeline/v1beta1/resource_types_test.go +++ b/pkg/apis/pipeline/v1beta1/resource_types_test.go @@ -14,6 +14,7 @@ limitations under the License. package v1beta1_test import ( + "encoding/json" "testing" "github.com/google/go-cmp/cmp" @@ -143,3 +144,37 @@ func TestApplyTaskModifier_AlreadyAdded(t *testing.T) { }) } } + +func TestPipelineResourceResult_UnmarshalJSON(t *testing.T) { + + testcases := []struct { + name string + data string + pr v1beta1.PipelineResourceResult + }{{ + name: "type defined as string - TaskRunResult", + data: "{\"key\":\"resultName\",\"value\":\"resultValue\", \"type\": \"TaskRunResult\"}", + pr: v1beta1.PipelineResourceResult{Key: "resultName", Value: "resultValue", ResultType: v1beta1.TaskRunResultType}, + }, + { + name: "type defined as string - InternalTektonResult", + data: "{\"key\":\"resultName\",\"value\":\"\", \"type\": \"InternalTektonResult\"}", + pr: v1beta1.PipelineResourceResult{Key: "resultName", Value: "", ResultType: v1beta1.InternalTektonResultType}, + }, { + name: "type defined as int", + data: "{\"key\":\"resultName\",\"value\":\"\", \"type\": 1}", + pr: v1beta1.PipelineResourceResult{Key: "resultName", Value: "", ResultType: v1beta1.TaskRunResultType}, + }} + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + pipRes := v1beta1.PipelineResourceResult{} + if err := json.Unmarshal([]byte(tc.data), &pipRes); err != nil { + t.Errorf("Unexpected error when unmarshalling the json into PipelineResourceResult") + } + if d := cmp.Diff(tc.pr, pipRes); d != "" { + t.Errorf(diff.PrintWantGot(d)) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index c6a1e1ca8f8..90ae925e861 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -803,7 +803,8 @@ "$ref": "#/definitions/v1beta1.PipelineResourceRef" }, "type": { - "type": "string" + "type": "integer", + "format": "int32" }, "value": { "type": "string", diff --git a/pkg/apis/pipeline/v1beta1/task_types.go b/pkg/apis/pipeline/v1beta1/task_types.go index b41a8edcb7e..89d2224d957 100644 --- a/pkg/apis/pipeline/v1beta1/task_types.go +++ b/pkg/apis/pipeline/v1beta1/task_types.go @@ -23,13 +23,13 @@ import ( const ( // TaskRunResultType default task run result value - TaskRunResultType ResultType = "TaskRunResult" + TaskRunResultType ResultType = 1 // PipelineResourceResultType default pipeline result value - PipelineResourceResultType ResultType = "PipelineResourceResult" + PipelineResourceResultType = 2 // InternalTektonResultType default internal tekton result value - InternalTektonResultType ResultType = "InternalTektonResult" + InternalTektonResultType = 3 // UnknownResultType default unknown result type value - UnknownResultType ResultType = "" + UnknownResultType = 10 ) // +genclient diff --git a/pkg/pod/status_test.go b/pkg/pod/status_test.go index 746adff3618..b7075199622 100644 --- a/pkg/pod/status_test.go +++ b/pkg/pod/status_test.go @@ -549,7 +549,7 @@ func TestMakeTaskRunStatus(t *testing.T) { Name: "step-bar", State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + Message: `[{"key":"resultName","value":"resultValue", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, }, }, }}, @@ -560,7 +560,7 @@ func TestMakeTaskRunStatus(t *testing.T) { Steps: []v1beta1.StepState{{ ContainerState: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","type":"TaskRunResult"}]`, + Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","type":1}]`, }}, Name: "bar", ContainerName: "step-bar", @@ -587,7 +587,7 @@ func TestMakeTaskRunStatus(t *testing.T) { Name: "step-banana", State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + Message: `[{"key":"resultName","value":"resultValue", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, }, }, }}, @@ -598,7 +598,7 @@ func TestMakeTaskRunStatus(t *testing.T) { Steps: []v1beta1.StepState{{ ContainerState: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","type":"TaskRunResult"}]`, + Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","type":1}]`, }}, Name: "banana", ContainerName: "step-banana", @@ -625,14 +625,14 @@ func TestMakeTaskRunStatus(t *testing.T) { Name: "step-one", State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"resultValueOne", "type": "TaskRunResult"}, {"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, + Message: `[{"key":"resultNameOne","value":"resultValueOne", "type":1}, {"key":"resultNameTwo","value":"resultValueTwo", "type":1}]`, }, }, }, { Name: "step-two", State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"resultValueThree","type":"TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo","type":"TaskRunResult"}]`, + Message: `[{"key":"resultNameOne","value":"resultValueThree","type":1},{"key":"resultNameTwo","value":"resultValueTwo","type":1}]`, }, }, }}, @@ -643,14 +643,14 @@ func TestMakeTaskRunStatus(t *testing.T) { Steps: []v1beta1.StepState{{ ContainerState: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"resultValueOne","type":"TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo","type":"TaskRunResult"}]`, + Message: `[{"key":"resultNameOne","value":"resultValueOne","type":1},{"key":"resultNameTwo","value":"resultValueTwo","type":1}]`, }}, Name: "one", ContainerName: "step-one", }, { ContainerState: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"resultValueThree","type":"TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo","type":"TaskRunResult"}]`, + Message: `[{"key":"resultNameOne","value":"resultValueThree","type":1},{"key":"resultNameTwo","value":"resultValueTwo","type":1}]`, }}, Name: "two", ContainerName: "step-two", @@ -721,6 +721,43 @@ func TestMakeTaskRunStatus(t *testing.T) { }, }, { desc: "filter internaltektonresult", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-pear", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"","type":2}, {"key":"resultNameTwo","value":"","type":3}, {"key":"resultNameThree","value":"","type":1}]`}, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: statusSuccess(), + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"","type":2},{"key":"resultNameThree","value":"","type":1}]`, + }}, + Name: "pear", + ContainerName: "step-pear", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "resultNameOne", + Value: "", + ResultType: v1beta1.PipelineResourceResultType, + }}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultNameThree", + Value: "", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "filter internaltektonresult with `type` as string", podStatus: corev1.PodStatus{ Phase: corev1.PodSucceeded, ContainerStatuses: []corev1.ContainerStatus{{ @@ -738,7 +775,7 @@ func TestMakeTaskRunStatus(t *testing.T) { Steps: []v1beta1.StepState{{ ContainerState: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"","type":"PipelineResourceResult"},{"key":"resultNameThree","value":"","type":"TaskRunResult"}]`, + Message: `[{"key":"resultNameOne","value":"","type":2},{"key":"resultNameThree","value":"","type":1}]`, }}, Name: "pear", ContainerName: "step-pear", @@ -747,7 +784,7 @@ func TestMakeTaskRunStatus(t *testing.T) { ResourcesResult: []v1beta1.PipelineResourceResult{{ Key: "resultNameOne", Value: "", - ResultType: "PipelineResourceResult", + ResultType: v1beta1.PipelineResourceResultType, }}, TaskRunResults: []v1beta1.TaskRunResult{{ Name: "resultNameThree",