Skip to content

Commit

Permalink
Route cluster-local visibility should take precedence.
Browse files Browse the repository at this point in the history 
When Route has cluster-local visibility, all sub-Route should use that
and ignore settings on placeholder Services.
  • Loading branch information
Nghia Tran committed Sep 6, 2019
1 parent d2bf8fd commit 9f0de90
Show file tree
Hide file tree
Showing 4 changed files with 166 additions and 14 deletions.
5 changes: 0 additions & 5 deletions pkg/reconciler/route/resources/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
netv1alpha1 "knative.dev/serving/pkg/apis/networking/v1alpha1"
"knative.dev/serving/pkg/apis/serving"
"knative.dev/serving/pkg/apis/serving/v1alpha1"
"knative.dev/serving/pkg/reconciler/route/config"
"knative.dev/serving/pkg/reconciler/route/domains"
)

Expand Down Expand Up @@ -109,10 +108,6 @@ func makeK8sService(ctx context.Context, route *v1alpha1.Route, targetName strin
serving.RouteLabelKey: route.Name,
}

if visibility, ok := route.Labels[config.VisibilityLabelKey]; ok {
svcLabels[config.VisibilityLabelKey] = visibility
}

return &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: hostname,
Expand Down
3 changes: 1 addition & 2 deletions pkg/reconciler/route/resources/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,7 @@ func TestMakeK8sPlaceholderService(t *testing.T) {
SessionAffinity: corev1.ServiceAffinityNone,
},
expectedLabels: map[string]string{
serving.RouteLabelKey: "test-route",
config.VisibilityLabelKey: config.VisibilityClusterLocal,
serving.RouteLabelKey: "test-route",
},
}}
for _, tt := range tests {
Expand Down
19 changes: 12 additions & 7 deletions pkg/reconciler/route/route.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package route
import (
"context"
"encoding/json"
"fmt"

"go.uber.org/zap"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -191,6 +192,7 @@ func (c *Reconciler) reconcile(ctx context.Context, r *v1alpha1.Route) error {
logger.Infof("Reconciling route: %#v", r)

serviceNames, err := c.getServiceNames(ctx, r)
fmt.Printf("serviceNames = %#v\n", serviceNames)
if err != nil {
return err
}
Expand Down Expand Up @@ -485,17 +487,20 @@ func (c *Reconciler) getServiceNames(ctx context.Context, route *v1alpha1.Route)
if err != nil {
return nil, err
}
if labels.IsObjectLocalVisibility(route.ObjectMeta) {
return &serviceNames{
existingPublicServiceNames: existingPublicServiceNames,
existingClusterLocalServiceNames: existingClusterLocalServiceNames,
desiredPublicServiceNames: sets.NewString(),
desiredClusterLocalServiceNames: desiredServiceNames,
}, nil
}
desiredPublicServiceNames := desiredServiceNames.Intersection(existingPublicServiceNames)
desiredClusterLocalServiceNames := desiredServiceNames.Intersection(existingClusterLocalServiceNames)

// Any new desired services will follow the default route visibility. We only need to consider the case where it is
// "cluster-local". The alternative visibility means it should not be cluster local.
// Any new desired services will follow the default route visibility, which is public.
serviceWithDefaultVisibility := desiredServiceNames.Difference(existingServiceNames)
if labels.IsObjectLocalVisibility(route.ObjectMeta) {
desiredClusterLocalServiceNames = desiredClusterLocalServiceNames.Union(serviceWithDefaultVisibility)
} else {
desiredPublicServiceNames = desiredPublicServiceNames.Union(serviceWithDefaultVisibility)
}
desiredPublicServiceNames = desiredPublicServiceNames.Union(serviceWithDefaultVisibility)

return &serviceNames{
existingPublicServiceNames: existingPublicServiceNames,
Expand Down
153 changes: 153 additions & 0 deletions pkg/reconciler/route/table_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,159 @@ func TestReconcile(t *testing.T) {
}},
Key: "default/new-latest-ready",
SkipNamespaceValidation: true,
}, {
Name: "public becomes cluster local",
Objects: []runtime.Object{
route("default", "becomes-local", WithConfigTarget("config"),
WithRouteLabel("serving.knative.dev/visibility", "cluster-local"),
WithRouteUID("65-23")),
cfg("default", "config",
WithGeneration(1), WithLatestCreated("config-00001"), WithLatestReady("config-00001")),
rev("default", "config", 1, MarkRevisionReady, WithRevName("config-00001"), WithServiceName("tb")),
simpleIngress(
route("default", "becomes-local", WithConfigTarget("config"), WithRouteUID("65-23")),
&traffic.Config{
Targets: map[string]traffic.RevisionTargets{
traffic.DefaultTarget: {{
TrafficTarget: v1beta1.TrafficTarget{
// Use the Revision name from the config.
RevisionName: "config-00001",
Percent: ptr.Int64(100),
},
ServiceName: "tb",
Active: true,
}},
},
},
),
simpleK8sService(route("default", "becomes-local", WithConfigTarget("config"),
WithRouteLabel("serving.knative.dev/visibility", "cluster-local"),
WithRouteUID("65-23"))),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: simpleIngressWithVisibility(
route("default", "becomes-local", WithConfigTarget("config"),
WithRouteUID("65-23"),
WithRouteLabel("serving.knative.dev/visibility", "cluster-local")),
&traffic.Config{
Targets: map[string]traffic.RevisionTargets{
traffic.DefaultTarget: {{
TrafficTarget: v1beta1.TrafficTarget{
// Use the Revision name from the config.
RevisionName: "config-00001",
Percent: ptr.Int64(100),
},
ServiceName: "tb",
Active: true,
}},
},
},
sets.NewString("becomes-local"),
),
}},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers("default", "becomes-local"),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: route("default", "becomes-local", WithConfigTarget("config"),
WithRouteUID("65-23"),
MarkTrafficAssigned, MarkIngressNotConfigured,
WithLocalDomain, WithAddress, WithInitRouteConditions,
WithRouteLabel("serving.knative.dev/visibility", "cluster-local"),
WithStatusTraffic(v1alpha1.TrafficTarget{
TrafficTarget: v1beta1.TrafficTarget{
RevisionName: "config-00001",
Percent: ptr.Int64(100),
LatestRevision: ptr.Bool(true),
},
})),
}},
WantDeleteCollections: []clientgotesting.DeleteCollectionActionImpl{{
ListRestrictions: clientgotesting.ListRestrictions{
Labels: labels.Set(map[string]string{
serving.RouteLabelKey: "becomes-local",
serving.RouteNamespaceLabelKey: "default",
}).AsSelector(),
Fields: fields.Nothing(),
},
}},
Key: "default/becomes-local",
SkipNamespaceValidation: true,
}, {
Name: "cluster local becomes public",
Objects: []runtime.Object{
route("default", "becomes-public", WithConfigTarget("config"),
WithRouteUID("65-23")),
cfg("default", "config",
WithGeneration(1), WithLatestCreated("config-00001"), WithLatestReady("config-00001")),
rev("default", "config", 1, MarkRevisionReady, WithRevName("config-00001"), WithServiceName("tb")),
simpleIngressWithVisibility(
route("default", "becomes-public", WithConfigTarget("config"), WithRouteUID("65-23"),
WithRouteLabel("serving.knative.dev/visibility", "cluster-local")),
&traffic.Config{
Targets: map[string]traffic.RevisionTargets{
traffic.DefaultTarget: {{
TrafficTarget: v1beta1.TrafficTarget{
// Use the Revision name from the config.
RevisionName: "config-00001",
Percent: ptr.Int64(100),
},
ServiceName: "tb",
Active: true,
}},
},
},
sets.NewString("becomes-public"),
),
simpleK8sService(route("default", "becomes-public", WithConfigTarget("config"),
WithRouteUID("65-23"))),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: simpleIngress(
route("default", "becomes-public", WithConfigTarget("config"),
WithRouteUID("65-23")),
&traffic.Config{
Targets: map[string]traffic.RevisionTargets{
traffic.DefaultTarget: {{
TrafficTarget: v1beta1.TrafficTarget{
// Use the Revision name from the config.
RevisionName: "config-00001",
Percent: ptr.Int64(100),
},
ServiceName: "tb",
Active: true,
}},
},
},
),
}},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers("default", "becomes-public"),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: route("default", "becomes-public", WithConfigTarget("config"),
WithRouteUID("65-23"),
MarkTrafficAssigned, MarkIngressNotConfigured,
WithAddress, WithInitRouteConditions, WithURL,
WithStatusTraffic(v1alpha1.TrafficTarget{
TrafficTarget: v1beta1.TrafficTarget{
RevisionName: "config-00001",
Percent: ptr.Int64(100),
LatestRevision: ptr.Bool(true),
},
})),
}},
WantDeleteCollections: []clientgotesting.DeleteCollectionActionImpl{{
ListRestrictions: clientgotesting.ListRestrictions{
Labels: labels.Set(map[string]string{
serving.RouteLabelKey: "becomes-public",
serving.RouteNamespaceLabelKey: "default",
}).AsSelector(),
Fields: fields.Nothing(),
},
}},
Key: "default/becomes-public",
SkipNamespaceValidation: true,
}, {
Name: "failure updating cluster ingress",
// Starting from the new latest ready, induce a failure updating the cluster ingress.
Expand Down

0 comments on commit 9f0de90

Please sign in to comment.