Add DomainInternal to status for Route and Service #1586

scothis · 2018-07-12T21:38:09Z

Resources exposed by a dns name outside of the cluster should also have a dns name that can target them from inside the cluster. Routes and Services (which already have a Domain status property) now include a DomainInternal status property.

Fixes: #1584

Proposed Changes

Add .status.domainInternal to Service and Route

Release Note

NONE

/assign @vaikas-google

mattmoor

We should probably update these: https://github.com/knative/serving/blob/master/docs/spec/spec.md#route

cc @cooperneil @evankanderson

mattmoor · 2018-07-12T22:55:56Z

pkg/apis/serving/v1alpha1/route_types.go

@@ -131,6 +131,12 @@ type RouteStatus struct {
 	// +optional
 	Domain string `json:"domain,omitempty"`

+	// ServiceName holds the name of a core Kubernetes Service resource that
+	// fonts this Route, this service would be an appropriate ingress target for


mattmoor · 2018-07-12T22:56:17Z

pkg/apis/serving/v1alpha1/service_types.go

@@ -134,6 +134,13 @@ type ServiceStatus struct {
 	// +optional
 	Domain string `json:"domain,omitempty"`

+	// From RouteStatus.
+	// ServiceName holds the name of a core Kubernetes Service resource that
+	// fonts this Route, this service would be an appropriate ingress target for


scothis · 2018-07-13T13:19:35Z

@mattmoor PTAL

dprotaso · 2018-07-13T13:47:14Z

Since you're modifying the spec you should update the conformance test as per the recommendation here:

https://github.com/knative/serving/tree/master/docs/spec#knative-serving-api-spec

mattmoor

/approve
/lgtm

mattmoor · 2018-07-13T13:49:26Z

/hold

Just saw @dprotaso comment

dprotaso · 2018-07-13T18:15:55Z

/lgtm

scothis · 2018-07-13T19:05:27Z

@mattmoor added conformance test PTYAL

mattmoor

/lgtm

vaikas · 2018-07-13T20:10:09Z

pkg/apis/serving/v1alpha1/route_types.go

@@ -131,6 +131,12 @@ type RouteStatus struct {
 	// +optional
 	Domain string `json:"domain,omitempty"`

+	// ServiceName holds the name of a core Kubernetes Service resource that


Could this also be Virtual Service? Since it's a string there's really no different from this point, but just curious if we should document it somewhere?

A VirtualService maps traffic from one Service to another Service, so there is still a core Service created by the route controller. The VirtualService is an implementation detail behind the Service much like a Deployment is, as far as the sender knows, they are talking to a Service.

vaikas · 2018-07-13T20:12:05Z

/lgtm

evankanderson

Question: how does serviceName differ from domain? From my POV:

Both should be DNS-resolvable names
Both should be reachable within the cluster
Both should apply Istio rules

I believe @tcnghia ia already creating a k8s service for existing VirtualServices. If we want an "internal only" name, we could call it internalDomain, but I'd rather pretend that the DNS names presented between knative/serving and knative/eventing are unified within a single k8s cluster (which is as far as an ObjectReference can reach).

Is this solely about getting a shorter hostname for the dispatcher to make HTTP calls to?

evankanderson · 2018-07-13T20:18:11Z

pkg/controller/route/cruds.go

@@ -81,6 +81,7 @@ func (c *Controller) reconcilePlaceholderService(ctx context.Context, route *v1a
 			return err
 		}
 		logger.Infof("Created service %s", name)
+		route.Status.ServiceName = resourcenames.K8sService(route)


Why doesn't this use desiredService.Name?

scothis · 2018-07-13T22:06:06Z

@evankanderson funny enough I started this work by adding an DomainInternal property to RouteStatus, but then I saw that Revisions already had a ServiceName property and instead standardized on that.

how does serviceName differ from domain?

I generally agree with your points. The difference is that domain is set using external dns. {route}.{ns}.example.com

This has two major drawbacks:

it requires external DNS to be configured correctly, example.com won't cut it
it implies the resource is accessible from outside the cluster. This is true for routes, but not true for revisions or channels.

Is this solely about getting a shorter hostname for the dispatcher to make HTTP calls to?

The length isn't my concern, but instead having a resource I know I can reliably address from inside the cluster without needing to know the naming conventions used by the serving controller.

evankanderson · 2018-07-13T23:05:41Z

For #1, would defaulting to svc.cluster.local as a prefix for the domain instead of example.com resolve the DNS concerns?

With respect to whether the name is reachable outside the cluster, I wouldn't expect that traffic sent directly to a Revision for any purpose would necessarily work (for example, we might scale down to zero, remove Revision's service, and instead route that traffic to an activator). If we're treating domain as an opaque DNS name, I'd expect that a Channel could report a domain (or dnsname) property that pointed to .cluster.local even if knative had a different configured prefix (and using a fully-qualified name might prevent additional lookups due to ndots settings).

scothis · 2018-07-14T01:54:36Z

I'm 👍 on creating a .status.domainInternal property with the value {name}.{ns}.svc.cluster.local.

For resoruces that own a core Kubernetes Services, the name of the service should be exposed on the resoruce's status. The Revision resources already exposes `.status.serviceName`, this PR adds the same property for the Route and Knative Service resources. Yes, it's a bit strange to have a Serivce include the Service name in it's status, ¯\_(ツ)_/¯ Fixes: #1584

- typos - added status property to spec.md

scothis · 2018-07-16T03:41:36Z

/test pull-knative-serving-integration-tests

scothis · 2018-07-16T13:31:58Z

@evankanderson I added .status.domainInternal in addition to .status.serviceName PTAL

I skipped conformance tests for the new property since it's well covered by unit tests and we'd need to resolve DNS from inside the cluster. The best method to do that I could figure out is to deploy a pod into the cluster with nslookup or dig and then kubectl run against it to get the IP. I can take a pass at implementing that behavior if you think it's worthwhile.

evankanderson · 2018-07-16T16:57:57Z

We should (separately) probably do better about making sure that the cluster is in a proper state with respect to DNS. One thing I'm thinking of trying out would be to change the config-domain.yaml to default to svc.cluster.local, which would make the cluster "work" for internal traffic out of the box, and make it clear that DNS + Knative configuration would be needed to make external traffic to the cluster work (which is already the case).

With respect to having spec include a service name vs a DNS name, my preference would be for using (only) a DNS name, rather than both a DNS name and a service name. The service name seems like it:

Is leaking lower-level k8s abstractions to the user that aren't necessary (especially since knative actually wants users to use an istio VirtualService, rather than a k8s service).
Introduces an additional set of object types which need to be exposed. This is more of a concern for other platforms which are introducing a "Knative adapter layer".
Is only used to provide a host:port, which is also provided by the SRV records published by Service.
A k8s Service can include multiple ports. In such a case, it's not clear what port should be used to connect to.

It sounds like Ville is not a huge fan of using domainname to mean dnsname. I don't have a strong feeling here, though I think it's a bit late to rename the field in v1alpha1 with client tools and so forth written. It sounds like a good way to test CRD versioning for v1alpha2 in a few weeks (beta feature in 1.11).

evankanderson · 2018-07-16T17:14:56Z

Opened #1598 for the serving changes to make DNS usually work.

scothis · 2018-07-16T18:27:35Z

@evankanderson I'm on board with using a dns name instead of a service name, however, I don't like conflating internal and external dns names. If you know you're talking to something inside the cluster, it seems better to use an internal dns name:

it's clear the connection is staying inside the cluster. Some organizations have higher audit requirements for traffic that leaves a system vs traffic that staying within a system
some resources should never be exposed outside of the cluster, while it's possible to setup rules to restrict traffic flow, failing in a safer way is better. For example, it should be possible to send traffic to a channel which is not exposed publicly.

This PR currently uses DomainInternal to expose the in cluster dns name along side Domain which is the external dns name. Even if we change the name in v1alpha2, I like the symmetry this offers today.

evankanderson

I'm happy to approve the DomainInternal part, but let's keep k8s Service out (particularly of Knative Service).

For v1alpha2, we might want to rename domain and domainInternal to something like dnsName and localDnsName, but let's keep DomainInternal or LocalDomain names in v1alpha1 for parallelism.

evankanderson · 2018-07-16T21:57:19Z

docs/spec/spec.md

@@ -78,6 +78,10 @@ status:
  #   along with a cluster-specific prefix (here, mydomain.com).
  domain: my-service.default.mydomain.com

+  # serviceName: The name for the core Kubernetes Service that fronts this
+  #   route. Typically, the name will be the same as the name of the route.
+  serviceName: my-service


Change this to domainInternal (or domainLocal), documented as:

A DNS name for the default (traffic-split) route which can be accessed without leaving the cluster environment.

evankanderson · 2018-07-16T21:59:27Z

docs/spec/spec.md

+  # serviceName: The name for the core Kubernetes Service that fronts this
+  #   revision. Typically, the name will be the same as the name of the
+  #   revision.
+  serviceName: myservice-a1e34


I don't think we want a serviceName/domainInternal on Revision -- a Revision may have zero traffic and be non-routable if it is not addressed by any Route (and clients should not assume that they will know when that transition may happen). If clients want a name for a specific Revision, they should use the traffic.name subrouting feature in Route.

ServiceName predates this PR on RevisionStatus, but was missing from the spec.md. I left the docs since the values should be documented.

That sounds fine, as long as we don't suggest this is a good way to reach the revision. Thanks for documenting!

evankanderson · 2018-07-16T22:00:26Z

pkg/apis/serving/v1alpha1/revision_types.go

+	// over the provided targets from inside the cluster. It generally has the
+	// form {revision-name}.{revision-namespace}.svc.cluster.local
+	// +optional
+	DomainInternal string `json:"domainInternal,omitempty"`


I don't think we should add a DomainInternal to Revision -- this feels like something which should be managed through the Route if needed.

evankanderson · 2018-07-16T22:00:49Z

pkg/apis/serving/v1alpha1/route_types.go

+	// fronts this Route, this service would be an appropriate ingress target
+	// for targeting the revision.
+	// +optional
+	ServiceName string `json:"serviceName,omitempty"`


Only DomainInternal here.

evankanderson · 2018-07-16T22:01:01Z

pkg/apis/serving/v1alpha1/service_types.go

+	// fronts this Route, this service would be an appropriate ingress target
+	// for targeting the revision.
+	// +optional
+	ServiceName string `json:"serviceName,omitempty"`


evankanderson · 2018-07-16T22:05:01Z

test/conformance/route_test.go

@@ -106,6 +106,14 @@ func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.Sugared
 	if err != nil {
 		t.Fatalf("The Route %s was not updated to route traffic to the Revision %s: %v", names.Route, names.Revision, err)
 	}
+	logger.Infof("The Route has a core Kubernetes Service referenced in it's status")
+	err = test.CheckRouteState(clients.Routes, names.Route, func(r *v1alpha1.Route) (bool, error) {
+		err := test.CheckCoreServiceState(clients.Kube.CoreV1().Services(r.Namespace), r.Status.ServiceName, test.IsCoreServiceCreated)


Switch this to check DomainInternal?

Should this test also attempt to actually reach the DomainInternal DNS name? My preference would be "yes", since we're adding this to verify that there will always be a name that will be within-cluster reachable for the Route.

scothis · 2018-07-16T22:41:52Z

@evankanderson @vaikas-google PTAL

pmorie · 2018-07-16T23:42:01Z

pkg/apis/serving/v1alpha1/route_types.go

@@ -131,6 +131,12 @@ type RouteStatus struct {
 	// +optional
 	Domain string `json:"domain,omitempty"`

+	// DomainInternal holds the top-level domain that will distribute traffic over the provided


The field name DomainInternal sounds very odd to me. How about InClusterDomain or DomainInCluster?

I would avoid 'internal' because it's not clear what internal is referring to

This is for concistency with Domain. We'll change these later but to minimize the name changes, wanted to keep them consistent for now. Ok with changing in a follow on PR?

I'm most definitely OK with it as a follow-on :)

I think we'll rename this and "domain" in v1alpha2. (Ville doesn't like it either, and I agree that it sounds a little weird given what it actually does). Up here in Seattle, we've bandied around dnsName' and localDnsName` as terms for these, since "internal" is unclear whether it's "internal to the implementation", "internal to the cluster", or "internal to the customer's deployed environment" (which may be != a single cluster).

Kubernetes 1.11 has beta support for CRD versioning, which will enable transitioning cleanly from v1alpha1 to v1alpha2.

evankanderson

I still dream of unified in-cluster and out-of-cluster naming, but this LGTM for now. @cooperneil may be interested in this API addition as well.

/lgtm
/approve

evankanderson · 2018-07-16T23:44:23Z

docs/spec/spec.md

+  # serviceName: The name for the core Kubernetes Service that fronts this
+  #   revision. Typically, the name will be the same as the name of the
+  #   revision.
+  serviceName: myservice-a1e34


That sounds fine, as long as we don't suggest this is a good way to reach the revision. Thanks for documenting!

evankanderson · 2018-07-16T23:44:52Z

docs/spec/spec.md

@@ -367,7 +376,11 @@ status:
  # domain: The hostname used to access the default (traffic-split)
  #   route. Typically, this will be composed of the name and namespace
  #   along with a cluster-specific prefix (here, mydomain.com).
-  domain: my-service.default.mydomain.com
+  domain: myservice.default.mydomain.com


Hah, thanks for fixing this name!

evankanderson · 2018-07-16T23:47:25Z

pkg/apis/serving/v1alpha1/route_types.go

@@ -131,6 +131,12 @@ type RouteStatus struct {
 	// +optional
 	Domain string `json:"domain,omitempty"`

+	// DomainInternal holds the top-level domain that will distribute traffic over the provided


I think we'll rename this and "domain" in v1alpha2. (Ville doesn't like it either, and I agree that it sounds a little weird given what it actually does). Up here in Seattle, we've bandied around dnsName' and localDnsName` as terms for these, since "internal" is unclear whether it's "internal to the implementation", "internal to the cluster", or "internal to the customer's deployed environment" (which may be != a single cluster).

Kubernetes 1.11 has beta support for CRD versioning, which will enable transitioning cleanly from v1alpha1 to v1alpha2.

google-prow-robot · 2018-07-16T23:48:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, mattmoor, scothis

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~~ [evankanderson,mattmoor]

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

evankanderson · 2018-07-16T23:50:51Z

What happened to /test/conformance/route_test.go ?

/lgtm cancel

scothis · 2018-07-16T23:57:49Z

@evankanderson the conformance tests were for the ServiceName property.

per #1586 (comment)

I skipped conformance tests for the new property since it's well covered by unit tests and we'd need to resolve DNS from inside the cluster. The best method to do that I could figure out is to deploy a pod into the cluster with nslookup or dig and then kubectl run against it to get the IP. I can take a pass at implementing that behavior if you think it's worthwhile.

knative-metrics-robot · 2018-07-17T00:48:34Z

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/apis/serving/v1alpha1/service_types.go	98.4%	98.4%	0.0
pkg/controller/route/cruds.go	92.6%	92.7%	0.1

evankanderson

/lgtm

google-prow-robot assigned vaikas Jul 12, 2018

google-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 12, 2018

google-prow-robot requested review from evankanderson and mattmoor July 12, 2018 21:38

mattmoor reviewed Jul 12, 2018

View reviewed changes

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

mattmoor approved these changes Jul 13, 2018

View reviewed changes

google-prow-robot assigned mattmoor Jul 13, 2018

google-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 13, 2018

google-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Jul 13, 2018

google-prow-robot assigned dprotaso Jul 13, 2018

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

mattmoor approved these changes Jul 13, 2018

View reviewed changes

vaikas reviewed Jul 13, 2018

View reviewed changes

evankanderson reviewed Jul 13, 2018

View reviewed changes

scothis added 3 commits July 15, 2018 19:53

Review feedback

ef5d583

- typos - added status property to spec.md

Add conformance tests

bc23435

Add DomainInternal to status for Service, Route and Revision

19e5aaf

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2018

evankanderson reviewed Jul 16, 2018

View reviewed changes

scothis added 4 commits July 16, 2018 18:14

Drop ServiceName from Service and Route

aa4ae46

Update resource spec

4fa9d06

Remove DomainInternal from Revision

3be8173

dead code

c37655b

scothis changed the title ~~Add ServiceName to status for resources that create services~~ Add DomainInternal to status for Route and Service Jul 16, 2018

pmorie reviewed Jul 16, 2018

View reviewed changes

evankanderson reviewed Jul 16, 2018

View reviewed changes

google-prow-robot assigned evankanderson Jul 16, 2018

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

vaikas removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2018

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2018

Update conformance test with TODOs

80d5f73

evankanderson reviewed Jul 17, 2018

View reviewed changes

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

google-prow-robot merged commit 2326173 into knative:master Jul 17, 2018

scothis deleted the incluster-domain-status branch July 17, 2018 01:44

Add DomainInternal to status for Route and Service #1586

Add DomainInternal to status for Route and Service #1586

Conversation

scothis commented Jul 12, 2018 • edited Loading

Proposed Changes

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

scothis commented Jul 13, 2018

dprotaso commented Jul 13, 2018

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor commented Jul 13, 2018 • edited Loading

dprotaso commented Jul 13, 2018

scothis commented Jul 13, 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

vaikas commented Jul 13, 2018

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scothis commented Jul 13, 2018

evankanderson commented Jul 13, 2018

scothis commented Jul 14, 2018

scothis commented Jul 16, 2018

scothis commented Jul 16, 2018

evankanderson commented Jul 16, 2018

evankanderson commented Jul 16, 2018

scothis commented Jul 16, 2018

evankanderson 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

scothis commented Jul 16, 2018

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

evankanderson 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

google-prow-robot commented Jul 16, 2018

evankanderson commented Jul 16, 2018

scothis commented Jul 16, 2018

knative-metrics-robot commented Jul 17, 2018

evankanderson left a comment

Choose a reason for hiding this comment

scothis commented Jul 12, 2018 •

edited

Loading

mattmoor commented Jul 13, 2018 •

edited

Loading