Using the EdcHttpClient without fallbacks in HttpDataSource

[AndrYurk]

In the HttpDataSource class, we execute requests using EdcHttpClient without any fallbacks. This means that FailsafeCall will return a FailsafeException after unsuccessful attempts for any kind of response error. However, errors like 404, 401, and 403 (and possibly others) do not need to be retried and should be returned after the first attempt.

From a code perspective, it was designed to receive and handle such errors, but currently, this functionality is not reachable.

Connector/extensions/data-plane/data-plane-http/src/main/java/org/eclipse/edc/connector/dataplane/http/pipeline/HttpDataSource.java

Lines 62 to 91 in 0802581

	var response = httpClient.execute(request);
	if (response.isSuccessful()) {
	var body = response.body();
	if (body == null) {
	throw new EdcException(format("Received empty response body transferring HTTP data for request %s: %s", requestId, response.code()));
	}
	var stream = body.byteStream();
	responseBodyStream.set(new ResponseBodyStream(body, stream));
	var mediaType = Optional.ofNullable(body.contentType()).map(MediaType::toString).orElse(OCTET_STREAM);
	return success(Stream.of(new HttpPart(name, stream, mediaType)));
	} else {
	try {
	if (NOT_AUTHORIZED == response.code() || FORBIDDEN == response.code()) {
	return StreamResult.notAuthorized();
	} else if (NOT_FOUND == response.code()) {
	return StreamResult.notFound();
	} else {
	return error(format("Received code transferring HTTP data: %s - %s.", response.code(), response.message()));
	}
	} finally {
	try {
	response.close();
	} catch (Exception e) {
	monitor.severe("Error closing failed response", e);
	}
	}
	}
	} catch (IOException e) {
	throw new EdcException(e);
	}

The proposal is to use fallbacks, but the question is: which error codes should not be retried? Should it include all error codes from 400 to 500, or only specific ones?

[ndr-brt]

It really depends, I can see cases when at the first call the service returns 404 because the resource is not available "yet", and so at the second time it works.

My take on this would be to add a configuration setting in the data-plane-http extension, to exclude certain response codes from the retry policy, so eventually providers can decide to enable this feature or not.

[AndrYurk]

It really depends, I can see cases when at the first call the service returns 404 because the resource is not available "yet", and so at the second time it works.

For such cases, there is no guarantee that the resource will be available even after the 10th retry. In my opinion, this is not a good solution. Additionally, what about errors with status codes 401 and 403, what is the reason to retry them?

Please also take a look at the HttpDataSourceTest, where there exists a unit test called verifyCallFailed that expects to receive such errors from the server. It seems it was not designed to handle FailsafeException, so achieving some consistency would be great because it is really confusing.

My take on this would be to add a configuration setting in the data-plane-http extension, to exclude certain response codes from the retry policy, so eventually providers can decide to enable this feature or not.

Adding a configuration setting is a good idea for the system's flexibility, but what about the default behavior?
Additionally, if you look at the usage of

Connector/core/common/lib/http-lib/src/main/java/org/eclipse/edc/http/client/EdcHttpClientImpl.java

Lines 50 to 52 in 65a9f3f

	public Response execute(Request request) throws IOException {
	return execute(request, emptyList());
	}

, you will find other places where it was expected to receive some error code from the response. However, instead of that, we now get a FailsafeException:

Connector/extensions/common/vault/vault-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/client/HashicorpVaultClient.java

Lines 90 to 111 in 65a9f3f

	try (var response = httpClient.execute(request)) {
	if (response.isSuccessful()) {
	return Result.success();
	}
	var code = response.code();
	var errMsg = switch (code) {
	case 429 -> "Vault is in standby";
	case 472 -> "Vault is in recovery mode";
	case 473 -> "Vault is in performance standby";
	case 501 -> "Vault is not initialized";
	case 503 -> "Vault is sealed";
	default -> "Vault returned unspecified code %d".formatted(code);
	};
	var body = response.body();
	if (body == null) {
	return Result.failure("Healthcheck returned empty response body");
	}
	return Result.failure("Vault is not available. Reason: %s, additional information: %s".formatted(errMsg, body.string()));
	} catch (IOException e) {
	return Result.failure("Failed to perform healthcheck with reason: %s".formatted(e.getMessage()));
	}

Connector/extensions/common/vault/vault-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/client/HashicorpVaultClient.java

Lines 247 to 253 in 65a9f3f

	try (var response = httpClient.execute(request)) {
	return response.isSuccessful() || response.code() == HTTP_CODE_404
	? Result.success()
	: Result.failure("Failed to destroy secret with status %d".formatted(response.code()));
	} catch (IOException e) {
	return Result.failure("Failed to destroy secret with reason: %s".formatted(e.getMessage()));
	}

Connector/extensions/control-plane/provision/provision-http/src/main/java/org/eclipse/edc/connector/controlplane/provision/http/impl/HttpProviderProvisioner.java

Lines 98 to 109 in 65a9f3f

	try (var response = httpClient.execute(request)) {
	if (response.isSuccessful()) {
	return completedFuture(StatusResult.success(ProvisionResponse.Builder.newInstance().inProcess(true).build()));
	} else if (response.code() >= 500 && response.code() <= 504) {
	return completedFuture(StatusResult.failure(ResponseStatus.ERROR_RETRY, "HttpProviderProvisioner: received error code: " + response.code()));
	} else {
	return completedFuture(StatusResult.failure(ResponseStatus.FATAL_ERROR, "HttpProviderProvisioner: received fatal error code: " + response.code()));
	}
	} catch (IOException e) {
	monitor.severe("Error invoking provisioner: " + name, e);
	return completedFuture(StatusResult.failure(ResponseStatus.ERROR_RETRY, "HttpProviderProvisioner: received error: " + e.getMessage()));
	}

However, in these cases I have to admit, we at least return Result.failure() or StatusResult.failure() upon encountering an exception.

[jimmarino]

I do not think we should allow custom configuration for the client when it is used in core parts of the codebase as that can change runtime behavior, which is not desired. Also, these contexts are different than the HTTP streaming flow in the data plane.

The data plane could be setup with a custom client that logs and then maps these exceptions to a non-retriable response without revealing the specifics to the client.