diff --git a/plugin/pkg/admission/namespace/autoprovision/admission_test.go b/plugin/pkg/admission/namespace/autoprovision/admission_test.go index bed6a20dcba..57e444b6946 100644 --- a/plugin/pkg/admission/namespace/autoprovision/admission_test.go +++ b/plugin/pkg/admission/namespace/autoprovision/admission_test.go @@ -129,7 +129,6 @@ 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 { diff --git a/plugin/pkg/admission/persistentvolume/label/admission_test.go b/plugin/pkg/admission/persistentvolume/label/admission_test.go index 44084757531..ddf02e69e6d 100644 --- a/plugin/pkg/admission/persistentvolume/label/admission_test.go +++ b/plugin/pkg/admission/persistentvolume/label/admission_test.go @@ -77,7 +77,8 @@ func mockVolumeLabels(labels map[string]string) *mockVolumes { // TestAdmission func TestAdmission(t *testing.T) { - handler := NewPersistentVolumeLabel() + pvHandler := NewPersistentVolumeLabel() + handler := admission.NewChainHandler(admission.NewNamedHandler("pv", pvHandler)) ignoredPV := api.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"}, Spec: api.PersistentVolumeSpec{ @@ -100,8 +101,9 @@ func TestAdmission(t *testing.T) { } // Non-cloud PVs are ignored - if handler.Handles(admission.Delete) { - t.Errorf("Expected to only handle create") + err := handler.Admit(admission.NewAttributesRecord(&ignoredPV, nil, api.Kind("PersistentVolume").WithVersion("version"), ignoredPV.Namespace, ignoredPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil)) + if err != nil { + t.Errorf("Unexpected error returned from admission handler (on ignored pv): %v", err) } // We only add labels on creation @@ -111,7 +113,7 @@ func TestAdmission(t *testing.T) { } // Errors from the cloudprovider block creation of the volume - handler.ebsVolumes = mockVolumeFailure(fmt.Errorf("invalid volume")) + pvHandler.ebsVolumes = mockVolumeFailure(fmt.Errorf("invalid volume")) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil)) if err == nil { t.Errorf("Expected error when aws pv info fails") @@ -119,7 +121,7 @@ func TestAdmission(t *testing.T) { // Don't add labels if the cloudprovider doesn't return any labels := make(map[string]string) - handler.ebsVolumes = mockVolumeLabels(labels) + pvHandler.ebsVolumes = mockVolumeLabels(labels) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil)) if err != nil { t.Errorf("Expected no error when creating aws pv") @@ -129,7 +131,7 @@ func TestAdmission(t *testing.T) { } // Don't panic if the cloudprovider returns nil, nil - handler.ebsVolumes = mockVolumeFailure(nil) + pvHandler.ebsVolumes = mockVolumeFailure(nil) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil)) if err != nil { t.Errorf("Expected no error when cloud provider returns empty labels") @@ -139,7 +141,7 @@ func TestAdmission(t *testing.T) { labels = make(map[string]string) labels["a"] = "1" labels["b"] = "2" - handler.ebsVolumes = mockVolumeLabels(labels) + pvHandler.ebsVolumes = mockVolumeLabels(labels) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, nil)) if err != nil { t.Errorf("Expected no error when creating aws pv") diff --git a/staging/src/k8s.io/apiserver/pkg/admission/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/BUILD index 57e826b8e81..c6279ba7739 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/admission/BUILD @@ -21,9 +21,8 @@ go_test( deps = [ "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", "//vendor/github.com/prometheus/client_model/go:go_default_library", - "//vendor/github.com/stretchr/testify/assert:go_default_library", - "//vendor/github.com/stretchr/testify/require: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", diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain.go b/staging/src/k8s.io/apiserver/pkg/admission/chain.go index 3a0f016629a..ddb28bbd08b 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/chain.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/chain.go @@ -27,6 +27,13 @@ func NewChainHandler(handlers ...NamedHandler) chainAdmissionHandler { return chainAdmissionHandler(handlers) } +func NewNamedHandler(name string, i Interface) NamedHandler { + return &pluginHandler{ + i: i, + name: name, + } +} + const ( stepValidating = "validating" stepMutating = "mutating" @@ -34,20 +41,21 @@ const ( // Admit performs an admission control check using a chain of handlers, and returns immediately on first error func (admissionHandler chainAdmissionHandler) Admit(a Attributes) error { - var err error start := time.Now() - defer func() { - Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepMutating) - }() + err := admissionHandler.admit(a) + Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepMutating) + 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) + err := mutator.Admit(a) + Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepMutating) if err != nil { return err } @@ -58,20 +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 { - var err error start := time.Now() - defer func() { - Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidating) - }() + err := admissionHandler.validate(a) + Metrics.ObserveAdmissionStep(time.Since(start), err != nil, a, stepValidating) + 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) + err := validator.Validate(a) + Metrics.ObserveAdmissionController(time.Since(t), err != nil, handler, a, stepValidating) if err != nil { return err } @@ -83,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 } } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go index 71bdc9f31c8..868d7dcb0d3 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/chain_test.go @@ -17,46 +17,15 @@ limitations under the License. package admission import ( - "fmt" + "strconv" "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" ) -type FakeHandler struct { - *Handler - name string - admit, admitCalled bool - validate, validateCalled bool -} - -func (h *FakeHandler) Admit(a Attributes) (err error) { - h.admitCalled = true - if h.admit { - return nil - } - return fmt.Errorf("Don't admit") -} - -func (h *FakeHandler) Validate(a Attributes) (err error) { - h.validateCalled = true - if h.validate { - return nil - } - return fmt.Errorf("Don't validate") -} - -func makeHandler(name string, accept bool, ops ...Operation) Interface { - return &FakeHandler{ - name: name, - admit: accept, - validate: accept, - Handler: NewHandler(ops...), - } -} - func TestAdmitAndValidate(t *testing.T) { - sysns := "kube-system" + sysns := metav1.NamespaceSystem otherns := "default" tests := []struct { name string @@ -64,7 +33,6 @@ func TestAdmitAndValidate(t *testing.T) { operation Operation chain chainAdmissionHandler accept bool - reject bool calls map[string]bool }{ { @@ -72,9 +40,9 @@ func TestAdmitAndValidate(t *testing.T) { ns: sysns, operation: Create, chain: []NamedHandler{ - makeHandler("a", true, Update, Delete, Create), - makeHandler("b", true, Delete, Create), - makeHandler("c", true, Create), + makeNamedHandler("a", true, Update, Delete, Create), + makeNamedHandler("b", true, Delete, Create), + makeNamedHandler("c", true, Create), }, calls: map[string]bool{"a": true, "b": true, "c": true}, accept: true, @@ -84,9 +52,9 @@ func TestAdmitAndValidate(t *testing.T) { ns: otherns, operation: Create, chain: []NamedHandler{ - makeHandler("a", true, Update, Delete, Create), - makeHandler("b", false, Delete), - makeHandler("c", true, Create), + makeNamedHandler("a", true, Update, Delete, Create), + makeNamedHandler("b", false, Delete), + makeNamedHandler("c", true, Create), }, calls: map[string]bool{"a": true, "c": true}, accept: true, @@ -96,9 +64,9 @@ func TestAdmitAndValidate(t *testing.T) { ns: sysns, operation: Connect, chain: []NamedHandler{ - makeHandler("a", true, Update, Delete, Create), - makeHandler("b", false, Delete), - makeHandler("c", true, Create), + makeNamedHandler("a", true, Update, Delete, Create), + makeNamedHandler("b", false, Delete), + makeNamedHandler("c", true, Create), }, calls: map[string]bool{}, accept: true, @@ -108,77 +76,112 @@ func TestAdmitAndValidate(t *testing.T) { ns: otherns, operation: Delete, chain: []NamedHandler{ - makeHandler("a", true, Update, Delete, Create), - makeHandler("b", false, Delete), - makeHandler("c", true, Create), + makeNamedHandler("a", true, Update, Delete, Create), + makeNamedHandler("b", false, Delete), + makeNamedHandler("c", true, Create), }, calls: map[string]bool{"a": true, "b": true}, accept: false, - reject: true, }, } for _, test := range tests { Metrics.reset() t.Logf("testcase = %s", test.name) // call admit and check that validate was not called at all - err = test.chain.Admit(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "", schema.GroupVersionResource{}, "", test.operation, nil)) + err := test.chain.Admit(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, test.ns, "", schema.GroupVersionResource{}, "", test.operation, nil)) accepted := (err == nil) if accepted != test.accept { t.Errorf("unexpected result of admit call: %v", accepted) } for _, h := range test.chain { - fake := h.(*FakeHandler) - _, shouldBeCalled := test.calls[fake.name] + fake := h.Interface().(*FakeHandler) + _, shouldBeCalled := test.calls[h.Name()] if shouldBeCalled != fake.admitCalled { - t.Errorf("%s: admit handler %s not called as expected: %v", test.name, fake.name, fake.admitCalled) + t.Errorf("admit handler %s not called as expected: %v", h.Name(), fake.admitCalled) continue } if fake.validateCalled { - t.Errorf("%s: validate handler %s called during admit", test.name, fake.name) + t.Errorf("validate handler %s called during admit", h.Name()) } - // reset value for validation test + // reset value for validation test fake.admitCalled = false } + labelFilter := map[string]string{ + "is_system_ns": strconv.FormatBool(test.ns == sysns), + "type": "mutating", + } + + checkAdmitAndValidateMetrics(t, labelFilter, test.accept, test.calls) + Metrics.reset() // call validate and check that admit was not called at all - err = test.chain.Validate(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "", schema.GroupVersionResource{}, "", test.operation, nil)) + err = test.chain.Validate(NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, test.ns, "", schema.GroupVersionResource{}, "", test.operation, nil)) accepted = (err == nil) if accepted != test.accept { - t.Errorf("%s: unexpected result of validate call: %v\n", test.name, accepted) + t.Errorf("unexpected result of validate call: %v\n", accepted) } for _, h := range test.chain { - fake := h.(*FakeHandler) - _, shouldBeCalled := test.calls[fake.name] + fake := h.Interface().(*FakeHandler) + + _, shouldBeCalled := test.calls[h.Name()] if shouldBeCalled != fake.validateCalled { - t.Errorf("%s: validate handler %s not called as expected: %v", test.name, fake.name, fake.validateCalled) + t.Errorf("validate handler %s not called as expected: %v", h.Name(), fake.validateCalled) continue } if fake.admitCalled { - t.Errorf("%s: admit handler %s called during admit", test.name, fake.name) + t.Errorf("mutating handler unexpectedly called: %s", h.Name()) } } - labels := metricLabels{ - isSystemNs: test.ns == sysns, - } - if test.reject { - expectCountMetric(t, "apiserver_admission_step_rejected_total", labels, 1) + labelFilter = map[string]string{ + "is_system_ns": strconv.FormatBool(test.ns == sysns), + "type": "validating", } - if test.accept { - expectCountMetric(t, "apiserver_admission_step_total", labels, 1) - } + checkAdmitAndValidateMetrics(t, labelFilter, test.accept, test.calls) + } +} +func checkAdmitAndValidateMetrics(t *testing.T, labelFilter map[string]string, accept bool, calls map[string]bool) { + acceptFilter := map[string]string{"rejected": "false"} + for k, v := range labelFilter { + acceptFilter[k] = v + } + + rejectFilter := map[string]string{"rejected": "true"} + for k, v := range labelFilter { + rejectFilter[k] = v + } + + if accept { + // Ensure exactly one admission end-to-end admission accept should have been recorded. + expectHistogramCountTotal(t, "apiserver_admission_step_latencies", acceptFilter, 1) + + // Ensure the expected count of admission controllers have been executed. + expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", acceptFilter, len(calls)) + } else { + // When not accepted, ensure exactly one end-to-end rejection has been recorded. + expectHistogramCountTotal(t, "apiserver_admission_step_latencies", rejectFilter, 1) + if len(calls) > 0 { + if len(calls) > 1 { + // When not accepted, ensure that all but the last controller had been accepted, since + // the chain stops execution at the first rejection. + expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", acceptFilter, len(calls)-1) + } + + // When not accepted, ensure exactly one controller has been rejected. + expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", rejectFilter, 1) + } } } func TestHandles(t *testing.T) { chain := chainAdmissionHandler{ - makeHandler("a", true, Update, Delete, Create), - makeHandler("b", true, Delete, Create), - makeHandler("c", true, Create), + makeNamedHandler("a", true, Update, Delete, Create), + makeNamedHandler("b", true, Delete, Create), + makeNamedHandler("c", true, Create), } tests := []struct { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go index 91b546df71b..0f3bea661c9 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics.go @@ -34,33 +34,42 @@ const ( ) var ( - latencyBuckets = prometheus.ExponentialBuckets(10000, 2.0, 8) + latencyBuckets = prometheus.ExponentialBuckets(25000, 2.0, 7) latencySummaryMaxAge = 5 * time.Hour + // Metrics provides access to all admission metrics. Metrics = newAdmissionMetrics() ) +// NamedHandler requires each admission.Interface be named, primarly for metrics tracking purposes. +type NamedHandler interface { + Interface() Interface + Name() string +} + +// AdmissionMetrics instruments admission with prometheus metrics. type AdmissionMetrics struct { step *metricSet controller *metricSet externalWebhook *metricSet } +// newAdmissionMetrics create a new AdmissionMetrics, configured with default metric names. func newAdmissionMetrics() *AdmissionMetrics { // Admission metrics for a step of the admission flow. The entire admission flow is broken down into a series of steps // Each step is identified by a distinct type label value. - step := newMetricSet("step_", - []string{"operation", "group", "version", "resource", "subresource", "type", "is_system_ns"}, + step := newMetricSet("step", + []string{"type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"}, "Admission sub-step %s, broken out for each operation and API resource and step type (validating or mutating).") // Built-in admission controller metrics. Each admission controller is identified by name. - controller := newMetricSet("controller_", - []string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns"}, + controller := newMetricSet("controller", + []string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"}, "Admission controller %s, identified by name and broken out for each operation and API resource and type (validating or mutating).") // External admission webhook metrics. Each webhook is identified by name. - externalWebhook := newMetricSet("external_webhook_", - []string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns"}, + externalWebhook := newMetricSet("external_webhook", + []string{"name", "type", "operation", "group", "version", "resource", "subresource", "is_system_ns", "rejected"}, "External admission webhook %s, identified by name and broken out for each operation and API resource and type (validating or mutating).") step.mustRegister() @@ -75,30 +84,23 @@ func (m *AdmissionMetrics) reset() { m.externalWebhook.reset() } -// namedHandler requires each admission.Interface be named, primarly for metrics tracking purposes. -type NamedHandler interface { - Interface - GetName() string -} - // ObserveAdmissionStep records admission related metrics for a admission step, identified by step type. func (m *AdmissionMetrics) ObserveAdmissionStep(elapsed time.Duration, rejected bool, attr Attributes, stepType string) { gvr := attr.GetResource() - m.step.observe(elapsed, rejected, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), stepType, isSystemNsLabel(attr)) + m.step.observe(elapsed, stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected)) } // ObserveAdmissionController records admission related metrics for a built-in admission controller, identified by it's plugin handler name. -func (m *AdmissionMetrics) ObserveAdmissionController(elapsed time.Duration, rejected bool, handler NamedHandler, attr Attributes) { - t := typeToLabel(handler) +func (m *AdmissionMetrics) ObserveAdmissionController(elapsed time.Duration, rejected bool, handler NamedHandler, attr Attributes, stepType string) { gvr := attr.GetResource() - m.controller.observe(elapsed, rejected, handler.GetName(), t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr)) + m.controller.observe(elapsed, handler.Name(), stepType, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected)) } // ObserveExternalWebhook records admission related metrics for a external admission webhook. func (m *AdmissionMetrics) ObserveExternalWebhook(elapsed time.Duration, rejected bool, hook *v1alpha1.ExternalAdmissionHook, attr Attributes) { t := "validating" // TODO: pass in type (validating|mutating) once mutating webhook functionality has been implemented gvr := attr.GetResource() - m.externalWebhook.observe(elapsed, rejected, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr)) + m.externalWebhook.observe(elapsed, hook.Name, t, string(attr.GetOperation()), gvr.Group, gvr.Version, gvr.Resource, attr.GetSubresource(), isSystemNsLabel(attr), strconv.FormatBool(rejected)) } // isSystemNsLabel returns the value to use for the `is_system_ns` metric label. @@ -106,73 +108,18 @@ func isSystemNsLabel(a Attributes) string { return strconv.FormatBool(a.GetNamespace() == metav1.NamespaceSystem) } -func typeToLabel(i Interface) string { - switch i.(type) { - case MutationInterface: - return "mutating" - case ValidationInterface: - return "validating" - default: - return "UNRECOGNIZED_ADMISSION_TYPE" - } -} - type metricSet struct { - total *prometheus.CounterVec - rejectedTotal *prometheus.CounterVec latencies *prometheus.HistogramVec latenciesSummary *prometheus.SummaryVec } -func (m *metricSet) mustRegister() { - prometheus.MustRegister(m.total) - prometheus.MustRegister(m.rejectedTotal) - prometheus.MustRegister(m.latencies) - prometheus.MustRegister(m.latenciesSummary) -} - -func (m *metricSet) reset() { - m.total.Reset() - m.rejectedTotal.Reset() - m.latencies.Reset() - m.latenciesSummary.Reset() -} - -func (m *metricSet) observe(elapsed time.Duration, rejected bool, labels ...string) { - elapsedMicroseconds := float64(elapsed / time.Microsecond) - m.total.WithLabelValues(labels...).Inc() - if rejected { - m.rejectedTotal.WithLabelValues(labels...).Inc() - } - m.latencies.WithLabelValues(labels...).Observe(elapsedMicroseconds) - m.latenciesSummary.WithLabelValues(labels...).Observe(elapsedMicroseconds) -} - func newMetricSet(name string, labels []string, helpTemplate string) *metricSet { return &metricSet{ - total: prometheus.NewCounterVec( - prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: fmt.Sprintf("%stotal", name), - Help: fmt.Sprintf(helpTemplate, "count"), - }, - labels, - ), - rejectedTotal: prometheus.NewCounterVec( - prometheus.CounterOpts{ - Namespace: namespace, - Subsystem: subsystem, - Name: fmt.Sprintf("%srejected_total", name), - Help: fmt.Sprintf(helpTemplate, "rejected count"), - }, - labels, - ), latencies: prometheus.NewHistogramVec( prometheus.HistogramOpts{ Namespace: namespace, Subsystem: subsystem, - Name: fmt.Sprintf("%slatencies", name), + Name: fmt.Sprintf("%s_latencies", name), Help: fmt.Sprintf(helpTemplate, "latency histogram"), Buckets: latencyBuckets, }, @@ -182,7 +129,7 @@ func newMetricSet(name string, labels []string, helpTemplate string) *metricSet prometheus.SummaryOpts{ Namespace: namespace, Subsystem: subsystem, - Name: fmt.Sprintf("%slatencies_summary", name), + Name: fmt.Sprintf("%s_latencies_summary", name), Help: fmt.Sprintf(helpTemplate, "latency summary"), MaxAge: latencySummaryMaxAge, }, @@ -190,3 +137,22 @@ func newMetricSet(name string, labels []string, helpTemplate string) *metricSet ), } } + +// MustRegister registers all the prometheus metrics in the metricSet. +func (m *metricSet) mustRegister() { + prometheus.MustRegister(m.latencies) + prometheus.MustRegister(m.latenciesSummary) +} + +// Reset resets all the prometheus metrics in the metricSet. +func (m *metricSet) reset() { + m.latencies.Reset() + m.latenciesSummary.Reset() +} + +// Observe records an observed admission event to all metrics in the metricSet. +func (m *metricSet) observe(elapsed time.Duration, labels ...string) { + elapsedMicroseconds := float64(elapsed / time.Microsecond) + m.latencies.WithLabelValues(labels...).Observe(elapsedMicroseconds) + m.latenciesSummary.WithLabelValues(labels...).Observe(elapsedMicroseconds) +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go index c602169ae0f..202f7f84dea 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/metrics_test.go @@ -34,48 +34,54 @@ var ( func TestObserveAdmissionStep(t *testing.T) { Metrics.reset() Metrics.ObserveAdmissionStep(2*time.Second, false, attr, "mutating") - wantLabels := metricLabels{ - operation: string(Create), - group: resource.Group, - version: resource.Version, - resource: resource.Resource, - subresource: "subresource", - tpe: "mutating", - isSystemNs: false, + wantLabels := map[string]string{ + "operation": string(Create), + "group": resource.Group, + "version": resource.Version, + "resource": resource.Resource, + "subresource": "subresource", + "type": "mutating", + "isSystemNs": "false", + "rejected": "false", } - expectCountMetric(t, "apiserver_admission_step_total", wantLabels, 1) + expectHistogramCountTotal(t, "apiserver_admission_step_latencies", wantLabels, 1) + expectFindMetric(t, "apiserver_admission_step_latencies_summary", wantLabels) } func TestObserveAdmissionController(t *testing.T) { Metrics.reset() - handler := makeHandler("a", true, Create) - Metrics.ObserveAdmissionController(2*time.Second, false, handler, attr) - wantLabels := metricLabels{ - name: "a", - operation: string(Create), - group: resource.Group, - version: resource.Version, - resource: resource.Resource, - subresource: "subresource", - tpe: "mutating", - isSystemNs: false, + handler := makeValidatingNamedHandler("a", true, Create) + Metrics.ObserveAdmissionController(2*time.Second, false, handler, attr, "validating") + wantLabels := map[string]string{ + "name": "a", + "operation": string(Create), + "group": resource.Group, + "version": resource.Version, + "resource": resource.Resource, + "subresource": "subresource", + "type": "validating", + "isSystemNs": "false", + "rejected": "false", } - expectCountMetric(t, "apiserver_admission_controller_total", wantLabels, 1) + expectHistogramCountTotal(t, "apiserver_admission_controller_latencies", wantLabels, 1) + expectFindMetric(t, "apiserver_admission_controller_latencies_summary", wantLabels) } func TestObserveExternalWebhook(t *testing.T) { Metrics.reset() hook := &v1alpha1.ExternalAdmissionHook{Name: "x"} Metrics.ObserveExternalWebhook(2*time.Second, false, hook, attr) - wantLabels := metricLabels{ - name: "x", - operation: string(Create), - group: resource.Group, - version: resource.Version, - resource: resource.Resource, - subresource: "subresource", - tpe: "validating", - isSystemNs: false, + wantLabels := map[string]string{ + "name": "x", + "operation": string(Create), + "group": resource.Group, + "version": resource.Version, + "resource": resource.Resource, + "subresource": "subresource", + "type": "validating", + "isSystemNs": "false", + "rejected": "false", } - expectCountMetric(t, "apiserver_admission_external_webhook_total", wantLabels, 1) + expectHistogramCountTotal(t, "apiserver_admission_external_webhook_latencies", wantLabels, 1) + expectFindMetric(t, "apiserver_admission_external_webhook_latencies_summary", wantLabels) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugins.go b/staging/src/k8s.io/apiserver/pkg/admission/plugins.go index f1ee98e83a3..db1add66a63 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugins.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugins.go @@ -41,11 +41,15 @@ type Plugins struct { // pluginHandler associates name with a admission.Interface handler. type pluginHandler struct { - Interface + i Interface name string } -func (h *pluginHandler) GetName() string { +func (h *pluginHandler) Interface() Interface { + return h.i +} + +func (h *pluginHandler) Name() string { return h.name } @@ -143,7 +147,7 @@ func (ps *Plugins) NewFromPlugins(pluginNames []string, configProvider ConfigPro return nil, err } if plugin != nil { - handler := &pluginHandler{Interface: plugin, name: pluginName} + handler := &pluginHandler{i: plugin, name: pluginName} handlers = append(handlers, handler) } } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go b/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go index 25f7a988098..a2f1cc980e2 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/testutil_test.go @@ -18,24 +18,19 @@ package admission import ( "fmt" - "strconv" "testing" "github.com/prometheus/client_golang/prometheus" ptype "github.com/prometheus/client_model/go" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) +// FakeHandler provide a mock implement both MutationInterface and ValidationInterface that tracks which +// methods have been called and always returns an error if admit is false. type FakeHandler struct { *Handler - name string - admit, admitCalled bool - validate, validateCalled bool -} - -func (h *FakeHandler) GetName() string { - return h.name + admit bool + admitCalled bool + validateCalled bool } func (h *FakeHandler) Admit(a Attributes) (err error) { @@ -47,105 +42,125 @@ func (h *FakeHandler) Admit(a Attributes) (err error) { } func (h *FakeHandler) Validate(a Attributes) (err error) { - h.admitCalled = true + h.validateCalled = true if h.admit { return nil } return fmt.Errorf("Don't admit") } -// makeHandler creates a mock handler for testing purposes. -func makeHandler(name string, admit bool, ops ...Operation) *FakeHandler { +func makeHandler(admit bool, ops ...Operation) *FakeHandler { return &FakeHandler{ - name: name, admit: admit, Handler: NewHandler(ops...), } } -type metricLabels struct { - operation string - group string - version string - resource string - subresource string - name string - tpe string - isSystemNs bool +func makeNamedHandler(name string, admit bool, ops ...Operation) NamedHandler { + return &pluginHandler{ + i: &FakeHandler{ + admit: admit, + Handler: NewHandler(ops...), + }, + name: name, + } } -// matches checks if the reciever matches the pattern. Empty strings in the pattern are treated as wildcards. -func (l metricLabels) matches(pattern metricLabels) bool { - return matches(l.operation, pattern.operation) && - matches(l.group, pattern.group) && - matches(l.version, pattern.version) && - matches(l.resource, pattern.resource) && - matches(l.subresource, pattern.subresource) && - matches(l.tpe, pattern.tpe) && - l.isSystemNs == pattern.isSystemNs +// FakeValidatingHandler provide a mock of ValidationInterface that tracks which +// methods have been called and always returns an error if validate is false. +type FakeValidatingHandler struct { + *Handler + validate, validateCalled bool } -// matches checks if a string matches a "pattern" string, where an empty pattern string is treated as a wildcard. -func matches(s string, pattern string) bool { - return pattern == "" || s == pattern +func (h *FakeValidatingHandler) Validate(a Attributes) (err error) { + h.validateCalled = true + if h.validate { + return nil + } + return fmt.Errorf("Don't validate") } -// readLabels marshalls the labels from a prometheus metric type to a simple struct, producing test errors if -// if any unrecognized labels are encountered. -func readLabels(t *testing.T, metric *ptype.Metric) metricLabels { - l := metricLabels{} +func makeValidatingHandler(validate bool, ops ...Operation) *FakeValidatingHandler { + return &FakeValidatingHandler{ + validate: validate, + Handler: NewHandler(ops...), + } +} + +func makeValidatingNamedHandler(name string, validate bool, ops ...Operation) NamedHandler { + return &pluginHandler{ + i: &FakeValidatingHandler{ + validate: validate, + Handler: NewHandler(ops...), + }, + name: name, + } +} + +func labelsMatch(metric *ptype.Metric, labelFilter map[string]string) bool { for _, lp := range metric.GetLabel() { - val := lp.GetValue() - switch lp.GetName() { - case "operation": - l.operation = val - case "group": - l.group = val - case "version": - l.version = val - case "resource": - l.resource = val - case "subresource": - l.subresource = val - case "name": - l.name = val - case "type": - l.tpe = val - case "is_system_ns": - ns, err := strconv.ParseBool(lp.GetValue()) - if err != nil { - t.Errorf("Expected boole for is_system_ns label value, got %s", lp.GetValue()) - } else { - l.isSystemNs = ns - } - default: - t.Errorf("Unexpected metric label %s", lp.GetName()) + if value, ok := labelFilter[lp.GetName()]; ok && lp.GetValue() != value { + return false } } - return l + return true } -// expectCounterMetric ensures that exactly one counter metric with the given name and patternLabels exists and has -// the provided count. -func expectCountMetric(t *testing.T, name string, patternLabels metricLabels, wantCount int64) { +// expectFindMetric find a metric with the given name nad labels or reports a fatal test error. +func expectFindMetric(t *testing.T, name string, expectedLabels map[string]string) *ptype.Metric { metrics, err := prometheus.DefaultGatherer.Gather() - require.NoError(t, err) + if err != nil { + t.Fatalf("Failed to gather metrics: %s", err) + } - count := 0 + for _, mf := range metrics { + if mf.GetName() == name { + for _, metric := range mf.GetMetric() { + if labelsMatch(metric, expectedLabels) { + gotLabelCount := len(metric.GetLabel()) + wantLabelCount := len(expectedLabels) + if wantLabelCount != gotLabelCount { + t.Errorf("Got metric with %d labels, but wanted %d labels. Wanted %#+v for %s", + gotLabelCount, wantLabelCount, expectedLabels, metric.String()) + } + return metric + } + } + } + } + t.Fatalf("No metric found with name %s and labels %#+v", name, expectedLabels) + return nil +} + +// expectHistogramCountTotal ensures that the sum of counts of metrics matching the labelFilter is as +// expected. +func expectHistogramCountTotal(t *testing.T, name string, labelFilter map[string]string, wantCount int) { + metrics, err := prometheus.DefaultGatherer.Gather() + if err != nil { + t.Fatalf("Failed to gather metrics: %s", err) + } + + counterSum := 0 for _, mf := range metrics { if mf.GetName() != name { continue // Ignore other metrics. } for _, metric := range mf.GetMetric() { - gotLabels := readLabels(t, metric) - if !gotLabels.matches(patternLabels) { + if !labelsMatch(metric, labelFilter) { continue } - count += 1 - assert.EqualValues(t, wantCount, metric.GetCounter().GetValue()) + counterSum += int(metric.GetHistogram().GetSampleCount()) } } - if count != 1 { - t.Errorf("Want 1 metric with name %s, got %d", name, count) + if wantCount != counterSum { + t.Errorf("Wanted count %d, got %d for metric %s with labels %#+v", wantCount, counterSum, name, labelFilter) + for _, mf := range metrics { + if mf.GetName() == name { + for _, metric := range mf.GetMetric() { + t.Logf("\tnear match: %s", metric.String()) + } + } + } } }