Migrate to Istio v1alpha3 Gateway / VirtualService #1228

tcnghia · 2018-06-15T17:26:03Z

In this change we also move from an N Ingresses model to a single shared Gateway for all of Knative services. We also create our own load-balanced service to do so in order to avoid conflict with users when using the istio-ingressgateway service, since Gateways can not share the service for ingresses.

ZhiminXiang

Verified that helloworld sample still works with this PR.
Reviewed about one third of this PR. Will try to finish the rest of this PR within this week.

ZhiminXiang · 2018-06-18T22:22:28Z

pkg/apis/istio/v1alpha3/register.go

-		&RouteRule{},
-		&RouteRuleList{},
+		&VirtualService{},
+		&Gateway{},


We also define GatewayList. Do we want to add it here?

ZhiminXiang · 2018-06-18T22:32:20Z

pkg/apis/istio/v1alpha3/virtualservice_types.go

+	WebsocketUpgrade bool `json:"websocketUpgrade,omitempty"`
+
+	// Timeout for HTTP requests.
+	Timeout string `json:"timeout,omitempty"`


no objection but just curious: why the type of timeout is string? Looks like originally it is Duration prototype: https://istio.io/docs/reference/config/istio.networking.v1alpha3/#HTTPRoute

I don't know if there is a way to deserialize the duration strings "10m", "1h", etc into a Duration struct.

All Istio objects are processed as JSON, which is basically map[string]string, and then parsed into protobuf on their side.

ZhiminXiang · 2018-06-18T22:48:21Z

pkg/apis/serving/v1alpha1/revision_types.go

