spring-cloud / spring-cloud-commons

Common classes used in different Spring Cloud implementations
Apache License 2.0
709 stars 705 forks source link

SimpleServcieInstance should override getScheme() #823

Open dagerber opened 4 years ago

dagerber commented 4 years ago

I'm submitting a bugreport for

Versions used:

When using SimpleDiscoveryClient, non-HTTPS Urls do not work, they are always resolved as https:// URLs. We are using https in production, but http in Tests to avoid the need to setup certificates for localhost. There are workarounds, but I think this is a bug that should be fixed in spring-cloud-commons

e.g.

"spring.cloud.discovery.client.simple.instances.foo-service[0].uri= http://localhost:8889", "spring.cloud.discovery.client.simple.instances.foo-service[0].secure = false",

foo-service is always resolved as https://localhost:8889, because SimpleServcieInstance (inner class of SimpleServiceProperties) does not override getScheme().

LoadBalancerUriTools calls getScheme() and in case of SimpleServiceInstance always receives null and thus always sets Scheme to https.

Proposed fix for org.springframework.cloud.client.discovery.simple.SimpleDiscoveryProperties#SimpleServiceInstance

public String getScheme() { return this.isSecure() ? "https" : "http"; }

spencergibb commented 4 years ago

This should be done in DefaultServiceInstance as well.

dagerber commented 4 years ago

Has been fixed with https://github.com/spring-cloud/spring-cloud-commons/commit/72762d76ed9eccc5876e5bf099c2495c4c44ff3e

dagerber commented 4 years ago

Sorry, thought this was solved, but it is not.

dagerber commented 3 years ago

https://github.com/spring-cloud/spring-cloud-commons/pull/926

spencergibb commented 3 years ago

Closed via #926

spencergibb commented 3 years ago

Reverted, at least commons, gateway and contract have behavior that depends on the null implementation that will need to be addressed in the future.

OlgaMaciaszek commented 2 years ago

Requires more discussion on impact in GW. Reverting the change and reopening.