Repository: openservicemesh/osm (go)

Issue:
Make app protocol detection of service port less error prone
**Please describe the Improvement and/or Feature Request**
Currently, OSM implicitly uses http as the app protocol for a k8s service port if it can't derive the port's protocol using the `appProtocol` field or from the port's name. 
https://github.com/openservicemesh/osm/blob/ee7e846a2ff0005f962e8bd867944c3f2a46b6f7/pkg/compute/kube/client.go#L511-L525

It's possible for a k8s service to use the same port for multiple protocols, e.g. port 53 for DNS can be TCP or UDP. When a port's protocol is unset based on the [app protocol selection behavior](https://release-v1-2.docs.openservicemesh.io/docs/guides/app_onboarding/app_protocol_selection/), the configuration can result in duplicate Envoy filter_chain_match configuration. This happens because OSM implicitly sets the appProtocol to `http` for unknown port protocols. So a configuration such as the below can result in duplicate filter_match_configuration:
```
ports:
  - name: myPort
    port: 8080
    protocol: udp
  - name: myPort2
    port: 8080
    protocol: tcp
    appProtocol: http
```

> Note: the protocol is implicitly set to http when it's unspecified for backward compatibility reasons. We do not want to break existing users relying on this behavior, as http apps are more common. In the future, we can consider making the protocol specification explicit at all times.

This issue proposes to do the following:
1. When `port.protocol` is UDP, to not set MeshService.Protocol to `http`, but to leave it as is and let it result in an error log when the code that builds the config sees an unsupported protocol
2. Add a warn log when MeshService.Protocol is implicitly set to `http`. This log will be useful to identify scenarios where there is a protocol related error with traffic.


Pull request description:
k8s: update app protocol detection for k8s service port
<!--

Please describe the motivation for this PR and provide enough
information so that others can review it.

-->
**Description**:
This change updates the app protocol detection for a service port to eliminate a misleading error scenario that results from using the same port for multiple protocols.

It does the following:

1. Only uses http implicitly for TCP based ports that do not have appProtocol or a valid protocol prefix in the port name.

2. Adds a warn log when implicitly using http as the app protocol for TCP based ports.

The result is that duplicate filter chain matches
are eliminated with the same port is used for different protocols, eg. port 8080 being used for TCP and UDP.

Resolves #5223

<!--

Please describe how this change was tested. You could include supporting information
such as logs, snippets, and screenshots.

-->
**Testing done**:
Unit tests

<!--

Please mark with X for applicable areas.

-->
**Affected area**:
| Functional Area            |     |
| -------------------------- | --- |
| Control Plane              | [X] |

Please answer the following questions with yes/no.

1. Does this change contain code from or inspired by another project? `no`
    -   Did you notify the maintainers and provide attribution?

2. Is this a breaking change? `no`

3. Has documentation corresponding to this change been updated in the [osm-docs](https://github.com/openservicemesh/osm-docs) repo (if applicable)? `TBD`

Patch:
diff --git a/docs/release_guide.md b/docs/release_guide.md
index 35381bc2..03767be1 100644
--- a/docs/release_guide.md
+++ b/docs/release_guide.md
@@ -13,6 +13,7 @@ This guide describes the process to create a GitHub Release for this project.
 Once an RC has been found to be stable, cut a release tagged `vX.Y.Z` using the following steps.
 
 - [Release Guide](#release-guide)
+  - [Release candidates](#release-candidates)
   - [Create a release branch](#create-a-release-branch)
   - [Add changes to be backported](#add-changes-to-be-backported)
   - [Create and push the pre-release Git tag](#create-and-push-the-pre-release-git-tag)
@@ -29,6 +30,9 @@ Once an RC has been found to be stable, cut a release tagged `vX.Y.Z` using the
   - [Make version changes on main branch](#make-version-changes-on-main-branch)
   - [Workflow Diagram](#workflow-diagram)
 
+## Release candidates
+Release candidates (RCs) should be created before each significant release so final testing can be performed. RCs are tagged as `vX.Y.Z-rc.W`. See [here](release_process.md#release-candidates) for additional information.
+
 ## Create a release branch
 
 Look for a branch on the upstream repo named `release-vX.Y`, where `X` and `Y` correspond to the major and minor version of the semver tag to be used for the new release. If the branch already exists, skip to the next step.
@@ -47,8 +51,23 @@ $ git push upstream release-<version> # ex: git push upstream release-v0.4
 ## Add changes to be backported
 
 Create a new branch off of the release branch to maintain updates specific to the new version. Let's call it the patch branch. The patch branch should not be created in the upstream repo.
-
+```console
+$ git checkout upstream/release-<version> # ex: git checkout upstream/release-v1.2
+$ git switch -c <patch-branch-name> # Create the patch branch in your repo 
+```
 If there are other commits on the `main` branch to be included in the release (such as for successive release candidates or patch releases), cherry-pick those onto the patch branch.
+```console
+$ git cherry-pick <commit-id> # ex: git cherry-pick 1bdc75a
+```
+Fix any merge conflicts and change the commit message to begin with [backport]:
+```bash
+[backport] allow exporting access log to OTEL collector # previously - telemetry: allow exporting access log to OTEL collector
+```
+Push your changes to your branch.
+```console
+$ git push <local-repo> <patch-branch-name>
+```
+ Create a pull request to backport the changes to the release branch, see [example PR](https://github.com/openservicemesh/osm/pull/5210). Use a merge commit instead of a squash commit when merging to preserve the original commits.
 
 ## Create and push the pre-release Git tag
 
@@ -90,7 +109,7 @@ Create a new commit on the patch branch to update the hardcoded version informat
 
 Once patches and version information have been updated on the patch branch off of the release branch, create a pull request from the patch branch to the release branch. When creating your pull request, generate the release checklist for the description by adding the following to the PR URL: `?expand=1&template=release_pull_request_template.md`. Alternatively, copy the raw template from [release_pull_request_template.md](/.github/PULL_REQUEST_TEMPLATE/release_pull_request_template.md).
 
-Proceed to the next step once the pull request is approved and merged.
+Proceed to the next step once the pull request is approved and merged, see [example PR](https://github.com/openservicemesh/osm/pull/5215).
 
 ## Create and push the release Git tag
 
diff --git a/pkg/compute/kube/client.go b/pkg/compute/kube/client.go
index c8acda1c..135fa3c4 100644
--- a/pkg/compute/kube/client.go
+++ b/pkg/compute/kube/client.go
@@ -661,8 +661,8 @@ func (c *client) serviceToMeshServices(svc corev1.Service) []service.MeshService
 		// Order of Preference is:
 		// 1. port.appProtocol field
 		// 2. protocol prefixed to port name (e.g. tcp-my-port)
-		// 3. default to http
-		protocol := constants.ProtocolHTTP
+		// 3. default to http for TCP ports
+		var protocol string
 		for _, p := range constants.SupportedProtocolsInMesh {
 			if strings.HasPrefix(portSpec.Name, p+"-") {
 				protocol = p
@@ -673,6 +673,22 @@ func (c *client) serviceToMeshServices(svc corev1.Service) []service.MeshService
 		// use port.appProtocol if specified, else use port protocol
 		meshSvc.Protocol = pointer.StringDeref(portSpec.AppProtocol, protocol)
 
+		// If app protocol was not detected and the protocol is TCP, use HTTP
+		// as the default app protocol.
+		// This is required for backward compatibility where users may be relying
+		// on implicit HTTP protocol selection.
+		//
+		// Note: it's possible for MeshService.Protocol to be unset for
+		// non TCP ports (eg. UDP) if we could not infer the corresponding
+		// app protocol via the appProtocol field or the port name.
+		// This will result in an error log for the specific port when the
+		// Envoy config is built for that port.
+		if meshSvc.Protocol == "" && portSpec.Protocol == corev1.ProtocolTCP {
+			log.Warn().Msgf("Did not detect app protocol for TCP port name=%s number=%d for service %s/%s, defaulting to http",
+				portSpec.Name, portSpec.Port, svc.Namespace, svc.Name)
+			meshSvc.Protocol = constants.ProtocolHTTP
+		}
+
 		// The endpoints for the kubernetes service carry information that allows
 		// us to retrieve the TargetPort for the MeshService.
 		endpoints, _ := c.kubeController.GetEndpoints(svc.Name, svc.Namespace)


Test patch:
diff --git a/pkg/compute/kube/client_test.go b/pkg/compute/kube/client_test.go
index f6674ed5..d16ce162 100644
--- a/pkg/compute/kube/client_test.go
+++ b/pkg/compute/kube/client_test.go
@@ -438,7 +438,11 @@ func TestGetServicesForServiceIdentity(t *testing.T) {
 						Selector: map[string]string{
 							"k1": "v1", // matches labels on pod ns1/p1
 						},
-						Ports: []corev1.ServicePort{{}},
+						Ports: []corev1.ServicePort{
+							{
+								Protocol: corev1.ProtocolTCP,
+							},
+						},
 					},
 				},
 				{
@@ -960,7 +964,11 @@ func TestListServicesForProxy(t *testing.T) {
 						Selector: map[string]string{
 							"k1": "v1",
 						},
-						Ports: []corev1.ServicePort{{}},
+						Ports: []corev1.ServicePort{
+							{
+								Protocol: corev1.ProtocolTCP,
+							},
+						},
 					},
 				},
 				{
@@ -972,7 +980,11 @@ func TestListServicesForProxy(t *testing.T) {
 						Selector: map[string]string{
 							"k2": "v2", // does not match labels on pod ns1/p1
 						},
-						Ports: []corev1.ServicePort{{}},
+						Ports: []corev1.ServicePort{
+							{
+								Protocol: corev1.ProtocolTCP,
+							},
+						},
 					},
 				},
 			},
@@ -2197,7 +2209,7 @@ func TestListServiceIdentitiesForService(t *testing.T) {
 	}
 }
 
-func TestK8sServicesToMeshServices(t *testing.T) {
+func TestServiceToMeshServices(t *testing.T) {
 	testCases := []struct {
 		name         string
 		svc          corev1.Service
@@ -2208,7 +2220,7 @@ func TestK8sServicesToMeshServices(t *testing.T) {
 			name: "k8s service with single port and endpoint, no appProtocol set",
 			// Single port on the service maps to a single MeshService.
 			// Since no appProtocol is specified, MeshService.Protocol should default
-			// to http.
+			// to http since Port.Protocol=TCP.
 			svc: corev1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "ns1",
@@ -2217,8 +2229,9 @@ func TestK8sServicesToMeshServices(t *testing.T) {
 				Spec: corev1.ServiceSpec{
 					Ports: []corev1.ServicePort{
 						{
-							Name: "p1",
-							Port: 80,
+							Name:     "p1",
+							Port:     80,
+							Protocol: corev1.ProtocolTCP,
 						},
 					},
 					ClusterIP: "10.0.0.1",
@@ -2306,7 +2319,7 @@ func TestK8sServicesToMeshServices(t *testing.T) {
 			name: "k8s headless service with single port and endpoint, no appProtocol set",
 			// Single port on the service maps to a single MeshService.
 			// Since no appProtocol is specified, MeshService.Protocol should default
-			// to http.
+			// to http because Port.Protocol=TCP
 			svc: corev1.Service{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "ns1",
@@ -2315,8 +2328,9 @@ func TestK8sServicesToMeshServices(t *testing.T) {
 				Spec: corev1.ServiceSpec{
 					Ports: []corev1.ServicePort{
 						{
-							Name: "p1",
-							Port: 80,
+							Name:     "p1",
+							Port:     80,
+							Protocol: corev1.ProtocolTCP,
 						},
 					},
 					ClusterIP: corev1.ClusterIPNone,
@@ -2500,6 +2514,71 @@ func TestK8sServicesToMeshServices(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "duplicate ports on k8s service with different protocols",
+			svc: corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "ns1",
+					Name:      "s1",
+				},
+				Spec: corev1.ServiceSpec{
+					ClusterIP: "10.0.0.1",
+					Ports: []corev1.ServicePort{
+						{
+							Name:     "p1",
+							Port:     80,
+							Protocol: corev1.ProtocolTCP,
+						},
+						{
+							Name:     "p2",
+							Port:     80,
+							Protocol: corev1.ProtocolUDP,
+						},
+					},
+				},
+			},
+			svcEndpoints: []runtime.Object{
+				&corev1.Endpoints{
+					ObjectMeta: metav1.ObjectMeta{
+						// Should match svc.Name and svc.Namespace
+						Namespace: "ns1",
+						Name:      "s1",
+					},
+					Subsets: []corev1.EndpointSubset{
+						{
+							Ports: []corev1.EndpointPort{
+								{
+									// Must match the port of 'svc.Spec.Ports[0]'
+									Name: "p1",
+									Port: 8080, // TargetPort
+								},
+								{
+									// Must match the port of 'svc.Spec.Ports[1]'
+									Name: "p2",
+									Port: 8080, // TargetPort
+								},
+							},
+						},
+					},
+				},
+			},
+			expected: []service.MeshService{
+				{
+					Namespace:  "ns1",
+					Name:       "s1",
+					Port:       80,
+					TargetPort: 8080,
+					Protocol:   "http",
+				},
+				{
+					Namespace:  "ns1",
+					Name:       "s1",
+					Port:       80,
+					TargetPort: 8080,
+					Protocol:   "",
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {