[NET-7534] v2: Make port names in consul-k8s compatible with NET-5586 (…

…#3528)

- [This change in consul](hashicorp/consul#20371) involves now interpreting whether xRoute/FailoverPolicy/DestinationPolicy resource service references use either the service port (virtualPort in consul) or service target port (targetPort in consul). To make this decision unambiguously:
> This change updates our interpretation of these reference fields/keys (parent, backend, destination), s.t.:
>
> * A numeric value will be exclusively interpreted to indicate a ServicePort.virtual_port
> * A non-numeric value will be exclusively interpreted to indicate a ServicePort.target_port (this supports VMs/Nomad and other cases where network virtual ports are not used, and port names are expected to be in reference to workload ports, not service ports)

- If a K8s service targetport is allowed to be the stringified version of a number, it will be ambiguous in consul what to interpret the string "portID" as. 

- This change makes it such that the string port can never be a number, and will always also have alpha characters by prefixing "cslport-" to the workload port if the workload port name is unspecified.

control-plane/connect-inject/common/common.go

@@ -73,6 +73,20 @@ func PortValue(pod corev1.Pod, value string) (int32, error) {
 	return int32(raw), err
 }
 
+// WorkloadPortName returns the container port's name if it has one, and if not, constructs a name from the port number
+// and adds a constant prefix. The port name must be 1-15 characters and must have at least 1 alpha character.
+func WorkloadPortName(port *corev1.ContainerPort) string {
+	name := port.Name
+	var isNum bool
+	if _, err := strconv.Atoi(name); err == nil {
+		isNum = true
+	}
+	if name == "" || isNum {
+		name = constants.UnnamedWorkloadPortNamePrefix + strconv.Itoa(int(port.ContainerPort))
+	}
+	return name
+}
+
 // TransparentProxyEnabled returns true if transparent proxy should be enabled for this pod.
 // It returns an error when the annotation value cannot be parsed by strconv.ParseBool or if we are unable
 // to read the pod's namespace label when it exists.

control-plane/connect-inject/common/common_test.go

@@ -167,6 +167,46 @@ func TestCommonDetermineAndValidatePort(t *testing.T) {
 	}
 }
 
+func TestWorkloadPortName(t *testing.T) {
+	cases := []struct {
+		Name     string
+		Port     *corev1.ContainerPort
+		Expected string
+	}{
+		{
+			Name: "named port",
+			Port: &corev1.ContainerPort{
+				Name:          "http",
+				ContainerPort: 8080,
+			},
+			Expected: "http",
+		},
+		{
+			Name: "unnamed port",
+			Port: &corev1.ContainerPort{
+				Name:          "",
+				ContainerPort: 8080,
+			},
+			Expected: "cslport-8080",
+		},
+		{
+			Name: "number port name",
+			Port: &corev1.ContainerPort{
+				Name:          "8080",
+				ContainerPort: 8080,
+			},
+			Expected: "cslport-8080",
+		},
+	}
+
+	for _, tt := range cases {
+		t.Run(tt.Name, func(t *testing.T) {
+			name := WorkloadPortName(tt.Port)
+			require.Equal(t, tt.Expected, name)
+		})
+	}
+}
+
 func TestPortValue(t *testing.T) {
 	cases := []struct {
 		Name     string

control-plane/connect-inject/constants/constants.go

@@ -80,6 +80,8 @@ const (
 	CACertFileEnvVar    = "CONSUL_CACERT_FILE"
 	CACertPEMEnvVar     = "CONSUL_CACERT_PEM"
 	TLSServerNameEnvVar = "CONSUL_TLS_SERVER_NAME"
+
+	UnnamedWorkloadPortNamePrefix = "cslport-"
 )
 
 // GetNormalizedConsulNamespace returns the default namespace if the passed namespace

control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go

@@ -444,12 +444,7 @@ func getEffectiveTargetPort(targetPort intstr.IntOrString, prefixedPods selector
 	targetPortInt := int32(targetPort.IntValue())
 	var mostPrevalentContainerPort *corev1.ContainerPort
 	maxCount := 0
-	effectiveNameForPort := func(port *corev1.ContainerPort) string {
-		if port.Name != "" {
-			return port.Name
-		}
-		return targetPort.String()
-	}
+	effectiveNameForPort := inject.WorkloadPortName
 	for _, podData := range prefixedPods {
 		containerPort := getTargetContainerPort(targetPortInt, podData.samplePod)
 
@@ -493,7 +488,7 @@ func getEffectiveTargetPort(targetPort intstr.IntOrString, prefixedPods selector
 
 	// If still no match for the target port, fall back to string-ifying the target port name, which
 	// is what the PodController will do when converting unnamed ContainerPorts to Workload ports.
-	return targetPort.String()
+	return constants.UnnamedWorkloadPortNamePrefix + targetPort.String()
 }
 
 // getTargetContainerPort returns the pod ContainerPort matching the given numeric port value, or nil if none is found.

control-plane/connect-inject/controllers/endpointsv2/endpoints_controller_test.go

@@ -229,7 +229,7 @@ func TestReconcile_CreateService(t *testing.T) {
 								Name:       "other",
 								Port:       10001,
 								Protocol:   "TCP",
-								TargetPort: intstr.FromString("10001"),
+								TargetPort: intstr.FromString("cslport-10001"),
 								// no app protocol specified
 							},
 						},
@@ -260,7 +260,7 @@ func TestReconcile_CreateService(t *testing.T) {
 						},
 						{
 							VirtualPort: 10001,
-							TargetPort:  "10001",
+							TargetPort:  "cslport-10001",
 							Protocol:    pbcatalog.Protocol_PROTOCOL_TCP,
 						},
 						{
@@ -554,12 +554,12 @@ func TestReconcile_CreateService(t *testing.T) {
 						},
 						{
 							VirtualPort: 9090,
-							TargetPort:  "6789", // Matches service target number
+							TargetPort:  "cslport-6789", // Matches service target number
 							Protocol:    pbcatalog.Protocol_PROTOCOL_GRPC,
 						},
 						{
 							VirtualPort: 10010,
-							TargetPort:  "10010", // Matches service target number (unmatched by container ports)
+							TargetPort:  "cslport-10010", // Matches service target number (unmatched by container ports)
 							Protocol:    pbcatalog.Protocol_PROTOCOL_HTTP,
 						},
 						{
@@ -713,7 +713,7 @@ func TestReconcile_CreateService(t *testing.T) {
 						},
 						{
 							VirtualPort: 9090,
-							TargetPort:  "6789", // Matches service target number due to unnamed being most common
+							TargetPort:  "cslport-6789", // Matches service target number due to unnamed being most common
 							Protocol:    pbcatalog.Protocol_PROTOCOL_GRPC,
 						},
 						{
@@ -1269,7 +1269,7 @@ func TestReconcile_UpdateService(t *testing.T) {
 						},
 						{
 							VirtualPort: 10001,
-							TargetPort:  "10001",
+							TargetPort:  "unspec-port", //this might need to be changed to "my_unspecified_port"
 							Protocol:    pbcatalog.Protocol_PROTOCOL_UNSPECIFIED,
 						},
 						{
@@ -1390,7 +1390,7 @@ func TestReconcile_UpdateService(t *testing.T) {
 								Name:       "other",
 								Port:       10001,
 								Protocol:   "TCP",
-								TargetPort: intstr.FromString("10001"),
+								TargetPort: intstr.FromString("cslport-10001"),
 								// no app protocol specified
 							},
 						},
@@ -1421,7 +1421,7 @@ func TestReconcile_UpdateService(t *testing.T) {
 						},
 						{
 							VirtualPort: 10001,
-							TargetPort:  "10001",
+							TargetPort:  "cslport-10001",
 							Protocol:    pbcatalog.Protocol_PROTOCOL_TCP,
 						},
 						{

control-plane/connect-inject/controllers/pod/pod_controller.go

@@ -9,7 +9,6 @@ import (
 	"errors"
 	"fmt"
 	"regexp"
-	"strconv"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -619,10 +618,7 @@ func getWorkloadPorts(pod corev1.Pod) ([]string, map[string]*pbcatalog.WorkloadP
 
 	for _, container := range pod.Spec.Containers {
 		for _, port := range container.Ports {
-			name := port.Name
-			if name == "" {
-				name = strconv.Itoa(int(port.ContainerPort))
-			}
+			name := inject.WorkloadPortName(&port)
 
 			// TODO: error check reserved "mesh" keyword and 20000
 

control-plane/connect-inject/controllers/pod/pod_controller_test.go

@@ -239,14 +239,14 @@ func TestWorkloadWrite(t *testing.T) {
 			},
 			expectedWorkload: &pbcatalog.Workload{
 				Addresses: []*pbcatalog.WorkloadAddress{
-					{Host: "10.0.0.1", Ports: []string{"80", "8080", "mesh"}},
+					{Host: "10.0.0.1", Ports: []string{"cslport-80", "cslport-8080", "mesh"}},
 				},
 				Ports: map[string]*pbcatalog.WorkloadPort{
-					"80": {
+					"cslport-80": {
 						Port:     80,
 						Protocol: pbcatalog.Protocol_PROTOCOL_UNSPECIFIED,
 					},
-					"8080": {
+					"cslport-8080": {
 						Port:     8080,
 						Protocol: pbcatalog.Protocol_PROTOCOL_UNSPECIFIED,
 					},

control-plane/connect-inject/webhookv2/mesh_webhook.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
+	"strings"
 
 	mapset "github.com/deckarep/golang-set"
 	"github.com/go-logr/logr"
@@ -251,6 +252,16 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi
 
 	w.Log.Info("received pod", "name", req.Name, "ns", req.Namespace)
 
+	// Validate that none of the pod ports start with the prefix "cslport-" as that may result in conflicts with ports
+	// created by the pod controller when creating workloads.
+	for _, c := range pod.Spec.Containers {
+		for _, p := range c.Ports {
+			if strings.HasPrefix(p.Name, constants.UnnamedWorkloadPortNamePrefix) {
+				return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error creating pod: port names cannot be prefixed with \"cslport-\" as that prefix is reserved"))
+			}
+		}
+	}
+
 	// Add our volume that will be shared by the init container and
 	// the sidecar for passing data in the pod.
 	pod.Spec.Volumes = append(pod.Spec.Volumes, w.containerVolume())

control-plane/connect-inject/webhookv2/mesh_webhook_test.go

@@ -1164,6 +1164,115 @@ func TestHandlerHandle_ValidateOverwriteProbes(t *testing.T) {
 	}
 }
 
+func TestHandlerValidatePorts(t *testing.T) {
+	cases := []struct {
+		Name string
+		Pod  *corev1.Pod
+		Err  string
+	}{
+		{
+			"basic pod, with ports",
+			&corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "web",
+							Ports: []corev1.ContainerPort{
+								{
+									Name:          "http",
+									ContainerPort: 8080,
+								},
+							},
+						},
+						{
+							Name: "web-side",
+						},
+					},
+				},
+			},
+			"",
+		},
+		{
+			"basic pod, with unnamed ports",
+			&corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "web",
+							Ports: []corev1.ContainerPort{
+								{
+									ContainerPort: 8080,
+								},
+							},
+						},
+						{
+							Name: "web-side",
+						},
+					},
+				},
+			},
+			"",
+		},
+		{
+			"basic pod, with invalid prefix name",
+			&corev1.Pod{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name: "web",
+							Ports: []corev1.ContainerPort{
+								{
+									Name:          "cslport-8080",
+									ContainerPort: 8080,
+								},
+							},
+						},
+						{
+							Name: "web-side",
+						},
+					},
+				},
+			},
+			"error creating pod: port names cannot be prefixed with \"cslport-\" as that prefix is reserved",
+		},
+	}
+	for _, tt := range cases {
+		t.Run(tt.Name, func(t *testing.T) {
+			s := runtime.NewScheme()
+			s.AddKnownTypes(schema.GroupVersion{
+				Group:   "",
+				Version: "v1",
+			}, &corev1.Pod{})
+			decoder, err := admission.NewDecoder(s)
+			require.NoError(t, err)
+
+			w := MeshWebhook{
+				Log:                    logrtest.New(t),
+				AllowK8sNamespacesSet:  mapset.NewSetWith("*"),
+				DenyK8sNamespacesSet:   mapset.NewSet(),
+				EnableTransparentProxy: true,
+				TProxyOverwriteProbes:  true,
+				decoder:                decoder,
+				ConsulConfig:           &consul.Config{HTTPPort: 8500},
+				Clientset:              defaultTestClientWithNamespace(),
+			}
+			req := admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					Namespace: namespaces.DefaultNamespace,
+					Object:    encodeRaw(t, tt.Pod),
+				},
+			}
+			resp := w.Handle(context.Background(), req)
+			if tt.Err == "" {
+				require.True(t, resp.Allowed)
+			} else {
+				require.False(t, resp.Allowed)
+				require.Contains(t, resp.Result.Message, tt.Err)
+			}
+
+		})
+	}
+}
 func TestHandlerDefaultAnnotations(t *testing.T) {
 	cases := []struct {
 		Name     string