spring-cloud / spring-cloud-consul

Spring Cloud Consul
http://cloud.spring.io/spring-cloud-consul/
Apache License 2.0
813 stars 541 forks source link

Not able to override ConsulDiscoveryClient #316

Closed JagmohanSharma closed 5 years ago

JagmohanSharma commented 7 years ago

@spencergibb I need to override ConsulDiscoveryClient, so for that I am making configuration by defining this bean,

@Autowired
    private ConsulClient consulClient;

    @Autowired(required = false)
    private ServerProperties serverProperties;

 @Bean
    public ConsulDiscoveryClient consulDiscoveryClient(ConsulLifecycle lifecycle,
                                                       ConsulDiscoveryProperties discoveryProperties) {
        ConsulDiscoveryClientImpl discoveryClient = new ConsulDiscoveryClientImpl(consulClient,
                lifecycle, discoveryProperties);
        discoveryClient.setServerProperties(serverProperties); //null ok
        return discoveryClient;
    }

But this is not able to find ConsulLifecycle bean when I upgraded to 1.2.0.RELEASE version as it is giving error with

NoSuchBeanDefinitionException: No qualifying bean of type 'org.springframework.cloud.consul.discovery.ConsulLifecycle' available

Even if I tried to get ConsulLifecycle bean from applicationContext , still it comes as null only.

@Bean
    public ConsulDiscoveryClient consulDiscoveryClient(ApplicationContext context, ConsulDiscoveryProperties discoveryProperties) {
       ConsulLifecycle lifecycle = getBean(ConsulLifecycle.class, context);
        ConsulDiscoveryClientImpl discoveryClient = new ConsulDiscoveryClientImpl(consulClient,
                lifecycle, discoveryProperties);
        discoveryClient.setServerProperties(serverProperties); //null ok
        return discoveryClient;
    }

Now if I want to provide parameter of type LocalResolver as being done in ConsulDiscoveryClientConfiguration

@Bean
    @ConditionalOnMissingBean
    public ConsulDiscoveryClient consulDiscoveryClient(ConsulDiscoveryProperties discoveryProperties, final ApplicationContext context) {
        ConsulDiscoveryClient discoveryClient = new ConsulDiscoveryClient(consulClient,
                discoveryProperties, new LifecycleRegistrationResolver(context));
        discoveryClient.setServerProperties(serverProperties); //null ok
        return discoveryClient;
    }

    class LifecycleRegistrationResolver implements ConsulDiscoveryClient.LocalResolver {
        private ApplicationContext context;

        public LifecycleRegistrationResolver(ApplicationContext context) {
            this.context = context;
        }

        ...some code ahead
        }
    }

But this can not be done in our configuration class as you have set default scope for LocalResolver in ConsulDiscoveryClient

public class ConsulDiscoveryClient implements DiscoveryClient {
  interface LocalResolver {
        String getInstanceId();
        Integer getPort();
    }
...
}

Can you guide us to resolve this issue as why we are not getting ConsulLifecycle bean in application context and also would it be required to changes the scope of LocalResolver to public so that this can be used while overriding ConsulDiscoveryClient.

Any help would be appreciated.

spencergibb commented 7 years ago

Can I ask why you want a custom ConsulDiscoveryClient?

JagmohanSharma commented 7 years ago

@spencergibb Actually as we are using CommonsInstanceDiscovery in our turbine application which gets instances data from consulDiscoveryClient. we have configured services at consul in a manner that we require certain formatting of tags before we can use them. And also we are making use of KvStore to keep a property called "hystrix_enabled" which is used to fetch only hystrix enabled services from consul. As each service is storing this property in its kv store(kvPrefix).

Now as InstanceObservable keep fetching instances from consulDiscoveryClient so we have to override consulDiscoveryClient before it actually start each instance monitor by making call to ClusterMonitorInstanceManager.

Also we got certain cases where we need to add additional meta data for containing hystrix stream url for any particular service instances which is later used in overridden AggregateClusterMonitor's ClusterConfigBasedUrlClosure directly. So we had to use custom consulDiscoveryClient to accommodate these requirements.

JagmohanSharma commented 7 years ago

any feedback or help on this?

csalembier commented 7 years ago

The issue seems to be in the ConditionalOnClass in ConditionalOnConsulEnabled `public @interface ConditionalOnConsulEnabled {

class OnConsulEnabledCondition extends AllNestedConditions {
    ...
    @ConditionalOnClass(ConsulClient.class)
     static class FoundClass {}
}

}`

It specifically requires, com.ecwid.consul.v1.ConsulClient

I also found this trying to replace the client. Our use case is security related: we are using Consul ACLs with everything blacklisted, so no anonymous requests are permitted; however the ConsulClient does not send the token for a variety of request (our issue was with deregister).

spencergibb commented 7 years ago

spring-cloud-consul has hard dependencies throughout on ConsulClient. I'm not sure it's worth much without it.

JagmohanSharma commented 7 years ago

But can we think about default scope for LocalResolver to change as it is currently not possible to provide parameter of type LocalResolver as being done in ConsulDiscoveryClientConfiguration?

sirlatrom commented 7 years ago

I've got a more Consul-related use case here: Process the list of services in org.springframework.cloud.consul.discovery.ConsulDiscoveryClient#getServices() to perform advanced filtering/transformation of services before handing them off to org.springframework.cloud.netflix.zuul.filters.discovery.DiscoveryClientRouteLocator#locateRoutes().

pengisgood commented 7 years ago

Previously, in Camden.SR6, I used this way to customize the instance id:

public class DiscoveryConfiguration {

    @Autowired
    private ConsulDiscoveryClientConfiguration configuration;

    @Autowired
    private ApplicationContext context;

    @Bean
    public ConsulLifecycle consulLifecycle(ConsulDiscoveryProperties discoveryProperties,
                                           HeartbeatProperties heartbeatProperties) {

        String suffix;
        try {
            suffix = InetAddress.getLocalHost().getHostName();
        } catch (UnknownHostException e) {
            suffix = UUID.randomUUID().toString();
            log.error("get hostname error, {}", e.getMessage());
        }
        String instanceId = context.getId() + ConsulLifecycle.SEPARATOR + suffix;
        log.info("register service id: {}", instanceId);
        discoveryProperties.setInstanceId(instanceId);
        return configuration.consulLifecycle(discoveryProperties, heartbeatProperties);
    }
}

But after I upgrade to Dalston.SR3, ConsulLifecycle has been deprecated and consulLifecycle method has been removed. :(

How can I customize the instance id now? I know we can give a pattern in the bootstrap.yml, but I lost the chance to get hostname.

zhuSilence commented 6 years ago

spring-consul how to Subscribe to healthy services??

spencergibb commented 6 years ago

@zhuSilence please ask unrelated questions on stack overflow or gitter

johnsabath commented 5 years ago

444 is another relevant use case (multi-datacenter failover using a Consul prepared query)

spencergibb commented 5 years ago

I don't think this is an issue anymore, can anyone confirm?

pengisgood commented 5 years ago

Not a problem with me now. Go ahead!