spring-cloud / spring-cloud-gateway

An API Gateway built on Spring Framework and Spring Boot providing routing and more.
http://cloud.spring.io
Apache License 2.0
4.53k stars 3.33k forks source link

The issue for discovery locator mode with Hoxton.M3 #1346

Closed HaojunRen closed 5 years ago

HaojunRen commented 5 years ago

I have tried the new version for Spring Cloud Gateway as follows

<spring.cloud.version>Hoxton.M3</spring.cloud.version>
<spring.boot.version>2.2.0.RC1</spring.boot.version>

If I add the following config

spring.cloud.gateway.discovery.locator.enabled=true

The gateway will not work and report error in postman

{
    "timestamp": "2019-10-12T17:51:29.973+0000",
    "path": "/discovery-guide-service-a/invoke/gateway",
    "status": 404,
    "error": "Not Found",
    "message": null,
    "requestId": "0600eee9"
}

But remove it and instead with followings, IT WORKS FINE!

spring.cloud.gateway.routes[0].id=discovery-guide-service-a
spring.cloud.gateway.routes[0].predicates[0]=Path=/discovery-guide-service-a/**
spring.cloud.gateway.routes[0].filters[0]=StripPrefix=1
spring.cloud.gateway.routes[0].uri=lb://discovery-guide-service-a

And helps for this issue? Thanks!

BTW, my code all work fine in Greenwich and Finchley, but fail in Hoxton.M3 with discovery.locator.enabled=true

spencergibb commented 5 years ago

You can add StripPrefix to routes created with discovery. It's in the docs, I'm on mobile or I'd point you too it.

HaojunRen commented 5 years ago

Is this a new config?i dont find IT in docs of Spring Cloud Gateway

HaojunRen commented 5 years ago

My requirement is just adding spring.cloud.gateway.discovery.locator.enabled=true to application.properties,DONT ladd more any other configs, keep the behaviors like G and F versions. Thanks

spencergibb commented 5 years ago

Can you provide a complete, minimal, verifiable sample that reproduces the problem? It should be available as a GitHub (or similar) project or attached to this issue as a zip file.

HaojunRen commented 5 years ago

Please refer to my github https://github.com/Nepxion/DiscoveryGuide and import it in IDE。You should download Alibaba Nacos first,and run it。

In parent pom,you can use Greenwich or Finchley version,they can work fine,but failed in Honxon。

HaojunRen commented 5 years ago

Sorry,fotget to tell an important step:change middleware.host=192.168.0.107 to your local IP in Gateway and Service config file

HaojunRen commented 5 years ago

@spencergibb I have setup an origin demo for spring cloud gateway (without my open source), the issue also would be reproduced. Please confirm it is an issue or not. Many thanks!

HaojunRen commented 5 years ago

Please refer to my pictures for test result:

xinghui322 commented 5 years ago

I encountered the same problem. This maybe a spring boot bug and I added the flowing configuration solved the problem.

    @Bean
    @ConditionalOnMissingBean
    @ConditionalOnProperty(name = "spring.cloud.gateway.discovery.locator.enabled")
    public DiscoveryClientRouteDefinitionLocator discoveryClientRouteDefinitionLocator(
            ReactiveDiscoveryClient discoveryClient,
            DiscoveryLocatorProperties properties) {
        return new DiscoveryClientRouteDefinitionLocator(discoveryClient, properties);
    }
HaojunRen commented 5 years ago

@xinghui322 did u also use M version?

xinghui322 commented 5 years ago

@xinghui322 did u also use M version?

Yes.

HaojunRen commented 5 years ago

@xinghui322 still does not work for your solution with @Bean

xinghui322 commented 5 years ago

@HaojunRen You can check whether the bean DiscoveryClientRouteDefinitionLocator is initialized. org.springframework.cloud.gateway.discovery.GatewayDiscoveryClientAutoConfiguration#discoveryClientRouteDefinitionLocator

HaojunRen commented 5 years ago

@spencergibb Maybe I found the root cause in spring-cloud-gateway-2.2.0.M3, DiscoveryClient will be replaced with ReactiveDiscoveryClient. That is not a better solution, we have do some enhancements for DiscoveryClient. SCG must be backward compatibility with DiscoveryClient

    @Bean
    @ConditionalOnBean(ReactiveDiscoveryClient.class)
    @ConditionalOnProperty(name = "spring.cloud.gateway.discovery.locator.enabled")
    public DiscoveryClientRouteDefinitionLocator discoveryClientRouteDefinitionLocator(
            ReactiveDiscoveryClient discoveryClient,
            DiscoveryLocatorProperties properties) {
        return new DiscoveryClientRouteDefinitionLocator(discoveryClient, properties);
    }

DiscoveryClientRouteDefinitionLocator would not be initialized

OlgaMaciaszek commented 5 years ago

I agree we should be backwards compatible. @TYsewyn Could you look at this?

HaojunRen commented 5 years ago

I suggest the non-reactive mode may be set as the default solution, because that people may not accept reactive at soon. So Spring Cloud can provide a flag of 'spring.cloud.discovery.reactive.enabled=true' for who like reactive mode, the flag' value is false if missing. It's just a suggestion, thanks a lot

TYsewyn commented 5 years ago

Hi @HaojunRen, thank you for this issue! You are correct, this was an oversight from my part. Spring Cloud Hoxton should have been non-reactive by default, but unfortunately we made a mistake in one of the conditionals in spring-cloud-commons.

The reason why we deprecated the constructor of DiscoveryClientRouteDefinitionLocator using the blocking client is that it doesn't make sense anymore to use that client in an already reactive context.

EDIT: I just remembered that we discussed internally to support both blocking and reactive discovery clients at the same time. Since SC Gateway is already running in a reactive context it doesn't make sense to still create a DiscoveryClientRouteDefinitionLocator with the blocking client. If you manually disable that feature via spring.cloud.discovery.reactive.enabled=false then it's my opinion that you need to take care of a (custom) DiscoveryClientRouteDefinitionLocator. If you don't change anything it should just work out of the box. However we identified an issue in spring-cloud-commons which might explain/resolve your problem. Have you taken a look at https://github.com/spring-cloud/spring-cloud-commons/issues/617 before filing this issue?

HaojunRen commented 5 years ago

My opinion, In fact, users might like old discovery client and ribbon than those reactive components, so better to default with non-react context, not only for gateway But also for services. I have a plugin opensource to support SpringCloud gray-release based on ribbon, if default for reactive, customers maybe should change their code or config

TYsewyn commented 5 years ago

I honestly don’t see the problem in our change. Both blocking and reactive discovery clients are available for the user.

Internally we switched between discovery clients but that does not impact the user. Actually, it does have a positive impact: SC Gateway is now fully reactive and the route definition locator is not blocking anymore.

Could you point me to code which would have an issue because we switched our internal usage?

HaojunRen commented 5 years ago

Sure, that is not a problem.

No matter how, i think it's better to disable reactive components (loadbalance and discovery client) for downside, except that users self want to use reactive mode via setting spring.cloud.discovery.reactive.enabled=true or any other flag. No need to intialize reactive context if they dont want to use reactive mode. Please refer to ReactiveCompositeDiscoveryClientAutoConfiguration

@Configuration
@ConditionalOnDiscoveryEnabled
@ConditionalOnReactiveDiscoveryEnabled
public class ReactiveCompositeDiscoveryClientAutoConfiguration {

    @Bean
    @Primary
    public ReactiveCompositeDiscoveryClient reactiveCompositeDiscoveryClient(
            List<ReactiveDiscoveryClient> discoveryClients) {
        return new ReactiveCompositeDiscoveryClient(discoveryClients);
    }

}

The bean of ReactiveCompositeDiscoveryClient will always been initialized by default. By the way if i am wrong, please point me out? Thanks a lot!

ryanjbaxter commented 5 years ago

But the Gateway was already reactive, even before this change, it is based on Project Reactor. If you were not using service discovery or load balancing the Gateway was fully reactive. It was a bug that our load balancer and discovery client were not reactive.

HaojunRen commented 5 years ago

I knew that, my point is only just for parts of service discovery and load balancing, not including invoking with reactive. I like reactor, but my team did a lot of enhancement for old DiscoveryClient and Ribbon in SCG, and we have difficulty to move those enhancement to reactive mode, but we can accept to do some minor changes in Hoxton version, such as add config in application.properties

So the conclusion is that it is better to all support non-reactive and reactive in SCG, users can select one of them at will

spencergibb commented 5 years ago

It was a bug that our load balancer and discovery client were not reactive.

TYsewyn commented 5 years ago

No need to intialize reactive context if they dont want to use reactive mode.

So if they don’t want to use reactive, why would they put spring-webflux on the classpath? That check is part of the @ConditionalOnReactiveDiscoveryEnabled.

my point is only just for parts of service discovery and load balancing

Like @ryanjbaxter already said Spring Cloud Gateway is built on top of reactive. Everything is created in a reactive context, not just one or 2 features. Why would we still use the blocking discovery client? Before Hoxton.M3 that code had a bug which blocks all threads when doing a registry lookup, rendering your gateway useless for some time. FYI Greenwich.SR3 still has this bug.

but my team did a lot of enhancement for old DiscoveryClient and Ribbon in SCG

This is custom code and not used by 1000s of others. Unfortunately, we as a team can’t solve every use case. That’s why we need to be opinionated. This change should be transparent for maybe 90% of the use cases. Again, it is still possible to use your enhancements. So for now, if you manually disable the reactive discovery feature via spring.cloud.discovery.reactive.enabled=false and create a DiscoveryClientRouteDefinitionLocator with your blocking discovery client then it should work just like before.

EDIT: we’ll create that bean automatically for you in a next Hoxton milestone/rc if you set the property to false.

spencergibb commented 5 years ago

Reopening to create the DiscoveryClientRouteDefinitionLocator when reactive discovery is disabled