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

Config simplifications #1096

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Config simplifications #1096

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a7441fc
refactor(web): Remove unnecessary import
jvz Aug 25, 2023
050734a
refactor(web): Simplify RetrofitConfig
jvz Aug 25, 2023
cbe0d15
refactor(web): Simplify FiatConfig
jvz Aug 25, 2023
7e5f07e
refactor(web): Simplify ResourcesConfig
jvz Aug 25, 2023
06c5ef6
refactor(api): Simplify FiatAuthenticationConfig
jvz Aug 25, 2023
0e302d8
fix(api): Use same anonymous principal as Gate
jvz Aug 25, 2023
735abc1
Merge branch 'master' into config-simplifications
jvz Aug 31, 2023
d36e939
Merge branch 'master' into config-simplifications
jasonmcintosh Sep 6, 2023
dca80be
Merge branch 'master' into config-simplifications
jvz Sep 15, 2023
8b1253a
Merge branch 'master' into config-simplifications
jasonmcintosh Oct 3, 2023
8f25053
Merge branch 'master' into config-simplifications
jasonmcintosh Oct 10, 2023
723eade
Merge branch 'master' into config-simplifications
jasonmcintosh Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
75 changes: 12 additions & 63 deletions fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,11 @@

package com.netflix.spinnaker.fiat.config;

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.fiat.providers.ProviderHealthTracker;
import com.netflix.spinnaker.fiat.providers.internal.*;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger;
import lombok.Setter;
import org.springframework.beans.factory.annotation.Autowired;
import com.netflix.spinnaker.kork.client.ServiceClientProvider;
import lombok.RequiredArgsConstructor;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.beans.factory.config.ConfigurableBeanFactory;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
Expand All @@ -35,45 +30,18 @@
import org.springframework.context.annotation.Primary;
import org.springframework.context.annotation.PropertySource;
import org.springframework.context.annotation.Scope;
import retrofit.Endpoints;
import retrofit.RestAdapter;
import retrofit.converter.JacksonConverter;

@Configuration
@EnableConfigurationProperties(ProviderCacheConfig.class)
@PropertySource("classpath:resilience4j-defaults.properties")
@RequiredArgsConstructor
public class ResourcesConfig {
@Autowired @Setter private RestAdapter.LogLevel retrofitLogLevel;

@Autowired @Setter private ObjectMapper objectMapper;

@Autowired @Setter private OkHttpClientProvider clientProvider;

@Value("${services.front50.base-url}")
@Setter
private String front50Endpoint;

@Value("${services.clouddriver.base-url}")
@Setter
private String clouddriverEndpoint;

@Value("${services.igor.base-url}")
@Setter
private String igorEndpoint;
private final ServiceClientProvider provider;

@Bean
Front50Api front50Api() {
return new RestAdapter.Builder()
.setEndpoint(Endpoints.newFixedEndpoint(front50Endpoint))
.setClient(
new Ok3Client(
clientProvider.getClient(new DefaultServiceEndpoint("front50", front50Endpoint))))
.setConverter(new JacksonConverter(objectMapper))
.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
.setLogLevel(retrofitLogLevel)
.setLog(new Slf4jRetrofitLogger(Front50Api.class))
.build()
.create(Front50Api.class);
Front50Api front50Api(@Value("${services.front50.base-url}") String front50Endpoint) {
return provider.getService(
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this ends up with RetrofitServiceFactory creating the returned object. At the moment that code doesn't use

.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())

so this ends up being a change in behavior. Adding the setErrorHandler call to RetrofitServiceFactory is in the plans, but is gonna take some care (and potentially a config flag controlling whether it happens or not) since I believe both gate and echo use it, and we need to coordinate changes in handling of RetrofitError to Spinnaker*Exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the new error handler for running in Retrofit2? Right now, ServiceClientFactory implementations check for the use of the Call<T> API for determining which Retrofit version to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

SpinnakerRetrofitErrorHandler is for retrofit1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But Retrofit only throws RetrofitError in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, retrofit1 throws RetrofitError, and SpinnakerRetrofitErrorHandler turns RetrofitError into SpinnakerServerException (and children).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, so the point of the error handler Bean is to translate the retrofit exception into a spinnaker one?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a bean, but yes.

Front50Api.class, new DefaultServiceEndpoint("front50", front50Endpoint));
}

@Bean
Expand All @@ -96,19 +64,10 @@ Front50Service front50Service(
}

@Bean
ClouddriverApi clouddriverApi() {
return new RestAdapter.Builder()
.setEndpoint(Endpoints.newFixedEndpoint(clouddriverEndpoint))
.setClient(
new Ok3Client(
clientProvider.getClient(
new DefaultServiceEndpoint("clouddriver", clouddriverEndpoint))))
.setConverter(new JacksonConverter(objectMapper))
.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
.setLogLevel(retrofitLogLevel)
.setLog(new Slf4jRetrofitLogger(ClouddriverApi.class))
.build()
.create(ClouddriverApi.class);
ClouddriverApi clouddriverApi(
@Value("${services.clouddriver.base-url}") String clouddriverEndpoint) {
return provider.getService(
ClouddriverApi.class, new DefaultServiceEndpoint("clouddriver", clouddriverEndpoint));
}

@Bean
Expand Down Expand Up @@ -154,17 +113,7 @@ ClouddriverService clouddriverServiceWithoutApplicationLoader(
@Bean
@ConditionalOnProperty("services.igor.enabled")
IgorApi igorApi(@Value("${services.igor.base-url}") String igorEndpoint) {
return new RestAdapter.Builder()
.setEndpoint(Endpoints.newFixedEndpoint(igorEndpoint))
.setClient(
new Ok3Client(
clientProvider.getClient(new DefaultServiceEndpoint("igor", igorEndpoint))))
.setConverter(new JacksonConverter(objectMapper))
.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
.setLogLevel(retrofitLogLevel)
.setLog(new Slf4jRetrofitLogger(IgorApi.class))
.build()
.create(IgorApi.class);
return provider.getService(IgorApi.class, new DefaultServiceEndpoint("igor", igorEndpoint));
}

@Bean
Expand Down