Add support loadBalancerKey in RibbonRoutingFilter of Zuul for Canary test #2439

yongsungyoon · 2017-11-13T14:01:11Z

Background

To support canary testing in Zuul, we need to pass the information called loadBalancerKey to Ribbon that could be used in IRule to choose a server for special treatment. But it was not easy until Ribbon 2.2.2 because it doesn't pass loadBalancerKey in ClientRequest to serverLocator(loadBalancerKey) in LoadBalancerCommand.

From Ribbon 2.2.3 (currently 2.2.4 is the latest), it is possible by Ribbon's PR now.
This PR is about supporting to pass loadBalancerKey from Zuul to Ribbon.

Description

This PR doesn't change any behavior of Zuul unless there is loadBalancerKey in RequestContext of Zuul.
If RibbonRoutingFilter finds loadBalancerKey from RequestContext, it will pass it into RibbonCommandContext. And ContextAwareRequest will include this value from RibbonCommandContext and it will be passed to LoadBalancerCommand finally.

How to Use

User can add loadBalancerKey into RequestContrext in his own pre-filter like below.

RequestContext.getCurrentContext().set(LOAD_BALANCER_KEY, "any value you want");

Because user can access much information of HTTP request like headers and query params in pre-filter, user can use any information of HTTP request to determine loadBalancerKey.

The value for loadBalancerKey could be any Object and it will be passed when IRule is called. User can use this value to determine the address for special routing.

Note

There is another subclass of AbstractLoadBalancerAwareClient - FeignLoadBalancer. I think that we can support the same feature for Feign with the similar way. But there is no easy way for user to add his own logic for adding loadBalancerKey. So we need to consider more about how to support it in Feign.

codecov-io · 2017-11-13T14:29:02Z

Codecov Report

Merging #2439 into master will increase coverage by 0.42%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #2439      +/-   ##
==========================================
+ Coverage   68.27%   68.69%   +0.42%     
==========================================
  Files         236      234       -2     
  Lines        8308     8232      -76     
  Branches     1031     1021      -10     
==========================================
- Hits         5672     5655      -17     
+ Misses       2248     2193      -55     
+ Partials      388      384       -4

Impacted Files	Coverage Δ
.../netflix/zuul/filters/support/FilterConstants.java	`0% <ø> (ø)`	⬆️
...ud/netflix/ribbon/support/ContextAwareRequest.java	`100% <100%> (ø)`	⬆️
...etflix/zuul/filters/route/RibbonRoutingFilter.java	`75% <100%> (+0.27%)`	⬆️
...ix/ribbon/support/AbstractLoadBalancingClient.java	`68.62% <100%> (+1.96%)`	⬆️
...tflix/zuul/filters/route/RibbonCommandContext.java	`74.02% <60%> (+17.5%)`	⬆️
.../netflix/eureka/EurekaClientAutoConfiguration.java	`90% <0%> (-1.31%)`	⬇️
.../cloud/netflix/ribbon/RibbonAutoConfiguration.java	`90.9% <0%> (-0.4%)`	⬇️
...ign/ribbon/FeignRibbonClientAutoConfiguration.java	`100% <0%> (ø)`	⬆️
...x/ribbon/apache/HttpClientRibbonConfiguration.java	`92% <0%> (ø)`	⬆️
...tflix/ribbon/okhttp/OkHttpRibbonConfiguration.java	`77.5% <0%> (ø)`	⬆️
... and 9 more

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 6e3b4fd...25781a8. Read the comment docs.

ryanjbaxter

Could you also provide some documentation on how one would use this?

ryanjbaxter · 2017-11-13T16:40:25Z

