From 67f54766396cbb860e46781cfe73cc47bc199ffc Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Thu, 18 Jun 2020 12:53:41 -0400 Subject: [PATCH 1/3] Don't require pointer to binding Seems like bumping the golangci version (https://github.com/tektoncd/plumbing/pull/430) revealed some new linting issues. Pointers to items being ranged over are reused, so if this pointer is stored anywhere and used later, it can lead to bugs. I didn't see any reason why this needed to be a pointer so passing around the value instead. --- .../pipelinerun/resources/pipelinerunresolution.go | 2 +- pkg/reconciler/taskrun/resources/image_exporter.go | 2 +- .../taskrun/resources/taskresourceresolution.go | 6 +++--- .../taskrun/resources/taskresourceresolution_test.go | 10 +++++----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 4a2b143f50b..14a0dad982f 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -234,7 +234,7 @@ type GetTaskRun func(name string) (*v1beta1.TaskRun, error) func GetResourcesFromBindings(pr *v1beta1.PipelineRun, getResource resources.GetResource) (map[string]*resourcev1alpha1.PipelineResource, error) { rs := map[string]*resourcev1alpha1.PipelineResource{} for _, resource := range pr.Spec.Resources { - r, err := resources.GetResourceFromBinding(&resource, getResource) + r, err := resources.GetResourceFromBinding(resource, getResource) if err != nil { return rs, fmt.Errorf("error following resource reference for %s: %w", resource.Name, err) } diff --git a/pkg/reconciler/taskrun/resources/image_exporter.go b/pkg/reconciler/taskrun/resources/image_exporter.go index f9b19a9568f..8634378cb36 100644 --- a/pkg/reconciler/taskrun/resources/image_exporter.go +++ b/pkg/reconciler/taskrun/resources/image_exporter.go @@ -45,7 +45,7 @@ func AddOutputImageDigestExporter( return fmt.Errorf("failed to get bound resource: %w while adding output image digest exporter", err) } - resource, err := GetResourceFromBinding(&boundResource.PipelineResourceBinding, gr) + resource, err := GetResourceFromBinding(boundResource.PipelineResourceBinding, gr) if err != nil { return fmt.Errorf("failed to get output pipeline Resource for taskRun %q resource %v; error: %w while adding output image digest exporter", tr.Name, boundResource, err) } diff --git a/pkg/reconciler/taskrun/resources/taskresourceresolution.go b/pkg/reconciler/taskrun/resources/taskresourceresolution.go index e01efe83cb5..83d2a0646a9 100644 --- a/pkg/reconciler/taskrun/resources/taskresourceresolution.go +++ b/pkg/reconciler/taskrun/resources/taskresourceresolution.go @@ -55,7 +55,7 @@ func ResolveTaskResources(ts *v1beta1.TaskSpec, taskName string, kind v1beta1.Ta } for _, r := range inputs { - rr, err := GetResourceFromBinding(&r.PipelineResourceBinding, gr) + rr, err := GetResourceFromBinding(r.PipelineResourceBinding, gr) if err != nil { return nil, fmt.Errorf("couldn't retrieve referenced input PipelineResource: %w", err) } @@ -64,7 +64,7 @@ func ResolveTaskResources(ts *v1beta1.TaskSpec, taskName string, kind v1beta1.Ta } for _, r := range outputs { - rr, err := GetResourceFromBinding(&r.PipelineResourceBinding, gr) + rr, err := GetResourceFromBinding(r.PipelineResourceBinding, gr) if err != nil { return nil, fmt.Errorf("couldn't retrieve referenced output PipelineResource: %w", err) @@ -77,7 +77,7 @@ func ResolveTaskResources(ts *v1beta1.TaskSpec, taskName string, kind v1beta1.Ta // GetResourceFromBinding will return an instance of a PipelineResource to use for r, either by getting it with getter or by // instantiating it from the embedded spec. -func GetResourceFromBinding(r *v1beta1.PipelineResourceBinding, getter GetResource) (*resourcev1alpha1.PipelineResource, error) { +func GetResourceFromBinding(r v1beta1.PipelineResourceBinding, getter GetResource) (*resourcev1alpha1.PipelineResource, error) { if (r.ResourceRef != nil && r.ResourceRef.Name != "") && r.ResourceSpec != nil { return nil, errors.New("Both ResourseRef and ResourceSpec are defined. Expected only one") } diff --git a/pkg/reconciler/taskrun/resources/taskresourceresolution_test.go b/pkg/reconciler/taskrun/resources/taskresourceresolution_test.go index da0974088a5..cc72c424b96 100644 --- a/pkg/reconciler/taskrun/resources/taskresourceresolution_test.go +++ b/pkg/reconciler/taskrun/resources/taskresourceresolution_test.go @@ -273,7 +273,7 @@ func TestGetResourceFromBinding_Ref(t *testing.T) { Name: "git-repo", }, } - binding := &v1beta1.PipelineResourceBinding{ + binding := v1beta1.PipelineResourceBinding{ ResourceRef: &v1beta1.PipelineResourceRef{ Name: "foo-resource", }, @@ -292,7 +292,7 @@ func TestGetResourceFromBinding_Ref(t *testing.T) { } func TestGetResourceFromBinding_Spec(t *testing.T) { - binding := &v1beta1.PipelineResourceBinding{ + binding := v1beta1.PipelineResourceBinding{ ResourceSpec: &resourcev1alpha1.PipelineResourceSpec{ Type: resourcev1alpha1.PipelineResourceTypeGit, Params: []resourcev1alpha1.ResourceParam{{ @@ -318,7 +318,7 @@ func TestGetResourceFromBinding_Spec(t *testing.T) { } func TestGetResourceFromBinding_NoNameOrSpec(t *testing.T) { - binding := &v1beta1.PipelineResourceBinding{} + binding := v1beta1.PipelineResourceBinding{} gr := func(n string) (*resourcev1alpha1.PipelineResource, error) { return nil, nil } @@ -330,7 +330,7 @@ func TestGetResourceFromBinding_NoNameOrSpec(t *testing.T) { } func TestGetResourceFromBinding_NameAndSpec(t *testing.T) { - binding := &v1beta1.PipelineResourceBinding{ + binding := v1beta1.PipelineResourceBinding{ ResourceSpec: &resourcev1alpha1.PipelineResourceSpec{ Type: resourcev1alpha1.PipelineResourceTypeGit, Params: []resourcev1alpha1.ResourceParam{{ @@ -353,7 +353,7 @@ func TestGetResourceFromBinding_NameAndSpec(t *testing.T) { } func TestGetResourceFromBinding_ErrorGettingResource(t *testing.T) { - binding := &v1beta1.PipelineResourceBinding{ + binding := v1beta1.PipelineResourceBinding{ ResourceRef: &v1beta1.PipelineResourceRef{ Name: "foo-resource", }, From ba1aaa678a4cbc59235ad3891e50946e50a0b8ff Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Thu, 18 Jun 2020 15:08:49 -0400 Subject: [PATCH 2/3] Don't lint the generated deepcopy file I found it a bit hard to understand how to configure this but looking at the example config (https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml) this seems right. Since we don't control these files, it makes sense to skip linting them. Recently we bumped the linter, which pulled in a new version of gosec, which started flagging "Implicit memory aliasing in for loop" in the generated files. This is a bit weird but at least we know that the values are being used immediately and not stored, so it seems (famous last words) unlikely to hit the bug these are trying to catch (this issue has an example of the kind of bug this catches: https://github.com/trailofbits/gosec/issues/1) e.g. the code getting flagged: ``` for key, val := range *in { var outVal *PipelineRunTaskRunStatus if val == nil { (*out)[key] = nil } else { in, out := &val, &outVal *out = new(PipelineRunTaskRunStatus) (*in).DeepCopyInto(*out) } (*out)[key] = outVal } ``` --- .golangci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.golangci.yml b/.golangci.yml index 2269e0d2eb6..963ff69ae5a 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -24,6 +24,8 @@ run: issues-exit-code: 1 build-tags: - e2e + skip-files: + - .*/zz_generated.deepcopy.go skip-dirs: - vendor - pkg/client From 4abc11b61868a3ec9ae564a9b3724c338a16459a Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Thu, 18 Jun 2020 15:29:44 -0400 Subject: [PATCH 3/3] Don't use pointer when reading termination message Using a pointer here caused us to get the warning "Implicit memory aliasing in for loop" from gosec. Not to mention that functions with side effects are hard to maintain and reason about over time, so instead, we'll now have one function that gets the values to use and we'll actually do the mutation outside the function, so no reason to pass in a pointer. --- pkg/pod/status.go | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/pkg/pod/status.go b/pkg/pod/status.go index 55046be8bc5..fb35ee310d7 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -116,9 +116,14 @@ func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev for _, s := range pod.Status.ContainerStatuses { if IsContainerStep(s.Name) { if s.State.Terminated != nil && len(s.State.Terminated.Message) != 0 { - if err := updateStatusStartTime(&s); err != nil { + message, time, err := removeStartInfoFromTerminationMessage(s) + if err != nil { logger.Errorf("error setting the start time of step %q in taskrun %q: %w", s.Name, tr.Name, err) } + if time != nil { + s.State.Terminated.StartedAt = *time + s.State.Terminated.Message = message + } } trs.Steps = append(trs.Steps, v1beta1.StepState{ ContainerState: *s.State.DeepCopy(), @@ -151,34 +156,35 @@ func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev return *trs } -// updateStatusStartTime searches for a result called "StartedAt" in the JSON-formatted termination message -// of a step and sets the State.Terminated.StartedAt field to this time if it's found. The "StartedAt" result -// is also removed from the list of results in the container status. -func updateStatusStartTime(s *corev1.ContainerStatus) error { +// removeStartInfoFromTerminationMessage searches for a result called "StartedAt" in the JSON-formatted +// termination message of a step and returns the values to use for sets State.Terminated if it's +// found. The "StartedAt" result is also removed from the list of results in the container status. +func removeStartInfoFromTerminationMessage(s corev1.ContainerStatus) (string, *metav1.Time, error) { r, err := termination.ParseMessage(s.State.Terminated.Message) if err != nil { - return fmt.Errorf("termination message could not be parsed as JSON: %w", err) + return "", nil, fmt.Errorf("termination message could not be parsed as JSON: %w", err) } for index, result := range r { if result.Key == "StartedAt" { t, err := time.Parse(timeFormat, result.Value) if err != nil { - return fmt.Errorf("could not parse time value %q in StartedAt field: %w", result.Value, err) + return "", nil, fmt.Errorf("could not parse time value %q in StartedAt field: %w", result.Value, err) } - s.State.Terminated.StartedAt = metav1.NewTime(t) + message := "" + startedAt := metav1.NewTime(t) // remove the entry for the starting time r = append(r[:index], r[index+1:]...) if len(r) == 0 { - s.State.Terminated.Message = "" + message = "" } else if bytes, err := json.Marshal(r); err != nil { - return fmt.Errorf("error marshalling remaining results back into termination message: %w", err) + return "", nil, fmt.Errorf("error marshalling remaining results back into termination message: %w", err) } else { - s.State.Terminated.Message = string(bytes) + message = string(bytes) } - break + return message, &startedAt, nil } } - return nil + return "", nil, nil } func updateCompletedTaskRun(trs *v1beta1.TaskRunStatus, pod *corev1.Pod) {