Allow config ingress gateway #2434

lichuqiang · 2018-11-08T12:25:52Z

Fixes #1969

Proposed Changes

add clusteringress config
update ingress reconciler to watch the configmap
remove knative-ingressgateway usage
add docs to tell how to set up gateway

Release Note

- Remove `knative-ingressgateway`.
- Allow users to choose an alternative gateway other than `istio-ingressgateway`.

/area networking
/kind feature

knative-prow-robot

@lichuqiang: 6 warnings.

In response to this:

Fixes #1969

Proposed Changes

add clusteringress config

update ingress reconciler to watch the configmap

remove knative-ingressgateway usage

add docs to tell how to set up gateway

Release Note
allow config ingress gateway
/area networking
/kind feature

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/config/store.go

knative-prow-robot

@lichuqiang: 2 unresolved warnings and 0 new warnings.

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-11-19T22:43:04Z

/retest

lichuqiang · 2018-11-20T01:32:39Z

The integration tests won't pass until we update the test dependency in pkg repo. But I'd make sure the code is fine to merge before that. As it will block things if we update the pkg repo first and not be able to get this merged.

Let me rebase onto the newest code first :)

lichuqiang · 2018-11-20T08:35:38Z

The PR is ready for review :)

tcnghia

/approve

Thanks a lot for making this change.

There are two comments relating to replacing some docs with example bash commands and automation of knative-shared-gateway updates, but feel free to address them in future PR.

tcnghia · 2018-11-20T15:42:35Z

docs/setting-up-custom-ingress-gateway.md

+Replace its label selector with the label of your service:
+
+```
+istio: ingressgateway


I think this step can be automated by reading the label selector of custom-ingressgateway.istio-system.svc, may be from the ClusterIngress controller.

Good point, I think we could address this before merging the PR.

A tiny issue is that the automatically generated label selector can be different from what users initially wanted to see.
e.g. in current knative-shared-gateway, the labels in selector is "knative: ingressgateway", while in the service, the labels in selector are "app: knative-ingressgateway, knative: ingressgateway".
But it has no functional effect, so I think it's fine.

I've got an implementation here: lichuqiang@a624bee

But the modification is bigger than I expected. So let's address it in another PR, to keep things light.

And I can't decide how to deal with the servers(port) info update in gateway. The fields can also change, and might not so regular like that of selector: Users probably only want to expose some of the service ports in the gateway.

Maybe we should just leave that to users for manually update?

Let's discuss in the follow up PR instead.

I think we should only propagate changes related to the label selector to Gateway.

tcnghia · 2018-11-20T15:52:06Z

docs/setting-up-ingress-static-ip.md

 ```

 The result should be something like

 ```console
 NAME                     TYPE           CLUSTER-IP      EXTERNAL-IP     PORT(S)                                      AGE
-knative-ingressgateway   LoadBalancer   10.50.250.120   35.210.48.100   80:32380/TCP,443:32390/TCP,32400:32400/TCP   5h
+istio-ingressgateway   LoadBalancer   10.50.250.120   35.210.48.100   80:31380/TCP,443:31390/TCP,31400:31400/TCP...   5h


indent columns?

tcnghia · 2018-11-20T16:00:58Z

docs/setting-up-custom-ingress-gateway.md

+
+Here is an example:
+
+```


Can we instead provide some bash command instead? Either way we will run into staleness issue, which may be fine for an example, but the substitution command will (hopefully) explain the idea better since it's not obvious from the yaml what the difference.

That's not blocking, we can follow up with another PR.

I have a one-liner here

https://github.com/knative/serving/pull/2546/files#diff-d11ce1cfee915c30534db0ffe418081f

mattmoor · 2018-11-22T14:21:50Z

/retest

tcnghia · 2018-11-22T14:50:36Z

@mattmoor these tests won't pass until we pull in the new hard coded from knative/pkg, but if we land the change in pkg too early it would break other people's tests

tcnghia · 2018-11-22T15:11:02Z

@liquchiang I am not sure your changes in knative/pkg would address upgrade test. The only way I can think of is instead we get the IP address to test based on the ExternalName pre-Route Service.

