Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the generic webhook admission controller use the dynamic webhook config manager #46808

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions pkg/kubeapiserver/admission/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ go_test(
library = ":go_default_library",
tags = ["automanaged"],
deps = [
"//pkg/apis/admissionregistration:go_default_library",
"//vendor/k8s.io/apiserver/pkg/admission:go_default_library",
"//vendor/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
],
Expand All @@ -25,7 +24,6 @@ go_library(
srcs = ["initializer.go"],
tags = ["automanaged"],
deps = [
"//pkg/apis/admissionregistration:go_default_library",
"//pkg/client/clientset_generated/clientset:go_default_library",
"//pkg/client/clientset_generated/internalclientset:go_default_library",
"//pkg/client/informers/informers_generated/internalversion:go_default_library",
Expand Down
7 changes: 6 additions & 1 deletion pkg/kubeapiserver/admission/configuration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,17 @@ load(

go_test(
name = "go_default_test",
srcs = ["initializer_manager_test.go"],
srcs = [
"configuration_manager_test.go",
"initializer_manager_test.go",
],
library = ":go_default_library",
tags = ["automanaged"],
deps = [
"//pkg/apis/admissionregistration/v1alpha1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
],
)

Expand Down
73 changes: 59 additions & 14 deletions pkg/kubeapiserver/admission/configuration/configuration_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ import (
)

const (
defaultInterval = 1 * time.Second
defaultFailureThreshold = 5
defaultInterval = 1 * time.Second
defaultFailureThreshold = 5
defaultBootstrapRetries = 5
defaultBootstrapGraceperiod = 5 * time.Second
)

var (
Expand All @@ -47,26 +49,43 @@ type poller struct {
// a function to consistently read the latest configuration
get getFunc
// consistent read interval
// read-only
interval time.Duration
// if the number of consecutive read failure equals or exceeds the failureThreshold , the
// configuration is regarded as not ready.
// read-only
failureThreshold int
// number of consecutive failures so far.
failures int
// If the poller has passed the bootstrap phase. The poller is considered
// bootstrapped either bootstrapGracePeriod after the first call of
// configuration(), or when setConfigurationAndReady() is called, whichever
// comes first.
bootstrapped bool
// configuration() retries bootstrapRetries times if poller is not bootstrapped
// read-only
bootstrapRetries int
// Grace period for bootstrapping
// read-only
bootstrapGracePeriod time.Duration
once sync.Once
// if the configuration is regarded as ready.
ready bool
mergedConfiguration runtime.Object
// lock much be hold when reading ready or mergedConfiguration
lock sync.RWMutex
lastErr error
lastErr error
// lock must be hold when reading/writing the data fields of poller.
lock sync.RWMutex
}

func newPoller(get getFunc) *poller {
return &poller{
get: get,
interval: defaultInterval,
failureThreshold: defaultFailureThreshold,
p := poller{
get: get,
interval: defaultInterval,
failureThreshold: defaultFailureThreshold,
bootstrapRetries: defaultBootstrapRetries,
bootstrapGracePeriod: defaultBootstrapGraceperiod,
}
return &p
}

func (a *poller) lastError(err error) {
Expand All @@ -81,21 +100,47 @@ func (a *poller) notReady() {
a.ready = false
}

func (a *poller) bootstrapping() {
// bootstrapGracePeriod is read-only, so no lock is required
timer := time.NewTimer(a.bootstrapGracePeriod)
go func() {
<-timer.C
a.lock.Lock()
defer a.lock.Unlock()
a.bootstrapped = true
}()
}

// If the poller is not bootstrapped yet, the configuration() gets a few chances
// to retry. This hides transient failures during system startup.
func (a *poller) configuration() (runtime.Object, error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton how's this version? (ignore the unit tests, i'll update them if the implementation is right)

Also, in your previous comments, when you said "smeared out", do you mean "spread out"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton could you take another look? If bootstrap==false at the time configuration() is called, configuration() retries 5 times. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks ok to me

a.once.Do(a.bootstrapping)
a.lock.RLock()
defer a.lock.RUnlock()
if !a.ready {
if a.lastErr != nil {
return nil, a.lastErr
retries := 1
if !a.bootstrapped {
retries = a.bootstrapRetries
}
for count := 0; count < retries; count++ {
if count > 0 {
a.lock.RUnlock()
time.Sleep(a.interval)
a.lock.RLock()
}
return nil, ErrNotReady
if a.ready {
return a.mergedConfiguration, nil
}
}
if a.lastErr != nil {
return nil, a.lastErr
}
return a.mergedConfiguration, nil
return nil, ErrNotReady
}

func (a *poller) setConfigurationAndReady(value runtime.Object) {
a.lock.Lock()
defer a.lock.Unlock()
a.bootstrapped = true
a.mergedConfiguration = value
a.ready = true
a.lastErr = nil
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
Copyright 2017 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package configuration

import (
"fmt"
"math"
"sync"
"testing"
"time"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/wait"
)

func TestTolerateBootstrapFailure(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good

var fakeGetSucceed bool
var fakeGetSucceedLock sync.RWMutex
fakeGetFn := func() (runtime.Object, error) {
fakeGetSucceedLock.RLock()
defer fakeGetSucceedLock.RUnlock()
if fakeGetSucceed {
return nil, nil
} else {
return nil, fmt.Errorf("this error shouldn't be exposed to caller")
}
}
poller := newPoller(fakeGetFn)
poller.bootstrapGracePeriod = 100 * time.Second
poller.bootstrapRetries = math.MaxInt32
// set failureThreshold to 0 so that one single failure will set "ready" to false.
poller.failureThreshold = 0
stopCh := make(chan struct{})
defer close(stopCh)
go poller.Run(stopCh)
go func() {
// The test might have false negative, but won't be flaky
timer := time.NewTimer(2 * time.Second)
<-timer.C
fakeGetSucceedLock.Lock()
defer fakeGetSucceedLock.Unlock()
fakeGetSucceed = true
}()

done := make(chan struct{})
go func(t *testing.T) {
_, err := poller.configuration()
if err != nil {
t.Errorf("unexpected error: %v", err)
}
close(done)
}(t)
<-done
}

func TestNotTolerateNonbootstrapFailure(t *testing.T) {
fakeGetFn := func() (runtime.Object, error) {
return nil, fmt.Errorf("this error should be exposed to caller")
}
poller := newPoller(fakeGetFn)
poller.bootstrapGracePeriod = 1 * time.Second
poller.interval = 1 * time.Millisecond
stopCh := make(chan struct{})
defer close(stopCh)
go poller.Run(stopCh)
// to kick the bootstrap timer
go poller.configuration()

wait.PollInfinite(1*time.Second, func() (bool, error) {
poller.lock.Lock()
defer poller.lock.Unlock()
return poller.bootstrapped, nil
})

_, err := poller.configuration()
if err == nil {
t.Errorf("unexpected no error")
}
}
24 changes: 0 additions & 24 deletions pkg/kubeapiserver/admission/init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (

"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/authorization/authorizer"
"k8s.io/kubernetes/pkg/apis/admissionregistration"
)

// TestAuthorizer is a testing struct for testing that fulfills the authorizer interface.
Expand Down Expand Up @@ -123,26 +122,3 @@ func TestWantsClientCert(t *testing.T) {
t.Errorf("plumbing fail - %v %v", ccw.gotCert, ccw.gotKey)
}
}

type fakeHookSource struct{}

func (f *fakeHookSource) List() ([]admissionregistration.ExternalAdmissionHook, error) {
return nil, nil
}

type hookSourceWanter struct {
doNothingAdmission
got WebhookSource
}

func (s *hookSourceWanter) SetWebhookSource(w WebhookSource) { s.got = w }

func TestWantsWebhookSource(t *testing.T) {
hsw := &hookSourceWanter{}
fhs := &fakeHookSource{}
i := &PluginInitializer{}
i.SetWebhookSource(fhs).Initialize(hsw)
if got, ok := hsw.got.(*fakeHookSource); !ok || got != fhs {
t.Errorf("plumbing fail - %v %v#", ok, got)
}
}
27 changes: 0 additions & 27 deletions pkg/kubeapiserver/admission/initializer.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/authorization/authorizer"
"k8s.io/kubernetes/pkg/apis/admissionregistration"
"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion"
Expand Down Expand Up @@ -83,23 +82,12 @@ type WantsClientCert interface {
SetClientCert(cert, key []byte)
}

// WantsWebhookSource defines a function that accepts a webhook lister for the
// dynamic webhook plugin.
type WantsWebhookSource interface {
SetWebhookSource(WebhookSource)
}

// ServiceResolver knows how to convert a service reference into an actual
// location.
type ServiceResolver interface {
ResolveEndpoint(namespace, name string) (*url.URL, error)
}

// WebhookSource can list dynamic webhook plugins.
type WebhookSource interface {
List() ([]admissionregistration.ExternalAdmissionHook, error)
}

type PluginInitializer struct {
internalClient internalclientset.Interface
externalClient clientset.Interface
Expand All @@ -109,7 +97,6 @@ type PluginInitializer struct {
restMapper meta.RESTMapper
quotaRegistry quota.Registry
serviceResolver ServiceResolver
webhookSource WebhookSource

// for proving we are apiserver in call-outs
clientCert []byte
Expand Down Expand Up @@ -155,13 +142,6 @@ func (i *PluginInitializer) SetClientCert(cert, key []byte) *PluginInitializer {
return i
}

// SetWebhookSource sets the webhook source-- admittedly this is probably
// specific to the external admission hook plugin.
func (i *PluginInitializer) SetWebhookSource(w WebhookSource) *PluginInitializer {
i.webhookSource = w
return i
}

// Initialize checks the initialization interfaces implemented by each plugin
// and provide the appropriate initialization data
func (i *PluginInitializer) Initialize(plugin admission.Interface) {
Expand Down Expand Up @@ -206,11 +186,4 @@ func (i *PluginInitializer) Initialize(plugin admission.Interface) {
}
wants.SetClientCert(i.clientCert, i.clientKey)
}

if wants, ok := plugin.(WantsWebhookSource); ok {
if i.webhookSource == nil {
panic("An admission plugin wants a webhook source, but it was not provided.")
}
wants.SetWebhookSource(i.webhookSource)
}
}
8 changes: 6 additions & 2 deletions plugin/pkg/admission/webhook/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ go_test(
"//pkg/api:go_default_library",
"//pkg/apis/admission/install:go_default_library",
"//pkg/apis/admission/v1alpha1:go_default_library",
"//pkg/apis/admissionregistration:go_default_library",
"//pkg/apis/admissionregistration/v1alpha1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apiserver/pkg/admission:go_default_library",
Expand All @@ -41,14 +41,18 @@ go_library(
"//pkg/api:go_default_library",
"//pkg/apis/admission/install:go_default_library",
"//pkg/apis/admission/v1alpha1:go_default_library",
"//pkg/apis/admissionregistration:go_default_library",
"//pkg/apis/admissionregistration/v1alpha1:go_default_library",
"//pkg/client/clientset_generated/clientset:go_default_library",
"//pkg/kubeapiserver/admission:go_default_library",
"//pkg/kubeapiserver/admission/configuration:go_default_library",
"//vendor/github.com/golang/glog:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
"//vendor/k8s.io/apiserver/pkg/admission:go_default_library",
"//vendor/k8s.io/client-go/rest:go_default_library",
],
Expand Down