helm chart failed with sidecar injector #2957

linsun · 2018-01-30T03:15:33Z

#2935, able to get a dry run working helm install --dry-run --debug istio, will work on clean up tmr.

sdake · 2018-01-30T03:28:04Z

@linsun master(7ce862d) works for me (with helm --dry-run) with Kubernetes 1.9.2 after running updateVersion.sh -a $HUB,$TAG where the images are built from the same commit. Running updateVersion.sh is mandatory - or other problems are reported via Helm. My kubeadm configuration without Mutating webhook is:

apiVersion: kubeadm.k8s.io/v1alpha1
kind: MasterConfiguration
apiServerExtraArgs:
  runtime-config: admissionregistration.k8s.io/v1beta1
  feature-gates: AllAlpha=true
pod-network-cidr: 192.168.0.0/16

I'm sorting out how to enable the mutating webhook now.

Is there a defined policy on a minimum version pin for Kubernetes for the installation tools? It appears master has been modified to work with kubernetes>=1.9.0. updateVersion.sh indicates that the configs are designed to work with Kubernetes 1.9.0+. Some of the changes in this PR are changing from released stable versions to beta versions. It would help me to understand if such a policy exists.

Cheers
-steve

linsun · 2018-01-30T14:02:15Z

So we support k8s 1.7, 1.8 and 1.9. Only mutating web hook is supported in 1.9 as an alpha feature. The default helm chart should be targeted for 1.7 or 1.8 with manual injection, with a flag to allow users to enable mutating web hooks on 1.9 if they choose to.

sdake · 2018-01-30T14:53:11Z

@linsun thanks for the response. Note the initializer worked for me in 1.8 without manual injection. Does the new injection code work on 1.8, or only 1.9?

I have searched a bit for mutating web hook on google, and can't seem to find a definition of what it does in documentation. Do you happen to have a link handy?

andraxylia · 2018-01-30T16:42:09Z

The mutating webhooks require k8s 1.9. Unfortunately, the initializers have been deprecated in 1.9, so we were forced to deprecate them as well. We will have to use manual injection for k8s < 1.9.

https://istio.io/docs/setup/kubernetes/sidecar-injection.html

ayj · 2018-01-30T16:50:13Z

See https://github.com/kubernetes/community/blob/master/contributors/design-proposals/api-machinery/admission-control-webhooks.md for background information on mutating webhooks.

https://kubernetes.io/docs/admin/extensible-admission-controllers/#external-admission-webhooks hasn't been updated yet for Mutating webhooks and the admissionregistration.k8s.io/v1beta1` API group.

ayj · 2018-01-30T16:51:26Z

install/kubernetes/helm/istio/charts/mixer/templates/clusterrole.yaml

@@ -1,5 +1,5 @@
 {{- if .Values.global.rbacEnabled }}
-apiVersion: rbac.authorization.k8s.io/v1
+apiVersion: rbac.authorization.k8s.io/v1beta1


Why are we regressing on version here?

because we have clusters on 1.7.

if you look at istio-auth.yaml, we are also using v1beta1

linsun · 2018-01-30T16:57:13Z

Able to deploy helm istio chart. This is work in progress but could we get it in so that we can also merge @luisyonaldo too?

ayj · 2018-01-30T17:04:27Z

install/kubernetes/helm/istio/charts/sidecar-injector/templates/mutatingwebhook.yaml

@@ -0,0 +1,29 @@
+{{- if .Values.global.sidecarInjectEnabled }}


Should service and deployment use the same sidecarInjectEnabled guard?

there is also a global.sidecar.enabled flag - which probably does the same thing as thing as this conditional. I'm unclear if this helm chart has been tested on 1.9 with the webhooks (or simply 1.7).

@ayj I think that makes sense. I'll do it in a separate PR.

sdake · 2018-01-30T17:16:46Z

We have several defects in this Helm chart as it stands. I also think all of @linsun's work is an improvement given the policy of supporting kubernetes>=1.7. Trying to fix all the various problems in one patch stream seems problematic (harder to review and test as there is no gating on the helm chart).

