diff --git a/.changelog/2743.txt b/.changelog/2743.txt new file mode 100644 index 0000000000..4e8db233b1 --- /dev/null +++ b/.changelog/2743.txt @@ -0,0 +1,3 @@ +```release-note:improvement +control-plane: Changed the container ordering in connect-inject to insert consul-dataplane container first if lifecycle is enabled. Container ordering is unchanged if lifecycle is disabled. +``` diff --git a/acceptance/framework/connhelper/connect_helper.go b/acceptance/framework/connhelper/connect_helper.go index f5ee134b04..8a3fb12c78 100644 --- a/acceptance/framework/connhelper/connect_helper.go +++ b/acceptance/framework/connhelper/connect_helper.go @@ -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 ( @@ -357,7 +358,7 @@ func (c *ConnectHelper) TestConnectionFailureWhenUnhealthy(t *testing.T) { opts := c.KubectlOptsForApp(t) logger.Log(t, "testing k8s -> consul health checks sync by making the static-server unhealthy") - k8s.RunKubectl(t, opts, "exec", "deploy/"+StaticServerName, "--", "touch", "/tmp/unhealthy") + k8s.RunKubectl(t, opts, "exec", "deploy/"+StaticServerName, "-c", "static-server", "--", "touch", "/tmp/unhealthy") // The readiness probe should take a moment to be reflected in Consul, // CheckStaticServerConnection will retry until Consul marks the service @@ -382,7 +383,7 @@ func (c *ConnectHelper) TestConnectionFailureWhenUnhealthy(t *testing.T) { } // Return the static-server to a "healthy state". - k8s.RunKubectl(t, opts, "exec", "deploy/"+StaticServerName, "--", "rm", "/tmp/unhealthy") + k8s.RunKubectl(t, opts, "exec", "deploy/"+StaticServerName, "-c", "static-server", "--", "rm", "/tmp/unhealthy") } // helmValues uses the Secure and AutoEncrypt fields to set values for the Helm diff --git a/acceptance/tests/connect/connect_external_servers_test.go b/acceptance/tests/connect/connect_external_servers_test.go index 46f9e14573..c0a61f160f 100644 --- a/acceptance/tests/connect/connect_external_servers_test.go +++ b/acceptance/tests/connect/connect_external_servers_test.go @@ -128,7 +128,7 @@ func TestConnectInject_ExternalServers(t *testing.T) { // Test that kubernetes readiness status is synced to Consul. // Create the file so that the readiness probe of the static-server pod fails. logger.Log(t, "testing k8s -> consul health checks sync by making the static-server unhealthy") - k8s.RunKubectl(t, ctx.KubectlOptions(t), "exec", "deploy/"+connhelper.StaticServerName, "--", "touch", "/tmp/unhealthy") + k8s.RunKubectl(t, ctx.KubectlOptions(t), "exec", "deploy/"+connhelper.StaticServerName, "-c", "static-server", "--", "touch", "/tmp/unhealthy") // The readiness probe should take a moment to be reflected in Consul, CheckStaticServerConnection will retry // until Consul marks the service instance unavailable for mesh traffic, causing the connection to fail. diff --git a/acceptance/tests/connect/connect_inject_namespaces_test.go b/acceptance/tests/connect/connect_inject_namespaces_test.go index 7b7785a44f..03200cc75b 100644 --- a/acceptance/tests/connect/connect_inject_namespaces_test.go +++ b/acceptance/tests/connect/connect_inject_namespaces_test.go @@ -221,7 +221,7 @@ func TestConnectInjectNamespaces(t *testing.T) { // Test that kubernetes readiness status is synced to Consul. // Create the file so that the readiness probe of the static-server pod fails. logger.Log(t, "testing k8s -> consul health checks sync by making the static-server unhealthy") - k8s.RunKubectl(t, staticServerOpts, "exec", "deploy/"+connhelper.StaticServerName, "--", "touch", "/tmp/unhealthy") + k8s.RunKubectl(t, staticServerOpts, "exec", "deploy/"+connhelper.StaticServerName, "-c", "static-server", "--", "touch", "/tmp/unhealthy") // The readiness probe should take a moment to be reflected in Consul, CheckStaticServerConnection will retry // until Consul marks the service instance unavailable for mesh traffic, causing the connection to fail. diff --git a/acceptance/tests/connect/connect_inject_test.go b/acceptance/tests/connect/connect_inject_test.go index 39235db36e..3a54cae517 100644 --- a/acceptance/tests/connect/connect_inject_test.go +++ b/acceptance/tests/connect/connect_inject_test.go @@ -328,9 +328,9 @@ func TestConnectInject_MultiportServices(t *testing.T) { // and check inbound connections to the multi port pods' services. // Create the files so that the readiness probes of the multi port pod fails. logger.Log(t, "testing k8s -> consul health checks sync by making the multiport unhealthy") - k8s.RunKubectl(t, ctx.KubectlOptions(t), "exec", "deploy/"+multiport, "--", "touch", "/tmp/unhealthy-multiport") + k8s.RunKubectl(t, ctx.KubectlOptions(t), "exec", "deploy/"+multiport, "-c", "multiport", "--", "touch", "/tmp/unhealthy-multiport") logger.Log(t, "testing k8s -> consul health checks sync by making the multiport-admin unhealthy") - k8s.RunKubectl(t, ctx.KubectlOptions(t), "exec", "deploy/"+multiport, "--", "touch", "/tmp/unhealthy-multiport-admin") + k8s.RunKubectl(t, ctx.KubectlOptions(t), "exec", "deploy/"+multiport, "-c", "multiport-admin", "--", "touch", "/tmp/unhealthy-multiport-admin") // The readiness probe should take a moment to be reflected in Consul, CheckStaticServerConnection will retry // until Consul marks the service instance unavailable for mesh traffic, causing the connection to fail. diff --git a/acceptance/tests/metrics/metrics_test.go b/acceptance/tests/metrics/metrics_test.go index 3d40841e36..ca80d38b75 100644 --- a/acceptance/tests/metrics/metrics_test.go +++ b/acceptance/tests/metrics/metrics_test.go @@ -74,12 +74,12 @@ func TestComponentMetrics(t *testing.T) { k8s.DeployKustomize(t, ctx.KubectlOptions(t), cfg.NoCleanupOnFailure, cfg.NoCleanup, cfg.DebugDirectory, "../fixtures/bases/static-client") // Server Metrics - metricsOutput, err := k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), "exec", "deploy/"+StaticClientName, "--", "curl", "--silent", "--show-error", fmt.Sprintf("http://%s:8500/v1/agent/metrics?format=prometheus", fmt.Sprintf("%s-consul-server.%s.svc", releaseName, ns))) + metricsOutput, err := k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), "exec", "deploy/"+StaticClientName, "-c", "static-client", "--", "curl", "--silent", "--show-error", fmt.Sprintf("http://%s:8500/v1/agent/metrics?format=prometheus", fmt.Sprintf("%s-consul-server.%s.svc", releaseName, ns))) require.NoError(t, err) require.Contains(t, metricsOutput, `consul_acl_ResolveToken{quantile="0.5"}`) // Client Metrics - metricsOutput, err = k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), "exec", "deploy/"+StaticClientName, "--", "sh", "-c", "curl --silent --show-error http://$HOST_IP:8500/v1/agent/metrics?format=prometheus") + metricsOutput, err = k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), "exec", "deploy/"+StaticClientName, "-c", "static-client", "--", "sh", "-c", "curl --silent --show-error http://$HOST_IP:8500/v1/agent/metrics?format=prometheus") require.NoError(t, err) require.Contains(t, metricsOutput, `consul_acl_ResolveToken{quantile="0.5"}`) @@ -133,7 +133,7 @@ func TestAppMetrics(t *testing.T) { // Retry because sometimes the merged metrics server takes a couple hundred milliseconds // to start. retry.RunWith(&retry.Counter{Count: 20, Wait: 2 * time.Second}, t, func(r *retry.R) { - metricsOutput, err := k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), "exec", "deploy/"+StaticClientName, "--", "curl", "--silent", "--show-error", fmt.Sprintf("http://%s:20200/metrics", podIP)) + metricsOutput, err := k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), "exec", "deploy/"+StaticClientName, "-c", "static-client", "--", "curl", "--silent", "--show-error", fmt.Sprintf("http://%s:20200/metrics", podIP)) require.NoError(r, err) // This assertion represents the metrics from the envoy sidecar. require.Contains(r, metricsOutput, `envoy_cluster_assignment_stale{local_cluster="server",consul_source_service="server"`) @@ -147,7 +147,7 @@ func assertGatewayMetricsEnabled(t *testing.T, ctx environment.TestContext, ns, require.NoError(t, err) for _, pod := range pods.Items { podIP := pod.Status.PodIP - metricsOutput, err := k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), "exec", "deploy/"+StaticClientName, "--", "curl", "--silent", "--show-error", fmt.Sprintf("http://%s:20200/metrics", podIP)) + metricsOutput, err := k8s.RunKubectlAndGetOutputE(t, ctx.KubectlOptions(t), "exec", "deploy/"+StaticClientName, "-c", "static-client", "--", "curl", "--silent", "--show-error", fmt.Sprintf("http://%s:20200/metrics", podIP)) require.NoError(t, err) require.Contains(t, metricsOutput, metricsAssertion) } diff --git a/acceptance/tests/partitions/partitions_connect_test.go b/acceptance/tests/partitions/partitions_connect_test.go index c53e5b6171..444e70b952 100644 --- a/acceptance/tests/partitions/partitions_connect_test.go +++ b/acceptance/tests/partitions/partitions_connect_test.go @@ -405,8 +405,8 @@ func TestPartitions_Connect(t *testing.T) { // Test that kubernetes readiness status is synced to Consul. // Create the file so that the readiness probe of the static-server pod fails. logger.Log(t, "testing k8s -> consul health checks sync by making the static-server unhealthy") - k8s.RunKubectl(t, defaultPartitionClusterStaticServerOpts, "exec", "deploy/"+staticServerName, "--", "touch", "/tmp/unhealthy") - k8s.RunKubectl(t, secondaryPartitionClusterStaticServerOpts, "exec", "deploy/"+staticServerName, "--", "touch", "/tmp/unhealthy") + k8s.RunKubectl(t, defaultPartitionClusterStaticServerOpts, "exec", "deploy/"+staticServerName, "-c", "static-server", "--", "touch", "/tmp/unhealthy") + k8s.RunKubectl(t, secondaryPartitionClusterStaticServerOpts, "exec", "deploy/"+staticServerName, "-c", "static-server", "--", "touch", "/tmp/unhealthy") // The readiness probe should take a moment to be reflected in Consul, CheckStaticServerConnection will retry // until Consul marks the service instance unavailable for mesh traffic, causing the connection to fail. @@ -578,8 +578,8 @@ func TestPartitions_Connect(t *testing.T) { // Test that kubernetes readiness status is synced to Consul. // Create the file so that the readiness probe of the static-server pod fails. logger.Log(t, "testing k8s -> consul health checks sync by making the static-server unhealthy") - k8s.RunKubectl(t, defaultPartitionClusterStaticServerOpts, "exec", "deploy/"+staticServerName, "--", "touch", "/tmp/unhealthy") - k8s.RunKubectl(t, secondaryPartitionClusterStaticServerOpts, "exec", "deploy/"+staticServerName, "--", "touch", "/tmp/unhealthy") + k8s.RunKubectl(t, defaultPartitionClusterStaticServerOpts, "exec", "deploy/"+staticServerName, "-c", "static-server", "--", "touch", "/tmp/unhealthy") + k8s.RunKubectl(t, secondaryPartitionClusterStaticServerOpts, "exec", "deploy/"+staticServerName, "-c", "static-server", "--", "touch", "/tmp/unhealthy") // The readiness probe should take a moment to be reflected in Consul, CheckStaticServerConnection will retry // until Consul marks the service instance unavailable for mesh traffic, causing the connection to fail. diff --git a/control-plane/connect-inject/webhook/mesh_webhook.go b/control-plane/connect-inject/webhook/mesh_webhook.go index 25443b7e1b..5bca2bd33d 100644 --- a/control-plane/connect-inject/webhook/mesh_webhook.go +++ b/control-plane/connect-inject/webhook/mesh_webhook.go @@ -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" @@ -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 ( @@ -297,7 +298,10 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi // port. annotatedSvcNames := w.annotatedServiceNames(pod) multiPort := len(annotatedSvcNames) > 1 - + lifecycleEnabled, ok := w.LifecycleConfig.EnableProxyLifecycle(pod) + if ok != nil { + w.Log.Error(err, "unable to get lifecycle enabled status") + } // For single port pods, add the single init container and envoy sidecar. if !multiPort { // Add the init container that registers the service and sets up the Envoy configuration. @@ -314,8 +318,14 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi w.Log.Error(err, "error configuring injection sidecar container", "request name", req.Name) return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error configuring injection sidecar container: %s", err)) } - // TODO: invert to start the Envoy sidecar before the application container - pod.Spec.Containers = append(pod.Spec.Containers, envoySidecar) + //Append the Envoy sidecar before the application container only if lifecycle enabled. + + if lifecycleEnabled && ok == nil { + pod.Spec.Containers = append([]corev1.Container{envoySidecar}, pod.Spec.Containers...) + } else { + pod.Spec.Containers = append(pod.Spec.Containers, envoySidecar) + } + } else { // For multi port pods, check for unsupported cases, mount all relevant service account tokens, and mount an init // container and envoy sidecar per port. Tproxy, metrics, and metrics merging are not supported for multi port pods. @@ -330,6 +340,10 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi w.Log.Error(err, "checking unsupported cases for multi port pods") return admission.Errored(http.StatusInternalServerError, err) } + + //List of sidecar containers for each service. Build as a list to preserve correct ordering in relation + //to services. + sidecarContainers := []corev1.Container{} for i, svc := range annotatedSvcNames { w.Log.Info(fmt.Sprintf("service: %s", svc)) if w.AuthMethod != "" { @@ -385,9 +399,20 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi w.Log.Error(err, "error configuring injection sidecar container", "request name", req.Name) return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error configuring injection sidecar container: %s", err)) } - // TODO: invert to start the Envoy sidecar container before the - // application container - pod.Spec.Containers = append(pod.Spec.Containers, envoySidecar) + // If Lifecycle is enabled, add to the list of sidecar containers to be added + // to pod containers at the end in order to preserve relative ordering. + if lifecycleEnabled { + sidecarContainers = append(sidecarContainers, envoySidecar) + } else { + pod.Spec.Containers = append(pod.Spec.Containers, envoySidecar) + + } + + } + + //Add sidecar containers first if lifecycle enabled. + if lifecycleEnabled { + pod.Spec.Containers = append(sidecarContainers, pod.Spec.Containers...) } } @@ -512,20 +537,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. + 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 diff --git a/control-plane/connect-inject/webhook/mesh_webhook_test.go b/control-plane/connect-inject/webhook/mesh_webhook_test.go index 946933f7d7..64dbd21c9a 100644 --- a/control-plane/connect-inject/webhook/mesh_webhook_test.go +++ b/control-plane/connect-inject/webhook/mesh_webhook_test.go @@ -6,16 +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/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" @@ -27,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) { @@ -138,6 +142,73 @@ func TestHandlerHandle(t *testing.T) { }, }, }, + { + "empty pod basic with lifecycle", + MeshWebhook{ + Log: logrtest.New(t), + AllowK8sNamespacesSet: mapset.NewSetWith("*"), + DenyK8sNamespacesSet: mapset.NewSet(), + decoder: decoder, + Clientset: defaultTestClientWithNamespace(), + LifecycleConfig: lifecycle.Config{DefaultEnableProxyLifecycle: true}, + }, + admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Namespace: namespaces.DefaultNamespace, + Object: encodeRaw(t, &corev1.Pod{ + Spec: basicSpec, + }), + }, + }, + "", + []jsonpatch.Operation{ + { + Operation: "add", + Path: "/metadata/labels", + }, + { + Operation: "add", + Path: "/metadata/annotations", + }, + { + Operation: "add", + Path: "/spec/volumes", + }, + { + Operation: "add", + Path: "/spec/initContainers", + }, + { + Operation: "add", + Path: "/spec/containers/1", + }, + + { + Operation: "add", + Path: "/spec/containers/0/readinessProbe", + }, + { + Operation: "add", + Path: "/spec/containers/0/securityContext", + }, + { + 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", + }, + }, + }, { "pod with upstreams specified", @@ -811,6 +882,91 @@ func TestHandlerHandle(t *testing.T) { }, }, }, + { + "multiport pod kube < 1.24 with AuthMethod, serviceaccount has secret ref, lifecycle enabled", + MeshWebhook{ + Log: logrtest.New(t), + AllowK8sNamespacesSet: mapset.NewSetWith("*"), + DenyK8sNamespacesSet: mapset.NewSet(), + decoder: decoder, + Clientset: testClientWithServiceAccountAndSecretRefs(), + AuthMethod: "k8s", + LifecycleConfig: lifecycle.Config{DefaultEnableProxyLifecycle: true}, + }, + admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Namespace: namespaces.DefaultNamespace, + Object: encodeRaw(t, &corev1.Pod{ + Spec: basicSpec, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + constants.AnnotationService: "web,web-admin", + }, + }, + }), + }, + }, + "", + []jsonpatch.Operation{ + { + Operation: "add", + Path: "/spec/containers/0/env", + }, + { + Operation: "add", + Path: "/spec/containers/0/volumeMounts", + }, + { + Operation: "add", + Path: "/spec/containers/0/readinessProbe", + }, + { + Operation: "add", + Path: "/spec/containers/0/securityContext", + }, + { + Operation: "replace", + Path: "/spec/containers/0/name", + }, + { + Operation: "add", + Path: "/spec/containers/0/args", + }, + + { + Operation: "add", + Path: "/spec/volumes", + }, + { + Operation: "add", + Path: "/spec/initContainers", + }, + { + Operation: "add", + Path: "/spec/containers/1", + }, + { + Operation: "add", + Path: "/spec/containers/2", + }, + { + Operation: "add", + Path: "/metadata/annotations/" + escapeJSONPointer(constants.KeyInjectStatus), + }, + { + Operation: "add", + Path: "/metadata/annotations/" + escapeJSONPointer(constants.AnnotationOriginalPod), + }, + { + Operation: "add", + Path: "/metadata/annotations/" + escapeJSONPointer(constants.AnnotationConsulK8sVersion), + }, + { + Operation: "add", + Path: "/metadata/labels", + }, + }, + }, { "dns redirection enabled", MeshWebhook{ @@ -976,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 { + 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") + 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 + } + } + // 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 diff --git a/control-plane/connect-inject/webhook/redirect_traffic.go b/control-plane/connect-inject/webhook/redirect_traffic.go index b0cbefeeaa..f928df4afd 100644 --- a/control-plane/connect-inject/webhook/redirect_traffic.go +++ b/control-plane/connect-inject/webhook/redirect_traffic.go @@ -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. @@ -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++ } }