Codecov Report

Merging #1789 into master will decrease coverage by 0.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #1789      +/-   ##
==========================================
- Coverage   75.38%   75.37%   -0.02%     
==========================================
  Files         203      203              
  Lines        6240     6241       +1     
  Branches      781      782       +1     
==========================================
  Hits         4704     4704              
  Misses       1196     1196              
- Partials      340      341       +1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8b170f...139bc94. Read the comment docs.

I don't think this is a good idea. It's just a default and can be changed. Users have changed it and this would break them.

@spencergibb wouldnt that be OK in Dalston though?

I don't think so

even the handling of application name versus vip address within EurekaDiscoverClient is inconsistent. see for instance getServices():

	@Override
	public List<String> getServices() {
		Applications applications = this.eurekaClient.getApplications();
		if (applications == null) {
			return Collections.emptyList();
		}
		List<Application> registered = applications.getRegisteredApplications();
		List<String> names = new ArrayList<>();
		for (Application app : registered) {
			if (app.getInstances().isEmpty()) {
				continue;
			}
			names.add(app.getName().toLowerCase());

		}
		return names;
	}

Clearly uses application name and not vip address. So basically this code wouldn't give the expected results:

List<String> services = eurekaDiscoverClient.getServices();
for (String service : services) {
  List<ServiceInstance> instances = eurekaDiscoverClient.getInstances(service);
  // do something
}

As mentioned, getServices() returns a list of application names while getInstances(..) makes a lookup based on vip address. If you look at the javadoc of DiscoveryClient it's clear that this isn't what is expected:

	/**
	 * Get all ServiceInstances associated with a particular serviceId
	 * @param serviceId the serviceId to query
	 * @return a List of ServiceInstance
	 */
	public List<ServiceInstance> getInstances(String serviceId);

	/**
	 * @return all known service ids
	 */
	public List<String> getServices();

By default, spring-cloud sets both to ${spring.application.name} so it won't really be a problem.

As soon as you force the change you LOSE the ability to customize the vip. There is something lost in translation between serviceId -> application name/vip, I agree, but I'm against making this breaking change.

@spencergibb I don't think you do, it's just that serviceId is used differently in a eureka context (application name) and ribbon context (vip address). Zuul uses eureka application names as id and vip addresses as serviceId. As both are equal by default it's inviting to be treated as if they were equivalent. But especially if you customize the vip address, they aren't. The implementation of EurekaDiscoveryClient treats them inconsistently:

// eurekaDiscoveryClient.getInstances(serviceId) returns one instance with different application name and vip address
assertEquals(serviceId, eurekaDiscoveryClient.getInstances(serviceId).get(0).getServiceId()); // fails
assertNotNull(eurekaDiscoveryClient.getInstances(eurekaDiscoveryClient.getInstances(serviceId).get(0).getServiceId())); // fails

Of course I understand your concerns regarding changing existing behavior but in this case it's pretty clear that the existing behavior isn't the expected behavior and that people relying on the current behavior should rather use EurekaClient directly. But maybe changing the method name would be an option? So to intentionally break code that relies on the current behavior?

To get some more insight, I've just spent some time stepping through the code and it got increasingly clear that they are in fact treated as being equivalent. It might of course be that I'm missing something significant here. If so please let me know if and where I get something wrong.

We're using zuul with the standard DiscoveryClientRouteLocator which does the following to add dynamic routes (comments and simplifications by me):

// list of eureka application names
List<String> services = this.discovery.getServices();
for (String serviceId : services) {

	String key = "/" + mapRouteToService(serviceId) + "/**"; // by default "/${servieId}/**"
	if (!PatternMatchUtils.simpleMatch(ignored, serviceId) && !routesMap.containsKey(key)) {
		// new ZuulRoute(..) extracts `id` from `key` and sets `location` to `serviceId`
		routesMap.put(key, new ZuulRoute(key, serviceId));
	}
}

So by default ZuulRoute.id and ZuulRoute.location are set to serviceId which is the eureka application name. when used with zuul, the PreDecorationFilter sets the following in the context:

