From 69b9167dcbc8eea2ca5653fa42584539920a1fd4 Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Tue, 12 Feb 2019 15:16:19 -0800 Subject: [PATCH] Make apply conflict errors more readable --- .../apimachinery/pkg/api/errors/errors.go | 14 +-- .../apimachinery/pkg/apis/meta/v1/types.go | 3 + .../handlers/fieldmanager/fieldmanager.go | 3 +- .../fieldmanager/internal/conflict.go | 82 ++++++++++++++ .../fieldmanager/internal/conflict_test.go | 106 ++++++++++++++++++ 5 files changed, 194 insertions(+), 14 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/conflict.go create mode 100644 staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/conflict_test.go diff --git a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go index fa748a45061..afd97f7adcb 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/errors/errors.go @@ -27,7 +27,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" - "sigs.k8s.io/structured-merge-diff/merge" ) const ( @@ -186,16 +185,7 @@ func NewConflict(qualifiedResource schema.GroupResource, name string, err error) } // NewApplyConflict returns an error including details on the requests apply conflicts -func NewApplyConflict(conflicts merge.Conflicts) *StatusError { - causes := make([]metav1.StatusCause, 0, len(conflicts)) - for _, conflict := range conflicts { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseType("conflict"), - Message: conflict.Error(), - Field: conflict.Path.String(), - }) - } - +func NewApplyConflict(causes []metav1.StatusCause, message string) *StatusError { return &StatusError{ErrStatus: metav1.Status{ Status: metav1.StatusFailure, Code: http.StatusConflict, @@ -204,7 +194,7 @@ func NewApplyConflict(conflicts merge.Conflicts) *StatusError { // TODO: Get obj details here? Causes: causes, }, - Message: fmt.Sprintf("Apply failed with %d conflicts: %s", len(conflicts), conflicts.Error()), + Message: message, }} } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go index 0d82c2d5240..a13a6b71aa9 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go @@ -825,6 +825,9 @@ const ( // without the expected return type. The presence of this cause indicates the error may be // due to an intervening proxy or the server software malfunctioning. CauseTypeUnexpectedServerResponse CauseType = "UnexpectedServerResponse" + // FieldManagerConflict is used to report when another client claims to manage this field, + // It should only be returned for a request using server-side apply. + CauseTypeFieldManagerConflict CauseType = "FieldManagerConflict" ) // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go index b42c35dde29..a890e932500 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go @@ -20,7 +20,6 @@ import ( "fmt" "time" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -178,7 +177,7 @@ func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, force bool) ( newObjTyped, managed, err := f.updater.Apply(liveObjTyped, patchObjTyped, apiVersion, managed, manager, force) if err != nil { if conflicts, ok := err.(merge.Conflicts); ok { - return nil, errors.NewApplyConflict(conflicts) + return nil, internal.NewConflictError(conflicts) } return nil, err } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/conflict.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/conflict.go new file mode 100644 index 00000000000..282fe80fbbe --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/conflict.go @@ -0,0 +1,82 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package internal + +import ( + "encoding/json" + "fmt" + "sort" + "strings" + "time" + + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/structured-merge-diff/fieldpath" + "sigs.k8s.io/structured-merge-diff/merge" +) + +// NewConflictError returns an error including details on the requests apply conflicts +func NewConflictError(conflicts merge.Conflicts) *errors.StatusError { + causes := []metav1.StatusCause{} + for _, conflict := range conflicts { + causes = append(causes, metav1.StatusCause{ + Type: metav1.CauseTypeFieldManagerConflict, + Message: fmt.Sprintf("conflict with %v", printManager(conflict.Manager)), + Field: conflict.Path.String(), + }) + } + return errors.NewApplyConflict(causes, getConflictMessage(conflicts)) +} + +func getConflictMessage(conflicts merge.Conflicts) string { + if len(conflicts) == 1 { + return fmt.Sprintf("Apply failed with 1 conflict: conflict with %v: %v", printManager(conflicts[0].Manager), conflicts[0].Path) + } + + m := map[string][]fieldpath.Path{} + for _, conflict := range conflicts { + m[conflict.Manager] = append(m[conflict.Manager], conflict.Path) + } + + uniqueManagers := []string{} + for manager := range m { + uniqueManagers = append(uniqueManagers, manager) + } + + // Print conflicts by sorted managers. + sort.Strings(uniqueManagers) + + messages := []string{} + for _, manager := range uniqueManagers { + messages = append(messages, fmt.Sprintf("conflicts with %v:", printManager(manager))) + for _, path := range m[manager] { + messages = append(messages, fmt.Sprintf("- %v", path)) + } + } + return fmt.Sprintf("Apply failed with %d conflicts: %s", len(conflicts), strings.Join(messages, "\n")) +} + +func printManager(manager string) string { + encodedManager := &metav1.ManagedFieldsEntry{} + if err := json.Unmarshal([]byte(manager), encodedManager); err != nil { + return fmt.Sprintf("%q", manager) + } + if encodedManager.Operation == metav1.ManagedFieldsOperationUpdate { + return fmt.Sprintf("%q using %v at %v", encodedManager.Manager, encodedManager.APIVersion, encodedManager.Time.UTC().Format(time.RFC3339)) + } + return fmt.Sprintf("%q", encodedManager.Manager) +} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/conflict_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/conflict_test.go new file mode 100644 index 00000000000..f5c5dc6b566 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/conflict_test.go @@ -0,0 +1,106 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package internal_test + +import ( + "net/http" + "reflect" + "testing" + + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal" + "sigs.k8s.io/structured-merge-diff/fieldpath" + "sigs.k8s.io/structured-merge-diff/merge" +) + +// TestNewConflictError tests that NewConflictError creates the correct StatusError for a given smd Conflicts +func TestNewConflictError(t *testing.T) { + testCases := []struct { + conflict merge.Conflicts + expected *errors.StatusError + }{ + { + conflict: merge.Conflicts{ + merge.Conflict{ + Manager: `{"manager":"foo","operation":"Update","apiVersion":"v1","time":"2001-02-03T04:05:06Z"}`, + Path: fieldpath.MakePathOrDie("spec", "replicas"), + }, + }, + expected: &errors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusConflict, + Reason: metav1.StatusReasonConflict, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{ + { + Type: metav1.CauseTypeFieldManagerConflict, + Message: `conflict with "foo" using v1 at 2001-02-03T04:05:06Z`, + Field: ".spec.replicas", + }, + }, + }, + Message: `Apply failed with 1 conflict: conflict with "foo" using v1 at 2001-02-03T04:05:06Z: .spec.replicas`, + }, + }, + }, + { + conflict: merge.Conflicts{ + merge.Conflict{ + Manager: `{"manager":"foo","operation":"Update","apiVersion":"v1","time":"2001-02-03T04:05:06Z"}`, + Path: fieldpath.MakePathOrDie("spec", "replicas"), + }, + merge.Conflict{ + Manager: `{"manager":"bar","operation":"Apply"}`, + Path: fieldpath.MakePathOrDie("metadata", "labels", "app"), + }, + }, + expected: &errors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusConflict, + Reason: metav1.StatusReasonConflict, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{ + { + Type: metav1.CauseTypeFieldManagerConflict, + Message: `conflict with "foo" using v1 at 2001-02-03T04:05:06Z`, + Field: ".spec.replicas", + }, + { + Type: metav1.CauseTypeFieldManagerConflict, + Message: `conflict with "bar"`, + Field: ".metadata.labels.app", + }, + }, + }, + Message: `Apply failed with 2 conflicts: conflicts with "bar": +- .metadata.labels.app +conflicts with "foo" using v1 at 2001-02-03T04:05:06Z: +- .spec.replicas`, + }, + }, + }, + } + for _, tc := range testCases { + actual := internal.NewConflictError(tc.conflict) + if !reflect.DeepEqual(tc.expected, actual) { + t.Errorf("Expected to get\n%+v\nbut got\n%+v", tc.expected.ErrStatus, actual.ErrStatus) + } + } +}