Skip to content

Commit d9f63cb

Browse files
MatKuhrMatKuhr
committed
Improve error handling for Missing Connectivity Proxy information
1 parent a7fe631 commit d9f63cb

File tree

6 files changed

+99
-113
lines changed

6 files changed

+99
-113
lines changed

‎cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java

+4-5
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ private Collection<Header> getHeadersForAuthType()
224224
.stream()
225225
.filter(Objects::nonNull)
226226
.map(value -> new Header(HttpHeaders.AUTHORIZATION, value))
227-
.collect(Collectors.toList());
227+
.toList();
228228

229229
if( headersToAdd.isEmpty() ) {
230230
final String msg =
@@ -972,10 +972,10 @@ public DefaultHttpDestination build()
972972
throw new IllegalArgumentException("Cannot build a HttpDestination without a URL.");
973973
}
974974

975-
// NOT using the typed property here since this would break change detection in our ScpCfDestinationLoader
976-
property(DestinationProperty.TYPE.getKeyName(), DestinationType.HTTP.toString());
975+
if( get(DestinationProperty.TYPE).isEmpty() ) {
976+
property(DestinationProperty.TYPE, DestinationType.HTTP);
977+
}
977978

978-
// handle proxy type == OnPremise
979979
if( builder.get(DestinationProperty.PROXY_TYPE).contains(ProxyType.ON_PREMISE) ) {
980980
try {
981981
return proxyHandler.handle(this);
@@ -986,7 +986,6 @@ public DefaultHttpDestination build()
986986
log.error(msg, e);
987987
}
988988
}
989-
990989
return buildInternal();
991990
}
992991

‎cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestinationBuilderProxyHandler.java

+34-45
Original file line numberDiff line numberDiff line change
@@ -7,35 +7,26 @@
77
import java.util.Collections;
88
import java.util.List;
99
import java.util.function.Predicate;
10-
import java.util.stream.Collectors;
1110

1211
import javax.annotation.Nonnull;
13-
import javax.annotation.Nullable;
1412

1513
import com.sap.cloud.environment.servicebinding.api.DefaultServiceBindingAccessor;
1614
import com.sap.cloud.environment.servicebinding.api.ServiceBinding;
1715
import com.sap.cloud.environment.servicebinding.api.ServiceBindingAccessor;
1816
import com.sap.cloud.environment.servicebinding.api.ServiceIdentifier;
1917
import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationAccessException;
2018
import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationNotFoundException;
19+
import com.sap.cloud.sdk.cloudplatform.exception.ShouldNotHappenException;
2120
import com.sap.cloud.sdk.cloudplatform.security.AuthTokenAccessor;
2221

2322
import io.vavr.control.Option;
24-
import lombok.AccessLevel;
2523
import lombok.NoArgsConstructor;
26-
import lombok.Setter;
2724
import lombok.extern.slf4j.Slf4j;
2825

