Add mesh expansion gateway by costinm · Pull Request #6896 · istio/istio

costinm · 2018-07-06T21:49:23Z

Add an option for gateway to enable 2 additional ports. This must be set on a specific gateway - can be set for example on the ingress-gateway (to not waste an IP) or on a different one. ILB-type gateways
could do this - but separate PR.

In addition, a global option will enable the gateway and VirtualServices needed to connect to pilot and
citadel. The mesh expansion or multi-cluster must be setup with the IP of the ingress. For pilot, only mtls connections are exposed, for security reasons.

Along few cleanups and polish:

remote was out of sync with main config
if image has a "/", use it as a fully qualified name (as we do for the proxy annotation).
It is not clear what was the purpose of image - we don't publish other pilot/mixer/etc images under istio/istio, so having it as an option to be composed with base hub and tag doesn't make sense.
The image with / is currently enabled for grafana, pilot and initializer - each may require tweaking
(grafana is currently broken, but previous version works, initializer needs to be tweaked for tproxy testing)

istio-testing · 2018-07-06T21:49:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm

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~~ [costinm]

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

codecov · 2018-07-06T23:04:23Z

Codecov Report

Merging #6896 into release-1.0 will decrease coverage by 1%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release-1.0   #6896    +/-   ##
============================================
- Coverage           71%     71%   -<1%     
============================================
  Files              360     369     +9     
  Lines            31273   31648   +375     
============================================
+ Hits             22123   22374   +251     
- Misses            8275    8377   +102     
- Partials           875     897    +22

Impacted Files	Coverage Δ
pilot/pkg/config/memory/monitor.go	`82% <0%> (-9%)`	⬇️
mixer/adapter/bypass/bypass.go	`62% <0%> (-1%)`	⬇️
mixer/adapter/bypass/util.go	`7% <0%> (ø)`	⬇️
pilot/pkg/kube/inject/inject.go	`85% <0%> (ø)`	⬇️
mixer/adapter/memquota/rollingWindow.go	`100% <0%> (ø)`	⬆️
pilot/pkg/kube/inject/initializer.go	`100% <0%> (ø)`	⬆️
mixer/adapter/memquota/dedup.go	`100% <0%> (ø)`	⬆️
mixer/adapter/solarwinds/solarwinds.go	`0% <0%> (ø)`	⬆️
galley/pkg/mcp/client/client.go
broker/pkg/platform/kube/crd/types.go	`56% <0%> (ø)`
... and 18 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 426b63e...8a788eb. Read the comment docs.

rshriram

Wrong place to add ports.

rshriram · 2018-07-07T14:11:46Z

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

+    - port: 8060
+      targetPort: 8060
+      name: tcp-citadel
+{{- end }}


Do we need to overload the gateway for this ?

Since this works only with ilb why not have a separate ilb gateway instead of overloading consumer facing ingress gateway? Also this is not the right place to add ports. It’s in values.yaml

On the first question: somehow the gateway needs to listen on the port. We can add it in values.yaml, but it's harder for the user, we may have other boiler plage.

On the second: this is not based on ILB. We may add an ILB if anyone asks, but current solution is exposing mtls only and can be used on the regular gateway

Having a separate gateway is possible - that's why it's an option on the gateway, a user can add add more gateways and select to enable mesh expansion ports selectively.

rshriram · 2018-07-07T14:18:16Z

install/kubernetes/helm/istio/values.yaml

+
+    # If set to true, the pilot and citadel mtls ports will be exposed on the ingress gateway
+    meshExpansion: false
+


This is where you should be adding these ports. I suggest creating a separate ingress-gateway-with-mesh-expansion that has same ports as ingress plus additional ports. Same service name as well.

This gateway should be disabled by default. Then in docs, you simply need to tell people to override the existing ingress gateway service with the new service (no changes to running gateway), add new servers to the gateway spec and so on. This keeps it cleaner and incremental, without having to add these ports to the templated spec.

Running 2 gateways is more expensive ( IP and LB cost ). Large users are likely to do that, but for dev and small developers we can't require that. The mesh expansion gateway is disabled by default (false). I am planning to add another ingress-gateway-ILB - this will get a separate internal IP - but since not everyone has access to ILBs the first priority is using a normal gateway.

Note that this change can be done without helm changes - it's using just standard API - but it just makes it easier for a user.

costinm · 2018-07-07T15:24:48Z

My intention is for users to be able to install mesh expansion without editing the values.yaml manually.
They need to override 2 settings:
--set global.meshExpansion=true and
either --set gateways.istio-ingressagateway.meshExpansion=true or
--set gateways.istio-ilbgateway.enabled=true if they want ILB type.
(the second is WIP since the first type is going to work everywhere )

rshriram · 2018-07-07T23:14:58Z

install/kubernetes/helm/istio/values.yaml

