From 1a8e24c1513a94a27b94f4d43053387253d021fa Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Mon, 1 Jun 2020 16:27:45 -0700 Subject: [PATCH] Code review updates --- connect-inject/envoy_sidecar.go | 8 ++++---- connect-inject/envoy_sidecar_test.go | 23 +++++++++-------------- subcommand/inject-connect/command.go | 21 +++++++++++++++++---- subcommand/inject-connect/command_test.go | 14 ++++++++++++++ 4 files changed, 44 insertions(+), 22 deletions(-) diff --git a/connect-inject/envoy_sidecar.go b/connect-inject/envoy_sidecar.go index 2f9356793b..6dbf884b74 100644 --- a/connect-inject/envoy_sidecar.go +++ b/connect-inject/envoy_sidecar.go @@ -112,7 +112,7 @@ func (h *Handler) envoySidecarResources(pod *corev1.Pod) (corev1.ResourceRequire if anno, ok := pod.Annotations[annotationSidecarProxyCPULimit]; ok { cpuLimit, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf("parsing %s annotation: %s", annotationSidecarProxyCPULimit, err) + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", annotationSidecarProxyCPULimit, anno, err) } resources.Limits[corev1.ResourceCPU] = cpuLimit } else if h.DefaultProxyCPULimit != zeroQuantity { @@ -123,7 +123,7 @@ func (h *Handler) envoySidecarResources(pod *corev1.Pod) (corev1.ResourceRequire if anno, ok := pod.Annotations[annotationSidecarProxyCPURequest]; ok { cpuRequest, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf("parsing %s annotation: %s", annotationSidecarProxyCPURequest, err) + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", annotationSidecarProxyCPURequest, anno, err) } resources.Requests[corev1.ResourceCPU] = cpuRequest } else if h.DefaultProxyCPURequest != zeroQuantity { @@ -134,7 +134,7 @@ func (h *Handler) envoySidecarResources(pod *corev1.Pod) (corev1.ResourceRequire if anno, ok := pod.Annotations[annotationSidecarProxyMemoryLimit]; ok { memoryLimit, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf("parsing %s annotation: %s", annotationSidecarProxyMemoryLimit, err) + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", annotationSidecarProxyMemoryLimit, anno, err) } resources.Limits[corev1.ResourceMemory] = memoryLimit } else if h.DefaultProxyMemoryLimit != zeroQuantity { @@ -145,7 +145,7 @@ func (h *Handler) envoySidecarResources(pod *corev1.Pod) (corev1.ResourceRequire if anno, ok := pod.Annotations[annotationSidecarProxyMemoryRequest]; ok { memoryRequest, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf("parsing %s annotation: %s", annotationSidecarProxyMemoryRequest, err) + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", annotationSidecarProxyMemoryRequest, anno, err) } resources.Requests[corev1.ResourceMemory] = memoryRequest } else if h.DefaultProxyMemoryRequest != zeroQuantity { diff --git a/connect-inject/envoy_sidecar_test.go b/connect-inject/envoy_sidecar_test.go index 453eeb9a2a..1e783a0885 100644 --- a/connect-inject/envoy_sidecar_test.go +++ b/connect-inject/envoy_sidecar_test.go @@ -206,16 +206,11 @@ func TestHandlerEnvoySidecar_NamespacesAndAuthMethod(t *testing.T) { } func TestHandlerEnvoySidecar_Resources(t *testing.T) { - mem1, err := resource.ParseQuantity("100Mi") - require.NoError(t, err) - mem2, err := resource.ParseQuantity("200Mi") - require.NoError(t, err) - cpu1, err := resource.ParseQuantity("100m") - require.NoError(t, err) - cpu2, err := resource.ParseQuantity("200m") - require.NoError(t, err) - zero, err := resource.ParseQuantity("0") - require.NoError(t, err) + 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 @@ -336,28 +331,28 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { annotations: map[string]string{ annotationSidecarProxyCPURequest: "invalid", }, - expErr: "parsing consul.hashicorp.com/sidecar-proxy-cpu-request annotation: quantities must match the regular expression", + 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 consul.hashicorp.com/sidecar-proxy-cpu-limit annotation: quantities must match the regular expression", + 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 consul.hashicorp.com/sidecar-proxy-memory-request annotation: quantities must match the regular expression", + 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 consul.hashicorp.com/sidecar-proxy-memory-limit annotation: quantities must match the regular expression", + expErr: "parsing annotation consul.hashicorp.com/sidecar-proxy-memory-limit:\"invalid\": quantities must match the regular expression", }, } diff --git a/subcommand/inject-connect/command.go b/subcommand/inject-connect/command.go index 49cec8933a..b2621e1637 100644 --- a/subcommand/inject-connect/command.go +++ b/subcommand/inject-connect/command.go @@ -113,10 +113,10 @@ func (c *Command) init() { "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.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()) @@ -152,6 +152,13 @@ func (c *Command) Run(args []string) int { 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 { @@ -166,6 +173,12 @@ func (c *Command) Run(args []string) int { 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 { diff --git a/subcommand/inject-connect/command_test.go b/subcommand/inject-connect/command_test.go index 64d4b9dfab..9c763f37d0 100644 --- a/subcommand/inject-connect/command_test.go +++ b/subcommand/inject-connect/command_test.go @@ -38,6 +38,20 @@ func TestRun_FlagValidation(t *testing.T) { 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 {