From 6b8e2cc24e407dca6557a43c7a3abee208612244 Mon Sep 17 00:00:00 2001 From: Jesus Herrera Date: Wed, 20 May 2020 20:29:51 -0400 Subject: [PATCH 1/2] Fix runtime admission flaky test due to race condition --- plugin/pkg/admission/runtimeclass/BUILD | 3 + .../pkg/admission/runtimeclass/admission.go | 24 ++- .../admission/runtimeclass/admission_test.go | 153 ++++++++++++++++++ 3 files changed, 176 insertions(+), 4 deletions(-) diff --git a/plugin/pkg/admission/runtimeclass/BUILD b/plugin/pkg/admission/runtimeclass/BUILD index 04b2d5221eb..f049e46bf20 100644 --- a/plugin/pkg/admission/runtimeclass/BUILD +++ b/plugin/pkg/admission/runtimeclass/BUILD @@ -14,9 +14,12 @@ go_library( "//staging/src/k8s.io/api/node/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/initializer:go_default_library", "//staging/src/k8s.io/client-go/informers:go_default_library", + "//staging/src/k8s.io/client-go/kubernetes:go_default_library", + "//staging/src/k8s.io/client-go/kubernetes/typed/node/v1beta1:go_default_library", "//staging/src/k8s.io/client-go/listers/node/v1beta1:go_default_library", "//staging/src/k8s.io/component-base/featuregate:go_default_library", ], diff --git a/plugin/pkg/admission/runtimeclass/admission.go b/plugin/pkg/admission/runtimeclass/admission.go index 4af7e826683..f88742ad59a 100644 --- a/plugin/pkg/admission/runtimeclass/admission.go +++ b/plugin/pkg/admission/runtimeclass/admission.go @@ -29,9 +29,12 @@ import ( v1beta1 "k8s.io/api/node/v1beta1" apiequality "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/admission" genericadmissioninitailizer "k8s.io/apiserver/pkg/admission/initializer" "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" + nodev1beta1client "k8s.io/client-go/kubernetes/typed/node/v1beta1" nodev1beta1listers "k8s.io/client-go/listers/node/v1beta1" "k8s.io/component-base/featuregate" api "k8s.io/kubernetes/pkg/apis/core" @@ -58,6 +61,7 @@ func Register(plugins *admission.Plugins) { type RuntimeClass struct { *admission.Handler runtimeClassLister nodev1beta1listers.RuntimeClassLister + runtimeClassClient nodev1beta1client.RuntimeClassInterface inspectedFeatures bool runtimeClassEnabled bool @@ -68,6 +72,11 @@ var _ admission.MutationInterface = &RuntimeClass{} var _ admission.ValidationInterface = &RuntimeClass{} var _ genericadmissioninitailizer.WantsExternalKubeInformerFactory = &RuntimeClass{} +var _ genericadmissioninitailizer.WantsExternalKubeClientSet = &RuntimeClass{} + +func (r *RuntimeClass) SetExternalKubeClientSet(client kubernetes.Interface) { + r.runtimeClassClient = client.NodeV1beta1().RuntimeClasses() +} // InspectFeatureGates allows setting bools without taking a dep on a global variable func (r *RuntimeClass) InspectFeatureGates(featureGates featuregate.FeatureGate) { @@ -97,6 +106,9 @@ func (r *RuntimeClass) ValidateInitialization() error { if r.runtimeClassLister == nil { return fmt.Errorf("missing RuntimeClass lister") } + if r.runtimeClassClient == nil { + return fmt.Errorf("missing RuntimeClass client") + } return nil } @@ -111,7 +123,7 @@ func (r *RuntimeClass) Admit(ctx context.Context, attributes admission.Attribute return nil } - pod, runtimeClass, err := r.prepareObjects(attributes) + pod, runtimeClass, err := r.prepareObjects(ctx, attributes) if err != nil { return err } @@ -139,7 +151,7 @@ func (r *RuntimeClass) Validate(ctx context.Context, attributes admission.Attrib return nil } - pod, runtimeClass, err := r.prepareObjects(attributes) + pod, runtimeClass, err := r.prepareObjects(ctx, attributes) if err != nil { return err } @@ -160,7 +172,7 @@ func NewRuntimeClass() *RuntimeClass { } // prepareObjects returns pod and runtimeClass types from the given admission attributes -func (r *RuntimeClass) prepareObjects(attributes admission.Attributes) (pod *api.Pod, runtimeClass *v1beta1.RuntimeClass, err error) { +func (r *RuntimeClass) prepareObjects(ctx context.Context, attributes admission.Attributes) (pod *api.Pod, runtimeClass *v1beta1.RuntimeClass, err error) { pod, ok := attributes.GetObject().(*api.Pod) if !ok { return nil, nil, apierrors.NewBadRequest("Resource was marked with kind Pod but was unable to be converted") @@ -173,7 +185,11 @@ func (r *RuntimeClass) prepareObjects(attributes admission.Attributes) (pod *api // get RuntimeClass object runtimeClass, err = r.runtimeClassLister.Get(*pod.Spec.RuntimeClassName) if apierrors.IsNotFound(err) { - return pod, nil, admission.NewForbidden(attributes, fmt.Errorf("pod rejected: RuntimeClass %q not found", *pod.Spec.RuntimeClassName)) + // if not found, our informer cache could be lagging, do a live lookup + runtimeClass, err = r.runtimeClassClient.Get(ctx, *pod.Spec.RuntimeClassName, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return pod, nil, admission.NewForbidden(attributes, fmt.Errorf("pod rejected: RuntimeClass %q not found", *pod.Spec.RuntimeClassName)) + } } // return the pod and runtimeClass. diff --git a/plugin/pkg/admission/runtimeclass/admission_test.go b/plugin/pkg/admission/runtimeclass/admission_test.go index c5552ee332e..6e299fcb156 100644 --- a/plugin/pkg/admission/runtimeclass/admission_test.go +++ b/plugin/pkg/admission/runtimeclass/admission_test.go @@ -1,3 +1,4 @@ + /* Copyright 2019 The Kubernetes Authors. @@ -30,6 +31,13 @@ import ( "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/client-go/kubernetes/fake" + api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" + "k8s.io/component-base/featuregate" + "k8s.io/kubernetes/pkg/controller" + "k8s.io/kubernetes/pkg/features" "github.com/stretchr/testify/assert" ) @@ -315,6 +323,151 @@ func NewObjectInterfacesForTest() admission.ObjectInterfaces { return admission.NewObjectInterfacesFromScheme(scheme) } +func newRuntimeClassForTest(runtimeClassEnabled bool, + featureInspection bool, + addLister bool, + listerObject *v1beta1.RuntimeClass, + addClient bool, + clientObject *v1beta1.RuntimeClass) *RuntimeClass { + runtimeClass := NewRuntimeClass() + + if featureInspection { + relevantFeatures := map[featuregate.Feature]featuregate.FeatureSpec{ + features.RuntimeClass: {Default: runtimeClassEnabled}, + features.PodOverhead: {Default: false}, + } + fg := featuregate.NewFeatureGate() + fg.Add(relevantFeatures) + runtimeClass.InspectFeatureGates(fg) + } + + if addLister { + informerFactory := informers.NewSharedInformerFactory(nil, controller.NoResyncPeriodFunc()) + runtimeClass.SetExternalKubeInformerFactory(informerFactory) + if listerObject != nil { + informerFactory.Node().V1beta1().RuntimeClasses().Informer().GetStore().Add(listerObject) + } + } + + if addClient { + var client kubernetes.Interface + if clientObject != nil { + client = fake.NewSimpleClientset(clientObject) + } else { + client = fake.NewSimpleClientset() + } + runtimeClass.SetExternalKubeClientSet(client) + } + + return runtimeClass +} + +func TestValidateInitialization(t *testing.T) { + tests := []struct { + name string + expectError bool + runtimeClass *RuntimeClass + }{ + { + name: "runtimeClass disabled, success", + expectError: false, + runtimeClass: newRuntimeClassForTest(false, true, true, nil, true, nil), + }, + { + name: "runtimeClass enabled, success", + expectError: false, + runtimeClass: newRuntimeClassForTest(true, true, true, nil, true, nil), + }, + { + name: "runtimeClass enabled, no feature inspection", + expectError: true, + runtimeClass: newRuntimeClassForTest(true, false, true, nil, true, nil), + }, + { + name: "runtimeClass enabled, no lister", + expectError: true, + runtimeClass: newRuntimeClassForTest(true, true, false, nil, true, nil,), + }, + { + name: "runtimeClass enabled, no client", + expectError: true, + runtimeClass: newRuntimeClassForTest(true, true, true, nil, false, nil), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := tc.runtimeClass.ValidateInitialization() + if tc.expectError { + assert.NotEmpty(t, err) + } else { + assert.Empty(t, err) + } + }) + } +} + +func TestAdmit(t *testing.T) { + runtimeClassName := "runtimeClassName" + + rc := &v1beta1.RuntimeClass{ + ObjectMeta: metav1.ObjectMeta{Name: runtimeClassName}, + } + + pod := api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "podname"}, + Spec: api.PodSpec{ + RuntimeClassName: &runtimeClassName, + }, + } + + attributes := admission.NewAttributesRecord(&pod, + nil, + api.Kind("kind").WithVersion("version"), + "", + "", + api.Resource("pods").WithVersion("version"), + "", + admission.Create, + nil, + false, + nil) + + + tests := []struct { + name string + expectError bool + runtimeClass *RuntimeClass + }{ + { + name: "runtimeClass found by lister", + expectError: false, + runtimeClass: newRuntimeClassForTest(true, true, true, rc, true, nil), + }, + { + name: "runtimeClass found by client", + expectError: false, + runtimeClass: newRuntimeClassForTest(true, true, true, nil, true, rc), + }, + { + name: "runtimeClass not found by lister nor client", + expectError: true, + runtimeClass: newRuntimeClassForTest(true, true, true, nil, true, nil), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := tc.runtimeClass.Admit(context.TODO(), attributes, nil) + if tc.expectError { + assert.NotEmpty(t, err) + } else { + assert.Empty(t, err) + } + }) + } +} + func TestValidate(t *testing.T) { tests := []struct { name string From a5800ab4cbbe39b949d75e21cec03a04c9a2ed27 Mon Sep 17 00:00:00 2001 From: Jesus Herrera Date: Thu, 21 May 2020 23:06:56 -0400 Subject: [PATCH 2/2] Fix linter and bazel errors --- plugin/pkg/admission/runtimeclass/BUILD | 6 ++++++ plugin/pkg/admission/runtimeclass/admission.go | 1 + plugin/pkg/admission/runtimeclass/admission_test.go | 12 +++++------- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/plugin/pkg/admission/runtimeclass/BUILD b/plugin/pkg/admission/runtimeclass/BUILD index f049e46bf20..e8932e2f18d 100644 --- a/plugin/pkg/admission/runtimeclass/BUILD +++ b/plugin/pkg/admission/runtimeclass/BUILD @@ -31,6 +31,8 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/apis/core:go_default_library", + "//pkg/controller:go_default_library", + "//pkg/features:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/api/node/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", @@ -38,6 +40,10 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library", + "//staging/src/k8s.io/client-go/informers:go_default_library", + "//staging/src/k8s.io/client-go/kubernetes:go_default_library", + "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library", + "//staging/src/k8s.io/component-base/featuregate:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", ], ) diff --git a/plugin/pkg/admission/runtimeclass/admission.go b/plugin/pkg/admission/runtimeclass/admission.go index f88742ad59a..5b087082509 100644 --- a/plugin/pkg/admission/runtimeclass/admission.go +++ b/plugin/pkg/admission/runtimeclass/admission.go @@ -74,6 +74,7 @@ var _ admission.ValidationInterface = &RuntimeClass{} var _ genericadmissioninitailizer.WantsExternalKubeInformerFactory = &RuntimeClass{} var _ genericadmissioninitailizer.WantsExternalKubeClientSet = &RuntimeClass{} +// SetExternalKubeClientSet sets the client for the plugin func (r *RuntimeClass) SetExternalKubeClientSet(client kubernetes.Interface) { r.runtimeClassClient = client.NodeV1beta1().RuntimeClasses() } diff --git a/plugin/pkg/admission/runtimeclass/admission_test.go b/plugin/pkg/admission/runtimeclass/admission_test.go index 6e299fcb156..70d7085f013 100644 --- a/plugin/pkg/admission/runtimeclass/admission_test.go +++ b/plugin/pkg/admission/runtimeclass/admission_test.go @@ -1,4 +1,3 @@ - /* Copyright 2019 The Kubernetes Authors. @@ -30,12 +29,12 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/authentication/user" - "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/client-go/kubernetes/fake" - api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" "k8s.io/component-base/featuregate" + "k8s.io/kubernetes/pkg/apis/core" + api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/controller" "k8s.io/kubernetes/pkg/features" @@ -352,7 +351,7 @@ func newRuntimeClassForTest(runtimeClassEnabled bool, if addClient { var client kubernetes.Interface if clientObject != nil { - client = fake.NewSimpleClientset(clientObject) + client = fake.NewSimpleClientset(clientObject) } else { client = fake.NewSimpleClientset() } @@ -386,7 +385,7 @@ func TestValidateInitialization(t *testing.T) { { name: "runtimeClass enabled, no lister", expectError: true, - runtimeClass: newRuntimeClassForTest(true, true, false, nil, true, nil,), + runtimeClass: newRuntimeClassForTest(true, true, false, nil, true, nil), }, { name: "runtimeClass enabled, no client", @@ -433,7 +432,6 @@ func TestAdmit(t *testing.T) { false, nil) - tests := []struct { name string expectError bool