lichuqiang · 2018-11-23T01:39:59Z

I am not sure your changes in knative/pkg would address upgrade test. The only way I can think of is instead we get the IP address to test based on the ExternalName pre-Route Service.

Hmm, as I see, the upgrade test is to test if we still can access to an existing service in the env after serving components upgrade.
And with the change in knative/pkg, both the ingress service under knative shared gateway and the request url in spoof client should be updated to istio-ingressgateway. Am I missing something?

tcnghia · 2018-11-23T22:21:42Z

@lichuqiang I see. I was only worried that the upgrade test will use the same hard-coded load balancer to test that a service in both the old-version and new-version is accessible through the same gateway IP address. I am not sure if this is the case, may be the upgrade test won't both with checking the service is accessible in the old-version. @jonjohnsonjr can you please clarify whether we send requests to a service when bringing it up in old-version?

jonjohnsonjr · 2018-11-24T03:35:43Z

I haven't ready too much into this so forgive me if I'm missing something, but I think we'd have to make some changes either to the upgrade test or to the spoof client. The upgrade test uses the spoof client from HEAD, so you'd be unable to hit the old service (pre-upgrade and post-downgrade) if you just fix the spoof client to use istio-ingressgateway.

We might need some fancy logic to fallback to knative-ingressgateway here if istio-ingressgateway doesn't exist.

lichuqiang · 2018-11-28T02:22:18Z

I get your point. Let me add some compatibility into the spoof client

googlebot · 2018-11-28T07:29:45Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-11-28T07:43:13Z

CLAs look good, thanks!

knative-metrics-robot · 2018-11-28T07:45:52Z

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	70.1%	82.4%	12.3

tcnghia · 2018-11-30T01:12:16Z

/test pull-knative-serving-integration-tests

tcnghia · 2018-11-30T01:42:07Z

/test pull-knative-serving-integration-tests

tcnghia · 2018-11-30T02:12:06Z

/retest

tcnghia · 2018-11-30T03:11:42Z

/test pull-knative-serving-integration-tests

mattmoor · 2018-11-30T04:08:41Z

/approve

knative-prow-robot · 2018-11-30T04:08:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lichuqiang, 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-11-30T04:17:30Z

/lgtm

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/networking kind/feature Well-understood/specified features, ready for coding. labels Nov 8, 2018

knative-prow-robot requested review from dushyanthsc, ericKlawitter, mdemirhan and tcnghia November 8, 2018 12:25

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 8, 2018

knative-prow-robot reviewed Nov 8, 2018

View reviewed changes

lichuqiang force-pushed the remove_gateway branch from 10742d4 to 16500d5 Compare November 8, 2018 12:37

knative-prow-robot reviewed Nov 8, 2018

View reviewed changes

lichuqiang force-pushed the remove_gateway branch 2 times, most recently from cdb16d1 to ef0e6a7 Compare November 12, 2018 01:49

lichuqiang mentioned this pull request Nov 12, 2018

Route Global Resync test is still flaky #2443

Closed

lichuqiang force-pushed the remove_gateway branch 2 times, most recently from f9f4dfe to 7ed751f Compare November 13, 2018 12:39

lichuqiang changed the title ~~WIP: Allow config ingress gateway~~ Allow config ingress gateway Nov 13, 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 Nov 13, 2018

lichuqiang mentioned this pull request Nov 14, 2018

Add configmap for clusteringress #2486

Merged

lichuqiang force-pushed the remove_gateway branch 2 times, most recently from f1bf86a to b4333b6 Compare November 16, 2018 08:52

lichuqiang force-pushed the remove_gateway branch from b4333b6 to 9677c9e Compare November 20, 2018 02:37

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 Nov 20, 2018

mattmoor assigned tcnghia Nov 20, 2018

tcnghia approved these changes Nov 20, 2018

View reviewed changes

lichuqiang mentioned this pull request Nov 21, 2018

update gateway knative/pkg#170

Merged

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2018

lichuqiang added 5 commits November 28, 2018 15:35

