From 04812a945594898b2369d7885208e922edfd0b3a Mon Sep 17 00:00:00 2001 From: Eugene Glotov Date: Tue, 17 Dec 2019 11:18:00 +0200 Subject: [PATCH] Address PR review comments (third pass): return `-seconds` suffix Signed-off-by: Eugene Glotov --- charts/linkerd2/README.md | 2 +- charts/linkerd2/values.yaml | 2 +- charts/partials/templates/_proxy.tpl | 4 +- cli/cmd/inject.go | 16 ++-- cli/cmd/install.go | 2 +- cli/cmd/root.go | 2 +- pkg/charts/linkerd2/values.go | 2 +- pkg/inject/inject.go | 29 +++--- pkg/inject/inject_test.go | 131 ++++++++++++++------------- pkg/k8s/labels.go | 2 +- 10 files changed, 97 insertions(+), 95 deletions(-) diff --git a/charts/linkerd2/README.md b/charts/linkerd2/README.md index e109660c8ea84..3d5e4675a5f19 100644 --- a/charts/linkerd2/README.md +++ b/charts/linkerd2/README.md @@ -126,7 +126,7 @@ The following table lists the configurable parameters of the Linkerd2 chart and | `proxy.trace.collectorSvcAccount` | Service account associated with the Trace collector instance || | `proxy.trace.collectorSvcAddr` | Collector Service address for the proxies to send Trace Data || | `proxy.uid` | User id under which the proxy runs | `2102` | -| `Proxy.WaitBeforeExitSeconds` | The proxy sidecar will stay alive for at least the given period before receiving SIGTERM signal from Kubernetes but no longer than pod's `terminationGracePeriodSeconds`. | `0` | +| `proxy.waitBeforeExitSeconds` | The proxy sidecar will stay alive for at least the given period before receiving SIGTERM signal from Kubernetes but no longer than pod's `terminationGracePeriodSeconds`. | `0` | | `proxyInit.ignoreInboundPorts` | Inbound ports the proxy should ignore || | `proxyInit.ignoreOutboundPorts` | Outbound ports the proxy should ignore || | `proxyInit.image.name` | Docker image for the proxy-init container | `gcr.io/linkerd-io/proxy-init` | diff --git a/charts/linkerd2/values.yaml b/charts/linkerd2/values.yaml index 06cdedf2004ce..b941f9babac89 100644 --- a/charts/linkerd2/values.yaml +++ b/charts/linkerd2/values.yaml @@ -91,7 +91,7 @@ proxy: # and wait for this duration before letting the proxy process the SIGTERM signal. # See https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/#container-hooks # for more info on container lifecycle hooks. - WaitBeforeExitSeconds: 0 + waitBeforeExitSeconds: 0 # proxy-init configuration proxyInit: diff --git a/charts/partials/templates/_proxy.tpl b/charts/partials/templates/_proxy.tpl index 778fbe58876a5..3d4e51cf53fd0 100644 --- a/charts/partials/templates/_proxy.tpl +++ b/charts/partials/templates/_proxy.tpl @@ -110,14 +110,14 @@ securityContext: readOnlyRootFilesystem: true runAsUser: {{.Values.proxy.uid}} terminationMessagePolicy: FallbackToLogsOnError -{{- if .Values.Proxy.WaitBeforeExitSeconds }} +{{- if .Values.proxy.waitBeforeExitSeconds }} lifecycle: preStop: exec: command: - /bin/bash - -c - - sleep {{.Values.Proxy.WaitBeforeExitSeconds}} + - sleep {{.Values.proxy.waitBeforeExitSeconds}} {{- end }} {{- if or (not .Values.proxy.disableIdentity) (.Values.proxy.saMountPath) }} volumeMounts: diff --git a/cli/cmd/inject.go b/cli/cmd/inject.go index 05c5bc120286f..904b4f8e28b22 100644 --- a/cli/cmd/inject.go +++ b/cli/cmd/inject.go @@ -10,8 +10,6 @@ import ( "strconv" "strings" - "github.com/docker/docker/api/types/time" - jsonpatch "github.com/evanphx/json-patch" cfg "github.com/linkerd/linkerd2/controller/gen/config" "github.com/linkerd/linkerd2/controller/gen/public" @@ -102,10 +100,10 @@ sub-folders, or coming from stdin.`, &manualOption, "manual", manualOption, "Include the proxy sidecar container spec in the YAML output (the auto-injector won't pick it up, so config annotations aren't supported) (default false)", ) - flags.DurationVar( - &options.waitBeforeExit, "wait-before-exit", options.waitBeforeExit, + flags.UintVar( + &options.waitBeforeExitSeconds, "wait-before-exit-seconds", options.waitBeforeExitSeconds, "The period during which the proxy sidecar must stay alive while its pod is terminating. "+ - "Must be smaller than terminationGracePeriodSeconds for the pod (default 0). E.g.: 2m, 10s, 2m3s", + "Must be smaller than terminationGracePeriodSeconds for the pod (default 0)", ) flags.BoolVar( &options.disableIdentity, "disable-identity", options.disableIdentity, @@ -446,11 +444,15 @@ func (options *proxyConfigOptions) overrideConfigs(configs *cfg.All, overrideAnn if options.traceCollectorSvcAccount != "" { overrideAnnotations[k8s.ProxyTraceCollectorSvcAccountAnnotation] = options.traceCollectorSvcAccount } - if options.waitBeforeExit != 0 { - overrideAnnotations[k8s.ProxyWaitBeforeExitAnnotation] = time.DurationToSecondsString(options.waitBeforeExit) + "s" + if options.waitBeforeExitSeconds != 0 { + overrideAnnotations[k8s.ProxyWaitBeforeExitAnnotation] = uintToString(options.waitBeforeExitSeconds) } } +func uintToString(v uint) string { + return strconv.FormatUint(uint64(v), 10) +} + func toPort(p uint) *cfg.Port { return &cfg.Port{Port: uint32(p)} } diff --git a/cli/cmd/install.go b/cli/cmd/install.go index ca549398394cf..e08d3da02e48a 100644 --- a/cli/cmd/install.go +++ b/cli/cmd/install.go @@ -198,7 +198,7 @@ func newInstallOptionsWithDefaults() (*installOptions, error) { proxyCPULimit: defaults.Proxy.Resources.CPU.Limit, proxyMemoryLimit: defaults.Proxy.Resources.Memory.Limit, enableExternalProfiles: defaults.Proxy.EnableExternalProfiles, - waitBeforeExit: time.Duration(defaults.Proxy.WaitBeforeExitSeconds) * time.Second, + waitBeforeExitSeconds: defaults.Proxy.WaitBeforeExitSeconds, }, identityOptions: &installIdentityOptions{ trustDomain: defaults.Identity.TrustDomain, diff --git a/cli/cmd/root.go b/cli/cmd/root.go index 72fc765534408..9169216251699 100644 --- a/cli/cmd/root.go +++ b/cli/cmd/root.go @@ -199,7 +199,7 @@ type proxyConfigOptions struct { enableExternalProfiles bool traceCollector string traceCollectorSvcAccount string - waitBeforeExit time.Duration + waitBeforeExitSeconds uint // ignoreCluster is not validated by validate(). ignoreCluster bool disableIdentity bool diff --git a/pkg/charts/linkerd2/values.go b/pkg/charts/linkerd2/values.go index 00db144d065f4..8281f48db2bb2 100644 --- a/pkg/charts/linkerd2/values.go +++ b/pkg/charts/linkerd2/values.go @@ -95,7 +95,7 @@ type ( Resources *Resources `json:"resources"` Trace *Trace `json:"trace"` UID int64 `json:"uid"` - WaitBeforeExitSeconds int32 `json:"waitBeforeExitSeconds"` + WaitBeforeExitSeconds uint `json:"waitBeforeExitSeconds"` } // ProxyInit contains the fields to set the proxy-init container diff --git a/pkg/inject/inject.go b/pkg/inject/inject.go index 01321a749c520..a03a5591b15ed 100644 --- a/pkg/inject/inject.go +++ b/pkg/inject/inject.go @@ -4,12 +4,6 @@ import ( "encoding/json" "errors" "fmt" - "regexp" - "sort" - "strconv" - "strings" - "time" - "github.com/linkerd/linkerd2/controller/gen/config" "github.com/linkerd/linkerd2/pkg/charts" l5dcharts "github.com/linkerd/linkerd2/pkg/charts/linkerd2" @@ -24,7 +18,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/helm/pkg/chartutil" + "regexp" "sigs.k8s.io/yaml" + "sort" + "strconv" + "strings" ) const ( @@ -452,6 +450,7 @@ func (conf *ResourceConfig) complete(template *corev1.PodTemplateSpec) { // injectPodSpec adds linkerd sidecars to the provided PodSpec. func (conf *ResourceConfig) injectPodSpec(values *patch) { + waitBeforeExitSeconds, _ := conf.proxyWaitBeforeExitSeconds() values.Proxy = &l5dcharts.Proxy{ Component: conf.pod.labels[k8s.ProxyDeploymentLabel], EnableExternalProfiles: conf.enableExternalProfiles(), @@ -470,7 +469,7 @@ func (conf *ResourceConfig) injectPodSpec(values *patch) { }, UID: conf.proxyUID(), Resources: conf.proxyResourceRequirements(), - WaitBeforeExitSeconds: int32(conf.proxyWaitBeforeExit() / time.Second), + WaitBeforeExitSeconds: waitBeforeExitSeconds, } if v := conf.pod.meta.Annotations[k8s.ProxyEnableDebugAnnotation]; v != "" { @@ -759,20 +758,18 @@ func (conf *ResourceConfig) tapDisabled() bool { return false } -func (conf *ResourceConfig) proxyWaitBeforeExit() time.Duration { +func (conf *ResourceConfig) proxyWaitBeforeExitSeconds() (uint, error) { if override := conf.getOverride(k8s.ProxyWaitBeforeExitAnnotation); override != "" { - waitBeforeExit, err := time.ParseDuration(override) - - // Try to parse plain integer as value in seconds if it is not formatted as Duration - if err != nil { - waitInSeconds, _ := strconv.ParseUint(override, 10, 32) - waitBeforeExit = time.Duration(waitInSeconds) * time.Second + waitBeforeExitSeconds, err := strconv.ParseUint(override, 10, 32) + if nil != err { + log.Warnf("unrecognized value used for the %s annotation, unsigned int is expected: %s", + k8s.ProxyWaitBeforeExitAnnotation, override) } - return waitBeforeExit + return uint(waitBeforeExitSeconds), err } - return 0 + return 0, nil } func (conf *ResourceConfig) proxyResourceRequirements() *l5dcharts.Resources { diff --git a/pkg/inject/inject_test.go b/pkg/inject/inject_test.go index d7bb164e7cf6b..9cb0b98bdea76 100644 --- a/pkg/inject/inject_test.go +++ b/pkg/inject/inject_test.go @@ -3,7 +3,6 @@ package inject import ( "reflect" "testing" - "time" "github.com/linkerd/linkerd2/controller/gen/config" l5dcharts "github.com/linkerd/linkerd2/pkg/charts/linkerd2" @@ -16,24 +15,24 @@ import ( ) type expectedProxyConfigs struct { - identityContext *config.IdentityContext - image string - imagePullPolicy string - proxyVersion string - controlPort int32 - inboundPort int32 - adminPort int32 - outboundPort int32 - proxyWaitBeforeExit time.Duration - logLevel string - resourceRequirements *l5dcharts.Resources - proxyUID int64 - initImage string - initImagePullPolicy string - initVersion string - inboundSkipPorts string - outboundSkipPorts string - trace *l5dcharts.Trace + identityContext *config.IdentityContext + image string + imagePullPolicy string + proxyVersion string + controlPort int32 + inboundPort int32 + adminPort int32 + outboundPort int32 + proxyWaitBeforeExitSeconds uint + logLevel string + resourceRequirements *l5dcharts.Resources + proxyUID int64 + initImage string + initImagePullPolicy string + initVersion string + inboundSkipPorts string + outboundSkipPorts string + trace *l5dcharts.Trace } func TestConfigAccessors(t *testing.T) { @@ -109,22 +108,22 @@ func TestConfigAccessors(t *testing.T) { k8s.ProxyVersionOverrideAnnotation: proxyVersionOverride, k8s.ProxyTraceCollectorSvcAddrAnnotation: "oc-collector.tracing:55678", k8s.ProxyTraceCollectorSvcAccountAnnotation: "default", - k8s.ProxyWaitBeforeExitAnnotation: "2m3s", + k8s.ProxyWaitBeforeExitAnnotation: "123", }, }, Spec: corev1.PodSpec{}, }, }, expected: expectedProxyConfigs{ - image: "gcr.io/linkerd-io/proxy", - imagePullPolicy: "Always", - proxyVersion: proxyVersionOverride, - controlPort: int32(4000), - inboundPort: int32(5000), - adminPort: int32(5001), - outboundPort: int32(5002), - proxyWaitBeforeExit: time.Duration(123) * time.Second, - logLevel: "debug,linkerd2_proxy=debug", + image: "gcr.io/linkerd-io/proxy", + imagePullPolicy: "Always", + proxyVersion: proxyVersionOverride, + controlPort: int32(4000), + inboundPort: int32(5000), + adminPort: int32(5001), + outboundPort: int32(5002), + proxyWaitBeforeExitSeconds: 123, + logLevel: "debug,linkerd2_proxy=debug", resourceRequirements: &l5dcharts.Resources{ CPU: l5dcharts.Constraints{ Limit: "1500m", @@ -155,16 +154,16 @@ func TestConfigAccessors(t *testing.T) { }, }, expected: expectedProxyConfigs{ - identityContext: &config.IdentityContext{}, - image: "gcr.io/linkerd-io/proxy", - imagePullPolicy: "IfNotPresent", - proxyVersion: proxyVersion, - controlPort: int32(9000), - inboundPort: int32(6000), - adminPort: int32(6001), - outboundPort: int32(6002), - proxyWaitBeforeExit: time.Duration(0), - logLevel: "info,linkerd2_proxy=debug", + identityContext: &config.IdentityContext{}, + image: "gcr.io/linkerd-io/proxy", + imagePullPolicy: "IfNotPresent", + proxyVersion: proxyVersion, + controlPort: int32(9000), + inboundPort: int32(6000), + adminPort: int32(6001), + outboundPort: int32(6002), + proxyWaitBeforeExitSeconds: 0, + logLevel: "info,linkerd2_proxy=debug", resourceRequirements: &l5dcharts.Resources{ CPU: l5dcharts.Constraints{ Limit: "1", @@ -205,7 +204,7 @@ func TestConfigAccessors(t *testing.T) { k8s.ProxyVersionOverrideAnnotation: proxyVersionOverride, k8s.ProxyTraceCollectorSvcAddrAnnotation: "oc-collector.tracing:55678", k8s.ProxyTraceCollectorSvcAccountAnnotation: "default", - k8s.ProxyWaitBeforeExitAnnotation: "2m3s", + k8s.ProxyWaitBeforeExitAnnotation: "123", }, spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{ @@ -213,15 +212,15 @@ func TestConfigAccessors(t *testing.T) { }, }, expected: expectedProxyConfigs{ - image: "gcr.io/linkerd-io/proxy", - imagePullPolicy: "Always", - proxyVersion: proxyVersionOverride, - controlPort: int32(4000), - inboundPort: int32(5000), - adminPort: int32(5001), - outboundPort: int32(5002), - proxyWaitBeforeExit: time.Duration(123) * time.Second, - logLevel: "debug,linkerd2_proxy=debug", + image: "gcr.io/linkerd-io/proxy", + imagePullPolicy: "Always", + proxyVersion: proxyVersionOverride, + controlPort: int32(4000), + inboundPort: int32(5000), + adminPort: int32(5001), + outboundPort: int32(5002), + proxyWaitBeforeExitSeconds: 123, + logLevel: "debug,linkerd2_proxy=debug", resourceRequirements: &l5dcharts.Resources{ CPU: l5dcharts.Constraints{ Limit: "1500m", @@ -244,9 +243,9 @@ func TestConfigAccessors(t *testing.T) { }, }, }, - {id: "wait before exit annotation is integer", + {id: "use not a uint value for ProxyWaitBeforeExitAnnotation annotation", nsAnnotations: map[string]string{ - k8s.ProxyWaitBeforeExitAnnotation: "222", + k8s.ProxyWaitBeforeExitAnnotation: "-111", }, spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{ @@ -255,16 +254,16 @@ func TestConfigAccessors(t *testing.T) { }, }, expected: expectedProxyConfigs{ - identityContext: &config.IdentityContext{}, - image: "gcr.io/linkerd-io/proxy", - imagePullPolicy: "IfNotPresent", - proxyVersion: proxyVersion, - controlPort: int32(9000), - inboundPort: int32(6000), - adminPort: int32(6001), - outboundPort: int32(6002), - proxyWaitBeforeExit: time.Duration(222) * time.Second, - logLevel: "info,linkerd2_proxy=debug", + identityContext: &config.IdentityContext{}, + image: "gcr.io/linkerd-io/proxy", + imagePullPolicy: "IfNotPresent", + proxyVersion: proxyVersion, + controlPort: int32(9000), + inboundPort: int32(6000), + adminPort: int32(6001), + outboundPort: int32(6002), + proxyWaitBeforeExitSeconds: 0, + logLevel: "info,linkerd2_proxy=debug", resourceRequirements: &l5dcharts.Resources{ CPU: l5dcharts.Constraints{ Limit: "1", @@ -361,9 +360,13 @@ func TestConfigAccessors(t *testing.T) { } }) - t.Run("proxyWaitBeforeExit", func(t *testing.T) { - expected := testCase.expected.proxyWaitBeforeExit - if actual := resourceConfig.proxyWaitBeforeExit(); expected != actual { + t.Run("proxyWaitBeforeExitSeconds", func(t *testing.T) { + expected := testCase.expected.proxyWaitBeforeExitSeconds + actual, err := resourceConfig.proxyWaitBeforeExitSeconds() + if nil != err { + // negative test cases above cover the situation + } + if expected != actual { t.Errorf("Expected: %v Actual: %v", expected, actual) } }) diff --git a/pkg/k8s/labels.go b/pkg/k8s/labels.go index 9912639439edf..60cb3fec02a43 100644 --- a/pkg/k8s/labels.go +++ b/pkg/k8s/labels.go @@ -184,7 +184,7 @@ const ( // ProxyWaitBeforeExitAnnotation makes the proxy container to wait for the given period before exiting // after the Pod entered the Terminating state. Must be smaller than terminationGracePeriodSeconds // configured for the Pod - ProxyWaitBeforeExitAnnotation = ProxyConfigAnnotationsPrefixAlpha + "/proxy-wait-before-exit" + ProxyWaitBeforeExitAnnotation = ProxyConfigAnnotationsPrefixAlpha + "/proxy-wait-before-exit-seconds" // ProxyTraceCollectorSvcAccountAnnotation is used to specify the service account // associated with the trace collector. It is used to create the service's