From 094eb614fe5554d3747123463fc46e890789e28b Mon Sep 17 00:00:00 2001 From: Mehdy Bohlool Date: Thu, 7 Mar 2019 08:43:05 -0800 Subject: [PATCH 1/3] Refactor CR converter code to share code between nopConverter and webhookConverter --- .../pkg/apiserver/conversion/converter.go | 96 ++++++--- .../apiserver/conversion/converter_test.go | 195 ++++++++++++++++++ .../pkg/apiserver/conversion/nop_converter.go | 56 +---- .../apiserver/conversion/webhook_converter.go | 81 +------- 4 files changed, 276 insertions(+), 152 deletions(-) create mode 100644 staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go index 9345e9bd27b..1970b33c3c7 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go @@ -55,35 +55,42 @@ func (m *CRConverterFactory) NewConverter(crd *apiextensions.CustomResourceDefin validVersions[schema.GroupVersion{Group: crd.Spec.Group, Version: version.Name}] = true } + var converter crConverterInterface switch crd.Spec.Conversion.Strategy { case apiextensions.NoneConverter: - unsafe = &crConverter{ - clusterScoped: crd.Spec.Scope == apiextensions.ClusterScoped, - delegate: &nopConverter{ - validVersions: validVersions, - }, - } - return &safeConverterWrapper{unsafe}, unsafe, nil + converter = &nopConverter{} case apiextensions.WebhookConverter: if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) { return nil, nil, fmt.Errorf("webhook conversion is disabled on this cluster") } - unsafe, err := m.webhookConverterFactory.NewWebhookConverter(validVersions, crd) + converter, err = m.webhookConverterFactory.NewWebhookConverter(crd) if err != nil { return nil, nil, err } - return &safeConverterWrapper{unsafe}, unsafe, nil + default: + return nil, nil, fmt.Errorf("unknown conversion strategy %q for CRD %s", crd.Spec.Conversion.Strategy, crd.Name) } - - return nil, nil, fmt.Errorf("unknown conversion strategy %q for CRD %s", crd.Spec.Conversion.Strategy, crd.Name) + unsafe = &crConverter{ + validVersions: validVersions, + clusterScoped: crd.Spec.Scope == apiextensions.ClusterScoped, + converter: converter, + } + return &safeConverterWrapper{unsafe}, unsafe, nil } -var _ runtime.ObjectConvertor = &crConverter{} +// crConverterInterface is the interface all cr converters must implement +type crConverterInterface interface { + // Convert converts in object to the given gvk and returns the converted object. + // Note that the function may mutate in object and return it. A safe wrapper will make sure + // a safe converter will be returned. + Convert(in runtime.Object, targetGVK schema.GroupVersion) (runtime.Object, error) +} -// crConverter extends the delegate with generic CR conversion behaviour. The delegate will implement the +// crConverter extends the delegate converter with generic CR conversion behaviour. The delegate will implement the // user defined conversion strategy given in the CustomResourceDefinition. type crConverter struct { - delegate runtime.ObjectConvertor + converter crConverterInterface + validVersions map[schema.GroupVersion]bool clusterScoped bool } @@ -100,29 +107,60 @@ func (c *crConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, valu } func (c *crConverter) Convert(in, out, context interface{}) error { - return c.delegate.Convert(in, out, context) + unstructIn, ok := in.(*unstructured.Unstructured) + if !ok { + return fmt.Errorf("input type %T in not valid for unstructured conversion", in) + } + + unstructOut, ok := out.(*unstructured.Unstructured) + if !ok { + return fmt.Errorf("output type %T in not valid for unstructured conversion", out) + } + + outGVK := unstructOut.GroupVersionKind() + converted, err := c.ConvertToVersion(unstructIn, outGVK.GroupVersion()) + if err != nil { + return err + } + unstructuredConverted, ok := converted.(runtime.Unstructured) + if !ok { + // this should not happened + return fmt.Errorf("CR conversion failed") + } + unstructOut.SetUnstructuredContent(unstructuredConverted.UnstructuredContent()) + return nil } // ConvertToVersion converts in object to the given gvk in place and returns the same `in` object. +// The in object can be a single object or a UnstructuredList. CRD storage implementation creates an +// UnstructuredList with the request's GV, populates it from storage, then calls conversion to convert +// the individual items. This function assumes it never gets a v1.List. func (c *crConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) { - // Run the converter on the list items instead of list itself + fromGVK := in.GetObjectKind().GroupVersionKind() + toGVK, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{fromGVK}) + if !ok { + // TODO: should this be a typed error? + return nil, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", fromGVK.String(), target) + } + if !c.validVersions[toGVK.GroupVersion()] { + return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", toGVK.GroupVersion().String()) + } + // Note that even if the request is for a list, the GV of the request UnstructuredList is what + // is expected to convert to. As mentioned in the function's document, it is not expected to + // get a v1.List. + if !c.validVersions[fromGVK.GroupVersion()] { + return nil, fmt.Errorf("request to convert CR from an invalid group/version: %s", fromGVK.GroupVersion().String()) + } + // Check list item's apiVersion if list, ok := in.(*unstructured.UnstructuredList); ok { for i := range list.Items { - obj, err := c.delegate.ConvertToVersion(&list.Items[i], target) - if err != nil { - return nil, err + expectedGV := list.Items[i].GroupVersionKind().GroupVersion() + if !c.validVersions[expectedGV] { + return nil, fmt.Errorf("request to convert CR list failed, list index %d has invalid group/version: %s", i, expectedGV.String()) } - - u, ok := obj.(*unstructured.Unstructured) - if !ok { - return nil, fmt.Errorf("output type %T in not valid for unstructured conversion", obj) - } - list.Items[i] = *u } - return list, nil } - - return c.delegate.ConvertToVersion(in, target) + return c.converter.Convert(in, toGVK.GroupVersion()) } // safeConverterWrapper is a wrapper over an unsafe object converter that makes copy of the input and then delegate to the unsafe converter. @@ -130,7 +168,7 @@ type safeConverterWrapper struct { unsafe runtime.ObjectConvertor } -var _ runtime.ObjectConvertor = &nopConverter{} +var _ runtime.ObjectConvertor = &safeConverterWrapper{} // ConvertFieldLabel delegate the call to the unsafe converter. func (c *safeConverterWrapper) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go new file mode 100644 index 00000000000..933b1c037cf --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go @@ -0,0 +1,195 @@ +/* +Copyright 2019 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 conversion + +import ( + "reflect" + "strings" + "testing" + + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/util/webhook" +) + +func TestConversion(t *testing.T) { + tests := []struct { + Name string + ValidVersions []string + ClusterScoped bool + ToVersion string + SourceObject runtime.Object + ExpectedObject runtime.Object + ExpectedFailure string + }{ + { + Name: "simple_conversion", + ValidVersions: []string{"example.com/v1", "example.com/v2"}, + ClusterScoped: false, + ToVersion: "example.com/v2", + SourceObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "other": "data", + "kind": "foo", + }, + }, + ExpectedObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "example.com/v2", + "other": "data", + "kind": "foo", + }, + }, + ExpectedFailure: "", + }, + { + Name: "failed_conversion_invalid_gv", + ValidVersions: []string{"example.com/v1", "example.com/v2"}, + ClusterScoped: false, + ToVersion: "example.com/v3", + SourceObject: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "other": "data", + }, + }, + ExpectedFailure: "invalid group/version: example.com/v3", + }, + { + Name: "simple_list_conversion", + ValidVersions: []string{"example.com/v1", "example.com/v2"}, + ClusterScoped: false, + ToVersion: "example.com/v2", + SourceObject: &unstructured.UnstructuredList{ + Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "fooList", + }, + Items: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "foo", + "other": "data", + }, + }, + { + Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "foo", + "other": "data2", + }, + }, + }, + }, + ExpectedObject: &unstructured.UnstructuredList{ + Object: map[string]interface{}{ + "apiVersion": "example.com/v2", + "kind": "fooList", + }, + Items: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "apiVersion": "example.com/v2", + "kind": "foo", + "other": "data", + }, + }, + { + Object: map[string]interface{}{ + "apiVersion": "example.com/v2", + "kind": "foo", + "other": "data2", + }, + }, + }, + }, + ExpectedFailure: "", + }, + { + Name: "list_with_invalid_gv", + ValidVersions: []string{"example.com/v1", "example.com/v2"}, + ClusterScoped: false, + ToVersion: "example.com/v2", + SourceObject: &unstructured.UnstructuredList{ + Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "fooList", + }, + Items: []unstructured.Unstructured{ + { + Object: map[string]interface{}{ + "apiVersion": "example.com/v1", + "kind": "foo", + "other": "data", + }, + }, + { + Object: map[string]interface{}{ + "apiVersion": "example.com/v3", + "kind": "foo", + "other": "data2", + }, + }, + }, + }, + ExpectedFailure: "invalid group/version: example.com/v3", + }, + } + + CRConverterFactory, err := NewCRConverterFactory(nil, func(resolver webhook.AuthenticationInfoResolver) webhook.AuthenticationInfoResolver { return nil }) + if err != nil { + t.Fatalf("Cannot create conversion factory: %v", err) + } + for _, test := range tests { + testCRD := apiextensions.CustomResourceDefinition{ + Spec: apiextensions.CustomResourceDefinitionSpec{ + Conversion: &apiextensions.CustomResourceConversion{ + Strategy: apiextensions.NoneConverter, + }, + }, + } + for _, v := range test.ValidVersions { + gv, _ := schema.ParseGroupVersion(v) + testCRD.Spec.Versions = append(testCRD.Spec.Versions, apiextensions.CustomResourceDefinitionVersion{Name: gv.Version, Served: true}) + testCRD.Spec.Group = gv.Group + } + safeConverter, _, err := CRConverterFactory.NewConverter(&testCRD) + if err != nil { + t.Fatalf("Cannot create converter: %v", err) + } + o := test.SourceObject.DeepCopyObject() + toVersion, _ := schema.ParseGroupVersion(test.ToVersion) + toVersions := schema.GroupVersions{toVersion} + actual, err := safeConverter.ConvertToVersion(o, toVersions) + if test.ExpectedFailure != "" { + if err == nil || !strings.Contains(err.Error(), test.ExpectedFailure) { + t.Fatalf("%s: Expected the call to fail with error message `%s` but err=%v", test.Name, test.ExpectedFailure, err) + } + } else { + if err != nil { + t.Fatalf("%s: conversion failed with : %v", test.Name, err) + } + if !reflect.DeepEqual(test.ExpectedObject, actual) { + t.Fatalf("%s: Expected = %v, Actual = %v", test.Name, test.ExpectedObject, actual) + } + } + } +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go index 8791238c5d0..8254fdfc0b9 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go @@ -17,9 +17,6 @@ limitations under the License. package conversion import ( - "errors" - "fmt" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -27,53 +24,18 @@ import ( // nopConverter is a converter that only sets the apiVersion fields, but does not real conversion. type nopConverter struct { - validVersions map[schema.GroupVersion]bool } -var _ runtime.ObjectConvertor = &nopConverter{} +var _ crConverterInterface = &nopConverter{} -func (nopConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) { - return "", "", errors.New("unstructured cannot convert field labels") -} - -func (c *nopConverter) Convert(in, out, context interface{}) error { - unstructIn, ok := in.(*unstructured.Unstructured) - if !ok { - return fmt.Errorf("input type %T in not valid for unstructured conversion", in) +// ConvertToVersion converts in object to the given gv in place and returns the same `in` object. +func (c *nopConverter) Convert(in runtime.Object, targetGV schema.GroupVersion) (runtime.Object, error) { + // Run the converter on the list items instead of list itself + if list, ok := in.(*unstructured.UnstructuredList); ok { + for i := range list.Items { + list.Items[i].SetGroupVersionKind(targetGV.WithKind(list.Items[i].GroupVersionKind().Kind)) + } } - - unstructOut, ok := out.(*unstructured.Unstructured) - if !ok { - return fmt.Errorf("output type %T in not valid for unstructured conversion", out) - } - - outGVK := unstructOut.GroupVersionKind() - if !c.validVersions[outGVK.GroupVersion()] { - return fmt.Errorf("request to convert CR from an invalid group/version: %s", outGVK.String()) - } - inGVK := unstructIn.GroupVersionKind() - if !c.validVersions[inGVK.GroupVersion()] { - return fmt.Errorf("request to convert CR to an invalid group/version: %s", inGVK.String()) - } - - unstructOut.SetUnstructuredContent(unstructIn.UnstructuredContent()) - _, err := c.ConvertToVersion(unstructOut, outGVK.GroupVersion()) - if err != nil { - return err - } - return nil -} - -func (c *nopConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) { - kind := in.GetObjectKind().GroupVersionKind() - gvk, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{kind}) - if !ok { - // TODO: should this be a typed error? - return nil, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", kind, target) - } - if !c.validVersions[gvk.GroupVersion()] { - return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", gvk.String()) - } - in.GetObjectKind().SetGroupVersionKind(gvk) + in.GetObjectKind().SetGroupVersionKind(targetGV.WithKind(in.GetObjectKind().GroupVersionKind().Kind)) return in, nil } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go index 0957ae9f8a3..a5f8c463c9c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go @@ -18,7 +18,6 @@ package conversion import ( "context" - "errors" "fmt" "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -55,7 +54,6 @@ func newWebhookConverterFactory(serviceResolver webhook.ServiceResolver, authRes // webhookConverter is a converter that calls an external webhook to do the CR conversion. type webhookConverter struct { - validVersions map[schema.GroupVersion]bool clientManager webhook.ClientManager restClient *rest.RESTClient name string @@ -85,61 +83,23 @@ func webhookClientConfigForCRD(crd *internal.CustomResourceDefinition) *webhook. return &ret } -var _ runtime.ObjectConvertor = &webhookConverter{} +var _ crConverterInterface = &webhookConverter{} -func (f *webhookConverterFactory) NewWebhookConverter(validVersions map[schema.GroupVersion]bool, crd *internal.CustomResourceDefinition) (*webhookConverter, error) { +func (f *webhookConverterFactory) NewWebhookConverter(crd *internal.CustomResourceDefinition) (*webhookConverter, error) { restClient, err := f.clientManager.HookClient(*webhookClientConfigForCRD(crd)) if err != nil { return nil, err } return &webhookConverter{ clientManager: f.clientManager, - validVersions: validVersions, restClient: restClient, name: crd.Name, - nopConverter: nopConverter{validVersions: validVersions}, + nopConverter: nopConverter{}, conversionReviewVersions: crd.Spec.Conversion.ConversionReviewVersions, }, nil } -func (webhookConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) { - return "", "", errors.New("unstructured cannot convert field labels") -} - -func (c *webhookConverter) Convert(in, out, context interface{}) error { - unstructIn, ok := in.(*unstructured.Unstructured) - if !ok { - return fmt.Errorf("input type %T in not valid for unstructured conversion", in) - } - - unstructOut, ok := out.(*unstructured.Unstructured) - if !ok { - return fmt.Errorf("output type %T in not valid for unstructured conversion", out) - } - - outGVK := unstructOut.GroupVersionKind() - if !c.validVersions[outGVK.GroupVersion()] { - return fmt.Errorf("request to convert CR from an invalid group/version: %s", outGVK.String()) - } - inGVK := unstructIn.GroupVersionKind() - if !c.validVersions[inGVK.GroupVersion()] { - return fmt.Errorf("request to convert CR to an invalid group/version: %s", inGVK.String()) - } - - converted, err := c.ConvertToVersion(unstructIn, outGVK.GroupVersion()) - if err != nil { - return err - } - unstructuredConverted, ok := converted.(runtime.Unstructured) - if !ok { - // this should not happened - return fmt.Errorf("CR conversion failed") - } - unstructOut.SetUnstructuredContent(unstructuredConverted.UnstructuredContent()) - return nil -} - // hasConversionReviewVersion check whether a version is accepted by a given webhook. func (c *webhookConverter) hasConversionReviewVersion(v string) bool { for _, b := range c.conversionReviewVersions { @@ -187,48 +147,17 @@ func getRawExtensionObject(rx runtime.RawExtension) (runtime.Object, error) { return &u, nil } -// getTargetGroupVersion returns group/version which should be used to convert in objects to. -// String version of the return item is APIVersion. -func getTargetGroupVersion(in runtime.Object, target runtime.GroupVersioner) (schema.GroupVersion, error) { - fromGVK := in.GetObjectKind().GroupVersionKind() - toGVK, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{fromGVK}) - if !ok { - // TODO: should this be a typed error? - return schema.GroupVersion{}, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", fromGVK.String(), target) - } - return toGVK.GroupVersion(), nil -} - -func (c *webhookConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) { +func (c *webhookConverter) Convert(in runtime.Object, toGV schema.GroupVersion) (runtime.Object, error) { // In general, the webhook should not do any defaulting or validation. A special case of that is an empty object // conversion that must result an empty object and practically is the same as nopConverter. // A smoke test in API machinery calls the converter on empty objects. As this case happens consistently // it special cased here not to call webhook converter. The test initiated here: // https://github.com/kubernetes/kubernetes/blob/dbb448bbdcb9e440eee57024ffa5f1698956a054/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go#L201 if isEmptyUnstructuredObject(in) { - return c.nopConverter.ConvertToVersion(in, target) + return c.nopConverter.Convert(in, toGV) } - toGV, err := getTargetGroupVersion(in, target) - if err != nil { - return nil, err - } - if !c.validVersions[toGV] { - return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", toGV.String()) - } - fromGV := in.GetObjectKind().GroupVersionKind().GroupVersion() - if !c.validVersions[fromGV] { - return nil, fmt.Errorf("request to convert CR from an invalid group/version: %s", fromGV.String()) - } listObj, isList := in.(*unstructured.UnstructuredList) - if isList { - for i, item := range listObj.Items { - fromGV := item.GroupVersionKind().GroupVersion() - if !c.validVersions[fromGV] { - return nil, fmt.Errorf("input list has invalid group/version `%v` at `%v` index", fromGV, i) - } - } - } // Currently converter only supports `v1beta1` ConversionReview // TODO: Make CRD webhooks caller capable of sending/receiving multiple ConversionReview versions From b62c8bca7a63e8c42cbf2992f2494543400252cf Mon Sep 17 00:00:00 2001 From: Mehdy Bohlool Date: Thu, 7 Mar 2019 12:57:27 -0800 Subject: [PATCH 2/3] Add latency metric for CR Webhook Conversion --- .../pkg/apiserver/conversion/converter.go | 9 +- .../pkg/apiserver/conversion/metrics.go | 88 +++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/metrics.go diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go index 1970b33c3c7..5d464bac505 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go @@ -33,11 +33,14 @@ type CRConverterFactory struct { // webhookConverterFactory is the factory for webhook converters. // This field should not be used if CustomResourceWebhookConversion feature is disabled. webhookConverterFactory *webhookConverterFactory + converterMetricFactory *converterMetricFactory } // NewCRConverterFactory creates a new CRConverterFactory func NewCRConverterFactory(serviceResolver webhook.ServiceResolver, authResolverWrapper webhook.AuthenticationInfoResolverWrapper) (*CRConverterFactory, error) { - converterFactory := &CRConverterFactory{} + converterFactory := &CRConverterFactory{ + converterMetricFactory: newConverterMertricFactory(), + } if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) { webhookConverterFactory, err := newWebhookConverterFactory(serviceResolver, authResolverWrapper) if err != nil { @@ -67,6 +70,10 @@ func (m *CRConverterFactory) NewConverter(crd *apiextensions.CustomResourceDefin if err != nil { return nil, nil, err } + converter, err = m.converterMetricFactory.addMetrics("webhook", crd.Name, converter) + if err != nil { + return nil, nil, err + } default: return nil, nil, fmt.Errorf("unknown conversion strategy %q for CRD %s", crd.Spec.Conversion.Strategy, crd.Name) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/metrics.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/metrics.go new file mode 100644 index 00000000000..89090d7f999 --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/metrics.go @@ -0,0 +1,88 @@ +/* +Copyright 2019 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 conversion + +import ( + "fmt" + "strconv" + "sync" + "time" + + "github.com/prometheus/client_golang/prometheus" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +var ( + latencyBuckets = prometheus.ExponentialBuckets(0.001, 2, 15) +) + +// converterMetricFactory holds metrics for all CRD converters +type converterMetricFactory struct { + // A map from a converter name to it's metric. Allows the converterMetric to be created + // again with the same metric for a specific converter (e.g. 'webhook'). + durations map[string]*prometheus.HistogramVec + factoryLock sync.Mutex +} + +func newConverterMertricFactory() *converterMetricFactory { + return &converterMetricFactory{durations: map[string]*prometheus.HistogramVec{}, factoryLock: sync.Mutex{}} +} + +var _ crConverterInterface = &converterMetric{} + +type converterMetric struct { + delegate crConverterInterface + latencies *prometheus.HistogramVec + crdName string +} + +func (c *converterMetricFactory) addMetrics(converterName string, crdName string, converter crConverterInterface) (crConverterInterface, error) { + c.factoryLock.Lock() + defer c.factoryLock.Unlock() + metric, exists := c.durations[converterName] + if !exists { + metric = prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: fmt.Sprintf("apiserver_crd_%s_conversion_duration_seconds", converterName), + Help: fmt.Sprintf("CRD %s conversion duration in seconds", converterName), + Buckets: latencyBuckets, + }, + []string{"crd_name", "from_version", "to_version", "succeeded"}) + err := prometheus.Register(metric) + if err != nil { + return nil, err + } + c.durations[converterName] = metric + } + return &converterMetric{latencies: metric, delegate: converter, crdName: crdName}, nil +} + +func (m *converterMetric) Convert(in runtime.Object, targetGV schema.GroupVersion) (runtime.Object, error) { + start := time.Now() + obj, err := m.delegate.Convert(in, targetGV) + fromVersion := in.GetObjectKind().GroupVersionKind().Version + toVersion := targetGV.Version + + // only record this observation if the version is different + if fromVersion != toVersion { + m.latencies.WithLabelValues( + m.crdName, fromVersion, toVersion, strconv.FormatBool(err == nil)).Observe(time.Since(start).Seconds()) + } + return obj, err +} From 18be830680371d8becef7f2b51f4e94e49842dc5 Mon Sep 17 00:00:00 2001 From: Mehdy Bohlool Date: Thu, 7 Mar 2019 11:00:41 -0800 Subject: [PATCH 3/3] update generated files --- .../pkg/apiserver/conversion/BUILD | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/BUILD index 560e9388341..2639f2ab6e3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/BUILD @@ -1,9 +1,10 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = [ "converter.go", + "metrics.go", "nop_converter.go", "webhook_converter.go", ], @@ -22,6 +23,7 @@ go_library( "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/webhook:go_default_library", "//staging/src/k8s.io/client-go/rest:go_default_library", + "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", ], ) @@ -38,3 +40,16 @@ filegroup( tags = ["automanaged"], visibility = ["//visibility:public"], ) + +go_test( + name = "go_default_test", + srcs = ["converter_test.go"], + embed = [":go_default_library"], + deps = [ + "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/webhook:go_default_library", + ], +)