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

Add DomainInternal to status for Route and Service #1586

Merged
merged 9 commits into from
Jul 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion docs/spec/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ status:
# along with a cluster-specific prefix (here, mydomain.com).
domain: my-service.default.mydomain.com

# domainInternal: A DNS name for the default (traffic-split) route which can
# be accessed without leaving the cluster environment.
domainInternal: my-service.default.svc.cluster.local

traffic:
# current rollout status list. configurationName references
# are dereferenced to latest revision
Expand Down Expand Up @@ -286,6 +290,11 @@ status:
# Note that logs may still be access controlled separately from
# access to the API object.
logUrl: "http://logging.infra.mycompany.com/...?filter=revision_uid=a1e34&..."

# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

```


Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, thanks for fixing this name!


# domainInternal: A DNS name for the default (traffic-split) route which can
# be accessed without leaving the cluster environment.
domainInternal: myservice.default.svc.cluster.local

# current rollout status list. configurationName references
# are dereferenced to latest revision
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/serving/v1alpha1/route_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

// Traffic holds the configured traffic distribution.
// These entries will always contain RevisionName references.
// When ConfigurationName appears in the spec, this will hold the
Expand Down
8 changes: 8 additions & 0 deletions pkg/apis/serving/v1alpha1/service_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,13 @@ type ServiceStatus struct {
// +optional
Domain string `json:"domain,omitempty"`

// From RouteStatus.
// DomainInternal holds the top-level domain that will distribute traffic over the provided
// targets from inside the cluster. It generally has the form
// {route-name}.{route-namespace}.svc.cluster.local
// +optional
DomainInternal string `json:"domainInternal,omitempty"`

// From RouteStatus.
// Traffic holds the configured traffic distribution.
// These entries will always contain RevisionName references.
Expand Down Expand Up @@ -274,6 +281,7 @@ func (ss *ServiceStatus) PropagateConfigurationStatus(cs ConfigurationStatus) {

func (ss *ServiceStatus) PropagateRouteStatus(rs RouteStatus) {
ss.Domain = rs.Domain
ss.DomainInternal = rs.DomainInternal
ss.Traffic = rs.Traffic

rc := rs.GetCondition(RouteConditionReady)
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/route/cruds.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ func (c *Controller) reconcilePlaceholderService(ctx context.Context, route *v1a
return err
}
logger.Infof("Created service %s", name)
route.Status.DomainInternal = resourcenames.K8sServiceFullname(route)
c.Recorder.Eventf(route, corev1.EventTypeNormal, "Created", "Created service %q", name)
} else if err != nil {
return err
Expand Down
84 changes: 56 additions & 28 deletions pkg/controller/route/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "first-reconcile", "not-ready", &v1alpha1.RouteStatus{
Domain: "first-reconcile.default.example.com",
Domain: "first-reconcile.default.example.com",
DomainInternal: "first-reconcile.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionUnknown,
Expand Down Expand Up @@ -101,7 +102,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "first-reconcile", "permanently-failed", &v1alpha1.RouteStatus{
Domain: "first-reconcile.default.example.com",
Domain: "first-reconcile.default.example.com",
DomainInternal: "first-reconcile.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionFalse,
Expand Down Expand Up @@ -142,7 +144,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "first-reconcile", "not-ready", &v1alpha1.RouteStatus{
Domain: "first-reconcile.default.example.com",
Domain: "first-reconcile.default.example.com",
DomainInternal: "first-reconcile.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionUnknown,
Expand Down Expand Up @@ -193,7 +196,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "becomes-ready", "config", &v1alpha1.RouteStatus{
Domain: "becomes-ready.default.example.com",
Domain: "becomes-ready.default.example.com",
DomainInternal: "becomes-ready.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -235,7 +239,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "label-config-failure", "config", &v1alpha1.RouteStatus{
Domain: "label-config-failure.default.example.com",
Domain: "label-config-failure.default.example.com",
DomainInternal: "label-config-failure.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionUnknown,
Expand Down Expand Up @@ -318,7 +323,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "vs-create-failure", "config", &v1alpha1.RouteStatus{
Domain: "vs-create-failure.default.example.com",
Domain: "vs-create-failure.default.example.com",
DomainInternal: "vs-create-failure.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionUnknown,
Expand All @@ -333,7 +339,8 @@ func TestReconcile(t *testing.T) {
Name: "steady state",
Objects: []runtime.Object{
simpleRunLatest("default", "steady-state", "config", &v1alpha1.RouteStatus{
Domain: "steady-state.default.example.com",
Domain: "steady-state.default.example.com",
DomainInternal: "steady-state.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -379,7 +386,8 @@ func TestReconcile(t *testing.T) {
Objects: []runtime.Object{
addRouteLabel(
simpleRunLatest("default", "different-domain", "config", &v1alpha1.RouteStatus{
Domain: "different-domain.default.another-example.com",
Domain: "different-domain.default.another-example.com",
DomainInternal: "different-domain.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -426,7 +434,8 @@ func TestReconcile(t *testing.T) {
Name: "new latest created revision",
Objects: []runtime.Object{
simpleRunLatest("default", "new-latest-created", "config", &v1alpha1.RouteStatus{
Domain: "new-latest-created.default.example.com",
Domain: "new-latest-created.default.example.com",
DomainInternal: "new-latest-created.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -477,7 +486,8 @@ func TestReconcile(t *testing.T) {
Name: "new latest ready revision",
Objects: []runtime.Object{
simpleRunLatest("default", "new-latest-ready", "config", &v1alpha1.RouteStatus{
Domain: "new-latest-ready.default.example.com",
Domain: "new-latest-ready.default.example.com",
DomainInternal: "new-latest-ready.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -541,7 +551,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "new-latest-ready", "config", &v1alpha1.RouteStatus{
Domain: "new-latest-ready.default.example.com",
Domain: "new-latest-ready.default.example.com",
DomainInternal: "new-latest-ready.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand All @@ -566,7 +577,8 @@ func TestReconcile(t *testing.T) {
},
Objects: []runtime.Object{
simpleRunLatest("default", "update-vs-failure", "config", &v1alpha1.RouteStatus{
Domain: "update-vs-failure.default.example.com",
Domain: "update-vs-failure.default.example.com",
DomainInternal: "update-vs-failure.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -634,7 +646,8 @@ func TestReconcile(t *testing.T) {
Name: "reconcile service mutation",
Objects: []runtime.Object{
simpleRunLatest("default", "svc-mutation", "config", &v1alpha1.RouteStatus{
Domain: "svc-mutation.default.example.com",
Domain: "svc-mutation.default.example.com",
DomainInternal: "svc-mutation.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -687,7 +700,8 @@ func TestReconcile(t *testing.T) {
},
Objects: []runtime.Object{
simpleRunLatest("default", "svc-mutation", "config", &v1alpha1.RouteStatus{
Domain: "svc-mutation.default.example.com",
Domain: "svc-mutation.default.example.com",
DomainInternal: "svc-mutation.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -735,7 +749,8 @@ func TestReconcile(t *testing.T) {
Name: "allow cluster ip",
Objects: []runtime.Object{
simpleRunLatest("default", "cluster-ip", "config", &v1alpha1.RouteStatus{
Domain: "cluster-ip.default.example.com",
Domain: "cluster-ip.default.example.com",
DomainInternal: "cluster-ip.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -780,7 +795,8 @@ func TestReconcile(t *testing.T) {
Name: "reconcile virtual service mutation",
Objects: []runtime.Object{
simpleRunLatest("default", "virt-svc-mutation", "config", &v1alpha1.RouteStatus{
Domain: "virt-svc-mutation.default.example.com",
Domain: "virt-svc-mutation.default.example.com",
DomainInternal: "virt-svc-mutation.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -842,7 +858,8 @@ func TestReconcile(t *testing.T) {
Name: "config labelled by another route",
Objects: []runtime.Object{
simpleRunLatest("default", "licked-cookie", "config", &v1alpha1.RouteStatus{
Domain: "licked-cookie.default.example.com",
Domain: "licked-cookie.default.example.com",
DomainInternal: "licked-cookie.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -889,7 +906,8 @@ func TestReconcile(t *testing.T) {
Objects: []runtime.Object{
// The status reflects "oldconfig", but the spec "newconfig".
simpleRunLatest("default", "change-configs", "newconfig", &v1alpha1.RouteStatus{
Domain: "change-configs.default.example.com",
Domain: "change-configs.default.example.com",
DomainInternal: "change-configs.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -965,7 +983,8 @@ func TestReconcile(t *testing.T) {
}, {
// Status updated to "newconfig"
Object: simpleRunLatest("default", "change-configs", "newconfig", &v1alpha1.RouteStatus{
Domain: "change-configs.default.example.com",
Domain: "change-configs.default.example.com",
DomainInternal: "change-configs.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand All @@ -991,7 +1010,8 @@ func TestReconcile(t *testing.T) {
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: simpleRunLatest("default", "config-missing", "not-found", &v1alpha1.RouteStatus{
Domain: "config-missing.default.example.com",
Domain: "config-missing.default.example.com",
DomainInternal: "config-missing.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionFalse,
Expand All @@ -1018,7 +1038,8 @@ func TestReconcile(t *testing.T) {
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: simplePinned("default", "missing-revision-direct", "not-found", &v1alpha1.RouteStatus{
Domain: "missing-revision-direct.default.example.com",
Domain: "missing-revision-direct.default.example.com",
DomainInternal: "missing-revision-direct.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionFalse,
Expand Down Expand Up @@ -1051,7 +1072,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "missing-revision-indirect", "config", &v1alpha1.RouteStatus{
Domain: "missing-revision-indirect.default.example.com",
Domain: "missing-revision-indirect.default.example.com",
DomainInternal: "missing-revision-indirect.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionFalse,
Expand Down Expand Up @@ -1112,7 +1134,8 @@ func TestReconcile(t *testing.T) {
Object: simplePinned("default", "pinned-becomes-ready",
// Use the config's revision name.
simpleReadyConfig("default", "config").Status.LatestReadyRevisionName, &v1alpha1.RouteStatus{
Domain: "pinned-becomes-ready.default.example.com",
Domain: "pinned-becomes-ready.default.example.com",
DomainInternal: "pinned-becomes-ready.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -1210,7 +1233,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: routeWithTraffic("default", "named-traffic-split", &v1alpha1.RouteStatus{
Domain: "named-traffic-split.default.example.com",
Domain: "named-traffic-split.default.example.com",
DomainInternal: "named-traffic-split.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -1241,7 +1265,8 @@ func TestReconcile(t *testing.T) {
// Start from a steady state referencing "blue", and modify the route spec to point to "green" instead.
Objects: []runtime.Object{
simpleRunLatest("default", "switch-configs", "green", &v1alpha1.RouteStatus{
Domain: "switch-configs.default.example.com",
Domain: "switch-configs.default.example.com",
DomainInternal: "switch-configs.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -1312,7 +1337,8 @@ func TestReconcile(t *testing.T) {
),
}, {
Object: simpleRunLatest("default", "switch-configs", "green", &v1alpha1.RouteStatus{
Domain: "switch-configs.default.example.com",
Domain: "switch-configs.default.example.com",
DomainInternal: "switch-configs.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand All @@ -1338,7 +1364,8 @@ func TestReconcile(t *testing.T) {
},
Objects: []runtime.Object{
simpleRunLatest("default", "rmlabel-config-failure", "green", &v1alpha1.RouteStatus{
Domain: "rmlabel-config-failure.default.example.com",
Domain: "rmlabel-config-failure.default.example.com",
DomainInternal: "rmlabel-config-failure.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down Expand Up @@ -1397,7 +1424,8 @@ func TestReconcile(t *testing.T) {
},
Objects: []runtime.Object{
simpleRunLatest("default", "addlabel-config-failure", "green", &v1alpha1.RouteStatus{
Domain: "addlabel-config-failure.default.example.com",
Domain: "addlabel-config-failure.default.example.com",
DomainInternal: "addlabel-config-failure.default.svc.cluster.local",
Conditions: []v1alpha1.RouteCondition{{
Type: v1alpha1.RouteConditionAllTrafficAssigned,
Status: corev1.ConditionTrue,
Expand Down
5 changes: 5 additions & 0 deletions test/conformance/route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ 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("TODO: The Route is accessible from inside the cluster without external DNS")
err = test.CheckRouteState(clients.Routes, names.Route, test.TODO_RouteTrafficToRevisionWithInClusterDNS)
if err != nil {
t.Fatalf("The Route %s was not able to route traffic to the Revision %s with in cluster DNS: %v", names.Route, names.Revision, err)
}
}

func getNextRevisionName(clients *test.Clients, names test.ResourceNames) (string, error) {
Expand Down
6 changes: 6 additions & 0 deletions test/conformance/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,12 @@ func assertServiceResourcesUpdated(t *testing.T, logger *zap.SugaredLogger, clie
t.Fatalf("The Route %s was not updated to route traffic to the Revision %s: %v", names.Route, names.Revision, err)
}

logger.Infof("TODO: The Service's Route is accessible from inside the cluster without external DNS")
err = test.CheckServiceState(clients.Services, names.Service, test.TODO_ServiceTrafficToRevisionWithInClusterDNS)
if err != nil {
t.Fatalf("The Service %s was not able to route traffic to the Revision %s with in cluster DNS: %v", names.Service, names.Revision, err)
}

// TODO(#1381): Check labels and annotations.
}

Expand Down
Loading