From c678b44c5b10cb895054a9829e5bd2cbb422e824 Mon Sep 17 00:00:00 2001 From: Dan Lorenc Date: Wed, 15 Jul 2020 19:29:35 -0500 Subject: [PATCH] Use patch instead of update to mutate annotations. This helps prevent some race conditions, where we try to update an object when simultaneous updates are happening. They eventually work themselves out because we continue to retry, but this helps speed things along. --- pkg/patch/patch.go | 21 ++++++++++++ pkg/patch/patch_test.go | 49 ++++++++++++++++++++++++++++ pkg/signing/signing.go | 15 ++++++--- pkg/signing/signing_test.go | 7 +++- pkg/signing/storage/tekton/tekton.go | 15 ++++++--- 5 files changed, 96 insertions(+), 11 deletions(-) create mode 100644 pkg/patch/patch.go create mode 100644 pkg/patch/patch_test.go diff --git a/pkg/patch/patch.go b/pkg/patch/patch.go new file mode 100644 index 0000000000..cf99ee4957 --- /dev/null +++ b/pkg/patch/patch.go @@ -0,0 +1,21 @@ +package patch + +import "encoding/json" + +// GetAnnotationsPatch returns patch bytes that can be used with kubectl patch +func GetAnnotationsPatch(newAnnotations map[string]string) ([]byte, error) { + p := patch{ + Metadata: metadata{ + Annotations: newAnnotations, + }, + } + return json.Marshal(p) +} + +// These are used to get proper json formatting +type patch struct { + Metadata metadata `json:"metadata,omitempty"` +} +type metadata struct { + Annotations map[string]string `json:"annotations,omitempty"` +} diff --git a/pkg/patch/patch_test.go b/pkg/patch/patch_test.go new file mode 100644 index 0000000000..42ab2a3a75 --- /dev/null +++ b/pkg/patch/patch_test.go @@ -0,0 +1,49 @@ +package patch + +import ( + "reflect" + "testing" +) + +func TestGetAnnotationsPatch(t *testing.T) { + tests := []struct { + name string + newAnnotations map[string]string + want string + wantErr bool + }{ + { + name: "empty", + newAnnotations: map[string]string{}, + want: `{"metadata":{}}`, + }, + { + name: "one", + newAnnotations: map[string]string{ + "foo": "bar", + }, + want: `{"metadata":{"annotations":{"foo":"bar"}}}`, + }, + { + name: "many", + newAnnotations: map[string]string{ + "foo": "bar", + "baz": "bat", + }, + want: `{"metadata":{"annotations":{"baz":"bat","foo":"bar"}}}`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := GetAnnotationsPatch(tt.newAnnotations) + if (err != nil) != tt.wantErr { + t.Errorf("GetAnnotationsPatch() error = %v, wantErr %v", err, tt.wantErr) + return + } + gotStr := string(got) + if !reflect.DeepEqual(gotStr, tt.want) { + t.Errorf("GetAnnotationsPatch() = %v, want %v", gotStr, tt.want) + } + }) + } +} diff --git a/pkg/signing/signing.go b/pkg/signing/signing.go index eb284e9345..676c1c5f29 100644 --- a/pkg/signing/signing.go +++ b/pkg/signing/signing.go @@ -14,11 +14,13 @@ limitations under the License. package signing import ( + "github.com/tektoncd/chains/pkg/patch" "github.com/tektoncd/chains/pkg/signing/formats" "github.com/tektoncd/chains/pkg/signing/storage" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" versioned "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" "go.uber.org/zap" + "k8s.io/apimachinery/pkg/types" ) const ( @@ -46,11 +48,14 @@ func IsSigned(tr *v1beta1.TaskRun) bool { // MarkSigned marks a TaskRun as signed. func MarkSigned(tr *v1beta1.TaskRun, ps versioned.Interface) error { - if tr.ObjectMeta.Annotations == nil { - tr.ObjectMeta.Annotations = map[string]string{} + // Use patch instead of update to help prevent race conditions. + patchBytes, err := patch.GetAnnotationsPatch(map[string]string{ + ChainsAnnotation: "true", + }) + if err != nil { + return err } - tr.ObjectMeta.Annotations[ChainsAnnotation] = "true" - if _, err := ps.TektonV1beta1().TaskRuns(tr.Namespace).Update(tr); err != nil { + if _, err := ps.TektonV1beta1().TaskRuns(tr.Namespace).Patch(tr.Name, types.MergePatchType, patchBytes); err != nil { return err } return nil @@ -69,7 +74,7 @@ func (ts *TaskRunSigner) SignTaskRun(tr *v1beta1.TaskRun) error { for _, b := range backends { for payloadType, payload := range payloads { if err := b.StorePayload(payload, payloadType, tr); err != nil { - ts.Logger.Errorf("error storing payloadType %s on storageBackend %s for taskRun %s/%s", payloadType, b.Type(), tr.Namespace, tr.Name) + ts.Logger.Errorf("error storing payloadType %s on storageBackend %s for taskRun %s/%s: %v", payloadType, b.Type(), tr.Namespace, tr.Name, err) // continue and store others } } diff --git a/pkg/signing/signing_test.go b/pkg/signing/signing_test.go index 012538565e..29cad53630 100644 --- a/pkg/signing/signing_test.go +++ b/pkg/signing/signing_test.go @@ -157,6 +157,11 @@ func TestTaskRunSigner_SignTaskRun(t *testing.T) { t.Errorf("TaskRunSigner.SignTaskRun() error = %v", err) } + // Fetch a new TR! + tr, err := ps.TektonV1beta1().TaskRuns(tr.Namespace).Get(tr.Name, metav1.GetOptions{}) + if err != nil { + t.Errorf("error fetching fake taskrun: %v", err) + } // Check it is marked as signed if !IsSigned(tr) { t.Errorf("TaskRun %s/%s should be marked as signed, was not", tr.Namespace, tr.Name) @@ -198,7 +203,7 @@ type mockBackend struct { // StorePayload implements the Payloader interface. func (b *mockBackend) StorePayload(payload interface{}, payloadType string, tr *v1beta1.TaskRun) error { if b.shouldErr { - return errors.New("error storing") + return errors.New("mock error storing") } if b.storedPayloads == nil { b.storedPayloads = map[string]interface{}{} diff --git a/pkg/signing/storage/tekton/tekton.go b/pkg/signing/storage/tekton/tekton.go index 61210c8b03..5a99ee6291 100644 --- a/pkg/signing/storage/tekton/tekton.go +++ b/pkg/signing/storage/tekton/tekton.go @@ -17,9 +17,11 @@ import ( "encoding/base64" "encoding/json" + "github.com/tektoncd/chains/pkg/patch" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" versioned "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" "go.uber.org/zap" + "k8s.io/apimachinery/pkg/types" ) const ( @@ -45,9 +47,6 @@ func NewStorageBackend(ps versioned.Interface, logger *zap.SugaredLogger) *Backe // StorePayload implements the Payloader interface. func (b *Backend) StorePayload(payload interface{}, payloadType string, tr *v1beta1.TaskRun) error { b.logger.Infof("Storing payload type %s on TaskRun %s/%s", payloadType, tr.Namespace, tr.Name) - if tr.ObjectMeta.Annotations == nil { - tr.ObjectMeta.Annotations = map[string]string{} - } jsonPayload, err := json.Marshal(payload) if err != nil { @@ -56,8 +55,14 @@ func (b *Backend) StorePayload(payload interface{}, payloadType string, tr *v1be textPayload := base64.StdEncoding.EncodeToString(jsonPayload) - tr.ObjectMeta.Annotations[PayloadAnnotation] = textPayload - if _, err := b.pipelienclientset.TektonV1beta1().TaskRuns(tr.Namespace).Update(tr); err != nil { + // Use patch instead of update to prevent race conditions. + patchBytes, err := patch.GetAnnotationsPatch(map[string]string{ + PayloadAnnotation: textPayload, + }) + if err != nil { + return err + } + if _, err := b.pipelienclientset.TektonV1beta1().TaskRuns(tr.Namespace).Patch(tr.Name, types.MergePatchType, patchBytes); err != nil { return err } return nil