2926
@Slf4j
3027
@NoArgsConstructor
3128
class DefaultHttpDestinationBuilderProxyHandler
3229
{
33-
@Setter( AccessLevel.PACKAGE )
34-
private static ServiceBinding serviceBindingConnectivity = null;
35-
36-
@Setter( AccessLevel.PACKAGE )
37-
private static ServiceBindingDestinationLoader serviceBindingDestinationLoader = null;
38-
3930
/**
4031
* Handler to work resolve a proxied {@link DefaultHttpDestination} object.
4132
*
@@ -49,14 +40,17 @@ DefaultHttpDestination handle( @Nonnull final DefaultHttpDestination.Builder bui
4940
throws DestinationAccessException,
5041
DestinationNotFoundException
5142
{
43+
if( !builder.get(DestinationProperty.PROXY_TYPE).contains(ProxyType.ON_PREMISE) ) {
44+
throw new ShouldNotHappenException(
45+
"Proxy handler was invoked for destination "
46+
+ builder.get(DestinationProperty.NAME).getOrElse("unnamed-destination")
47+
+ " that is not supposed to be proxied.");
48+
}
5249
// add location id header provider, useful to identify one of multiple cloud connectors
5350
builder.headerProviders(new SapConnectivityLocationIdHeaderProvider());
5451

5552
// resolve connectivity service binding, necessary for on-premise proxying
5653
final ServiceBinding serviceBinding = getServiceBindingConnectivity();
57-
if( serviceBinding == null ) {
58-
throw new DestinationAccessException("Unable to resolve connectivity service binding.");
59-
}
6054

6155
final OnBehalfOf derivedOnBehalfOf = deriveOnBehalfOf(builder);
6256
final DefaultHttpDestination destination = builder.buildInternal();
@@ -95,15 +89,14 @@ private static OnBehalfOf deriveOnBehalfOf( @Nonnull final DefaultHttpDestinatio
9589
.get(DestinationProperty.PRINCIPAL_PROPAGATION_MODE)
9690
.getOrElse(PrincipalPropagationMode.TOKEN_FORWARDING);
9791

98-
switch( mode ) {
99-
case TOKEN_EXCHANGE:
100-
return OnBehalfOf.NAMED_USER_CURRENT_TENANT;
101-
case TOKEN_FORWARDING: // default
92+
return switch (mode) {
93+
case TOKEN_EXCHANGE -> OnBehalfOf.NAMED_USER_CURRENT_TENANT;
94+
case TOKEN_FORWARDING -> { // default
10295
builder.headerProviders(new SapConnectivityAuthenticationHeaderProvider());
103-
return OnBehalfOf.TECHNICAL_USER_CURRENT_TENANT;
104-
case UNKNOWN:
105-
throw new IllegalStateException("Principal propagation mode is unknown.");
106-
}
96+
yield OnBehalfOf.TECHNICAL_USER_CURRENT_TENANT;
97+
}
98+
case UNKNOWN -> throw new IllegalStateException("Principal propagation mode is unknown.");
99+
};
107100
}
108101

109102
final boolean isProvider = builder.get(DestinationProperty.TENANT_ID).filter(String::isEmpty).isDefined();
@@ -117,27 +110,26 @@ private static OnBehalfOf deriveOnBehalfOf( @Nonnull final DefaultHttpDestinatio
117110
* @return The service binding representing Connectivity Service. Or {@code null} if no service binding could be
118111
* found.
119112
*/
120-
@Nullable
121-
private ServiceBinding getServiceBindingConnectivity()
113+
@Nonnull
114+
ServiceBinding getServiceBindingConnectivity()
122115
{
123-
if( serviceBindingConnectivity == null ) {
124-
final ServiceBindingAccessor serviceBindingAccessor = DefaultServiceBindingAccessor.getInstance();
125-
126-
final Predicate<ServiceBinding> predicate =
127-
b -> ServiceIdentifier.CONNECTIVITY.equals(b.getServiceIdentifier().orElse(null));
128-
129-
final List<ServiceBinding> serviceBindings =
130-
serviceBindingAccessor.getServiceBindings().stream().filter(predicate).collect(Collectors.toList());
131-
132-
if( serviceBindings.isEmpty() ) {
133-
log.debug("No service bindings found matching Connectivity.");
134-
} else if( serviceBindings.size() > 1 ) {
135-
log.debug("More than one service bindings found that match Connectivity.");
136-
} else {
137-
serviceBindingConnectivity = serviceBindings.get(0);
138-
}
116+
final ServiceBindingAccessor serviceBindingAccessor = DefaultServiceBindingAccessor.getInstance();
117+
118+
final Predicate<ServiceBinding> predicate =
119+
b -> ServiceIdentifier.CONNECTIVITY.equals(b.getServiceIdentifier().orElse(null));
120+
121+
final List<ServiceBinding> serviceBindings =
122+
serviceBindingAccessor.getServiceBindings().stream().filter(predicate).toList();
123+
124+
if( serviceBindings.isEmpty() ) {
125+
throw new DestinationAccessException(
126+
"Unable to resolve connectivity service binding. No service bindings found matching Connectivity.");
127+
} else if( serviceBindings.size() > 1 ) {
128+
throw new DestinationAccessException(
129+
"Unable to resolve connectivity service binding. More than one service bindings found that match Connectivity.");
130+
} else {
131+
return serviceBindings.get(0);
139132
}
140-
return serviceBindingConnectivity;
141133
}
142134

143135
/**
@@ -146,12 +138,9 @@ private ServiceBinding getServiceBindingConnectivity()
146138
* @return The loader object.
147139
*/
148140
@Nonnull
149-
private ServiceBindingDestinationLoader getServiceBindingDestinationLoader()
141+
ServiceBindingDestinationLoader getServiceBindingDestinationLoader()
150142
{
151-
if( serviceBindingDestinationLoader == null ) {
152-
serviceBindingDestinationLoader = ServiceBindingDestinationLoader.defaultLoaderChain();
153-
}
154-
return serviceBindingDestinationLoader;
143+
return ServiceBindingDestinationLoader.defaultLoaderChain();
155144
}
156145

157146
/**

‎cloudplatform/cloudplatform-connectivity/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestinationBuilderProxyHandlerTest.java

+33-39
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,19 @@
44
import static org.assertj.core.api.Assertions.assertThat;
55
import static org.assertj.core.api.Assertions.assertThatThrownBy;
66
import static org.mockito.ArgumentMatchers.argThat;
7+
import static org.mockito.Mockito.doReturn;
78
import static org.mockito.Mockito.mock;
89
import static org.mockito.Mockito.spy;
910
import static org.mockito.Mockito.verify;
1011
import static org.mockito.Mockito.verifyNoInteractions;
12+
import static org.mockito.Mockito.when;
1113

1214
import java.net.URI;
1315

1416
import javax.annotation.Nonnull;
1517

16-
import org.junit.jupiter.api.AfterEach;
1718
import org.junit.jupiter.api.BeforeEach;
19+
import org.junit.jupiter.api.DisplayName;
1820
import org.junit.jupiter.api.Test;
1921
import org.junit.jupiter.api.extension.RegisterExtension;
2022
import org.mockito.Answers;
@@ -25,6 +27,7 @@
2527
import com.sap.cloud.environment.servicebinding.api.ServiceBinding;
2628
import com.sap.cloud.environment.servicebinding.api.ServiceIdentifier;
2729
import com.sap.cloud.sdk.cloudplatform.connectivity.exception.DestinationAccessException;
30+
import com.sap.cloud.sdk.cloudplatform.exception.ShouldNotHappenException;
2831
import com.sap.cloud.sdk.cloudplatform.security.AuthToken;
2932
import com.sap.cloud.sdk.cloudplatform.security.BasicCredentials;
3033
import com.sap.cloud.sdk.testutil.TestContext;
@@ -61,61 +64,65 @@ public Try<HttpDestination> tryGetDestination( @Nonnull ServiceBindingDestinatio
6164
@RegisterExtension
6265
static TestContext context = TestContext.withThreadContext();
6366

67+
private DefaultHttpDestinationBuilderProxyHandler sut;
68+
6469
@BeforeEach
65-
@AfterEach
66-
void clean()
70+
void setUp()
6771
{
68-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingConnectivity(null);
69-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingDestinationLoader(null);
72+
sut = spy(new DefaultHttpDestinationBuilderProxyHandler());
73+
doReturn(destinationLoader).when(sut).getServiceBindingDestinationLoader();
74+
doReturn(connectivityService).when(sut).getServiceBindingConnectivity();
7075
}
7176

7277
@Test
73-
void testNoProxy()
78+
@DisplayName( "Handler should only be invoked for destinations with proxy type ON_PREMISE" )
79+
void testProxyTypeNotOnpremise()
7480
{
7581
final DefaultHttpDestination.Builder builder = DefaultHttpDestination.builder("http://foo");
7682

77-
assertThatThrownBy(() -> new DefaultHttpDestinationBuilderProxyHandler().handle(builder))
78-
.isExactlyInstanceOf(DestinationAccessException.class)
79-
.hasMessage("Unable to resolve connectivity service binding.");
83+
assertThatThrownBy(() -> sut.handle(builder)).isExactlyInstanceOf(ShouldNotHappenException.class);
8084
verifyNoInteractions(destinationLoader);
8185
}
8286

8387
@Test
8488
void testNoOnProxy()
8589
{
90+
when(sut.getServiceBindingDestinationLoader()).thenCallRealMethod();
91+
when(sut.getServiceBindingConnectivity()).thenCallRealMethod();
92+
8693
final DefaultHttpDestination.Builder builder =
8794
DefaultHttpDestination.builder("http://foo").proxyType(ProxyType.INTERNET);
8895

89-
assertThatThrownBy(() -> new DefaultHttpDestinationBuilderProxyHandler().handle(builder))
96+
assertThatThrownBy(() -> sut.handle(builder))
9097
.isExactlyInstanceOf(DestinationAccessException.class)
91-
.hasMessage("Unable to resolve connectivity service binding.");
98+
.hasMessageContaining("Unable to resolve connectivity service binding.");
9299
verifyNoInteractions(destinationLoader);
93100
}
94101

95102
@Test
96103
void testNoConnectivity()
97104
{
105+
when(sut.getServiceBindingDestinationLoader()).thenCallRealMethod();
106+
when(sut.getServiceBindingConnectivity()).thenCallRealMethod();
107+
98108
final DefaultHttpDestination.Builder builder =
99109
DefaultHttpDestination.builder("http://foo").proxyType(ProxyType.ON_PREMISE);
100110

101-
assertThatThrownBy(() -> new DefaultHttpDestinationBuilderProxyHandler().handle(builder))
111+
assertThatThrownBy(() -> sut.handle(builder))
102112
.isExactlyInstanceOf(DestinationAccessException.class)
103-
.hasMessage("Unable to resolve connectivity service binding.");
113+
.hasMessageContaining("Unable to resolve connectivity service binding.");
104114
verifyNoInteractions(destinationLoader);
105115
}
106116

107117
@Test
108118
void testNoAuth()
109119
{
110-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingDestinationLoader(destinationLoader);
111-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingConnectivity(connectivityService);
112-
113120
final URI uri = URI.create("http://foo");
114121
final DefaultHttpDestination.Builder builder =
115122
DefaultHttpDestination.builder(uri).proxyType(ProxyType.ON_PREMISE);
116123

117124
// test
118-
final DefaultHttpDestination result = new DefaultHttpDestinationBuilderProxyHandler().handle(builder);
125+
final DefaultHttpDestination result = sut.handle(builder);
119126

120127
assertThat(result).isNotNull();
121128
verify(destinationLoader).tryGetDestination(argThat(options -> {
@@ -129,16 +136,13 @@ void testNoAuth()
129136
@Test
130137
void testNotPrincipalPropagation()
131138
{
132-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingDestinationLoader(destinationLoader);
133-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingConnectivity(connectivityService);
134-
135139
final URI uri = URI.create("http://foo");
136140
final BasicCredentials basicCredentials = new BasicCredentials("foo", "bar");
137141
final DefaultHttpDestination.Builder builder =
138142
DefaultHttpDestination.builder(uri).proxyType(ProxyType.ON_PREMISE).basicCredentials(basicCredentials);
139143

140144
// test
141-
final DefaultHttpDestination result = new DefaultHttpDestinationBuilderProxyHandler().handle(builder);
145+
final DefaultHttpDestination result = sut.handle(builder);
142146

143147
assertThat(result).isNotNull();
144148
verify(destinationLoader).tryGetDestination(argThat(options -> {
@@ -154,8 +158,6 @@ void testNotPrincipalPropagation()
154158
void testPrincipalPropagationDefault()
155159
{
156160
context.setAuthToken(token1);
157-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingDestinationLoader(destinationLoader);
158-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingConnectivity(connectivityService);
159161

160162
final URI uri = URI.create("http://foo");
161163
final DefaultHttpDestination.Builder builder =
@@ -165,7 +167,7 @@ void testPrincipalPropagationDefault()
165167
.authenticationType(AuthenticationType.PRINCIPAL_PROPAGATION);
166168

167169
// test
168-
final DefaultHttpDestination result = new DefaultHttpDestinationBuilderProxyHandler().handle(builder);
170+
final DefaultHttpDestination result = sut.handle(builder);
169171

170172
assertThat(result).isNotNull();
171173
verify(destinationLoader).tryGetDestination(argThat(options -> {
@@ -182,8 +184,6 @@ void testPrincipalPropagationDefault()
182184
void testPrincipalPropagationCompatibility()
183185
{
184186
context.setAuthToken(token1);
185-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingDestinationLoader(destinationLoader);
186-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingConnectivity(connectivityService);
187187

188188
final URI uri = URI.create("http://foo");
189189
final DefaultHttpDestination.Builder builder =
@@ -194,7 +194,7 @@ void testPrincipalPropagationCompatibility()
194194
.property(DestinationProperty.PRINCIPAL_PROPAGATION_MODE, PrincipalPropagationMode.TOKEN_FORWARDING);
195195

196196
// test
197-
final DefaultHttpDestination result = new DefaultHttpDestinationBuilderProxyHandler().handle(builder);
197+
final DefaultHttpDestination result = sut.handle(builder);
198198

199199
assertThat(result).isNotNull();
200200
verify(destinationLoader).tryGetDestination(argThat(options -> {
@@ -210,9 +210,6 @@ void testPrincipalPropagationCompatibility()
210210
@Test
211211
void testPrincipalPropagationRecommended()
212212
{
213-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingDestinationLoader(destinationLoader);
214-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingConnectivity(connectivityService);
215-
216213
final URI uri = URI.create("http://foo");
217214
final DefaultHttpDestination.Builder builder =
218215
DefaultHttpDestination
@@ -222,7 +219,7 @@ void testPrincipalPropagationRecommended()
222219
.property(DestinationProperty.PRINCIPAL_PROPAGATION_MODE, PrincipalPropagationMode.TOKEN_EXCHANGE);
223220

224221
// test
225-
final DefaultHttpDestination result = new DefaultHttpDestinationBuilderProxyHandler().handle(builder);
222+
final DefaultHttpDestination result = sut.handle(builder);
226223

227224
assertThat(result).isNotNull();
228225
verify(destinationLoader).tryGetDestination(argThat(options -> {
@@ -236,9 +233,6 @@ void testPrincipalPropagationRecommended()
236233
@Test
237234
void testNoAuthWithTenantId()
238235
{
239-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingDestinationLoader(destinationLoader);
240-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingConnectivity(connectivityService);
241-
242236
final URI uri = URI.create("http://foo");
243237
final DefaultHttpDestinationBuilderProxyHandler sut = new DefaultHttpDestinationBuilderProxyHandler();
244238

@@ -287,11 +281,8 @@ void testNoAuthWithTenantId()
287281
void testPrincipalPropagationWithTenantId()
288282
{
289283
context.setAuthToken(token1);
290-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingDestinationLoader(destinationLoader);
291-
DefaultHttpDestinationBuilderProxyHandler.setServiceBindingConnectivity(connectivityService);
292284

293285
final URI uri = URI.create("http://foo");
294-
final DefaultHttpDestinationBuilderProxyHandler sut = new DefaultHttpDestinationBuilderProxyHandler();
295286

296287
// provider tenant
297288
{
@@ -339,12 +330,15 @@ void testPrincipalPropagationWithTenantId()
339330
@Test
340331
void testNotProxyTheProxy()
341332
{
333+
when(sut.getServiceBindingDestinationLoader()).thenCallRealMethod();
334+
when(sut.getServiceBindingConnectivity()).thenCallRealMethod();
335+
342336
final DefaultHttpDestination.Builder builder1 = DefaultHttpDestination.builder("http://foo");
343337
final DefaultHttpDestination.Builder builder = DefaultHttpDestination.fromDestination(builder1.build());
344338

345-
assertThatThrownBy(() -> new DefaultHttpDestinationBuilderProxyHandler().handle(builder))
339+
assertThatThrownBy(() -> sut.handle(builder))
346340
.isExactlyInstanceOf(DestinationAccessException.class)
347-
.hasMessage("Unable to resolve connectivity service binding.");
341+
.hasMessageContaining("Unable to resolve connectivity service binding.");
348342
verifyNoInteractions(destinationLoader);
349343
}
350344

0 commit comments

Comments
 (0)