-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add new local Gateway sharing same deployment as ingress Gateway. #237
Add new local Gateway sharing same deployment as ingress Gateway. #237
Conversation
/assign @nak3 |
I am looking into the test failure. |
I need more data. |
I am root-causing these new flakes (they also affect |
@JRBANCEL does the |
The We specifically don't want that port (8081) to be open, because then cluster local Knative Services would be accessible from outside. |
Yes, this migration plan sounds good to me. |
This plan sounds good to me. Now the envoy pods will have the configurations of both public Ingress and cluster-local Ingress. I think envoy should have the mechanism to isolate the public config and cluster-local config. But we may still want to double check that users cannot access cluster-local ingress from outside of the cluster just in case. |
You may want to add a RELEASE NOTE section in the PR description to make it easier to remember to include in release notes / announcements. Otherwise |
Yes, I tested that. Since the cluster local Gateway is on a port that is not exposed internally, it can't be accessed. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JRBANCEL, ZhiminXiang 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:
Approvers can indicate their approval by writing |
@JRBANCEL is this ready for |
Also I assigned you knative/serving#6269 since I thought it would be related to this work. |
/hold cancel |
}, { | ||
Namespace: system.Namespace(), | ||
Name: KnativeLocalGateway, | ||
ServiceURL: fmt.Sprintf(KnativeLocalGateway+".knative-serving.svc.%s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be using system.Namespace()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, we have a helper for synthesizing service hostnames that this should be using too in place of GetClusterDomainName()
Related to #212.
The idea is to create a new
Gateway
namedknative-local-gateway
(more consistent withknative-ingress-gateway
thancluster-local-gateway
). This Gateway binds to the same Envoy Pods as the Istio ingress Gateway, but on a different port (8081
because8080
is already used by Istio). There is a corresponding Kubernetes Service mapping namedknative-local-gateway
living inknative-serving
(cleaner thancluster-local-gateway
living inistio-system
) mapping port80
to port8081
.knative-local-gateway
is added to the default Istio Gateways, this way the VirtualServices of local Knative Services bind to it (i.e. ready to serve traffic), but because theKIngress
status uses the first local Gateway in its status, the Services of typeExternalName
still point tocluster-local-gateway
(and it is not accessible to external requests since8081
is not exposed through theLoadBalancer
Service).Effectively, this is a no-op, but in the next release we can remove reference to
cluster-local-gateway
in defaultconfig-istio
and there will be no traffic interruption and the requests will flow toknative-local-gateway
.After that, everything related to
cluster-local-gateway
can be safely removed, including theDeployment
, which is the end goal here./hold
/assign @tcnghia
/assign @ZhiminXiang
Let me know what you think.