spring-cloud / spring-cloud-circuitbreaker

Spring Cloud Circuit Breaker API and Implementations
Apache License 2.0
328 stars 109 forks source link

Resilience4jCircuitBreaker ignoring Context set by ContextPropagator #182

Closed Kustosh closed 6 months ago

Kustosh commented 6 months ago

Hello everyone, recently we have upgraded our Spring Cloud version to 2023.0.0 that updated spring-cloud-starter-circuitbreaker-resilience4j from version 3.0.3 to version 3.1.0. After that migration our service was not able to propagate security context when calling external service through OpenFeign client. We were using ContextPropagator to pass the the SecurityContext to the circuitbreaker in order to retrieve custom authentication token and pass it in the next request. It seems that after the upgrade to latest version the thread that is running the Feign request is not populated with context as it was before.

After some investigation we suspect that this change might be responsible. According to some issues from the resilience4j GitHub such as this spawning additional threads after ThreadPoolBulkhead already created one might result in issues that we are observing.

We have found a solution to circumvent this issue by customizing the Resilience4jCircuitBreakerFactory with DelegatingSecurityContextExecutorService, but this would force us to disable the groups as it is not yet supported (https://github.com/spring-cloud/spring-cloud-circuitbreaker/issues/180) and we need to have groups enabled.

For now we have reverted to version 3.0.3 of spring-cloud-starter-circuitbreaker-resilience4j which works with the ContextPropagator. Would it be possible to re-enable support for it?

Sample code below:

Feign client and configuration

@FeignClient(url = "localhost:8090",
    name = "FeignApiClient",
    configuration = {FeignClientConfiguration.class})
public interface FeignApiClient extends ApiDescription {

}
public class FeignClientConfiguration {

    @Bean
    public RequestInterceptor oauth2RequestInterceptor() {
        return new SecurityContextAwareRequestInterceptor();
    }

}

Our custom Feign request interceptor to retrieve the security context and set proper headers (it is just a dummy one, so it does not set the actual values):

public class SecurityContextAwareRequestInterceptor implements RequestInterceptor {

    @Override
    public void apply(final RequestTemplate template) {
        getAuthenticationOrNull().ifPresentOrElse(authentication -> template.header(HttpHeaders.AUTHORIZATION, "Bearer " + authentication), () -> System.out.println("Security context is empty"));
    }

    private Optional<UsernamePasswordAuthenticationToken> getAuthenticationOrNull() {
        return Optional.ofNullable(SecurityContextHolder.getContext()).map(SecurityContext::getAuthentication).map(authentication -> (UsernamePasswordAuthenticationToken) authentication);
    }
}

And our ContextPropagator to set SecurityContext in threads spawned by resilience4j:

public class SecurityContextPropagator implements ContextPropagator<SecurityContext> {

    @Override
    public Supplier<Optional<SecurityContext>> retrieve() {
        return () -> Optional.ofNullable(SecurityContextHolder.getContext());
    }

    @Override
    public Consumer<Optional<SecurityContext>> copy() {
        return context -> SecurityContextHolder.setContext(context.orElse(null));
    }

    @Override
    public Consumer<Optional<SecurityContext>> clear() {
        return context -> {

        };
    }
}
ryanjbaxter commented 6 months ago

Group support should be there. In fact #180 should have been closed by https://github.com/spring-cloud/spring-cloud-circuitbreaker/pull/181

spring-cloud-issues commented 6 months ago

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

Kustosh commented 6 months ago

Group support should be there. In fact #180 should have been closed by #181

Oh, that's great, I missed that the PR was already merged. Then we will switch to this approach when the change is released.

Still, I would appreciate if someone would look into support for ContextPropagator from resilience4j or at least mention in the docs that ContextPropagator is not compatible and other approach is required.

ryanjbaxter commented 6 months ago

Have you tried to see if it works without using OpenFeign?

Kustosh commented 6 months ago

I tried adding something like this:

public String getValue() {
        return circuitBreakerFactory.create("no-feign-circuitbreaker", "no-feign").run(() -> {
            SecurityContext context = SecurityContextHolder.getContext();
            return Optional.ofNullable(context).map(SecurityContext::getAuthentication)
                .filter(authentication -> authentication instanceof UsernamePasswordAuthenticationToken)
                .map(UsernamePasswordAuthenticationToken.class::cast)
                .map(authentication -> {
                    System.out.println("Context is here");
                    return restClient.get().uri("http://localhost:8090").header("Authentication",
                        authentication.getName()).exchange((clientRequest, clientResponse) -> {
                        return new String(clientResponse.getBody().readAllBytes(), StandardCharsets.UTF_8);
                    });
                })
                .orElseGet(() -> {
                    System.out.println("No Context Available");
                    return "";
                });
        }, throwable -> fallback());
    }

    public String fallback() {
        return "Ups, something's broken";
    }

The result is the same, using version 3.0.3 of spring-cloud-circuitbreaker-resilience4j I can access the properly populated SecurityContext within the method wrapped by the circuitbreaker, but using version 3.1.0 results in an empty context. In both cases I can see that the ContextPropagator is called, but only version 3.0.3 actually allows me to access the context.

ryanjbaxter commented 6 months ago

You mentioned...

We have found a solution to circumvent this issue by customizing the Resilience4jCircuitBreakerFactory with DelegatingSecurityContextExecutorService, but this would force us to disable the groups as it is not yet supported (https://github.com/spring-cloud/spring-cloud-circuitbreaker/issues/180) and we need to have groups enabled.

As mentioned above groups are now supported, so does this workaround work for your use case then?

Can you describe how you are customizing Resilience4jCircuitBreakerFactory?

Kustosh commented 6 months ago

Sorry, I think there was some miscommunication there. Yes, the latest code from the master branch works with this workaround when I added following configuration:

@Configuration
public class Resilience4JSecurityContextDelegatingConfig {

    @Bean
    public Customizer<Resilience4JCircuitBreakerFactory> groupExecutorServiceCustomizer() {
        return factory -> factory.configureGroupExecutorService(group -> new DelegatingSecurityContextExecutorService(Executors.newVirtualThreadPerTaskExecutor()));
    }

}

This, along with the ContextPropagator, solves the issue of the missing context, both for the raw spring-cloud-circuitbreaker as well as for the OpenFeign client. But the group support is not yet released if I'm not mistaken?

ryanjbaxter commented 6 months ago

But the group support is not yet released if I'm not mistaken?

Correct, it should be in the 2202.0.5 release which is currently scheduled for the end of the month. Could you try 2202.0.5-SNAPSHOT to see if it addresses the concern?

We can add the Customizer configuration to the docs to document how to make the context propagation work.

Kustosh commented 6 months ago

Yes, the snapshot version together with the Customizer fixes the problem as far as I can tell. Thank you for help and information.

luisalves00 commented 1 month ago

Adding:

@Bean public Customizer<Resilience4JCircuitBreakerFactory> groupExecutorServiceCustomizer() { return factory -> factory.configureGroupExecutorService(group -> new DelegatingSecurityContextExecutorService(Executors.newVirtualThreadPerTaskExecutor())); }

is enough or do we need the ContextPropagator? How to configure it?

gls-luis-alves commented 1 month ago

Tried:

@Bean
    public Customizer< Resilience4JCircuitBreakerFactory > executorServiceCustomizer()
    {
        return factory ->
        {
            SecurityContextPropagator securityContextPropagator = new SecurityContextPropagator();
            final ContextAwareScheduledThreadPoolExecutor.Builder executorBuilder = ContextAwareScheduledThreadPoolExecutor.newScheduledThreadPool();
            executorBuilder.corePoolSize( 6 );
            executorBuilder.contextPropagators( securityContextPropagator );
            factory.configureExecutorService( executorBuilder.build() );
            factory.configureGroupExecutorService( groupName -> executorBuilder.build() );
        };
    }

and would expect my thread to have in the name the following:

THREAD_PREFIX = "ContextAwareScheduledThreadPool"

But from the logs, it seems that the pool that was defined is not the used one:

2024-06-07 10:36:29.926 DEBUG [pool-3-thread-2] - (xxx.business.service.client.DummyClient:72) - [DummyClient#get] ---> GET http://dummy-service/v1/user/abc HTTP/1.1
2024-06-07 10:36:30.927 ERROR [http-nio-8080-exec-4] - (xxx.validation.ErrorHandlingControllerAdvice:66) - Unhandled Exception
org.springframework.cloud.client.circuitbreaker.NoFallbackAvailableException: No fallback available.

Request failed because it had no Bearer token.

gls-luis-alves commented 1 month ago

Finally I've figured out. The Customizer was not applied, because of wrong import.

Make sure you import:

import org.springframework.cloud.client.circuitbreaker.Customizer

Then you can use configuration properties:

resilience4j:
  scheduled:
    executor:
      corePoolSize: 8
      contextPropagators:
        - xxx.config.SecurityContextPropagator

or you can do it programmatically:

    @Bean
    public ContextAwareScheduledThreadPoolExecutor contextAwareScheduledThreadPool(){
           return ContextAwareScheduledThreadPoolExecutor.newScheduledThreadPool()
              .corePoolSize(8)
                  .contextPropagators(new SecurityContextPropagator()).build();
        }

Then like in the Spring Cloud docs, you can use the Customizer:

    @Bean
    public Customizer< Resilience4JCircuitBreakerFactory > executorServiceCustomizer( ContextAwareScheduledThreadPoolExecutor executor )
    {
        log.info( "Configuring Resilience4JCircuitBreaker executor service with context propagation!" );
        return factory ->
        {
            // https://docs.spring.io/spring-cloud-circuitbreaker/reference/spring-cloud-circuitbreaker-resilience4j/specific-circuit-breaker-configuration.html
            // https://docs.spring.io/spring-cloud-circuitbreaker/docs/current/reference/html/spring-cloud-circuitbreaker-resilience4j.html#_customizing_the_executorservice
            factory.configureExecutorService( executor );
            factory.configureGroupExecutorService( groupName -> executor );
        };
    }