Add Service Brokers Endpoints

[theghost5800]

Resolve #1076

[twoseat]

Thanks for the PR @theghost5800! Unfortunately I've just pushed an implementation of service offerings, so if you could remove that part and resubmit I'll get started on a merge.

[theghost5800]

Okay, I'll remove service offering part and update the PR.

[twoseat]

I took a pass through and everything looks great, with one exception. You're absolutely right that serviceInstance should be serviceinstance. Unfortunately we're strict about backwards compatibility, so it's not a change I can make in an existing major version. The good news is that I'll be releasing a 5.x line in the next few days. So if you could withdraw the serviceinstance change here I can accept this PR (once I've added integration tests), and will be sure to fix that issue in 5.x

[theghost5800]

Hello, did you have some comments on last changes? Also did you expect from me to fix merge conflicts or you will fix them? Probably they are related with package renaming.

[twoseat]

Hi @theghost5800 No comments, just working through PRs at the moment. No need to resolve conflicts, I'll handle that.

[twoseat]

(Edited title to help with tracking)

[theghost5800]

As I can see, these changes are not comitted in main brach. So when I can expect these changes to be merged in main branch and released?

[theghost5800]

Hi @dmikusa-pivotal ,
I have opened PR since end of last year and I am still waiting this contribution to be accepted in your repository. I would like to ask what else I have to do to accept this PR? Do you want to fix merge conflicts?

[dmikusa]

@theghost5800 My apologies & thanks for the PR. We've had some changes on the team and this has caused some delays with some PRs/Issues. In particular, CI has not been in good shape. I have been working through that and getting it fixed.

If you could fix up the merge conflicts on this PR, I will take a look and review it for you. Thanks for your time, it is appreciated!

[theghost5800]

Hi @dmikusa-pivotal , I have updated PR with fixed merge conflicts, so now you can take a look and if you want to make some changes, please let me know.

[dmikusa]

Thanks again for the PR. It looks good. A couple points of feedback:

1. There are a handful of files like CloudFoundryCleaner.java and ServiceInstancesTest.java (and some others) where the only changes appear to be the reordering of imports. This sometimes happens with IDE code formatters. In general, we don't want code like that checked in. It's confusing because it complicates the PR an can make merges harder. Please go and exclude any non-material changes like space-only changes to unaltered code or reordering imports in otherwise unaltered files. That should shrink the surface area of this PR.

2. There need to be integration tests. There needs to be a client-specific test for the broker here, something like ServiceBrokersTest.java.

   It's not an easy aspect to test because you actually need some sort of service broker, so you can add the broker to the CF to test it. If you check out ServiceBrokerUtils, I think this might be enough to get what you need.

Thanks again, let me know if you have questions on this feedback.

[theghost5800]

Hi @dmikusa-pivotal , I fixed files with switched imports and try to copy existing ServiceBrokersTest from v2 package and adapt it to v3 but I don't have cf environment with admin access where I can run integration tests. Could you please try to execute this new integration tests on your environment? As far as I’ve heard previosly was not necessary contributors to write integration tests because there is no other options if you don't have CF environment with admin access.

[dmikusa]

Thanks. We need to have integration tests before anything can be merged. You can write them or I can write them. If you are unable to provide them, I can write the tests but I cannot promise priority to that work so it will be much quicker to merge this PR if you are able to provide them.

The tests do require full admin access to a CF installation. If you do not have access to one, you can run one locally using bosh-deployment & cf-deployment & Virtualbox.

1. See Install Section to install Bosh.
2. Follow the deployment guide to get CF installed. I think you can skip to step 4, since you're installing locally. Definitely read this section before you get started though. It has some info for running locally.

Before you start the tests, I would recommend taking a snapshot of the VMs in the environment. That way you can quickly restore back to the initial state (tests should clean up after themselves, but this gives you another way to refresh your env).

Thanks

[dmikusa]

@theghost5800 I took these and ran them locally against a CF foundation & got some errors. Here's the output:

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.cloudfoundry.client.v2.ServiceBrokersTest
14:01:11.569 reactor-http-nio-2          cloudfoundry-client.compatibility     Client supports API version 2.150.0 and is connected to server with API version 2.164.0. Things may not work as expected.
14:01:11.615 main                        cloudfoundry-client.test              CloudFoundryCleaner.clean is running
14:01:15.995 reactor-http-nio-7          cloudfoundry-client.compatibility     Client supports API version 2.150.0 and is connected to server with API version 2.164.0. Things may not work as expected.
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 257.686 s - in org.cloudfoundry.client.v2.ServiceBrokersTest
[INFO] Running org.cloudfoundry.client.v3.ServiceBrokersTest
[ERROR] Tests run: 5, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 131.304 s <<< FAILURE! - in org.cloudfoundry.client.v3.ServiceBrokersTest
[ERROR] create  Time elapsed: 41.075 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onComplete())

