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

Add admission metrics for webhooks #55183

Merged
merged 5 commits into from Nov 14, 2017
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
Expand Up @@ -129,19 +129,15 @@ func TestAdmissionNamespaceExists(t *testing.T) {

// TestIgnoreAdmission validates that a request is ignored if its not a create
func TestIgnoreAdmission(t *testing.T) {
namespace := "test"
mockClient := newMockClientForTest([]string{})
handler, informerFactory, err := newHandlerForTest(mockClient)
if err != nil {
t.Errorf("unexpected error initializing handler: %v", err)
}
informerFactory.Start(wait.NeverStop)
chainHandler := admission.NewChainHandler(handler)

pod := newPod(namespace)
err = chainHandler.Admit(admission.NewAttributesRecord(&pod, nil, api.Kind("Pod").WithVersion("version"), pod.Namespace, pod.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
if err != nil {
t.Errorf("unexpected error returned from admission handler")
if handler.Handles(admission.Update) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jpbetz This check isn't the same as the Admit call that was previously done. You need a followup to restore the test.

t.Errorf("expected not to handle Update")
}
if hasCreateNamespaceAction(mockClient) {
t.Errorf("unexpected create namespace action")
Expand Down
Expand Up @@ -78,7 +78,7 @@ func mockVolumeLabels(labels map[string]string) *mockVolumes {
// TestAdmission
func TestAdmission(t *testing.T) {
pvHandler := NewPersistentVolumeLabel()
handler := admission.NewChainHandler(pvHandler)
handler := admission.NewChainHandler(admission.NewNamedHandler("pv", pvHandler))
ignoredPV := api.PersistentVolume{
ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"},
Spec: api.PersistentVolumeSpec{
Expand Down
11 changes: 4 additions & 7 deletions plugin/pkg/admission/serviceaccount/admission_test.go
Expand Up @@ -35,13 +35,10 @@ import (
)

func TestIgnoresNonCreate(t *testing.T) {
pod := &api.Pod{}
for _, op := range []admission.Operation{admission.Delete, admission.Connect} {
attrs := admission.NewAttributesRecord(pod, nil, api.Kind("Pod").WithVersion("version"), "myns", "myname", api.Resource("pods").WithVersion("version"), "", op, nil)
handler := admission.NewChainHandler(NewServiceAccount())
err := handler.Admit(attrs)
if err != nil {
t.Errorf("Expected %s operation allowed, got err: %v", op, err)
handler := NewServiceAccount()
if handler.Handles(op) {
t.Errorf("Expected not to handle operation %s", op)
}
}
}
Expand All @@ -50,7 +47,7 @@ func TestIgnoresUpdateOfInitializedPod(t *testing.T) {
pod := &api.Pod{}
oldPod := &api.Pod{}
attrs := admission.NewAttributesRecord(pod, oldPod, api.Kind("Pod").WithVersion("version"), "myns", "myname", api.Resource("pods").WithVersion("version"), "", admission.Update, nil)
handler := admission.NewChainHandler(NewServiceAccount())
handler := NewServiceAccount()
err := handler.Admit(attrs)
if err != nil {
t.Errorf("Expected update of initialized pod allowed, got err: %v", err)
Expand Down
9 changes: 9 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/admission/BUILD
Expand Up @@ -13,10 +13,16 @@ go_test(
"config_test.go",
"errors_test.go",
"handler_test.go",
"metrics_test.go",
"testutil_test.go",
],
importpath = "k8s.io/apiserver/pkg/admission",
library = ":go_default_library",
deps = [
"//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
"//vendor/github.com/prometheus/client_model/go:go_default_library",
"//vendor/k8s.io/api/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/runtime/schema:go_default_library",
"//vendor/k8s.io/apiserver/pkg/apis/apiserver:go_default_library",
Expand All @@ -32,12 +38,15 @@ go_library(
"errors.go",
"handler.go",
"interfaces.go",
"metrics.go",
"plugins.go",
],
importpath = "k8s.io/apiserver/pkg/admission",
deps = [
"//vendor/github.com/ghodss/yaml:go_default_library",
"//vendor/github.com/golang/glog:go_default_library",
"//vendor/github.com/prometheus/client_golang/prometheus:go_default_library",
"//vendor/k8s.io/api/admissionregistration/v1alpha1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apimachinery/announced:go_default_library",
Expand Down
49 changes: 41 additions & 8 deletions staging/src/k8s.io/apiserver/pkg/admission/chain.go
Expand Up @@ -16,22 +16,46 @@ limitations under the License.

package admission

// chainAdmissionHandler is an instance of admission.Interface that performs admission control using a chain of admission handlers
type chainAdmissionHandler []Interface
import "time"

// chainAdmissionHandler is an instance of admission.NamedHandler that performs admission control using
// a chain of admission handlers
type chainAdmissionHandler []NamedHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

@jpbetz This type is a union type of Interfaces to produce a union Interface. This change breaks the composition model of having a union type that unions the types it is exposing. Please make a followup that restores the composition model.

You can do this by creating an Interface wrapper which provides both Admit and Validate, and does type casting on the delegate. That will preserve the abstractions and allows layering.


// NewChainHandler creates a new chain handler from an array of handlers. Used for testing.
func NewChainHandler(handlers ...Interface) chainAdmissionHandler {
func NewChainHandler(handlers ...NamedHandler) chainAdmissionHandler {
Copy link
Member

Choose a reason for hiding this comment

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

I think you'll need to update a couple unit tests:

chainHandler := admission.NewChainHandler(handler)

handler := admission.NewChainHandler(pvHandler)

(See https://github.com/kubernetes/kubernetes/pull/55086/files#diff-97778d709e3ef8a841d3749d2c16072a)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @tallclair! I've fixed the tests, making heavy use of the work you did for #55086.

return chainAdmissionHandler(handlers)
}

func NewNamedHandler(name string, i Interface) NamedHandler {
return &pluginHandler{
i: i,
name: name,
}
}

const (
stepValidate = "validate"
stepAdmit = "admit"
)

// Admit performs an admission control check using a chain of handlers, and returns immediately on first error
func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {
start := time.Now()
err := admissionHandler.admit(a)
Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepAdmit)
return err
}

func (admissionHandler chainAdmissionHandler) admit(a Attributes) error {
for _, handler := range admissionHandler {
if !handler.Handles(a.GetOperation()) {
if !handler.Interface().Handles(a.GetOperation()) {
continue
}
if mutator, ok := handler.(MutationInterface); ok {
if mutator, ok := handler.Interface().(MutationInterface); ok {
t := time.Now()
err := mutator.Admit(a)
Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepAdmit)
if err != nil {
return err
}
Expand All @@ -42,12 +66,21 @@ func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error {

// Validate performs an admission control check using a chain of handlers, and returns immediately on first error
func (admissionHandler chainAdmissionHandler) Validate(a Attributes) error {
start := time.Now()
err := admissionHandler.validate(a)
Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidate)
return err
}

func (admissionHandler chainAdmissionHandler) validate(a Attributes) (err error) {
for _, handler := range admissionHandler {
if !handler.Handles(a.GetOperation()) {
if !handler.Interface().Handles(a.GetOperation()) {
continue
}
if validator, ok := handler.(ValidationInterface); ok {
if validator, ok := handler.Interface().(ValidationInterface); ok {
t := time.Now()
err := validator.Validate(a)
Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepValidate)
if err != nil {
return err
}
Expand All @@ -59,7 +92,7 @@ func (admissionHandler chainAdmissionHandler) Validate(a Attributes) error {
// Handles will return true if any of the handlers handles the given operation
func (admissionHandler chainAdmissionHandler) Handles(operation Operation) bool {
for _, handler := range admissionHandler {
if handler.Handles(operation) {
if handler.Interface().Handles(operation) {
return true
}
}
Expand Down