Add possibility to opt-out from secured connections in ServicePortSecureResolver

[HanMas123]

Is your feature request related to a problem? Please describe. In https://docs.spring.io/spring-cloud-kubernetes/docs/2.1.1/reference/html/#loadbalancer-for-kubernetes it says:

If a service needs to be accessed over HTTPS you need to add a label or annotation to your service definition with the name secured and the value true and the load balancer will then use HTTPS to make requests to the service.

Currently, if no secured: true label/annotation is found, it is using HTTP. For our services we sould like to have it vice versa. You need to explicitly opt-out via secured: false to use HTTP, otherwise if it's absent (or true) it will use HTTPS.

Describe the solution you'd like Enhance the KubernetesDiscoveryProperties and provide a way to define the default value when input.serviceLabels.getOrDefault("secured", "false"); and input.serviceAnnotations.getOrDefault("secured", "false"); is called in ServicePortSecureResolver. The default value could be false to not interfere with any current implementations.

Describe alternatives you've considered I don't see how I could achive it in any other way easily. I don't see how I could customize the ServicePortSecureResolver to add the required functionality. If you have any idea, I am happy to here it.

[ryanjbaxter]

You could provide your own ServicePortSecureResolver implementation and use that in your own KubernetesDiscoveryClient bean I think.

[HanMas123]

Hey @ryanjbaxter thank you for your feedback. I am now providing a custom ReactiveDiscoveryClient. DiscoveryClient and a customized version of ServicePortSecureResolver. That seems to do the trick. I basically copied the KubernetesReactiveDiscoveryClient and adjusted it to use my customized DiscoveryClient. That is just a copy of KubernetesDiscoveryClient and uses the customized ServicePortSecureResolver.

It's working but I am not the biggest fan ob copying and pasting framework code, who knows how the implementations will change in the future, we might miss that. Do you think this feature request makes sense at all or would you rather close it and leave it as it is?

[ryanjbaxter]

I'm confused....I don't think you should have to copy any code.

Couldn't you just do this...

@Bean
		@ConditionalOnMissingBean
		public KubernetesDiscoveryClient kubernetesDiscoveryClient(KubernetesClient client,
				KubernetesDiscoveryProperties properties,
				KubernetesClientServicesFunction kubernetesClientServicesFunction) {
			return new KubernetesDiscoveryClient(client, properties, kubernetesClientServicesFunction,
					new MyServicePortSecureResolver(properties));
		}

Where MyServicePortSecureResolver is your resolver which defaults to true and allows for opt out?

At the end of the day, I am not against having a property that switches the default behavior, would you be interested in contributing a PR?

[HanMas123]

Yeah that would be possible, although the constructor is package private...

We decided to go for the default behaviour (adding secured: true to all services that require https) but since I suggested the feature, I am happy to contribute here.

TBH I was suprised that I had to add any configuration to tell the LB to use https when my port was already named https. The KubernetesDiscoveryClient has the findEndpointPort() method, that tries to find the port by name (primary-port-name config, https, http). The information about the port name is not used afterwards. IMO it would make sense to use this information when determining if the port is secure in ServicePortSecureResolver. It could be like that:

• Check value of secured annotation / label if present
• Check port is part of known ports
• Check port name (https/http)
• Use default configuration from KubernetesDiscoveryProperties (what I initially suggested as feature request)

What do you think about this?

[ryanjbaxter]

Seems to make sense to me....

I would also be fine if we just adjusted the scope of ServicePortSecureResolver so you can provide your own implementation

[wind57]

the path of least resistance here is indeed the part that you mention:

The information about the port name is not used afterwards

We do discover the port number and it's a bit weird from our side not to use the port name - especially since it's right there, we do not need to do anything else to get it.

Seems like one more issue that I could fix, since I have already done some changes in this area and it looks really familiar to me.