Skip to content

Commit 4a55a55

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#86560 from kwiesmueller/change-apply-signature
change apply signature and move it's decoding into handler
2 parents c0e245f + a252628 commit 4a55a55

File tree

12 files changed

+146
-50
lines changed

12 files changed

+146
-50
lines changed

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ go_library(
7070
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion:go_default_library",
7171
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/internalversion/scheme:go_default_library",
7272
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
73+
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
7374
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation:go_default_library",
7475
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1:go_default_library",
7576
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1beta1/validation:go_default_library",
@@ -104,6 +105,7 @@ go_library(
104105
"//vendor/golang.org/x/net/websocket:go_default_library",
105106
"//vendor/k8s.io/klog:go_default_library",
106107
"//vendor/k8s.io/utils/trace:go_default_library",
108+
"//vendor/sigs.k8s.io/yaml:go_default_library",
107109
],
108110
)
109111

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/BUILD‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,13 @@ go_library(
1717
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
1818
"//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library",
1919
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
20-
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
2120
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
2221
"//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
2322
"//staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal:go_default_library",
2423
"//vendor/k8s.io/klog:go_default_library",
2524
"//vendor/k8s.io/kube-openapi/pkg/util/proto:go_default_library",
2625
"//vendor/sigs.k8s.io/structured-merge-diff/fieldpath:go_default_library",
2726
"//vendor/sigs.k8s.io/structured-merge-diff/merge:go_default_library",
28-
"//vendor/sigs.k8s.io/yaml:go_default_library",
2927
],
3028
)
3129

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/buildmanagerinfo.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,12 @@ func (f *buildManagerInfoManager) Update(liveObj, newObj runtime.Object, managed
5151
}
5252

