Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"net"
"sort"
"strconv"
"strings"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it belongs in some constants package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep will refactor

}

// getTargetContainerPort returns the pod ContainerPort matching the given numeric port value, or nil if none is found.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
},
Expand Down Expand Up @@ -260,7 +260,7 @@ func TestReconcile_CreateService(t *testing.T) {
},
{
VirtualPort: 10001,
TargetPort: "10001",
TargetPort: "cslport-10001",
Protocol: pbcatalog.Protocol_PROTOCOL_TCP,
},
{
Expand Down Expand Up @@ -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,
},
{
Expand Down Expand Up @@ -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,
},
{
Expand Down Expand Up @@ -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,
},
{
Expand Down Expand Up @@ -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
},
},
Expand Down Expand Up @@ -1421,7 +1421,7 @@ func TestReconcile_UpdateService(t *testing.T) {
},
{
VirtualPort: 10001,
TargetPort: "10001",
TargetPort: "cslport-10001",
Protocol: pbcatalog.Protocol_PROTOCOL_TCP,
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down
11 changes: 11 additions & 0 deletions control-plane/connect-inject/webhookv2/mesh_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"net/http"
"strconv"
"strings"

mapset "github.com/deckarep/golang-set"
"github.com/go-logr/logr"
Expand Down Expand Up @@ -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())
Expand Down
109 changes: 109 additions & 0 deletions control-plane/connect-inject/webhookv2/mesh_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading