APB tool does not catch invalid Specs

[dymurray]

The APB tool needs better linting of the APB spec. You can break apb push by setting a blank description. The broker bootstraps the spec successfully and an error only occurs when the Service Catalog tries to list the catalog. To reproduce:

apb.yml

---
version: 1.0
name: test55-apb
description: ''
bindable: false
async: optional
metadata:
  displayName: Demo1 (APB)
  imageUrl: http://localhost:4000/pictures/1.jpg
plans:
- name: default
  description: Default deployment plan for Demo1-apb
  free: true
  metadata:
    displayName: Default
    longDescription: This plan deploys an instance of Demo1
    cost: "$0.0"
  parameters: []

Do a successful apb push.

See this in the controller manager logs:

I0316 13:37:50.911754       1 event.go:218] Event(v1.ObjectReference{Kind:"ClusterServiceBroker", Namespace:"", Name:"ansible-service-broker", UID:"6d268387-291e-11e8-8bff-0242ac110005", APIVersion:"servicecatal
og.k8s.io", ResourceVersion:"42", FieldPath:""}): type: 'Warning' reason: 'ErrorSyncingCatalog' Error reconciling ClusterServiceClass (K8S: "d93141e93ca94cca7c022801463185a5" ExternalName: "localregistry-test55-
apb") (broker "ansible-service-broker"): ClusterServiceClass.servicecatalog.k8s.io "d93141e93ca94cca7c022801463185a5" is invalid: spec.description: Required value: description is required

[mkanoor]

@dymurray SyncingCatalog Error syncing catalog from ServiceBroker. Error reconciling ClusterServiceClass (K8S: "ab69b2bb5a3b1fe973ce05af20ba5047" ExternalName: "localregistry-cfme_rhev-apb") (broker "ansible-service-broker"): ClusterServiceClass.servicecatalog.k8s.io "ab69b2bb5a3b1fe973ce05af20ba5047" is invalid: spec.externalName: Invalid value: "localregistry-cfme_rhev-apb": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is 'a-z0-9?(.a-z0-9?)*')}] 3 2018-03-16 14:08:33 +0000 UTC}}

[djzager]

I'm genuinely curious how the APB tool is going to do validation that even the Broker is not doing. Is this validation something that we should be pushing onto the broker?

What I do know about the apb push flow is that we push the image to the internal OpenShift registry, tell our Broker to bootstrap, and bump the relistRequests on the ClusterServiceBroker. The interesting part about this is the Broker portion where we take a Spec object and transform that into a (Cluster)ServiceClass the catalog will understand. I trust that @eriknelson could correct me if I am totally off base. I don't want us to be in a scenario where we are doing client-side validation that really should be done by the broker (or duplicating effort).

From an APB developer's perspective, this is totally something that we should be catching. I'm just not sure what the existing thoughts are on how to fix it.

[eriknelson]

we push the image to the internal OpenShift registry, tell our Broker to bootstrap, and bump the relistRequests on the ClusterServiceBroker.

Exactly correct!

I don't want us to be in a scenario where we are doing client-side validation that really should be done by the broker (or duplicating effort).

I don't actually think this is a bad thing? I just want to see everyone using the same rules. If we can catch it in the client using the same logic the broker does, that's great.

I've been pushing for a libapb (now lib-bundle) because I want to see a reusable lib that canonically defines what a bundle is, and can validate whether or not it is valid. The broker ultimately shouldn't care or take any opinion on the topic; it will delegate to bundle-lib and vendor it as any other project, because it's not an authority on bundles. Likewise, I'm seeing apb v2, (bundle-cli?), also as a go cli layer on top of bundle-lib, so that we can execute the canonical bundle-lib validation on your apb in the tooling before letting you push. Meaning: if you try to push a bad apb, the tool can immediately tell you it's bad thanks to the re-use of bundle-lib, which is the same logic the broker works off of.

[mkanoor]

The Open Service Broker API spec has restrictions on names. https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md

e.g. name* string The CLI-friendly name of the plan. MUST only contain lowercase characters, numbers and hyphens (no spaces). MUST be unique within the service. MUST be a non-empty string.

description* string A short description of the plan. MUST be a non-empty string.

So these kinds of constraint checking would aid in debugging.

[eriknelson]

@mkanoor I didn't realize that was a spec level constraint! Thank you for pointing that out, we need to get our stuff updated asap. @dymurray

That's the kind of stuff I would love to capture in the lib, so everything inherits the library's validation rules.

[dymurray]

+1, I am definitely opposed to closing this. This is a real crappy experience for a developer and it would be pretty trivial to put client side validation to prevent this.

[djzager]

+1, I am definitely opposed to closing this.

That was totally my fault. I was trying to exit out of a reply to @eriknelson and hit the "Close and comment" because I always confuse it for "Cancel". I agree that this is a crappy experience and should be fixed. @eriknelson had a great point when he said:

I just want to see everyone using the same rules. If we can catch it in the client using the same logic the broker does, that's great.

[mkanoor]

Another issue is the parameter names used internally by the APB

"_apb_plan_id":"default", "_apb_service_class_id":"7a531cb95ab6e6fdeae1fc7211474d9c", "_apb_service_instance_id":"1039b299-f131-4bca-a701-6faa8c876d6d" "namespace": "My Project"

If the user created a parameter called namespace they won't know its reserved and would end up overwriting it

[isacssouza]

I also had a hard time debugging an invalid plan name (https://github.com/ansibleplaybookbundle/ansible-playbook-bundle/issues/244#issuecomment-373863718) and agree that client-side checking of the spec would help a lot in development of apbs.