Update templates for initialzier and pilot external admission webhook by ayj · Pull Request #793 · istio/istio

ayj · 2017-09-14T00:25:16Z

Update RBAC templates for sidecar initialzier and pilot external
admission webhook
Update istio-pilot template with external admission webook for
config validation. Self-registration is gated on availability of
secret with cert/key for signing requests. Secret is not created by
default due to GKE 1.7.x bug which requires additional external
IP. Workaround script is included for curious users to test but is not
integrated in e2e tests.
Initializer deployment is in its own file. The expectation is that
this file will be included in cluster-wide specific tests where the
initializer will not interfere with other tests in a shared cluster.

istio/old_pilot_repo#1153 (or equivalent) is required
for sidecar configuration per-namespace. The following command
an be temporarily used in cluster-wide specific tests to propogate
sidecar configmap to app namespaces.

kubectl -n <new-namespace> create cm istio \ 
    --from-file=<(kubectl -n istio-system get cm istio -o jsonpath={.data}

Release note:

NONE

1) Update RBAC templates for sidecar initialzier and pilot external admission webhook 2) Update istio-pilot template with external admission webook for config validation. Self-registration is gated on availability of secret with cert/key for signing requests. Secret is not created by default due to GKE 1.7.x bug which requires additional external IP. Workaround script is included for curious users to test but is not integrated in e2e tests. 3) Initializer deployment is in its own file. The expectation is that this file will be included in cluster-wide specific tests where the initializer will not interfere with other tests in a shared cluster. istio/old_pilot_repo#1153 (or equivalent) is required for sidecar configuration per-namespace. The following command can be temporarily used in cluster-wide specific tests to propogate sidecar configmap to app namespaces. kubectl -n <new-namespace> create cm istio --from-file=<(kubectl -n istio-system get cm istio -o jsonpath={.data}

andraxylia · 2017-09-14T01:31:01Z

install/kubernetes/istio-auth.yaml

        image: gcr.io/istio-testing/pilot:5acaa9e26e7d9b239cbc72da63f5f9a3dac5a54e
        imagePullPolicy: IfNotPresent
-        args: ["discovery", "-v", "2"]
+        args: ["discovery", "-v", "2", "--admission-service", "istio-pilot-external"]


So this will be on even in non cluster-wide? Is it for validation?

Yes. This shouldn't be a problem because the admission control service only self-registers when the dependent secret w/certs is created. That secret is not created by default due to GKE 1.7.x bug (see workaround script).

andraxylia · 2017-09-14T01:35:20Z

install/kubernetes/istio-sidecar-initializer.yaml

+          - deployments
+          - statefulsets
+          - jobs
+          - daemonsets


Do we need other workloads?

Initializer doesn't support cronjob yet.

replicatsets and replicationcontrollers had some issues with current initializer support that aren't resolved yet. Deployments are recommended instead of using replicasets or replicationctonrollers (see here)

pod also did not work but isn't recommended on its own since some controllers will complain if the pod template spec in the deployment doesn't match the pod spec itself.

…-for-initializer-and-pilot-admission-webhook

andraxylia · 2017-09-15T22:37:39Z

Let's go with the plan we discussed in the meeting and close on this:

validation webhooks on by default
initializer in a separate yaml file

andraxylia · 2017-09-15T22:39:13Z

My only concern is with the name, sidecar initializer somehow makes me think it runs as a sidecar. I would call it simply istio-initializer.

andraxylia · 2017-09-15T22:39:39Z

install/kubernetes/istio-auth.yaml

+  name: istio-sidecar-initializer-admin-role-binding-istio-system
+subjects:
+- kind: ServiceAccount
+  name: istio-sidecar-initializer-service-account


Remove sidecar from the name

The naming was derived from kelsey heightower's envoy-initializer, i.e. s/envoy/istio-sidecar. I can change it if its confusing, but I should start with istio/pilot first. Also, any chance istio will use other initializers?

We never know what future holds in store, there may be other initializers. The name is still confusing. We call the proxy a side car proxy, so I expect a sidecar initializer to be a sidecar...

How about istio-proxy-initializer or istio-proxy-injector?

andraxylia · 2017-09-15T22:41:50Z

install/updateVersion.sh

  cp $ISTIO $ISTIO_AUTH
  sed -i=.bak "s/# authPolicy: MUTUAL_TLS/authPolicy: MUTUAL_TLS/" $ISTIO_AUTH

+  # TODO(andra/jason) - copy this into ${ISTIO_CLUSTER_WIDE} after initial cluster-wide test is verified.


remove todo

andraxylia · 2017-09-15T22:44:11Z

install/kubernetes/istio.yaml

    name: http-discovery
-  - port: 8081
-    name: http-apiserver
+  - port: 443


Is this the only change needed to enable webhooks?