@linsun can you confirm if you have tested on 1.9? If not, I'll take a look (thanks @ayj and @andraxylia for the pointers on injection).

sdake · 2018-01-30T17:16:52Z

/lgtm

sdake

Helm chart needs attention - however - to distribute the work, I agree with @linsun we should merge this patch as is (its an improvement and in line with Kubernetes 1.7 as a minimum pin policy).

Cheers
-steve

linsun · 2018-01-30T17:25:54Z

Thank you @sdake - I don't have a 1.9 env, it would be great if you could take up on that.

I'll work on a gating test cc @luisyonaldo

linsun · 2018-01-30T17:27:24Z

Just make sure we are all on the same page, the goal is istio works on manual injection for 1.7 and 1.8, and manual & auto injection on 1.9+ while auto injection starts as alpha/dev feature for now till it is improved. cc @sdake @luisyonaldo @ayj

ayj · 2018-01-30T17:28:07Z

sgtm

luisyonaldo · 2018-01-30T17:30:23Z

Current logic check that you have sidecar-injector enabled and that you have mutation we hook api enabled. If not the only manual injection is possible. Haven't tested the auto injection yet.

ayj

/approve

istio-merge-robot · 2018-01-30T17:39:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ayj, sdake
We suggest the following additional approver: ldemailly

Assign the PR to them by writing /assign @ldemailly in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

* helm charts fail when initializer is deployed #2935 * helm charts fail when initializer is deployed #2935 * additional tweaks and merge some changes from luisyonaldo thanks

