Skip to content

fix: lifecycle enabled iptables mismatch #2842

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

Merged
DanStough merged 1 commit into from
Aug 25, 2023
Merged

fix: lifecycle enabled iptables mismatch #2842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 additions & 4 deletions acceptance/framework/connhelper/connect_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@ import (
"time"

terratestK8s "github.com/gruntwork-io/terratest/modules/k8s"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/hashicorp/consul-k8s/acceptance/framework/config"
"github.com/hashicorp/consul-k8s/acceptance/framework/consul"
"github.com/hashicorp/consul-k8s/acceptance/framework/environment"
"github.com/hashicorp/consul-k8s/acceptance/framework/helpers"
"github.com/hashicorp/consul-k8s/acceptance/framework/k8s"
"github.com/hashicorp/consul-k8s/acceptance/framework/logger"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const (
Expand Down
27 changes: 16 additions & 11 deletions control-plane/connect-inject/webhook/mesh_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,6 @@ import (

mapset "github.com/deckarep/golang-set"
"github.com/go-logr/logr"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/common"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/lifecycle"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/metrics"
"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/namespaces"
"github.com/hashicorp/consul-k8s/control-plane/version"
"gomodules.xyz/jsonpatch/v2"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand All @@ -30,6 +23,14 @@ import (
"k8s.io/client-go/kubernetes"
_ "k8s.io/client-go/plugin/pkg/client/auth"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

"github.com/hashicorp/consul-k8s/control-plane/connect-inject/common"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/lifecycle"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/metrics"
"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/namespaces"
"github.com/hashicorp/consul-k8s/control-plane/version"
)

const (
Expand Down Expand Up @@ -533,20 +534,24 @@ func (w *MeshWebhook) overwriteProbes(ns corev1.Namespace, pod *corev1.Pod) erro
}

if tproxyEnabled && overwriteProbes {
for i, container := range pod.Spec.Containers {
// We don't use the loop index because this needs to line up w.withiptablesConfigJSON,
// which is performed before the sidecar is injected.
Comment on lines +537 to +538
Copy link
Copy Markdown
Contributor

@t-eckert t-eckert Aug 25, 2023

Choose a reason for hiding this comment

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

Great comment. I think it would be easy to get confused on this otherwise.

idx := 0
for _, container := range pod.Spec.Containers {
// skip the "envoy-sidecar" container from having it's probes overridden
if container.Name == sidecarContainer {
continue
}
if container.LivenessProbe != nil && container.LivenessProbe.HTTPGet != nil {
container.LivenessProbe.HTTPGet.Port = intstr.FromInt(exposedPathsLivenessPortsRangeStart + i)
container.LivenessProbe.HTTPGet.Port = intstr.FromInt(exposedPathsLivenessPortsRangeStart + idx)
}
if container.ReadinessProbe != nil && container.ReadinessProbe.HTTPGet != nil {
container.ReadinessProbe.HTTPGet.Port = intstr.FromInt(exposedPathsReadinessPortsRangeStart + i)
container.ReadinessProbe.HTTPGet.Port = intstr.FromInt(exposedPathsReadinessPortsRangeStart + idx)
}
if container.StartupProbe != nil && container.StartupProbe.HTTPGet != nil {
container.StartupProbe.HTTPGet.Port = intstr.FromInt(exposedPathsStartupPortsRangeStart + i)
container.StartupProbe.HTTPGet.Port = intstr.FromInt(exposedPathsStartupPortsRangeStart + idx)
}
idx++
}
}
return nil
Expand Down
229 changes: 223 additions & 6 deletions control-plane/connect-inject/webhook/mesh_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,13 @@ package webhook
import (
"context"
"encoding/json"
"strconv"
"strings"
"testing"

mapset "github.com/deckarep/golang-set"
logrtest "github.com/go-logr/logr/testr"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/lifecycle"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/metrics"
"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/namespaces"
"github.com/hashicorp/consul-k8s/control-plane/version"
"github.com/hashicorp/consul/sdk/iptables"
"github.com/stretchr/testify/require"
"gomodules.xyz/jsonpatch/v2"
admissionv1 "k8s.io/api/admission/v1"
Expand All @@ -28,6 +24,13 @@ import (
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/fake"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/lifecycle"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/metrics"
"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/namespaces"
"github.com/hashicorp/consul-k8s/control-plane/version"
)

func TestHandlerHandle(t *testing.T) {
Expand Down Expand Up @@ -1129,6 +1132,220 @@ func TestHandlerHandle(t *testing.T) {
}
}

// This test validates that overwrite probes match the iptables configuration fromiptablesConfigJSON()
// Because they happen at different points in the injection, the port numbers can get out of sync.
func TestHandlerHandle_ValidateOverwriteProbes(t *testing.T) {
t.Parallel()
s := runtime.NewScheme()
s.AddKnownTypes(schema.GroupVersion{
Group: "",
Version: "v1",
}, &corev1.Pod{})
decoder, err := admission.NewDecoder(s)
require.NoError(t, err)

cases := []struct {
Name string
Webhook MeshWebhook
Req admission.Request
Err string // expected error string, not exact
Patches []jsonpatch.Operation
}{
{
"tproxy with overwriteProbes is enabled",
MeshWebhook{
Log: logrtest.New(t),
AllowK8sNamespacesSet: mapset.NewSetWith("*"),
DenyK8sNamespacesSet: mapset.NewSet(),
EnableTransparentProxy: true,
TProxyOverwriteProbes: true,
LifecycleConfig: lifecycle.Config{DefaultEnableProxyLifecycle: true},
decoder: decoder,
Clientset: defaultTestClientWithNamespace(),
},
admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Namespace: namespaces.DefaultNamespace,
Object: encodeRaw(t, &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{},
// We're setting an existing annotation so that we can assert on the
// specific annotations that are set as a result of probes being overwritten.
Annotations: map[string]string{"foo": "bar"},
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: "web",
LivenessProbe: &corev1.Probe{
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Port: intstr.FromInt(8080),
},
},
},
ReadinessProbe: &corev1.Probe{
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Port: intstr.FromInt(8081),
},
},
},
StartupProbe: &corev1.Probe{
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Port: intstr.FromInt(8082),
},
},
},
},
},
},
}),
},
},
"",
[]jsonpatch.Operation{
{
Operation: "add",
Path: "/spec/volumes",
},
{
Operation: "add",
Path: "/spec/initContainers",
},
{
Operation: "add",
Path: "/spec/containers/1",
},
{
Operation: "replace",
Path: "/spec/containers/0/name",
},
{
Operation: "add",
Path: "/spec/containers/0/args",
},
{
Operation: "add",
Path: "/spec/containers/0/env",
},
{
Operation: "add",
Path: "/spec/containers/0/volumeMounts",
},
{
Operation: "add",
Path: "/spec/containers/0/readinessProbe/tcpSocket",
},
{
Operation: "add",
Path: "/spec/containers/0/readinessProbe/initialDelaySeconds",
},
{
Operation: "remove",
Path: "/spec/containers/0/readinessProbe/httpGet",
},
{
Operation: "add",
Path: "/spec/containers/0/securityContext",
},
{
Operation: "remove",
Path: "/spec/containers/0/startupProbe",
},
{
Operation: "remove",
Path: "/spec/containers/0/livenessProbe",
},
{
Operation: "add",
Path: "/metadata/labels",
},
{
Operation: "add",
Path: "/metadata/annotations/" + escapeJSONPointer(constants.KeyInjectStatus),
},
{
Operation: "add",
Path: "/metadata/annotations/" + escapeJSONPointer(constants.KeyTransparentProxyStatus),
},

{
Operation: "add",
Path: "/metadata/annotations/" + escapeJSONPointer(constants.AnnotationOriginalPod),
},
{
Operation: "add",
Path: "/metadata/annotations/" + escapeJSONPointer(constants.AnnotationConsulK8sVersion),
},
},
},
}