No, users need to run install/kubernetes/external-admission-webhook-gke-1-7-x-workaround.sh. This concerts the istio-pilot service to type=LoadBalancer, generates certs, and plumbs them through to k8s secret. The secret creation triggers the admission webhook to self-register.

We will need to document the extra step somewhere on istio.io.

…-admission-webhook

istio-merge-robot · 2017-09-16T00:58:52Z

@ayj PR needs rebase

andraxylia · 2017-09-16T01:00:14Z

/lgtm

we can do the renaming in a separate commit, if you fix the conflicts and find a window to merge

gyliu513 · 2017-09-18T03:08:39Z

install/kubernetes/istio-sidecar-initializer.yaml

+metadata:
+  name: istio-sidecar-initializer
+  namespace: istio-system
+  annotations:


Do we really need the annotation here? We already defined the initializers as [], so seems here do not need the annotations.

initializers: pending: []

gyliu513 · 2017-09-18T03:13:47Z

install/kubernetes/istio-sidecar-initializer.yaml

+      labels:
+        istio: sidecar-initializer
+      annotations:
+        sidecar.istio.io/inject: "false"


ditto as above

gyliu513 · 2017-09-18T03:17:28Z

This also fixed #636

…-for-initializer-and-pilot-admission-webhook

…bhook' of github.com:ayj/istio into update-templates-for-initializer-and-pilot-admission-webhook

istio-merge-robot · 2017-09-18T19:16:50Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @andraxylia @ayj

