kubernetes-retired / go-open-service-broker-client Goto Github PK

Hello guys, are there any plans to support Go Modules instead of dep?

great library so far, would be nice to improve error message from the client so that users of the client do not need to wrap them in something more user friendy.

current errors received:

• missing config.URL

Fetching catalog: Get /v2/catalog: unsupported protocol scheme ""

• wrong auth creds

Using 'http://10.244.0.2:8080' service broker
ERROR: logging before flag.Parse: I0828 18:52:18.359582   97591 client.go:192] handling failure responses

Fetching catalog: Status: 401; ErrorMessage: <nil>; Description: <nil>; ResponseError: invalid character 'N' looking for beginning of value

wdyt?

cc @youreddy

kubernetes for sure, and whatever the cloudfoundry one is if it is intended to be supported.

kubernetes-retired/service-catalog#1240 (comment)

As @luksa pointed out in #96, parts of this library's types don't match the OSB schema.

• The ParameterSchemas field of Plan should be called Schemas
• The type ParameterSchemas should be called Schemas
• The ServiceInstances field of Schemas should be called ServiceInstance
• The ServiceBindings field of Schemas should be called ServiceBindings

Found this today debugging starter pack.

As part of #146 I noticed that ServiceBindingSchema.Create are of type RequestResponseSchema rather than simply InputParametersSchema: https://github.com/kubernetes-sigs/go-open-service-broker-client/blob/56a01c84bf43813ca1f99edb5033812927cd67ab/v2/types.go#L149

This is not in line with any released version of the OSBAPI spec or with master:

• master: https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#schemas-object
• 2.15: https://github.com/openservicebrokerapi/servicebroker/blob/v2.15/spec.md#schemas-object
• 2.14: https://github.com/openservicebrokerapi/servicebroker/blob/v2.14/spec.md#schemas-object
• 2.13: https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md#schema-object

It seems like it was part of an earlier draft version of the schema spec: https://github.com/n3wscott/servicebroker/blob/95910853f5287b7b647fce28e4165f42d3a5b715/spec.md#schema-object

I propose that we switch ServiceBindingSchema.Create to just be of type InputParametersSchema to simplify the logic we have in get_catalog.go

Instead of just a bool, it should return a HTTPStatusCodeError and a bool. If the given error is actually an HTTPStatusCodeError, the caller will get the specific error instead of having to type-assert it later

