Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add native sidecar support #11465

Merged
merged 22 commits into from
Nov 22, 2023
Merged

Add native sidecar support #11465

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
dcffd36
Add native sidecar support
teejaded Oct 6, 2023
2c1d8a1
Update viz/jaeger injectors to support native sidecars
teejaded Oct 10, 2023
945cd81
Update initContainer ordering for native sidecars
teejaded Oct 10, 2023
ccf447d
Only control initContainer order when doing native sidecars
teejaded Oct 18, 2023
8dd3eea
Add native sidecar startupProbe and preStop
teejaded Oct 18, 2023
9bf8f86
Reuse input file for native sidecar test
teejaded Oct 24, 2023
50fb0ee
Add cli doc for native sidecar
teejaded Oct 24, 2023
6cb451d
Fix control plane install with native sidecars
teejaded Oct 24, 2023
f082f51
Allow default authorizations to work with native sidecars
teejaded Oct 24, 2023
28b0aca
Allow native sidecars in controlplane without startupProbe
teejaded Nov 3, 2023
f6b2ddb
Cleanup additional unneeded testdata file
teejaded Nov 3, 2023
6e8f068
Remove native sidecar proxy preStop shutdown hook
teejaded Nov 3, 2023
6303ff3
Mimic await behavior with startupProbe
teejaded Nov 7, 2023
db2f92b
Override startupProbe parameters for destination and injector components
teejaded Nov 8, 2023
27b1328
Use empty string when proxypath is not found
teejaded Nov 10, 2023
2cda129
Remove unused file inject_emojivoto_deployment_native_sidecar.report.…
teejaded Nov 10, 2023
8e91ba2
Update helm docs
teejaded Nov 10, 2023
6d68d65
Disallow nativeSidecar and waitBeforeExitSeconds together
teejaded Nov 10, 2023
7b36117
Disable nativeSidecar for identity and increase startupProbeInitialDe…
teejaded Nov 10, 2023
fb3f58c
Add comment and rewrite, for readability, patch $initIndex
teejaded Nov 20, 2023
7cd3679
Add default proxy startupProbe parameters to values.yaml
teejaded Nov 20, 2023
62bcf87
Update `cli/cmd/doc.go` ProxyEnableNativeSidecarAnnotation description
teejaded Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/linkerd-control-plane/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ Kubernetes: `>=1.21.0-0`
| proxy.inboundDiscoveryCacheUnusedTimeout | string | `"90s"` | Maximum time allowed before an unused inbound discovery result is evicted from the cache |
| proxy.logFormat | string | `"plain"` | Log format (`plain` or `json`) for the proxy |
| proxy.logLevel | string | `"warn,linkerd=info,trust_dns=error"` | Log level for the proxy |
| proxy.nativeSidecar | bool | `false` | Enable KEP-753 native sidecars This is an experimental feature. It requires Kubernetes >= 1.28 and the SidecarContainers feature gate to be enabled. Note the setting .proxy.waitBeforeExitSeconds is discarded in this mode because sidecar support for preStop hooks is not yet implemeted. It is expected in Kubernetes version 1.29. |
| proxy.opaquePorts | string | `"25,587,3306,4444,5432,6379,9300,11211"` | Default set of opaque ports - SMTP (25,587) server-first - MYSQL (3306) server-first - Galera (4444) server-first - PostgreSQL (5432) server-first - Redis (6379) server-first - ElasticSearch (9300) server-first - Memcached (11211) clients do not issue any preamble, which breaks detection |
| proxy.outboundConnectTimeout | string | `"1000ms"` | Maximum time allowed for the proxy to establish an outbound TCP connection |
| proxy.outboundDiscoveryCacheUnusedTimeout | string | `"5s"` | Maximum time allowed before an unused outbound discovery result is evicted from the cache |
Expand Down
8 changes: 8 additions & 0 deletions charts/linkerd-control-plane/templates/destination.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,9 @@ spec:
*/}}
{{- $_ := set $tree.Values.proxy "defaultInboundPolicy" "all-unauthenticated" }}
{{- $_ := set $tree.Values.proxy "capabilities" (dict "drop" (list "ALL")) }}
{{- if not $tree.Values.proxy.nativeSidecar }}
- {{- include "partials.proxy" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{- end }}
- args:
- destination
- -addr=:8086
Expand Down Expand Up @@ -341,6 +343,12 @@ spec:
{{- $_ := set $tree.Values.proxyInit "ignoreOutboundPorts" .Values.proxyInit.kubeAPIServerPorts -}}
- {{- include "partials.proxy-init" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{ end -}}
{{- if $tree.Values.proxy.nativeSidecar }}
{{- $_ := set $tree.Values.proxy "startupProbeInitialDelaySeconds" 20 }}
teejaded marked this conversation as resolved.
Show resolved Hide resolved
{{- $_ := set $tree.Values.proxy "startupProbePeriodSeconds" 5 }}
{{- $_ := set $tree.Values.proxy "startupProbeFailureThreshold" 20 }}
- {{- include "partials.proxy" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{ end -}}
{{- if .Values.priorityClassName -}}
priorityClassName: {{ .Values.priorityClassName }}
{{ end -}}
Expand Down
5 changes: 5 additions & 0 deletions charts/linkerd-control-plane/templates/identity.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,9 @@ spec:
{{- $_ := set $tree.Values.proxy "capabilities" (dict "drop" (list "ALL")) }}
{{- $_ := set $tree.Values.proxy "outboundDiscoveryCacheUnusedTimeout" "5s" }}
{{- $_ := set $tree.Values.proxy "inboundDiscoveryCacheUnusedTimeout" "90s" }}
{{- if not $tree.Values.proxy.nativeSidecar }}
teejaded marked this conversation as resolved.
Show resolved Hide resolved
- {{- include "partials.proxy" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{- end }}
teejaded marked this conversation as resolved.
Show resolved Hide resolved
initContainers:
{{ if .Values.cniEnabled -}}
- {{- include "partials.network-validator" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
Expand All @@ -228,6 +230,9 @@ spec:
{{- $_ := set $tree.Values.proxyInit "ignoreOutboundPorts" .Values.proxyInit.kubeAPIServerPorts -}}
- {{- include "partials.proxy-init" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{ end -}}
{{- if $tree.Values.proxy.nativeSidecar }}
- {{- include "partials.proxy" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{ end -}}
teejaded marked this conversation as resolved.
Show resolved Hide resolved
{{- if .Values.priorityClassName -}}
priorityClassName: {{ .Values.priorityClassName }}
{{ end -}}
Expand Down
8 changes: 8 additions & 0 deletions charts/linkerd-control-plane/templates/proxy-injector.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ spec:
{{- $_ := set $tree.Values.proxy "capabilities" (dict "drop" (list "ALL")) }}
{{- $_ := set $tree.Values.proxy "outboundDiscoveryCacheUnusedTimeout" "5s" }}
{{- $_ := set $tree.Values.proxy "inboundDiscoveryCacheUnusedTimeout" "90s" }}
{{- if not $tree.Values.proxy.nativeSidecar }}
- {{- include "partials.proxy" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{- end }}
- args:
- proxy-injector
- -log-level={{.Values.controllerLogLevel}}
Expand Down Expand Up @@ -127,6 +129,12 @@ spec:
{{- $_ := set $tree.Values.proxyInit "ignoreOutboundPorts" .Values.proxyInit.kubeAPIServerPorts -}}
- {{- include "partials.proxy-init" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{ end -}}
{{- if $tree.Values.proxy.nativeSidecar }}
{{- $_ := set $tree.Values.proxy "startupProbeInitialDelaySeconds" 20 }}
teejaded marked this conversation as resolved.
Show resolved Hide resolved
{{- $_ := set $tree.Values.proxy "startupProbePeriodSeconds" 5 }}
{{- $_ := set $tree.Values.proxy "startupProbeFailureThreshold" 20 }}
- {{- include "partials.proxy" $tree | indent 8 | trimPrefix (repeat 7 " ") }}
{{ end -}}
{{- if .Values.priorityClassName -}}
priorityClassName: {{ .Values.priorityClassName }}
{{ end -}}
Expand Down
7 changes: 7 additions & 0 deletions charts/linkerd-control-plane/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,13 @@ proxy:
# "all-unauthenticated", "cluster-authenticated", "cluster-unauthenticated", "deny"
# @default -- "all-unauthenticated"
defaultInboundPolicy: "all-unauthenticated"
# -- Enable KEP-753 native sidecars
# This is an experimental feature. It requires Kubernetes >= 1.28 and the
# SidecarContainers feature gate to be enabled. Note the setting
# .proxy.waitBeforeExitSeconds is discarded in this mode because
# sidecar support for preStop hooks is not yet implemeted. It is expected
# in Kubernetes version 1.29.
teejaded marked this conversation as resolved.
Show resolved Hide resolved
nativeSidecar: false

# proxy-init configuration
proxyInit:
Expand Down
16 changes: 14 additions & 2 deletions charts/partials/templates/_proxy.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,15 @@ readinessProbe:
path: /ready
port: {{.Values.proxy.ports.admin}}
initialDelaySeconds: 2
{{- if and .Values.proxy.nativeSidecar .Values.proxy.await }}
startupProbe:
httpGet:
path: /ready
port: {{.Values.proxy.ports.admin}}
initialDelaySeconds: {{.Values.proxy.startupProbeInitialDelaySeconds | default 0}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these variables should be added to values.yaml. This will allow us to set default values in values.yaml instead of needing to do defaulting inline in the template like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline defaults are for injection because I didn't plumb the values into pkg/charts/linkerd2.Proxy.

I'm happy to do this, but I think this would expose those settings to users to modify which we discussed above as being out of scope or perhaps undesired. Please let me know if I'm mistaken.

periodSeconds: {{.Values.proxy.startupProbePeriodSeconds | default 1}}
failureThreshold: {{.Values.proxy.startupProbeFailureThreshold | default 120}}
{{- end }}
{{- if .Values.proxy.resources }}
{{ include "partials.resources" .Values.proxy.resources }}
{{- end }}
Expand All @@ -182,9 +191,9 @@ securityContext:
seccompProfile:
type: RuntimeDefault
terminationMessagePolicy: FallbackToLogsOnError
{{- if or (.Values.proxy.await) (.Values.proxy.waitBeforeExitSeconds) }}
{{- if or .Values.proxy.await .Values.proxy.waitBeforeExitSeconds .Values.proxy.nativeSidecar }}
lifecycle:
{{- if .Values.proxy.await }}
{{- if and .Values.proxy.await (not .Values.proxy.nativeSidecar) }}
teejaded marked this conversation as resolved.
Show resolved Hide resolved
postStart:
exec:
command:
Expand Down Expand Up @@ -212,4 +221,7 @@ volumeMounts:
name: {{.Values.proxy.saMountPath.name}}
readOnly: {{.Values.proxy.saMountPath.readOnly}}
{{- end -}}
{{- if .Values.proxy.nativeSidecar }}
restartPolicy: Always
{{- end -}}
{{- end }}
9 changes: 6 additions & 3 deletions charts/patch/templates/patch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{{ $prefix := .Values.pathPrefix -}}
{{ $initIndex := ternary "0" "-" (dig "proxy" "nativeSidecar" false (merge (dict) .Values)) -}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to have a template comment here explaining this. if I understand correctly, the reasoning is that in native sidecar mode, we can put our containers first so that other init containers can take advantage of the proxy but without native sidecar, the proxy-init and network-validator init containers must be last to ensure there are no containers that start after iptables has been configured but before the proxy has started. in other words, the proxy-init, network-validator, and proxy contains must be started consecutively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree a comment is needed. My goal here was partially to preserve the current behavior when nativeSidecar was not enabled since it's entirely possible someone is relying on the order of the containers in a kubectl patch, kustomize or something.

[
{{- if .Values.addRootMetadata }}
{
Expand Down Expand Up @@ -62,14 +63,14 @@
},
{
"op": "add",
"path": "{{$prefix}}/spec/initContainers/-",
"path": "{{$prefix}}/spec/initContainers/{{$initIndex}}{{$initIndex = add1 $initIndex}}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when add1 is called on "-"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It silently becomes 1. I thought it was an acceptable quirk since in that case the variable is not used again.

https://helm-playground.com/#t=LQhQG92ACASBDaAuAvNARMd0C%2BPQCmAHvALYAOANgUtJHIgD7QCOArgPYAuBu%2BkMBNDTwAJqICMDPoRIVqAJlr0heUEA&v=LQhQqA

"value":
{{- include "partials.proxy-init" . | fromYaml | toPrettyJson | nindent 6 }}
},
{{- else if and .Values.proxy .Values.cniEnabled }}
{
"op": "add",
"path": "{{$prefix}}/spec/initContainers/-",
"path": "{{$prefix}}/spec/initContainers/{{$initIndex}}{{$initIndex = add1 $initIndex}}",
"value":
{{- include "partials.network-validator" . | fromYaml | toPrettyJson | nindent 6 }}
},
Expand Down Expand Up @@ -103,7 +104,9 @@
{{- end }}
{
"op": "add",
{{- if .Values.proxy.await }}
{{- if .Values.proxy.nativeSidecar }}
"path": "{{$prefix}}/spec/initContainers/{{$initIndex}}",
{{- else if .Values.proxy.await }}
"path": "{{$prefix}}/spec/containers/0",
{{- else }}
"path": "{{$prefix}}/spec/containers/-",
Expand Down
4 changes: 4 additions & 0 deletions cli/cmd/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,5 +280,9 @@ func generateAnnotationsDocs() []annotationDoc {
Name: k8s.ProxyShutdownGracePeriodAnnotation,
Description: "Grace period for graceful proxy shutdowns. If this timeout elapses before all open connections have completed, the proxy will terminate forcefully, closing any remaining connections.",
},
{
Name: k8s.ProxyEnableNativeSidecarAnnotation,
Description: "Enables native sidecars as init containers. Requires Kubernetes >=1.28, SidecarContainers feature gate, and disables waitBeforeExitSeconds.",
teejaded marked this conversation as resolved.
Show resolved Hide resolved
},
}
}
4 changes: 4 additions & 0 deletions cli/cmd/inject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,10 @@ func getOverrideAnnotations(values *linkerd2.Values, base *linkerd2.Values) map[
overrideAnnotations[k8s.ProxyShutdownGracePeriodAnnotation] = proxy.ShutdownGracePeriod
}

if proxy.NativeSidecar != baseProxy.NativeSidecar {
overrideAnnotations[k8s.ProxyEnableNativeSidecarAnnotation] = strconv.FormatBool(proxy.NativeSidecar)
}

return overrideAnnotations
}

Expand Down
13 changes: 13 additions & 0 deletions cli/cmd/inject_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,17 @@ func TestUninjectAndInject(t *testing.T) {
return values
}(),
},
{
inputFileName: "inject_emojivoto_deployment.input.yml",
goldenFileName: "inject_emojivoto_deployment_native_sidecar.golden.yml",
reportFileName: "inject_emojivoto_deployment.report",
injectProxy: true,
testInjectConfig: func() *linkerd2.Values {
values := defaultConfig()
values.Proxy.NativeSidecar = true
return values
}(),
},
}

for i, tc := range testCases {
Expand Down Expand Up @@ -678,6 +689,7 @@ func TestProxyConfigurationAnnotations(t *testing.T) {
values.Proxy.Await = false
values.Proxy.AccessLog = "apache"
values.Proxy.ShutdownGracePeriod = "60s"
values.Proxy.NativeSidecar = true

expectedOverrides := map[string]string{
k8s.ProxyIgnoreInboundPortsAnnotation: "8500-8505",
Expand All @@ -699,6 +711,7 @@ func TestProxyConfigurationAnnotations(t *testing.T) {
k8s.ProxyAwait: "disabled",
k8s.ProxyAccessLogAnnotation: "apache",
k8s.ProxyShutdownGracePeriodAnnotation: "60s",
k8s.ProxyEnableNativeSidecarAnnotation: "true",
}

overrides := getOverrideAnnotations(values, baseValues)
Expand Down
6 changes: 6 additions & 0 deletions cli/cmd/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,12 @@ func makeInjectFlags(defaults *l5dcharts.Values) ([]flag.Flag, *pflag.FlagSet) {
injectFlags := pflag.NewFlagSet("inject", pflag.ExitOnError)

flags := []flag.Flag{
flag.NewBoolFlag(injectFlags, "native-sidecar", false, "Enable native sidecar",
func(values *l5dcharts.Values, value bool) error {
values.Proxy.NativeSidecar = value
return nil
}),

flag.NewInt64Flag(injectFlags, "wait-before-exit-seconds", int64(defaults.Proxy.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)",
Expand Down
Loading
Loading