Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SimpleServcieInstance should override getScheme() #823

Open
dagerber opened this issue Sep 14, 2020 · 7 comments · Fixed by #1168 · May be fixed by #926
Open

SimpleServcieInstance should override getScheme() #823

dagerber opened this issue Sep 14, 2020 · 7 comments · Fixed by #1168 · May be fixed by #926

Comments

@dagerber
Copy link
Contributor

I'm submitting a bugreport for

  • spring-cloud-commons
  • class org.springframework.cloud.client.discovery.simple.SimpleDiscoveryProperties

Versions used:

  • spring-boot: 2.3.3
  • spring-cloud: Hoxton.SR7
  • spring-cloud-commons: 2.2.4.RELEASE

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 spencergibb self-assigned this Sep 14, 2020
@spencergibb spencergibb changed the title SimpleServiceInstance should override getScheme() DefaultServiceInstance should override getScheme() Sep 14, 2020
@spencergibb spencergibb changed the title DefaultServiceInstance should override getScheme() SimpleServcieInstance should override getScheme() Sep 14, 2020
@spencergibb
Copy link
Member

This should be done in DefaultServiceInstance as well.

@dagerber
Copy link
Contributor Author

dagerber commented Dec 2, 2020

Has been fixed with 72762d7

@dagerber dagerber closed this as completed Dec 2, 2020
@dagerber
Copy link
Contributor Author

dagerber commented Dec 2, 2020

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

@dagerber dagerber reopened this Dec 2, 2020
@spencergibb
Copy link
Member

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment