Skip to content

Commit

Permalink
Code review updates
Browse files Browse the repository at this point in the history
  • Loading branch information
@lkysow
lkysow committed Jun 1, 2020
1 parent a829da0 commit 1a8e24c
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 22 deletions.
8 changes: 4 additions & 4 deletions connect-inject/envoy_sidecar.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
23 changes: 9 additions & 14 deletions connect-inject/envoy_sidecar_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
},
}

Expand Down
21 changes: 17 additions & 4 deletions subcommand/inject-connect/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
14 changes: 14 additions & 0 deletions subcommand/inject-connect/command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 1a8e24c

Please sign in to comment.