update ingress reconciler to watch the configmap

d400109

remove knative-ingressgateway usage

a0981a5

add docs to tell how to set up gateway

a3b70bd

indent columns

4f9f46c

sync pkg

f64c1ca

lichuqiang force-pushed the remove_gateway branch from a26334c to f64c1ca Compare November 28, 2018 07:43

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

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2018

knative-prow-robot merged commit 2a6b811 into knative:master Nov 30, 2018

sjanota mentioned this pull request Dec 5, 2018

Proposal - How to run Kyma on top of Knative kyma-project/community#150

Closed

lichuqiang mentioned this pull request Dec 7, 2018

sync gateway label selector on config update #2663

Closed

sjanota mentioned this pull request Jan 3, 2019

Upgrade Knative to newest compatible version kyma-project/kyma#2153

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow config ingress gateway #2434

Allow config ingress gateway #2434

lichuqiang commented Nov 8, 2018 •

edited by tcnghia

knative-prow-robot left a comment

knative-prow-robot left a comment

tcnghia commented Nov 19, 2018

lichuqiang commented Nov 20, 2018

lichuqiang commented Nov 20, 2018

tcnghia left a comment

tcnghia Nov 20, 2018

lichuqiang Nov 21, 2018 •

edited

lichuqiang Nov 21, 2018

tcnghia Nov 22, 2018

tcnghia Nov 20, 2018

tcnghia Nov 20, 2018

tcnghia Nov 26, 2018

mattmoor commented Nov 22, 2018

tcnghia commented Nov 22, 2018

tcnghia commented Nov 22, 2018

lichuqiang commented Nov 23, 2018 •

edited

tcnghia commented Nov 23, 2018

jonjohnsonjr commented Nov 24, 2018

lichuqiang commented Nov 28, 2018

googlebot commented Nov 28, 2018

googlebot commented Nov 28, 2018

knative-metrics-robot commented Nov 28, 2018

tcnghia commented Nov 30, 2018

tcnghia commented Nov 30, 2018

tcnghia commented Nov 30, 2018

tcnghia commented Nov 30, 2018

mattmoor commented Nov 30, 2018

knative-prow-robot commented Nov 30, 2018

tcnghia commented Nov 30, 2018

Allow config ingress gateway #2434

Allow config ingress gateway #2434

Conversation

lichuqiang commented Nov 8, 2018 • edited by tcnghia

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

tcnghia commented Nov 19, 2018

lichuqiang commented Nov 20, 2018

lichuqiang commented Nov 20, 2018

tcnghia left a comment

Choose a reason for hiding this comment

tcnghia Nov 20, 2018

Choose a reason for hiding this comment

lichuqiang Nov 21, 2018 • edited

Choose a reason for hiding this comment

lichuqiang Nov 21, 2018

Choose a reason for hiding this comment

tcnghia Nov 22, 2018

Choose a reason for hiding this comment

tcnghia Nov 20, 2018

Choose a reason for hiding this comment

tcnghia Nov 20, 2018

Choose a reason for hiding this comment

tcnghia Nov 26, 2018

Choose a reason for hiding this comment

mattmoor commented Nov 22, 2018

tcnghia commented Nov 22, 2018

tcnghia commented Nov 22, 2018

lichuqiang commented Nov 23, 2018 • edited

tcnghia commented Nov 23, 2018

jonjohnsonjr commented Nov 24, 2018

lichuqiang commented Nov 28, 2018

googlebot commented Nov 28, 2018

googlebot commented Nov 28, 2018

knative-metrics-robot commented Nov 28, 2018

tcnghia commented Nov 30, 2018

tcnghia commented Nov 30, 2018

tcnghia commented Nov 30, 2018

tcnghia commented Nov 30, 2018

mattmoor commented Nov 30, 2018

knative-prow-robot commented Nov 30, 2018

tcnghia commented Nov 30, 2018

lichuqiang commented Nov 8, 2018 •

edited by tcnghia

lichuqiang Nov 21, 2018 •

edited

lichuqiang commented Nov 23, 2018 •

edited