UI-API-Layer does not handle wrong ServicePlans schemas gracefully

mszostok commented 5 years ago

Description

When ClusterServicePlan/ServicePlan have incorrect schemas then ui-api-layer cannot convert it.

Example ClusterServicePlan

        {
            "apiVersion": "servicecatalog.k8s.io/v1beta1",
            "kind": "ClusterServicePlan",
            "metadata": {
                "creationTimestamp": "2018-12-26T18:06:37Z",
                "name": "381ddd7c-fcf9-4de8-9c63-f27bd447d241",
                "namespace": "",
                "ownerReferences": [
                    {
                        "apiVersion": "servicecatalog.k8s.io/v1beta1",
                        "blockOwnerDeletion": false,
                        "controller": true,
                        "kind": "ClusterServiceBroker",
                        "name": "sm-proxy-b02f0a63-fa83-402a-8c38-173b4c734792",
                        "uid": "fbf1450e-0938-11e9-8f57-fa1589369e7b"
                    }
                ],
                "resourceVersion": "2791",
                "selfLink": "/apis/servicecatalog.k8s.io/v1beta1/clusterserviceplans/381ddd7c-fcf9-4de8-9c63-f27bd447d241",
                "uid": "fc80aa31-0938-11e9-8f57-fa1589369e7b"
            },
            "spec": {
                "bindable": true,
                "clusterServiceBrokerName": "sm-proxy-b02f0a63-fa83-402a-8c38-173b4c734792",
                "clusterServiceClassRef": {
                    "name": "7e9b961e-5146-4676-8d9a-13e0eac45c88"
                },
                "description": "Provides a UUID.",
                "externalID": "381ddd7c-fcf9-4de8-9c63-f27bd447d241",
                "externalMetadata": {
                    "bullets": [
                        "regular and random UUID"
                    ],
                    "displayName": "UUID"
                },
                "externalName": "one-uuid",
                "free": true,
                "instanceCreateParameterSchema": "",
                "instanceUpdateParameterSchema": ""
            },
            "status": {
                "removedFromBrokerCatalog": false
            }
        },

Log from ui-api-layer pod:

E1226 20:08:03.356787       1 clusterserviceclass_resolver.go:116] while converting Cluster Service Plans: while converting item 381ddd7c-fcf9-4de8-9c63-f27bd447d241 to ClusterServicePlan: while extracting creation parameter schema: json: cannot unmarshal string into Go value of type map[string]interface {}

Expected result

UI-API-Layer handles the wrong schemas in ClusterServicePlans/ServicePlans more gracefuly. E.g. returning error that the schemas cannot be decoded but I can still see the ServiceClass details.

Actual result

Returns nothing saying error:

{"data":{"clusterServiceClass":null,"serviceClass":null},"errors":[{"message":"internal error","path":["clusterServiceClass","plans"]}]}

And on UI it's even worst because this internal error is converted to the misleading message: screen shot 2018-12-27 at 13 14 10

Steps to reproduce Edit any kind of available ClusterServicePlan/ServicePlan on your cluster and add just the schema with empty string, like this one: "instanceUpdateParameterSchema": ""

derberg commented 5 years ago

I don't think we should display bad SC if all plans schemas are broken because we should not bother the user with not usable functionality:

in case of 2 or more plans and only one invalid, we should ignore invalid plan in the UI and show the valid ones only
in case if all plans are invalid, we should not display the class at all and the operator/admin should only see a proper error in the logs to understand that given class was not registered due to a given error

derberg commented 5 years ago

also please take a look on OSB API spec: https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#input-parameters-schema-object

instanceUpdateParameterSchema cannot be empty string. It must be a JSON schema object

mszostok commented 5 years ago

@derberg I know what is in OSB API and because of that I only said that for now, the error on UI is misleading.

I'm expecting sth like that:

UI-API-Layer handles the wrong schemas in ClusterServicePlans/ServicePlans more gracefully. E.g. returning error that the schemas cannot be decoded but I can still see the ServiceClass details. The provisioning can be blocked

IMO your suggestion about hiding invalid classes is also misleading. We should be transparent. Why just do not display that the schemas are wrong? Thanks to that I will know that broker has this classes but they are incorrect, so I know what kind of action I should take.

derberg commented 5 years ago

I agree we have a bug and we need to fix it but first we need to find a common ground. For now UI has a bug because the expectation is that all connected brokers are created in respect with OSB API

Even though you bolded out some content I don't really take it 😄

who should be transparent?
what transparency has to do now

As of now when we work on designs and development we envision different personas using the Console UI. So we have an operator that registers the brokers and we have a developer that consumes the classes. With this approach me, as an operator, I would not like my consumers to see classes that are useless/broken.

Don't get me wrong, I get your point, but you're view on this issue is from a service catalog contributor and a guy that writes brokers. Consumer of the catalog should not see broken classes, it should be seen only by the person that has rights to register brokers. UI is not the only layer for the user, he also has API and CLI, so the same level of awareness and transparency should be available on all layers, not just the UI. I hope you get my point here, but please try to understand my point too.

@PK85 maybe we should hold some meeting, and sync all together on short term and mid term approach here. Short term let us just hide broken classes, long term let us work on some proposal, maybe core service catalog contribution or some custom validation controller? or even a validation webhook service configured to always validate service classes and plans created in the catalog

derberg commented 5 years ago

ok, long term approach confirmed: https://github.com/kubernetes-incubator/service-catalog/issues/2088 Service catalog is the one that should fail with registration of the broker that is not fully following the OSB API

short term, let us make sure we are handling errors properly and we do not display broken service classes. @pkosiec how much is https://github.com/kyma-project/kyma/issues/2113 addressing the errors problem?

pkosiec commented 5 years ago

@derberg Sorry, I missed that. The #2113 doesn't have anything related to this bug.

The easiest and quickest improvement would be to make (Cluster)ServicePlans field nullable. Then the other (Cluster)ServiceClass details would be returned correctly.