This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: 2656f34 Update Envoy SHA to latest with MetricImpl optimizations (release-1.0). (istio#1939) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (istio#4190) 36809d80a fuzz: coverage profile generation instructions. (istio#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075) 05c0d52d3 build: use clang-6.0. (istio#4168) 01f403ec4 thrift_proxy: introduce header transport (istio#4082) 564d256fb tcp: allow connection pool callers to store protocol state (istio#4131) 3e1d643b9 configs: match latest API changes (istio#4185) bc6a10c2f Fix wrong mock function name. (istio#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182) 3d1325e89 Converting envoy configs to V2 (istio#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157) 71152b710 ratelimit: Add ratelimit custom response headers (istio#4015) 306287418 ssl: make Ssl::Connection const everywhere (istio#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130) 8c189a552 Make over provisioning factor configurable (istio#4003) 6c08fb43c Making test less flaky (istio#4149) 592775b7b fuzz: bare bones HCM fuzzer. (istio#4118) For istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (istio#1938) a32a587 Add a check cache test for string map sub keys (istio#1931) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (istio#4190) 36809d80a fuzz: coverage profile generation instructions. (istio#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075) 05c0d52d3 build: use clang-6.0. (istio#4168) 01f403ec4 thrift_proxy: introduce header transport (istio#4082) 564d256fb tcp: allow connection pool callers to store protocol state (istio#4131) 3e1d643b9 configs: match latest API changes (istio#4185) bc6a10c2f Fix wrong mock function name. (istio#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182) 3d1325e89 Converting envoy configs to V2 (istio#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157) 71152b710 ratelimit: Add ratelimit custom response headers (istio#4015) 306287418 ssl: make Ssl::Connection const everywhere (istio#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130) 8c189a552 Make over provisioning factor configurable (istio#4003) 6c08fb43c Making test less flaky (istio#4149) 592775b7b fuzz: bare bones HCM fuzzer. (istio#4118) For istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

…). (#8161) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: 2656f34 Update Envoy SHA to latest with MetricImpl optimizations (release-1.0). (#1939) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (#4190) 36809d80a fuzz: coverage profile generation instructions. (#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (#4075) 05c0d52d3 build: use clang-6.0. (#4168) 01f403ec4 thrift_proxy: introduce header transport (#4082) 564d256fb tcp: allow connection pool callers to store protocol state (#4131) 3e1d643b9 configs: match latest API changes (#4185) bc6a10c2f Fix wrong mock function name. (#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182) 3d1325e89 Converting envoy configs to V2 (#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157) 71152b710 ratelimit: Add ratelimit custom response headers (#4015) 306287418 ssl: make Ssl::Connection const everywhere (#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (#4130) 8c189a552 Make over provisioning factor configurable (#4003) 6c08fb43c Making test less flaky (#4149) 592775b7b fuzz: bare bones HCM fuzzer. (#4118) For #7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (#1938) a32a587 Add a check cache test for string map sub keys (#1931) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (#4190) 36809d80a fuzz: coverage profile generation instructions. (#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (#4075) 05c0d52d3 build: use clang-6.0. (#4168) 01f403ec4 thrift_proxy: introduce header transport (#4082) 564d256fb tcp: allow connection pool callers to store protocol state (#4131) 3e1d643b9 configs: match latest API changes (#4185) bc6a10c2f Fix wrong mock function name. (#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182) 3d1325e89 Converting envoy configs to V2 (#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157) 71152b710 ratelimit: Add ratelimit custom response headers (#4015) 306287418 ssl: make Ssl::Connection const everywhere (#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (#4130) 8c189a552 Make over provisioning factor configurable (#4003) 6c08fb43c Making test less flaky (#4149) 592775b7b fuzz: bare bones HCM fuzzer. (#4118) For #7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

linsun added 2 commits January 29, 2018 22:09

helm charts fail when initializer is deployed #2935

26c7823

helm charts fail when initializer is deployed #2935

25c54cc

linsun requested a review from a team January 30, 2018 03:15

googlebot added the cla: yes label Jan 30, 2018

linsun mentioned this pull request Jan 30, 2018

helm: add add-ons #2965

Merged

ayj reviewed Jan 30, 2018

View reviewed changes

additional tweaks and merge some changes from luisyonaldo thanks

618f6f2

linsun changed the title ~~WIP helm chart failed with sidecar injector~~ helm chart failed with sidecar injector Jan 30, 2018

ayj reviewed Jan 30, 2018

View reviewed changes

istio-testing assigned sdake Jan 30, 2018

istio-testing added the lgtm label Jan 30, 2018

sdake approved these changes Jan 30, 2018

View reviewed changes

ayj approved these changes Jan 30, 2018

View reviewed changes

linsun merged commit 1913c04 into master Jan 30, 2018

linsun deleted the linsun-1 branch January 30, 2018 17:51

This was referenced Aug 23, 2018

Update Proxy SHA to latest with MetricImpl optimizations. #8160

Merged

Update Proxy SHA to latest with MetricImpl optimizations (release-1.0). #8161

Merged

		@@ -0,0 +1,29 @@
		{{- if .Values.global.sidecarInjectEnabled }}

helm chart failed with sidecar injector #2957

helm chart failed with sidecar injector #2957

Uh oh!

Conversation

linsun commented Jan 30, 2018

Uh oh!

sdake commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linsun commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdake commented Jan 30, 2018

Uh oh!

andraxylia commented Jan 30, 2018

Uh oh!

ayj commented Jan 30, 2018

Uh oh!

ayj Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

linsun Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

linsun Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

linsun commented Jan 30, 2018

Uh oh!

ayj Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

sdake Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linsun Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

sdake commented Jan 30, 2018

Uh oh!

sdake commented Jan 30, 2018

Uh oh!

sdake left a comment

Choose a reason for hiding this comment

Uh oh!

linsun commented Jan 30, 2018

Uh oh!

linsun commented Jan 30, 2018

Uh oh!

ayj commented Jan 30, 2018

Uh oh!

luisyonaldo commented Jan 30, 2018

Uh oh!

ayj left a comment

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Jan 30, 2018

Uh oh!

Uh oh!

sdake commented Jan 30, 2018 •

edited

Loading

linsun commented Jan 30, 2018 •

edited

Loading

sdake Jan 30, 2018 •

edited

Loading