Add nil path to mapping when a CR has no "scale" subresource

nodo · nodo · commit 5b666a61a170 · 2021-04-21T20:27:47.000+02:00
This is to prevent the ScaleHandler to drop the entry. In this way
entries just get ignored.
diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go
@@ -709,6 +709,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
 			return nil, fmt.Errorf("the server could not properly serve the CR subresources")
 		}
 		if subresources == nil || subresources.Scale == nil {
+			replicasPathInCustomResource[schema.GroupVersion{Group: crd.Spec.Group, Version: v.Name}.String()] = nil
 			continue
 		}
 		path := fieldpath.Path{}
diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/scalehandler.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/scalehandler.go
@@ -69,8 +69,8 @@ func (h *ScaleHandler) ToSubresource() ([]metav1.ManagedFieldsEntry, error) {
 	t := map[string]*metav1.Time{}
 	for manager, versionedSet := range managed.Fields() {
 		path, ok := h.mappings[string(versionedSet.APIVersion())]
-		// Drop the field if the APIVersion of the managed field is unknown
-		if !ok {
+		// Skip the entry if the APIVersion is unknown
+		if !ok || path == nil {
 			continue
 		}
 
@@ -110,13 +110,15 @@ func (h *ScaleHandler) ToParent(scaleEntries []metav1.ManagedFieldsEntry) ([]met
 	for manager, versionedSet := range parentFields {
 		// Get the main resource "replicas" path
 		path, ok := h.mappings[string(versionedSet.APIVersion())]
-		// Drop the field if the APIVersion of the managed field is unknown
+		// Drop the entry if the APIVersion is unknown.
 		if !ok {
 			continue
 		}
 
-		// If the parent entry does not have the replicas path, just keep it as it is
-		if !versionedSet.Set().Has(path) {
+		// If the parent entry does not have the replicas path or it is nil, just
+		// keep it as it is. The path is nil for Custom Resources without scale
+		// subresource.
+		if path == nil || !versionedSet.Set().Has(path) {
 			f[manager] = versionedSet
 			t[manager] = decodedParentEntries.Times()[manager]
 			continue
diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/scalehandler_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/scalehandler_test.go
@@ -578,7 +578,7 @@ func TestTransformingManagedFieldsToParent(t *testing.T) {
 			},
 		},
 		{
-			desc: "drops other fields if the api version is unknown",
+			desc: "drops the entry if the api version is unknown",
 			parent: []metav1.ManagedFieldsEntry{
 				{
 					Manager:    "test",
@@ -636,14 +636,16 @@ func TestTransformingManagedFieldsToParent(t *testing.T) {
 
 func TestTransformingManagedFieldsToParentMultiVersion(t *testing.T) {
 	tests := []struct {
-		desc        string
-		mappings    ResourcePathMappings
-		parent      []metav1.ManagedFieldsEntry
-		subresource []metav1.ManagedFieldsEntry
-		expected    []metav1.ManagedFieldsEntry
+		desc         string
+		groupVersion schema.GroupVersion
+		mappings     ResourcePathMappings
+		parent       []metav1.ManagedFieldsEntry
+		subresource  []metav1.ManagedFieldsEntry
+		expected     []metav1.ManagedFieldsEntry
 	}{
 		{
-			desc: "multi-version",
+			desc:         "multi-version",
+			groupVersion: schema.GroupVersion{Group: "apps", Version: "v1"},
 			mappings: ResourcePathMappings{
 				"apps/v1": fieldpath.MakePathOrDie("spec", "the-replicas"),
 				"apps/v2": fieldpath.MakePathOrDie("spec", "not-the-replicas"),
@@ -699,13 +701,71 @@ func TestTransformingManagedFieldsToParentMultiVersion(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc:         "Custom resource without scale subresource, scaling a version with `scale`",
+			groupVersion: schema.GroupVersion{Group: "mygroup", Version: "v1"},
+			mappings: ResourcePathMappings{
+				"mygroup/v1": fieldpath.MakePathOrDie("spec", "the-replicas"),
+				"mygroup/v2": nil,
+			},
+			parent: []metav1.ManagedFieldsEntry{
+				{
+					Manager:    "test",
+					Operation:  metav1.ManagedFieldsOperationApply,
+					APIVersion: "mygroup/v1",
+					FieldsType: "FieldsV1",
+					FieldsV1:   &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:the-replicas":{},"f:selector":{}}}`)},
+				},
+				{
+					Manager:    "test-other",
+					Operation:  metav1.ManagedFieldsOperationApply,
+					APIVersion: "mygroup/v2",
+					FieldsType: "FieldsV1",
+					FieldsV1:   &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:test-other":{}}}`)},
+				},
+			},
+			subresource: []metav1.ManagedFieldsEntry{
+				{
+					Manager:     "scale",
+					Operation:   metav1.ManagedFieldsOperationUpdate,
+					APIVersion:  "autoscaling/v1",
+					FieldsType:  "FieldsV1",
+					FieldsV1:    &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:replicas":{}}}`)},
+					Subresource: "scale",
+				},
+			},
+			expected: []metav1.ManagedFieldsEntry{
+				{
+					Manager:    "test",
+					Operation:  metav1.ManagedFieldsOperationApply,
+					APIVersion: "mygroup/v1",
+					FieldsType: "FieldsV1",
+					FieldsV1:   &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:selector":{}}}`)},
+				},
+				{
+					Manager:    "test-other",
+					Operation:  metav1.ManagedFieldsOperationApply,
+					APIVersion: "mygroup/v2",
+					FieldsType: "FieldsV1",
+					FieldsV1:   &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:test-other":{}}}`)},
+				},
+				{
+					Manager:     "scale",
+					Operation:   metav1.ManagedFieldsOperationUpdate,
+					APIVersion:  "mygroup/v1",
+					FieldsType:  "FieldsV1",
+					FieldsV1:    &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:the-replicas":{}}}`)},
+					Subresource: "scale",
+				},
+			},
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
 			handler := NewScaleHandler(
 				test.parent,
-				schema.GroupVersion{Group: "apps", Version: "v1"},
+				test.groupVersion,
 				test.mappings,
 			)
 			parentEntries, err := handler.ToParent(test.subresource)

Original file line number	Diff line number	Diff line change
`@@ -709,6 +709,7 @@ func (r crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (crd`
`709`	`709`	`return nil, fmt.Errorf("the server could not properly serve the CR subresources")`
`710`	`710`	`}`
`711`	`711`	`if subresources == nil \|\| subresources.Scale == nil {`
	`712`	`+ replicasPathInCustomResource[schema.GroupVersion{Group: crd.Spec.Group, Version: v.Name}.String()] = nil`
`712`	`713`	`continue`
`713`	`714`	`}`
`714`	`715`	`path := fieldpath.Path{}`