for _, tt := range cases {
t.Run(tt.Name, func(t *testing.T) {
tt.Webhook.ConsulConfig = &consul.Config{HTTPPort: 8500}
ctx := context.Background()
resp := tt.Webhook.Handle(ctx, tt.Req)
if (tt.Err == "") != resp.Allowed {
Comment thread
DanStough marked this conversation as resolved.
t.Fatalf("allowed: %v, expected err: %v", resp.Allowed, tt.Err)
}
if tt.Err != "" {
require.Contains(t, resp.Result.Message, tt.Err)
return
}

var iptablesCfg iptables.Config
var overwritePorts []string
actual := resp.Patches
if len(actual) > 0 {
for i := range actual {

// We want to grab the iptables configuration from the connect-init container's
// environment.
if actual[i].Path == "/spec/initContainers" {
value := actual[i].Value.([]any)
valueMap := value[0].(map[string]any)
envs := valueMap["env"].([]any)
redirectEnv := envs[8].(map[string]any)
require.Equal(t, redirectEnv["name"].(string), "CONSUL_REDIRECT_TRAFFIC_CONFIG")
Comment thread
DanStough marked this conversation as resolved.
iptablesJson := redirectEnv["value"].(string)

err := json.Unmarshal([]byte(iptablesJson), &iptablesCfg)
require.NoError(t, err)
}

// We want to accumulate the httpGet Probes from the application container to
// compare them to the iptables rules. This is now the second container in the spec
if strings.Contains(actual[i].Path, "/spec/containers/1") {
valueMap, ok := actual[i].Value.(map[string]any)
require.True(t, ok)

for k, v := range valueMap {
if strings.Contains(k, "Probe") {
probe := v.(map[string]any)
httpProbe := probe["httpGet"]
httpProbeMap := httpProbe.(map[string]any)
port := httpProbeMap["port"]
portNum := port.(float64)

overwritePorts = append(overwritePorts, strconv.Itoa(int(portNum)))
}
}
}

// nil out all the patch values to just compare the keys changing.
actual[i].Value = nil
Comment thread
DanStough marked this conversation as resolved.
}
}
// Make sure the iptables excluded ports match the ports on the container
require.ElementsMatch(t, iptablesCfg.ExcludeInboundPorts, overwritePorts)
require.ElementsMatch(t, tt.Patches, actual)
})
}
}

