Skip to content
Closed
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
34 changes: 34 additions & 0 deletions pkg/route/api/allocator/allocator.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package allocator
Copy link

Choose a reason for hiding this comment

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

@smarterclayton is this something you think warrants an interface so it can have different implementations in the future (perhaps for different allocation strategies)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

On Mar 10, 2015, at 9:03 AM, Paul [email protected] wrote:

In pkg/route/api/allocator/allocator.go:

@@ -0,0 +1,30 @@
+package allocator
@smarterclayton is this something you think warrants an interface so it can have different implementations in the future (perhaps for different allocation strategies)?


Reply to this email directly or view it on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - will do. Though I suspect this may need some change when the sharding support is added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Essentially the interface should only expose the input (route) and the output (key to router), and then the interface would be used by the allocator (which won't be in the rest call). In fact, allocations like this being in the rest call are deceptive and should have a clear TODO: this does not belong here

On Mar 10, 2015, at 1:42 PM, ramr [email protected] wrote:

In pkg/route/api/allocator/allocator.go:

@@ -0,0 +1,30 @@
+package allocator
Ok - will do. Though I suspect this may need some change when the sharding support is added.


Reply to this email directly or view it on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - let me add a TODO for now. Its going to take me some time to get a handle on how to do that/how it works, so it might be better to do it in a separate PR.
That sound good @smarterclayton ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer it to be presented as an interface to consumers, even if you use the static defaultAllocator method.

On Mar 10, 2015, at 2:14 PM, ramr [email protected] wrote:

In pkg/route/api/allocator/allocator.go:

@@ -0,0 +1,30 @@
+package allocator
Ok - let me add a TODO for now. Its going to take me some time to get a handle on how to do that/how it works, so it might be safer to do it in a separate PR.
That sound good @smarterclayton ?


Reply to this email directly or view it on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allrighty, let me just do the whole shebang in this one then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still muddling through ... But I am going to close this PR out and create a new one.


import (
"fmt"

"code.google.com/p/go-uuid/uuid"
routeapi "github.com/openshift/origin/pkg/route/api"
)

// This should be something we get from config but we would still need a
// default name if nothing's passed. The first pass uses the default name.
// Would be better if we could use "v3.openshift.app", someone bought that!
const defaultDNSSuffix = "v3.openshift.com"

// Generate a host name for a route - using the service name,
// namespace (if provided) and the router shard dns suffix.
func Generate(route *routeapi.Route, shard *routeapi.RouterShard) string {
Copy link

Choose a reason for hiding this comment

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

godoc comments

name := route.ServiceName
if len(name) == 0 {
name = uuid.NewUUID().String()
}

if len(route.Namespace) <= 0 {
return fmt.Sprintf("%s.%s", name, shard.DNSSuffix)
}

return fmt.Sprintf("%s-%s.%s", name, route.Namespace, shard.DNSSuffix)
}

// Allocate a router shard for the given route.
// Placeholder for now ... returns the "global" router shard.
func Allocate(route *routeapi.Route) *routeapi.RouterShard {
return &routeapi.RouterShard{ShardName: "global", DNSSuffix: defaultDNSSuffix}
}
63 changes: 63 additions & 0 deletions pkg/route/api/allocator/allocator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package allocator

import (
"testing"

kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api"
"github.com/openshift/origin/pkg/route/api"
)

func TestAllocateRoute(t *testing.T) {
tests := []struct {
name string
route *api.Route
}{
{
name: "No Name",
route: &api.Route{
ObjectMeta: kapi.ObjectMeta{
Namespace: "namespace",
},
ServiceName: "service",
},
},
{
name: "No namespace",
route: &api.Route{
ObjectMeta: kapi.ObjectMeta{
Name: "name",
},
ServiceName: "nonamespace",
},
},
{
name: "No service name",
route: &api.Route{
ObjectMeta: kapi.ObjectMeta{
Name: "name",
Namespace: "foo",
},
},
},
{
name: "Valid route",
route: &api.Route{
ObjectMeta: kapi.ObjectMeta{
Name: "name",
Namespace: "foo",
},
Host: "www.example.com",
ServiceName: "serviceName",
},
},
}

for _, tc := range tests {
shard := Allocate(tc.route)
name := Generate(tc.route, shard)

if len(name) <= 0 {
t.Errorf("Test case %s got %d length name.", tc.name, len(name))
}
}
}
14 changes: 14 additions & 0 deletions pkg/route/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,20 @@ type RouteList struct {
Items []Route `json:"items"`
}

// RouterShard has information of a routing shard and is used to
// generate host names and routing table entries when a routing shard is
// allocated for a specific route.
// Caveat: This is WIP and will likely undergo modifications when sharding
// support is added.
type RouterShard struct {
// Shard name uniquely identifies a router shard in the "set" of
// routers used for routing traffic to the services.
ShardName string
Copy link

Choose a reason for hiding this comment

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

Needs comments


// The DNS suffix for the shard ala: shard-1.v3.openshift.com
DNSSuffix string
}

// TLSConfig defines config used to secure a route and provide termination
type TLSConfig struct {
// Termination indicates termination type. If termination type is not set, any termination config will be ignored
Expand Down
8 changes: 8 additions & 0 deletions pkg/route/registry/route/rest.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/GoogleCloudPlatform/kubernetes/pkg/watch"

"github.com/openshift/origin/pkg/route/api"
"github.com/openshift/origin/pkg/route/api/allocator"
"github.com/openshift/origin/pkg/route/api/validation"
)

Expand Down Expand Up @@ -71,6 +72,13 @@ func (rs *REST) Create(ctx kapi.Context, obj runtime.Object) (runtime.Object, er
return nil, errors.NewConflict("route", route.Namespace, fmt.Errorf("Route.Namespace does not match the provided context"))
}

// shards will be eventually allocated via a separate controller.
shard := allocator.Allocate(route)

if len(route.Host) == 0 {
Copy link

Choose a reason for hiding this comment

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

I think this should occur prior to ValidateRoute. That way we can ensure that whatever is generated passes the same validation as a user submitted name (util.IsDNSSubdomain).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - will move it up/prior to that check.

route.Host = allocator.Generate(route, shard)
}

if errs := validation.ValidateRoute(route); len(errs) > 0 {
return nil, errors.NewInvalid("route", route.Name, errs)
}
Expand Down