Skip to content

Commit

Permalink
refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler …
Browse files Browse the repository at this point in the history 
…for DeploymentMonitorService

Update the error handler for the retrofit client, DeploymentMonitorService, to use SpinnakerRetrofitErrorHandler. Additionally, modify catch blocks with RetrofitError to use Spinnaker*Exception.

Purpose : This commit is foundational work for upgrading the Retrofit version to retrofit2.x

Behavioural change : The modification includes updating catch blocks, leading to a change in the format of logger messages. This aligns with the upcoming modifications discussed in the PR: #4608

Additionally, a new specific exception handler is introduced to manage JsonProcessing during the serialization of the HTTP error body. The impact of these changes should be compared with another related PR: #4617
  • Loading branch information
@Pranav-b-7
Pranav-b-7 committed Jan 2, 2024
1 parent fb0594e commit 133f1c5
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 47 deletions.
44 changes: 21 additions & 23 deletions ...com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@

package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy;

import com.google.common.io.CharStreams;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.config.DeploymentMonitorDefinition;
import com.netflix.spinnaker.kork.annotations.VisibleForTesting;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
import com.netflix.spinnaker.orca.api.pipeline.RetryableTask;
import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus;
Expand All @@ -31,19 +34,13 @@
import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse;
import com.netflix.spinnaker.orca.deploymentmonitor.models.MonitoredDeployInternalStageData;
import com.netflix.spinnaker.orca.deploymentmonitor.models.StatusExplanation;
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.*;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import retrofit.RetrofitError;
import retrofit.client.Header;
import retrofit.client.Response;

