Take2 on config unification by costinm · Pull Request #3594 · istio/istio

costinm · 2018-02-18T05:52:25Z

Changes:

move values.yaml to the normal place ( so updateValues is not needed for normal operation, --set in helm
is more natural )
added few targets to test generation of istio.yaml from helm
use the same names as in the (release) istio.yaml - we want to be able to gracefully upgrade
removed some of the configuration that is not supported ( like arbitrary names )
use the single repo/tag - we don't really test or support getting each piece at a different version,
and the UX is pretty bad (user having to type the tag 7 times)

I did more manual diffs - they're very time consuming, fixed few things missing (ingress certs), but
we'll need more people and eyes to get them in sync and make sure nothing gets lost.

istio-merge-robot · 2018-02-18T05:53:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: sebastienvas

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

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

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

sdake

nice work @costinm! I don't see any glaring errors, although would like to give it a test which I'll do today and report back.

sdake · 2018-02-22T17:38:01Z

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

            mountPath: /etc/statsd
        - name: {{ template "mixer.name" . }}
-          image: "{{ .Values.global.mixer.repository }}:{{ .Values.global.mixer.tag }}"
+          image: "{{ .Values.global.hub }}/mixer:{{ .Values.global.tag }}"


this is a good improvement.

sdake · 2018-02-22T17:41:46Z

install/kubernetes/helm/istio/charts/ingress/templates/deployment.yaml

          - "2"
          - --discoveryAddress
-          - {{ .Release.Name }}-pilot:15003
+          - istio-pilot:15003


yup this is much better.

sdake · 2018-02-22T17:43:18Z

install/kubernetes/helm/istio/charts/ingress/templates/service.yaml

    heritage: {{ .Release.Service }}
    istio: ingress
 spec:
-{{- if .Values.service.nodePort.enabled }}


the nodePort.enabled feature is to permit people to eval if they lack a load balancer. I think the current istio generated manifest has this nodeport feature commented out. I think the conditional is warranted.

Super useful for minikube - but instead of conditional I added 2 services, one with node port
and one load balancer. This way we can have both.

sdake · 2018-02-22T17:43:55Z

install/kubernetes/helm/istio/charts/ingress/templates/service.yaml

+    istio: ingress
+
+---
+{{- if .Values.service.nodePort.enabled }}


cool if this works, wfm.

sdake · 2018-02-22T17:46:49Z

install/kubernetes/helm/istio/values.yaml

+# Common settings.
 global:
+  # Default tag for Istio images.
+  hub: istio


is this hub correct? Wouldn't it be docker.io/istio or gcr.io/istio?

docker.io is added automatically, and it seems to work.

would be good to specify it out as I'm not sure it'd work when custom registry is used in various cloud provider. also for clarity :)

Will do it in a separate PR, more changes will likely be needed before we change the tests.

costinm · 2018-02-22T17:54:09Z

install/kubernetes/helm/istio/charts/servicegraph/templates/deployment.yaml

-            httpGet:
-              path: /
-              port: {{ .Values.service.internalPort }}
+#          livenessProbe:


commented out because it fails - 404. We need to add it back, but not sure what URL.

sdake · 2018-02-23T15:01:41Z

When I tested, I accidentally deployed to the istio namespace rather than the istio-system namespace which results in:

sdake@falkor-07:~/go/src/istio.io/istio/samples/bookinfo/kube$ istioctl kube-inject -i istio -f bookinfo.yaml -o bookfino-injected.yaml
Error: failed to convert to proto. unknown field "mixerCheckServer" in v1alpha1.MeshConfig
sdake@falkor-07:~/go/src/istio.io/istio/samples/bookinfo/kube$ istioctl version
Version: 0.5.1
GitRevision: 30acfe6528107ea333543309095659b93364b30d
User: root@2e4a18076b04
Hub: docker.io/istio
GolangVersion: go1.9
BuildStatus: Clean

I'll try istio-system. In the past, the helm charts have always deployed by default to the default namespace. IOW there may be a latent issue with the helm chart related to custom namespaces. Otherwise, things look good!

costinm · 2018-02-23T15:16:51Z

Interesting point - we typically recommend using istio-system, and any upgrade from the existing template to the new ones would go there. The existing template has explicit 'namespace' statements, I should probably add them as a conditional. At the same time - there is no reason the template shouldn't work in the default or other namespaces (except it's not a recommended config). I suspect it's a 0.5.1 bug - I tested with a '--set' option pointed to master.

…

