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

Config simplifications #1096

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

Config simplifications #1096

Show file tree
Hide file tree
Changes from all commits
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 16 additions & 43 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,14 @@
package com.netflix.spinnaker.fiat.shared;

import static org.springframework.core.Ordered.HIGHEST_PRECEDENCE;
import static org.springframework.security.config.Customizer.withDefaults;

import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.jakewharton.retrofit.Ok3Client;
import com.netflix.spinnaker.config.DefaultServiceEndpoint;
import com.netflix.spinnaker.config.ErrorConfiguration;
import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
import com.netflix.spinnaker.kork.client.ServiceClientProvider;
import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator;
import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor;
import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger;
import lombok.Setter;
import lombok.val;
import okhttp3.OkHttpClient;
import org.springframework.beans.factory.annotation.Autowired;
import java.util.Optional;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
Expand All @@ -40,15 +33,13 @@
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Import;
import org.springframework.core.annotation.Order;
import org.springframework.http.converter.json.Jackson2ObjectMapperBuilder;
import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity;
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
import org.springframework.security.web.authentication.AnonymousAuthenticationFilter;
import org.springframework.security.web.authentication.AuthenticationConverter;
import retrofit.Endpoints;
import retrofit.RestAdapter;
import retrofit.converter.JacksonConverter;

@Import(ErrorConfiguration.class)
@EnableWebSecurity
Expand All @@ -58,35 +49,20 @@
@ComponentScan("com.netflix.spinnaker.fiat.shared")
public class FiatAuthenticationConfig {

@Autowired(required = false)
@Setter
private RestAdapter.LogLevel retrofitLogLevel = RestAdapter.LogLevel.BASIC;

@Bean
@ConditionalOnMissingBean(FiatService.class) // Allows for override
public FiatService fiatService(
FiatClientConfigurationProperties fiatConfigurationProperties,
SpinnakerRequestInterceptor interceptor,
OkHttpClientProvider okHttpClientProvider) {
ServiceClientProvider provider,
Optional<Jackson2ObjectMapperBuilder> maybeBuilder) {
// New role providers break deserialization if this is not enabled.
val objectMapper = new ObjectMapper();
objectMapper.enable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL);
objectMapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);

OkHttpClient okHttpClient =
okHttpClientProvider.getClient(
new DefaultServiceEndpoint("fiat", fiatConfigurationProperties.getBaseUrl()));

return new RestAdapter.Builder()
.setEndpoint(Endpoints.newFixedEndpoint(fiatConfigurationProperties.getBaseUrl()))
.setRequestInterceptor(interceptor)
.setClient(new Ok3Client(okHttpClient))
.setConverter(new JacksonConverter(objectMapper))
.setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
.setLogLevel(retrofitLogLevel)
.setLog(new Slf4jRetrofitLogger(FiatService.class))
.build()
.create(FiatService.class);
var objectMapper =
maybeBuilder
.orElseGet(Jackson2ObjectMapperBuilder::json)
.featuresToEnable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL)
.build();
var endpoint = new DefaultServiceEndpoint("fiat", fiatConfigurationProperties.getBaseUrl());
return provider.getService(FiatService.class, endpoint, objectMapper);
}