5353
// Apply implements Manager.
54-
func (f *buildManagerInfoManager) Apply(liveObj runtime.Object, patch []byte, managed Managed, manager string, force bool) (runtime.Object, Managed, error) {
54+
func (f *buildManagerInfoManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, manager string, force bool) (runtime.Object, Managed, error) {
5555
manager, err := f.buildManagerInfo(manager, metav1.ManagedFieldsOperationApply)
5656
if err != nil {
5757
return nil, nil, fmt.Errorf("failed to build manager identifier: %v", err)
5858
}
59-
return f.fieldManager.Apply(liveObj, patch, managed, manager, force)
59+
return f.fieldManager.Apply(liveObj, appliedObj, managed, manager, force)
6060
}
6161

6262
func (f *buildManagerInfoManager) buildManagerInfo(prefix string, operation metav1.ManagedFieldsOperationType) (string, error) {

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,8 @@ func (f *capManagersManager) Update(liveObj, newObj runtime.Object, managed Mana
5858
}
5959

6060
// Apply implements Manager.
61-
func (f *capManagersManager) Apply(liveObj runtime.Object, patch []byte, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) {
62-
return f.fieldManager.Apply(liveObj, patch, managed, fieldManager, force)
61+
func (f *capManagersManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) {
62+
return f.fieldManager.Apply(liveObj, appliedObj, managed, fieldManager, force)
6363
}
6464

6565
// capUpdateManagers merges a number of the oldest update entries into versioned buckets,

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/capmanagers_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func (*fakeManager) Update(_, newObj runtime.Object, managed fieldmanager.Manage
4141
return newObj, managed, nil
4242
}
4343

44-
func (*fakeManager) Apply(_ runtime.Object, _ []byte, _ fieldmanager.Managed, _ string, force bool) (runtime.Object, fieldmanager.Managed, error) {
44+
func (*fakeManager) Apply(_, _ runtime.Object, _ fieldmanager.Managed, _ string, force bool) (runtime.Object, fieldmanager.Managed, error) {
4545
panic("not implemented")
4646
return nil, nil, nil
4747
}

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ type Manager interface {
5151

5252
// Apply is used when server-side apply is called, as it merges the
5353
// object and updates the managed fields.
54-
Apply(liveObj runtime.Object, patch []byte, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error)
54+
Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error)
5555
}
5656

5757
// FieldManager updates the managed fields and merge applied
@@ -135,7 +135,7 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (o
135135

136136
// Apply is used when server-side apply is called, as it merges the
137137
// object and updates the managed fields.
138-
func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, manager string, force bool) (object runtime.Object, err error) {
138+
func (f *FieldManager) Apply(liveObj, appliedObj runtime.Object, manager string, force bool) (object runtime.Object, err error) {
139139
// If the object doesn't have metadata, apply isn't allowed.
140140
if _, err = meta.Accessor(liveObj); err != nil {
141141
return nil, fmt.Errorf("couldn't get accessor: %v", err)
@@ -149,7 +149,7 @@ func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, manager strin
149149

150150
internal.RemoveObjectManagedFields(liveObj)
151151

152-
if object, managed, err = f.fieldManager.Apply(liveObj, patch, managed, manager, force); err != nil {
152+
if object, managed, err = f.fieldManager.Apply(liveObj, appliedObj, managed, manager, force); err != nil {
153153
return nil, err
154154
}
155155

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager_test.go‎

Lines changed: 99 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ func (f *TestFieldManager) Reset() {
106106
f.liveObj = f.emptyObj.DeepCopyObject()
107107
}
108108

109-
func (f *TestFieldManager) Apply(obj []byte, manager string, force bool) error {
109+
func (f *TestFieldManager) Apply(obj runtime.Object, manager string, force bool) error {
110110
out, err := fieldmanager.NewFieldManager(f.fieldManager).Apply(f.liveObj, obj, manager, force)
111111
if err == nil {
112112
f.liveObj = out
@@ -174,7 +174,8 @@ func TestUpdateApplyConflict(t *testing.T) {
174174
t.Fatalf("failed to apply object: %v", err)
175175
}
176176

177-
err := f.Apply([]byte(`{
177+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
178+
if err := yaml.Unmarshal([]byte(`{
178179
"apiVersion": "apps/v1",
179180
"kind": "Deployment",
180181
"metadata": {
@@ -183,7 +184,11 @@ func TestUpdateApplyConflict(t *testing.T) {
183184
"spec": {
184185
"replicas": 101,
185186
}
186-
}`), "fieldmanager_conflict", false)
187+
}`), &appliedObj.Object); err != nil {
188+
t.Fatalf("error decoding YAML: %v", err)
189+
}
190+
191+
err := f.Apply(appliedObj, "fieldmanager_conflict", false)
187192
if err == nil || !apierrors.IsConflict(err) {
188193
t.Fatalf("Expecting to get conflicts but got %v", err)
189194
}
@@ -225,20 +230,68 @@ func TestApplyStripsFields(t *testing.T) {
225230
func TestVersionCheck(t *testing.T) {
226231
f := NewTestFieldManager(schema.FromAPIVersionAndKind("apps/v1", "Deployment"))
227232

228-
// patch has 'apiVersion: apps/v1' and live version is apps/v1 -> no errors
229-
err := f.Apply([]byte(`{
233+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
234+
if err := yaml.Unmarshal([]byte(`{
230235
"apiVersion": "apps/v1",
231236
"kind": "Deployment",
232-
}`), "fieldmanager_test", false)
237+
}`), &appliedObj.Object); err != nil {
238+
t.Fatalf("error decoding YAML: %v", err)
239+
}
240+
241+
// patch has 'apiVersion: apps/v1' and live version is apps/v1 -> no errors
242+
err := f.Apply(appliedObj, "fieldmanager_test", false)
233243
if err != nil {
234244
t.Fatalf("failed to apply object: %v", err)
235245
}
236246

237-
// patch has 'apiVersion: apps/v2' but live version is apps/v1 -> error
238-
err = f.Apply([]byte(`{
247+
appliedObj = &unstructured.Unstructured{Object: map[string]interface{}{}}
248+
if err := yaml.Unmarshal([]byte(`{
239249
"apiVersion": "apps/v1beta1",
240250
"kind": "Deployment",
241-
}`), "fieldmanager_test", false)
251+
}`), &appliedObj.Object); err != nil {
252+
t.Fatalf("error decoding YAML: %v", err)
253+
}
254+
255+
// patch has 'apiVersion: apps/v2' but live version is apps/v1 -> error
256+
err = f.Apply(appliedObj, "fieldmanager_test", false)
257+
if err == nil {
258+
t.Fatalf("expected an error from mismatched patch and live versions")
259+
}
260+
switch typ := err.(type) {
261+
default:
262+
t.Fatalf("expected error to be of type %T was %T", apierrors.StatusError{}, typ)
263+
case apierrors.APIStatus:
264+
if typ.Status().Code != http.StatusBadRequest {
265+
t.Fatalf("expected status code to be %d but was %d",
266+
http.StatusBadRequest, typ.Status().Code)
267+
}
268+
}
269+
}
270+
func TestVersionCheckDoesNotPanic(t *testing.T) {
271+
f := NewTestFieldManager(schema.FromAPIVersionAndKind("apps/v1", "Deployment"))
272+
273+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
274+
if err := yaml.Unmarshal([]byte(`{
275+
"apiVersion": "apps/v1",
276+
"kind": "Deployment",
277+
}`), &appliedObj.Object); err != nil {
278+
t.Fatalf("error decoding YAML: %v", err)
279+
}
280+
281+
// patch has 'apiVersion: apps/v1' and live version is apps/v1 -> no errors
282+
err := f.Apply(appliedObj, "fieldmanager_test", false)
283+
if err != nil {
284+
t.Fatalf("failed to apply object: %v", err)
285+
}
286+
287+
appliedObj = &unstructured.Unstructured{Object: map[string]interface{}{}}
288+
if err := yaml.Unmarshal([]byte(`{
289+
}`), &appliedObj.Object); err != nil {
290+
t.Fatalf("error decoding YAML: %v", err)
291+
}
292+
293+
// patch has 'apiVersion: apps/v2' but live version is apps/v1 -> error
294+
err = f.Apply(appliedObj, "fieldmanager_test", false)
242295
if err == nil {
243296
t.Fatalf("expected an error from mismatched patch and live versions")
244297
}
@@ -256,15 +309,20 @@ func TestVersionCheck(t *testing.T) {
256309
func TestApplyDoesNotStripLabels(t *testing.T) {
257310
f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod"))
258311

259-
err := f.Apply([]byte(`{
312+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
313+
if err := yaml.Unmarshal([]byte(`{
260314
"apiVersion": "v1",
261315
"kind": "Pod",
262316
"metadata": {
263317
"labels": {
264318
"a": "b"
265319
},
266320
}
267-
}`), "fieldmanager_test", false)
321+
}`), &appliedObj.Object); err != nil {
322+
t.Fatalf("error decoding YAML: %v", err)
323+
}
324+
325+
err := f.Apply(appliedObj, "fieldmanager_test", false)
268326
if err != nil {
269327
t.Fatalf("failed to apply object: %v", err)
270328
}
@@ -305,7 +363,12 @@ func TestApplyNewObject(t *testing.T) {
305363
t.Run(test.gvk.String(), func(t *testing.T) {
306364
f := NewTestFieldManager(test.gvk)
307365

308-
if err := f.Apply(test.obj, "fieldmanager_test", false); err != nil {
366+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
367+
if err := yaml.Unmarshal(test.obj, &appliedObj.Object); err != nil {
368+
t.Fatalf("error decoding YAML: %v", err)
369+
}
370+
371+
if err := f.Apply(appliedObj, "fieldmanager_test", false); err != nil {
309372
t.Fatal(err)
310373
}
311374
})
@@ -362,7 +425,11 @@ func BenchmarkNewObject(b *testing.B) {
362425
b.ReportAllocs()
363426
b.ResetTimer()
364427
for n := 0; n < b.N; n++ {
365-
err := f.Apply(test.obj, "fieldmanager_test", false)
428+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
429+
if err := yaml.Unmarshal(test.obj, &appliedObj.Object); err != nil {
430+
b.Fatalf("error decoding YAML: %v", err)
431+
}
432+
err := f.Apply(appliedObj, "fieldmanager_test", false)
366433
if err != nil {
367434
b.Fatal(err)
368435
}
@@ -387,7 +454,12 @@ func BenchmarkRepeatedUpdate(b *testing.B) {
387454
obj.Spec.Containers[0].Image = "nginx:4.3"
388455
objs = append(objs, obj)
389456

390-
err := f.Apply(podBytes, "fieldmanager_apply", false)
457+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
458+
if err := yaml.Unmarshal(podBytes, &appliedObj.Object); err != nil {
459+
b.Fatalf("error decoding YAML: %v", err)
460+
}
461+
462+
err := f.Apply(appliedObj, "fieldmanager_apply", false)
391463
if err != nil {
392464
b.Fatal(err)
393465
}
@@ -410,7 +482,8 @@ func BenchmarkRepeatedUpdate(b *testing.B) {
410482
func TestApplyFailsWithManagedFields(t *testing.T) {
411483
f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod"))
412484

413-
err := f.Apply([]byte(`{
485+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
486+
if err := yaml.Unmarshal([]byte(`{
414487
"apiVersion": "v1",
415488
"kind": "Pod",
416489
"metadata": {
@@ -420,7 +493,11 @@ func TestApplyFailsWithManagedFields(t *testing.T) {
420493
}
421494
]
422495
}
423-
}`), "fieldmanager_test", false)
496+
}`), &appliedObj.Object); err != nil {
497+
t.Fatalf("error decoding YAML: %v", err)
498+
}
499+
500+
err := f.Apply(appliedObj, "fieldmanager_test", false)
424501

425502
if err == nil {
426503
t.Fatalf("successfully applied with set managed fields")
@@ -430,15 +507,19 @@ func TestApplyFailsWithManagedFields(t *testing.T) {
430507
func TestApplySuccessWithNoManagedFields(t *testing.T) {
431508
f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod"))
432509

433-
err := f.Apply([]byte(`{
510+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
511+
if err := yaml.Unmarshal([]byte(`{
434512
"apiVersion": "v1",
435513
"kind": "Pod",
436514
"metadata": {
437515
"labels": {
438516
"a": "b"
439517
},
440518
}
441-
}`), "fieldmanager_test", false)
519+
}`), &appliedObj.Object); err != nil {
520+
t.Fatalf("error decoding YAML: %v", err)
521+
}
522+
err := f.Apply(appliedObj, "fieldmanager_test", false)
442523

443524
if err != nil {
444525
t.Fatalf("failed to apply object: %v", err)

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func (f *skipNonAppliedManager) Update(liveObj, newObj runtime.Object, managed M
5151
}
5252

5353
// Apply implements Manager.
54-
func (f *skipNonAppliedManager) Apply(liveObj runtime.Object, patch []byte, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) {
54+
func (f *skipNonAppliedManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) {
5555
if len(managed.Fields()) == 0 {
5656
emptyObj, err := f.objectCreater.New(f.gvk)
5757
if err != nil {
@@ -62,5 +62,5 @@ func (f *skipNonAppliedManager) Apply(liveObj runtime.Object, patch []byte, mana
6262
return nil, nil, fmt.Errorf("failed to create manager for existing fields: %v", err)
6363
}
6464
}
65-
return f.fieldManager.Apply(liveObj, patch, managed, fieldManager, force)
65+
return f.fieldManager.Apply(liveObj, appliedObj, managed, fieldManager, force)
6666
}

‎staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/skipnonapplied_test.go‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"k8s.io/apimachinery/pkg/runtime"
2727
"k8s.io/apimachinery/pkg/runtime/schema"
2828
"k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager"
29+
"sigs.k8s.io/yaml"
2930
)
3031

3132
type fakeObjectCreater struct {
@@ -49,7 +50,8 @@ func TestNoUpdateBeforeFirstApply(t *testing.T) {
4950
schema.GroupVersionKind{},
5051
)
5152

52-
if err := f.Apply([]byte(`{
53+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
54+
if err := yaml.Unmarshal([]byte(`{
5355
"apiVersion": "v1",
5456
"kind": "Pod",
5557
"metadata": {
@@ -62,7 +64,11 @@ func TestNoUpdateBeforeFirstApply(t *testing.T) {
6264
"image": "nginx:latest"
6365
}]
6466
}
65-
}`), "fieldmanager_test_apply", false); err != nil {
67+
}`), &appliedObj.Object); err != nil {
68+
t.Fatalf("error decoding YAML: %v", err)
69+
}
70+
71+
if err := f.Apply(appliedObj, "fieldmanager_test_apply", false); err != nil {
6672
t.Fatalf("failed to update object: %v", err)
6773
}
6874

@@ -96,7 +102,8 @@ func TestUpdateBeforeFirstApply(t *testing.T) {
96102
t.Fatalf("managedFields were tracked on update only: %v", m)
97103
}
98104

99-
appliedBytes := []byte(`{
105+
appliedObj := &unstructured.Unstructured{Object: map[string]interface{}{}}
106+
if err := yaml.Unmarshal([]byte(`{
100107
"apiVersion": "v1",
101108
"kind": "Pod",
102109
"metadata": {
@@ -109,9 +116,11 @@ func TestUpdateBeforeFirstApply(t *testing.T) {
109116
"image": "nginx:latest"
110117
}]
111118
}
112-
}`)
119+
}`), &appliedObj.Object); err != nil {
120+
t.Fatalf("error decoding YAML: %v", err)
121+
}
113122

114-
err := f.Apply(appliedBytes, "fieldmanager_test_apply", false)
123+
err := f.Apply(appliedObj, "fieldmanager_test_apply", false)
115124
apiStatus, _ := err.(apierrors.APIStatus)
116125
if err == nil || !apierrors.IsConflict(err) || len(apiStatus.Status().Details.Causes) != 1 {
117126
t.Fatalf("Expecting to get one conflict but got %v", err)
@@ -125,7 +134,7 @@ func TestUpdateBeforeFirstApply(t *testing.T) {
125134
t.Fatalf("Expecting conflict message to contain %q but got %q: %v", e, a, err)
126135
}
127136

128-
if err := f.Apply(appliedBytes, "fieldmanager_test_apply", true); err != nil {
137+
if err := f.Apply(appliedObj, "fieldmanager_test_apply", true); err != nil {
129138
t.Fatalf("failed to update object: %v", err)
130139
}
131140

0 commit comments

Comments
 (0)