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.

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

fix(exceptions): Utilize exception handler in kork #1727

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions gate-core/gate-core.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ dependencies {

implementation "io.spinnaker.kork:kork-artifacts"
implementation "io.spinnaker.kork:kork-plugins"
implementation "io.spinnaker.kork:kork-retrofit"
implementation "com.jakewharton.retrofit:retrofit1-okhttp3-client:1.1.0"
implementation "com.squareup.retrofit:converter-jackson"
implementation "com.squareup.okhttp:okhttp"
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,20 @@

package com.netflix.spinnaker.gate.services;


import com.netflix.spinnaker.fiat.model.UserPermission;
import com.netflix.spinnaker.fiat.model.resources.Role;
import com.netflix.spinnaker.fiat.model.resources.ServiceAccount;
import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator;
import com.netflix.spinnaker.fiat.shared.FiatService;
import com.netflix.spinnaker.fiat.shared.FiatStatus;
import com.netflix.spinnaker.gate.retrofit.UpstreamBadRequest;
import com.netflix.spinnaker.gate.security.SpinnakerUser;
import com.netflix.spinnaker.gate.services.internal.ExtendedFiatService;
import com.netflix.spinnaker.kork.core.RetrySupport;
import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
import com.netflix.spinnaker.kork.exceptions.SystemException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
import com.netflix.spinnaker.kork.retrofit.exceptions.UpstreamBadRequest;
import com.netflix.spinnaker.security.AuthenticatedRequest;
import com.netflix.spinnaker.security.User;
import java.time.Duration;
Expand Down Expand Up @@ -83,7 +85,7 @@ public void login(final String userId) {
permissionEvaluator.invalidatePermission(userId);
return null;
});
} catch (RetrofitError e) {
} catch (SpinnakerServerException e) {
throw UpstreamBadRequest.classifyError(e);
}
}
Expand All @@ -98,7 +100,7 @@ public void loginWithRoles(final String userId, final Collection<String> roles)
permissionEvaluator.invalidatePermission(userId);
return null;
});
} catch (RetrofitError e) {
} catch (SpinnakerServerException e) {
throw UpstreamBadRequest.classifyError(e);
}
}
Expand All @@ -109,7 +111,7 @@ public void logout(String userId) {
try {
getFiatServiceForLogin().logoutUser(userId);
permissionEvaluator.invalidatePermission(userId);
} catch (RetrofitError e) {
} catch (SpinnakerServerException e) {
throw UpstreamBadRequest.classifyError(e);
}
}
Expand All @@ -119,7 +121,7 @@ public void sync() {
if (fiatStatus.isEnabled()) {
try {
getFiatServiceForLogin().sync(List.of());
} catch (RetrofitError e) {
} catch (SpinnakerServerException e) {
throw UpstreamBadRequest.classifyError(e);
}
}
Expand All @@ -133,21 +135,27 @@ public Set<Role.View> getRoles(String userId) {
var permission = permissionEvaluator.getPermission(userId);
var roles = permission != null ? permission.getRoles() : null;
return roles != null ? roles : Set.of();
} catch (RetrofitError e) {
} catch (SpinnakerServerException e) {
throw UpstreamBadRequest.classifyError(e);
}
}