public class MonitoredDeployBaseTask implements RetryableTask {
static final int MAX_RETRY_COUNT = 3;
Expand All @@ -52,6 +49,7 @@ public class MonitoredDeployBaseTask implements RetryableTask {
private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider;
private final Map<EvaluateHealthResponse.NextStepDirective, String> summaryMapping =
new HashMap<>();
ObjectMapper objectMapper = new ObjectMapper();

MonitoredDeployBaseTask(
DeploymentMonitorServiceProvider deploymentMonitorServiceProvider, Registry registry) {
Expand Down Expand Up @@ -138,12 +136,12 @@ public long getDynamicTimeout(StageExecution stage) {

try {
return executeInternal(stage, monitorDefinition);
} catch (RetrofitError e) {
} catch (SpinnakerServerException e) {
log.warn(
"HTTP Error encountered while talking to {}->{}, {}}",
monitorDefinition,
e.getUrl(),
getRetrofitLogMessage(e.getResponse()),
getErrorMessage(e),
e);

return handleError(stage, e, true, monitorDefinition);
Expand Down Expand Up @@ -269,28 +267,28 @@ private MonitoredDeployStageData getStageContext(StageExecution stage) {
}

@VisibleForTesting
String getRetrofitLogMessage(Response response) {
if (response == null) {
String getErrorMessage(SpinnakerServerException spinnakerException) {
if (!(spinnakerException instanceof SpinnakerHttpException)) {
return "<NO RESPONSE>";
}

String body = "";
String status = "";
String headers = "";
SpinnakerHttpException httpException = (SpinnakerHttpException) spinnakerException;

try {
status = String.format("%d (%s)", response.getStatus(), response.getReason());
body =
CharStreams.toString(
new InputStreamReader(response.getBody().in(), StandardCharsets.UTF_8));
headers =
response.getHeaders().stream().map(Header::toString).collect(Collectors.joining("\n"));
String body = "";
if (httpException.getResponseBody() != null) {
body = objectMapper.writeValueAsString(httpException.getResponseBody());
}
return String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body);
} catch (JsonProcessingException e) {
log.error("Failed to serialize the error response body : ", e);
return String.format(
"headers: %s\nresponse body: %s", httpException.getHeaders(), e.getMessage());
} catch (Exception e) {
log.error(
"Failed to fully parse retrofit error while reading response from deployment monitor", e);
log.error("Failed to fully serialize http error response details", e);
}

return String.format("status: %s\nheaders: %s\nresponse body: %s", status, headers, body);
return "headers: \nresponse body: ";
}

private boolean shouldFailOnError(StageExecution stage, DeploymentMonitorDefinition definition) {
Expand Down
11 changes: 6 additions & 5 deletions .../spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy
import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spectator.api.NoopRegistry
import com.netflix.spinnaker.config.DeploymentMonitorDefinition
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
import com.netflix.spinnaker.orca.api.pipeline.TaskResult
import com.netflix.spinnaker.orca.clouddriver.MortServiceSpec
Expand Down Expand Up @@ -46,11 +48,11 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
PipelineExecutionImpl pipe = pipeline {
}

def "should retry retrofit errors"() {
def "should retry on SpinnakerServerException"() {
given:
def monitorServiceStub = Stub(DeploymentMonitorService) {
evaluateHealth(_) >> {
throw RetrofitError.networkError("url", new IOException())
throw new SpinnakerServerException(RetrofitError.networkError("url", new IOException()))
}
}

Expand Down Expand Up @@ -203,8 +205,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
false | null || ExecutionStatus.FAILED_CONTINUE
}

def "should return status as RUNNING when Retrofit http error is thrown"() {

def "should return status as RUNNING when SpinnakerHttpException is thrown"() {
def converter = new JacksonConverter(new ObjectMapper())

Response response =
Expand All @@ -224,7 +225,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
given:
def monitorServiceStub = Stub(DeploymentMonitorService) {
evaluateHealth(_) >> {
throw RetrofitError.httpError("https://foo.com/deployment/evaluateHealth", response, converter, null)
throw new SpinnakerHttpException(RetrofitError.httpError("https://foo.com/deployment/evaluateHealth", response, converter, null))
}
}

Expand Down
89 changes: 70 additions & 19 deletions ...netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,17 @@
package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spectator.api.NoopRegistry;
import com.netflix.spinnaker.config.DeploymentMonitorDefinition;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerConversionException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
import com.netflix.spinnaker.orca.deploymentmonitor.DeploymentMonitorServiceProvider;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
Expand Down Expand Up @@ -50,6 +57,8 @@ public class MonitoredDeployBaseTaskTest {

private final ObjectMapper objectMapper = new ObjectMapper();

private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider;

@BeforeEach
void setup() {
OkClient okClient = new OkClient();
Expand All @@ -63,7 +72,7 @@ void setup() {
var deploymentMonitorDefinitions = new ArrayList<DeploymentMonitorDefinition>();
deploymentMonitorDefinitions.add(deploymentMonitorDefinition);

DeploymentMonitorServiceProvider deploymentMonitorServiceProvider =
deploymentMonitorServiceProvider =
new DeploymentMonitorServiceProvider(
okClient, retrofitLogLevel, requestInterceptor, deploymentMonitorDefinitions);
monitoredDeployBaseTask =
Expand Down Expand Up @@ -93,14 +102,14 @@ public void shouldParseHttpErrorResponseDetailsWhenHttpErrorHasOccurred() {
RetrofitError.httpError(
"https://foo.com/deployment/evaluateHealth", response, new JacksonConverter(), null);

String logMessageOnHttpError =
monitoredDeployBaseTask.getRetrofitLogMessage(httpError.getResponse());
String status = HttpStatus.BAD_REQUEST.value() + " (" + HttpStatus.BAD_REQUEST.name() + ")";
SpinnakerHttpException httpException = new SpinnakerHttpException(httpError);

String logMessageOnSpinHttpException = monitoredDeployBaseTask.getErrorMessage(httpException);
String body = "{\"error\":\"400 - Bad request, application name cannot be empty\"}";

assertThat(logMessageOnHttpError)
assertThat(logMessageOnSpinHttpException)
.isEqualTo(
String.format("status: %s\nheaders: %s\nresponse body: %s", status, header, body));
String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body));
}

@Test
Expand Down Expand Up @@ -130,14 +139,13 @@ public void shouldParseHttpErrorResponseDetailsWhenConversionErrorHasOccurred()
null,
new ConversionException("Failed to parse response"));

String status = HttpStatus.BAD_REQUEST.value() + " (" + HttpStatus.BAD_REQUEST.name() + ")";
String body = "{\"error\":\"400 - Bad request, application name cannot be empty\"}";
String logMessageOnConversionError =
monitoredDeployBaseTask.getRetrofitLogMessage(conversionError.getResponse());
SpinnakerConversionException conversionException =
new SpinnakerConversionException(conversionError);

assertThat(logMessageOnConversionError)
.isEqualTo(
String.format("status: %s\nheaders: %s\nresponse body: %s", status, header, body));
String logMessageOnSpinConversionException =
monitoredDeployBaseTask.getErrorMessage(conversionException);

assertThat(logMessageOnSpinConversionException).isEqualTo("<NO RESPONSE>");
}

@Test
Expand All @@ -148,10 +156,12 @@ void shouldReturnDefaultLogMsgWhenNetworkErrorHasOccurred() {
"https://foo.com/deployment/evaluateHealth",
new IOException("Failed to connect to the host : foo.com"));

String logMessageOnNetworkError =
monitoredDeployBaseTask.getRetrofitLogMessage(networkError.getResponse());
SpinnakerNetworkException networkException = new SpinnakerNetworkException(networkError);

assertThat(logMessageOnNetworkError).isEqualTo("<NO RESPONSE>");
String logMessageOnSpinNetworkException =
monitoredDeployBaseTask.getErrorMessage(networkException);

assertThat(logMessageOnSpinNetworkException).isEqualTo("<NO RESPONSE>");
}

@Test
Expand All @@ -162,10 +172,51 @@ void shouldReturnDefaultLogMsgWhenUnexpectedErrorHasOccurred() {
"https://foo.com/deployment/evaluateHealth",
new IOException("Failed to connect to the host : foo.com"));

String logMessageOnUnexpectedError =
monitoredDeployBaseTask.getRetrofitLogMessage(unexpectedError.getResponse());
SpinnakerServerException serverException = new SpinnakerServerException(unexpectedError);

String logMessageOnSpinServerException =
monitoredDeployBaseTask.getErrorMessage(serverException);

assertThat(logMessageOnSpinServerException).isEqualTo("<NO RESPONSE>");
}

@Test
void shouldReturnHeadersAndErrorMessageWhenSerializingFailsWhileReadingBody()
throws JsonProcessingException {

var converter = new JacksonConverter(objectMapper);
var responseBody = new HashMap<String, String>();
var headers = new ArrayList<Header>();
var header = new Header(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE);
monitoredDeployBaseTask.objectMapper = mock(ObjectMapper.class);

headers.add(header);
responseBody.put("error", "400 - Bad request, application name cannot be empty");

assertThat(logMessageOnUnexpectedError).isEqualTo("<NO RESPONSE>");
Response response =
new Response(
"/deployment/evaluateHealth",
HttpStatus.BAD_REQUEST.value(),
HttpStatus.BAD_REQUEST.name(),
headers,
new MockTypedInput(converter, responseBody));

RetrofitError httpError =
RetrofitError.httpError(
"https://foo.com/deployment/evaluateHealth", response, converter, null);

SpinnakerHttpException httpException = new SpinnakerHttpException(httpError);
JsonProcessingException jpe = new JsonProcessingException("Failed to parse the error body") {};

when(monitoredDeployBaseTask.objectMapper.writeValueAsString(httpException.getResponseBody()))
.thenThrow(jpe);

String logMessageOnSpinHttpException = monitoredDeployBaseTask.getErrorMessage(httpException);
String body = jpe.getMessage();

assertThat(logMessageOnSpinHttpException)
.isEqualTo(
String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body));
}

static class MockTypedInput implements TypedInput {
Expand Down
1 change: 1 addition & 0 deletions orca-deploymentmonitor/orca-deploymentmonitor.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ apply from: "$rootDir/gradle/spock.gradle"
dependencies {
implementation(project(":orca-core"))
implementation(project(":orca-retrofit"))
implementation("io.spinnaker.kork:kork-retrofit")

implementation("org.springframework.boot:spring-boot-autoconfigure")

Expand Down
2 changes: 2 additions & 0 deletions ...n/java/com/netflix/spinnaker/orca/deploymentmonitor/DeploymentMonitorServiceProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.netflix.spinnaker.config.DeploymentMonitorDefinition;
import com.netflix.spinnaker.kork.exceptions.UserException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
import com.netflix.spinnaker.orca.retrofit.logging.RetrofitSlf4jLog;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -81,6 +82,7 @@ private synchronized DeploymentMonitorService getServiceByDefinition(
.setLogLevel(retrofitLogLevel)
.setLog(new RetrofitSlf4jLog(DeploymentMonitorService.class))
.setConverter(new JacksonConverter())
.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
.build()
.create(DeploymentMonitorService.class);

Expand Down

0 comments on commit 133f1c5

Please sign in to comment.