+  # If set to true, the pilot and citadel mtls and the plain text pilot ports
+  # will be exposed on an internal gateway
+  meshExpansionILB: false
+


You don’t need these two.

Why not ? How else can user control if they add or not the gateway (and expose pilot/ca) ?

rshriram · 2018-07-07T23:16:43Z

install/kubernetes/helm/istio/values.yaml

      name: tcp
      nodePort: 31400
+    # Pilot and Citadel MTLS ports are enabled in gateway - but will only redirect
+    # to pilot/citadel if global.meshExpansion settings are enabled.


Change comment to say that while ports are exposed by the service, routing will happen only after user adds gateway config and virtual service per tutorial.
This way we don’t need to tweak helm for ingress based mesh expansion. You can add or remove routing config to enable or disable mesh expansion.

rshriram · 2018-07-07T23:17:14Z

install/kubernetes/helm/istio/values.yaml

+    # to pilot/citadel if global.meshExpansion settings are enabled.
+    - port: 15011
+      targetPort: 15011
+      name: tcp-pilot-grcp-tls


rshriram

Typos and unused vars.

rshriram · 2018-07-08T22:26:31Z

install/kubernetes/helm/istio/charts/pilot/templates/meshexpansion.yaml

+  name: meshexpansion-pilot
+spec:
+  hosts:
+  - "pilot.istio-system"


istio-pilot.istio-system ?
I would also add more variants.. like istio-pilot.istio-system.svc.cluster.local

rshriram · 2018-07-08T22:28:50Z

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

+  name: meshexpansion-ilb-citadel
+spec:
+  hosts:
+  - "citadel.istio-system"


istio-citadel

rshriram · 2018-07-08T22:28:56Z

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

+  name: meshexpansion-citadel
+spec:
+  hosts:
+  - "citadel.istio-system"


istio-citadel

rshriram

wrong hostnames in virtual services

costinm · 2018-07-09T05:29:45Z

Oh - it doesn't seem to matter, interesting. Are the hostnames in port matchers used for anything ?

…

On Sun, Jul 8, 2018 at 3:30 PM Shriram Rajagopalan ***@***.***> wrote: ***@***.**** commented on this pull request. wrong hostnames in virtual services — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6896 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6l2yUy39NkHzJNn2s24nrvsbiq8gks5uEoftgaJpZM4VGER0> .

rshriram · 2018-07-09T14:23:35Z

There is no hostname stuff in the port match.. The authority match stuff goes into envoy's header match fields.. It doesn't get into the virtual host stuff.

tiswanso · 2018-07-09T15:32:24Z

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

+apiVersion: networking.istio.io/v1alpha3
+kind: Gateway
+metadata:
+  name: meshexpansion-ilb-gatewa


Add mesh expansion gateway

ae178cc

googlebot added the cla: yes label Jul 6, 2018

costinm requested a review from sdake July 6, 2018 21:49

istio-testing requested review from ayj and ijsnellf July 6, 2018 21:49

istio-testing added the approved label Jul 6, 2018

costinm requested review from rshriram and removed request for ayj and ijsnellf July 6, 2018 21:49

costinm added 2 commits July 6, 2018 15:38

Merge branch 'release-1.0' of github.com:istio/istio into 10-gateway

ad61b5e

Match the old, unused value used by tests

9203af7

rshriram reviewed Jul 7, 2018

View reviewed changes

costinm added 3 commits July 7, 2018 08:55

Merge branch 'release-1.0' of github.com:istio/istio into 10-gateway

81f4429

Fix pilot port number and add ilb gateway

fa9783e

Move the ports to values.yaml as suggested, add ILB

c71860c

rshriram reviewed Jul 7, 2018

View reviewed changes

rshriram approved these changes Jul 7, 2018

View reviewed changes

The gateway files were missing, fix typo

bdd6fbd

rshriram reviewed Jul 8, 2018

View reviewed changes

tiswanso added the area/environments label Jul 9, 2018

tiswanso reviewed Jul 9, 2018

View reviewed changes

costinm added 2 commits July 9, 2018 11:52

Merge branch 'release-1.0' of github.com:istio/istio into 10-gateway

0d4aa58

Typos, based on review

8a788eb

costinm merged commit bab2220 into istio:release-1.0 Jul 9, 2018

baodongli mentioned this pull request Sep 12, 2018

Use ILB for istio mc #5535

Closed

gdoctor mentioned this pull request Sep 20, 2018

Test/document multi-cloud with CoreDNS #3361

Closed


		# If set to true, the pilot and citadel mtls ports will be exposed on the ingress gateway
		meshExpansion: false

Conversation

costinm commented Jul 6, 2018

Uh oh!

istio-testing commented Jul 6, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

rshriram 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!

costinm commented Jul 7, 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!

rshriram 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!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

costinm commented Jul 9, 2018 via email

Uh oh!

rshriram commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jul 6, 2018 •

edited

Loading