...src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContext.java

 	}

 	public RibbonCommandContext(String serviceId, String method, String uri,
 								Boolean retryable, MultiValueMap<String, String> headers,
 								MultiValueMap<String, String> params, InputStream requestEntity,
 								List<RibbonRequestCustomizer> requestCustomizers) {
 		this(serviceId, method, uri, retryable, headers, params, requestEntity,
-			requestCustomizers, null);
+			requestCustomizers, null, null);
 	}

 	public RibbonCommandContext(String serviceId, String method, String uri,


To avoid breaking anyone I think we should create a new constructor with the new parameter. You could also mark the old constructor deprecated if it is not necessary anymore.

Okay. I will add a new constructor and some documentation soon.

lowzj · 2017-11-14T01:40:18Z

I have a idea about how to add loadBalancerKey in Feign a year ago, see #1272 (comment) .
And I suggest that sc-netflix should have another new class to inherit AbstractLoadBalancerAwareClient. It will be convenient for other modules to support loadBalancerKey.
More detailed see #1272 (comment) , and the implementation: #1556 .

yongsungyoon · 2017-11-14T03:36:33Z

@ryanjbaxter I've updated code and added some documentation

yongsungyoon · 2017-11-14T03:53:33Z

@lowzj Hi. Thanks for your comment.
I've seen your suggest and here is what I think.
Only change what we need in those classes is passing ClientRequest's loadBalancerKey to LoadBalancerCommand's serverLocator(internally the property name of serverLocator is also loadBalancerKey). And currently only 2 direct subclasses of AbstractLoadBalancerAwareClient exist at this moment - AbstractLoadBalancingClient and FeignLoadBalancer. The other one RestClient is already deprecated. So, I think that just adding one override method to existing classes is better than adding new sub classes.

Personally I think that those changes (passing loadBalancerKey from ClientRequest to LoadBalancerCommand should be done in AbstractLoadBAlancerAwareClient in Ribbon as default someday. Because two fields - loadBalancerKey in ClientRequest and serverLocator in LoadBalancerCommand seems to be defined for the same purpose. If it was passed in AbstractLoadBAlancerAwareClient class, we even don't need to override one method. But at this moment (with ribbon 2.2.4), overriding one method is the safest way to support this feature as I think.

ryanjbaxter

Would it be possible to add a test that shows how this would be used with Zuul? For example adding the IRule and then making sure the request is routed correctly?

ryanjbaxter · 2017-11-14T17:51:12Z

...src/main/java/org/springframework/cloud/netflix/zuul/filters/route/RibbonCommandContext.java

 	}

+	@Deprecated


Can I change my mind on this? Sorry, maybe we should keep this constructor around since the command key is optional based on the use case. Sorry again.

yongsungyoon · 2017-11-16T13:27:47Z

@ryanjbaxter I've added the integration test for loadBalancerKey support. Also deleted @deprecated annotation that you requested to remove.

ryanjbaxter · 2017-11-18T12:54:55Z

...src/test/java/org/springframework/cloud/netflix/test/ApacheHttpClientConfigurationTests.java

@@ -124,7 +124,7 @@ public void testHttpClientWithFeign() {
 	public void testRibbonLoadBalancingHttpClient() {
 		RibbonCommandContext context = new RibbonCommandContext("foo"," GET", "http://localhost",
 				false, new LinkedMultiValueMap<String, String>(), new LinkedMultiValueMap<String, String>(),
-				null, new ArrayList<RibbonRequestCustomizer>(), 0l);
+				null, new ArrayList<RibbonRequestCustomizer>(), 0l, null);


you can change these tests back to using the old constructor now

yongsungyoon · 2017-11-19T11:33:38Z

@ryanjbaxter I've fixed.

xiaominger · 2019-05-11T03:01:22Z

Background

To support canary testing in Zuul, we need to pass the information called loadBalancerKey to Ribbon that could be used in IRule to choose a server for special treatment. But it was not easy until Ribbon 2.2.2 because it doesn't pass loadBalancerKey in ClientRequest to serverLocator(loadBalancerKey) in LoadBalancerCommand.

From Ribbon 2.2.3 (currently 2.2.4 is the latest), it is possible by Ribbon's PR now.
This PR is about supporting to pass loadBalancerKey from Zuul to Ribbon.

Description

This PR doesn't change any behavior of Zuul unless there is loadBalancerKey in RequestContext of Zuul.
If RibbonRoutingFilter finds loadBalancerKey from RequestContext, it will pass it into RibbonCommandContext. And ContextAwareRequest will include this value from RibbonCommandContext and it will be passed to LoadBalancerCommand finally.

How to Use

User can add loadBalancerKey into RequestContrext in his own pre-filter like below.
RequestContext.getCurrentContext().set(LOAD_BALANCER_KEY, "any value you want");
Because user can access much information of HTTP request like headers and query params in pre-filter, user can use any information of HTTP request to determine loadBalancerKey.

The value for loadBalancerKey could be any Object and it will be passed when IRule is called. User can use this value to determine the address for special routing.

Note

There is another subclass of AbstractLoadBalancerAwareClient - FeignLoadBalancer. I think that we can support the same feature for Feign with the similar way. But there is no easy way for user to add his own logic for adding loadBalancerKey. So we need to consider more about how to support it in Feign.

Thanks,but it doesn't work when I use RetryableRibbonLoadBalancingHttpClient, which pass serviceId instead of loadBalancerKey

ryanjbaxter requested changes Nov 13, 2017

View reviewed changes

Add support loadBalancerKey in RibbonRoutingFilter

f428b89

yongsungyoon force-pushed the zuul-loadbalancerkey-support branch from defcc62 to 6447c3d Compare November 14, 2017 03:33

Add documentation about how to provide a loadBalancerKey to Ribbon

838f9b7

yongsungyoon force-pushed the zuul-loadbalancerkey-support branch from 6447c3d to 838f9b7 Compare November 14, 2017 08:51

ryanjbaxter reviewed Nov 14, 2017

View reviewed changes

lowzj mentioned this pull request Nov 16, 2017

Canary Support in Zuul #2337

Closed

ryanjbaxter approved these changes Nov 18, 2017

View reviewed changes

Add integrationtest for RibbonRoutingFilter's loadBalancerKey support

25781a8

yongsungyoon force-pushed the zuul-loadbalancerkey-support branch from acd4a03 to 25781a8 Compare November 19, 2017 11:01

ryanjbaxter merged commit 60c4e1b into spring-cloud:master Nov 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support loadBalancerKey in RibbonRoutingFilter of Zuul for Canary test #2439

Add support loadBalancerKey in RibbonRoutingFilter of Zuul for Canary test #2439

yongsungyoon commented Nov 13, 2017

codecov-io commented Nov 13, 2017 •

edited

ryanjbaxter left a comment

ryanjbaxter Nov 13, 2017

yongsungyoon Nov 14, 2017

lowzj commented Nov 14, 2017

yongsungyoon commented Nov 14, 2017

yongsungyoon commented Nov 14, 2017 •

edited

ryanjbaxter left a comment

ryanjbaxter Nov 14, 2017

yongsungyoon Nov 15, 2017

yongsungyoon commented Nov 16, 2017 •

edited

ryanjbaxter Nov 18, 2017

yongsungyoon commented Nov 19, 2017

xiaominger commented May 11, 2019 •

edited

Add support loadBalancerKey in RibbonRoutingFilter of Zuul for Canary test #2439

Add support loadBalancerKey in RibbonRoutingFilter of Zuul for Canary test #2439

Conversation

yongsungyoon commented Nov 13, 2017

codecov-io commented Nov 13, 2017 • edited

Codecov Report

ryanjbaxter left a comment

Choose a reason for hiding this comment

ryanjbaxter Nov 13, 2017

Choose a reason for hiding this comment

yongsungyoon Nov 14, 2017

Choose a reason for hiding this comment

lowzj commented Nov 14, 2017

yongsungyoon commented Nov 14, 2017

yongsungyoon commented Nov 14, 2017 • edited

ryanjbaxter left a comment

Choose a reason for hiding this comment

ryanjbaxter Nov 14, 2017

Choose a reason for hiding this comment

yongsungyoon Nov 15, 2017

Choose a reason for hiding this comment

yongsungyoon commented Nov 16, 2017 • edited

ryanjbaxter Nov 18, 2017

Choose a reason for hiding this comment

yongsungyoon commented Nov 19, 2017

xiaominger commented May 11, 2019 • edited

codecov-io commented Nov 13, 2017 •

edited

yongsungyoon commented Nov 14, 2017 •

edited

yongsungyoon commented Nov 16, 2017 •

edited

xiaominger commented May 11, 2019 •

edited