From 172a3f51f5bc88a4e8aa2c47e01cc9e454ccfe8b Mon Sep 17 00:00:00 2001 From: trevorLeonHC <137422945+trevorLeonHC@users.noreply.github.com> Date: Wed, 23 Aug 2023 10:08:28 -0500 Subject: [PATCH] Net 1784 inject sidecar first (#2743) * change container creation order. Change order of container creation so that envoy container is created before app container. * change tests to fit proxy container added first * add sidecar first iff lifecycle enabled * update tests to include/exclude lifecycle * container ordering in multiport + lifecycle, test case * create changelog * change exec calls to specify container specify containers when exec'ing * Update 2743.txt * small fixes to appending sidecar --- .changelog/2743.txt | 3 + .../framework/connhelper/connect_helper.go | 4 +- .../connect/connect_external_servers_test.go | 2 +- .../connect/connect_inject_namespaces_test.go | 2 +- .../tests/connect/connect_inject_test.go | 4 +- acceptance/tests/metrics/metrics_test.go | 8 +- .../partitions/partitions_connect_test.go | 8 +- .../connect-inject/webhook/mesh_webhook.go | 36 ++++- .../webhook/mesh_webhook_test.go | 152 ++++++++++++++++++ 9 files changed, 199 insertions(+), 20 deletions(-) create mode 100644 .changelog/2743.txt 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 9df2ac8b8c..8a3fb12c78 100644 --- a/acceptance/framework/connhelper/connect_helper.go +++ b/acceptance/framework/connhelper/connect_helper.go @@ -358,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 @@ -383,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 4308b3f0fa..5bca2bd33d 100644 --- a/control-plane/connect-inject/webhook/mesh_webhook.go +++ b/control-plane/connect-inject/webhook/mesh_webhook.go @@ -298,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. @@ -315,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. @@ -331,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 != "" { @@ -386,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...) } } diff --git a/control-plane/connect-inject/webhook/mesh_webhook_test.go b/control-plane/connect-inject/webhook/mesh_webhook_test.go index 4f273f2b63..64dbd21c9a 100644 --- a/control-plane/connect-inject/webhook/mesh_webhook_test.go +++ b/control-plane/connect-inject/webhook/mesh_webhook_test.go @@ -142,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", @@ -815,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{