• proxy = route.getId()
• serviceId = route.getLocation()

RibbonRoutingFilter then does the following:

protected RibbonCommandContext buildCommandContext(RequestContext context) {
	// SNIP
	String serviceId = (String) context.get("serviceId");
	// SNIP
	return new RibbonCommandContext(serviceId, verb, uri, retryable, headers, params,
			requestEntity, this.requestCustomizers, contentLength);
}

protected ClientHttpResponse forward(RibbonCommandContext context) throws Exception {
	// removed exception handling and logging
	return this.ribbonCommandFactory.create(context).execute();
}

// HttpClientRibbonCommandFactory
public HttpClientRibbonCommand create(final RibbonCommandContext context) {
	ZuulFallbackProvider zuulFallbackProvider = getFallbackProvider(context.getServiceId());
	final String serviceId = context.getServiceId();
	// get RibbonLoadBalancingHttpClient by serviceId, i.e. eureka application name
	final RibbonLoadBalancingHttpClient client = this.clientFactory.getClient(
			serviceId, RibbonLoadBalancingHttpClient.class);
	client.setLoadBalancer(this.clientFactory.getLoadBalancer(serviceId));

	return new HttpClientRibbonCommand(serviceId, client, context, zuulProperties, zuulFallbackProvider);
}

SpringClientFactory creates a new context for serviceId in which the serviceId is set as ribbon.client.name (in NamedContextFactory.createContext(String))

The client name is later set as DeploymentContextBasedVipAddresses in EurekaRibbonClientConfiguration.preprocess():

setRibbonProperty(this.serviceId, DeploymentContextBasedVipAddresses.key(), this.serviceId);

At this point, the eureka application name is passed to ribbon as vip address. It's then used in DiscoveryEnabledNIWSServerList to fetch instances from eureka:

if (vipAddresses!=null){
    for (String vipAddress : vipAddresses.split(",")) {
         List<InstanceInfo> listOfInstanceInfo = eurekaClient.getInstancesByVipAddress(vipAddress, isSecure, targetRegion);
        // SNIP
    }
}

The translation from application name to vip address should likely have happened in DiscoveryClientRouteLocator where Route.location should not simply be serviceId - which is the application name at this point - but select a vip address (as there could be different addresses to choose from). As a consequence there would be a ribbon context per vip address, not per application. As to the right approach to choosing the vip address from discovered instances? I don't know. Currently it has to be done manually (zuul.routes.${applicationName}.serviceId=${vipAddress}).

@sfussenegger I have marked this "for discussion". I agree it's inconsistent, but almost exclusively in zuul's DiscoveryClientRouteLocator (and many disable the automatic zuul/discovery client routes, which should be opt-in, not opt-out). In more than two years, as far as I can remember, no one has complained.

@spencergibb we've been using this stuff for over a year now too and never noticed anything. But I guess that's to be expected as long as you run exclusively spring-cloud services. The only usage of DiscoveryClient.getInstances(serviceId) is in DynamicServiceInstanceProvider which seems not to be documented anywhere but in this commit. So it's probably save to assume that this method really isn't used a whole lot.

A good starting point for this discussion is probably the documentation of theses things and their usage:

• application name
• virtual host name
• host name
• instance id
• service id

@sfussenegger I agree that the best thing to do is document the list you stated above. Care to open a new issue? I think then, this PR can be closed as well.

@sfussenegger I've been gathering information not documented in the Spring Cloud official manual, and a description of the various identifiers would be very useful. Would you mind taking a crack at that? Based on the feedback I got, my Eureka blog is now one of the most popular sources of Eureka, and I like to believe is helping people.

@asarkar we finally ended up dropping eureka altogether with #1788 being somewhat the final straw (we now use AWS internal load-balancers in combination with zuul and ribbon instead). so unfortunately it would be quite an effort to get back to a working eureka environment to further explore this. the comments on this PR should be a good starting point though.

Closing see #1788 (comment)