Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate using istio-ingressgateway by default #1969

Closed
tcnghia opened this issue Aug 29, 2018 · 7 comments
Closed

Investigate using istio-ingressgateway by default #1969

tcnghia opened this issue Aug 29, 2018 · 7 comments
Assignees
Labels
area/API API objects and controllers area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/doc Something isn't clear kind/feature Well-understood/specified features, ready for coding.
Milestone

Comments

@tcnghia
Copy link
Contributor

tcnghia commented Aug 29, 2018

Expected Behavior

Knative Serving installation does not need to install something in istio-system namespace. Knative Serving does not have to keep up with changes in Envoy proxy images.

Actual Behavior

We are installing an ELB knative-ingressgateway into istio-system namespace. Also, we are using a copy of Istio ELB istio-ingressgateway which requires making sure it is up-to-date with Istio's.

This was so that our Gateway can use those pods without conflicting with users' own Gateways.

However, Istio 1.0.1 may in better merging of Gateways, so may be we can base on Gateway Hosts to avoid conflicting (this didn't work with 0.8). Also Istio now have validation so users Gateway conflict may be less confusing than before (silent unavailability of service)

We should investigate allowing the users to choose what ingressgateway ELB they can use, which defaults to the Istio default istio-ingressgateway. This way we not only avoid maintaining this service ourselves, we also avoid installing things into istio-system namespace and save the user 1 static IP.

@knative-prow-robot knative-prow-robot added area/API API objects and controllers area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 29, 2018
@knative-prow-robot knative-prow-robot added kind/doc Something isn't clear kind/feature Well-understood/specified features, ready for coding. labels Aug 29, 2018
tcnghia pushed a commit to tcnghia/serving that referenced this issue Aug 29, 2018
TODO(knative#1969): We should allow the users to use `istio-ingressgateway`
by default, while having the choice to specify their own Ingress
Gateway service if that isn't enough for them.  That way we can get
out of maintaining these YAML ourselves.
tcnghia pushed a commit to tcnghia/serving that referenced this issue Aug 29, 2018
TODO(knative#1969): We should allow the users to use `istio-ingressgateway`
by default, while having the choice to specify their own Ingress
Gateway service if that isn't enough for them.  That way we can get
out of maintaining these YAML ourselves.
@tcnghia
Copy link
Contributor Author

tcnghia commented Aug 29, 2018

However for backward compatibility we probably want to keep providing knative-ingressgateway Service until the next release and update docs to transition away.

@SergeC
Copy link

SergeC commented Aug 30, 2018

@tcnghia Since Knative is in alpha or beta stage it's common case if backward compatibility got broken. More important to keep fast development to get it to the production stage.

knative-prow-robot pushed a commit that referenced this issue Aug 31, 2018
TODO(#1969): We should allow the users to use `istio-ingressgateway`
by default, while having the choice to specify their own Ingress
Gateway service if that isn't enough for them.  That way we can get
out of maintaining these YAML ourselves.
@lichuqiang
Copy link
Contributor

/cc

@tcnghia
Copy link
Contributor Author

tcnghia commented Nov 6, 2018

/assign @lichuqiang

@lichuqiang
Copy link
Contributor

lichuqiang commented Nov 8, 2018

Took a closer look at the issue, here are general thoughts:
keep the 'Gateway' resource as is, what changed is in fact underlying gateway service/deployment, so on gateway update, we'll need to:

  1. feel the change in ingress reconciler, and update Loadbalancer in ingress status accordingly.
  2. update the label selector and ports fields accordingly in knative-shared-gateway

and here are the tasks I think needed to achieve this:

  1. add a config item for the gateway service url in a new configmap, and default its value to istio-ingressgateway.istio-system.svc.cluster.local, ClusterIngress reconciler is supposed to watch the configmap, and trigger global resync on a change.

  2. update existing knative-ingressgateway usage in test codes to the default istio-ingressgateway. I don't see the requirement to change the config in tests, so hard code it should be enough. @tcnghia thoughts?

  3. update the docs to tell users how to replace the default gateway with their own. Currently we are not managing configs with tools like helm, so I'm afraid users will have to update fields in Gateway template manually. Suggestions?

@tcnghia
Copy link
Contributor Author

tcnghia commented Nov 8, 2018

@lichuqiang #1,#2 sound good to me.
For #3, we could include current knative-ingressgateway template as an example, and may be link to the appropriate Istio discussion so our users know where to get help. In the end I think this area belongs to the Istio side and users would get better support there.

@tcnghia
Copy link
Contributor Author

tcnghia commented Dec 7, 2018

Per discussion with @jonjohnsonjr and @mattmoor we will extend feature this to allow exposing to multiple ingress gateway to allow users to easily migrate without disruption.

In 0.3 we will expose to both knative-ingressgateway and istio-ingressgateway. In 0.4 we will only expose to istio-ingressgateway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/doc Something isn't clear kind/feature Well-understood/specified features, ready for coding.
Projects
None yet
Development

No branches or pull requests

5 participants