@morvencao: The following tests failed, say /retest to rerun them all:

@sdake can you approve this?

All tests are passing which is good.

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: linsun, morvencao

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

The pull request process is described here

@sdake can you approve this?

All tests are passing which is good.

/lgtm

@linsun I see the tests are passing, but this doesn't look correct in the helm template case. with helm install, everything should be fine as the admin SA is used.

@morvencao can you test the helm template case?

Perhaps there is a different bug in this chart that the correct sa is not used for the job and in-cluster apps have some type of admin context?

Cheers
-steve

@linsun placing a hold on this PR. I don't believe we actually validate that istio-init is used in both the helm template and helm install cases. I'd really like to see a manual test of this change with both modes.

Also, can you please submit to master, and we can backport?

Cheers
-steve

@sdake Sorry for the late response
Have tested the helm template case locally:

root@master:~/istio# helm template install/kubernetes/helm/istio-init --name istio-init --namespace istio-system > istio-init.yaml
root@master:~/istio# kubectl create ns istio-system
namespace/istio-system created
root@master:~/istio# kubectl apply -f istio-init.yaml
configmap/istio-crd-10 created
configmap/istio-crd-11 created
serviceaccount/istio-init-service-account created
clusterrole.rbac.authorization.k8s.io/istio-init-istio-system created
clusterrolebinding.rbac.authorization.k8s.io/istio-init-admin-role-binding-istio-system created
job.batch/istio-init-crd-10 created
job.batch/istio-init-crd-11 created
root@master:~/istio# kubectl -n istio-system get pod
NAME                      READY     STATUS              RESTARTS   AGE
istio-init-crd-10-l8xpm   0/1       ContainerCreating   0          11s
istio-init-crd-11-dkhsd   0/1       ContainerCreating   0          11s
root@master:~/istio# kubectl -n istio-system get pod
NAME                      READY     STATUS      RESTARTS   AGE
istio-init-crd-10-l8xpm   0/1       Completed   0          2m
istio-init-crd-11-dkhsd   0/1       Completed   0          2m
root@master:~/istio# kubectl get crd | grep istio.io | wc -l
53

My understanding is that the configMap is mounted into the pod, so the kubectl in the pod doesn't need the permission to list/watch the configmap from kube-api-server, on the contrary, for the CRDs, the kubectl does need the permission to apply them into the kube-api-server.

Anyway, it would be all right to merge to master to avoid break the 1.1.x release.

Close this in favor of #12978

@sdake Sorry for the late response
Have tested the helm template case locally:

root@master:~/istio# helm template install/kubernetes/helm/istio-init --name istio-init --namespace istio-system > istio-init.yaml
root@master:~/istio# kubectl create ns istio-system
namespace/istio-system created
root@master:~/istio# kubectl apply -f istio-init.yaml
configmap/istio-crd-10 created
configmap/istio-crd-11 created
serviceaccount/istio-init-service-account created
clusterrole.rbac.authorization.k8s.io/istio-init-istio-system created
clusterrolebinding.rbac.authorization.k8s.io/istio-init-admin-role-binding-istio-system created
job.batch/istio-init-crd-10 created
job.batch/istio-init-crd-11 created
root@master:~/istio# kubectl -n istio-system get pod
NAME                      READY     STATUS              RESTARTS   AGE
istio-init-crd-10-l8xpm   0/1       ContainerCreating   0          11s
istio-init-crd-11-dkhsd   0/1       ContainerCreating   0          11s
root@master:~/istio# kubectl -n istio-system get pod
NAME                      READY     STATUS      RESTARTS   AGE
istio-init-crd-10-l8xpm   0/1       Completed   0          2m
istio-init-crd-11-dkhsd   0/1       Completed   0          2m
root@master:~/istio# kubectl get crd | grep istio.io | wc -l
53

My understanding is that the configMap is mounted into the pod, so the kubectl in the pod doesn't need the permission to list/watch the configmap from kube-api-server, on the contrary, for the CRDs, the kubectl does need the permission to apply them into the kube-api-server.

Anyway, it would be all right to merge to master to avoid break the 1.1.x release.

your right, mounted, no need for CRBs for configmaps.

Cheers
-steve