Support exposing ClusterIngress to two or more shared Gateways. #2666

tcnghia · 2018-12-07T06:55:21Z

Proposed Changes

In order for existing users to migrate cleanly off existing knative-ingressgateway we need to have a mechanism to allow them to expose their Services to two or more External LoadBalancer and cleanly update the domain to point to new IP address. This change makes that possible.

Hence even though #1969 is fixed we will leave the knative-ingressgateway around and will have it removed may be when 0.4 rolls out.

Release Note

Reinstate the `knative-ingressgateway` to allow existing users to upgrade cleanly.

knative-prow-robot

@tcnghia: 4 warnings.

In response to this:

Proposed Changes

In order for existing users to migrate cleanly off existing knative-ingressgateway we need to have a mechanism to allow them to expose their Services to two or more External LoadBalancer and cleanly update the domain to point to new IP address. This change makes that possible.

So even though #1969 is fixed we will leave the knative-ingressgateway around and will have it removed may be when 0.4 rolls out.

Release Note
Reinstate the `knative-ingressgateway` to allow existing users to upgrade cleanly.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

pkg/reconciler/v1alpha1/clusteringress/config/istio.go

tcnghia · 2018-12-07T07:03:21Z

/lint

knative-prow-robot

@tcnghia: 1 unresolved warning and 0 new warning.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tcnghia · 2018-12-07T07:28:00Z

/test pull-knative-serving-unit-tests

tcnghia · 2018-12-07T08:18:05Z

/test pull-knative-serving-integration-tests

mattmoor · 2018-12-07T19:10:05Z

config/config-istio.yaml

+  # ${gatewayName}:${gatewayService}, where ${gatewayName} is the name of
+  # the Gateway, and ${gatewayService} is the URL of the service backing
+  # that gateway.
+  ingress-gateway: "knative-ingress-gateway:istio-ingressgateway.istio-system.svc.cluster.local;knative-shared-gateway:knative-ingressgateway.istio-system.svc.cluster.local"


let's use a wildcard on keys, like this:

serving/config/config-logging.yaml

Lines 48 to 52 in 0e7cbf3

loglevel.controller: "info"

loglevel.autoscaler: "info"

loglevel.queueproxy: "info"

loglevel.webhook: "info"

loglevel.activator: "info"

*-gateway: in config-istio.yaml seems like a safe compromise :)

Since I don't want the Istio Gateway CRD to be named with -gateway suffix all the time, I opted to use a "gateway." prefix instead.

tcnghia · 2018-12-07T19:16:55Z

/test pull-knative-serving-upgrade-tests

mattmoor

couple nits / questions

mattmoor · 2018-12-07T21:41:07Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go

-	originGateway = "origin.ns.svc.cluster.local"
-	customGateway = "custom.ns.svc.cluster.local"
+	originDomainInternal = "origin.ns.svc.cluster.local"
+	newDomainInternal    = "custom.ns.svc.cluster.local"


use these constants above?

mattmoor · 2018-12-07T21:41:56Z

pkg/reconciler/v1alpha1/clusteringress/config/istio.go

