components: Add istio-operator #686

surajssd · 2020-06-30T08:48:23Z

Fixes #687

Usage

Install using lokoctl component apply experimental-istio-operator:

Then to test it on an app, clone this repo and goto configs/bookinfo dir and run following commands:

kubectl create ns bookinfo
kubectl label namespace bookinfo istio-injection=enabled
helm install bookinfo --namespace bookinfo .

TODOs

Test:

See if the pod installs correctly.
Check the metrics collected are available in Prometheus.
Decide on docs.

Other:

How do we qualify this component as experimental? (Although this is not a blocker for this PR).
Are there any other configs to expose to the user?
Add istio dashboards if any.

ipochi · 2020-06-30T14:52:12Z

docs are going in a separate PR ?

invidian

In general LGTM, except the comment. Also e2e tests needs to be added.

pkg/components/util/helm.go

surajssd · 2020-07-20T14:55:51Z

We have decided to name this component experimental-istio-operator.

surajssd · 2020-07-21T15:45:05Z

cc: @invidian @ipochi @iaguis PTAL.

invidian · 2020-07-21T15:50:35Z

@surajssd do you mind rebasing before I review (to get rid of fixup commits)?

surajssd · 2020-07-22T07:51:30Z

@invidian done :-)

surajssd · 2020-07-22T13:18:29Z

@invidian PTAL.

invidian

Overall things looks OK to me, however, I would re-organize and improve commit messages, as there is a lot of things going in regards to how we manage the namespaces for the components, which seems like something, which should be definitely described with the reasoning behind. It could even be done in separate pull request to simplify and separate the review comments.

