feat: IAS App-to-Service Communication #331

Johannes-Schneider · 2024-02-27T14:36:16Z

Context

SAP/cloud-sdk-java-backlog#373.

This PR adds a new ServiceBindingDestinationLoader implementation that is capable of dealing with (currently) imaginary service bindings from re-use services that use IAS for authentication.

The suggested Service Binding format looks like this:

{
  "credentials": {
    "authentication-service": {
      "service-label": "identity", // mandatory
      "app-name": "my-app-name" // optional
    },
    "endpoints": {
      "some-endpoint": {
        "protocol": "http", // optional, "http" by default
        "uri": "http://some-endpoint.com", // mandatory for "http" protocol
        "always-requires-token": true, // optional, defaults to true
        "requires-mtls": true // optional, defaults to true
      }
    }
  }
}

Feature scope:

Assume and parse a service binding format

Definition of Done

Functionality scope stated & covered
Tests cover the scope above
Error handling created / updated & covered by the tests above
~~Documentation updated~~
~~Release notes updated~~

…-use services

newtork

Progress looks good. I'm only concerned about

customizability (for future JSON format changes)
testability (for current event-broker JSON format)

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

newtork · 2024-02-28T08:19:47Z

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

+        return delegateLoader.tryGetDestination(optionsBuilder.build());
+    }
+
+    private static boolean isIdentityAuthenticationServiceBinding( @Nonnull final ServiceBinding serviceBinding )


(Comment)

I think this is fine for now. I wonder whether we'll need to make this an accessible / modifiable Predicate<ServiceBinding> instead in the future.

As we are working together with the Event Broker colleagues (who are taking a front runner position in the entire IAS topic) to define a standard format for IAS-backed services, I'd hope that we can get around further configuration options in this implementation.

In other words: Services either adhere to our standard or won't be supported by the SDK. As we are the de-facto only library (AFAIK) in the entire SAP ecosystem to support IAS connectivity, I feel we are in a very strong position to "force" our standard onto service providers.

Nevertheless, I don't see any blockers for future extensibility of our implementation in case there are (once again) varying service binding formats.

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

newtork · 2024-02-28T09:28:48Z

...TA-INF/services/com.sap.cloud.sdk.cloudplatform.connectivity.ServiceBindingDestinationLoader

@@ -1 +1,2 @@
 com.sap.cloud.sdk.cloudplatform.connectivity.OAuth2ServiceBindingDestinationLoader
+com.sap.cloud.sdk.cloudplatform.connectivity.IdentityAuthenticationServiceBindingDestinationLoader


(Minor)

Until we have a JSON syntax aligned, let's disable the auto loading.

Any downside to having it loaded? I would argue the matching criteria of having an entry("authentication-service.service-label", "identity") so so specific it is basically impossible to match it by accident to some existing binding..

Initially, Alex and I discussed whether it would be a good idea to auto load this new implementation into all Cloud SDK applications without having a (somewhat) aligned standard for the Service Binding format.

However, as we further refined how we think those service bindings should look like, I'd agree that it doesn't really hurt to already load this new implementation.

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

Johannes-Schneider · 2024-02-29T07:04:01Z

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

