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

Jump to bottom

Injection: enable support for automatic detection of native sidecars #49570

Closed
wants to merge 1 commit into from
Closed

Injection: enable support for automatic detection of native sidecars #49570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 4 additions & 5 deletions manifests/charts/istio-control/istio-discovery/files/injection-template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
{{- end }}
{{- end }}
{{- end }}
{{ $nativeSidecar := (eq (env "ENABLE_NATIVE_SIDECARS" "false") "true") }}
{{- $containers := list }}
{{- range $index, $container := .Spec.Containers }}{{ if not (eq $container.Name "istio-proxy") }}{{ $containers = append $containers $container.Name }}{{end}}{{- end}}
metadata:
Expand Down Expand Up @@ -67,7 +66,7 @@ metadata:
spec:
{{- $holdProxy := and
(or .ProxyConfig.HoldApplicationUntilProxyStarts.GetValue .Values.global.proxy.holdApplicationUntilProxyStarts)
(not $nativeSidecar) }}
(not .NativeSidecars) }}
initContainers:
{{ if ne (annotation .ObjectMeta `sidecar.istio.io/interceptionMode` .ProxyConfig.InterceptionMode) `NONE` }}
{{ if or .Values.pilot.cni.enabled .Values.istio_cni.enabled -}}
Expand Down Expand Up @@ -187,7 +186,7 @@ spec:
runAsNonRoot: false
runAsUser: 0
{{ end }}
{{ if not $nativeSidecar }}
{{ if not .NativeSidecars }}
containers:
{{ end }}
- name: istio-proxy
Expand All @@ -196,7 +195,7 @@ spec:
{{- else }}
image: "{{ .ProxyImage }}"
{{- end }}
{{ if $nativeSidecar }}restartPolicy: Always{{end}}
{{ if .NativeSidecars }}restartPolicy: Always{{end}}
ports:
- containerPort: 15090
protocol: TCP
Expand Down Expand Up @@ -225,7 +224,7 @@ spec:
command:
- pilot-agent
- wait
{{- else if $nativeSidecar }}
{{- else if .NativeSidecars }}
{{- /* preStop is called when the pod starts shutdown. Initialize drain. We will get SIGTERM once applications are torn down. */}}
lifecycle:
preStop:
Expand Down
9 changes: 4 additions & 5 deletions manifests/charts/istiod-remote/files/injection-template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
{{- end }}
{{- end }}
{{- end }}
{{ $nativeSidecar := (eq (env "ENABLE_NATIVE_SIDECARS" "false") "true") }}
{{- $containers := list }}
{{- range $index, $container := .Spec.Containers }}{{ if not (eq $container.Name "istio-proxy") }}{{ $containers = append $containers $container.Name }}{{end}}{{- end}}
metadata:
Expand Down Expand Up @@ -67,7 +66,7 @@ metadata:
spec:
{{- $holdProxy := and
(or .ProxyConfig.HoldApplicationUntilProxyStarts.GetValue .Values.global.proxy.holdApplicationUntilProxyStarts)
(not $nativeSidecar) }}
(not .NativeSidecars) }}
initContainers:
{{ if ne (annotation .ObjectMeta `sidecar.istio.io/interceptionMode` .ProxyConfig.InterceptionMode) `NONE` }}
{{ if or .Values.pilot.cni.enabled .Values.istio_cni.enabled -}}
Expand Down Expand Up @@ -187,7 +186,7 @@ spec:
runAsNonRoot: false
runAsUser: 0
{{ end }}
{{ if not $nativeSidecar }}
{{ if not .NativeSidecars }}
containers:
{{ end }}
- name: istio-proxy
Expand All @@ -196,7 +195,7 @@ spec:
{{- else }}
image: "{{ .ProxyImage }}"
{{- end }}
{{ if $nativeSidecar }}restartPolicy: Always{{end}}
{{ if .NativeSidecars }}restartPolicy: Always{{end}}
ports:
- containerPort: 15090
protocol: TCP
Expand Down Expand Up @@ -225,7 +224,7 @@ spec:
command:
- pilot-agent
- wait
{{- else if $nativeSidecar }}
{{- else if .NativeSidecars }}
{{- /* preStop is called when the pod starts shutdown. Initialize drain. We will get SIGTERM once applications are torn down. */}}
lifecycle:
preStop:
Expand Down
35 changes: 32 additions & 3 deletions pilot/pkg/features/experimental.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,25 @@ var (
EnableOptimizedServicePush = env.RegisterBoolVar("ISTIO_ENABLE_OPTIMIZED_SERVICE_PUSH", true,
"If enabled, Istiod will not push changes on arbitrary annotation change.").Get()

// This is used in injection templates, it is not unused.
EnableNativeSidecars = env.Register("ENABLE_NATIVE_SIDECARS", false,
"If set, used Kubernetes native Sidecar container support. Requires SidecarContainer feature flag.")
EnableNativeSidecars = func() NativeSidecarMode {
v := env.Register("ENABLE_NATIVE_SIDECARS", "false",
"If set, used Kubernetes native Sidecar container support. Requires SidecarContainer feature flag."+
" Set to true or false to unconditionally enable. Set to auto-beta or auto-stable to automatically enable"+
" if support is detected (at the beta or stable level).").Get()
switch v {
case "never", "false":
return NativeSidecarModeNever
case "always", "true":
return NativeSidecarModeAlways
case "auto-beta":
Copy link
Contributor

@bleggett bleggett Mar 5, 2024 •
edited

Choose a reason for hiding this comment

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

Can you articulate why we need both auto-beta and auto-stable versus just auto?

That feels like a lot of knobs for a single feature (admittedly it's a critical one), and also I don't think it makes a lot of sense to have feature-level logic for opting-in depending on stability level - that should/will be defined elsewhere and can be implicit in auto.

(Yes I realize this means people who are using pure stable will not use this. That's probably fine?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Eventually I think the default should be auto-stable. We should not default to beta k8s features, only stable ones IMO. So the possible one to remove is auto-beta IMO.

The benefit of auto-beta is that a lot of users do want to use beta k8s features, and its IMO pretty reasonable to. So what they get is a bit of safety in doing that, rather than just on.

Most of the complexity is in the detection anyways, beta vs stable is pretty simple

Copy link
Contributor

@bleggett bleggett Mar 5, 2024 •
edited

Choose a reason for hiding this comment

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

Can we just do

never, always and auto for now?

Whether auto should ignore k8s beta features or not probably should be controlled globally by e.g. @whitneygriffith's experimental/stable profiles and implicit in that (if you are in istio stable you do not get to use k8s experimental stuff, and if you are in istio experimental you do, it is not per-feature), and not at the feature level.

Comment on lines +225 to +230
Copy link
Member

Choose a reason for hiding this comment

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

From my perspective this is not improve UX, on the opposite it is increasing complexity

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it the auto-beta vs auto-stable? Or even auto would be too complex?

Copy link
Member

Choose a reason for hiding this comment

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

I mean auto-beta auto-stable, rare people can distinguish them

Copy link
Contributor

@bleggett bleggett Mar 25, 2024 •
edited

Choose a reason for hiding this comment

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

I would rather it just be never/always/auto for this PR - whether we use k8s beta features or not should not be a per-feature decision anyway, and should be controlled elsewhere globally for all features (by stable profiles, etc etc).

return NativeSidecarModeAutoBeta
case "auto-stable":
return NativeSidecarModeAutoStable
default:
log.Warnf("unknown ENABLE_NATIVE_SIDECARS value %q", v)
return NativeSidecarModeNever
}
}()

OptimizedConfigRebuild = env.Register("ENABLE_OPTIMIZED_CONFIG_REBUILD", true,
"If enabled, pilot will only rebuild config for resources that have changed").Get()
Expand All @@ -228,3 +244,16 @@ var (
"If enabled, istiod will persist the oldest first heuristic for subtly conflicting traffic policy selection"+
"(such as with overlapping wildcard hosts)").Get()
)

type NativeSidecarMode int

const (
// Never use native sidecar
NativeSidecarModeNever NativeSidecarMode = iota
// Always use native sidecar
NativeSidecarModeAlways = iota
// AutoBeta will use native sidecars if its detected as supported at a beta+ stability level
NativeSidecarModeAutoBeta = iota
// AutoStable will use native sidecars if its detected as supported at a stable stability level
NativeSidecarModeAutoStable = iota
)
28 changes: 18 additions & 10 deletions pkg/kube/inject/inject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ type SidecarTemplateData struct {
MeshConfig *meshconfig.MeshConfig
Values map[string]any
Revision string
NativeSidecars bool
ProxyImage string
ProxyUID int64
ProxyGID int64
Expand Down Expand Up @@ -421,15 +422,17 @@ func RunTemplate(params InjectionParameters) (mergedPod *corev1.Pod, templatePod

// When changing this, make sure to change TemplateInput in deploymentcontroller.go
data := SidecarTemplateData{
TypeMeta: params.typeMeta,
DeploymentMeta: params.deployMeta,
ObjectMeta: strippedPod.ObjectMeta,
Spec: strippedPod.Spec,
ProxyConfig: params.proxyConfig,
MeshConfig: meshConfig,
Values: params.valuesConfig.asMap,
Revision: params.revision,
ProxyImage: ProxyImage(params.valuesConfig.asStruct, params.proxyConfig.Image, strippedPod.Annotations),
TypeMeta: params.typeMeta,
DeploymentMeta: params.deployMeta,
ObjectMeta: strippedPod.ObjectMeta,
Spec: strippedPod.Spec,
ProxyConfig: params.proxyConfig,
MeshConfig: meshConfig,
Values: params.valuesConfig.asMap,
Revision: params.revision,
ProxyImage: ProxyImage(params.valuesConfig.asStruct, params.proxyConfig.Image, strippedPod.Annotations),

NativeSidecars: params.nativeSidecar,
ProxyUID: proxyUID,
ProxyGID: proxyGID,
InboundTrafficPolicyMode: InboundTrafficPolicyMode(meshConfig),
Expand Down Expand Up @@ -458,7 +461,7 @@ func RunTemplate(params InjectionParameters) (mergedPod *corev1.Pod, templatePod
// these will be in the `containers` field.
// So if we see the proxy container in `containers` in the original pod, and in `initContainers` in the template pod,
// move the container.
if features.EnableNativeSidecars.Get() &&
if params.nativeSidecar &&
FindContainer(ProxyContainerName, templatePod.Spec.InitContainers) != nil &&
FindContainer(ProxyContainerName, mergedPod.Spec.Containers) != nil {
mergedPod = mergedPod.DeepCopy()
Expand Down Expand Up @@ -793,10 +796,15 @@ func IntoObject(injector Injector, sidecarTemplate Templates, valuesConfig Value
meshConfig: meshconfig,
proxyConfig: meshconfig.GetDefaultConfig(),
valuesConfig: valuesConfig,
nativeSidecar: false,
revision: revision,
proxyEnvs: map[string]string{},
injectedAnnotations: nil,
}
// The 'auto' mode is not supported for this codepath, only for webhook injection.
if features.EnableNativeSidecars == features.NativeSidecarModeAlways {
params.nativeSidecar = true
}
patchBytes, err = injectPod(params)
}
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions pkg/kube/inject/inject_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func TestInjection(t *testing.T) {
in: "proxy-override-args.yaml",
want: "proxy-override-args-native.yaml.injected",
setup: func(t test.Failer) {
test.SetEnvForTest(t, features.EnableNativeSidecars.Name, "true")
test.SetForTest(t, &features.EnableNativeSidecars, features.NativeSidecarModeAlways)
},
},
{
Expand All @@ -294,14 +294,14 @@ func TestInjection(t *testing.T) {
in: "gateway.yaml",
want: "gateway.yaml.injected",
setup: func(t test.Failer) {
test.SetEnvForTest(t, features.EnableNativeSidecars.Name, "true")
test.SetForTest(t, &features.EnableNativeSidecars, features.NativeSidecarModeAlways)
},
},
{
in: "native-sidecar.yaml",
want: "native-sidecar.yaml.injected",
setup: func(t test.Failer) {
test.SetEnvForTest(t, features.EnableNativeSidecars.Name, "true")
test.SetForTest(t, &features.EnableNativeSidecars, features.NativeSidecarModeAlways)
},
},
{
Expand Down
9 changes: 4 additions & 5 deletions pkg/kube/inject/testdata/inputs/custom-template.yaml.37.template.gen.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 4 additions & 5 deletions pkg/kube/inject/testdata/inputs/default.template.gen.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 4 additions & 5 deletions pkg/kube/inject/testdata/inputs/enable-core-dump.yaml.5.template.gen.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 4 additions & 5 deletions pkg/kube/inject/testdata/inputs/hello-existing-cncf-networks-json.yaml.16.template.gen.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.