Make (cluster-local) DNS work out of the box by default. by tcnghia · Pull Request #1687 · knative/serving

tcnghia · 2018-07-25T17:35:23Z

Originally proposed by @evankanderson in #1609. Related to #1598

Proposed Changes

Change the default out-of-the-box domain to svc.cluster.local, which:
1. Documents that the name only works locally until the cluster is further configured.
2. Makes spec.domain be a DNS-resolvable name by default, in case we want to implement Consider having Route not become Ready until DNS resolves #1598
Change the k8s Service created as the public route endpoint to be an ExternalName to the knative gateway in the ingress. This makes pods launched without the istio sidecar still able to call knative. (Tested by hand.)
Fix a bug in managing the VirtualService where the same domain could be added to the VirtualService twice if the cluster's domain suffix were set to svc.cluster.local. This causes Istio to fail internal configuration propagation checks and route updates to not be applied.
Fix a bug in the VirtualService where match headers are added twice when suffix is svc.cluster.local.

Release Note

Change the default out-of-the-box domain to `svc.cluster.local`, to provide local no-DNS-setup installation of knative/serving.
Changes the route.ns.svc.cluster.local DNS name to point to the istio ingress to enable non-instio-injected pods to call knative.

tcnghia · 2018-07-25T19:00:47Z

/cc @evankanderson

ZhiminXiang · 2018-07-26T06:36:25Z

-				Port: PortNumber,
-			}},
+			Type:         corev1.ServiceTypeExternalName,
+			ExternalName: "knative-ingressgateway.istio-system.svc.cluster.local",


I would prefer using K8sGatewayServiceFullname here.

tcnghia · 2018-08-02T00:32:18Z

@ZhiminXiang PTAL

Assign @mattmoor for approval
/assign @mattmoor

tcnghia · 2018-08-02T00:45:28Z

/test pull-knative-serving-unit-tests

ZhiminXiang · 2018-08-02T06:09:07Z

/lgtm
/approve

This keeps existing behavior with `cluster.local` domain, but allow Pods without sidecar to access the same.

google-prow-robot · 2018-08-03T02:34:21Z

New changes are detected. LGTM label has been removed.

google-prow-robot · 2018-08-03T02:34:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tcnghia, ZhiminXiang

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/reconciler/OWNERS~~ [tcnghia]

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

knative-metrics-robot · 2018-08-03T02:36:32Z

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/reconciler/v1alpha1/route/resources/service.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/route/resources/names/names.go	Do not exist	100.0%

From ROLES.md 1/ Reviewer of the codebase for at least 3 months. knative#1687 is 1 year old. 2/ Primary reviewer for at least 10 substantial PRs to the codebase. 7 XLs: https://github.com/knative/serving/pulls?q=is%3Apr+assignee%3AZhiminXiang+is%3Aclosed+reviewed-by%3AZhiminXiang+label%3Asize%2FXXL 3 Ls: https://github.com/knative/serving/pulls?q=is%3Apr+assignee%3AZhiminXiang+is%3Aclosed+reviewed-by%3AZhiminXiang+label%3Asize%2FXL+ 9 Ms: https://github.com/knative/serving/pulls?q=is%3Apr+assignee%3AZhiminXiang+is%3Aclosed+reviewed-by%3AZhiminXiang+label%3Asize%2FM 3/ 33 Reviewed PRs https://github.com/knative/serving/pulls?q=is%3Apr+assignee%3AZhiminXiang+is%3Aclosed+reviewed-by%3AZhiminXiang+ 4/ 81 Merged PRs https://github.com/knative/serving/pulls?q=is%3Apr+is%3Aclosed+author%3AZhiminXiang 4/ Nominated by nghia@ with no objections from other leads

* Add ZhiminXiang to approvers. From ROLES.md 1/ Reviewer of the codebase for at least 3 months. #1687 is 1 year old. 2/ Primary reviewer for at least 10 substantial PRs to the codebase. 7 XLs: https://github.com/knative/serving/pulls?q=is%3Apr+assignee%3AZhiminXiang+is%3Aclosed+reviewed-by%3AZhiminXiang+label%3Asize%2FXXL 3 Ls: https://github.com/knative/serving/pulls?q=is%3Apr+assignee%3AZhiminXiang+is%3Aclosed+reviewed-by%3AZhiminXiang+label%3Asize%2FXL+ 9 Ms: https://github.com/knative/serving/pulls?q=is%3Apr+assignee%3AZhiminXiang+is%3Aclosed+reviewed-by%3AZhiminXiang+label%3Asize%2FM 3/ 33 Reviewed PRs https://github.com/knative/serving/pulls?q=is%3Apr+assignee%3AZhiminXiang+is%3Aclosed+reviewed-by%3AZhiminXiang+ 4/ 81 Merged PRs https://github.com/knative/serving/pulls?q=is%3Apr+is%3Aclosed+author%3AZhiminXiang 4/ Nominated by nghia@ with no objections from other leads * Sort the lines.

google-prow-robot assigned mattmoor and ZhiminXiang Jul 25, 2018

google-prow-robot requested review from grantr and vaikas July 25, 2018 17:35

google-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2018

google-prow-robot requested a review from evankanderson July 25, 2018 19:00

ZhiminXiang reviewed Jul 26, 2018

View reviewed changes

tcnghia force-pushed the domain-local branch 2 times, most recently from 62ac6ac to 68d9d9a Compare August 2, 2018 00:29

google-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 2, 2018

tcnghia force-pushed the domain-local branch from 68d9d9a to 59b6fd8 Compare August 2, 2018 00:31

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

Make cluster.local domain works without sidecar.

568a627

This keeps existing behavior with `cluster.local` domain, but allow Pods without sidecar to access the same.

tcnghia force-pushed the domain-local branch from 59b6fd8 to 568a627 Compare August 3, 2018 02:34

google-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 3, 2018

tcnghia closed this Aug 3, 2018

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

tcnghia mentioned this pull request Mar 11, 2020

Add ZhiminXiang to networking-approvers. #7215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make (cluster-local) DNS work out of the box by default. #1687

Make (cluster-local) DNS work out of the box by default. #1687
tcnghia wants to merge 1 commit into
knative:masterfrom
tcnghia:domain-local

tcnghia commented Jul 25, 2018

Uh oh!

tcnghia commented Jul 25, 2018

Uh oh!

ZhiminXiang Jul 26, 2018

Uh oh!

tcnghia Jul 27, 2018

Uh oh!

tcnghia commented Aug 2, 2018

Uh oh!

tcnghia commented Aug 2, 2018

Uh oh!

ZhiminXiang commented Aug 2, 2018

Uh oh!

google-prow-robot commented Aug 3, 2018

Uh oh!

google-prow-robot commented Aug 3, 2018

Uh oh!

knative-metrics-robot commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

tcnghia commented Jul 25, 2018

Proposed Changes

Uh oh!

tcnghia commented Jul 25, 2018

Uh oh!

ZhiminXiang Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

tcnghia Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

tcnghia commented Aug 2, 2018

Uh oh!

tcnghia commented Aug 2, 2018

Uh oh!

ZhiminXiang commented Aug 2, 2018

Uh oh!

google-prow-robot commented Aug 3, 2018

Uh oh!

google-prow-robot commented Aug 3, 2018

Uh oh!

knative-metrics-robot commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants