Fix: Ribbon Implementation Does not Support "https" when using Eureka / Service Discovery

[gmcouto]

This PR is meant to make url expander implementation to consider eureka data and change schema to https when necessary

Fixes #230

[tdanylchuk]

please revert whitespaces changes

[kptfh]

can we simply use here server.getScheme() instead all this logic?

[gmcouto]

The library Ribbon-Eureka is also not populating that field... and would require a Pull Request to their project for this to work. :(
Reference:
https://github.com/Netflix/ribbon/blob/0b2253fa2e0ff1d1ad65e1f99dfaa2e5e37d560b/ribbon-eureka/src/main/java/com/netflix/niws/loadbalancer/DiscoveryEnabledNIWSServerList.java#L196

And:
https://github.com/Netflix/ribbon/blob/e327877cb47a56b68c705a68698ab67ded481aef/ribbon-eureka/src/main/java/com/netflix/niws/loadbalancer/DiscoveryEnabledServer.java#L41

[kptfh]

Can you find out how open feign had fixed this issue?

[gmcouto]

Sure, will look into it and reply here.

[gmcouto]

@kptfh It is actually a very similar solution... see in DefaultServerIntrospector:

public boolean isSecure(Server server) {
    return this.serverIntrospectorProperties.getSecurePorts().contains(server.getPort());
  }

called from EuerekaServerIntrospector:

public boolean isSecure(Server server) {
    if (server instanceof DiscoveryEnabledServer) {
      DiscoveryEnabledServer discoveryServer = (DiscoveryEnabledServer)server;
      return discoveryServer.getInstanceInfo().isPortEnabled(PortType.SECURE);
    } else {
      return super.isSecure(server);
    }
  }

called from RibbonUtils:

public static boolean isSecure(IClientConfig config, ServerIntrospector serverIntrospector, Server server) {
    if (config != null) {
      Boolean isSecure = (Boolean)config.get(CommonClientConfigKey.IsSecure);
      if (isSecure != null) {
        return isSecure;
      }
    }

    return serverIntrospector.isSecure(server);
  }

called from RibbonUtils:

  public static URI updateToSecureConnectionIfNeeded(URI uri, IClientConfig config, ServerIntrospector serverIntrospector, Server server) {
    String scheme = uri.getScheme();
    if (StringUtils.isEmpty(scheme)) {
      scheme = "http";
    }

    return !StringUtils.isEmpty(uri.toString()) && unsecureSchemeMapping.containsKey(scheme) && isSecure(config, serverIntrospector, server) ? upgradeConnection(uri, (String)unsecureSchemeMapping.get(scheme)) : uri;
  }

called from FeignLoadBalancer:

public URI reconstructURIWithServer(Server server, URI original) {
    URI uri = RibbonUtils.updateToSecureConnectionIfNeeded(original, this.clientConfig, this.serverIntrospector, server);
    return super.reconstructURIWithServer(server, uri);
  }

But these are all workarounds in the spring-cloud-netflix-ribbon library... I guess that openfeign does not have any implementation related to/for eureka service discovery, or even any implementation for upgrading schemes.
So, this spring cloud library is responsible for integrating them properly.

[kptfh]

Don't like to add 2 additional dependencies. I think it should be fixed on Spring side. It should return Server instance with correct schema.
Can you use "https" in URL as workaround?

[gmcouto]

Well, the fix is on the reactive feign spring integration module(cloud module), just like spring-cloud-netflix-ribbon library is an integration library... so, to me, it is fitting. I didn't want to add additional dependencies neither, but the "cloud1" is specifically to be used with ribbon and, optionally, eureka.
I guess if we want to use spring fix for this(which is already in place), then we would need to have LoadBalancerCommand to accept a AbstractLoadBalancerAwareClient as a way to reach the loadbalancer, and use spring implementation FeignLoadBalancer wrapper to the ILoadBalancer instead... as it is able to automatically upgrade to https when necessary.
However, I think this fix will impact reactive-feign way more than what I proposed.

In another note, if I add "https" to the url it won't use the loadbalancer...
Because of
https://github.com/Playtika/feign-reactive/blob/bafb08f33142e494829b638d3bdfe3f30419ba29/feign-reactor-spring-configuration/src/main/java/reactivefeign/spring/config/ReactiveFeignClientFactoryBean.java#L95
And
https://github.com/Playtika/feign-reactive/blob/bafb08f33142e494829b638d3bdfe3f30419ba29/feign-reactor-cloud/src/main/java/reactivefeign/cloud/CloudReactiveFeign.java#L195
Maybe changing this can be more appealing. What do you think?

[caseyjmorton]

Are there any updates on this? I am in dire need of this capability.

[p4pramod]

Me too..

[gmcouto]

I suggest you do what I did at the time. Compile and package your own version with the fix on this PR.

Unfortunately, I'm not working on this anymore

[kptfh]

@gmcouto can you check if it works with Spring Cloud 3.0?
You can take the latest version of reactive feign client