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 36.84%. Comparing base (13b3d82) to head (7e18c5b).
Report is 5 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5944   +/-   ##
=======================================
  Coverage   36.84%   36.84%           
=======================================
  Files        1309     1309           
  Lines      130967   130994   +27     
=======================================
+ Hits        48252    48268   +16     
- Misses      78531    78536    +5     
- Partials     4184     4190    +6     

Flag	Coverage Δ
unittests-datacatalog	51.58% <ø> (ø)
unittests-flyteadmin	54.10% <ø> (-0.02%)	⬇️
unittests-flytecopilot	11.73% <ø> (ø)
unittests-flytectl	62.40% <ø> (ø)
unittests-flyteidl	6.92% <ø> (ø)
unittests-flyteplugins	53.64% <ø> (ø)
unittests-flytepropeller	43.02% <ø> (+0.01%)	⬆️
unittests-flytestdlib	55.41% <ø> (+0.01%)	⬆️

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!