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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eef95ee
fix endpoints and pods controller
ndhanushkodi Jan 30, 2024
bfcab37
add validation
ndhanushkodi Jan 30, 2024
fd6aab5
refactor common logic and pull out constants
ndhanushkodi Jan 31, 2024
20c5404
review comments
ndhanushkodi Jan 31, 2024
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
14 changes: 14 additions & 0 deletions control-plane/connect-inject/common/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

can we add some unit tests for this function?

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 i forgot to add unit tests on this side after refactoring

name := port.Name
var isNum bool
if _, err := strconv.Atoi(name); err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

codegolf, nonblocking, unnecessary optimization:

You don't have to try to Atoi the empty string, since you know it won't work.

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 right, makes sense, I may skip for now because just trying to get the tests green to merge before code freeze 🤞

isNum = true
}
if name == "" || isNum {
name = "cslport-" + strconv.Itoa(int(port.ContainerPort))
Copy link
Member

Choose a reason for hiding this comment

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

should we use the constant UnnamedWorkloadPortNamePrefix here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup updated thanks

}
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.
Expand Down
2 changes: 2 additions & 0 deletions control-plane/connect-inject/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 2 additions & 7 deletions control-plane/connect-inject/controllers/endpointsv2/endpoints_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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.
Expand Down
16 changes: 8 additions & 8 deletions control-plane/connect-inject/controllers/endpointsv2/endpoints_controller_test.go
View file
Open in desktop
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
6 changes: 1 addition & 5 deletions control-plane/connect-inject/controllers/pod/pod_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"errors"
"fmt"
"regexp"
"strconv"
"strings"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -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

Expand Down
6 changes: 3 additions & 3 deletions control-plane/connect-inject/controllers/pod/pod_controller_test.go
View file
Open in desktop
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
View file
Open in desktop
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, constants.UnnamedWorkloadPortNamePrefix) {
Copy link
Member

Choose a reason for hiding this comment

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

not necessarily for this PR but are we tracking anywhere restrictions like this that we should document for end users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this particular case I'd not expect users to run into this and would expect if they do that it's easily discoverable through the pod not being accepted for injection. It definitely feels like a thing that'd be glazed over in any docs. Any other restrictions (so far) are enforced through kube validations

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
View file
Open in desktop
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