Skip to content

Transparent proxy: TP Loader Cloud SDK native integration #981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Transparent proxy: TP Loader Cloud SDK native integration #981

wants to merge 1 commit into from
+955 −0

Conversation

@VladislavAtanasov95
Copy link

@VladislavAtanasov95 VladislavAtanasov95 commented Oct 23, 2025

No description provided.

@cla-assistant
Copy link

cla-assistant bot commented Oct 23, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@VladislavAtanasov95 VladislavAtanasov95 force-pushed the tploader branch 7 times, most recently from b860033 to cb21b31 Compare October 23, 2025 15:59
KNikovv
KNikovv reviewed Oct 23, 2025
View reviewed changes
@VladislavAtanasov95 VladislavAtanasov95 force-pushed the tploader branch 11 times, most recently from bc1d50c to f7ee4bd Compare October 24, 2025 11:55
@VladislavAtanasov95
Copy link
Author

VladislavAtanasov95 commented Oct 24, 2025 •
edited
Loading

@MatKuhr please review

@VladislavAtanasov95 VladislavAtanasov95 force-pushed the tploader branch 2 times, most recently from 72dc4c1 to 70ee9ae Compare October 24, 2025 12:46
@VladislavAtanasov95 VladislavAtanasov95 force-pushed the tploader branch 2 times, most recently from 18db618 to de06843 Compare October 28, 2025 15:06
MatKuhr
MatKuhr reviewed Nov 19, 2025
View reviewed changes
Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall approach looks good! Haven't looked at tests yet. The only essential question for me is, if we can/should perform an actual call to TP already during tryGetDestination.

@VladislavAtanasov95 VladislavAtanasov95 force-pushed the tploader branch 2 times, most recently from 1f09efa to dd731b9 Compare November 20, 2025 13:53
@VladislavAtanasov95 VladislavAtanasov95 force-pushed the tploader branch 4 times, most recently from 628ae71 to 44ae3be Compare November 24, 2025 13:56
MatKuhr
MatKuhr reviewed Nov 25, 2025
View reviewed changes
...ion-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxy.java
Try<Destination>
tryGetDestination( @Nonnull final String destinationName, @Nonnull DestinationOptions options )
{
final TransparentProxyDestination destination =
Copy link
Member

@MatKuhr MatKuhr Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
final TransparentProxyDestination destination =
if (!options.getOptionKeys().isEmpty()) {
log.warn("DestinationOptions have been provided, but are not yet supported by the Transparent Proxy implementation and will be ignored.");
}
final TransparentProxyDestination destination =

...ion-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxy.java Outdated
final int statusCode = response.getStatusLine().getStatusCode();

boolean destinationNotFound = false;
if( statusCode == HttpStatus.SC_BAD_GATEWAY ) {
Copy link
Member

@MatKuhr MatKuhr Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if( statusCode == HttpStatus.SC_BAD_GATEWAY ) What does TP do, if the backend system itself responds with 504?

Copy link
Author

@VladislavAtanasov95 VladislavAtanasov95 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then there are no error headers so it wont enter in the conditions

...ion-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxy.java Outdated
TransparentProxyDestination.gateway(destinationName, uri).build();

if( isDestinationNotFound(destination, destinationName) ) {
return Try.failure(new DestinationAccessException("Destination not found: " + destinationName));
Copy link
Member

@MatKuhr MatKuhr Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's critical that DestinationNotFoundException is thrown, otherwise the chain will not try further loaders.

Suggested change
return Try.failure(new DestinationAccessException("Destination not found: " + destinationName));
return Try.failure(new DestinationNotFoundException("Destination '" + destinationName + "' could not be resolved for the current tenant " + tenant + " by Transparent Proxy running on " + uri));

Also, we should include more information in the exception, otherwise users have little information for troubleshooting

Copy link
Author

@VladislavAtanasov95 VladislavAtanasov95 Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to DestinationNotFoundException. Message would be retrieved from TP "X-Error-Message" header:
"Destination 'google44' not found in tenant 'tp-integration-tests-8imc9i7e'. Contact the Destination Administrator."
or if fragment exists
Destination 'google' with fragment 'blalala' not found in tenant 'tp-integration-tests-8imc9i7e'. Contact the Destination Administrator.

if these are okay

Copy link
Member

@MatKuhr MatKuhr Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that sounds good!

Does the TP allow the tenant to be null by the way? If not, we may have an issue, because the Cloud SDK generally allows this

...ion-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/TransparentProxy.java Outdated
boolean destinationNotFound = false;
if( statusCode == HttpStatus.SC_BAD_GATEWAY ) {
final String errorInternalCode = getHeaderValue(response, X_ERROR_INTERNAL_CODE_HEADER);
if( Integer.toString(HttpStatus.SC_NOT_FOUND).equals(errorInternalCode) ) {
Copy link
Member

@MatKuhr MatKuhr Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there also a dedicated error code in case the destination was found, but the authToken contains an error? If so, we could also directly throw a DestinationAccessException in that case, because that is also how the standard implementation behaves.

Copy link
Author

@VladislavAtanasov95 VladislavAtanasov95 Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not dedicated, but it is covered in current impl and throws DestinationAccessException in that case.

Also we are using TP error headers to return more detailed error message including Origin and TP error message

@VladislavAtanasov95 VladislavAtanasov95 force-pushed the tploader branch 6 times, most recently from 5971ceb to e2d8139 Compare November 28, 2025 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MatKuhr MatKuhr MatKuhr left review comments

+2 more reviewers

@IliyanVidenov IliyanVidenov IliyanVidenov left review comments

@KNikovv KNikovv KNikovv left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@VladislavAtanasov95 @IliyanVidenov @KNikovv @MatKuhr