Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ metadata:
data:
config: |-
policy: {{ .Values.global.proxy.autoInject }}
rewriteAppHTTPProbe: {{ .Values.sidecarInjectorWebhook.rewriteAppHTTPProbe }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confused - why ?

Typically we use pod annotations to allow user to opt in/out of a specific feature.

template: |-
{{- if or (not .Values.istio_cni.enabled) .Values.global.proxy.enableCoreDump }}
initContainers:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ data:
config: |-
policy: {{ .Values.global.proxy.autoInject }}
template: |-
rewriteAppHTTPProbe: {{ .Values.sidecarInjectorWebhook.rewriteAppHTTPProbe }}
{{- if or (not .Values.istio_cni.enabled) .Values.global.proxy.enableCoreDump }}
initContainers:
{{- if not .Values.istio_cni.enabled }}
Expand Down
1 change: 1 addition & 0 deletions install/kubernetes/helm/istio/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ gateways:
#
sidecarInjectorWebhook:
enabled: true
rewriteAppHTTPProbe: false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lots of comments please.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once again - after reverting please add comments explaining what it does. Nobody can understand - rewrite how, when to set, why, etc. Don't assume all users are familiar with the your implementation details and goals.


#
# galley configuration, refer to charts/galley/values.yaml
Expand Down
4 changes: 4 additions & 0 deletions istioctl/cmd/istioctl/kubeinject.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ var (
verbosity int
versionStr string // override build version
enableCoreDump bool
rewriteAppHTTPProbe bool
imagePullPolicy string
statusPort int
readinessInitialDelaySeconds uint32
Expand Down Expand Up @@ -274,6 +275,7 @@ istioctl kube-inject -f deployment.yaml -o deployment-injected.yaml --injectConf
if sidecarTemplate, err = inject.GenerateTemplateFromParams(&inject.Params{
InitImage: inject.InitImageName(hub, tag, debugMode),
ProxyImage: inject.ProxyImageName(hub, tag, debugMode),
RewriteAppHTTPProbe: rewriteAppHTTPProbe,
Verbosity: verbosity,
SidecarProxyUID: sidecarProxyUID,
Version: versionStr,
Expand Down Expand Up @@ -372,6 +374,8 @@ func init() {
injectCmd.PersistentFlags().BoolVar(&enableCoreDump, "coreDump",
true, "Enable/Disable core dumps in injected Envoy sidecar (--coreDump=true affects "+
"all pods in a node and should only be used the cluster admin)")
injectCmd.PersistentFlags().BoolVar(&rewriteAppHTTPProbe, "rewriteAppProbe", false, "Whether injector "+
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it's a flag - why do you need it on the config map ?

"rewrites the liveness health check to let kubelet health check the app when mtls is on.")
injectCmd.PersistentFlags().StringVar(&imagePullPolicy, "imagePullPolicy", inject.DefaultImagePullPolicy,
"Sets the container image pull policy. Valid options are Always,IfNotPresent,Never."+
"The default policy is IfNotPresent.")
Expand Down
3 changes: 3 additions & 0 deletions pilot/pkg/kube/inject/app_probe.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ func rewriteAppHTTPProbe(spec *SidecarInjectionSpec, podSpec *corev1.PodSpec) {
if spec == nil || podSpec == nil {
return
}
if !spec.RewriteAppHTTPProbe {
return
}
for _, c := range spec.Containers {
if c.Name != istioProxyContainerName {
continue
Expand Down
6 changes: 6 additions & 0 deletions pilot/pkg/kube/inject/app_probe_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func TestRewriteAppHTTPProbe(t *testing.T) {
{
name: "one-container",
sidecar: &SidecarInjectionSpec{
RewriteAppHTTPProbe: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't make any sense - why would this be part of the spec, next to containers ?

Containers: []corev1.Container{
{
Name: "istio-proxy",
Expand Down Expand Up @@ -89,6 +90,7 @@ func TestRewriteAppHTTPProbe(t *testing.T) {
{
name: "both-readiness-liveness-rewrite",
sidecar: &SidecarInjectionSpec{
RewriteAppHTTPProbe: true,
Containers: []corev1.Container{
{
Name: "istio-proxy",
Expand Down Expand Up @@ -160,6 +162,7 @@ func TestRewriteAppHTTPProbe(t *testing.T) {
{
name: "no-statusPort-find",
sidecar: &SidecarInjectionSpec{
RewriteAppHTTPProbe: true,
Containers: []corev1.Container{
{
Name: "istio-proxy",
Expand Down Expand Up @@ -209,6 +212,7 @@ func TestRewriteAppHTTPProbe(t *testing.T) {
{
name: "-statusPort=15020-parsing",
sidecar: &SidecarInjectionSpec{
RewriteAppHTTPProbe: true,
Containers: []corev1.Container{
{
Name: "istio-proxy",
Expand Down Expand Up @@ -261,6 +265,7 @@ func TestRewriteAppHTTPProbe(t *testing.T) {
{
name: "--statusPort=15020-parsing",
sidecar: &SidecarInjectionSpec{
RewriteAppHTTPProbe: true,
Containers: []corev1.Container{
{
Name: "istio-proxy",
Expand Down Expand Up @@ -313,6 +318,7 @@ func TestRewriteAppHTTPProbe(t *testing.T) {
{
name: "two-container-rewrite",
sidecar: &SidecarInjectionSpec{
RewriteAppHTTPProbe: true,
Containers: []corev1.Container{
{
Name: "istio-proxy",
Expand Down
12 changes: 8 additions & 4 deletions pilot/pkg/kube/inject/inject.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,13 @@ const (
// SidecarInjectionSpec collects all container types and volumes for
// sidecar mesh injection
type SidecarInjectionSpec struct {
InitContainers []corev1.Container `yaml:"initContainers"`
Containers []corev1.Container `yaml:"containers"`
Volumes []corev1.Volume `yaml:"volumes"`
ImagePullSecrets []corev1.LocalObjectReference `yaml:"imagePullSecrets"`
// RewriteHTTPProbe indicates whether Kubernetes HTTP prober in the PodSpec
// will be rewritten to be redirected by pilot agent.
RewriteAppHTTPProbe bool `yaml:"rewriteAppHTTPProbe"`
InitContainers []corev1.Container `yaml:"initContainers"`
Containers []corev1.Container `yaml:"containers"`
Volumes []corev1.Volume `yaml:"volumes"`
ImagePullSecrets []corev1.LocalObjectReference `yaml:"imagePullSecrets"`
}

// SidecarTemplateData is the data object to which the templated
Expand Down Expand Up @@ -181,6 +184,7 @@ func ProxyImageName(hub string, tag string, debug bool) string {
// into a kubernetes resource.
type Params struct {
InitImage string `json:"initImage"`
RewriteAppHTTPProbe bool `json:"rewriteAppHTTPProbe"`
ProxyImage string `json:"proxyImage"`
Verbosity int `json:"verbosity"`
SidecarProxyUID uint64 `json:"sidecarProxyUID"`
Expand Down
15 changes: 15 additions & 0 deletions pilot/pkg/kube/inject/inject_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ func TestIntoResourceFile(t *testing.T) {
readinessPeriodSeconds uint32
readinessFailureThreshold uint32
tproxy bool
rewriteAppHTTPProbe bool
}{
// "testdata/hello.yaml" is tested in http_test.go (with debug)
{
Expand Down Expand Up @@ -128,6 +129,16 @@ func TestIntoResourceFile(t *testing.T) {
debugMode: true,
tproxy: true,
},
{
in: "hello-probes.yaml",
want: "hello-probes.yaml.no-rewrite.injected",
includeIPRanges: DefaultIncludeIPRanges,
includeInboundPorts: DefaultIncludeInboundPorts,
statusPort: DefaultStatusPort,
readinessInitialDelaySeconds: DefaultReadinessInitialDelaySeconds,
readinessPeriodSeconds: DefaultReadinessPeriodSeconds,
readinessFailureThreshold: DefaultReadinessFailureThreshold,
},
{
in: "hello-probes.yaml",
want: "hello-probes.yaml.injected",
Expand All @@ -137,6 +148,7 @@ func TestIntoResourceFile(t *testing.T) {
readinessInitialDelaySeconds: DefaultReadinessInitialDelaySeconds,
readinessPeriodSeconds: DefaultReadinessPeriodSeconds,
readinessFailureThreshold: DefaultReadinessFailureThreshold,
rewriteAppHTTPProbe: true,
},
{
in: "hello.yaml",
Expand Down Expand Up @@ -219,6 +231,7 @@ func TestIntoResourceFile(t *testing.T) {
readinessInitialDelaySeconds: DefaultReadinessInitialDelaySeconds,
readinessPeriodSeconds: DefaultReadinessPeriodSeconds,
readinessFailureThreshold: DefaultReadinessFailureThreshold,
rewriteAppHTTPProbe: true,
},
{
in: "hello-readiness-multi.yaml",
Expand All @@ -229,6 +242,7 @@ func TestIntoResourceFile(t *testing.T) {
readinessInitialDelaySeconds: DefaultReadinessInitialDelaySeconds,
readinessPeriodSeconds: DefaultReadinessPeriodSeconds,
readinessFailureThreshold: DefaultReadinessFailureThreshold,
rewriteAppHTTPProbe: true,
},
{
in: "multi-init.yaml",
Expand Down Expand Up @@ -530,6 +544,7 @@ func TestIntoResourceFile(t *testing.T) {
ReadinessInitialDelaySeconds: c.readinessInitialDelaySeconds,
ReadinessPeriodSeconds: c.readinessPeriodSeconds,
ReadinessFailureThreshold: c.readinessFailureThreshold,
RewriteAppHTTPProbe: c.rewriteAppHTTPProbe,
}
if c.imagePullPolicy != "" {
params.ImagePullPolicy = c.imagePullPolicy
Expand Down
1 change: 1 addition & 0 deletions pilot/pkg/kube/inject/mesh.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const (
[[- $readinessPeriodValue := (annotation .ObjectMeta $readinessPeriodKey "{{ .ReadinessPeriodSeconds }}") ]]
[[- $readinessFailureThresholdValue := (annotation .ObjectMeta $readinessFailureThresholdKey {{ .ReadinessFailureThreshold }}) -]]
[[- $readinessApplicationPortsValue := (annotation .ObjectMeta $readinessApplicationPortsKey (applicationPorts .Spec.Containers)) -]]
rewriteAppHTTPProbe: {{ .RewriteAppHTTPProbe }}
initContainers:
- name: istio-init
image: {{ .InitImage }}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
creationTimestamp: null
name: hello
spec:
replicas: 7
strategy: {}
template:
metadata:
annotations:
sidecar.istio.io/status: '{"version":"","initContainers":["istio-init"],"containers":["istio-proxy"],"volumes":["istio-envoy","istio-certs"],"imagePullSecrets":null}'
creationTimestamp: null
labels:
app: hello
tier: backend
track: stable
spec:
containers:
- image: fake.docker.io/google-samples/hello-go-gke:1.0
livenessProbe:
httpGet:
port: http
name: hello
ports:
- containerPort: 80
name: http
readinessProbe:
httpGet:
port: 3333
resources: {}
- image: fake.docker.io/google-samples/hello-go-gke:1.0
livenessProbe:
httpGet:
port: http
name: world
ports:
- containerPort: 90
name: http
readinessProbe:
exec:
command:
- cat
- /tmp/healthy
resources: {}
- args:
- proxy
- sidecar
- --configPath
- /etc/istio/proxy
- --binaryPath
- /usr/local/bin/envoy
- --serviceCluster
- hello.default
- --drainDuration
- 2s
- --parentShutdownDuration
- 3s
- --discoveryAddress
- istio-pilot:15007
- --connectTimeout
- 1s
- --statsdUdpAddress
- ""
- --proxyAdminPort
- "15000"
- --controlPlaneAuthPolicy
- NONE
- --statusPort
- "15020"
- --applicationPorts
- 80,90
env:
- name: POD_NAME
valueFrom:
fieldRef:
fieldPath: metadata.name
- name: POD_NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: INSTANCE_IP
valueFrom:
fieldRef:
fieldPath: status.podIP
- name: ISTIO_META_POD_NAME
valueFrom:
fieldRef:
fieldPath: metadata.name
- name: ISTIO_META_INTERCEPTION_MODE
value: REDIRECT
image: docker.io/istio/proxyv2:unittest
imagePullPolicy: IfNotPresent
name: istio-proxy
ports:
- containerPort: 15090
name: http-envoy-prom
protocol: TCP
readinessProbe:
failureThreshold: 30
httpGet:
path: /healthz/ready
port: 15020
initialDelaySeconds: 1
periodSeconds: 2
resources:
requests:
cpu: 10m
securityContext:
readOnlyRootFilesystem: true
volumeMounts:
- mountPath: /etc/istio/proxy
name: istio-envoy
- mountPath: /etc/certs/
name: istio-certs
readOnly: true
initContainers:
- args:
- -p
- "15001"
- -u
- "1337"
- -m
- REDIRECT
- -i
- '*'
- -x
- ""
- -b
- 80,90
- -d
- "15020"
image: docker.io/istio/proxy_init:unittest
imagePullPolicy: IfNotPresent
name: istio-init
resources: {}
securityContext:
capabilities:
add:
- NET_ADMIN
volumes:
- emptyDir:
medium: Memory
name: istio-envoy
- name: istio-certs
secret:
optional: true
secretName: istio.default
status: {}
---
1 change: 1 addition & 0 deletions pilot/pkg/kube/inject/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ func (wh *Webhook) inject(ar *v1beta1.AdmissionReview) *v1beta1.AdmissionRespons
}
}

// TODO(incfly): plumbing wh.sidecarConfig.RewriteHTTPPRobe to the injectionData.
spec, status, err := injectionData(wh.sidecarConfig.Template, wh.sidecarTemplateVersion, &pod.ObjectMeta, &pod.Spec, &pod.ObjectMeta, wh.meshConfig.DefaultConfig, wh.meshConfig) // nolint: lll
if err != nil {
log.Infof("Injection data: err=%v spec=%v\n", err, status)
Expand Down