List<UserPermission.View> lookupServiceAccounts(String userId) {
try {
return extendedFiatService.getUserServiceAccounts(userId);
} catch (RetrofitError re) {
var response = re.getResponse();
if (response != null && response.getStatus() == HttpStatus.NOT_FOUND.value()) {
return List.of();
} catch (SpinnakerServerException e) {
RetrofitError re = e.getRetrofitError();
boolean shouldRetry = false;

if (re != null) {
var response = re.getResponse();
if (response != null && response.getStatus() == HttpStatus.NOT_FOUND.value()) {
return List.of();
}
shouldRetry =
response == null || HttpStatus.valueOf(response.getStatus()).is5xxServerError();
}
boolean shouldRetry =
response == null || HttpStatus.valueOf(response.getStatus()).is5xxServerError();

throw new SystemException("getUserServiceAccounts failed", re).setRetryable(shouldRetry);
}
}
Expand Down Expand Up @@ -217,8 +225,8 @@ public List<String> getServiceAccounts(@SpinnakerUser User user) {
return permission.getServiceAccounts().stream()
.map(ServiceAccount.View::getName)
.collect(Collectors.toList());
} catch (RetrofitError re) {
throw UpstreamBadRequest.classifyError(re);
} catch (SpinnakerServerException e) {
throw UpstreamBadRequest.classifyError(e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator
import com.netflix.spinnaker.fiat.shared.FiatStatus
import com.netflix.spinnaker.gate.services.internal.ExtendedFiatService
import com.netflix.spinnaker.kork.exceptions.SpinnakerException
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
import com.netflix.spinnaker.security.User
import retrofit.RetrofitError
import retrofit.client.Response
Expand Down Expand Up @@ -83,29 +84,30 @@ class PermissionServiceSpec extends Specification {
unexpectedError() || true
}

private RetrofitError conversionError(int code) {
RetrofitError.conversionError(
private static SpinnakerServerException conversionError(int code) {
new SpinnakerServerException(RetrofitError.conversionError(
'http://foo',
new Response('http://foo', code, 'you are bad', [], null),
null,
null,
new ConversionException('boom'))
new ConversionException('boom')))
}

private RetrofitError networkError() {
RetrofitError.networkError('http://foo', new IOException())
private static SpinnakerServerException networkError() {
new SpinnakerServerException(
RetrofitError.networkError('http://foo', new IOException()))
}

private RetrofitError unexpectedError() {
RetrofitError.unexpectedError('http://foo', new Throwable())
private static SpinnakerServerException unexpectedError() {
new SpinnakerServerException(RetrofitError.unexpectedError('http://foo', new Throwable()))
}

private RetrofitError httpRetrofitError(int code) {
RetrofitError.httpError(
private static SpinnakerServerException httpRetrofitError(int code) {
new SpinnakerServerException(RetrofitError.httpError(
'http://foo',
new Response('http://foo', code, 'you are bad', [], null),
null,
null)
null))
}


Expand Down
2 changes: 2 additions & 0 deletions gate-iap/gate-iap.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ dependencies {
implementation project(":gate-core")
implementation 'com.nimbusds:nimbus-jose-jwt'
implementation "com.github.ben-manes.caffeine:guava"
implementation "io.spinnaker.kork:kork-exceptions"
implementation "io.spinnaker.kork:kork-retrofit"
implementation "io.spinnaker.kork:kork-security"
implementation "io.spinnaker.fiat:fiat-api:$fiatVersion"
implementation "io.spinnaker.fiat:fiat-core:$fiatVersion"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.netflix.spinnaker.gate.security.iap.IapSsoConfig.IapSecurityConfigProperties;
import com.netflix.spinnaker.gate.services.PermissionService;
import com.netflix.spinnaker.gate.services.internal.Front50Service;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
import com.netflix.spinnaker.security.User;
import com.nimbusds.jose.JWSHeader;
import com.nimbusds.jose.JWSVerifier;
Expand Down Expand Up @@ -50,7 +51,6 @@
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken;
import org.springframework.web.filter.OncePerRequestFilter;
import retrofit.RetrofitError;

/**
* * This filter verifies the request header from Cloud IAP containing a JWT token, after the user
Expand Down Expand Up @@ -140,8 +140,8 @@ private boolean isServiceAccount(String email) {
}
}
return false;
} catch (RetrofitError re) {
log.warn("Could not get list of service accounts.", re);
} catch (SpinnakerServerException e) {
log.warn("Could not get list of service accounts.", e);
}
return false;
}
Expand Down
1 change: 1 addition & 0 deletions gate-oauth2/gate-oauth2.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ dependencies {
implementation "com.netflix.spectator:spectator-api"
implementation "io.spinnaker.fiat:fiat-api:$fiatVersion"
implementation "io.spinnaker.kork:kork-exceptions"
implementation "io.spinnaker.kork:kork-retrofit"
implementation "io.spinnaker.kork:kork-security"
implementation "org.codehaus.groovy:groovy-json"
implementation "org.springframework.security.oauth.boot:spring-security-oauth2-autoconfigure"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import com.netflix.spinnaker.gate.services.CredentialsService
import com.netflix.spinnaker.gate.services.PermissionService
import com.netflix.spinnaker.gate.services.internal.Front50Service
import com.netflix.spinnaker.kork.core.RetrySupport
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
import com.netflix.spinnaker.security.User
import groovy.json.JsonSlurper
import groovy.util.logging.Slf4j
Expand All @@ -39,7 +40,6 @@ import org.springframework.security.oauth2.provider.OAuth2Authentication
import org.springframework.security.oauth2.provider.OAuth2Request
import org.springframework.security.oauth2.provider.token.ResourceServerTokenServices
import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken
import retrofit.RetrofitError

import java.util.function.BiFunction
import java.util.regex.Pattern
Expand Down Expand Up @@ -188,8 +188,8 @@ class SpinnakerUserInfoTokenServices implements ResourceServerTokenServices {
try {
def serviceAccounts = front50Service.getServiceAccounts()
return serviceAccounts.find { email.equalsIgnoreCase(it.name) }
} catch (RetrofitError re) {
log.warn("Could not get list of service accounts.", re)
} catch (SpinnakerServerException e) {
log.warn("Could not get list of service accounts.", e)
}
return false
}
Expand Down
2 changes: 2 additions & 0 deletions gate-plugins/gate-plugins.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ dependencies {
implementation "com.google.guava:guava"
implementation "io.spinnaker.fiat:fiat-core:$fiatVersion"
implementation "io.spinnaker.fiat:fiat-api:$fiatVersion"
implementation "io.spinnaker.kork:kork-hubble"
implementation "io.spinnaker.kork:kork-plugins"
implementation "io.spinnaker.kork:kork-retrofit"
implementation "io.spinnaker.kork:kork-web"

implementation "io.swagger:swagger-annotations"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import com.netflix.spinnaker.gate.services.internal.SwabbieService
import com.netflix.spinnaker.kork.plugins.SpinnakerPluginDescriptor
import com.netflix.spinnaker.kork.plugins.SpinnakerPluginManager
import com.netflix.spinnaker.kork.plugins.update.SpinnakerUpdateManager
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
import io.swagger.annotations.ApiOperation
import java.util.stream.Collectors
import org.pf4j.PluginWrapper
Expand All @@ -22,7 +23,6 @@ import org.springframework.web.bind.annotation.RequestMapping
import org.springframework.web.bind.annotation.RequestMethod
import org.springframework.web.bind.annotation.RequestParam
import org.springframework.web.bind.annotation.RestController
import retrofit.RetrofitError

@RestController
@RequestMapping("/plugins/installed")
Expand Down Expand Up @@ -122,8 +122,8 @@ class PluginsInstalledController(
fun callService(call: () -> List<SpinnakerPluginDescriptor>): List<SpinnakerPluginDescriptor> {
return try {
call()
} catch (e: RetrofitError) {
log.warn("Unable to retrieve installed plugins from '${e.response?.url}' due to '${e.response?.status}'")
} catch (e: SpinnakerServerException) {
log.warn("Unable to retrieve installed plugins from '${e.retrofitError?.response?.url}' due to '${e.retrofitError?.response?.status}'")
return emptyList()
}
}
Expand Down
1 change: 1 addition & 0 deletions gate-web/gate-web.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ dependencies {
implementation "io.spinnaker.kork:kork-core"
implementation "io.spinnaker.kork:kork-config"
implementation "io.spinnaker.kork:kork-plugins"
implementation "io.spinnaker.kork:kork-retrofit"
implementation "io.spinnaker.kork:kork-web"
implementation "com.netflix.frigga:frigga"
implementation "redis.clients:jedis"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@ import com.netflix.spinnaker.gate.filters.ContentCachingFilter
import com.netflix.spinnaker.gate.interceptors.RequestContextInterceptor
import com.netflix.spinnaker.gate.interceptors.ResponseHeaderInterceptor
import com.netflix.spinnaker.gate.interceptors.ResponseHeaderInterceptorConfigurationProperties
import com.netflix.spinnaker.gate.retrofit.UpstreamBadRequest
import com.netflix.spinnaker.kork.retrofit.exceptions.UpstreamBadRequest
import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
import com.netflix.spinnaker.kork.web.interceptors.MetricsInterceptor
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.beans.factory.annotation.Value
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
import org.springframework.boot.context.properties.EnableConfigurationProperties
import org.springframework.context.ApplicationContext
import org.springframework.context.annotation.Bean
Expand All @@ -40,7 +41,6 @@ import org.springframework.web.servlet.config.annotation.ContentNegotiationConfi
import org.springframework.web.servlet.config.annotation.InterceptorRegistry
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer
import org.springframework.web.servlet.handler.HandlerMappingIntrospector
import retrofit.RetrofitError

import javax.servlet.Filter
import javax.servlet.http.HttpServletResponse
Expand Down Expand Up @@ -105,9 +105,9 @@ public class GateWebConfig implements WebMvcConfigurer {

def message = exception.message
def failureCause = exception.cause
if (failureCause instanceof RetrofitError) {
if (failureCause instanceof SpinnakerServerException) {
try {
def retrofitBody = failureCause.getBodyAs(Map) as Map
def retrofitBody = failureCause.retrofitError.getBodyAs(Map) as Map
message = retrofitBody.error ?: message
} catch (Exception ignored) {
// unable to extract error from response
Expand Down
Loading
Loading