Skip to content
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

refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service #4623

Merged
merged 6 commits into from
Apr 16, 2024
Merged

refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service #4623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a44b5bb
refactor(front50): use SpinnakerRetrofitErrorHandler with Front50Service
Nov 28, 2023
df4215d
refactor(applications): Cleanup RetrofitError in DeleteApplicationTask
Apr 5, 2024
7c3fe41
refactor(igor): Cleanup RetrofitError in GetCommitsTask
Apr 5, 2024
97b31f4
refactor(test/clouddriver): Cleanup impractical tests in TrafficGuard…
Apr 8, 2024
33f1991
refactor(test/applications): Cleanup impractical tests in UpsertAppli…
Apr 10, 2024
b9de3fb
refactor(test): adjust UpsertApplicationTaskSpec tests to use realist…
dbyron-sf Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions ...rc/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import com.netflix.spinnaker.orca.KeelService
import groovy.util.logging.Slf4j
import org.springframework.lang.Nullable
import org.springframework.stereotype.Component
import retrofit.RetrofitError

@Slf4j
@Component
Expand Down Expand Up @@ -76,25 +75,22 @@ class DeleteApplicationTask extends AbstractFront50Task {
front50Service.delete(application.name)
try {
front50Service.deletePermission(application.name)
} catch (RetrofitError re) {
if (re.response?.status == 404) {
} catch (SpinnakerHttpException re) {
if (re.responseCode == 404) {
return TaskResult.SUCCEEDED
}
log.error("Could not delete application permission", re)
return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
} catch (SpinnakerServerException e){
log.error("Could not delete application permission", e)
return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
}
// delete Managed Delivery data
if (keelService != null) {
log.debug("Deleting Managed Delivery data for {}", application.name)
keelService.deleteDeliveryConfig(application.name)
}
}
} catch (RetrofitError e) {
if (e.response?.status == 404) {
return TaskResult.SUCCEEDED
}
log.error("Could not delete application", e)
return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
} catch (SpinnakerHttpException httpException){
if (httpException.responseCode == 404) {
return TaskResult.SUCCEEDED
Expand Down
20 changes: 18 additions & 2 deletions ...est/groovy/com/netflix/spinnaker/orca/applications/tasks/UpsertApplicationTaskSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,12 @@ package com.netflix.spinnaker.orca.applications.tasks

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
import com.netflix.spinnaker.orca.front50.Front50Service
import com.netflix.spinnaker.orca.front50.model.Application
import retrofit.RetrofitError
import retrofit.client.Response
import spock.lang.Specification
import spock.lang.Subject
import spock.lang.Unroll
Expand Down Expand Up @@ -57,7 +60,7 @@ class UpsertApplicationTaskSpec extends Specification {
given:
def app = new Application(config.application + [user: config.user])
task.front50Service = Mock(Front50Service) {
1 * get(config.application.name) >> null
1 * get(config.application.name) >> { throw notFoundError() }
1 * create(app)
1 * updatePermission(*_)
0 * _._
Expand Down Expand Up @@ -99,7 +102,11 @@ class UpsertApplicationTaskSpec extends Specification {
application.user = config.user

task.front50Service = Mock(Front50Service) {
1 * get(config.application.name) >> initialState
if (initialState == null) {
1 * get(config.application.name) >> { throw notFoundError() }
} else {
1 * get(config.application.name) >> initialState
}
1 * "${operation}"(*_)
_ * updatePermission(*_)
0 * _._
Expand Down Expand Up @@ -150,4 +157,13 @@ class UpsertApplicationTaskSpec extends Specification {
[:] || 1
[READ: ["[email protected]"], WRITE: ["[email protected]"]] || 1
}

private static SpinnakerHttpException notFoundError() {
return new SpinnakerHttpException(RetrofitError.httpError(
"http://front50",
new Response("http://front50", 404, "Not Found", [], null),
null,
null
))
}
}
4 changes: 2 additions & 2 deletions orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/CreateBakeTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.kork.artifacts.model.Artifact
import com.netflix.spinnaker.kork.core.RetrySupport
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.RetryableTask
import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
Expand All @@ -41,7 +42,6 @@ import org.slf4j.Logger
import org.slf4j.LoggerFactory
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.stereotype.Component
import retrofit.RetrofitError


import static com.netflix.spinnaker.kork.web.selector.v2.SelectableService.*
Expand Down Expand Up @@ -86,7 +86,7 @@ class CreateBakeTask implements RetryableTask {
stage.context.user = user
}
}
} catch (RetrofitError e) {
} catch (SpinnakerServerException e) {
// ignore exception, we will just use the owner passed to us
if (!e.message.contains("404")) {
dbyron-sf marked this conversation as resolved.
Show resolved Hide resolved
log.warn("Error retrieving application {} from front50, ignoring.", stage.execution.application, e)
Expand Down
7 changes: 1 addition & 6 deletions ...c/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/job/WaitOnJobCompletion.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import org.slf4j.Logger
import org.slf4j.LoggerFactory

import org.springframework.stereotype.Component
import retrofit.RetrofitError

import javax.annotation.Nonnull
import javax.annotation.Nullable
Expand Down Expand Up @@ -198,11 +197,7 @@ public class WaitOnJobCompletion implements CloudProviderAware, OverridableTimeo
if (appName == null || front50Service == null) {
return false
}
try {
return front50Service.get(appName) != null
} catch (RetrofitError e) {
throw e
}
return front50Service.get(appName) != null
}

/**
Expand Down
9 changes: 6 additions & 3 deletions ...main/java/com/netflix/spinnaker/orca/clouddriver/pollers/EphemeralServerGroupsPoller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.kork.core.RetrySupport;
import com.netflix.spinnaker.kork.exceptions.SystemException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType;
import com.netflix.spinnaker.orca.clouddriver.CloudDriverService;
import com.netflix.spinnaker.orca.clouddriver.config.PollerConfigurationProperties;
Expand All @@ -49,7 +51,6 @@
import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Component;
import retrofit.RetrofitError;

@Slf4j
@Component
Expand Down Expand Up @@ -277,11 +278,13 @@ private Map<String, Object> buildDestroyServerGroupOperation(
private Optional<Application> getApplication(String applicationName) {
try {
return Optional.of(front50Service.get(applicationName));
} catch (RetrofitError e) {
if (e.getResponse().getStatus() == HttpStatus.NOT_FOUND.value()) {
} catch (SpinnakerHttpException e) {
if (e.getResponseCode() == HttpStatus.NOT_FOUND.value()) {
return Optional.empty();
}
throw new SystemException(format("Failed to retrieve application '%s'", applicationName), e);
} catch (SpinnakerServerException e) {
throw new SystemException(format("Failed to retrieve application '%s'", applicationName), e);
}
}

Expand Down
7 changes: 1 addition & 6 deletions ...-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/job/JobUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import java.util.Optional;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import retrofit.RetrofitError;

@Component
public class JobUtils implements CloudProviderAware {
Expand Down Expand Up @@ -93,10 +92,6 @@ private Boolean applicationExists(String appName) {
if (appName == null || front50Service == null) {
return false;
}
try {
return front50Service.get(appName) != null;
} catch (RetrofitError e) {
throw e;
}
return front50Service.get(appName) != null;
}
}
7 changes: 3 additions & 4 deletions ...-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.netflix.spectator.api.Registry;
import com.netflix.spectator.impl.Preconditions;
import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
import com.netflix.spinnaker.moniker.Moniker;
import com.netflix.spinnaker.orca.clouddriver.CloudDriverService;
import com.netflix.spinnaker.orca.clouddriver.model.Cluster;
Expand All @@ -39,7 +40,6 @@
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import retrofit.RetrofitError;

@Component
public class TrafficGuard {
Expand Down Expand Up @@ -466,10 +466,9 @@ public boolean hasDisableLock(Moniker clusterMoniker, String account, Location l
Application application;
try {
application = front50Service.get(clusterMoniker.getApp());
} catch (RetrofitError e) {
} catch (SpinnakerHttpException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a test in TrafficGuardSpec that doesn't strictly need to change because of what's happening in this PR. It does look like a broken test though...and there's a unnecessary null check of application a few lines below here. Because Front50Service.get returns a "real" type (i.e. not a Response), it never returns null. So it doesn't make sense to have a test that mocks a null return value. Can you clean this up please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a similar thing happens in UpsertApplicationTaskSpec ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating TrafficGuardSpec. The code below still has a null check that doesn't need to be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for updating TrafficGuardSpec. The code below still has a null check that doesn't need to be there.

Are you talking about this line : https://github.com/Pranav-b-7/orca/blob/0c986730d3918caf7b9d8509dea45cb426d9d81a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuard.java#L477 ?

If yes, this check is still relevant, because in the above catch block when the response code is 404 or 403, application variable is assigned to null.

// ignore an unknown (404) or unauthorized (403) application
if (e.getResponse() != null
&& Arrays.asList(404, 403).contains(e.getResponse().getStatus())) {
if (Arrays.asList(404, 403).contains(e.getResponseCode())) {
application = null;
} else {
throw e;
Expand Down
12 changes: 2 additions & 10 deletions ...iver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/utils/TrafficGuardSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spectator.api.NoopRegistry
import com.netflix.spectator.api.Registry
import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
import com.netflix.spinnaker.moniker.Moniker
import com.netflix.spinnaker.orca.clouddriver.CloudDriverService
import com.netflix.spinnaker.orca.clouddriver.ModelUtils
Expand Down Expand Up @@ -445,15 +446,6 @@ class TrafficGuardSpec extends Specification {
"app" | "test" | null | "zz" | "test" | "us-east-1" || false // different detail
}

void "hasDisableLock returns false on missing applications"() {
when:
boolean result = trafficGuard.hasDisableLock(new Moniker(app: "app", cluster: "app"), "test", location)

then:
result == false
1 * front50Service.get("app") >> null
}

void "hasDisableLock returns false on applications with no guards configured"() {
when:
boolean result = trafficGuard.hasDisableLock(new Moniker(app: "app", cluster: "app"), "test", location)
Expand All @@ -462,7 +454,7 @@ class TrafficGuardSpec extends Specification {
!applicationDetails.containsKey("trafficGuards")
result == false
1 * front50Service.get("app") >> {
throw new RetrofitError(null, null, new Response("http://stash.com", 404, "test reason", [], null), null, null, null, null)
throw new SpinnakerHttpException(new RetrofitError(null, null, new Response("http://stash.com", 404, "test reason", [], null), null, null, null, null))
}
}

Expand Down
1 change: 1 addition & 0 deletions orca-front50/orca-front50.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ dependencies {
implementation("org.springframework.security:spring-security-config")
implementation("org.springframework.security:spring-security-core")
implementation("org.springframework.security:spring-security-web")
implementation("io.spinnaker.kork:kork-retrofit")

compileOnly("org.projectlombok:lombok")
annotationProcessor("org.projectlombok:lombok")
Expand Down
2 changes: 2 additions & 0 deletions ...t50/src/main/groovy/com/netflix/spinnaker/orca/front50/config/Front50Configuration.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
import com.jakewharton.retrofit.Ok3Client
import com.netflix.spinnaker.config.DefaultServiceEndpoint
import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler
import com.netflix.spinnaker.orca.events.ExecutionEvent
import com.netflix.spinnaker.orca.events.ExecutionListenerAdapter
import com.netflix.spinnaker.orca.front50.Front50Service
Expand Down Expand Up @@ -78,6 +79,7 @@ class Front50Configuration {
.setLogLevel(retrofitLogLevel)
.setLog(new RetrofitSlf4jLog(Front50Service))
.setConverter(new JacksonConverter(mapper))
.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
.build()
.create(Front50Service)
}
Expand Down
7 changes: 3 additions & 4 deletions ...ont50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/AbstractFront50Task.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ package com.netflix.spinnaker.orca.front50.tasks

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
import com.netflix.spinnaker.orca.api.pipeline.TaskResult
import com.netflix.spinnaker.orca.front50.Front50Service
import com.netflix.spinnaker.orca.front50.model.Application
import org.springframework.lang.Nullable
import retrofit.RetrofitError

import javax.annotation.Nonnull
import java.util.concurrent.TimeUnit
Expand Down Expand Up @@ -84,11 +84,10 @@ abstract class AbstractFront50Task implements RetryableTask {
Application fetchApplication(String applicationName) {
try {
return front50Service.get(applicationName)
} catch (RetrofitError e) {
if (e.response?.status == 404) {
} catch (SpinnakerHttpException e) {
if (e.responseCode == 404) {
return null
}

throw e
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...0/src/main/java/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static java.lang.String.format;

import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
import com.netflix.spinnaker.orca.api.pipeline.models.Trigger;
import com.netflix.spinnaker.orca.front50.Front50Service;
Expand All @@ -30,7 +31,6 @@
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import retrofit.RetrofitError;

@Component
public class EnabledPipelineValidator implements PipelineValidator {
Expand Down Expand Up @@ -73,7 +73,7 @@ public void checkRunnable(PipelineExecution pipeline) {
}

return;
} catch (RetrofitError ignored) {
} catch (SpinnakerServerException ignored) {
// treat a failure to fetch pipeline config history as non-fatal and fallback to the
// previous behavior
// (handles the fast property case where the supplied pipeline config id does _not_
Expand Down
7 changes: 3 additions & 4 deletions ...nt50/src/main/java/com/netflix/spinnaker/orca/front50/tasks/DeleteDeliveryConfigTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
import com.netflix.spinnaker.orca.api.pipeline.Task;
import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
Expand All @@ -14,7 +15,6 @@
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import retrofit.RetrofitError;

@Component
public class DeleteDeliveryConfigTask implements Task {
Expand Down Expand Up @@ -62,10 +62,9 @@ public Optional<DeliveryConfig> getDeliveryConfig(String id) {
try {
DeliveryConfig deliveryConfig = front50Service.getDeliveryConfig(id);
return Optional.of(deliveryConfig);
} catch (RetrofitError e) {
} catch (SpinnakerHttpException e) {
// ignore an unknown (404) or unauthorized (403, 401)
if (e.getResponse() != null
&& Arrays.asList(404, 403, 401).contains(e.getResponse().getStatus())) {
if (Arrays.asList(404, 403, 401).contains(e.getResponseCode())) {
return Optional.empty();
} else {
throw e;
Expand Down
11 changes: 5 additions & 6 deletions orca-front50/src/main/java/com/netflix/spinnaker/orca/front50/tasks/MonitorFront50Task.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
import com.netflix.spinnaker.orca.api.pipeline.RetryableTask;
import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
Expand All @@ -36,7 +37,6 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Component;
import retrofit.RetrofitError;

@Component
public class MonitorFront50Task implements RetryableTask {
Expand Down Expand Up @@ -162,8 +162,8 @@ private TaskResult monitor(
private Optional<Map<String, Object>> getPipeline(String id) {
try {
return Optional.of(front50Service.getPipeline(id));
} catch (RetrofitError e) {
if (e.getResponse() != null && e.getResponse().getStatus() == HTTP_NOT_FOUND) {
} catch (SpinnakerHttpException e) {
if (e.getResponseCode() == HTTP_NOT_FOUND) {
return Optional.empty();
}
throw e;
Expand All @@ -175,10 +175,9 @@ private Optional<Map<String, Object>> getDeliveryConfig(String id) {
try {
DeliveryConfig deliveryConfig = front50Service.getDeliveryConfig(id);
return Optional.of(objectMapper.convertValue(deliveryConfig, Map.class));
} catch (RetrofitError e) {
} catch (SpinnakerHttpException e) {
// ignore an unknown (404) or unauthorized (403, 401)
if (e.getResponse() != null
&& Arrays.asList(404, 403, 401).contains(e.getResponse().getStatus())) {
if (Arrays.asList(404, 403, 401).contains(e.getResponseCode())) {
return Optional.empty();
} else {
throw e;
Expand Down
Loading