[ERROR] update  Time elapsed: 43.814 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onComplete())

14:07:37.756 SpringContextShutdownHook   cloudfoundry-client.test              CloudFoundryCleaner.clean is running
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR]   ServiceBrokersTest.create expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onComplete())
[ERROR]   ServiceBrokersTest.update expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onComplete())
[INFO]
[ERROR] Tests run: 10, Failures: 2, Errors: 0, Skipped: 0

There's not a lot to go on here. It just seems like it's expecting the broker to exist in the test, but it doesn't exist so fails.

[theghost5800]

Hi @dmikusa-pivotal ,
Thank you for running integration tests for me. I am trying now to setup local CF environment but unfortunately it is not allowed to use Virtualbox for VM management. Can you suggest me some other alternative for CF local setup without usage of virtualbox(for example docker or hyper-v on windows 10?

[dmikusa]

You can basically use anything Bosh supports, but most of those are public cloud/not something you run on your laptop. I think Virtualbox is the only one that's feasible to run on a laptop/desktop.

You could potentially try https://github.com/cloudfoundry/cf-for-k8s which runs on K8s. That has a section for deploying locally, https://cf-for-k8s.io/docs/deploying/deploy-local/. It should be compatible with cf-java-client, but we don't test it in our CI pipelines.

[theghost5800]

Hello @dmikusa-pivotal , I have tried to execute steps related with setup bosh-lite and deploy local CF using Virtualbox. I had successfully execute steps in this link https://bosh.io/docs/bosh-lite/ and now I have bosh vm director but when I try to deploy CF from this guide https://github.com/cloudfoundry/cf-deployment/blob/main/texts/deployment-guide.md#for-operators-deploying-cf-to-local-bosh-lite , I have always receive a lot of errors related with missing configurations in cloud-config.yml file(I am trying to use this one https://github.com/cloudfoundry/bosh-deployment/blob/master/virtualbox/cloud-config.yml). Tried some of them to add/edit them but again receive another errors related with network configuration(for example missing vm_extensions or some static configured ip described here https://github.com/cloudfoundry/cf-deployment/blob/main/operations/bosh-lite.yml doesn't belong to any subnets). So do you know some guide or exact descriptors which I can use to deploy CF locally? Just to mention that I am using master branches of cf-deployment and bosh-deployment repos.

[dmikusa]

Sorry, I can't really add anything beyond what's in the docs. I would suggest not running on main/master though. Pick one of the tagged releases. Those coincide with release versions. It's possible that main/master isn't in a good state.

[theghost5800]

Hi @dmikusa-pivotal I find out where is the issue with create and update tests. It looks like list request which assert that create/update operation is successful doesn't wait previous operation to finish and this cause result from list requests to be empty. Now it should be wait until first operation is executed and then verify operation. You can trigger your test pipeline including latest changes in the PR. I hope now everything will be okay and the PR can be merged.

[dmikusa]

Many thanks, @theghost5800! I will try to run these through our integration env on Mon or Tue next week.

[dmikusa]

I took a look at this today and there is still a similar error on the update integration test.

I suspect that what is happening is because the cloud controller API for service brokers may return immediately and process the job asynchronously.

The Location header refers to the created job which syncs the broker with the catalog. See Service broker jobs for more information and limitations.

http://v3-apidocs.cloudfoundry.org/version/3.101.0/index.html#create-a-service-broker

The update API has the same note:

This endpoint updates a service broker. Depending on the parameters specified, the endpoint may respond with a background job, and it may synchronize the service offerings and service plans with those in the broker’s catalog.

http://v3-apidocs.cloudfoundry.org/version/3.101.0/index.html#update-a-service-broker

In fact that one is a little more confusing cause it says that it MAY respond with a background job, so sometimes it will, sometimes it won't. The docs say that a 200 OK will indicate an immediate response, while a 202 Accepted indicates a background job. I capture the HTTP request/response data and it is returning a 202 in my test, so that's why I suspect that this isn't working for me. It seems like it's not handling that case where the response is non-immediate.

16:17:39.098 reactor-http-nio-7          cloudfoundry-client.request           PATCH  /v3/service_brokers/aa63bb62-f3b0-4ef5-8e86-0177e3c75101
16:17:39.165 reactor-http-nio-7          cloudfoundry-client.response          202    /v3/service_brokers/aa63bb62-f3b0-4ef5-8e86-0177e3c75101 (67 ms, b6ee9360-d184-47b0-65eb-04f62272fb7f::164efe08-8ec5-45d7-9a24-5eba233cb337)

It looks like delete does the same. I'm not sure why delete & create work, it might be just a timing thing in my environment, not sure.

I think we want this to be handled similar to other endpoints that return a Job, see Route and that uses this method instead which is returning a job id, so the client could monitor the Job and wait for it to complete. This helper also exists for post(..), but I don't see one for put(..) or patch(..) (where it pulls out the job id). I think that would need to be added.

http://v3-apidocs.cloudfoundry.org/version/3.101.0/index.html#asynchronous-operations

For the integration tests, the one test that I could find that appears to be doing this correctly is delete for Apps. I would suggest doing something like that where it uses JobUtils to ensure correct polling of the resulting Job. When the job is done, you can do the current logic of checking for the list of service brokers filtered by the new name.

[theghost5800]

Hi @dmikusa-pivotal , I have make changes about extraction of job id of async operations and use JobUtils to poll such async operations. Could you please get latest changes in PR and run integration tests in your environment.

[dmikusa]

I got some different failures this time:

[ERROR] Tests run: 5, Failures: 3, Errors: 0, Skipped: 0, Time elapsed: 263.263 s <<< FAILURE! - in org.cloudfoundry.client.v3.ServiceBrokersTest
[ERROR] create  Time elapsed: 53.091 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onError(org.cloudfoundry.client.v3.ClientV3Exception: CF-UnprocessableEntity(10008): Unknown field(s): 'authentication', Credentials must be a hash, Credentials type credentials.type must be one of ["basic"], Credentials data must be a hash, Credentials data Field(s) ["username", "password"] must be valid: ["Username must be a string", "Password must be a string"]))