func TestHandlerDefaultAnnotations(t *testing.T) {
cases := []struct {
Name string
Expand Down
19 changes: 12 additions & 7 deletions control-plane/connect-inject/webhook/redirect_traffic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ import (
"fmt"
"strconv"

"github.com/hashicorp/consul-k8s/control-plane/connect-inject/common"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
"github.com/hashicorp/consul/sdk/iptables"
corev1 "k8s.io/api/core/v1"

"github.com/hashicorp/consul-k8s/control-plane/connect-inject/common"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
)

// addRedirectTrafficConfigAnnotation creates an iptables.Config in JSON format based on proxy configuration.
Expand Down Expand Up @@ -62,20 +63,24 @@ func (w *MeshWebhook) iptablesConfigJSON(pod corev1.Pod, ns corev1.Namespace) (s
}

if overwriteProbes {
for i, container := range pod.Spec.Containers {
// skip the "envoy-sidecar" container from having its probes overridden
// We don't use the loop index because this needs to line up w.overwriteProbes(),
// which is performed after the sidecar is injected.
idx := 0
for _, container := range pod.Spec.Containers {
// skip the "consul-dataplane" container from having its probes overridden
if container.Name == sidecarContainer {
continue
}
if container.LivenessProbe != nil && container.LivenessProbe.HTTPGet != nil {
cfg.ExcludeInboundPorts = append(cfg.ExcludeInboundPorts, strconv.Itoa(exposedPathsLivenessPortsRangeStart+i))
cfg.ExcludeInboundPorts = append(cfg.ExcludeInboundPorts, strconv.Itoa(exposedPathsLivenessPortsRangeStart+idx))
}
if container.ReadinessProbe != nil && container.ReadinessProbe.HTTPGet != nil {
cfg.ExcludeInboundPorts = append(cfg.ExcludeInboundPorts, strconv.Itoa(exposedPathsReadinessPortsRangeStart+i))
cfg.ExcludeInboundPorts = append(cfg.ExcludeInboundPorts, strconv.Itoa(exposedPathsReadinessPortsRangeStart+idx))
}
if container.StartupProbe != nil && container.StartupProbe.HTTPGet != nil {
cfg.ExcludeInboundPorts = append(cfg.ExcludeInboundPorts, strconv.Itoa(exposedPathsStartupPortsRangeStart+i))
cfg.ExcludeInboundPorts = append(cfg.ExcludeInboundPorts, strconv.Itoa(exposedPathsStartupPortsRangeStart+idx))
}
idx++
}
}

Expand Down