From d6a45baddea26a05e200308cb1e3dac29e7bcfa9 Mon Sep 17 00:00:00 2001 From: Matthias Kuhr <52661546+MatKuhr@users.noreply.github.com> Date: Tue, 27 Feb 2024 10:41:50 +0100 Subject: [PATCH] fix: [Destinations] Allow for Custom Proxy Config in ON_PREMISE Destinations (#327) --- ...acliteServiceBindingDestinationLoader.java | 15 ++++++----- ...teServiceBindingDestinationLoaderTest.java | 23 ++++++++++++++++ ...OAuth2ServiceBindingDestinationLoader.java | 8 ++++-- ...h2ServiceBindingDestinationLoaderTest.java | 26 +++++++++++++++++++ release_notes.md | 1 + 5 files changed, 65 insertions(+), 8 deletions(-) diff --git a/cloudplatform/connectivity-dwc/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/MegacliteServiceBindingDestinationLoader.java b/cloudplatform/connectivity-dwc/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/MegacliteServiceBindingDestinationLoader.java index 62338e80f..bd7d5353d 100644 --- a/cloudplatform/connectivity-dwc/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/MegacliteServiceBindingDestinationLoader.java +++ b/cloudplatform/connectivity-dwc/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/MegacliteServiceBindingDestinationLoader.java @@ -92,18 +92,21 @@ private Try tryGetConnectivityDestination( private Try toProxiedDestination( @Nonnull final HttpDestination base ) { + final DefaultHttpDestination.Builder builder = + DefaultHttpDestination + .fromDestination(base) + // be sure to use exactly this instance of connectivityResolver since it has a cache attached + .headerProviders(connectivityResolver); + // don't override the proxy URL if it has been set explicitly/manually already + if( base.getProxyConfiguration().isDefined() ) { + return Try.of(builder::buildInternal); + } final Try proxyUrl = Try.of(connectivityResolver::getProxyUrl); if( proxyUrl.isFailure() ) { return Try .failure( new DestinationAccessException("Failed to resolve on-premise proxy URL.", proxyUrl.getCause())); } - final DefaultHttpDestination.Builder builder = - DefaultHttpDestination - .fromDestination(base) - // be sure to use exactly this instance of connectivityResolver since it has a cache attached - .headerProviders(connectivityResolver); - return proxyUrl.map(builder::proxy).map(DefaultHttpDestination.Builder::buildInternal); } diff --git a/cloudplatform/connectivity-dwc/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/MegacliteServiceBindingDestinationLoaderTest.java b/cloudplatform/connectivity-dwc/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/MegacliteServiceBindingDestinationLoaderTest.java index 7dbd1facf..6e852c29b 100644 --- a/cloudplatform/connectivity-dwc/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/MegacliteServiceBindingDestinationLoaderTest.java +++ b/cloudplatform/connectivity-dwc/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/MegacliteServiceBindingDestinationLoaderTest.java @@ -22,6 +22,7 @@ import org.apache.http.HttpHeaders; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -315,6 +316,28 @@ void testConnectivityWithProviderTenant() assertThatThrownBy(() -> sut.getDestination(options)).isInstanceOf(DestinationAccessException.class); } + @Test + @DisplayName( "An already existing proxy configuration should not be overridden." ) + void testConnectivityWithExistingProxyConfig() + { + final DefaultHttpDestination destination = + DefaultHttpDestination + .builder("foo") + .proxy(URI.create("http://bar")) + .proxyType(ProxyType.ON_PREMISE) + .buildInternal(); + + final ServiceBindingDestinationOptions options = + ServiceBindingDestinationOptions + .forService(MegacliteServiceBindingAccessor.CONNECTIVITY_BINDING) + .withOption(ProxyOptions.destinationToBeProxied(destination)) + .build(); + final HttpDestination result = sut.getDestination(options); + + assertThat(result.getProxyConfiguration()).isNotEmpty(); + assertThat(result.getProxyConfiguration().get().getUri()).isEqualTo(URI.create("http://bar")); + } + @Test void testGetMandateConfiguration() { diff --git a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoader.java b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoader.java index b098e666e..269d7519b 100644 --- a/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoader.java +++ b/cloudplatform/connectivity-oauth/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoader.java @@ -269,7 +269,7 @@ HttpDestination toProxiedDestination( final String destinationName = destinationToBeProxied.get(DestinationProperty.NAME).getOrElse(""); final DefaultHttpDestination.Builder destinationBuilder = - DefaultHttpDestination.fromDestination(destinationToBeProxied).proxy(proxyUrl); + DefaultHttpDestination.fromDestination(destinationToBeProxied); if( oAuth2Options.skipTokenRetrieval() ) { log.debug("Skipping OAuth2 token retrieval for proxied destination '{}'.", destinationName); @@ -293,7 +293,11 @@ HttpDestination toProxiedDestination( destinationBuilder.keyStore(oAuth2Options.getClientKeyStore()); } - return destinationBuilder.buildInternal(); + // don't override the proxy URL if it has been set explicitly/manually already + if( destinationToBeProxied.getProxyConfiguration().isDefined() ) { + return destinationBuilder.buildInternal(); + } + return destinationBuilder.proxy(proxyUrl).buildInternal(); } DestinationHeaderProvider createHeaderProvider( diff --git a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java index 3f1408eee..b850b8429 100644 --- a/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java +++ b/cloudplatform/connectivity-oauth/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/OAuth2ServiceBindingDestinationLoaderTest.java @@ -26,6 +26,7 @@ import org.apache.http.HttpHeaders; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.mockito.InOrder; import org.mockito.Mockito; @@ -404,6 +405,31 @@ void testProxiedDestination() eq(TEST_SERVICE)); } + @Test + @DisplayName( "An already existing proxy configuration should not be overridden." ) + void testConnectivityWithExistingProxyConfig() + { + final DefaultHttpDestination destination = + DefaultHttpDestination + .builder("foo") + .proxy(URI.create("http://bar")) + .proxyType(ProxyType.ON_PREMISE) + .buildInternal(); + final HttpDestination result = + sut + .toProxiedDestination( + destination, + URI.create("yolo"), + URI.create("foo"), + credentials, + OnBehalfOf.TECHNICAL_USER_CURRENT_TENANT, + OAuth2Options.DEFAULT, + TEST_SERVICE); + + assertThat(result.getProxyConfiguration()).isNotEmpty(); + assertThat(result.getProxyConfiguration().get().getUri()).isEqualTo(URI.create("http://bar")); + } + @Test void testSkipTokenRetrieval() { diff --git a/release_notes.md b/release_notes.md index c706824b8..b9caaa882 100644 --- a/release_notes.md +++ b/release_notes.md @@ -16,6 +16,7 @@ ### 📈 Improvements +- Improve the `DefaultHttpDestination` builder API: For destinations with proxy type `ON_PREMISE` the proxy URL can now be customized by using the `proxy` method of the builder. - Dependency Updates: - SAP dependency updates: - Update [thing](https://link-to-thing) from `a.b.c` to `x.z.y`