[ERROR] delete  Time elapsed: 45.432 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectComplete" failed (expected: onComplete(); actual: onError(org.cloudfoundry.client.v3.ClientV3Exception: CF-NotFound(10000): Unknown request))

[ERROR] update  Time elapsed: 49.466 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onError(org.cloudfoundry.client.v3.ClientV3Exception: CF-NotFound(10000): Unknown request))

[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR]   ServiceBrokersTest.create expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onError(org.cloudfoundry.client.v3.ClientV3Exception: CF-UnprocessableEntity(10008): Unknown field(s): 'authentication', Credentials must be a hash, Credentials type credentials.type must be one of ["basic"], Credentials data must be a hash, Credentials data Field(s) ["username", "password"] must be valid: ["Username must be a string", "Password must be a string"]))
[ERROR]   ServiceBrokersTest.delete expectation "expectComplete" failed (expected: onComplete(); actual: onError(org.cloudfoundry.client.v3.ClientV3Exception: CF-NotFound(10000): Unknown request))
[ERROR]   ServiceBrokersTest.update expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onError(org.cloudfoundry.client.v3.ClientV3Exception: CF-NotFound(10000): Unknown request))
[INFO]
[ERROR] Tests run: 5, Failures: 3, Errors: 0, Skipped: 0

