Add a gw/vs for kiali by sdake · Pull Request #8971 · istio/istio

sdake · 2018-09-26T06:26:50Z

Previously ingress was required to use Kiali. With this change
it is possible to use kiali using ingressgateway as per other
telemetry-gateway behavior.

I recognize this isn't the perfect organization - will get there
over time with multiple PRs.

Previously ingress was required to use Kiali. With this change it is possible to use kiali using ingressgateway as per other telemetry-gateway behavior. I recognize this isn't the perfect organization - will get there over time with multiple PRs.

sdake · 2018-09-26T06:27:14Z

/assign @gyliu513

sdake · 2018-09-26T16:28:29Z

/assign @GregHanson

ZackButcher · 2018-09-26T23:00:18Z

install/kubernetes/helm/istio/values-istio-gateways.yaml

      targetPort: 8060
      name: tcp-citadel-grpc-tls
+    # Addon ports for kiali are enabled in gateway - but will only redirect if
+    # the gateway focnfiguration for the various components are enabled.


s/focnfiguration/configuration

ZackButcher · 2018-09-26T23:02:09Z

install/kubernetes/helm/istio/charts/kiali/templates/gateway.yaml

+  namespace: {{ .Release.Namespace }}
+spec:
+  selector:
+    istio: {{ .Values.gatewayName }}


It's not required that the selector be istio: <name>, it could be arbitrary labels. We should change values.yaml to take selector, and provide default value istio: ingressgateway.

I prefer that we hardcode the selector here as istio: ingressgateway. As we can see the default ingress gateway in istio only has two labels as below:

labels: app: istio-ingressgateway istio: ingressgateway

If we allow end user to set arbitrary labels in values.yaml, the Gateway may not work if it cannot select an ingress gateway via the selector.

@gyliu513 sure I can hardcode the selector and agree it would be better to do so. Less chance for an operator to crater their install. I'm not sure why @douglas-reid chose to make this configurable. Will remove that configurability across the install codebase in a followup PR.

@ZackButcher sure - makes sense. I do agree with @gyliu513 's position that we should simplify this part by hardcoding. I don't see any good reason to make the selector here configurable given the nature of exposing services via ingressgateway. This saves on the expense of introducing another LoadBalancer service type, and the logistics of managing that IP.

@sdake @ZackButcher It is configurable because that was what was asked for. At the time of the PR, the feeling was that while some deployments would want to expose it via "ingressgateway" others may only have wanted to make it exposed via "ilbgateway" or whatever. This was an explicit part of the negotiations around the telemetry-gateway PR. I don't think we should hard-code it. I'm fine with having the selector itself be part of the configuration, however.

codecov · 2018-09-27T14:55:59Z

Codecov Report

Merging #8971 into master will increase coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #8971    +/-   ##
=======================================
+ Coverage      69%     70%    +1%     
=======================================
  Files         418     418            
  Lines       38551   38149   -402     
=======================================
- Hits        26587   26448   -139     
+ Misses      10715   10475   -240     
+ Partials     1249    1226    -23

Impacted Files	Coverage Δ
pilot/pkg/config/kube/ingress/status.go	`22% <0%> (-25%)`	⬇️
pilot/pkg/model/authorization.go	`72% <0%> (-8%)`	⬇️
pkg/mcp/creds/watcher.go	`84% <0%> (-5%)`	⬇️
pilot/pkg/proxy/envoy/v2/discovery.go	`61% <0%> (-5%)`	⬇️
galley/pkg/kube/source/listener.go	`94% <0%> (-2%)`	⬇️
mixer/adapter/servicecontrol/quotaprocessor.go	`80% <0%> (-2%)`	⬇️
pilot/pkg/model/conversion.go	`90% <0%> (-1%)`	⬇️
pilot/pkg/model/validation.go	`83% <0%> (ø)`	⬇️
mixer/adapter/kubernetesenv/cache.go	`86% <0%> (ø)`	⬇️
mixer/adapter/servicecontrol/reportbuilder.go	`89% <0%> (ø)`	⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c5eae3...33438a3. Read the comment docs.

gyliu513 · 2018-09-28T02:23:57Z

install/kubernetes/helm/istio/values.yaml

  replicaCount: 1
  hub: docker.io/kiali
  tag: v0.6
+  gatewayName: ingressgateway


Do we still need this variable in values.yaml?

stewart-yu · 2018-09-29T00:50:13Z

install/kubernetes/helm/istio/charts/kiali/templates/gateway.yaml

+        host: kiali.{{ .Release.Namespace }}.svc.cluster.local
+        port:
+          number: 20001
+---


we need --- in the end of file?

it has no impact but would be good to delete @sdake

gyliu513

/lgtm

istio-testing · 2018-09-29T12:46:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gyliu513

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~install/OWNERS~~ [gyliu513]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…8971) * Make trace pod annotation warning more visible. * Update content/en/docs/tasks/observability/distributed-tracing/configurability/index.md Co-authored-by: Eric Van Norman <ericvn@us.ibm.com> Co-authored-by: Pengyuan Bian <bianpengyuan@google.com> Co-authored-by: Eric Van Norman <ericvn@us.ibm.com>

Add a gw/vs for kiali

d47f022

Previously ingress was required to use Kiali. With this change it is possible to use kiali using ingressgateway as per other telemetry-gateway behavior. I recognize this isn't the perfect organization - will get there over time with multiple PRs.

istio-testing requested review from GregHanson and ostromart September 26, 2018 06:26

googlebot added the cla: yes label Sep 26, 2018

istio-testing assigned gyliu513 Sep 26, 2018

istio-testing assigned GregHanson Sep 26, 2018

ZackButcher reviewed Sep 26, 2018

View reviewed changes

address review comments

ec024e2

gyliu513 reviewed Sep 28, 2018

View reviewed changes

stewart-yu reviewed Sep 29, 2018

View reviewed changes

Address review comments istio#2

33438a3

gyliu513 reviewed Sep 29, 2018

View reviewed changes

istio-testing added the lgtm label Sep 29, 2018

istio-testing added the approved label Sep 29, 2018

istio-testing merged commit 588dadb into istio:master Sep 29, 2018

jmazzitelli mentioned this pull request Oct 1, 2018

replace Service Graph with Kiali #9066

Closed

7 tasks

sdake mentioned this pull request Oct 30, 2018

Add telemetry-gateway subchart to helm requirements.yaml file #9616

Closed

Conversation

sdake commented Sep 26, 2018

Uh oh!

sdake commented Sep 26, 2018

Uh oh!

sdake commented Sep 26, 2018

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 Sep 27, 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!

codecov bot commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

gyliu513 left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Sep 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sdake Sep 27, 2018 •

edited

Loading

codecov bot commented Sep 27, 2018 •

edited

Loading