fix(*) external service mixing tls endpoints #1080
Conversation
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…ices_tls_fix # Conflicts: # pkg/xds/envoy/types.go # pkg/xds/generator/outbound_proxy_generator.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
pkg/xds/topology/outbound.go
Outdated
| @@ -54,10 +54,15 @@ func BuildEndpointMap( | |||
| tlsEnabled = externalService.Spec.Networking.Tls.Enabled | |||
| } | |||
|
|
|||
| tags := externalService.Spec.GetTags() | |||
| if tlsEnabled { | |||
| tags[mesh_proto.ServiceTag+"_tls"] = "enabled" | |||
There was a problem hiding this comment.
I'd advise this:
tags[`kuma.io/external-service-name`] = externalService.GetName()
in a second you will introduce more TLS settings so simple enabled/disabled is not enough
There was a problem hiding this comment.
Well, should the tag denote that there is TLS involved? This sounds like if tlsEnabled is true, we just set the tag with the external service name.
There was a problem hiding this comment.
It's not even visible in the UI or anywhere.
The whole idea is that if you have
type: ExternalService
mesh: default
name: httpbin-1
tags:
kuma.io/service: httpbin
version: "1"
networking:
address: httpbin.org:80
tls:
enabled: true
caCert:
secret: sec-1
and
type: ExternalService
mesh: default
name: httpbin-2
tags:
kuma.io/service: httpbin
version: "2"
networking:
address: httpbin.org:80
tls:
enabled: true
caCert:
secret: sec-2
you've got different TLS settings for different ExternalService. Simple _tls is not enough to differentiate both of them.
You can set the
tags[`kuma.io/external-service-name`] = externalService.GetName()
regardless if TLS is on or off.
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
pkg/xds/topology/outbound.go
Outdated
| @@ -54,10 +54,15 @@ func BuildEndpointMap( | |||
| tlsEnabled = externalService.Spec.Networking.Tls.Enabled | |||
| } | |||
|
|
|||
| tags := externalService.Spec.GetTags() | |||
| if tlsEnabled { | |||
| tags[mesh_proto.ServiceTag+"_tls"] = "enabled" | |||
There was a problem hiding this comment.
It's not even visible in the UI or anywhere.
The whole idea is that if you have
type: ExternalService
mesh: default
name: httpbin-1
tags:
kuma.io/service: httpbin
version: "1"
networking:
address: httpbin.org:80
tls:
enabled: true
caCert:
secret: sec-1
and
type: ExternalService
mesh: default
name: httpbin-2
tags:
kuma.io/service: httpbin
version: "2"
networking:
address: httpbin.org:80
tls:
enabled: true
caCert:
secret: sec-2
you've got different TLS settings for different ExternalService. Simple _tls is not enough to differentiate both of them.
You can set the
tags[`kuma.io/external-service-name`] = externalService.GetName()
regardless if TLS is on or off.
| @@ -60,29 +60,18 @@ func (k *k8SDeployment) Deploy(cluster framework.Cluster) error { | |||
| } | |||
|
|
|||
| func (k *k8SDeployment) Delete(cluster framework.Cluster) error { | |||
| err := k8s.KubectlDeleteE(cluster.GetTesting(), | |||
| _ = k8s.KubectlDeleteE(cluster.GetTesting(), | |||
There was a problem hiding this comment.
why are we ignoring errors? Ignoring errors in E2E led to issues in the past (we weren't deleting Kuma in a proper way and we were ignoring it). I would avoid running tests anymore if there is a problem with the cleanup.
There was a problem hiding this comment.
Well, there is a problem ensuring that "all" External service pods are deleted before the next test starts. In the other tests we do create test apps in kuma-test namespace, and then we just delete that namespace making sure it completely disappeared cleaning up all associated resources before we continue. Now here I tried to delete the pods one by one and only deleting the namespace if there are 0 pods. In practice, the pods were staying available just enough so that the namespace for the external services was never deleted, which caused confusion in the follow up tests.
I probably can just remove the deleting of the resource and let the namespace be deleted upon the first call to Delete.
There was a problem hiding this comment.
Ok, I understand. I'm a little bit worried that we cannot get K8S to work for this simple scenario, but I don't want you to spend time on this. We can go back to it when we will actually need the ability to delete one external service deployment and not the other.
| if err != nil { | ||
| return err | ||
| } | ||
| // forcefully delete the namespace, no matter if there are other pods |
There was a problem hiding this comment.
well, technically it matters, a bit. If I have 2 external services deployments and delete one of them, actually two of them will be wiped out.
What was the problem here?
There was a problem hiding this comment.
Indeed it matters, see the upper explanation for details. Essentially we want to ensure that all ExternalServices are not available before we start the next test. Unfortunatelly I did not find a reliable way to ensure the pods are not there anymore, instead, the proposal is to get rid of the namespace.
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
|
|
||
| // forcefully delete the namespace, no matter if there are other pods | ||
| // Forcefully delete the namespace, no matter if there are other pods | ||
| // this is to ensure that all the relevan resources are cleaned up |
There was a problem hiding this comment.
relevan -> relevant
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Summary
Allow for multiple endpoints with mixed TLS requirements for ExternalServices.