From 6619442defceb9bdb2aa8cf26715b810d506e348 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 15 Feb 2022 13:54:52 -0500 Subject: [PATCH 1/2] Revert v1beta1 PodDisruptionBudget select patchStrategy --- api/openapi-spec/swagger.json | 3 +-- api/openapi-spec/v3/apis__policy__v1beta1_openapi.json | 3 +-- staging/src/k8s.io/api/policy/v1beta1/generated.proto | 1 - staging/src/k8s.io/api/policy/v1beta1/types.go | 3 +-- 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index 04af998f5bd..efea95dc3af 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -13849,8 +13849,7 @@ }, "selector": { "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.LabelSelector", - "description": "Label query over pods whose evictions are managed by the disruption budget. A null selector selects no pods. An empty selector ({}) also selects no pods, which differs from standard behavior of selecting all pods. In policy/v1, an empty selector will select all pods in the namespace.", - "x-kubernetes-patch-strategy": "replace" + "description": "Label query over pods whose evictions are managed by the disruption budget. A null selector selects no pods. An empty selector ({}) also selects no pods, which differs from standard behavior of selecting all pods. In policy/v1, an empty selector will select all pods in the namespace." } }, "type": "object" diff --git a/api/openapi-spec/v3/apis__policy__v1beta1_openapi.json b/api/openapi-spec/v3/apis__policy__v1beta1_openapi.json index 45b33e341dd..d9a0c7bd2a4 100644 --- a/api/openapi-spec/v3/apis__policy__v1beta1_openapi.json +++ b/api/openapi-spec/v3/apis__policy__v1beta1_openapi.json @@ -213,8 +213,7 @@ }, "selector": { "$ref": "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.LabelSelector", - "description": "Label query over pods whose evictions are managed by the disruption budget. A null selector selects no pods. An empty selector ({}) also selects no pods, which differs from standard behavior of selecting all pods. In policy/v1, an empty selector will select all pods in the namespace.", - "x-kubernetes-patch-strategy": "replace" + "description": "Label query over pods whose evictions are managed by the disruption budget. A null selector selects no pods. An empty selector ({}) also selects no pods, which differs from standard behavior of selecting all pods. In policy/v1, an empty selector will select all pods in the namespace." } }, "type": "object" diff --git a/staging/src/k8s.io/api/policy/v1beta1/generated.proto b/staging/src/k8s.io/api/policy/v1beta1/generated.proto index 133ba0493c6..8a2824b51bf 100644 --- a/staging/src/k8s.io/api/policy/v1beta1/generated.proto +++ b/staging/src/k8s.io/api/policy/v1beta1/generated.proto @@ -144,7 +144,6 @@ message PodDisruptionBudgetSpec { // A null selector selects no pods. // An empty selector ({}) also selects no pods, which differs from standard behavior of selecting all pods. // In policy/v1, an empty selector will select all pods in the namespace. - // +patchStrategy=replace // +optional optional k8s.io.apimachinery.pkg.apis.meta.v1.LabelSelector selector = 2; diff --git a/staging/src/k8s.io/api/policy/v1beta1/types.go b/staging/src/k8s.io/api/policy/v1beta1/types.go index 553cb316dcf..486f93461af 100644 --- a/staging/src/k8s.io/api/policy/v1beta1/types.go +++ b/staging/src/k8s.io/api/policy/v1beta1/types.go @@ -36,9 +36,8 @@ type PodDisruptionBudgetSpec struct { // A null selector selects no pods. // An empty selector ({}) also selects no pods, which differs from standard behavior of selecting all pods. // In policy/v1, an empty selector will select all pods in the namespace. - // +patchStrategy=replace // +optional - Selector *metav1.LabelSelector `json:"selector,omitempty" patchStrategy:"replace" protobuf:"bytes,2,opt,name=selector"` + Selector *metav1.LabelSelector `json:"selector,omitempty" protobuf:"bytes,2,opt,name=selector"` // An eviction is allowed if at most "maxUnavailable" pods selected by // "selector" are unavailable after the eviction, i.e. even in absence of From 33fc0b965178df5d00df15ec477c4790ca39d5a3 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 15 Feb 2022 14:45:22 -0500 Subject: [PATCH 2/2] Add PDB selector patch integration test --- .../integration/disruption/disruption_test.go | 149 +++++++++++++++++- 1 file changed, 146 insertions(+), 3 deletions(-) diff --git a/test/integration/disruption/disruption_test.go b/test/integration/disruption/disruption_test.go index 1229385cf4e..8d97f00aebd 100644 --- a/test/integration/disruption/disruption_test.go +++ b/test/integration/disruption/disruption_test.go @@ -19,20 +19,22 @@ package disruption import ( "context" "fmt" + "reflect" "testing" "time" - "k8s.io/apiextensions-apiserver/test/integration/fixtures" - - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "github.com/google/go-cmp/cmp" v1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" "k8s.io/api/policy/v1beta1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" + "k8s.io/apiextensions-apiserver/test/integration/fixtures" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/wait" cacheddiscovery "k8s.io/client-go/discovery/cached/memory" @@ -47,6 +49,7 @@ import ( "k8s.io/kubernetes/pkg/controller/disruption" "k8s.io/kubernetes/test/integration/etcd" "k8s.io/kubernetes/test/integration/framework" + "k8s.io/utils/pointer" ) func setup(t *testing.T) (*kubeapiservertesting.TestServer, *disruption.DisruptionController, informers.SharedInformerFactory, clientset.Interface, *apiextensionsclientset.Clientset, dynamic.Interface) { @@ -493,3 +496,143 @@ func waitToObservePods(t *testing.T, podInformer cache.SharedIndexInformer, podN t.Fatal(err) } } + +func TestPatchCompatibility(t *testing.T) { + s, _, _, clientSet, _, _ := setup(t) + defer s.TearDownFn() + + testcases := []struct { + name string + version string + startingSelector *metav1.LabelSelector + patchType types.PatchType + patch string + force *bool + fieldManager string + expectSelector *metav1.LabelSelector + }{ + { + name: "v1beta1-smp", + version: "v1beta1", + patchType: types.StrategicMergePatchType, + patch: `{"spec":{"selector":{"matchLabels":{"patchmatch":"true"},"matchExpressions":[{"key":"patchexpression","operator":"In","values":["true"]}]}}}`, + // matchLabels portion is merged, matchExpressions portion is replaced (because it's a list with no patchStrategy defined) + expectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"basematch": "true", "patchmatch": "true"}, + MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "patchexpression", Operator: "In", Values: []string{"true"}}}, + }, + }, + { + name: "v1-smp", + version: "v1", + patchType: types.StrategicMergePatchType, + patch: `{"spec":{"selector":{"matchLabels":{"patchmatch":"true"},"matchExpressions":[{"key":"patchexpression","operator":"In","values":["true"]}]}}}`, + // matchLabels and matchExpressions are both replaced (because selector patchStrategy=replace in v1) + expectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"patchmatch": "true"}, + MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "patchexpression", Operator: "In", Values: []string{"true"}}}, + }, + }, + + { + name: "v1beta1-mergepatch", + version: "v1beta1", + patchType: types.MergePatchType, + patch: `{"spec":{"selector":{"matchLabels":{"patchmatch":"true"},"matchExpressions":[{"key":"patchexpression","operator":"In","values":["true"]}]}}}`, + // matchLabels portion is merged, matchExpressions portion is replaced (because it's a list) + expectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"basematch": "true", "patchmatch": "true"}, + MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "patchexpression", Operator: "In", Values: []string{"true"}}}, + }, + }, + { + name: "v1-mergepatch", + version: "v1", + patchType: types.MergePatchType, + patch: `{"spec":{"selector":{"matchLabels":{"patchmatch":"true"},"matchExpressions":[{"key":"patchexpression","operator":"In","values":["true"]}]}}}`, + // matchLabels portion is merged, matchExpressions portion is replaced (because it's a list) + expectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"basematch": "true", "patchmatch": "true"}, + MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "patchexpression", Operator: "In", Values: []string{"true"}}}, + }, + }, + + { + name: "v1beta1-apply", + version: "v1beta1", + patchType: types.ApplyPatchType, + patch: `{"apiVersion":"policy/v1beta1","kind":"PodDisruptionBudget","spec":{"selector":{"matchLabels":{"patchmatch":"true"},"matchExpressions":[{"key":"patchexpression","operator":"In","values":["true"]}]}}}`, + force: pointer.Bool(true), + fieldManager: "test", + // entire selector is replaced (because structType=atomic) + expectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"patchmatch": "true"}, + MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "patchexpression", Operator: "In", Values: []string{"true"}}}, + }, + }, + { + name: "v1-apply", + version: "v1", + patchType: types.ApplyPatchType, + patch: `{"apiVersion":"policy/v1","kind":"PodDisruptionBudget","spec":{"selector":{"matchLabels":{"patchmatch":"true"},"matchExpressions":[{"key":"patchexpression","operator":"In","values":["true"]}]}}}`, + force: pointer.Bool(true), + fieldManager: "test", + // entire selector is replaced (because structType=atomic) + expectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"patchmatch": "true"}, + MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "patchexpression", Operator: "In", Values: []string{"true"}}}, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + ns := "default" + maxUnavailable := int32(2) + pdb := &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pdb", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: maxUnavailable}, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"basematch": "true"}, + MatchExpressions: []metav1.LabelSelectorRequirement{{Key: "baseexpression", Operator: "In", Values: []string{"true"}}}, + }, + }, + } + if _, err := clientSet.PolicyV1().PodDisruptionBudgets(ns).Create(context.TODO(), pdb, metav1.CreateOptions{}); err != nil { + t.Fatalf("Error creating PodDisruptionBudget: %v", err) + } + defer func() { + err := clientSet.PolicyV1().PodDisruptionBudgets(ns).Delete(context.TODO(), pdb.Name, metav1.DeleteOptions{}) + if err != nil { + t.Fatal(err) + } + }() + + var resultSelector *metav1.LabelSelector + switch tc.version { + case "v1": + result, err := clientSet.PolicyV1().PodDisruptionBudgets(ns).Patch(context.TODO(), pdb.Name, tc.patchType, []byte(tc.patch), metav1.PatchOptions{Force: tc.force, FieldManager: tc.fieldManager}) + if err != nil { + t.Fatal(err) + } + resultSelector = result.Spec.Selector + case "v1beta1": + result, err := clientSet.PolicyV1beta1().PodDisruptionBudgets(ns).Patch(context.TODO(), pdb.Name, tc.patchType, []byte(tc.patch), metav1.PatchOptions{Force: tc.force, FieldManager: tc.fieldManager}) + if err != nil { + t.Fatal(err) + } + resultSelector = result.Spec.Selector + default: + t.Error("unknown version") + } + + if !reflect.DeepEqual(resultSelector, tc.expectSelector) { + t.Fatalf("unexpected selector:\n%s", cmp.Diff(tc.expectSelector, resultSelector)) + } + }) + } + +}