Service key support

[gberche-orange]

This PR add ability for ServiceInstanceAppBindingWorkflow impls to use service keys to expose some backing services credentials (see #285 for more background about related use-case). It includes CT and AT.

Other improvements were added to the AT:

• documentation enhancements
• logging deployed broker recents logs

[gberche-orange]

Started rebasing and fixing new PMD and checkstyle rules from #291 Still some checkstyle fixes to be brought

Original PR before is available at https://github.com/orange-cloudfoundry/osb-cmdb-spike/tree/service-key-support-before-spring-style-rebase

[Albertoimpl]

Thanks for the contribution, would you mind rebasing this PR @gberche-orange? Thanks!

[gberche-orange]

@Albertoimpl the PR was rebased against master and adapted to new checkstyles and pmd rules. I'm looking forward to your review.

[Albertoimpl]

Please, delete comment to be consistent with the other methods.

[gberche-orange]

As the content of the returned value is assigned deep in CloudFoundryAppDeployer, and felt not that intuitive obvious to newcomers, I rather added javadoc on all methods of the class as to still preserve class consistency. Please let me know if you have a better suggestion on how to remove ambiguity to code readers on the intent and content of the returned type.

[Albertoimpl]

Please, delete comment to be consistent with the other methods.

[gberche-orange]

Same as https://github.com/spring-cloud/spring-cloud-app-broker/pull/292/files#r392504267

[Albertoimpl]

Thanks a lot for the contribution! I don't see any problem with the implementation nor with the way it is implemented, apart from the lack of ATs and CTs that should be included and the picky notes about the comments I left.

There are a couple of things that do not feel right but I am happy to re-evaluate them in a future release when we start implementing the k8s support.

• You can achieve this behaviour by just implementing a CreateServiceInstanceWorkflow.
• In a k8s world, there is not a direct mapping to a Service Key, so leaking that name, that is only CF-related, as part of the AppDeployer interface, which is a generic one, doesn't feel right.

As said, if you add the ATs and CTs, I am happy with including it. Thanks a lot for the great work!

[gberche-orange]

Thanks @Albertoimpl for your review. I'll try working on providing the CT and AT now that I see there is interest.

You can achieve this behaviour by just implementing a CreateServiceInstanceWorkflow.

Actually, I'm currently using the service key support in a custom CreateServiceInstanceAppBindingWorkflow, see osbcmdb/AppDeploymentCreateServiceBindingWorkflow.java. I first considered implementing this without SCAB support, but this required to duplicate some of the existing beans such as DefaultBackingServicesProvisionService and CloudFoundryAppDeployer, so I'm happy if this can make it into SCAB and be useful to others.

In a k8s world, there is not a direct mapping to a Service Key, so leaking that name, that is only CF-related, as part of the AppDeployer interface, which is a generic one, doesn't feel right.

Yes. I understand that the service key could map to a K8S service cat ServiceBinding and fetching credentials from the associated Secret, see https://kubernetes.io/docs/concepts/extend-kubernetes/service-catalog/#mapping-the-connection-credentials These pieces of information are stored in secrets that the application in the cluster can access and use to connect directly with the managed service.

So potentially, a K8S-deployer implementation of the service-key methods could be brought in the future. I do agree that then the name "ServiceKey" could be more generic such as "ServiceCredentials".

[Albertoimpl]

Great to hear!

Thanks @Albertoimpl for your review. I'll try working on providing the CT and AT now that I see there is interest.

Looking forward to reviewing them. I typically start working always with an AT, then a CT and then I start coding the rest.

DefaultBackingServicesProvisionService and CloudFoundryAppDeployer, so I'm happy if this can make it into SCAB and be useful to others.

Perfect! Thanks for contributing. I am sure this will benefit others.

So potentially, a K8S-deployer implementation of the service-key methods could be brought in the future. I do agree that then the name "ServiceKey" could be more generic such as "ServiceCredentials".

It would be ideal if you made this rename.

Again, thanks a lot for your contributions!

[gberche-orange]

@Albertoimpl Sorry for long silence on this PR. I've rebased the PR and added component test covering service key usage, and addressed suggested changes in the review. Can you please have a new look ?

Note that I'm struggling with checkstyle import rule on https://github.com/orange-cloudfoundry/osb-cmdb-spike/blob/5016f4f5b1bd28d7ba9066bdfe3bc766b8ce397f/spring-cloud-app-broker-integration-tests/src/test/java/org.springframework.cloud.appbroker/integration/CreateBindingWithServiceKeyComponentTest.java#L54

• Besides, gradle checkstyle fails on my development box with symptom
  Unable to create Root Module: config {.../src/checkstyle/checkstyle.xml}, which looks similar to https://stackoverflow.com/questions/49645147/checkstyle-unable-to-create-root-module which I need to pursue. Any hint on fixing this would help, as waiting for circleci builds feedback implies a 15 min feedback time.
• I tried to follow the CONTRIBUTING.adoc which recommends usage of the eclipse code formatter, and applied the intellij "optimize import" action. Is there other automated import formatting that the app broker team uses within intellij ?

I'll now try to provide an acceptance test. Is there a way I would be able see test results associated with the PR for acceptance tests (such as making the associated concourse pipeline public or adding 3rd party contributors as viewers) ?

[gberche-orange]

@Albertoimpl

The AT asks for a service key on the brokered service instance (i.e. an OSB service binding call with an empty appId in the bindResource), hence requires a service binding workflow. I have therefore made this workflow available in core (for integration-tests and acceptance-tests) to use, and leave it up to app broker users to in their spring configuration classes. This avoids having service key workflow fixtures in both integration and acceptance tests.

[gberche-orange]

Identified steps for contributing the AT

• set up the local environment to run the test
  • Refine the documentation which is missing the required authorities for the UAA client
• execute existing test cases
• develop new test
  • handle clean up of provisioned element on test failures
    • reverse engineer and document element provisionned
      • add debugging traces
    • document clean up script manually invoked
    • improve the CloudFoundryAcceptanceTest to be resilient to failed/interrupted tests that did not execute cleanup
  • troubleshoot failed test
    • access broker logs
    • configure broker logs verbosity
  • Add support for service keys in assertions
  • configure broker used in acceptance test with service binding workflow
    • understand how/when NoOp*Workflow in acceptance-tests are used by the app broker: accept() clause filters the service plan Id
  • Use a backing service which support service keys and returns credentials
    • replace NoOpServiceInstanceBindingService with service binding workflows
• execute the new test to get test status
  • import concourse ci pipeline

[Albertoimpl]

Thanks a lot for the contributions, it is looking great!
We added a different style guide than the documented in CONTRIBUTING, sorry about that, we are going to fix it: #345 and you can find all the check styles over here: https://github.com/spring-cloud/spring-cloud-app-broker/tree/master/src

Regarding the ATs, we have them ready to be public, we need to move the pipeline to a public concourse instance: #346
However, you should be able to run them in your own environments. Let me know if you find any issues with that.

Thanks!

[gberche-orange]

This osb-cmdb related feature seems to not make much sense into app-broker use-cases. Closing it.

Thanks however @Albertoimpl for your past review and support on this PR !