[velero] Follow istio naming convention

[sathieu]

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

Special notes for your reviewer:

Checklist

• DCO signed
• Chart Version bumped
• Variables are documented in the values.yaml or README.md
• Title of the PR starts with chart name (e.g. [velero])

[jenting]

If we do not follow the convention, will Istio + Velero not work?

[sathieu]

@jenting From the docs:

Istio can automatically detect HTTP and HTTP/2 traffic. If the protocol cannot automatically be determined, traffic will be treated as plain TCP traffic.

So Istio + Velero will work out of the box. But once I got an issue with http traffic which was fixed by using istio naming convention (see kubitus-project/kubitus-installer#15 and kubitus-project/kubitus-installer!267).

[jenting]

🤔 The current solution is like Velero favors Istio's convention.

What if the other tools favor other conventions and did not work with Istio's convention?

[sathieu]

@jenting, there are no other conventions around. And I don't think any incompatible convention will come (at least, not for http):

• linkerd has protocol detection, but no way to override it. The only available setting is to mark the stream as opaque or skip it (no proxying) with an annotation. See https://linkerd.io/2.11/features/protocol-detection/.
• traefik mesh also uses annotations

Also as mentioned in the istio doc, it's also possible to use the appProtocol field (defined in the Service spec):

In Kubernetes 1.18+, by the appProtocol field: appProtocol: .

ports.appProtocol (string)

The application protocol for this port. This field follows standard Kubernetes label syntax. Un-prefixed names are reserved for IANA standard service names (as per RFC-6335 and http://www.iana.org/assignments/service-names). Non-standard protocols should use prefixed names such as mycompany.com/my-custom-protocol.

But with this PR I went for a solution compatible with pre-1.8 versions too.

What if the other tools favor other conventions and did not work with Istio's convention?

If this is the case, we'll need another value (metrics.service.portName), but I think adding this complexity for an hypothetic usecase is over-engineering.

[jenting]

LGTM, thank you for the detailed explanation.

[dsu-igeek]

Looks like if you use the regular installer (velero install) this port is named "metrics" - I'd like to have everything be the same and I think we should discuss a little about whether this will break anything else (since Helm and the CLI installer are doing things differently, hopefully no one is dependent on the port name). Any thoughts on if linkerd will work with this as well?

[sathieu]

@dsu-igeek

Any thoughts on if linkerd will work with this as well?

As said above, linkerd has protocol detection, but no way to override it. The only available setting is to mark the stream as opaque or skip it (no proxying) with an annotation. I think that istio is the only service mesh affected by port names.

[reasonerjt]

Please rebase to resolve the conflict.

[sathieu]

@reasonerjt Rebased, and resolved the merge conflict.

@dsu-igeek Is the source of the installer available. I can make a PR there.

[sathieu]

Thanks @jenting & @reasonerjt for approving and merging 🙏!