On Fri, Feb 23, 2018 at 7:01 AM, Steven Dake ***@***.***> wrote: When I tested, I accidentally deployed to the istio namespace rather than the istio-system namespace which results in: Error: failed to convert to proto. unknown field "mixerCheckServer" in v1alpha1.MeshConfig ***@***.***:~/go/src/istio.io/istio/samples/bookinfo/kube$ istioctl version Version: 0.5.1 GitRevision: 30acfe6 User: ***@***.*** Hub: docker.io/istio GolangVersion: go1.9 BuildStatus: Clean``` I'll try istio-system. In the past, the helm charts have always deployed by default to the default namespace. IOW there may be a latent issue with the helm chart related to custom namespaces. Otherwise, things look good! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3594 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6kUmEpthUWt5UbdlGSOKUCg8vMMGks5tXtLVgaJpZM4SJlMM> .

sdake · 2018-02-23T18:43:11Z

Agreed it is likely a problem with the 0.51 images being out of sync with the manifests. I have just rebuilt latest master and will report back whether that works for me or not with helm install istio --name istio --set hub=docker.io/sdake --set tag=sdake.

Cheers
-steve

sdake

I tested with master and a master version of istioctl using manual injection. That works well enough. I think some more sanitization and testing is in order over time, but this PR looks good to go from my POV.

linsun · 2018-02-23T21:32:51Z

install/kubernetes/helm/istio/charts/grafana/templates/deployment.yaml

    metadata:
      labels:
-        app: {{ template "grafana.name" . }}
-        release: {{ .Release.Name }}


I think there is value to keep release label so users can easily query what makes up a helm release.

This breaks upgrading from istio.yaml ( release / upgradeVersion ) to istio.yaml (helm ), which is
pretty important for now.

We can add annotations or other markers. I'm also not sure if the cost is worth it (k8s keeps indexes by label)

linsun · 2018-02-23T21:34:26Z

install/kubernetes/helm/istio/charts/grafana/templates/service.yaml

      name: {{ .Values.service.name }}
  selector:
-    app: {{ template "grafana.name" . }}
-    release: {{ .Release.Name }}


same comment here...

Confirmed that the release/etc show up in the 'deployment' (as labels).
The change only removes them from the replica set and pods.

linsun · 2018-02-23T21:38:49Z

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

      labels:
-        app: {{ template "mixer.name" . }}
-        release: {{ .Release.Name }}
        istio: mixer


I think these labels are helm convention you may want to keep.

I tried. We can add them later ( but will break upgrade ).

k8s treats the label set as immutable for some strange reasons.

linsun · 2018-02-23T21:42:04Z

install/kubernetes/helm/istio/charts/security/templates/service.yaml

+  labels:
+    app: istio-ca
+spec:
+  type: LoadBalancer


do we always want to do loadbalancer type for this? I asked as istio ingress has a node port option.

That's what we have in istio.yaml - we can add a node port as well, but separate PR :-)

linsun · 2018-02-23T21:43:54Z

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

-    chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
-    release: {{ .Release.Name }}
-    heritage: {{ .Release.Service }}
    istio: sidecar-injector


same comment here on labels.

linsun · 2018-02-23T21:45:03Z

install/kubernetes/helm/istio/templates/configmap.yaml

 data:
  mesh: |-
-  {{- if .Values.global.securityEnabled }}
+  {{- if .Values.global.mtlsDefault }}


could we call it mtlsEnabled? default is confusing as to on or off...

Do you mind doing this in separate PR ? Don't want to go trough another wait :-)

I had to rebase, so updated it.

linsun · 2018-02-23T21:48:54Z

install/kubernetes/helm/istio/values.yaml

-  # create RBAC resources
+  # Default mtls policy. If true, mtls between services will be enabled by default.
+  mtlsDefault: false
+


I'm confused that we want to expose both securityEnabled and mtlsDefault or mtlsEnabled to our user... because you can't have mtlsEnabled = true while securityEnabled = false. Maybe having mtlEnabled as a child property under securityEnabled?

Well, technically you can... SecurityEnabled means istioCA is installed. In theory you can have
another component generating certificates ( for example using vault or a native CA). Or use
some other spiffee thing.

linsun · 2018-02-23T21:49:48Z

install/kubernetes/helm/istio/values.yaml

+
+  # create RBAC resources. Must be set for any cluster configured with rbac.
  rbacEnabled: true
-  # mutual TLS authentication


for rbacEnabled... I'd assume it is only valid when security is enabled? and mixer is enabled today...

AFAIK RBAC is required if the cluster has rbac ( and many have it on by default ).
RBAC only registers the cluster permissions so pilot can read stuff.

linsun

pending address of minor comments. Really good work Costin, I appreciate your effort so now we can generate istio yams from helm artifacts. :)

istio-merge-robot · 2018-02-24T08:02:51Z

@costinm PR needs rebase

)

costinm · 2018-02-26T16:36:15Z

PTAL - I addressed most of the comments, but also added prometheus change (to sync with the templates/addons config), and added the refresh setting for larger clusters.

ldemailly · 2018-02-26T17:01:19Z

please be careful to not commit accidental/unwanted changes to vendor, please undo that change for the build to pass

ldemailly · 2018-02-26T20:54:11Z

bin/init.sh

+if [ ! -f /usr/local/bin/helm -a ! -f ${ISTIO_OUT}/helm ] ; then
+    # Install helm. Please keep it in sync with .circleci
+    cd /tmp && \
+        curl -Lo /tmp/helm.tgz https://storage.googleapis.com/kubernetes-helm/helm-${HELM_VER}-linux-amd64.tar.gz && \


can we make this optional ? not everyone should have to install helm ? (only if you touch the templates,...)

not every build (lint, unit test, etc...) need helm either

We plan to use helm to generate istio.yaml and test configs - so it won't be optional.
Also helm ( full helm, with tiller ) will need to be part of one of the integration tests.

this is a bit risky though because helm can easily become required accidentally that way
it's also not hermetic, we should avoid downloading stuff
maybe the helm client can be vendored ? it should already have most of it as we use the libs in gen-deploy ?

in short I'm concerned it's really easy after that to break the "istio-auth.yaml" is all you need to install a basic istio (demo) setup - and pass all the tests

it still shouldn't be required

costinm · 2018-02-26T23:01:54Z

/test istio-pilot-e2e

costinm added 2 commits February 17, 2018 18:26

Take2 on config unification

0b3dc01

Few more fixes, add ingored values

2a73be5

costinm requested review from a team February 18, 2018 05:52

googlebot added the cla: yes label Feb 18, 2018

costinm added 4 commits February 20, 2018 14:52

Updates

5b4cc00

Merge branch 'master' of https://github.com/istio/istio into costin-0216

94b1154

Added the CA service for VM, update values

44a63e7

Merge branch 'master' of https://github.com/istio/istio into costin-0216

9b3abe6

costinm requested review from linsun and sdake February 22, 2018 01:00

sdake reviewed Feb 22, 2018

View reviewed changes

costinm commented Feb 22, 2018

View reviewed changes

costinm mentioned this pull request Feb 22, 2018

Istio 0.4.0 Helm chart installation fails #2349

Closed

sdake approved these changes Feb 23, 2018

View reviewed changes

sdake mentioned this pull request Feb 23, 2018

Remove all *.tpl files in helm charts #3479

Closed

linsun reviewed Feb 23, 2018

View reviewed changes

linsun approved these changes Feb 23, 2018

View reviewed changes

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 24, 2018

costinm added 6 commits February 25, 2018 08:38

Merge branch 'master' of https://github.com/istio/istio into costin-0216

d148789

Address review comments

a04f6e8

Use the nightly flag (will be updated once 0.6 is out)

a5e4499

Fix mixer

ba82631

Add missing ports

25b4cdf

Add missing ports

5c837b0

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 25, 2018

costinm added 3 commits February 25, 2018 12:24

Use clusterip for security

e241021

Fix configmap

ae61f16

Sync prometheus, add refresh (required for larger deployments

7c84695

)

Merge branch 'master' of github.com:istio/istio into costin-0216

0c10bd3

costinm added 2 commits February 26, 2018 11:32

Merge branch 'master' of github.com:istio/istio into costin-0216

de606e4

Thanks Laurent

dcc0bc7

ldemailly reviewed Feb 26, 2018

View reviewed changes

costinm merged commit 1bef28b into master Feb 26, 2018

costinm deleted the costin-0216 branch March 7, 2018 23:48

Conversation

costinm commented Feb 18, 2018

Uh oh!

istio-merge-robot commented Feb 18, 2018

Uh oh!

sdake left a comment

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!

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!

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

Uh oh!

Uh oh!

costinm commented Feb 23, 2018 via email

Uh oh!

sdake commented Feb 23, 2018

Uh oh!

sdake left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

linsun Feb 23, 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!

Choose a reason for hiding this comment

Uh oh!

linsun left a comment

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Feb 24, 2018

sdake commented Feb 23, 2018 •

edited

Loading

linsun Feb 23, 2018 •

edited

Loading