@@ -31,26 +33,51 @@ const (
 	// IngressGatewayKey is the name of the configuration entry
 	// that specifies ingress gateway url.
 	IngressGatewayKey = "ingress-gateway"


mattmoor · 2018-12-07T21:42:35Z

pkg/reconciler/v1alpha1/clusteringress/config/istio.go

+	urls := map[string]string{}
+	gatewayNames := []string{}
+	for k, v := range configMap.Data {
+		if strings.HasPrefix(k, GatewayKeyPrefix) {


nit: if !... { continue } to reduce indent.

mattmoor · 2018-12-07T21:43:45Z

pkg/reconciler/v1alpha1/clusteringress/config/store_test.go

@@ -32,12 +32,10 @@ func TestStoreLoadWithContext(t *testing.T) {
 	store.OnConfigChanged(istioConfig)
 	config := FromContext(store.ToContext(context.Background()))

-	t.Run("load istio", func(t *testing.T) {


Why'd you remove the t.Run?

the test name isn't changing inside a for loop, I didn't see any purpose for the t.Run

vagababov · 2018-12-07T22:09:11Z

pkg/reconciler/v1alpha1/clusteringress/config/istio.go

+		if !strings.HasPrefix(k, GatewayKeyPrefix) {
+			continue
+		}
+		gatewayName, serviceURL := strings.TrimPrefix(k, GatewayKeyPrefix), v


k[len(GatewayKeyPrefix):] gonna be much faster, since you already know the prefix is there

vagababov · 2018-12-07T22:09:53Z

pkg/reconciler/v1alpha1/clusteringress/config/istio.go

-	if errs := validation.IsDNS1123Subdomain(gateway); len(errs) > 0 {
-		return nil, fmt.Errorf("invalid gateway format: %v", errs)
+	if len(gatewayNames) == 0 {
+		return nil, fmt.Errorf("at least one gateway is required")


No formatting --> Error()

vagababov · 2018-12-07T22:10:19Z

pkg/reconciler/v1alpha1/clusteringress/config/istio.go

+		return nil, fmt.Errorf("at least one gateway is required")
+	}
+	sort.Strings(gatewayNames)
+	gateways := []IngressGateway{}


make([]IG, 0, len(gatewayNames)

mattmoor

LGTM, but I agree with @vagababov nits. Thanks @vagababov :)

knative-metrics-robot · 2018-12-07T22:17:40Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/clusteringress/clusteringress.go	82.7%	83.5%	0.9
pkg/reconciler/v1alpha1/clusteringress/config/istio.go	100.0%	94.1%	-5.9

mattmoor

/lgtm
/approve
/hold

@vagababov If this LGTY then /hold cancel

knative-prow-robot · 2018-12-07T22:22:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, 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

tcnghia · 2018-12-07T22:28:04Z

/hold cancel
@vagababov already /lgtmed

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 7, 2018

knative-prow-robot requested review from lichuqiang and markusthoemmes December 7, 2018 06:55

knative-prow-robot reviewed Dec 7, 2018

View reviewed changes

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 7, 2018

knative-prow-robot reviewed Dec 7, 2018

View reviewed changes

tcnghia force-pushed the two-gateways branch from e0fdcb8 to 4e2b482 Compare December 7, 2018 17:11

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 7, 2018

tcnghia force-pushed the two-gateways branch from 4e2b482 to ffc5aee Compare December 7, 2018 17:23

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 7, 2018

tcnghia changed the title ~~[WIP] Support exposing ClusterIngress to two or more shared Gateways.~~ Support exposing ClusterIngress to two or more shared Gateways. Dec 7, 2018

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2018

mattmoor self-assigned this Dec 7, 2018

mattmoor reviewed Dec 7, 2018

View reviewed changes

Nghia Tran added 4 commits December 7, 2018 14:21

Support exposing ClusterIngress to two or more Gateways.

e685e69

Fix golint. Update deps.

6b1674b

Run update-dep again.

ad3f514

Add documentation for ConfigMap entry.

a60f28a

tcnghia force-pushed the two-gateways branch from 0ec6d59 to a60f28a Compare December 7, 2018 19:22

Update config map layout per PR feedback.

119816b

mattmoor reviewed Dec 7, 2018

View reviewed changes

Address PR feedbacks.

02f9a26

vagababov reviewed Dec 7, 2018

View reviewed changes

mattmoor reviewed Dec 7, 2018

View reviewed changes

Address PR feedback.

94cc231

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Dec 7, 2018

mattmoor reviewed Dec 7, 2018

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 7, 2018

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2018

knative-prow-robot merged commit 241f470 into knative:master Dec 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support exposing ClusterIngress to two or more shared Gateways. #2666

Support exposing ClusterIngress to two or more shared Gateways. #2666

tcnghia commented Dec 7, 2018 •

edited

Loading

knative-prow-robot left a comment

tcnghia commented Dec 7, 2018

knative-prow-robot left a comment

tcnghia commented Dec 7, 2018

tcnghia commented Dec 7, 2018

mattmoor Dec 7, 2018

tcnghia Dec 7, 2018

tcnghia commented Dec 7, 2018

mattmoor left a comment

mattmoor Dec 7, 2018

tcnghia Dec 7, 2018

mattmoor Dec 7, 2018

tcnghia Dec 7, 2018

mattmoor Dec 7, 2018

tcnghia Dec 7, 2018

mattmoor Dec 7, 2018

tcnghia Dec 7, 2018

vagababov Dec 7, 2018

tcnghia Dec 7, 2018

vagababov Dec 7, 2018

tcnghia Dec 7, 2018

vagababov Dec 7, 2018

tcnghia Dec 7, 2018

mattmoor left a comment

knative-metrics-robot commented Dec 7, 2018

mattmoor left a comment

knative-prow-robot commented Dec 7, 2018

tcnghia commented Dec 7, 2018

	loglevel.controller: "info"
	loglevel.autoscaler: "info"
	loglevel.queueproxy: "info"
	loglevel.webhook: "info"
	loglevel.activator: "info"

Support exposing ClusterIngress to two or more shared Gateways. #2666

Support exposing ClusterIngress to two or more shared Gateways. #2666

Conversation

tcnghia commented Dec 7, 2018 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

tcnghia commented Dec 7, 2018

knative-prow-robot left a comment

Choose a reason for hiding this comment

tcnghia commented Dec 7, 2018

tcnghia commented Dec 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcnghia commented Dec 7, 2018

mattmoor 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

mattmoor left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Dec 7, 2018

mattmoor left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Dec 7, 2018

tcnghia commented Dec 7, 2018

tcnghia commented Dec 7, 2018 •

edited

Loading