Skip to content

Commit

Permalink
Merge pull request kubernetes#117666 from carlory/fix-008
Browse files Browse the repository at this point in the history 
Remove ability to re-enable serving deprecated policyv1beta1 APIs
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jul 18, 2023
2 parents b3fc4cf + 850dc61 commit 56b59c8
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 89 deletions.
2 changes: 0 additions & 2 deletions pkg/controlplane/instance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ import (
networkingapiv1alpha1 "k8s.io/api/networking/v1alpha1"
nodev1 "k8s.io/api/node/v1"
policyapiv1 "k8s.io/api/policy/v1"
policyapiv1beta1 "k8s.io/api/policy/v1beta1"
rbacv1 "k8s.io/api/rbac/v1"
resourcev1alpha2 "k8s.io/api/resource/v1alpha2"
schedulingapiv1 "k8s.io/api/scheduling/v1"
Expand Down Expand Up @@ -719,7 +718,6 @@ var (
// betaAPIGroupVersionsDisabledByDefault is for all future beta groupVersions.
betaAPIGroupVersionsDisabledByDefault = []schema.GroupVersion{
authenticationv1beta1.SchemeGroupVersion,
policyapiv1beta1.SchemeGroupVersion,
storageapiv1beta1.SchemeGroupVersion,
flowcontrolv1beta1.SchemeGroupVersion,
flowcontrolv1beta2.SchemeGroupVersion,
Expand Down
23 changes: 0 additions & 23 deletions pkg/registry/policy/rest/storage_policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package rest

import (
policyapiv1 "k8s.io/api/policy/v1"
policyapiv1beta1 "k8s.io/api/policy/v1beta1"
"k8s.io/apiserver/pkg/registry/generic"
"k8s.io/apiserver/pkg/registry/rest"
genericapiserver "k8s.io/apiserver/pkg/server"
Expand All @@ -35,12 +34,6 @@ func (p RESTStorageProvider) NewRESTStorage(apiResourceConfigSource serverstorag
// If you add a version here, be sure to add an entry in `k8s.io/kubernetes/cmd/kube-apiserver/app/aggregator.go with specific priorities.
// TODO refactor the plumbing to provide the information in the APIGroupInfo

if storageMap, err := p.v1beta1Storage(apiResourceConfigSource, restOptionsGetter); err != nil {
return genericapiserver.APIGroupInfo{}, err
} else if len(storageMap) > 0 {
apiGroupInfo.VersionedResourcesStorageMap[policyapiv1beta1.SchemeGroupVersion.Version] = storageMap
}

if storageMap, err := p.v1Storage(apiResourceConfigSource, restOptionsGetter); err != nil {
return genericapiserver.APIGroupInfo{}, err
} else if len(storageMap) > 0 {
Expand All @@ -50,22 +43,6 @@ func (p RESTStorageProvider) NewRESTStorage(apiResourceConfigSource serverstorag
return apiGroupInfo, nil
}

func (p RESTStorageProvider) v1beta1Storage(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter) (map[string]rest.Storage, error) {
storage := map[string]rest.Storage{}

// poddisruptionbudgets
if resource := "poddisruptionbudgets"; apiResourceConfigSource.ResourceEnabled(policyapiv1beta1.SchemeGroupVersion.WithResource(resource)) {
poddisruptionbudgetStorage, poddisruptionbudgetStatusStorage, err := poddisruptionbudgetstore.NewREST(restOptionsGetter)
if err != nil {
return storage, err
}
storage[resource] = poddisruptionbudgetStorage
storage[resource+"/status"] = poddisruptionbudgetStatusStorage
}

return storage, nil
}

func (p RESTStorageProvider) v1Storage(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter) (map[string]rest.Storage, error) {
storage := map[string]rest.Storage{}

Expand Down
94 changes: 38 additions & 56 deletions test/integration/disruption/disruption_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,17 @@ limitations under the License.
package disruption

import (
"bytes"
"context"
"fmt"
"path"
"reflect"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
clientv3 "go.etcd.io/etcd/client/v3"
v1 "k8s.io/api/core/v1"
policyv1 "k8s.io/api/policy/v1"
"k8s.io/api/policy/v1beta1"
Expand All @@ -34,9 +37,12 @@ import (
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/runtime/serializer/protobuf"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/wait"
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/apiserver/pkg/registry/rest"
cacheddiscovery "k8s.io/client-go/discovery/cached/memory"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/informers"
Expand All @@ -46,6 +52,7 @@ import (
"k8s.io/client-go/scale"
"k8s.io/client-go/tools/cache"
kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing"
"k8s.io/kubernetes/pkg/api/legacyscheme"
"k8s.io/kubernetes/pkg/controller/disruption"
"k8s.io/kubernetes/test/integration/etcd"
"k8s.io/kubernetes/test/integration/framework"
Expand Down Expand Up @@ -201,12 +208,12 @@ func TestPDBWithScaleSubresource(t *testing.T) {
func TestEmptySelector(t *testing.T) {
testcases := []struct {
name string
createPDBFunc func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error
createPDBFunc func(ctx context.Context, clientSet clientset.Interface, etcdClient *clientv3.Client, etcdStoragePrefix, name, nsName string, minAvailable intstr.IntOrString) error
expectedCurrentHealthy int32
}{
{
name: "v1beta1 should not target any pods",
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error {
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, etcdClient *clientv3.Client, etcdStoragePrefix, name, nsName string, minAvailable intstr.IntOrString) error {
pdb := &v1beta1.PodDisruptionBudget{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -216,14 +223,13 @@ func TestEmptySelector(t *testing.T) {
Selector: &metav1.LabelSelector{},
},
}
_, err := clientSet.PolicyV1beta1().PodDisruptionBudgets(nsName).Create(ctx, pdb, metav1.CreateOptions{})
return err
return createPDBUsingRemovedAPI(ctx, etcdClient, etcdStoragePrefix, nsName, pdb)
},
expectedCurrentHealthy: 0,
},
{
name: "v1 should target all pods",
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error {
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, etcdClient *clientv3.Client, etcdStoragePrefix, name, nsName string, minAvailable intstr.IntOrString) error {
pdb := &policyv1.PodDisruptionBudget{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand Down Expand Up @@ -266,7 +272,7 @@ func TestEmptySelector(t *testing.T) {
waitToObservePods(t, informers.Core().V1().Pods().Informer(), 4, v1.PodRunning)

pdbName := "test-pdb"
if err := tc.createPDBFunc(ctx, clientSet, pdbName, nsName, minAvailable); err != nil {
if err := tc.createPDBFunc(ctx, clientSet, s.EtcdClient, s.EtcdStoragePrefix, pdbName, nsName, minAvailable); err != nil {
t.Errorf("Error creating PodDisruptionBudget: %v", err)
}

Expand All @@ -287,12 +293,12 @@ func TestEmptySelector(t *testing.T) {
func TestSelectorsForPodsWithoutLabels(t *testing.T) {
testcases := []struct {
name string
createPDBFunc func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error
createPDBFunc func(ctx context.Context, clientSet clientset.Interface, etcdClient *clientv3.Client, etcdStoragePrefix, name, nsName string, minAvailable intstr.IntOrString) error
expectedCurrentHealthy int32
}{
{
name: "pods with no labels can be targeted by v1 PDBs with empty selector",
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error {
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, etcdClient *clientv3.Client, etcdStoragePrefix, name, nsName string, minAvailable intstr.IntOrString) error {
pdb := &policyv1.PodDisruptionBudget{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -309,7 +315,7 @@ func TestSelectorsForPodsWithoutLabels(t *testing.T) {
},
{
name: "pods with no labels can be targeted by v1 PDBs with DoesNotExist selector",
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error {
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, etcdClient *clientv3.Client, etcdStoragePrefix, name, nsName string, minAvailable intstr.IntOrString) error {
pdb := &policyv1.PodDisruptionBudget{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -333,7 +339,7 @@ func TestSelectorsForPodsWithoutLabels(t *testing.T) {
},
{
name: "pods with no labels can be targeted by v1beta1 PDBs with DoesNotExist selector",
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, name, nsName string, minAvailable intstr.IntOrString) error {
createPDBFunc: func(ctx context.Context, clientSet clientset.Interface, etcdClient *clientv3.Client, etcdStoragePrefix, name, nsName string, minAvailable intstr.IntOrString) error {
pdb := &v1beta1.PodDisruptionBudget{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Expand All @@ -350,8 +356,7 @@ func TestSelectorsForPodsWithoutLabels(t *testing.T) {
},
},
}
_, err := clientSet.PolicyV1beta1().PodDisruptionBudgets(nsName).Create(ctx, pdb, metav1.CreateOptions{})
return err
return createPDBUsingRemovedAPI(ctx, etcdClient, etcdStoragePrefix, nsName, pdb)
},
expectedCurrentHealthy: 1,
},
Expand All @@ -376,7 +381,7 @@ func TestSelectorsForPodsWithoutLabels(t *testing.T) {

// Create the PDB first and wait for it to settle.
pdbName := "test-pdb"
if err := tc.createPDBFunc(ctx, clientSet, pdbName, nsName, minAvailable); err != nil {
if err := tc.createPDBFunc(ctx, clientSet, s.EtcdClient, s.EtcdStoragePrefix, pdbName, nsName, minAvailable); err != nil {
t.Errorf("Error creating PodDisruptionBudget: %v", err)
}
waitPDBStable(ctx, t, clientSet, 0, nsName, pdbName)
Expand Down Expand Up @@ -513,6 +518,26 @@ func waitToObservePods(t *testing.T, podInformer cache.SharedIndexInformer, podN
}
}

// createPDBUsingRemovedAPI creates a PDB directly using etcd. This is must *ONLY* be used for checks of compatibility
// with removed data. Do not use this just because you don't want to update your test to use v1. Only use this
// when it actually matters.
func createPDBUsingRemovedAPI(ctx context.Context, etcdClient *clientv3.Client, etcdStoragePrefix, nsName string, betaPDB *v1beta1.PodDisruptionBudget) error {
betaPDB.APIVersion = v1beta1.SchemeGroupVersion.Group + "/" + v1beta1.SchemeGroupVersion.Version
betaPDB.Kind = "PodDisruptionBudget"
betaPDB.Namespace = nsName
betaPDB.Generation = 1
rest.FillObjectMetaSystemFields(betaPDB)
ctx = genericapirequest.WithNamespace(ctx, nsName)
key := path.Join("/", etcdStoragePrefix, "poddisruptionbudgets", nsName, betaPDB.Name)
protoSerializer := protobuf.NewSerializer(legacyscheme.Scheme, legacyscheme.Scheme)
buffer := bytes.NewBuffer(nil)
if err := protoSerializer.Encode(betaPDB, buffer); err != nil {
return err
}
_, err := etcdClient.Put(ctx, key, buffer.String())
return err
}

func TestPatchCompatibility(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())

Expand All @@ -533,17 +558,6 @@ func TestPatchCompatibility(t *testing.T) {
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",
Expand All @@ -555,18 +569,6 @@ func TestPatchCompatibility(t *testing.T) {
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",
Expand All @@ -578,20 +580,6 @@ func TestPatchCompatibility(t *testing.T) {
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",
Expand Down Expand Up @@ -641,12 +629,6 @@ func TestPatchCompatibility(t *testing.T) {
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")
}
Expand Down
8 changes: 0 additions & 8 deletions test/integration/etcd/data.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,6 @@ func GetEtcdStorageDataForNamespace(namespace string) map[schema.GroupVersionRes
},
// --

// k8s.io/kubernetes/pkg/apis/policy/v1beta1
gvr("policy", "v1beta1", "poddisruptionbudgets"): {
Stub: `{"metadata": {"name": "pdb1"}, "spec": {"selector": {"matchLabels": {"anokkey": "anokvalue"}}}}`,
ExpectedEtcdPath: "/registry/poddisruptionbudgets/" + namespace + "/pdb1",
ExpectedGVK: gvkP("policy", "v1", "PodDisruptionBudget"),
},
// --

// k8s.io/kubernetes/pkg/apis/storage/v1alpha1
gvr("storage.k8s.io", "v1alpha1", "csistoragecapacities"): {
Stub: `{"metadata": {"name": "csc-12345-1"}, "storageClassName": "sc1"}`,
Expand Down

0 comments on commit 56b59c8

Please sign in to comment.