Investigate the service catalog defaults and migrate to CRDs if needed

[mszostok]

Description

The Service Catalog api-server uses the defaults. Those defaults are generated by defaulter-gen. For now, I do not know how they are used. Maybe they will work as they are or maybe we will need to implement it in mutating webhook.

AC:

• we know how api-server defaults are used and if needed is replaced with CRDs functionality.

[piotrmiskiewicz]

Defaults mentioned in the description can be applied by the client app which uses (imports) the API. The sequence of the code can looks like:

func init() {

	_ = v1beta1.AddToScheme(runtimeScheme)

}

func useDefaults() {
// some code
runtimeScheme.Default(obj)

}

The original API server executes the Default method after every call to a webhook (https://github.com/kubernetes-incubator/service-catalog/blob/master/vendor/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go#L164).
On the other hand we do not see any advantage to use defaults and other ways to have default values. All of them can be in one place - in prepare for create webhook.

The webhook which applies defaults could looks like:

func (whsvr *WebhookServer) mutate(ar *v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
	req := ar.Request

	obj, err := runtimeScheme.New(schema.GroupVersionKind{
		Group: req.Kind.Group,
		Kind: req.Kind.Kind,
		Version: req.Kind.Version,
	})
	if err != nil {
		glog.Errorf("Could not create obj: %v", err)
		return &v1beta1.AdmissionResponse{
			Result: &metav1.Status{
				Message: err.Error(),
			},
		}
	}

        // modify the object
	defaulter.Default(obj)

        // prepare the patch in the AdmissionResponse struct
	
}

[piotrmiskiewicz]

The current Service Catalog implementation (with API server) uses the scheme.Default method only in the API Server. There is no any other place with a scheme.Default() method call. Some of setting default values logic are implemented in other place, for example:
https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/registry/servicecatalog/servicebroker/strategy.go#L88

We do not see any advantages to have the defaulting logic splitted into defaults (generated by defaulter-gen) and webhooks. The idea is to have it in one place - in the Webhook. We prefer to remove the defaulter-gen approach.

[mszostok]

About your approach with webhook

Baase on your snippet I assume that defaulter.Default(obj) is applying defaults by its own. I do not think that it is a good approach. Mutating webhook should return patch in AdmissionResponse and main api-server should apply final obj into etcd. What's more if u want to apply default to obj in etcd, then how it can be achieved on CREATE action when this obj is not yet in etcd?

My questions

• where those defaults are used in Service Catalog?
  This is the only place where defaults are used?
• how new defaults can be added using this defaulter-gen tool?
  does it really simplify the life of the developer?

My suggestion

If defaults are used only it two places:

	if spec.RelistBehavior == "" {
		spec.RelistBehavior = ServiceBrokerRelistBehaviorDuration
	}

and

	// If not specified, make the SecretName default to the binding name
	if binding.Spec.SecretName == "" {
		binding.Spec.SecretName = binding.Name
	}

and if adding new defaults using defaulter-gen tool is not significantly simplify the life of the developer

then my suggestion is to remove the defaulter-gen approach totally and inline those statements directly in webhooks.

[piotrmiskiewicz]

The defaulter-gen tool scans for top level types and generate efficient defaulters for an entire object from the sum of the SetDefault_XXX methods contained in the object tree. More about it in the tool description. So the way to use it is implement SetDefault_ methods and run defaulter-gen tool.