+        if( !endpoint.requiresTokenForTechnicalAccess
+            && options.getOnBehalfOf() != OnBehalfOf.NAMED_USER_CURRENT_TENANT ) {
+            optionsBuilder.withOption(BtpServiceOptions.IasOptions.withMutualTlsOnly());


I'm wondering whether this is correct for the "current Tenant is subscriber" case... Probably not, right?

So it should be

Suggested change

if( !endpoint.requiresTokenForTechnicalAccess

&& options.getOnBehalfOf() != OnBehalfOf.NAMED_USER_CURRENT_TENANT ) {

optionsBuilder.withOption(BtpServiceOptions.IasOptions.withMutualTlsOnly());

if( !endpoint.requiresTokenForTechnicalAccess

&& options.getOnBehalfOf() == OnBehalfOf.TECHNICAL_USER_PROVIDER ) {

optionsBuilder.withOption(BtpServiceOptions.IasOptions.withMutualTlsOnly());

As discussed, this implementation has been shifted towards the OAuth2PropertySupplier as this class (the IdentityAuthenticationServiceBindingDestinationLoader) cannot determine whether the current tenant is the provider.

Also see the changes for within the IasOptions class for reference.

Johannes-Schneider · 2024-02-29T07:24:22Z

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

+        if( bindingView.endpoints.size() > 1 ) {
+            log
+                .warn(
+                    "The IAS-based service binding for service '{}' contains multiple HTTP endpoints. Only the first one will be used.",
+                    bindingView.serviceIdentifier);
+        }
+
+        return bindingView.endpoints.get(0);


Discussion

Should we already introduce some API for customers to select the endpoint they want? Or do we wait until we actually encounter a feature request?

(Comment)

CAP reminded us; first element may not necessarily be consistent in JSON format.
Even in Java-case having LinkedHashMap as deserialization object (maybe) it may not be consistent in service binding side.

As discussed: We are now throwing an DestinationAccessException if there is not exactly one HTTP endpoint available.
This behavior will be changed when we introduce new API that lets customers pick the endpoint they want to connect to.

.../src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliers.java

newtork · 2024-03-06T09:15:46Z

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

+            catch( final ValueCastException e ) {
+                // the `service-label` entry is not a string
+                return false;
+            }


(Minor)

Please don't swallow exceptions, without a trace/debug log statement.

I'd argue that it doesn't provide any value if we were to log the exception here since it is a valid outcome that the authentication-service.service-label is not a String (even though the chances are pretty minimal).

Unfortunately, we don't have any API within the SBL to check whether a field is of a certain type so that we have to rely on throwing and catching exception for the moment.
Personally, I'd be happy to introduce such new API, but I don't know if the benefit justifies the effort.

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

MatKuhr · 2024-03-06T15:57:42Z

...TA-INF/services/com.sap.cloud.sdk.cloudplatform.connectivity.ServiceBindingDestinationLoader

@@ -1 +1,2 @@
 com.sap.cloud.sdk.cloudplatform.connectivity.OAuth2ServiceBindingDestinationLoader
+com.sap.cloud.sdk.cloudplatform.connectivity.IdentityAuthenticationServiceBindingDestinationLoader


Any downside to having it loaded? I would argue the matching criteria of having an entry("authentication-service.service-label", "identity") so so specific it is basically impossible to match it by accident to some existing binding..

.../src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliers.java

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

MatKuhr · 2024-03-06T16:15:19Z

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java

+            }
+
+            try {
+                return credentials.getMapView("authentication-service");


Feels a bit bad to have around 200 LoC just for loading a few JSON fields.. Personally I'd probably wrap some of this into a getOrDefault(str, default) method, but not sure how much better that looks..

Or we directly replace this with gson or jackson deserialization maybe?

Feels a bit bad to have around 200 LoC just for loading a few JSON fields

Agreed. Let me see what I can do about it.

MatKuhr · 2024-03-06T16:21:57Z

...dk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoaderTest.java

+
+import io.vavr.control.Try;
+
+@Isolated( "because the tests manipulate the global default ServiceBindingAccessor" )


MatKuhr · 2024-03-06T16:29:32Z

...dk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoaderTest.java

+                .containsExactly(URI.create("https://foo.uri"));
+            assertThat(delegateOptions.getOption(BtpServiceOptions.IasOptions.NoTokenForTechnicalProviderAccess.class))
+                .contains(true);
+            assertThat(delegateOptions.getOption(BtpServiceOptions.IasOptions.IasCommunicationOptions.class)).isEmpty();


these assertions are the same as for the case above, right? If so, what does this test really assert, other than the behalf not impacting the assertions? Maybe we can reduce the code duplication between these test cases still a bit to make it clearer what the differences are between them?

...nnectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServiceOptions.java

MatKuhr

lgtm 👍🏻

feat: implement new ServiceBindingDestinationLoader for IAS-backed re…

46143ae

…-use services

Johannes-Schneider added do not merge Pull request must not be merged please review Request to review a pull request labels Feb 27, 2024

Johannes Schneider added 2 commits February 27, 2024 15:40

refactor: move endpoint selection logic into its own method

e66db31

fix: dependency issues

365fa5f

newtork reviewed Feb 28, 2024

View reviewed changes

Johannes Schneider added 2 commits February 28, 2024 09:41

refactor: replace odd casing for mTLS flags

28b79d9

Merge branch 'main' into feat/ias-app-to-service

e80c6af

newtork reviewed Feb 28, 2024

View reviewed changes

...ud/sdk/cloudplatform/connectivity/IdentityAuthenticationServiceBindingDestinationLoader.java Outdated Show resolved Hide resolved

Johannes Schneider added 7 commits February 28, 2024 14:59

refactor: adjust assumed service binding format

985fff9

Merge branch 'main' into feat/ias-app-to-service

f5dc62b

fix: missing javadocs

13af2f2

fix: pmd warning

593880a

Merge branch 'main' into feat/ias-app-to-service

d2adb6d

chore: update since tag

8098014

Merge branch 'main' into feat/ias-app-to-service

d67c5fc

Johannes-Schneider commented Feb 29, 2024

View reviewed changes

Johannes Schneider added 2 commits February 29, 2024 10:40

refactor: slightly adjust service binding parsing

9b26688

refactor: address review feedback

d100b06

Johannes-Schneider marked this pull request as ready for review March 1, 2024 08:36

Johannes-Schneider added please merge Request to merge a pull request and removed do not merge Pull request must not be merged labels Mar 1, 2024

fix: PMD warning

c9284b6

newtork reviewed Mar 6, 2024

View reviewed changes

.../src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/BtpServicePropertySuppliers.java Outdated Show resolved Hide resolved

newtork reviewed Mar 6, 2024

View reviewed changes

Merge branch 'main' into feat/ias-app-to-service

aebdfc0

MatKuhr reviewed Mar 6, 2024

View reviewed changes

refactor: address review feedback

68238dd

Johannes Schneider and others added 4 commits March 7, 2024 14:22

refactor: rename Ias option

f0136ff

fix: compilation warnings

0de924e

fix: compilation warnings

05da217

Merge branch 'main' into feat/ias-app-to-service

277727d

MatKuhr approved these changes Mar 8, 2024

View reviewed changes

MatKuhr merged commit 2d9b1ee into main Mar 8, 2024
16 checks passed

MatKuhr deleted the feat/ias-app-to-service branch March 8, 2024 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: IAS App-to-Service Communication #331

feat: IAS App-to-Service Communication #331

Johannes-Schneider commented Feb 27, 2024 •

edited by MatKuhr

Loading

newtork left a comment

newtork Feb 28, 2024

Johannes-Schneider Feb 28, 2024

newtork Feb 28, 2024

MatKuhr Mar 6, 2024

Johannes-Schneider Mar 7, 2024

Johannes-Schneider Feb 29, 2024

Johannes-Schneider Mar 1, 2024

Johannes-Schneider Feb 29, 2024 •

edited

Loading

newtork Feb 29, 2024 •

edited

Loading

Johannes-Schneider Mar 1, 2024

newtork Mar 6, 2024 •

edited

Loading

Johannes-Schneider Mar 7, 2024

MatKuhr Mar 6, 2024

MatKuhr Mar 6, 2024

MatKuhr Mar 6, 2024

Johannes-Schneider Mar 7, 2024

MatKuhr Mar 6, 2024

MatKuhr Mar 6, 2024

MatKuhr left a comment

		@@ -1 +1,2 @@
		com.sap.cloud.sdk.cloudplatform.connectivity.OAuth2ServiceBindingDestinationLoader
		com.sap.cloud.sdk.cloudplatform.connectivity.IdentityAuthenticationServiceBindingDestinationLoader


		import io.vavr.control.Try;

		@Isolated( "because the tests manipulate the global default ServiceBindingAccessor" )

feat: IAS App-to-Service Communication #331

feat: IAS App-to-Service Communication #331

Conversation

Johannes-Schneider commented Feb 27, 2024 • edited by MatKuhr Loading

Context

Feature scope:

Definition of Done

newtork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johannes-Schneider Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

newtork Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newtork Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatKuhr left a comment

Choose a reason for hiding this comment

Johannes-Schneider commented Feb 27, 2024 •

edited by MatKuhr

Loading

Johannes-Schneider Feb 29, 2024 •

edited

Loading

newtork Feb 29, 2024 •

edited

Loading

newtork Mar 6, 2024 •

edited

Loading