centrifugal / helm-charts

Official Centrifugo Helm chart for Kubernetes
MIT License
33 stars 29 forks source link

Investigate deployment with Istio #23

Open FZambia opened 2 years ago

FZambia commented 2 years ago

Looks like we have a problem with port names which prevent Istio to properly pass WS traffic:

https://istio.io/latest/docs/ops/configuration/traffic-management/protocol-selection/

We can:

Needs investigation to confirm the issue – since Istio should select protocol automatically https://istio.io/latest/docs/ops/configuration/traffic-management/protocol-selection/#automatic-protocol-selection ?

matrix-root commented 2 years ago

I've tried another setup and looks like it works with current chart. Maybe I'm was wrong with something...

It seems that the first time istio was not able to set up the protocol on its own, no ideas why.

matrix-root commented 2 years ago

Hm, actually not at all. Setup on production from scratch caught same problem. Looks like really will be good to hardcode protocols for Istio

matrix-root commented 2 years ago

Finally it connects, but after 10 seconds. And before connection it doing some strange things... Istio logs on second screenshot

Снимок экрана 2021-11-28 в 7 54 47 PM Снимок экрана 2021-11-28 в 7 53 02 PM

matrix-root commented 2 years ago

Yes! Strange, but... I've setup appProtocol in my custom service - and it connects without any issues.

So, maybe we can just add appProtocol/setup name fields - so, Istio will strictly use correct protocol. Latest Istio 1.12

FZambia commented 2 years ago

@LvLs9 I found several issues in Istio which look very similar to this. For example this one - https://github.com/istio/istio/issues/32127. An this - https://github.com/istio/istio/issues/29427.

Unfortunately these issues are closed without any resolution from Istio team. So I think adding a possibility to define appProtocol for each port as first step here makes sense. I think should be empty by default and set in spec only if value provided. Thus we won't use it in older k8s. What do you think?

matrix-root commented 2 years ago

@FZambia Yes, agreed 💪

I can try build MR with fix

matrix-root commented 2 years ago

@FZambia Before merging PR...

I've tried to delete istiod pod (which will reconfigure network) - and it's worked correctly. But not sure it's really good solution.

FZambia commented 2 years ago

So we now have a possibility to define appProtocol for each service port (added in https://github.com/centrifugal/helm-charts/pull/24) - thus we have an explicit way to tell Istio protocols to use.

For the future I think would be awesome to investigate whether we should change current port names in some way to inherit automatic Istio deployment without customizing options.

matrix-root commented 2 years ago

Should we close that?

FZambia commented 2 years ago

@LvLs9 Let's leave this open for now – hope in the future we will figure out the remaining parts. I'd like Centrifugo work with Istio out-of-the-box without explicit appProtocol configuration – but requires some research and local experiments – I am not fully understand what Istio actually does.