/**
Expand Down Expand Up @@ -138,12 +114,9 @@ private FiatWebSecurityConfigurerAdapter(

@Override
protected void configure(HttpSecurity http) throws Exception {
http.servletApi()
.and()
.exceptionHandling()
.and()
.anonymous()
.and()
http.servletApi(withDefaults())
.exceptionHandling(withDefaults())
.anonymous(anonymous -> anonymous.principal("anonymous"))
.addFilterBefore(
new FiatAuthenticationFilter(fiatStatus, authenticationConverter),
AnonymousAuthenticationFilter.class);
Expand Down
3 changes: 0 additions & 3 deletions fiat-web/src/main/java/com/netflix/spinnaker/fiat/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package com.netflix.spinnaker.fiat;

import com.netflix.spinnaker.config.ErrorConfiguration;
import com.netflix.spinnaker.kork.boot.DefaultPropertiesBuilder;
import java.util.Map;
import org.springframework.boot.actuate.autoconfigure.ldap.LdapHealthContributorAutoConfiguration;
Expand All @@ -27,7 +26,6 @@
import org.springframework.boot.web.servlet.support.SpringBootServletInitializer;
import org.springframework.context.annotation.ComponentScan;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Import;
import org.springframework.scheduling.annotation.EnableScheduling;

@EnableScheduling
Expand All @@ -36,7 +34,6 @@
"com.netflix.spinnaker.fiat",
"com.netflix.spinnaker.config",
})
@Import(ErrorConfiguration.class)
@EnableAutoConfiguration(
exclude = {
GsonAutoConfiguration.class,
Expand Down
19 changes: 8 additions & 11 deletions fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.netflix.spinnaker.fiat.config;

import com.google.common.collect.ImmutableList;
import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.config.PluginsAutoConfiguration;
import com.netflix.spinnaker.fiat.model.Authorization;
Expand All @@ -24,7 +23,6 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import lombok.val;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
Expand All @@ -40,28 +38,27 @@
import org.springframework.scheduling.concurrent.ThreadPoolTaskScheduler;
import org.springframework.web.servlet.config.annotation.ContentNegotiationConfigurer;
import org.springframework.web.servlet.config.annotation.InterceptorRegistry;
import org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter;
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;

@Configuration
@Import({RetrofitConfig.class, PluginsAutoConfiguration.class})
@EnableConfigurationProperties(FiatServerConfigurationProperties.class)
public class FiatConfig extends WebMvcConfigurerAdapter {
public class FiatConfig implements WebMvcConfigurer {

@Autowired private Registry registry;

@Override
public void addInterceptors(InterceptorRegistry registry) {
var pathVarsToTag = ImmutableList.of("accountName", "applicationName", "resourceName");
List<String> exclude = ImmutableList.of("BasicErrorController");
var pathVarsToTag = List.of("accountName", "applicationName", "resourceName");
List<String> exclude = List.of("BasicErrorController");
MetricsInterceptor interceptor =
new MetricsInterceptor(this.registry, "controller.invocations", pathVarsToTag, exclude);
registry.addInterceptor(interceptor);
}

@Override
public void configureContentNegotiation(ContentNegotiationConfigurer configurer) {
super.configureContentNegotiation(configurer);
configurer.favorPathExtension(false).defaultContentType(MediaType.APPLICATION_JSON);
configurer.defaultContentType(MediaType.APPLICATION_JSON);
}

@Bean
Expand Down Expand Up @@ -123,11 +120,11 @@ public DefaultServiceAccountPredicateProvider defaultServiceAccountPredicateProv

/**
* This AuthenticatedRequestFilter pulls the email and accounts out of the Spring security context
* in order to enabling forwarding them to downstream components.
* in order to enable forwarding them to downstream components.
*/
@Bean
FilterRegistrationBean authenticatedRequestFilter() {
val frb = new FilterRegistrationBean(new AuthenticatedRequestFilter(true));
FilterRegistrationBean<AuthenticatedRequestFilter> authenticatedRequestFilter() {
var frb = new FilterRegistrationBean<>(new AuthenticatedRequestFilter(true));
jasonmcintosh marked this conversation as resolved.
Show resolved Hide resolved
frb.setOrder(Ordered.LOWEST_PRECEDENCE);
return frb;
}
Expand Down
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
32 changes: 6 additions & 26 deletions fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/RetrofitConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,11 @@
package com.netflix.spinnaker.fiat.config;

import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationFeature;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.util.List;
import lombok.AllArgsConstructor;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;
import okhttp3.Interceptor;
import okhttp3.Request;
Expand All @@ -33,37 +30,19 @@
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Primary;
import org.springframework.http.converter.json.Jackson2ObjectMapperBuilder;
import org.springframework.util.backoff.BackOffExecution;
import org.springframework.util.backoff.ExponentialBackOff;
import retrofit.RestAdapter;

/** This package is placed in fiat-core in order to be shared by fiat-web and fiat-shared. */
/** Provides base beans used for configuring Retrofit REST client facades. */
@Configuration
public class RetrofitConfig {

@Value("${ok-http-client.connection-pool.max-idle-connections:5}")
@Setter
private int maxIdleConnections;
jasonmcintosh marked this conversation as resolved.
Show resolved Hide resolved

@Value("${ok-http-client.connection-pool.keep-alive-duration-ms:300000}")
@Setter
private int keepAliveDurationMs;

@Value("${ok-http-client.retry-on-connection-failure:true}")
@Setter
private boolean retryOnConnectionFailure;

@Value("${ok-http-client.retries.max-elapsed-backoff-ms:5000}")
@Setter
private long maxElapsedBackoffMs;

@Bean
@Primary
ObjectMapper objectMapper() {
return new ObjectMapper()
.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
jasonmcintosh marked this conversation as resolved.
Show resolved Hide resolved
.configure(SerializationFeature.INDENT_OUTPUT, true)
.setSerializationInclusion(JsonInclude.Include.NON_NULL);
ObjectMapper objectMapper(Jackson2ObjectMapperBuilder builder) {
return builder.serializationInclusion(JsonInclude.Include.NON_NULL).indentOutput(true).build();
}

@Bean
Expand All @@ -72,7 +51,8 @@ RestAdapter.LogLevel retrofitLogLevel(@Value("${retrofit.log-level:BASIC}") Stri
}

@Bean
RetryingInterceptor retryingInterceptor() {
RetryingInterceptor retryingInterceptor(
@Value("${ok-http-client.retries.max-elapsed-backoff-ms:5000}") long maxElapsedBackoffMs) {
return new RetryingInterceptor(maxElapsedBackoffMs);
}

Expand Down