@surajssd can we move changes related to Helm and other components into separate PR, as they are not related to istio-operator (though I'm aware that this PR, so istio-operator depends on those changes).

assets/components/cert-manager/manifests/templates/namespace.yaml

assets/components/velero/manifests/templates/namespace.yaml

assets/components/prometheus-operator/manifests/templates/prometheus-operator/namespace.yaml

pkg/components/util/helm.go

assets/components/istio-operator/Chart.yaml

pkg/components/istio-operator/manifest.go

ci/aks/aks-cluster.lokocfg.envsubst

invidian · 2020-07-22T14:26:18Z

docs/configuration-reference/components/istio-operator.md

+
+Table of all the arguments accepted by the component.
+
+Example:


What does this Example mean here?

Good question. I am not sure what they mean. It is a remnant of the copy-paste of the skeleton from other components.

docs/configuration-reference/components/istio-operator.md

surajssd · 2020-07-22T17:49:01Z

Overall things looks OK to me, however, I would re-organize and improve commit messages, as there is a lot of things going in regards to how we manage the namespaces for the components, which seems like something, which should be definitely described with the reasoning behind. It could even be done in separate pull request to simplify and separate the review comments.

I think I can elaborate on the commits but creating a new PR will only prolong this work.

ipochi

Lovely work @surajssd 🎉

Generally LGTM.

Please take a look into creating a separate PR for allowing helm to create release namespace.

assets/components/istio-operator/Chart.yaml

pkg/components/istio-operator/manifest.go

pkg/components/util/helm.go

assets/components/cert-manager/manifests/templates/namespace.yaml

docs/configuration-reference/components/istio-operator.md

ipochi · 2020-07-24T13:39:22Z

@surajssd On second thoughts, its better not use the helm feature install.CreateNamespace = true

All it does is create a namespace object on the fly and seeing that we have a use case where we patch the namespace with labels, it becomes a two step operation.

I would rather use the existing code to create/update the release namespace and not depend on helm for it.

surajssd · 2020-08-17T11:30:17Z

@ipochi @invidian this is open for review again PTAL.

invidian · 2020-08-18T12:03:37Z

https://yard.lokomotive-k8s.net/builds/26675

TestAllNamespacesHaveNameLabels: namespaces_labels_test.go:50: expected "istio-system", got: ""

Ugh, it seems the test needs to be modified...

invidian

Just some nits, otherwise LGTM. The only missing bit is, that either the e2e test needs to be modified or we need to add a lokomotive name label to the istio namespace (the latter is definitely easier).

pkg/components/istio-operator/component.go

test/monitoring/components_metrics_test.go

invidian

LGTM

test/components/kubernetes/namespaces_labels_test.go

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

Adds ServiceMonitor configs for the control plane and istio-proxy sidecars which is toggled by a variable `enable_monitoring`. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

- Adds e2e test to verify if pods are up and running. - Adds test to verify if the metrics are scraped. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

invidian

LGTM

assets/charts/components/istio-operator/Chart.yaml

knrt10

Nice, another component. Great work @surajssd

Re requesting review

surajssd · 2020-08-28T15:02:01Z

Something is wrong with the istio release bundle, which is downloaded from https://github.com/istio/istio/releases/tag/1.7.0 :

$ ll
drwxr-x---@   - surajd 22 Aug  0:30 istio-1.7.0/
.rw-rw-r--@ 47M surajd 22 Aug  6:30 istio-1.7.0-linux-amd64.tar.gz

The image tag is latest ?

$ cat istio-1.7.0/manifests/charts/istio-operator/values.yaml 
hub: gcr.io/istio-testing
tag: latest

operatorNamespace: istio-operator

# Used to replace istioNamespace to support operator watch multiple namespaces.
watchedNamespaces: istio-system
waitForResourcesTimeout: 300s

# Used for helm2 to add the CRDs to templates.
enableCRDTemplates: false

# revision for the operator resources
revision: ""

# Operator resource defaults
operator:
  resources:
    limits:
      cpu: 200m
      memory: 256Mi
    requests:
      cpu: 50m
      memory: 128Mi

invidian · 2020-08-31T10:20:40Z

The image tag is latest ?

@surajssd this is for the operator image, but yeah, it's weird that they don't pin the version. Using "testing" image registry is weird too (gcr.io/istio-testing) 😄

I'll open an upstream issue about it, but I don't think this should block merging this PR.

EDIT: Created istio/istio#26920.

sdake · 2020-10-16T12:55:12Z

@invidian looks like an interesting project. Please note in the pending Istio 1.8, there is a make target that generates a proper manifest using helm template for the operator:

make gen-kustomize : https://github.com/istio/istio/blob/master/Makefile.core.mk#L364-L365

The pending 1.8 manifest is in a lot better shape than what was shipped in Istio 1.6 👍 Although what you have should be fine.

Thanks for adopting Istio in your project and reporting the pinning defect.

Cheers,
-steve

surajssd force-pushed the surajssd/add-istio-operator branch 4 times, most recently from 10f6ddd to b7a02d0 Compare June 30, 2020 14:00

surajssd marked this pull request as ready for review June 30, 2020 14:00

surajssd requested review from invidian, ipochi and pothos as code owners June 30, 2020 14:00

surajssd force-pushed the surajssd/add-istio-operator branch from b7a02d0 to d2f67d0 Compare July 1, 2020 06:56

invidian suggested changes Jul 1, 2020

View reviewed changes

pkg/components/util/helm.go Outdated Show resolved Hide resolved

invidian mentioned this pull request Jul 1, 2020

pkg/k8sutil: change NewClientset to load kubeconfig from file content, not from path #628

Merged

surajssd force-pushed the surajssd/add-istio-operator branch from d2f67d0 to 439b708 Compare July 3, 2020 13:50

surajssd force-pushed the surajssd/add-istio-operator branch 2 times, most recently from 7eb6986 to 0b575ff Compare July 21, 2020 07:30

surajssd requested a review from johananl as a code owner July 21, 2020 07:30

surajssd force-pushed the surajssd/add-istio-operator branch from 0b575ff to 7804932 Compare July 21, 2020 07:38

surajssd requested a review from invidian July 21, 2020 07:39

surajssd force-pushed the surajssd/add-istio-operator branch from 3c1fa21 to 0ff1ff5 Compare July 22, 2020 07:51

t-lo mentioned this pull request Jul 22, 2020

benchmarks gauge script kinvolk/service-mesh-benchmark#29

Merged

invidian suggested changes Jul 22, 2020

View reviewed changes

surajssd force-pushed the surajssd/add-istio-operator branch from 0ff1ff5 to 5363720 Compare July 22, 2020 17:06

ipochi previously requested changes Jul 23, 2020

View reviewed changes

invidian suggested changes Aug 18, 2020

View reviewed changes

pkg/components/istio-operator/component.go Outdated Show resolved Hide resolved

test/monitoring/components_metrics_test.go Show resolved Hide resolved

surajssd force-pushed the surajssd/add-istio-operator branch 5 times, most recently from 7e971a2 to 616e444 Compare August 24, 2020 14:54

surajssd requested a review from invidian August 25, 2020 10:31

invidian previously approved these changes Aug 25, 2020

View reviewed changes

test/components/kubernetes/namespaces_labels_test.go Outdated Show resolved Hide resolved

surajssd dismissed invidian’s stale review via 47b56a2 August 26, 2020 13:32

surajssd force-pushed the surajssd/add-istio-operator branch 3 times, most recently from a7ba6c6 to 6763446 Compare August 27, 2020 07:05

surajssd added 5 commits August 27, 2020 12:42

components: Add istio-operator

52a849e

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

ci: Install istio-operator

4afb2e6

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

istio-operator: Add variable to enable monitoring

1665124

Adds ServiceMonitor configs for the control plane and istio-proxy sidecars which is toggled by a variable `enable_monitoring`. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

istio-operator: Add e2e test

ee185b4

- Adds e2e test to verify if pods are up and running. - Adds test to verify if the metrics are scraped. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

docs: Add istio-operator configuration reference

893a1f2

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd force-pushed the surajssd/add-istio-operator branch from 6763446 to 893a1f2 Compare August 27, 2020 07:12

invidian approved these changes Aug 27, 2020

View reviewed changes

knrt10 reviewed Aug 27, 2020

View reviewed changes

assets/charts/components/istio-operator/Chart.yaml Show resolved Hide resolved

knrt10 approved these changes Aug 27, 2020

View reviewed changes

invidian mentioned this pull request Aug 31, 2020

Helm chart bundled with the release do not pin operator version and use testing image registry istio/istio#26920

Closed

invidian merged commit d41aa0c into master Aug 31, 2020

invidian deleted the surajssd/add-istio-operator branch August 31, 2020 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

components: Add istio-operator #686

components: Add istio-operator #686

surajssd commented Jun 30, 2020 •

edited

Loading

ipochi commented Jun 30, 2020

invidian left a comment

surajssd commented Jul 20, 2020

surajssd commented Jul 21, 2020

invidian commented Jul 21, 2020

surajssd commented Jul 22, 2020

surajssd commented Jul 22, 2020

invidian left a comment

invidian Jul 22, 2020

surajssd Jul 22, 2020

surajssd commented Jul 22, 2020

ipochi left a comment •

edited

Loading

ipochi commented Jul 24, 2020

surajssd commented Aug 17, 2020

invidian commented Aug 18, 2020

invidian left a comment

invidian left a comment

invidian left a comment

knrt10 left a comment

surajssd commented Aug 28, 2020 •

edited

Loading

invidian commented Aug 31, 2020 •

edited

Loading

sdake commented Oct 16, 2020


		Table of all the arguments accepted by the component.

		Example:

components: Add istio-operator #686

components: Add istio-operator #686

Conversation

surajssd commented Jun 30, 2020 • edited Loading

Usage

TODOs

Test:

Other:

ipochi commented Jun 30, 2020

invidian left a comment

Choose a reason for hiding this comment

surajssd commented Jul 20, 2020

surajssd commented Jul 21, 2020

invidian commented Jul 21, 2020

surajssd commented Jul 22, 2020

surajssd commented Jul 22, 2020

invidian left a comment

Choose a reason for hiding this comment

invidian Jul 22, 2020

Choose a reason for hiding this comment

surajssd Jul 22, 2020

Choose a reason for hiding this comment

surajssd commented Jul 22, 2020

ipochi left a comment • edited Loading

Choose a reason for hiding this comment

ipochi commented Jul 24, 2020

surajssd commented Aug 17, 2020

invidian commented Aug 18, 2020

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

knrt10 left a comment

Choose a reason for hiding this comment

surajssd commented Aug 28, 2020 • edited Loading

invidian commented Aug 31, 2020 • edited Loading

sdake commented Oct 16, 2020

surajssd commented Jun 30, 2020 •

edited

Loading

ipochi left a comment •

edited

Loading

surajssd commented Aug 28, 2020 •

edited

Loading

invidian commented Aug 31, 2020 •

edited

Loading