From eef95ee2284dbc6b2dcf07f3e07f342d81d7614b Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Tue, 30 Jan 2024 14:12:58 -0800 Subject: [PATCH 1/4] fix endpoints and pods controller --- .../endpointsv2/endpoints_controller.go | 13 +++++++++---- .../endpointsv2/endpoints_controller_test.go | 16 ++++++++-------- .../controllers/pod/pod_controller.go | 8 ++++++-- .../controllers/pod/pod_controller_test.go | 6 +++--- 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go b/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go index 82dd583dfc..8d79499059 100644 --- a/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go @@ -9,6 +9,7 @@ import ( "fmt" "net" "sort" + "strconv" "strings" "github.com/go-logr/logr" @@ -445,10 +446,14 @@ func getEffectiveTargetPort(targetPort intstr.IntOrString, prefixedPods selector var mostPrevalentContainerPort *corev1.ContainerPort maxCount := 0 effectiveNameForPort := func(port *corev1.ContainerPort) string { - if port.Name != "" { - return port.Name + var isNum bool + if _, err := strconv.Atoi(port.Name); err == nil { + isNum = true } - return targetPort.String() + if port.Name == "" || isNum { + return "cslport-" + targetPort.String() + } + return port.Name } for _, podData := range prefixedPods { containerPort := getTargetContainerPort(targetPortInt, podData.samplePod) @@ -493,7 +498,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 "cslport-" + targetPort.String() } // getTargetContainerPort returns the pod ContainerPort matching the given numeric port value, or nil if none is found. diff --git a/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller_test.go b/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller_test.go index c55ca06cdb..0c0733191b 100644 --- a/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller_test.go +++ b/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, }, { diff --git a/control-plane/connect-inject/controllers/pod/pod_controller.go b/control-plane/connect-inject/controllers/pod/pod_controller.go index f0d319a475..b39bcbcbe6 100644 --- a/control-plane/connect-inject/controllers/pod/pod_controller.go +++ b/control-plane/connect-inject/controllers/pod/pod_controller.go @@ -620,8 +620,12 @@ 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)) + var isNum bool + if _, err := strconv.Atoi(name); err == nil { + isNum = true + } + if name == "" || isNum { + name = "cslport-" + strconv.Itoa(int(port.ContainerPort)) } // TODO: error check reserved "mesh" keyword and 20000 diff --git a/control-plane/connect-inject/controllers/pod/pod_controller_test.go b/control-plane/connect-inject/controllers/pod/pod_controller_test.go index 55ff24ae5b..8d08852ed1 100644 --- a/control-plane/connect-inject/controllers/pod/pod_controller_test.go +++ b/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, }, From bfcab37cd1bca22845ca9e606840b96b125a1224 Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Tue, 30 Jan 2024 14:42:56 -0800 Subject: [PATCH 2/4] add validation --- .../connect-inject/webhookv2/mesh_webhook.go | 11 ++ .../webhookv2/mesh_webhook_test.go | 109 ++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/control-plane/connect-inject/webhookv2/mesh_webhook.go b/control-plane/connect-inject/webhookv2/mesh_webhook.go index 839b3fd4ab..d5f19fbd02 100644 --- a/control-plane/connect-inject/webhookv2/mesh_webhook.go +++ b/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, "cslport-") { + 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()) diff --git a/control-plane/connect-inject/webhookv2/mesh_webhook_test.go b/control-plane/connect-inject/webhookv2/mesh_webhook_test.go index ee68db5c14..8bb6dc7a2f 100644 --- a/control-plane/connect-inject/webhookv2/mesh_webhook_test.go +++ b/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 From fd6aab5e890630841577956eb0298c7105d3d3d1 Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Tue, 30 Jan 2024 19:16:33 -0800 Subject: [PATCH 3/4] refactor common logic and pull out constants --- control-plane/connect-inject/common/common.go | 14 ++++++++++++++ .../connect-inject/constants/constants.go | 2 ++ .../endpointsv2/endpoints_controller.go | 14 ++------------ .../controllers/pod/pod_controller.go | 10 +--------- .../connect-inject/webhookv2/mesh_webhook.go | 2 +- 5 files changed, 20 insertions(+), 22 deletions(-) diff --git a/control-plane/connect-inject/common/common.go b/control-plane/connect-inject/common/common.go index 9bbaeaab58..abb165624f 100644 --- a/control-plane/connect-inject/common/common.go +++ b/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 = "cslport-" + 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. diff --git a/control-plane/connect-inject/constants/constants.go b/control-plane/connect-inject/constants/constants.go index 8913019a19..d4d109ade5 100644 --- a/control-plane/connect-inject/constants/constants.go +++ b/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 diff --git a/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go b/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go index 8d79499059..6e98f5f714 100644 --- a/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go +++ b/control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go @@ -9,7 +9,6 @@ import ( "fmt" "net" "sort" - "strconv" "strings" "github.com/go-logr/logr" @@ -445,16 +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 { - var isNum bool - if _, err := strconv.Atoi(port.Name); err == nil { - isNum = true - } - if port.Name == "" || isNum { - return "cslport-" + targetPort.String() - } - return port.Name - } + effectiveNameForPort := inject.WorkloadPortName for _, podData := range prefixedPods { containerPort := getTargetContainerPort(targetPortInt, podData.samplePod) @@ -498,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 "cslport-" + targetPort.String() + return constants.UnnamedWorkloadPortNamePrefix + targetPort.String() } // getTargetContainerPort returns the pod ContainerPort matching the given numeric port value, or nil if none is found. diff --git a/control-plane/connect-inject/controllers/pod/pod_controller.go b/control-plane/connect-inject/controllers/pod/pod_controller.go index b39bcbcbe6..8bc29302fe 100644 --- a/control-plane/connect-inject/controllers/pod/pod_controller.go +++ b/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,14 +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 - var isNum bool - if _, err := strconv.Atoi(name); err == nil { - isNum = true - } - if name == "" || isNum { - name = "cslport-" + strconv.Itoa(int(port.ContainerPort)) - } + name := inject.WorkloadPortName(&port) // TODO: error check reserved "mesh" keyword and 20000 diff --git a/control-plane/connect-inject/webhookv2/mesh_webhook.go b/control-plane/connect-inject/webhookv2/mesh_webhook.go index d5f19fbd02..590608bce7 100644 --- a/control-plane/connect-inject/webhookv2/mesh_webhook.go +++ b/control-plane/connect-inject/webhookv2/mesh_webhook.go @@ -256,7 +256,7 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi // created by the pod controller when creating workloads. for _, c := range pod.Spec.Containers { for _, p := range c.Ports { - if strings.HasPrefix(p.Name, "cslport-") { + 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")) } } From 20c54049aef9246bfef5eaa7afc6154de74189fc Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Wed, 31 Jan 2024 12:59:25 -0800 Subject: [PATCH 4/4] review comments --- control-plane/connect-inject/common/common.go | 2 +- .../connect-inject/common/common_test.go | 40 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/control-plane/connect-inject/common/common.go b/control-plane/connect-inject/common/common.go index abb165624f..569b4d96e6 100644 --- a/control-plane/connect-inject/common/common.go +++ b/control-plane/connect-inject/common/common.go @@ -82,7 +82,7 @@ func WorkloadPortName(port *corev1.ContainerPort) string { isNum = true } if name == "" || isNum { - name = "cslport-" + strconv.Itoa(int(port.ContainerPort)) + name = constants.UnnamedWorkloadPortNamePrefix + strconv.Itoa(int(port.ContainerPort)) } return name } diff --git a/control-plane/connect-inject/common/common_test.go b/control-plane/connect-inject/common/common_test.go index cd4371062d..e3d6ac7227 100644 --- a/control-plane/connect-inject/common/common_test.go +++ b/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