I'm seeing messages that look like this:

 "services": [
    {
      "name": "dh-mongodb-apb",
      "id": "e9c042c4925dd0c7c25ceca4f5179e1c",
      "description": "Deploy MongoDB app on your Openshift Project",
      "bindable": true,
      "metadata": {
        "displayName": "MongoDB (APB)",
        "documentationUrl": "https://access.redhat.com",
        "imageUrl": "http://nightdeveloper.net/wp-content/uploads/2014/12/mongo_db.png",
        "longDescription": "MongoDB is a document database with the scalability and flexibility that you want with the querying and indexing that you need. This APB will cover an standalone and HA deployment.",
        "providerDisplayName": "Red Hat, Inc."
      },
      "instances_retrievable": true,
      "bindings_retrievable": true,
      "plans": [
        {
          "id": "85f2cc9b1c440e49dce41f2939dca1d2",
          "name": "ephemeral",
          "description": "This default plan deploys mongodb-apb",
          "metadata": {
            "cost": "€0.00",
            "displayName": "Ephemeral",
            "longDescription": "This plan will deploy a MongoDB standalone or HA instance/s using ephemeral storage",
            "schemas": {
              "service_binding": {
                "create": {
                  "openshift_form_definition": []
                }
              },
              "service_instance": {
                "create": {
                  "openshift_form_definition": [
                    {
                      "title": "Container Specs",
                      "type": "fieldset",
                      "items": [
                        {
                          "key": "MONGODB_VERSION",
                          "type": "select"
                        },
                        "MONGODB_IMAGE_TAG",
                        "USE_UPSTREAM_IMAGES"
                      ]
                    },
                    {
                      "title": "MongoDB Config",
                      "type": "fieldset",
                      "items": [
                        {
I0228 23:48:18.219946       1 leaderelection.go:199] successfully renewed lease kube-service-catalog/service-catalog-controller-manager
I0228 23:48:20.225271       1 leaderelection.go:199] successfully renewed lease kube-service-catalog/service-catalog-controller-manager
I0228 23:48:22.233711       1 leaderelection.go:199] successfully renewed lease kube-service-catalog/service-catalog-controller-manager

...
    {
      "name": "dh-rocketchat-apb",
      "id": "ba9c2d4db404ce97111bea80225de968",
      "description": "This APB deploys RocketChat backed by MongoDB",
      "bindable": false,
      "metadata": {
        "dependencies": [
          "rocket.chat:latest",
          "mongo:3.2"
        ],
        "displayName": "RocketChat (APB)",
        "documentationUrl": "https://rocket.chat",
        "imageUrl": "https://github.com/RocketChat/Rocket.Chat.Artwork/blob/master/Logos/rocketcat.png?raw=true",
        "longDescription": "An APB that deploys RocketChat to OpenShift backed by MongoDB",
        "providerDisplayName": "Red Hat, Inc."
      },
      "instances_retrievable": true,
      "bindings_retrievable": false,
      "plans": [
        {
          "id": "b2bba601df39d5774c2313bc716981e0",
          "name": "default",
          "description": "This plan deploys a single RocketChat application backed by MongoDB",
          "metadata": {
            "cost": "$0.00",
            "displayName": "Default",
            "longDescription": "This plan provides a RocketChat application backed by MongoDB",
            "schemas": {
              "service_binding": {
                "create": {
                  "openshift_form_definition": []
                }
              },
              "service_instance": {
                "create": {
                  "openshift_form_definition": [
                    "mongodb_user",
                    "mongodb_pass",
                    "mongodb_name",
                    "mongodb_admin_pass",
                    "mongodb_version",
                    "rocketchat_version"
                  ]
                },
                "update": {}
              }
            }
          },
          "free": true,
          "schemas": {
            "service_instance": {
              "create": {
                "parameters": {
                  "$schema": "http://json-schema.org/draft-04/schema",

The broker's response to GetCatalog is being interrupted or truncated somehow.

Currently, we expect error responses to be able to be marshaled into the following:

type failureResponseBody struct {
        Err         *string `json:"error,omitempty"`
        Description *string `json:"description,omitempty"`
}

The spec says we should ignore errors that are not of the expected format. However, responses that match these keys but are not strings but instead nested objects will cause an unmarshaling error, which creates a HTTPStatusCodeError object that looks like the following:

Status: 500; ErrorMessage: <nil>; Description:
      <nil>; ResponseError: json: cannot unmarshal object into Go struct field failureResponseBody.error
      of type string

This error is surfaced to the user when it should be ignored, and is misleading as the error reported here is not from the broker but from the go client.

The v2.Plan struct uses a boolean pointer to indicate if the service plan is free or not. The comment above the struct field says it defaults to true but the default value of boolean pointer would be nil. We need to verify if service plans are free by default according to OSB.

In deprovision_instance.go it appears that what should be query params according to the OSB spec on deprovisioning are being sent as body to the DELETE.

documentation should be added to the godoc of https://github.com/pmorie/go-open-service-broker-client/blob/0.0.10/v2/types.go#L177-L182 indicating that the fields have been deprecated in OSBAPI. The documentation should recommend and point to https://github.com/openservicebrokerapi/servicebroker/blob/v2.14/profile.md#kubernetes-context-object as containing the canonical version of the information.

Once the 2.15 support #161 is complete, support for 2.16 has to be added.
Below is a list of items from the relevant release notes:

• Handle the 412 returned by https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#api-version-header
• expose the handled error to the library user

LastOperationResponse and service broker error response fields should be updated to reflect

The instance_usable response fields should be included in:

• UpdateInstanceResponse
• DeprovisionResponse
• LastOperationResponse

If an update or deprovisioning operation failed, this flag indicates whether or not the Service Instance is still usable. If true, the Service Instance can still be used, false otherwise. This field MUST NOT be present for errors of other operations. Defaults to true.

update_repeatable should be available in all update operations.

If an update operation failed, this flag indicates whether this update can be repeated or not. If true, the same update operation MAY be repeated and MAY succeed; if false, repeating the same update operation will fail again. This field MUST NOT be present for errors of other operations. Defaults to true.

Per kubernetes-retired/service-catalog#1064

...that can be set by the client user and sent to the broker in X-Broker-Request-ID (or something like that) header, to allow for tracing requests from platform into broker

The contents of
https://github.com/pmorie/go-open-service-broker-client/blob/master/v2/types.go
should have convenience functions to allow them to be printed nicely rather than printing pieces of each struct.

According to openservicebrokerapi/servicebroker#431

Per openservicebrokerapi/servicebroker#193

v2.ProvisionResponse uses json field name operationKey here and provisionSuccessResponseBody uses json field name operation here.

v2.ProvisionResponse could be directly serialized and fed into v2.Client if the json fields b/w client and server match.

The response for PollLastOperation MAY also include the Retry-After HTTP header. This header will indicate how long the Platform SHOULD wait before polling again and is intended to prevent unnecessary, and premature, calls to the last_operation endpoint. It is RECOMMENDED that the header include a duration rather than a timestamp.

I created a broker and it's SSL Certificate was created by the platform with a platform generated CA. It would be nice if the broker client could automatically add platform CA's to its TLS config so brokers could skip registering the CA when it creates the ClusterServiceBroker resource.

details: OpenShift 3.9, CA is mounted in all pods at /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
more at https://docs.openshift.org/latest/dev_guide/secrets.html#service-serving-certificate-secrets

At this time, the kubernetes project is considering archiving this project. It may still be forked and maintained externally, however there does not appear to be enough momentum in project to keep it going.

If you are a user of this project, please chime in on this thread.

would be nice to expose http.Client in a configuration or a custom interface Do(*Request) (*Response, error). can even implement oauth token support via this method.

thoughts?

cc @youreddy

Currently ExtensionAPIs is included as a field on ProvisionResponse: https://github.com/kubernetes-sigs/go-open-service-broker-client/blob/a6136cae557aaec67949f38da4fa9a13e4fcadf4/v2/types.go#L228-L233

This corresponds to an old OSBAPI proposal, openservicebrokerapi/servicebroker#431 but this has been closed in favour of openservicebrokerapi/servicebroker#670 which has also yet to be accepted.

I feel like removing the current ExtensionAPIs seems like a reasonable step and that later on the new proposed API could be re-added as an alpha feature.

what it sounds like

Description

This repo has 18 stale(3years old) branches which were already merged to the master. IMO we safely delete them.

See: https://github.com/kubernetes-sigs/go-open-service-broker-client/branches/stale

If no one will raise any objection I will delete them on May 29th.

We'd like to get this client up to date with the current version of the OSBAPI spec (which is currently on 2.15). This is an interim step to add support for 2.14.

Below is a list of the 2.14 release notes. We plan to look at each feature and determine if the feature has already been implemented or if new changes are required.

• DONE = Feature is released
• ALPHA = implemented behind feature flag
• WORK REQUIRED = not currently implemented - requires work
• N/A = no changes required to client

In terms of implementation we were thinking to PR in new features as Alpha features, then once we think everything has been implemented we can open a final PR to move the features from Alpha and add Version2_14 to the client.

Does that all sound reasonable?

Thanks
@tedddyking and @Samze

... per openservicebrokerapi/servicebroker#222

Has a v1 and v2 release now https://github.com/kubernetes/klog.

Not sure if there is a standard go logging shim, but some may want to use this library outside of a kubernetes context and supply their own logger. This is a much longer term thought.

I'm currently using this library to build a component that's talking to on demand service broker. As written in the comment, the library is missing previous_values field in UpdateInstanceRequest here

This is important missing piece for me because the broker I talk to requires previous_values for updating service instance which is according to the spec. Specifically the broker will look for previous_values here which is currently empty since the library is not passing it on.

If you are ok, I can make a PR for this.

cc @frodenas

There has been some discussion around releasing a new version of go-open-service-broker-client and what version it should have. The point has been made that in order to be compliant with the import path github.com/kubernetes-sigs/go-open-service-broker-client/v2 that we should release a version that is v2.x.y. However in order to do this in go modules it is usual to indicate this in the module defined in go.mod https://github.com/kubernetes-sigs/go-open-service-broker-client/blob/4406bd2cb6b8ace5644ce50f7d4c790da8e00219/go.mod#L1 by updating it to include a /v2. This would result in the actual import path becoming github.com/kubernetes-sigs/go-open-service-broker-client/v2/v2 (since the libraries are currently in a v2 subdirectory). There are two possible solutions to this:

1. Move the contents of v2 to the top level and update go.mod to include v2
2. Make use of the "major version subdirectory strategy", see: https://blog.golang.org/v2-go-modules. This would entail moving go.mod and go.sum into the v2 directory and updating the module name to include /v2 at the end.

Once either of these is done we could cut a new release with v2.x.y.

The current mod file is：

module sigs.k8s.io/go-open-service-broker-client/v2

go 1.14

require k8s.io/klog/v2 v2.0.0

Currently, the PollLastOperation and PollBindingLastOperation functions send originating identity information in their requests. Is this expected behavior?

I realized this week that we have never released this repo and are depending on random shas in service-catalog. We should release this repo. There's a question of what the versioning strategy should be.

I'm thinking something like:

• 2.13 as a base version
• 2.13.x for fixes

I suspect @kibbles-n-bytes @staebler @MHBauer may have some opinions in this area

We had a use case wherein the Kubernetes Service Catalog had to talk to a OSB server via an HTTP proxy. The go OSB HTTP Client does not seem to honour the HTTP proxy environment variables "http_proxy" and "HTTP_PROXY". In package v2(client.go) of this project, a new HTTP transport is created as "&http.Transport{}", and in there the proxy environment variables are being ignored, as the transport.proxy function is not being set. This is unlike the transport http.DefaultTransport(in go http package), which sets the proxy as "ProxyFromEnvironment". Is this omission a carefully considered one or can this can be considered a bug?

Things like https://github.com/pmorie/go-open-service-broker-client/blob/master/v2/fake/fake.go#L285
should check to make sure that fields required by OSB API https://github.com/openservicebrokerapi/servicebroker/blob/v2.13/spec.md#body-2 are provided.

I can't think of a reason we'd want to allow a fake to get and respond to an incorrect request.

This is to be consistent with the upcoming changes in service-catalog.

It would be great to get this client up to date with the current version of the OSBAPI spec (which is now on 2.16). This is an interim step to add support for 2.15.

Below is the list of 2.15 release notes, the plan is to categorise them by whether the work has already been done, work is needed or if it is not relevant to this library.

• DONE = Feature is released
• ALPHA = implemented behind feature flag
• WORK REQUIRED = not currently implemented - requires work
• N/A = no changes required to client

For implementation it would probably make sense to follow the same pattern as for 2.14, #144, i.e. PR in features as Alpha features and once it is all implemented a final PR to move them out of Alpha and add Version2_15 to the client.

Hopefully that sounds reasonable?

This is related to PR #131. Even after 131 was merged in Service Catalog I saw reports that the TCP Connections in ESTABLISHED state continued to climb and result in consuming too many resources. I reproduced and spent a day tracking this down. I finally realized why:

https://github.com/pmorie/go-open-service-broker-client/blob/9cc214e88d00504888af633376c9462e0ab2bd8b/v2/client.go#L41

This is creating a new Transport with bad defaults. In fact, many of them are horrible, ie zero for Timeout, KeepAlive, MaxIdleConnections.... & more

We should pick up the default values as specified in DefaultTransport and make modifications as necessary.

We could hardcode the source for the current defaults from https://github.com/golang/go/blob/b5ed6ec14092b04156adcbaba101958dc9d9d74b/src/net/http/transport.go#L42-L53 but that doesn't set us up well for picking up new versions or fixes. There is this suggestion: golang/go#26013

I'm currently using this library to build a component that's talking to on demand service broker.

In current DeprovisionInstance implementation, we cannot really differentiate http.StatusOK from http.StatusGone: in either case, an empty response and a nil error are returned.

However, we do need this ability to take different actions. Besides, http.StatusGone has a 410 code which is supposed to mean ClientError. So I propose to return an empty response here and a non-nil error(returning a c.handleFailureResponse(response) should be good enough).

What do you think? If you feel ok, I'm happy to make a PR for this.

@pmorie
cc @frodenas