+	return rs.IsReady() && !rs.IsInactive()
+}
+
+func (rs *RevisionStatus) IsRoutable() bool {


what is the difference btw status Active and Routable?

when revisions are in Inactive state, we can still route traffic to it through the activator

ZhiminXiang · 2018-06-18T22:56:00Z

pkg/apis/serving/v1alpha1/route_types.go

@@ -113,9 +113,7 @@ const (
 	// RouteConditionReady is set when the service is configured
 	// and has available backends ready to receive traffic.
 	RouteConditionReady RouteConditionType = "Ready"
-	// RouteConditionIngressReady is set when the route's underlying ingress
-	// resource has been set up.
-	RouteConditionIngressReady RouteConditionType = "IngressReady"


Why do we just delete this condition instead of replacing it with RouteConditionGatewayReady?

Since we only have a single Gateway that we create at the time of setup (see config/202-gateway.yaml), the Gateway is always ready.

ZhiminXiang · 2018-06-18T22:57:07Z

pkg/client/clientset/versioned/clientset.go

-	configV1alpha2  *configv1alpha2.ConfigV1alpha2Client
-	servingV1alpha1 *servingv1alpha1.ServingV1alpha1Client
+	networkingV1alpha3 *networkingv1alpha3.NetworkingV1alpha3Client
+	servingV1alpha1    *servingv1alpha1.ServingV1alpha1Client


nit: extra space.

That's because of gofmt autoindent.

ZhiminXiang · 2018-06-18T23:07:24Z

pkg/client/clientset/versioned/typed/istio/v1alpha3/istio_client.go

+}
+
+// NewForConfig creates a new NetworkingV1alpha3Client for the given config.
+func NewForConfig(c *rest.Config) (*NetworkingV1alpha3Client, error) {


maybe rename this function to "NewForNetworking" ?

I think the Config here refers to the "c *rest.Config" parameter.

ZhiminXiang · 2018-06-18T23:14:19Z

pkg/client/clientset/versioned/typed/istio/v1alpha3/istio_client.go

 // panics if there is an error in the config.
-func NewForConfigOrDie(c *rest.Config) *ConfigV1alpha2Client {
+func NewForConfigOrDie(c *rest.Config) *NetworkingV1alpha3Client {


rename to "NewForNetworkingOrDie" ?

same as previous comment.

ZhiminXiang · 2018-06-24T22:00:36Z

For helloworld example, looks like the VirtualService only matches host "route-example.default.demo-domain.com" and "route-example-service.default.svc.cluster.local", but misses host "*.route-example.default.demo-domain.com". Below is the spec of VirtualService:

 spec:
    gateways:
    - route-example-gateway
    - mesh
    hosts:
    - '*.route-example.default.demo-domain.com'
    - route-example.default.demo-domain.com
    - route-example-service.default.svc.cluster.local
    http:
    - match:
      - authority:
          exact: route-example.default.demo-domain.com
      - authority:
          exact: route-example-service.default.svc.cluster.local
      route:
      - destination:
          host: configuration-example-00001-service.default.svc.cluster.local
          port:
            number: 80
        weight: 100

tcnghia · 2018-06-27T00:41:37Z

/test pull-knative-serving-go-coverage

tcnghia · 2018-06-27T02:48:20Z

/test pull-knative-serving-integration-tests

tcnghia · 2018-06-27T03:06:10Z

@ZhiminXiang since sample/helloworld/sample.yaml doesn't have named TrafficTarget, that is to be expected.

tcnghia · 2018-06-27T03:55:42Z

/test pull-knative-serving-integration-tests

tcnghia · 2018-06-27T04:08:58Z

/test pull-knative-serving-intergration-tests

evankanderson · 2018-06-27T17:12:12Z

/assign @mdemirhan

Feel free to /assign myself or mattmoor if you need approval after Mustafa has given a /lgtm

tcnghia · 2018-06-27T20:58:46Z

@mdemirhan I updated a link to #1376 (the upstream issue istio/istio#5588 has been fixed, will be out on 1.0 release).

I've also rebased. It's ready for another look.

The timeout headers were added when we used v1alpha2 in order to allow more time for 0->1 scaling. I accidentally undo that change when moving to v1alpha3. This adds it back.

knative-metrics-robot · 2018-06-29T18:08:31Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_types.go	98.5%	98.6%	0.1
pkg/apis/serving/v1alpha1/route_types.go	95.3%	92.7%	-2.7

*TestCoverage feature is being tested, do not rely on any info here yet

mattmoor · 2018-06-29T18:18:13Z

pkg/controller/route/route.go

+		c.ServingClientSet.ServingV1alpha1().Configurations(r.Namespace),
+		c.ServingClientSet.ServingV1alpha1().Revisions(r.Namespace),
+		r)
+	// Even if targetErr != n`il, we still need to finish updating the labels so that the updates to


Going to stamp this now to avoid more merge hell, let's get this in and fix the typo in a follow up

mattmoor · 2018-06-29T18:19:39Z

/lgtm
/approve

google-prow-robot · 2018-06-29T18:19:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, mdemirhan, tcnghia

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mattmoor]

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

* Update samples to not use Ingress. This is follow-up of knative/serving#1228. We moved to Istio v1alpha3 / Gateway which requires a different way to get the external IP address to send traffic to our samples. * Also update the command to find service domain.

tcnghia added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/networking labels Jun 15, 2018

tcnghia requested review from ZhiminXiang and mdemirhan June 15, 2018 17:26

google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 15, 2018

tcnghia force-pushed the gateway branch from e740032 to d7e75ae Compare June 15, 2018 17:37

scothis mentioned this pull request Jun 15, 2018

Update to Istio v1alpha3 types knative/eventing#93

Merged

evankanderson mentioned this pull request Jun 18, 2018

Supports gRPC as a protocol (without TLS) #987

Closed

3 tasks

ZhiminXiang reviewed Jun 18, 2018

View reviewed changes

tcnghia force-pushed the gateway branch from d7e75ae to 35d8090 Compare June 25, 2018 03:10

tcnghia removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2018

tcnghia changed the title ~~[wip] move to Istio v1alpha3~~ Migrate to Istio v1alpha3 Gateway / VirtualService Jun 26, 2018

tcnghia force-pushed the gateway branch 5 times, most recently from 466f032 to fd89f7b Compare June 26, 2018 23:17

tcnghia force-pushed the gateway branch from 4c3f8de to e874b55 Compare June 27, 2018 04:14

google-prow-robot assigned mdemirhan Jun 27, 2018

tcnghia mentioned this pull request Jun 27, 2018

Remove the delays between e2e tests when switching to Istio 1.0 #1376

Closed

tcnghia force-pushed the gateway branch from 040246b to 03015c5 Compare June 27, 2018 20:50

tcnghia force-pushed the gateway branch from 307948a to f306ef5 Compare June 27, 2018 22:45

Nghia Tran added 11 commits June 29, 2018 10:35

Move to using Istio v1alpha3 API.

0fda6be

Removing ingressInformer from controllers since we no longer need.

1d415ef

Update typo and utilization target for HPA.

da7a31d

Add README.md to explain how we create these types.

3793989

Update per PR feedback

11006dd

Update per PR feedback.

f0193f4

Add back the timeout settings.

4353ad4

The timeout headers were added when we used v1alpha2 in order to allow more time for 0->1 scaling. I accidentally undo that change when moving to v1alpha3. This adds it back.

Run hack/update-codegen.sh.

9eeb490

Update per PR feedback.

2aba148

Update per PR feedback.

3bcfca6

Update per PR feedback.

d2f48b4

tcnghia force-pushed the gateway branch from 57c86cf to d2f48b4 Compare June 29, 2018 18:06

mattmoor reviewed Jun 29, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2018

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2018

google-prow-robot merged commit 22e3cb0 into knative:master Jun 29, 2018

ZhiminXiang mentioned this pull request Jun 29, 2018

Update docs and knative-routing sample to use Gateway/VirtualService #1424

Closed

sslavic mentioned this pull request Jul 4, 2018

Update "Knative with Minikube" installation docs to use Gateway/VirtualService #1484

Closed

bsnchan mentioned this pull request Jul 4, 2018

support gRPC streams in addition to HTTP #813

Closed

tcnghia mentioned this pull request Jul 25, 2018

consider using Gateways instead of k8s ingress #1681

Closed

tcnghia mentioned this pull request Oct 19, 2018

Add mdemirhan to /OWNERS #2264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Istio v1alpha3 Gateway / VirtualService #1228

Migrate to Istio v1alpha3 Gateway / VirtualService #1228

tcnghia commented Jun 15, 2018 •

edited

Loading

ZhiminXiang left a comment

ZhiminXiang Jun 18, 2018

tcnghia Jun 27, 2018

ZhiminXiang Jun 18, 2018

tcnghia Jun 27, 2018

ZhiminXiang Jun 18, 2018

tcnghia Jun 27, 2018

ZhiminXiang Jun 18, 2018

tcnghia Jun 27, 2018

ZhiminXiang Jun 18, 2018

tcnghia Jun 27, 2018

ZhiminXiang Jun 18, 2018

tcnghia Jun 27, 2018

ZhiminXiang Jun 18, 2018

tcnghia Jun 27, 2018

ZhiminXiang commented Jun 24, 2018

tcnghia commented Jun 27, 2018

tcnghia commented Jun 27, 2018

tcnghia commented Jun 27, 2018

tcnghia commented Jun 27, 2018

tcnghia commented Jun 27, 2018 •

edited

Loading

evankanderson commented Jun 27, 2018

tcnghia commented Jun 27, 2018

knative-metrics-robot commented Jun 29, 2018

mattmoor Jun 29, 2018

mattmoor Jun 29, 2018

mattmoor commented Jun 29, 2018

google-prow-robot commented Jun 29, 2018

Migrate to Istio v1alpha3 Gateway / VirtualService #1228

Migrate to Istio v1alpha3 Gateway / VirtualService #1228

Conversation

tcnghia commented Jun 15, 2018 • edited Loading

ZhiminXiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhiminXiang commented Jun 24, 2018

tcnghia commented Jun 27, 2018

tcnghia commented Jun 27, 2018

tcnghia commented Jun 27, 2018

tcnghia commented Jun 27, 2018

tcnghia commented Jun 27, 2018 • edited Loading

evankanderson commented Jun 27, 2018

tcnghia commented Jun 27, 2018

knative-metrics-robot commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Jun 29, 2018

google-prow-robot commented Jun 29, 2018

tcnghia commented Jun 15, 2018 •

edited

Loading

tcnghia commented Jun 27, 2018 •

edited

Loading