The use of `sidecar` in the name of the initializer seems to be undesirable (see istio/istio#793 (comment)). Rename to simplify `istio-initializer` or `initializer` throughout.

andraxylia · 2017-09-18T21:10:08Z

install/kubernetes/addons/grafana.yaml

  template:
    metadata:
-      annotations:
-        alpha.istio.io/sidecar: ignore


This is generated, can you remove from grafana.yaml.tmpl and re-run updateVersion.sh

That should be the case already. Lines 20-21 show the result of the new annotation from the template.

Nvm, it's good, somehow I had missed the .tmpl files.

andraxylia · 2017-09-18T21:12:07Z

install/kubernetes/istio-initializer.yaml

+  config: |-
+    policy: "enabled"
+    namespaces: [""] # everything, aka v1.NamepsaceAll, aka cluster-wide
+    initializerName: "sidecar.initializer.istio.io"


Do we keep sidecar here?

That's up to. It requires another corresponding change in istio/pilot

…-admission-webhook

…-for-initializer-and-pilot-admission-webhook

…bhook' of github.com:ayj/istio into update-templates-for-initializer-and-pilot-admission-webhook

andraxylia · 2017-09-18T22:17:53Z

/retest all

ayj · 2017-09-18T23:19:21Z

/retest all

andraxylia · 2017-09-18T23:51:46Z

/lgtm

istio-merge-robot · 2017-09-19T00:22:59Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @andraxylia @ayj

andraxylia · 2017-09-19T00:45:24Z

/lgtm

istio-merge-robot · 2017-09-19T00:46:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andraxylia

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

Details

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

~~OWNERS~~ [andraxylia]

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

istio-merge-robot · 2017-09-19T00:48:19Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-09-19T01:13:10Z

Automatic merge from submit-queue

The use of `sidecar` in the name of the initializer seems to be undesirable (see istio/istio#793 (comment)). Rename to simplify `istio-initializer` or `initializer` throughout.

…#793) Automatic merge from submit-queue Update templates for initialzier and pilot external admission webhook 1) Update RBAC templates for sidecar initialzier and pilot external admission webhook 2) Update istio-pilot template with external admission webook for config validation. Self-registration is gated on availability of secret with cert/key for signing requests. Secret is not created by default due to GKE 1.7.x bug which requires additional external IP. Workaround script is included for curious users to test but is not integrated in e2e tests. 3) Initializer deployment is in its own file. The expectation is that this file will be included in cluster-wide specific tests where the initializer will not interfere with other tests in a shared cluster. istio/old_pilot_repo#1153 (or equivalent) is required for sidecar configuration per-namespace. The following command an be temporarily used in cluster-wide specific tests to propogate sidecar configmap to app namespaces. kubectl -n <new-namespace> create cm istio \ --from-file=<(kubectl -n istio-system get cm istio -o jsonpath={.data} **Release note**: ```release-note NONE ``` Former-commit-id: c6bccb9

…istio#793) Automatic merge from submit-queue Update templates for initialzier and pilot external admission webhook 1) Update RBAC templates for sidecar initialzier and pilot external admission webhook 2) Update istio-pilot template with external admission webook for config validation. Self-registration is gated on availability of secret with cert/key for signing requests. Secret is not created by default due to GKE 1.7.x bug which requires additional external IP. Workaround script is included for curious users to test but is not integrated in e2e tests. 3) Initializer deployment is in its own file. The expectation is that this file will be included in cluster-wide specific tests where the initializer will not interfere with other tests in a shared cluster. istio/old_pilot_repo#1153 (or equivalent) is required for sidecar configuration per-namespace. The following command an be temporarily used in cluster-wide specific tests to propogate sidecar configmap to app namespaces. kubectl -n <new-namespace> create cm istio \ --from-file=<(kubectl -n istio-system get cm istio -o jsonpath={.data} **Release note**: ```release-note NONE ``` Former-commit-id: c6bccb9

…#793) Automatic merge from submit-queue Update templates for initialzier and pilot external admission webhook 1) Update RBAC templates for sidecar initialzier and pilot external admission webhook 2) Update istio-pilot template with external admission webook for config validation. Self-registration is gated on availability of secret with cert/key for signing requests. Secret is not created by default due to GKE 1.7.x bug which requires additional external IP. Workaround script is included for curious users to test but is not integrated in e2e tests. 3) Initializer deployment is in its own file. The expectation is that this file will be included in cluster-wide specific tests where the initializer will not interfere with other tests in a shared cluster. istio/old_pilot_repo#1153 (or equivalent) is required for sidecar configuration per-namespace. The following command an be temporarily used in cluster-wide specific tests to propogate sidecar configmap to app namespaces. kubectl -n <new-namespace> create cm istio \ --from-file=<(kubectl -n istio-system get cm istio -o jsonpath={.data} **Release note**: ```release-note NONE ``` Former-commit-id: c6bccb9

ayj requested a review from andraxylia September 14, 2017 00:25

googlebot added the cla: yes label Sep 14, 2017

istio-merge-robot added the release-note-none label Sep 14, 2017

andraxylia reviewed Sep 14, 2017

View reviewed changes

ayj added 2 commits September 14, 2017 09:37

Merge branch 'master' of github.com:istio/istio into update-templates…

f593d5e

…-for-initializer-and-pilot-admission-webhook

update istio deployment annotations for sidecar injection

3cf9e1d

andraxylia reviewed Sep 15, 2017

View reviewed changes

Merge branch 'master' into update-templates-for-initializer-and-pilot…

4c6063d

…-admission-webhook

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 16, 2017

istio-testing assigned andraxylia Sep 16, 2017

istio-testing added the lgtm label Sep 16, 2017

istio-merge-robot added the approved label Sep 16, 2017

gyliu513 reviewed Sep 18, 2017

View reviewed changes

ayj added 5 commits September 18, 2017 11:22

rename to istio-initializer and update golang e2e framework

36add8c

Merge branch 'master' of github.com:istio/istio into update-templates…

08a0eac

…-for-initializer-and-pilot-admission-webhook

update templates

6afd02f

fixes

3a75467

Merge branch 'update-templates-for-initializer-and-pilot-admission-we…

e658c03

…bhook' of github.com:ayj/istio into update-templates-for-initializer-and-pilot-admission-webhook

istio-merge-robot removed lgtm needs-rebase Indicates a PR needs to be rebased before being merged labels Sep 18, 2017

andraxylia reviewed Sep 18, 2017

View reviewed changes

andraxylia approved these changes Sep 18, 2017

View reviewed changes

andraxylia and others added 4 commits September 18, 2017 14:16

Merge branch 'master' into update-templates-for-initializer-and-pilot…

b6b2162

…-admission-webhook

Merge branch 'master' of github.com:istio/istio into update-templates…

1b0db2d

…-for-initializer-and-pilot-admission-webhook

refresh versions

d5658ac

Merge branch 'update-templates-for-initializer-and-pilot-admission-we…

bab3c43

…bhook' of github.com:ayj/istio into update-templates-for-initializer-and-pilot-admission-webhook

fix cluster-wide regex substitution

f17b48a

istio-testing added the lgtm label Sep 18, 2017

fix typo

a004f53

istio-merge-robot removed the lgtm label Sep 19, 2017

istio-testing added the lgtm label Sep 19, 2017

istio-merge-robot merged commit c6bccb9 into istio:master Sep 19, 2017

ldemailly mentioned this pull request Sep 19, 2017

Enable update dependency for proxy debian package #856

Merged

Conversation

ayj commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andraxylia commented Sep 15, 2017

Uh oh!

andraxylia commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Sep 16, 2017

Uh oh!

andraxylia commented Sep 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyliu513 commented Sep 18, 2017

Uh oh!

istio-merge-robot commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andraxylia commented Sep 18, 2017

Uh oh!

ayj commented Sep 18, 2017

Uh oh!

andraxylia commented Sep 18, 2017

Uh oh!

istio-merge-robot commented Sep 19, 2017

Uh oh!

andraxylia commented Sep 19, 2017

Uh oh!

istio-merge-robot commented Sep 19, 2017

Uh oh!

istio-merge-robot commented Sep 19, 2017

Uh oh!

istio-merge-robot commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants