From ccb1ec7a3695082326fe60ec06890f91004dc043 Mon Sep 17 00:00:00 2001 From: yue9944882 <291271447@qq.com> Date: Thu, 28 Jun 2018 16:35:15 +0800 Subject: [PATCH] fixes operation for "create on update" remove create-on-update logic for quota controller review: add more error check remove unused args revert changes in patch.go use hasUID to judge if it's a create-on-update --- .../resourcequota/resource_quota_monitor.go | 2 +- pkg/quota/evaluator/core/registry.go | 2 +- pkg/quota/generic/evaluator.go | 7 +---- .../pkg/admission/resourcequota/controller.go | 2 +- .../apiserver/pkg/endpoints/handlers/patch.go | 1 + .../pkg/endpoints/handlers/update.go | 27 +++++++++++++++---- 6 files changed, 27 insertions(+), 14 deletions(-) diff --git a/pkg/controller/resourcequota/resource_quota_monitor.go b/pkg/controller/resourcequota/resource_quota_monitor.go index c437b48b27..be87777e0f 100644 --- a/pkg/controller/resourcequota/resource_quota_monitor.go +++ b/pkg/controller/resourcequota/resource_quota_monitor.go @@ -223,7 +223,7 @@ func (qm *QuotaMonitor) SyncMonitors(resources map[schema.GroupVersionResource]s if evaluator == nil { listerFunc := generic.ListerFuncForResourceFunc(qm.informerFactory.ForResource) listResourceFunc := generic.ListResourceUsingListerFunc(listerFunc, resource) - evaluator = generic.NewObjectCountEvaluator(false, resource.GroupResource(), listResourceFunc, "") + evaluator = generic.NewObjectCountEvaluator(resource.GroupResource(), listResourceFunc, "") qm.registry.Add(evaluator) glog.Infof("QuotaMonitor created object count evaluator for %s", resource.GroupResource()) } diff --git a/pkg/quota/evaluator/core/registry.go b/pkg/quota/evaluator/core/registry.go index 5a642b386a..ba54143c32 100644 --- a/pkg/quota/evaluator/core/registry.go +++ b/pkg/quota/evaluator/core/registry.go @@ -44,7 +44,7 @@ func NewEvaluators(f quota.ListerForResourceFunc) []quota.Evaluator { // these evaluators require an alias for backwards compatibility for gvr, alias := range legacyObjectCountAliases { result = append(result, - generic.NewObjectCountEvaluator(false, gvr.GroupResource(), generic.ListResourceUsingListerFunc(f, gvr), alias)) + generic.NewObjectCountEvaluator(gvr.GroupResource(), generic.ListResourceUsingListerFunc(f, gvr), alias)) } return result } diff --git a/pkg/quota/generic/evaluator.go b/pkg/quota/generic/evaluator.go index 7df5243e59..60da7d634b 100644 --- a/pkg/quota/generic/evaluator.go +++ b/pkg/quota/generic/evaluator.go @@ -167,9 +167,6 @@ func CalculateUsageStats(options quota.UsageStatsOptions, // that associates usage of the specified resource based on the number of items // returned by the specified listing function. type objectCountEvaluator struct { - // allowCreateOnUpdate if true will ensure the evaluator tracks create - // and update operations. - allowCreateOnUpdate bool // GroupResource that this evaluator tracks. // It is used to construct a generic object count quota name groupResource schema.GroupResource @@ -189,7 +186,7 @@ func (o *objectCountEvaluator) Constraints(required []api.ResourceName, item run // Handles returns true if the object count evaluator needs to track this attributes. func (o *objectCountEvaluator) Handles(a admission.Attributes) bool { operation := a.GetOperation() - return operation == admission.Create || (o.allowCreateOnUpdate && operation == admission.Update) + return operation == admission.Create } // Matches returns true if the evaluator matches the specified quota with the provided input item @@ -241,7 +238,6 @@ var _ quota.Evaluator = &objectCountEvaluator{} // purposes for the legacy object counting names in quota. Unless its supporting // backward compatibility, alias should not be used. func NewObjectCountEvaluator( - allowCreateOnUpdate bool, groupResource schema.GroupResource, listFuncByNamespace ListFuncByNamespace, alias api.ResourceName) quota.Evaluator { @@ -251,7 +247,6 @@ func NewObjectCountEvaluator( } return &objectCountEvaluator{ - allowCreateOnUpdate: allowCreateOnUpdate, groupResource: groupResource, listFuncByNamespace: listFuncByNamespace, resourceNames: resourceNames, diff --git a/plugin/pkg/admission/resourcequota/controller.go b/plugin/pkg/admission/resourcequota/controller.go index 5f0f6fd6e9..745bb2d5ba 100644 --- a/plugin/pkg/admission/resourcequota/controller.go +++ b/plugin/pkg/admission/resourcequota/controller.go @@ -592,7 +592,7 @@ func (e *quotaEvaluator) Evaluate(a admission.Attributes) error { if evaluator == nil { // create an object count evaluator if no evaluator previously registered // note, we do not need aggregate usage here, so we pass a nil informer func - evaluator = generic.NewObjectCountEvaluator(false, gr, nil, "") + evaluator = generic.NewObjectCountEvaluator(gr, nil, "") e.registry.Add(evaluator) glog.Infof("quota admission added evaluator for: %s", gr) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go index ed58449d59..4965d62edf 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -108,6 +108,7 @@ func PatchResource(r rest.Patcher, scope RequestScope, admit admission.Interface userInfo, _ := request.UserFrom(ctx) staticAdmissionAttributes := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo) admissionCheck := func(updatedObject runtime.Object, currentObject runtime.Object) error { + // if we allow create-on-patch, we have this TODO: call the mutating admission chain with the CREATE verb instead of UPDATE if mutatingAdmission, ok := admit.(admission.MutationInterface); ok && admit.Handles(admission.Update) { return mutatingAdmission.Admit(admission.NewAttributesRecord(updatedObject, currentObject, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo)) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go index ac318f2764..9d67fc327c 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go @@ -96,12 +96,24 @@ func UpdateResource(r rest.Updater, scope RequestScope, admit admission.Interfac } userInfo, _ := request.UserFrom(ctx) - staticAdmissionAttributes := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo) var transformers []rest.TransformFunc - if mutatingAdmission, ok := admit.(admission.MutationInterface); ok && mutatingAdmission.Handles(admission.Update) { + if mutatingAdmission, ok := admit.(admission.MutationInterface); ok { transformers = append(transformers, func(ctx context.Context, newObj, oldObj runtime.Object) (runtime.Object, error) { - return newObj, mutatingAdmission.Admit(admission.NewAttributesRecord(newObj, oldObj, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo)) + isNotZeroObject, err := hasUID(oldObj) + if err != nil { + return nil, fmt.Errorf("unexpected error when extracting UID from oldObj: %v", err.Error()) + } else if !isNotZeroObject { + if mutatingAdmission.Handles(admission.Create) { + return newObj, mutatingAdmission.Admit(admission.NewAttributesRecord(newObj, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Create, userInfo)) + } + } else { + if mutatingAdmission.Handles(admission.Update) { + return newObj, mutatingAdmission.Admit(admission.NewAttributesRecord(newObj, oldObj, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo)) + } + } + return newObj, nil }) + } createAuthorizerAttributes := authorizer.AttributesRecord{ @@ -124,8 +136,13 @@ func UpdateResource(r rest.Updater, scope RequestScope, admit admission.Interfac ctx, name, rest.DefaultUpdatedObjectInfo(obj, transformers...), - withAuthorization(rest.AdmissionToValidateObjectFunc(admit, staticAdmissionAttributes), scope.Authorizer, createAuthorizerAttributes), - rest.AdmissionToValidateObjectUpdateFunc(admit, staticAdmissionAttributes), + withAuthorization(rest.AdmissionToValidateObjectFunc( + admit, + admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Create, userInfo)), + scope.Authorizer, createAuthorizerAttributes), + rest.AdmissionToValidateObjectUpdateFunc( + admit, + admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Update, userInfo)), false, ) wasCreated = created