From ab967fa8d5101aae108d05cbb4cc9e75392f645a Mon Sep 17 00:00:00 2001 From: wuvs Date: Tue, 25 Jun 2019 11:39:40 +0700 Subject: [PATCH 1/2] add resources to injector for sidecar/init container --- connect-inject/container_init.go | 20 +++++++++++++++++ connect-inject/envoy_sidecar.go | 16 ++++++++++++++ connect-inject/handler.go | 14 ++++++++++++ connect-inject/handler_test.go | 33 ++++++++++++++++++++++++++++ go.sum | 1 + subcommand/inject-connect/command.go | 16 ++++++++++++++ 6 files changed, 100 insertions(+) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 8408f432ea..b59d5ef2fb 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -8,6 +8,7 @@ import ( "text/template" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" ) type initContainerCommandData struct { @@ -35,6 +36,10 @@ type initContainerCommandData struct { ConsulCACert string } +type initContainerResources struct { + Resources corev1.ResourceRequirements +} + type initContainerCommandUpstreamData struct { Name string LocalPort int32 @@ -73,6 +78,20 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co panic("No service found. This should be impossible since we default it.") } + icr := initContainerResources{} + if h.Resources { + icr.Resources = corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(h.CPULimit), + corev1.ResourceMemory: resource.MustParse(h.MemoryLimit), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(h.CPULimit), + corev1.ResourceMemory: resource.MustParse(h.MemoryLimit), + }, + } + } + // If a port is specified, then we determine the value of that port // and register that port for the host service. if raw, ok := pod.Annotations[annotationPort]; ok && raw != "" { @@ -223,6 +242,7 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co Value: fmt.Sprintf("$(POD_NAME)-%s", data.ProxyServiceName), }, }, + Resources: icr.Resources, VolumeMounts: volMounts, Command: []string{"/bin/sh", "-ec", buf.String()}, }, nil diff --git a/connect-inject/envoy_sidecar.go b/connect-inject/envoy_sidecar.go index 74d1c2ec86..b7a76a52f5 100644 --- a/connect-inject/envoy_sidecar.go +++ b/connect-inject/envoy_sidecar.go @@ -6,11 +6,13 @@ import ( "text/template" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" ) type sidecarContainerCommandData struct { AuthMethod string ConsulNamespace string + Resources corev1.ResourceRequirements } func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Container, error) { @@ -19,6 +21,19 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con ConsulNamespace: h.consulNamespace(k8sNamespace), } + if h.Resources { + templateData.Resources = corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(h.CPULimit), + corev1.ResourceMemory: resource.MustParse(h.MemoryLimit), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(h.CPULimit), + corev1.ResourceMemory: resource.MustParse(h.MemoryLimit), + }, + } + } + // Render the command var buf bytes.Buffer tpl := template.Must(template.New("root").Parse(strings.TrimSpace( @@ -39,6 +54,7 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con }, }, }, + Resources: templateData.Resources, VolumeMounts: []corev1.VolumeMount{ { Name: volumeName, diff --git a/connect-inject/handler.go b/connect-inject/handler.go index 977943f153..845c002735 100644 --- a/connect-inject/handler.go +++ b/connect-inject/handler.go @@ -163,6 +163,20 @@ type Handler struct { // Only necessary if ACLs are enabled. CrossNamespaceACLPolicy string + // Resources checks if cpu and memory resources for sidecar pods and + // init containers should be set. If this is false, no resources + // will be set. + Resources bool + // CPULimit sets cpu limit for pods + CPULimit string + // MemoryLimit sets memory limit for pods + MemoryLimit string + // CPURequest sets cpu requests for pods + CPURequest string + // MemoryRequest sets memory requests for pods + MemoryRequest string + + // Log Log hclog.Logger } diff --git a/connect-inject/handler_test.go b/connect-inject/handler_test.go index 5e1b8e5f0d..f8061f37c6 100644 --- a/connect-inject/handler_test.go +++ b/connect-inject/handler_test.go @@ -337,6 +337,39 @@ func TestHandlerHandle(t *testing.T) { }, }, }, + + { + "enable resources", + Handler{Resources: true, CPULimit: "1", MemoryLimit: "100Mi", CPURequest: "1", MemoryRequest: "100Mi", Log: hclog.Default().Named("handler")}, + v1beta1.AdmissionRequest{ + Object: encodeRaw(t, &corev1.Pod{ + Spec: basicSpec, + }), + }, + "", + []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/annotations", + }, + { + Operation: "add", + Path: "/spec/volumes", + }, + { + Operation: "add", + Path: "/spec/initContainers", + }, + { + Operation: "add", + Path: "/spec/containers/-", + }, + { + Operation: "add", + Path: "/metadata/annotations/" + escapeJSONPointer(annotationStatus), + }, + }, + }, } for _, tt := range cases { diff --git a/go.sum b/go.sum index 6f48f0c177..c92012c7b5 100644 --- a/go.sum +++ b/go.sum @@ -476,6 +476,7 @@ k8s.io/api v0.0.0-20190325185214-7544f9db76f6/go.mod h1:iuAfoD4hCxJ8Onx9kaTIt30j k8s.io/apimachinery v0.0.0-20180821005732-488889b0007f/go.mod h1:ccL7Eh7zubPUSh9A3USN90/OzHNSVN6zxzde07TDCL0= k8s.io/apimachinery v0.0.0-20190223001710-c182ff3b9841 h1:Q4RZrHNtlC/mSdC1sTrcZ5RchC/9vxLVj57pWiCBKv4= k8s.io/apimachinery v0.0.0-20190223001710-c182ff3b9841/go.mod h1:ccL7Eh7zubPUSh9A3USN90/OzHNSVN6zxzde07TDCL0= +k8s.io/apimachinery v0.0.0-20190620073744-d16981aedf33 h1:Lkd+QNFOB3DqrDyWo796aodJgFJautn/M+t9IGearPc= k8s.io/client-go v8.0.0+incompatible h1:tTI4hRmb1DRMl4fG6Vclfdi6nTM82oIrTT7HfitmxC4= k8s.io/client-go v8.0.0+incompatible/go.mod h1:7vJpHMYJwNQCWgzmNV+VYUl1zCObLyodBc8nIyt8L5s= k8s.io/kube-openapi v0.0.0-20180731170545-e3762e86a74c h1:3KSCztE7gPitlZmWbNwue/2U0YruD65DqX3INopDAQM= diff --git a/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go index 430abb5493..427ad89a37 100644 --- a/subcommand/inject-connect/command.go +++ b/subcommand/inject-connect/command.go @@ -49,6 +49,12 @@ type Command struct { flagDefaultProtocol string // Default protocol for use with central config flagConsulCACert string // [Deprecated] Path to CA Certificate to use when communicating with Consul clients + flagResources bool // True to enable resources for init and sidecar pods + flagCPULimit string // CPU Limits + flagMemoryLimit string // Memory Limits + flagCPURequest string // CPU Requests + flagMemoryRequest string // Memory Limits + // Flags to support namespaces flagEnableNamespaces bool // Use namespacing on all components flagConsulDestinationNamespace string // Consul namespace to register everything if not mirroring @@ -116,6 +122,12 @@ func (c *Command) init() { flags.Merge(c.flagSet, c.http.ClientFlags()) flags.Merge(c.flagSet, c.http.ServerFlags()) + c.flagSet.BoolVar(&c.flagResources, "enable-resources", false, + "Enable to set custom resources for init and sidecar pods") + c.flagSet.StringVar(&c.flagCPULimit, "cpu-limit", "", "CPU Limits for init and sidecar pods") + c.flagSet.StringVar(&c.flagMemoryLimit, "memory-limit", "", "Memory Limits for init and sidecar pods") + c.flagSet.StringVar(&c.flagCPURequest, "cpu-request", "", "CPU Requests for init and sidecar pods") + c.flagSet.StringVar(&c.flagMemoryRequest, "memory-request", "", "Memory Requests for init and sidecar pods") c.help = flags.Usage(help, c.flagSet) } @@ -216,6 +228,10 @@ func (c *Command) Run(args []string) int { WriteServiceDefaults: c.flagWriteServiceDefaults, DefaultProtocol: c.flagDefaultProtocol, ConsulCACert: string(consulCACert), + CPULimit: c.flagCPULimit, + MemoryLimit: c.flagMemoryLimit, + CPURequest: c.flagCPURequest, + MemoryRequest: c.flagMemoryRequest, EnableNamespaces: c.flagEnableNamespaces, AllowK8sNamespacesSet: allowSet, DenyK8sNamespacesSet: denySet, From d0a01a014c894030bad6ef4bc3b20bc70fb28d77 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Tue, 26 May 2020 13:17:48 -0700 Subject: [PATCH 2/2] Refactor resource settings - default resource settings for the envoy sidecar can be set via flags - these can be overridden by annotations on the injected pods - by default there are not resource settings - add resource settings to init container and sidecar container. These are always set so users who require resource settings don't need to turn them on. They are set to more than double observed usage so they won't affect any users. They are not customizable via annotation or flag because they don't change based on use-case. They could be exposed later. --- connect-inject/container_init.go | 38 ++--- connect-inject/container_init_test.go | 33 ++++ connect-inject/envoy_sidecar.go | 88 +++++++++-- connect-inject/envoy_sidecar_test.go | 180 ++++++++++++++++++++++ connect-inject/handler.go | 24 ++- connect-inject/handler_test.go | 33 ---- connect-inject/lifecycle_sidecar.go | 23 ++- connect-inject/lifecycle_sidecar_test.go | 21 +++ subcommand/inject-connect/command.go | 85 +++++++--- subcommand/inject-connect/command_test.go | 30 ++++ 10 files changed, 450 insertions(+), 105 deletions(-) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index b59d5ef2fb..81576bbdf4 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -11,6 +11,13 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) +const ( + initContainerCPULimit = "50m" + initContainerCPURequest = "50m" + initContainerMemoryLimit = "25Mi" + initContainerMemoryRequest = "25Mi" +) + type initContainerCommandData struct { ServiceName string ProxyServiceName string @@ -36,10 +43,6 @@ type initContainerCommandData struct { ConsulCACert string } -type initContainerResources struct { - Resources corev1.ResourceRequirements -} - type initContainerCommandUpstreamData struct { Name string LocalPort int32 @@ -78,20 +81,6 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co panic("No service found. This should be impossible since we default it.") } - icr := initContainerResources{} - if h.Resources { - icr.Resources = corev1.ResourceRequirements{ - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse(h.CPULimit), - corev1.ResourceMemory: resource.MustParse(h.MemoryLimit), - }, - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse(h.CPULimit), - corev1.ResourceMemory: resource.MustParse(h.MemoryLimit), - }, - } - } - // If a port is specified, then we determine the value of that port // and register that port for the host service. if raw, ok := pod.Annotations[annotationPort]; ok && raw != "" { @@ -205,6 +194,17 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co return corev1.Container{}, err } + resources := corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(initContainerCPULimit), + corev1.ResourceMemory: resource.MustParse(initContainerMemoryLimit), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(initContainerCPURequest), + corev1.ResourceMemory: resource.MustParse(initContainerMemoryRequest), + }, + } + return corev1.Container{ Name: "consul-connect-inject-init", Image: h.ImageConsul, @@ -242,7 +242,7 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co Value: fmt.Sprintf("$(POD_NAME)-%s", data.ProxyServiceName), }, }, - Resources: icr.Resources, + Resources: resources, VolumeMounts: volMounts, Command: []string{"/bin/sh", "-ec", buf.String()}, }, nil diff --git a/connect-inject/container_init_test.go b/connect-inject/container_init_test.go index 8b5791b841..38bc919581 100644 --- a/connect-inject/container_init_test.go +++ b/connect-inject/container_init_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -1401,3 +1402,35 @@ EOF`) export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502"`) } + +func TestHandlerContainerInit_Resources(t *testing.T) { + require := require.New(t) + h := Handler{} + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + annotationService: "foo", + }, + }, + + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "web", + }, + }, + }, + } + container, err := h.containerInit(pod, k8sNamespace) + require.NoError(err) + require.Equal(corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(initContainerCPULimit), + corev1.ResourceMemory: resource.MustParse(initContainerMemoryLimit), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(initContainerCPURequest), + corev1.ResourceMemory: resource.MustParse(initContainerMemoryRequest), + }, + }, container.Resources) +} diff --git a/connect-inject/envoy_sidecar.go b/connect-inject/envoy_sidecar.go index b7a76a52f5..6dbf884b74 100644 --- a/connect-inject/envoy_sidecar.go +++ b/connect-inject/envoy_sidecar.go @@ -2,6 +2,7 @@ package connectinject import ( "bytes" + "fmt" "strings" "text/template" @@ -12,7 +13,6 @@ import ( type sidecarContainerCommandData struct { AuthMethod string ConsulNamespace string - Resources corev1.ResourceRequirements } func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Container, error) { @@ -21,19 +21,6 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con ConsulNamespace: h.consulNamespace(k8sNamespace), } - if h.Resources { - templateData.Resources = corev1.ResourceRequirements{ - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse(h.CPULimit), - corev1.ResourceMemory: resource.MustParse(h.MemoryLimit), - }, - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse(h.CPULimit), - corev1.ResourceMemory: resource.MustParse(h.MemoryLimit), - }, - } - } - // Render the command var buf bytes.Buffer tpl := template.Must(template.New("root").Parse(strings.TrimSpace( @@ -43,6 +30,11 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con return corev1.Container{}, err } + resources, err := h.envoySidecarResources(pod) + if err != nil { + return corev1.Container{}, err + } + container := corev1.Container{ Name: "consul-connect-envoy-sidecar", Image: h.ImageEnvoy, @@ -54,7 +46,7 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con }, }, }, - Resources: templateData.Resources, + Resources: resources, VolumeMounts: []corev1.VolumeMount{ { Name: volumeName, @@ -97,6 +89,72 @@ func (h *Handler) envoySidecar(pod *corev1.Pod, k8sNamespace string) (corev1.Con return container, nil } +func (h *Handler) envoySidecarResources(pod *corev1.Pod) (corev1.ResourceRequirements, error) { + resources := corev1.ResourceRequirements{ + Limits: corev1.ResourceList{}, + Requests: corev1.ResourceList{}, + } + // zeroQuantity is used for comparison to see if a quantity was explicitly + // set. + var zeroQuantity resource.Quantity + + // NOTE: We only want to set the limit/request if the default or annotation + // was explicitly set. If it's not explicitly set, it will be the zero value + // which would show up in the pod spec as being explicitly set to zero if we + // set that key, e.g. "cpu" to zero. + // We want it to not show up in the pod spec at all if if it's not explicitly + // set so that users aren't wondering why it's set to 0 when they didn't specify + // a request/limit. If they have explicitly set it to 0 then it will be set + // to 0 in the pod spec because we're doing a comparison to the zero-valued + // struct. + + // CPU Limit. + if anno, ok := pod.Annotations[annotationSidecarProxyCPULimit]; ok { + cpuLimit, err := resource.ParseQuantity(anno) + if err != nil { + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", annotationSidecarProxyCPULimit, anno, err) + } + resources.Limits[corev1.ResourceCPU] = cpuLimit + } else if h.DefaultProxyCPULimit != zeroQuantity { + resources.Limits[corev1.ResourceCPU] = h.DefaultProxyCPULimit + } + + // CPU Request. + if anno, ok := pod.Annotations[annotationSidecarProxyCPURequest]; ok { + cpuRequest, err := resource.ParseQuantity(anno) + if err != nil { + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", annotationSidecarProxyCPURequest, anno, err) + } + resources.Requests[corev1.ResourceCPU] = cpuRequest + } else if h.DefaultProxyCPURequest != zeroQuantity { + resources.Requests[corev1.ResourceCPU] = h.DefaultProxyCPURequest + } + + // Memory Limit. + if anno, ok := pod.Annotations[annotationSidecarProxyMemoryLimit]; ok { + memoryLimit, err := resource.ParseQuantity(anno) + if err != nil { + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", annotationSidecarProxyMemoryLimit, anno, err) + } + resources.Limits[corev1.ResourceMemory] = memoryLimit + } else if h.DefaultProxyMemoryLimit != zeroQuantity { + resources.Limits[corev1.ResourceMemory] = h.DefaultProxyMemoryLimit + } + + // Memory Request. + if anno, ok := pod.Annotations[annotationSidecarProxyMemoryRequest]; ok { + memoryRequest, err := resource.ParseQuantity(anno) + if err != nil { + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", annotationSidecarProxyMemoryRequest, anno, err) + } + resources.Requests[corev1.ResourceMemory] = memoryRequest + } else if h.DefaultProxyMemoryRequest != zeroQuantity { + resources.Requests[corev1.ResourceMemory] = h.DefaultProxyMemoryRequest + } + + return resources, nil +} + const sidecarPreStopCommandTpl = ` /consul/connect-inject/consul services deregister \ {{- if .AuthMethod }} diff --git a/connect-inject/envoy_sidecar_test.go b/connect-inject/envoy_sidecar_test.go index e16147302d..1e783a0885 100644 --- a/connect-inject/envoy_sidecar_test.go +++ b/connect-inject/envoy_sidecar_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -203,3 +204,182 @@ func TestHandlerEnvoySidecar_NamespacesAndAuthMethod(t *testing.T) { && /consul/connect-inject/consul logout \ -token-file="/consul/connect-inject/acl-token"`) } + +func TestHandlerEnvoySidecar_Resources(t *testing.T) { + mem1 := resource.MustParse("100Mi") + mem2 := resource.MustParse("200Mi") + cpu1 := resource.MustParse("100m") + cpu2 := resource.MustParse("200m") + zero := resource.MustParse("0") + + cases := map[string]struct { + handler Handler + annotations map[string]string + expResources corev1.ResourceRequirements + expErr string + }{ + "no defaults, no annotations": { + handler: Handler{}, + annotations: nil, + expResources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{}, + Requests: corev1.ResourceList{}, + }, + }, + "all defaults, no annotations": { + handler: Handler{ + DefaultProxyCPURequest: cpu1, + DefaultProxyCPULimit: cpu2, + DefaultProxyMemoryRequest: mem1, + DefaultProxyMemoryLimit: mem2, + }, + annotations: nil, + expResources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: cpu2, + corev1.ResourceMemory: mem2, + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: cpu1, + corev1.ResourceMemory: mem1, + }, + }, + }, + "no defaults, all annotations": { + handler: Handler{}, + annotations: map[string]string{ + annotationSidecarProxyCPURequest: "100m", + annotationSidecarProxyMemoryRequest: "100Mi", + annotationSidecarProxyCPULimit: "200m", + annotationSidecarProxyMemoryLimit: "200Mi", + }, + expResources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: cpu2, + corev1.ResourceMemory: mem2, + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: cpu1, + corev1.ResourceMemory: mem1, + }, + }, + }, + "annotations override defaults": { + handler: Handler{ + DefaultProxyCPURequest: zero, + DefaultProxyCPULimit: zero, + DefaultProxyMemoryRequest: zero, + DefaultProxyMemoryLimit: zero, + }, + annotations: map[string]string{ + annotationSidecarProxyCPURequest: "100m", + annotationSidecarProxyMemoryRequest: "100Mi", + annotationSidecarProxyCPULimit: "200m", + annotationSidecarProxyMemoryLimit: "200Mi", + }, + expResources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: cpu2, + corev1.ResourceMemory: mem2, + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: cpu1, + corev1.ResourceMemory: mem1, + }, + }, + }, + "defaults set to zero, no annotations": { + handler: Handler{ + DefaultProxyCPURequest: zero, + DefaultProxyCPULimit: zero, + DefaultProxyMemoryRequest: zero, + DefaultProxyMemoryLimit: zero, + }, + annotations: nil, + expResources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: zero, + corev1.ResourceMemory: zero, + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: zero, + corev1.ResourceMemory: zero, + }, + }, + }, + "annotations set to 0": { + handler: Handler{}, + annotations: map[string]string{ + annotationSidecarProxyCPURequest: "0", + annotationSidecarProxyMemoryRequest: "0", + annotationSidecarProxyCPULimit: "0", + annotationSidecarProxyMemoryLimit: "0", + }, + expResources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: zero, + corev1.ResourceMemory: zero, + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: zero, + corev1.ResourceMemory: zero, + }, + }, + }, + "invalid cpu request": { + handler: Handler{}, + annotations: map[string]string{ + annotationSidecarProxyCPURequest: "invalid", + }, + expErr: "parsing annotation consul.hashicorp.com/sidecar-proxy-cpu-request:\"invalid\": quantities must match the regular expression", + }, + "invalid cpu limit": { + handler: Handler{}, + annotations: map[string]string{ + annotationSidecarProxyCPULimit: "invalid", + }, + expErr: "parsing annotation consul.hashicorp.com/sidecar-proxy-cpu-limit:\"invalid\": quantities must match the regular expression", + }, + "invalid memory request": { + handler: Handler{}, + annotations: map[string]string{ + annotationSidecarProxyMemoryRequest: "invalid", + }, + expErr: "parsing annotation consul.hashicorp.com/sidecar-proxy-memory-request:\"invalid\": quantities must match the regular expression", + }, + "invalid memory limit": { + handler: Handler{}, + annotations: map[string]string{ + annotationSidecarProxyMemoryLimit: "invalid", + }, + expErr: "parsing annotation consul.hashicorp.com/sidecar-proxy-memory-limit:\"invalid\": quantities must match the regular expression", + }, + } + + for name, c := range cases { + t.Run(name, func(tt *testing.T) { + require := require.New(tt) + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: c.annotations, + }, + + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "web", + }, + }, + }, + } + container, err := c.handler.envoySidecar(pod, k8sNamespace) + if c.expErr != "" { + require.NotNil(err) + require.Contains(err.Error(), c.expErr) + } else { + require.NoError(err) + require.Equal(c.expResources, container.Resources) + } + }) + } +} diff --git a/connect-inject/handler.go b/connect-inject/handler.go index 845c002735..01fe54cc65 100644 --- a/connect-inject/handler.go +++ b/connect-inject/handler.go @@ -14,6 +14,7 @@ import ( "github.com/mattbaird/jsonpatch" "k8s.io/api/admission/v1beta1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -76,6 +77,11 @@ const ( // consul-k8s lifecycle-sidecar command. This flag controls how often the // service is synced (i.e. re-registered) with the local agent. annotationSyncPeriod = "consul.hashicorp.com/connect-sync-period" + + annotationSidecarProxyCPULimit = "consul.hashicorp.com/sidecar-proxy-cpu-limit" + annotationSidecarProxyCPURequest = "consul.hashicorp.com/sidecar-proxy-cpu-request" + annotationSidecarProxyMemoryLimit = "consul.hashicorp.com/sidecar-proxy-memory-limit" + annotationSidecarProxyMemoryRequest = "consul.hashicorp.com/sidecar-proxy-memory-request" ) var ( @@ -163,19 +169,11 @@ type Handler struct { // Only necessary if ACLs are enabled. CrossNamespaceACLPolicy string - // Resources checks if cpu and memory resources for sidecar pods and - // init containers should be set. If this is false, no resources - // will be set. - Resources bool - // CPULimit sets cpu limit for pods - CPULimit string - // MemoryLimit sets memory limit for pods - MemoryLimit string - // CPURequest sets cpu requests for pods - CPURequest string - // MemoryRequest sets memory requests for pods - MemoryRequest string - + // Default resource settings for sidecar proxies. + DefaultProxyCPURequest resource.Quantity + DefaultProxyCPULimit resource.Quantity + DefaultProxyMemoryRequest resource.Quantity + DefaultProxyMemoryLimit resource.Quantity // Log Log hclog.Logger diff --git a/connect-inject/handler_test.go b/connect-inject/handler_test.go index f8061f37c6..5e1b8e5f0d 100644 --- a/connect-inject/handler_test.go +++ b/connect-inject/handler_test.go @@ -337,39 +337,6 @@ func TestHandlerHandle(t *testing.T) { }, }, }, - - { - "enable resources", - Handler{Resources: true, CPULimit: "1", MemoryLimit: "100Mi", CPURequest: "1", MemoryRequest: "100Mi", Log: hclog.Default().Named("handler")}, - v1beta1.AdmissionRequest{ - Object: encodeRaw(t, &corev1.Pod{ - Spec: basicSpec, - }), - }, - "", - []jsonpatch.JsonPatchOperation{ - { - Operation: "add", - Path: "/metadata/annotations", - }, - { - Operation: "add", - Path: "/spec/volumes", - }, - { - Operation: "add", - Path: "/spec/initContainers", - }, - { - Operation: "add", - Path: "/spec/containers/-", - }, - { - Operation: "add", - Path: "/metadata/annotations/" + escapeJSONPointer(annotationStatus), - }, - }, - }, } for _, tt := range cases { diff --git a/connect-inject/lifecycle_sidecar.go b/connect-inject/lifecycle_sidecar.go index 9fdc1a19ff..f548e8b2a9 100644 --- a/connect-inject/lifecycle_sidecar.go +++ b/connect-inject/lifecycle_sidecar.go @@ -2,9 +2,18 @@ package connectinject import ( corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + "strings" ) +const ( + lifecycleContainerCPULimit = "10m" + lifecycleContainerCPURequest = "10m" + lifecycleContainerMemoryLimit = "25Mi" + lifecycleContainerMemoryRequest = "25Mi" +) + func (h *Handler) lifecycleSidecar(pod *corev1.Pod) corev1.Container { command := []string{ "consul-k8s", @@ -52,6 +61,17 @@ func (h *Handler) lifecycleSidecar(pod *corev1.Pod) corev1.Container { }) } + resources := corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(lifecycleContainerCPULimit), + corev1.ResourceMemory: resource.MustParse(lifecycleContainerMemoryLimit), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(lifecycleContainerCPURequest), + corev1.ResourceMemory: resource.MustParse(lifecycleContainerMemoryRequest), + }, + } + return corev1.Container{ Name: "consul-connect-lifecycle-sidecar", Image: h.ImageConsulK8S, @@ -62,6 +82,7 @@ func (h *Handler) lifecycleSidecar(pod *corev1.Pod) corev1.Container { MountPath: "/consul/connect-inject", }, }, - Command: command, + Command: command, + Resources: resources, } } diff --git a/connect-inject/lifecycle_sidecar_test.go b/connect-inject/lifecycle_sidecar_test.go index 8bfd621f81..a7a8162d62 100644 --- a/connect-inject/lifecycle_sidecar_test.go +++ b/connect-inject/lifecycle_sidecar_test.go @@ -4,6 +4,7 @@ import ( "github.com/hashicorp/go-hclog" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "testing" ) @@ -53,6 +54,16 @@ func TestLifecycleSidecar_Default(t *testing.T) { "-service-config", "/consul/connect-inject/service.hcl", "-consul-binary", "/consul/connect-inject/consul", }, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(lifecycleContainerCPULimit), + corev1.ResourceMemory: resource.MustParse(lifecycleContainerMemoryLimit), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(lifecycleContainerCPURequest), + corev1.ResourceMemory: resource.MustParse(lifecycleContainerMemoryRequest), + }, + }, }, container) } @@ -160,5 +171,15 @@ func TestLifecycleSidecar_TLS(t *testing.T) { "-service-config", "/consul/connect-inject/service.hcl", "-consul-binary", "/consul/connect-inject/consul", }, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(lifecycleContainerCPULimit), + corev1.ResourceMemory: resource.MustParse(lifecycleContainerMemoryLimit), + }, + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(lifecycleContainerCPURequest), + corev1.ResourceMemory: resource.MustParse(lifecycleContainerMemoryRequest), + }, + }, }, container) } diff --git a/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go index 427ad89a37..b2621e1637 100644 --- a/subcommand/inject-connect/command.go +++ b/subcommand/inject-connect/command.go @@ -20,18 +20,12 @@ import ( "github.com/hashicorp/consul/command/flags" "github.com/hashicorp/go-hclog" "github.com/mitchellh/cli" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" ) -type arrayFlags []string - -func (i *arrayFlags) Set(value string) error { - *i = append(*i, value) - return nil -} - type Command struct { UI cli.Ui @@ -49,12 +43,6 @@ type Command struct { flagDefaultProtocol string // Default protocol for use with central config flagConsulCACert string // [Deprecated] Path to CA Certificate to use when communicating with Consul clients - flagResources bool // True to enable resources for init and sidecar pods - flagCPULimit string // CPU Limits - flagMemoryLimit string // Memory Limits - flagCPURequest string // CPU Requests - flagMemoryRequest string // Memory Limits - // Flags to support namespaces flagEnableNamespaces bool // Use namespacing on all components flagConsulDestinationNamespace string // Consul namespace to register everything if not mirroring @@ -64,6 +52,12 @@ type Command struct { flagK8SNSMirroringPrefix string // Prefix added to Consul namespaces created when mirroring flagCrossNamespaceACLPolicy string // The name of the ACL policy to add to every created namespace if ACLs are enabled + // Proxy resource settings. + flagDefaultSidecarProxyCPULimit string + flagDefaultSidecarProxyCPURequest string + flagDefaultSidecarProxyMemoryLimit string + flagDefaultSidecarProxyMemoryRequest string + flagSet *flag.FlagSet http *flags.HTTPFlags @@ -118,16 +112,15 @@ func (c *Command) init() { "[Enterprise Only] Name of the ACL policy to attach to all created Consul namespaces to allow service "+ "discovery across Consul namespaces. Only necessary if ACLs are enabled.") + // Resource setting flags. + c.flagSet.StringVar(&c.flagDefaultSidecarProxyCPULimit, "default-sidecar-proxy-cpu-limit", "", "Default sidecar proxy CPU limit.") + c.flagSet.StringVar(&c.flagDefaultSidecarProxyCPURequest, "default-sidecar-proxy-cpu-request", "", "Default sidecar proxy CPU request.") + c.flagSet.StringVar(&c.flagDefaultSidecarProxyMemoryLimit, "default-sidecar-proxy-memory-limit", "", "Default sidecar proxy memory limit.") + c.flagSet.StringVar(&c.flagDefaultSidecarProxyMemoryRequest, "default-sidecar-proxy-memory-request", "", "Default sidecar proxy memory request.") + c.http = &flags.HTTPFlags{} flags.Merge(c.flagSet, c.http.ClientFlags()) flags.Merge(c.flagSet, c.http.ServerFlags()) - - c.flagSet.BoolVar(&c.flagResources, "enable-resources", false, - "Enable to set custom resources for init and sidecar pods") - c.flagSet.StringVar(&c.flagCPULimit, "cpu-limit", "", "CPU Limits for init and sidecar pods") - c.flagSet.StringVar(&c.flagMemoryLimit, "memory-limit", "", "Memory Limits for init and sidecar pods") - c.flagSet.StringVar(&c.flagCPURequest, "cpu-request", "", "CPU Requests for init and sidecar pods") - c.flagSet.StringVar(&c.flagMemoryRequest, "memory-request", "", "Memory Requests for init and sidecar pods") c.help = flags.Usage(help, c.flagSet) } @@ -143,6 +136,50 @@ func (c *Command) Run(args []string) int { return 1 } + var cpuLimit, cpuRequest, memoryLimit, memoryRequest resource.Quantity + var err error + if c.flagDefaultSidecarProxyCPULimit != "" { + cpuLimit, err = resource.ParseQuantity(c.flagDefaultSidecarProxyCPULimit) + if err != nil { + c.UI.Error(fmt.Sprintf("-default-sidecar-proxy-cpu-limit is invalid: %s", err)) + return 1 + } + } + if c.flagDefaultSidecarProxyCPURequest != "" { + cpuRequest, err = resource.ParseQuantity(c.flagDefaultSidecarProxyCPURequest) + if err != nil { + c.UI.Error(fmt.Sprintf("-default-sidecar-proxy-cpu-request is invalid: %s", err)) + return 1 + } + } + if cpuLimit.Value() != 0 && cpuRequest.Cmp(cpuLimit) > 0 { + c.UI.Error(fmt.Sprintf( + "request must be <= limit: -default-sidecar-proxy-cpu-request value of %q is greater than the -default-sidecar-proxy-cpu-limit value of %q", + c.flagDefaultSidecarProxyCPURequest, c.flagDefaultSidecarProxyCPULimit)) + return 1 + } + + if c.flagDefaultSidecarProxyMemoryLimit != "" { + memoryLimit, err = resource.ParseQuantity(c.flagDefaultSidecarProxyMemoryLimit) + if err != nil { + c.UI.Error(fmt.Sprintf("-default-sidecar-proxy-memory-limit is invalid: %s", err)) + return 1 + } + } + if c.flagDefaultSidecarProxyMemoryRequest != "" { + memoryRequest, err = resource.ParseQuantity(c.flagDefaultSidecarProxyMemoryRequest) + if err != nil { + c.UI.Error(fmt.Sprintf("-default-sidecar-proxy-memory-request is invalid: %s", err)) + return 1 + } + } + if memoryLimit.Value() != 0 && memoryRequest.Cmp(memoryLimit) > 0 { + c.UI.Error(fmt.Sprintf( + "request must be <= limit: -default-sidecar-proxy-memory-request value of %q is greater than the -default-sidecar-proxy-memory-limit value of %q", + c.flagDefaultSidecarProxyMemoryRequest, c.flagDefaultSidecarProxyMemoryLimit)) + return 1 + } + // We must have an in-cluster K8S client if c.clientset == nil { config, err := rest.InClusterConfig() @@ -228,10 +265,10 @@ func (c *Command) Run(args []string) int { WriteServiceDefaults: c.flagWriteServiceDefaults, DefaultProtocol: c.flagDefaultProtocol, ConsulCACert: string(consulCACert), - CPULimit: c.flagCPULimit, - MemoryLimit: c.flagMemoryLimit, - CPURequest: c.flagCPURequest, - MemoryRequest: c.flagMemoryRequest, + DefaultProxyCPULimit: cpuLimit, + DefaultProxyCPURequest: cpuRequest, + DefaultProxyMemoryLimit: memoryLimit, + DefaultProxyMemoryRequest: memoryRequest, EnableNamespaces: c.flagEnableNamespaces, AllowK8sNamespacesSet: allowSet, DenyK8sNamespacesSet: denySet, diff --git a/subcommand/inject-connect/command_test.go b/subcommand/inject-connect/command_test.go index 0b3eca0b30..9c763f37d0 100644 --- a/subcommand/inject-connect/command_test.go +++ b/subcommand/inject-connect/command_test.go @@ -22,6 +22,36 @@ func TestRun_FlagValidation(t *testing.T) { flags: []string{"-consul-k8s-image", "foo", "-ca-file", "bar"}, expErr: "Error reading Consul's CA cert file \"bar\"", }, + { + flags: []string{"-consul-k8s-image", "foo", "-default-sidecar-proxy-cpu-limit=unparseable"}, + expErr: "-default-sidecar-proxy-cpu-limit is invalid", + }, + { + flags: []string{"-consul-k8s-image", "foo", "-default-sidecar-proxy-cpu-request=unparseable"}, + expErr: "-default-sidecar-proxy-cpu-request is invalid", + }, + { + flags: []string{"-consul-k8s-image", "foo", "-default-sidecar-proxy-memory-limit=unparseable"}, + expErr: "-default-sidecar-proxy-memory-limit is invalid", + }, + { + flags: []string{"-consul-k8s-image", "foo", "-default-sidecar-proxy-memory-request=unparseable"}, + expErr: "-default-sidecar-proxy-memory-request is invalid", + }, + { + flags: []string{"-consul-k8s-image", "foo", + "-default-sidecar-proxy-memory-request=50Mi", + "-default-sidecar-proxy-memory-limit=25Mi", + }, + expErr: "request must be <= limit: -default-sidecar-proxy-memory-request value of \"50Mi\" is greater than the -default-sidecar-proxy-memory-limit value of \"25Mi\"", + }, + { + flags: []string{"-consul-k8s-image", "foo", + "-default-sidecar-proxy-cpu-request=50m", + "-default-sidecar-proxy-cpu-limit=25m", + }, + expErr: "request must be <= limit: -default-sidecar-proxy-cpu-request value of \"50m\" is greater than the -default-sidecar-proxy-cpu-limit value of \"25m\"", + }, } for _, c := range cases {