Fix: Make appProtocols optional in flyteadmin and flyteconsole services in helm chart

[fg91]

Why are the changes needed?

#5240, among other things, set appProtocol values of TCP for the ports of the flyteadmin and flyteconsole services in the flyte-core helm chart.

This comment by @ddl-ebrown explains why this was necessary in their deployment which uses an ingress in combination with the istio service mesh:

[...] the ingress may be configured in a way that
it sends TLS traffic to internal Flyte services. Istio will use port
names to determine traffic - and may therefore assume the appProtocol
of http, even though traffic from ingress -> flyteadmin is actually
https. This misconfiguration prevents any traffic from flowing
through the ingress to the service.
Flyteadmin http and grcp ports are accessible using http and
grpc values for appProtocol respectively within the cluster (i.e.
from a user workspace), but as soon as traffic travels between the
ingress and the service those settings will not work. The most
"compatible" setting is tcp which works for any network stream.

---

In my deployment (which also uses istio but not with an nginx ingress but instead with a GCE ingress and an istio ingress gateway, see here for deplyoment guide), this particular change is problematic:

# flyteadmin service
    - name: grpc
      port: 81
      protocol: TCP
      # intentionally set to TCP instead of grpc
      appProtocol: TCP
      targetPort: 8089

#5240 intentionally sets the appProtocol for the grpc port to TCP over grpc. In my deployment this prevents traffic flowing from the istio ingress gateway to the flyteadmin pod.

This worked before the appProtocol was set (assumably because the protocol was derived from the name of the port) and it does also work if I change the protocol to grpc.

What changes were proposed in this pull request?

I currently cannot upgrade my deployment without manually patching the service after deployment. Because of this I'd like to make the appProtocol values in the helm chart optional.

Because the app protocols used to not be set since they are not required for the default nginx ingress/no service mesh deployment the helm chart provides, I propose to disable the app protocol fields by default.

This would mean that you, @ddl-ebrown, need to set the flags to true when upgrading to a helm chart version that contains this change.

If somebody has the opinion that the default should be true, I'm absolutely happy to discuss this.

How was this patch tested?

The app protocol values used to not be set and are not needed for the default nginx ingress deployment offered by the helm chart.

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.19%. Comparing base (25c89ee) to head (d59ba85).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #5944       +/-   ##
===========================================
+ Coverage   37.04%   49.19%   +12.14%     
===========================================
  Files        1316      835      -481     
  Lines      132262    72057    -60205     
===========================================
- Hits        48998    35445    -13553     
+ Misses      79002    33420    -45582     
+ Partials     4262     3192     -1070     

Flag	Coverage Δ
unittests-datacatalog	?
unittests-flyteadmin	54.07% <ø> (-0.03%)	⬇️
unittests-flytecopilot	?
unittests-flytectl	?
unittests-flyteidl	?
unittests-flyteplugins	53.73% <ø> (ø)
unittests-flytepropeller	42.63% <ø> (ø)
unittests-flytestdlib	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fg91]

@ddl-ebrown could you please take a look at whether these changes work for you? Thank you!

[davidmirror-ops]

@fg91 Agree with making it configurable and disabled by default as long as the idea is to maintain a lean but customizable base chart.
We'd need to communicate this as a breaking change for Istio users who rely on it.
Please look at the conflicts (maybe you'll have to rebase and make helm again) and I'll approve. Thank you