The create failure is caused by a 422 response from the create request.

15:59:42.567 reactor-http-nio-7          cloudfoundry-client.request           POST   /v3/service_brokers
15:59:42.624 reactor-http-nio-7          cloudfoundry-client.response          422    /v3/service_brokers (57 ms, ce25b3d2-43d2-4442-65c7-8492d9610a49::88ceae5c-d864-4b6a-9903-2a28bde6f4cc)

The other two are 404's.

16:33:53.839 reactor-http-nio-7          cloudfoundry-client.request           DELETE /v3/service_brokers/8fc4058f-084c-48da-bab8-3f47cd5a44b4
16:33:53.888 reactor-http-nio-7          cloudfoundry-client.response          404    /v3/service_brokers/8fc4058f-084c-48da-bab8-3f47cd5a44b4 (49 ms, 409e37ad-fa0c-4c23-6e0f-101b53508780::272d1450-c70b-44fb-bd57-

Those were run against a TAS 2.11 environment, with API versions:

   "api_version": "2.139.0",
   "osbapi_version": "2.15",

Apologizes, as I didn't have time to dig into them further so I'm not sure quite what is causing those failures.

[theghost5800]

Hi @dmikusa-pivotal , I can see that api version is quite old. If I am not wrong, it is coming with CAPI 1.84.0 which includes v3 version 3.74.0. From documentation about version 3.74.0, it seems that endpoints for update and delete of service broker are not included and create endpoint requires other fields which are changed in latest versions of capi 3.102.0. Could you please try to run integration tests on newer CF version.

[dmikusa]

Hmm, I can take another look. I believe that was run against TAS 2.11, which should be very recent.

[theghost5800]

I found out that Update service broker response can be different based on what will be updated. In case of update of metadata.labels or metadata.annotations then it won't return Location header including link to job and it will return updated entity itself. In such case, what result should return cf java client? It looks like such scenario is not covered in the code right now. What will be your suggestion about this issue? Because currently if someone tries to prepare update request of service broker metada only, it will throw IllegalArgumentException during call of UriComponentsBuilder.fromUriString method in extractJobId. Do you think that it will be okay to refactor extractJobId to return Mono<String> object and in case of nullable id to return Mono.empty()?

[dmikusa]

I found out that Update service broker response can be different based on what will be updated. In case of update of metadata.labels or metadata.annotations then it won't return Location header including link to job and it will return updated entity itself.

🤦

In such case, what result should return cf java client? It looks like such scenario is not covered in the code right now. What will be your suggestion about this issue?

I'm not 100% sure. I'm trying to think of things from the perspective of a user of the library. What would make sense if you're trying to update a service broker in your code? What results in an easy/smooth usable API?

A couple of thoughts:

• We could have an UpdateServiceBrokerResponse type. The type could signal the result of the action in some way. Perhaps an Optional jobId string that would be set for the case where a job is returned or an Optional ServiceBroker class if it's not populated? (or use some other way to use the types to indicate that you have a mutually exclusive response)

• I could also see perhaps two different update methods? update(..) or updateMetadata(..) where one returns the jobid and the other returns the updated object. This approach puts more burden on the user though, as they'd need to know which method to call, so I'm less of a fan of this one.

I'm open to other suggestions. I just want to make sure the experience of using the API is clean.

Because currently if someone tries to prepare update request of service broker metada only, it will throw IllegalArgumentException during call of UriComponentsBuilder.fromUriString method in extractJobId. Do you think that it will be okay to refactor extractJobId to return Mono<String> object and in case of nullable id to return Mono.empty()?

I wouldn't be in favor of this approach because typically an update request would return the updated object. If I understand what you're proposing, that would mean the case where it would return a Mono.empty() would be when there's a metadata only update and it returns immediately. It seems like that case should return the updated object.

[theghost5800]

Hi @dmikusa-pivotal , I have push new commit which implement mechanism to return jobid and service broker object as optional. I have test it manually and add additional integration test about this.

[dmikusa]

Sorry for the delay @theghost5800. I ran the tests and they all passed, so I wanted to dig in a little more and play with the API changes too. Looks good. I'll merge shortly.