From 43adc84dfdfc33f3be0268e09b193afc82f9bf9a Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Mon, 22 Aug 2016 17:16:13 -0600 Subject: [PATCH] Make moving to https urls consistent. This fixes a case where the ribbon property is set `ribbon.IsSecure=false`, but because the eureka secure port was enabled, the uri created by ribbon was prefixed with https when it shouldn't have. All places where isSecure is need have been consolidated into RibbonUtils. fixes gh-1270 --- .../feign/ribbon/FeignLoadBalancer.java | 15 +-- .../ribbon/RibbonClientConfiguration.java | 17 ++- .../ribbon/RibbonLoadBalancerClient.java | 11 +- .../netflix/ribbon/RibbonProperyUtils.java | 33 ------ .../cloud/netflix/ribbon/RibbonUtils.java | 79 +++++++++++++ .../feign/ribbon/FeignLoadBalancerTests.java | 23 ++++ .../netflix/ribbon/RibbonUtilsTests.java | 104 ++++++++++++++++++ .../EurekaRibbonClientConfiguration.java | 2 +- .../EurekaRibbonClientConfigurationTests.java | 8 +- 9 files changed, 226 insertions(+), 66 deletions(-) delete mode 100644 spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonProperyUtils.java create mode 100644 spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancer.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancer.java index 94a2141b..1c3ed45a 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancer.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancer.java @@ -23,7 +23,6 @@ import java.util.LinkedHashMap; import java.util.Map; import org.springframework.cloud.netflix.ribbon.ServerIntrospector; -import org.springframework.web.util.UriComponentsBuilder; import com.netflix.client.AbstractLoadBalancerAwareClient; import com.netflix.client.ClientException; @@ -38,10 +37,11 @@ import com.netflix.loadbalancer.Server; import feign.Client; import feign.Request; -import feign.RequestTemplate; import feign.Response; import feign.Util; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.updateToHttpsIfNeeded; + public class FeignLoadBalancer extends AbstractLoadBalancerAwareClient { @@ -98,13 +98,8 @@ public class FeignLoadBalancer extends @Override public URI reconstructURIWithServer(Server server, URI original) { - String scheme = original.getScheme(); - if (!"https".equals(scheme) && (this.serverIntrospector.isSecure(server) - || this.clientConfig.get(CommonClientConfigKey.IsSecure, false))) { - original = UriComponentsBuilder.fromUri(original).scheme("https").build() - .toUri(); - } - return super.reconstructURIWithServer(server, original); + URI uri = updateToHttpsIfNeeded(original, this.clientConfig, this.serverIntrospector, server); + return super.reconstructURIWithServer(server, uri); } static class RibbonRequest extends ClientRequest implements Cloneable { @@ -188,4 +183,4 @@ public class FeignLoadBalancer extends } -} \ No newline at end of file +} diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java index 3420d4f9..3292d6a4 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java @@ -52,7 +52,8 @@ import com.sun.jersey.api.client.Client; import com.sun.jersey.client.apache4.ApacheHttpClient4; import static com.netflix.client.config.CommonClientConfigKey.DeploymentContextBasedVipAddresses; -import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.setRibbonProperty; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.setRibbonProperty; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.updateToHttpsIfNeeded; /** * @author Dave Syer @@ -187,23 +188,21 @@ public class RibbonClientConfiguration { static class OverrideRestClient extends RestClient { + private IClientConfig config; private ServerIntrospector serverIntrospector; - protected OverrideRestClient(IClientConfig ncc, + protected OverrideRestClient(IClientConfig config, ServerIntrospector serverIntrospector) { super(); + this.config = config; this.serverIntrospector = serverIntrospector; - initWithNiwsConfig(ncc); + initWithNiwsConfig(this.config); } @Override public URI reconstructURIWithServer(Server server, URI original) { - String scheme = original.getScheme(); - if (!"https".equals(scheme) && this.serverIntrospector.isSecure(server)) { - original = UriComponentsBuilder.fromUri(original).scheme("https").build(true) - .toUri(); - } - return super.reconstructURIWithServer(server, original); + URI uri = updateToHttpsIfNeeded(original, this.config, this.serverIntrospector, server); + return super.reconstructURIWithServer(server, uri); } @Override diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonLoadBalancerClient.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonLoadBalancerClient.java index 28ca7b7d..4d00a9fb 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonLoadBalancerClient.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonLoadBalancerClient.java @@ -29,7 +29,6 @@ import org.springframework.util.Assert; import org.springframework.util.ReflectionUtils; import org.springframework.web.util.UriComponentsBuilder; -import com.netflix.client.config.CommonClientConfigKey; import com.netflix.client.config.IClientConfig; import com.netflix.loadbalancer.ILoadBalancer; import com.netflix.loadbalancer.Server; @@ -113,14 +112,8 @@ public class RibbonLoadBalancerClient implements LoadBalancerClient { private boolean isSecure(Server server, String serviceId) { IClientConfig config = this.clientFactory.getClientConfig(serviceId); - if (config != null) { - Boolean isSecure = config.get(CommonClientConfigKey.IsSecure); - if (isSecure != null) { - return isSecure; - } - } - - return serverIntrospector(serviceId).isSecure(server); + ServerIntrospector serverIntrospector = serverIntrospector(serviceId); + return RibbonUtils.isSecure(config, serverIntrospector, server); } protected Server getServer(String serviceId) { diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonProperyUtils.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonProperyUtils.java deleted file mode 100644 index b4881ede..00000000 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonProperyUtils.java +++ /dev/null @@ -1,33 +0,0 @@ -package org.springframework.cloud.netflix.ribbon; - -import com.netflix.config.ConfigurationManager; -import com.netflix.config.DynamicPropertyFactory; -import com.netflix.config.DynamicStringProperty; - -/** - * @author Spencer Gibb - */ -public class RibbonProperyUtils { - - public static final String VALUE_NOT_SET = "__not__set__"; - - public static final String DEFAULT_NAMESPACE = "ribbon"; - - public static void setRibbonProperty(String serviceId, String suffix, String value) { - // how to set the namespace properly? - String key = getRibbonKey(serviceId, suffix); - DynamicStringProperty property = getProperty(key); - if (property.get().equals(VALUE_NOT_SET)) { - ConfigurationManager.getConfigInstance().setProperty(key, value); - } - } - - public static String getRibbonKey(String serviceId, String suffix) { - return serviceId + "." + DEFAULT_NAMESPACE + "." + suffix; - } - - public static DynamicStringProperty getProperty(String key) { - return DynamicPropertyFactory.getInstance().getStringProperty(key, VALUE_NOT_SET); - } - -} diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java new file mode 100644 index 00000000..711b665b --- /dev/null +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java @@ -0,0 +1,79 @@ +package org.springframework.cloud.netflix.ribbon; + +import com.netflix.client.config.CommonClientConfigKey; +import com.netflix.client.config.IClientConfig; +import com.netflix.client.config.IClientConfigKey; +import com.netflix.config.ConfigurationManager; +import com.netflix.config.DynamicPropertyFactory; +import com.netflix.config.DynamicStringProperty; +import com.netflix.loadbalancer.Server; +import org.springframework.web.util.UriComponentsBuilder; + +import java.net.URI; + +/** + * @author Spencer Gibb + */ +public class RibbonUtils { + + public static final String VALUE_NOT_SET = "__not__set__"; + + public static final String DEFAULT_NAMESPACE = "ribbon"; + + public static void setRibbonProperty(String serviceId, String suffix, String value) { + // how to set the namespace properly? + String key = getRibbonKey(serviceId, suffix); + DynamicStringProperty property = getProperty(key); + if (property.get().equals(VALUE_NOT_SET)) { + ConfigurationManager.getConfigInstance().setProperty(key, value); + } + } + + public static String getRibbonKey(String serviceId, String suffix) { + return serviceId + "." + DEFAULT_NAMESPACE + "." + suffix; + } + + public static DynamicStringProperty getProperty(String key) { + return DynamicPropertyFactory.getInstance().getStringProperty(key, VALUE_NOT_SET); + } + + /** + * Determine if client is secure. If the supplied {@link IClientConfig} has the {@link CommonClientConfigKey#IsSecure} + * set, return that value. Otherwise, query the supplied {@link ServerIntrospector}. + * @param config the supplied client configuration. + * @param serverIntrospector + * @param server + * @return true if the client is secure + */ + public static boolean isSecure(IClientConfig config, ServerIntrospector serverIntrospector, Server server) { + if (config != null) { + Boolean isSecure = config.get(CommonClientConfigKey.IsSecure); + if (isSecure != null) { + return isSecure; + } + } + + return serverIntrospector.isSecure(server); + } + + /** + * Replace the scheme to https if needed. If the uri doesn't start with https and + * {@link #isSecure(IClientConfig, ServerIntrospector, Server)} is true, update the scheme. + * This assumes the uri is already encoded to avoid double encoding. + * + * @param uri + * @param config + * @param serverIntrospector + * @param server + * @return + */ + public static URI updateToHttpsIfNeeded(URI uri, IClientConfig config, ServerIntrospector serverIntrospector, Server server) { + String scheme = uri.getScheme(); + if (!"https".equals(scheme) && isSecure(config, serverIntrospector, server)) { + return UriComponentsBuilder.fromUri(uri).scheme("https").build(true) + .toUri(); + } + return uri; + } + +} diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancerTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancerTests.java index c5a60993..e8d52c12 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancerTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/feign/ribbon/FeignLoadBalancerTests.java @@ -19,6 +19,7 @@ import java.net.URI; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.Map; import org.junit.Before; import org.junit.Test; @@ -102,6 +103,28 @@ public class FeignLoadBalancerTests { assertThat(uri, is(new URI("https://foo:7777/"))); } + @Test + @SneakyThrows + public void testInsecureUriFromInsecureClientConfigToSecureServerIntrospector() { + when(this.config.get(IsSecure)).thenReturn(false); + this.feignLoadBalancer = new FeignLoadBalancer(this.lb, this.config, + new ServerIntrospector() { + @Override + public boolean isSecure(Server server) { + return true; + } + + @Override + public Map getMetadata(Server server) { + return null; + } + }); + Server server = new Server("foo", 7777); + URI uri = this.feignLoadBalancer.reconstructURIWithServer(server, + new URI("http://foo/")); + assertThat(uri, is(new URI("http://foo:7777/"))); + } + @Test @SneakyThrows public void testSecureUriFromClientConfigOverride() { diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java new file mode 100644 index 00000000..f8e8204f --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java @@ -0,0 +1,104 @@ +/* + * Copyright 2013-2016 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.springframework.cloud.netflix.ribbon; + +import com.netflix.client.config.CommonClientConfigKey; +import org.junit.Assert; +import org.junit.Test; + +import com.netflix.client.config.DefaultClientConfigImpl; +import com.netflix.loadbalancer.Server; + +import java.util.Map; + +import static org.hamcrest.Matchers.is; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.isSecure; + +/** + * @author Spencer Gibb + */ +public class RibbonUtilsTests { + + private static final ServerIntrospector NON_SECURE_INTROSPECTOR = new StaticServerIntrospector(false); + private static final ServerIntrospector SECURE_INTROSPECTOR = new StaticServerIntrospector(true); + private static final Server SERVER = new Server("localhost", 8080); + private static final DefaultClientConfigImpl SECURE_CONFIG = getConfig(true); + private static final DefaultClientConfigImpl NON_SECURE_CONFIG = getConfig(false); + private static final DefaultClientConfigImpl NO_IS_SECURE_CONFIG = new DefaultClientConfigImpl(); + + @Test + public void noRibbonPropSecureIntrospector() { + boolean secure = isSecure(NO_IS_SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("isSecure was wrong", secure, is(true)); + } + + @Test + public void noRibbonPropNonSecureIntrospector() { + boolean secure = isSecure(NO_IS_SECURE_CONFIG, NON_SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("isSecure was wrong", secure, is(false)); + } + + @Test + public void isSecureRibbonPropSecureIntrospector() { + boolean secure = isSecure(SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("isSecure was wrong", secure, is(true)); + } + + @Test + public void nonSecureRibbonPropNonSecureIntrospector() { + boolean secure = isSecure(NON_SECURE_CONFIG, NON_SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("isSecure was wrong", secure, is(false)); + } + + @Test + public void isSecureRibbonPropNonSecureIntrospector() { + boolean secure = isSecure(SECURE_CONFIG, NON_SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("isSecure was wrong", secure, is(true)); + } + + @Test + public void nonSecureRibbonPropSecureIntrospector() { + boolean secure = isSecure(NON_SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("isSecure was wrong", secure, is(false)); + } + + static DefaultClientConfigImpl getConfig(boolean value) { + DefaultClientConfigImpl config = new DefaultClientConfigImpl(); + config.setProperty(CommonClientConfigKey.IsSecure, value); + return config; + } + + static class StaticServerIntrospector implements ServerIntrospector { + + final boolean secure; + + public StaticServerIntrospector(boolean secure) { + this.secure = secure; + } + + @Override + public boolean isSecure(Server server) { + return this.secure; + } + + @Override + public Map getMetadata(Server server) { + return null; + } + } +} diff --git a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfiguration.java b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfiguration.java index 25fb5d25..06a1cf81 100644 --- a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfiguration.java +++ b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfiguration.java @@ -41,7 +41,7 @@ import com.netflix.niws.loadbalancer.NIWSDiscoveryPing; import static com.netflix.client.config.CommonClientConfigKey.DeploymentContextBasedVipAddresses; import static com.netflix.client.config.CommonClientConfigKey.EnableZoneAffinity; -import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.setRibbonProperty; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.setRibbonProperty; import lombok.extern.apachecommons.CommonsLog; diff --git a/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfigurationTests.java b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfigurationTests.java index 3e6866a5..0aee7709 100644 --- a/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfigurationTests.java +++ b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfigurationTests.java @@ -36,10 +36,10 @@ import com.netflix.niws.loadbalancer.DiscoveryEnabledServer; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; -import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.VALUE_NOT_SET; -import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.getProperty; -import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.getRibbonKey; -import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.setRibbonProperty; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.VALUE_NOT_SET; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.getProperty; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.getRibbonKey; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.setRibbonProperty; /** * @author Dave Syer