Fix ports for service and enable istio support out-of-the-box #639
Conversation
|
|
|
|
|
Also I signed the CLA, not sure why the old comment is drawn |
| protocol: TCP | ||
| name: grpc-rpc | ||
| - port: {{ $serviceValues.service.membershipPort }} |
There was a problem hiding this comment.
This port should not be wired up to a service, it's only used for direct connections between the pods.
There was a problem hiding this comment.
@robholland this change is for a headless service and not for regular service(that stays in this file upper). So headless service used for pod-to-pod communication https://stackoverflow.com/a/52713482.
Moreover, without this change, Istio not work properly. This is the main intention of this PR is to make Temporal compatible with Istio setup that has strict mTLS enabled.
And for Istio it's required https://istio.io/latest/docs/ops/configuration/traffic-management/traffic-routing/#headless-services
While for setup without Istio - headless service is optional(this is why it's working before).
There was a problem hiding this comment.
Added comments inside a file to reflect why this change is needed. For future maintainability.
There was a problem hiding this comment.
The headless service is not for pod to pod communication, it's only used for prometheus monitoring scraping. Temporal does not make use of any Kubernetes services internally.
There was a problem hiding this comment.
@robholland this change is for compatibility with Istio here is the documentation https://istio.io/latest/docs/ops/configuration/traffic-management/traffic-routing/#headless-services
Without it - Temporal don’t work with Istio setup that has strict mTLS enabled.
The change proposed in PR helped me to make it work.
There was a problem hiding this comment.
Hmm. I don't want to increase confusion about what these services are used for, it already often causes confusion. It's a shame that Istio relies on this indirect mechanism rather than a Deployment annotation or similar. Please add a new value istio: true|false (defaulted false). The monitor port should always be in the headless service, all other ports should only be added if istio is true. It's fine to leave appProtocol on the metrics port unconditionally.
There was a problem hiding this comment.
I think you can remove your comments once the ports are conditional, it should be self documenting enough that people can infer the reasons. Maybe include a one line comment above the istio value line explaining that it enables support for Istio service configuration.
There was a problem hiding this comment.
@robholland I would recommend to not introduce istio flag and make it like proposed in PR. Otherwise for those who use this helm chart - there will be confusion and expectation that there will be also added resources specific to the Istio like kind: VirtualService etc.
What PR's is really doing is making this helm chart friendly to be used in Kubernetes setups that use Istio out-of-the box.
In this PR we don’t introduce direct dependency on Istio.
Also from what I saw for helm charts that we use(we use many) for open-source tools - maintainers do not introduce special variable like istio to enable compatibility with Istio. They adjust helm charts to have this change as they are compatible with setup without Istio e.g. not change default behaviour.
Example: Mimir, Tempo(grafana/tempo#4326). They also have cluster discovery e.g. pod-to-pod.
Please let me know if my thoughts changed your mind and make sense.
And if not - I will introduce what you requested.
There was a problem hiding this comment.
It's not behaviour changes I'm worried about, but more confusion about what our headless services are for. Mimir is not a good comparison because they do use k8s for discovery: https://github.com/grafana/mimir/blob/ce9463ed2d6c864b1bc7f327471f38f746d05fc5/operations/helm/charts/mimir-distributed/values.yaml#L359. We don't do that, but adding the ports here implies that we do.
Let me consult with the team and see what they think.
There was a problem hiding this comment.
Ok, so unfortunately I'm not an expert in Istio, but do you think this on the deployments would work as an alternative:
proxy.istio.io/config: |
proxyMetadata:
ISTIO_META_INTERCEPTION_MODE: REDIRECT
As far as I understand it the addition of the headless service port is just to tell Istio to create a listener for <POD_IP>:<PORT>, I believe the above would have the effect of creating a listener for <POD_IP:*>. As we don't use the k8s for discovery we don't need the DNS features that the headless service port provides.
Doing it this way would avoid creating confusing service ports that aren't needed and also makes it very clear what is Istio specific.
Is that something you could test? Did I misunderstand something about requirements?
Hronom
force-pushed
the
main
branch
3 times, most recently
from
c5e8cef to
0456cf4
Compare
Signed-off-by: Yevhen Tienkaiev <[email protected]>
| protocol: TCP | ||
| name: grpc-rpc | ||
| - port: {{ $serviceValues.service.membershipPort }} |
There was a problem hiding this comment.
Hmm. I don't want to increase confusion about what these services are used for, it already often causes confusion. It's a shame that Istio relies on this indirect mechanism rather than a Deployment annotation or similar. Please add a new value istio: true|false (defaulted false). The monitor port should always be in the headless service, all other ports should only be added if istio is true. It's fine to leave appProtocol on the metrics port unconditionally.
| protocol: TCP | ||
| name: grpc-rpc | ||
| - port: {{ $serviceValues.service.membershipPort }} |
There was a problem hiding this comment.
I think you can remove your comments once the ports are conditional, it should be self documenting enough that people can infer the reasons. Maybe include a one line comment above the istio value line explaining that it enables support for Istio service configuration.
What was changed
Fix ports for service and enable Istio support out-of-the-box.
Why?
Checklist
Closes Temporal services are not starting when deployed in istio enabled namespace #338
How was this tested:
Tested on cluster with Istio enabled and strict mTLS.