Skip to content

Commit

Permalink
allow "*" routes in manifest
Browse files Browse the repository at this point in the history 
- added some additional tests
- added additional logs for debugging

[Fixes #155060885] (Github Issue #399)

Signed-off-by: Anand Gaitonde <agaitonde@pivotal.io>
  • Loading branch information
@vitreuz @XenoPhex
vitreuz authored and XenoPhex committed Feb 10, 2018
1 parent f6d7ccc commit 70e297c
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 31 deletions.
2 changes: 1 addition & 1 deletion actor/pushaction/actor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ type Actor struct {
}

const ProtocolRegexp = "^https?://|^tcp://"
const URLRegexp = "^(?:https?://|tcp://)?(?:[\\w-]+\\.)+\\w+(?:\\:\\d+)?(?:/.*)*(?:\\.\\w+)?$"
const URLRegexp = "^(?:https?://|tcp://)?(?:(?:[\\w-]+\\.)|(?:[*]\\.))+\\w+(?:\\:\\d+)?(?:/.*)*(?:\\.\\w+)?$"

// NewActor returns a new actor.
func NewActor(v2Actor V2Actor, sharedActor SharedActor) *Actor {
Expand Down
9 changes: 8 additions & 1 deletion actor/pushaction/merge_and_validate_settings_and_manifest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ func (actor Actor) MergeAndValidateSettingsAndManifests(settings CommandLineSett
mergedApps = actor.setSaneEndpoint(mergedApps)

log.Debugf("merged app settings: %#v", mergedApps)
return mergedApps, actor.validateMergedSettings(mergedApps)

err = actor.validateMergedSettings(mergedApps)
if err != nil {
log.Errorln("validation error post merge:", err)
return nil, err
}
return mergedApps, nil
}

func (Actor) selectApp(appName string, apps []manifest.Application) ([]manifest.Application, error) {
Expand Down Expand Up @@ -150,6 +156,7 @@ func (Actor) validatePremergedSettings(settings CommandLineSettings, apps []mani

func (actor Actor) validateMergedSettings(apps []manifest.Application) error {
for i, app := range apps {
log.WithField("index", i).Info("validating app")
if app.Name == "" {
log.WithField("index", i).Error("does not contain an app name")
return actionerror.MissingNameError{}
Expand Down
62 changes: 48 additions & 14 deletions actor/pushaction/merge_and_validate_settings_and_manifest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ var _ = Describe("MergeAndValidateSettingsAndManifest", func() {
{Name: "some-name-2"},
}

DescribeTable("validation errors",
DescribeTable("valid manifest settings",
func(settings CommandLineSettings, apps []manifest.Application, expectedErr error) {
currentDirectory, err := os.Getwd()
Expect(err).ToNot(HaveOccurred())
Expand All @@ -265,11 +265,54 @@ var _ = Describe("MergeAndValidateSettingsAndManifest", func() {
}

_, err = actor.MergeAndValidateSettingsAndManifests(settings, apps)
if expectedErr == nil {
Expect(err).ToNot(HaveOccurred())
} else {
Expect(err).To(MatchError(expectedErr))
Expect(err).ToNot(HaveOccurred())
},

Entry("valid route with a port",
CommandLineSettings{},
[]manifest.Application{{
Name: "some-name-1",
Path: RealPath,
Routes: []string{"www.hardknox.cli.fun:1234"},
}},
nil),

Entry("valid route with crazy characters",
CommandLineSettings{},
[]manifest.Application{{
Name: "some-name-1",
Path: RealPath,
Routes: []string{"www.hardknox.cli.fun/foo_1+2.html"},
}},
nil),

Entry("ValidRoute with a star",
CommandLineSettings{},
[]manifest.Application{{
Name: "some-name-1",
Path: RealPath,
Routes: []string{"*.hardknox.cli.fun"},
}},
nil),
)

DescribeTable("validation errors",
func(settings CommandLineSettings, apps []manifest.Application, expectedErr error) {
currentDirectory, err := os.Getwd()
Expect(err).ToNot(HaveOccurred())

if settings.ProvidedAppPath == RealPath {
settings.ProvidedAppPath = currentDirectory
}

for i, app := range apps {
if app.Path == RealPath {
apps[i].Path = currentDirectory
}
}

_, err = actor.MergeAndValidateSettingsAndManifests(settings, apps)
Expect(err).To(MatchError(expectedErr))
},

Entry("CommandLineOptionsWithMultipleAppsError",
Expand Down Expand Up @@ -392,15 +435,6 @@ var _ = Describe("MergeAndValidateSettingsAndManifest", func() {
}},
actionerror.DockerPasswordNotSetError{}),

Entry("ValidRoute",
CommandLineSettings{},
[]manifest.Application{{
Name: "some-name-1",
Path: RealPath,
Routes: []string{"www.hardknox.cli.fun:1234/foo_1+2.html"},
}},
nil),

Entry("InvalidRoute",
CommandLineSettings{},
[]manifest.Application{{
Expand Down
48 changes: 43 additions & 5 deletions actor/pushaction/route_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ var _ = Describe("Routes", func() {
"c.b.a.com",
"d.c.b.a.com",
"a.com/some-path",
"*.f.e.com",
"*.e.com",
}
orgGUID = "some-org-guid"
spaceGUID = "some-space-guid"
Expand Down Expand Up @@ -237,6 +239,8 @@ var _ = Describe("Routes", func() {
fakeV2Actor.GetDomainsByNameAndOrganizationReturns([]v2action.Domain{
{GUID: "domain-guid-1", Name: "a.com"},
{GUID: "domain-guid-2", Name: "b.a.com"},
{GUID: "domain-guid-3", Name: "f.e.com"},
{GUID: "domain-guid-4", Name: "e.com"},
}, v2action.Warnings{"domain-warnings-1", "domains-warnings-2"}, nil)
})

Expand Down Expand Up @@ -267,7 +271,7 @@ var _ = Describe("Routes", func() {

It("returns new and existing routes", func() {
Expect(executeErr).NotTo(HaveOccurred())
Expect(warnings).To(ConsistOf("domain-warnings-1", "domains-warnings-2", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning"))
Expect(warnings).To(ConsistOf("domain-warnings-1", "domains-warnings-2", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning"))
Expect(calculatedRoutes).To(ConsistOf(
v2action.Route{
Domain: v2action.Domain{
Expand Down Expand Up @@ -317,14 +321,30 @@ var _ = Describe("Routes", func() {
},
Path: "/some-path",
SpaceGUID: spaceGUID,
},
v2action.Route{
Host: "*",
Domain: v2action.Domain{
GUID: "domain-guid-3",
Name: "f.e.com",
},
SpaceGUID: spaceGUID,
},
v2action.Route{
Host: "*",
Domain: v2action.Domain{
GUID: "domain-guid-4",
Name: "e.com",
},
SpaceGUID: spaceGUID,
}))

Expect(fakeV2Actor.GetDomainsByNameAndOrganizationCallCount()).To(Equal(1))
domains, passedOrgGUID := fakeV2Actor.GetDomainsByNameAndOrganizationArgsForCall(0)
Expect(domains).To(ConsistOf("a.com", "b.a.com", "c.b.a.com", "d.c.b.a.com"))
Expect(domains).To(ConsistOf("a.com", "b.a.com", "c.b.a.com", "d.c.b.a.com", "*.f.e.com", "f.e.com", "*.e.com", "e.com"))
Expect(passedOrgGUID).To(Equal(orgGUID))

Expect(fakeV2Actor.FindRouteBoundToSpaceWithSettingsCallCount()).To(Equal(5))
Expect(fakeV2Actor.FindRouteBoundToSpaceWithSettingsCallCount()).To(Equal(7))
// One check is enough here - checking 4th call since it's the only
// existing one.
Expect(fakeV2Actor.FindRouteBoundToSpaceWithSettingsArgsForCall(3)).To(Equal(v2action.Route{
Expand Down Expand Up @@ -394,13 +414,15 @@ var _ = Describe("Routes", func() {
fakeV2Actor.GetDomainsByNameAndOrganizationReturns([]v2action.Domain{
{GUID: "domain-guid-1", Name: "a.com"},
{GUID: "domain-guid-2", Name: "b.a.com"},
{GUID: "domain-guid-3", Name: "f.e.com"},
{GUID: "domain-guid-4", Name: "e.com"},
}, v2action.Warnings{"domain-warnings-1", "domains-warnings-2"}, nil)
fakeV2Actor.FindRouteBoundToSpaceWithSettingsReturns(v2action.Route{}, v2action.Warnings{"find-route-warning"}, actionerror.RouteNotFoundError{})
})

It("does not lookup known routes", func() {
Expect(executeErr).NotTo(HaveOccurred())
Expect(warnings).To(ConsistOf("domain-warnings-1", "domains-warnings-2", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning"))
Expect(warnings).To(ConsistOf("domain-warnings-1", "domains-warnings-2", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning", "find-route-warning"))
Expect(calculatedRoutes).To(ConsistOf(
v2action.Route{
Domain: v2action.Domain{
Expand Down Expand Up @@ -441,11 +463,27 @@ var _ = Describe("Routes", func() {
},
Path: "/some-path",
SpaceGUID: spaceGUID,
},
v2action.Route{
Host: "*",
Domain: v2action.Domain{
GUID: "domain-guid-3",
Name: "f.e.com",
},
SpaceGUID: spaceGUID,
},
v2action.Route{
Host: "*",
Domain: v2action.Domain{
GUID: "domain-guid-4",
Name: "e.com",
},
SpaceGUID: spaceGUID,
}))

Expect(fakeV2Actor.GetDomainsByNameAndOrganizationCallCount()).To(Equal(1))
domains, passedOrgGUID := fakeV2Actor.GetDomainsByNameAndOrganizationArgsForCall(0)
Expect(domains).To(ConsistOf("a.com", "b.a.com", "c.b.a.com"))
Expect(domains).To(ConsistOf("a.com", "b.a.com", "c.b.a.com", "*.f.e.com", "f.e.com", "*.e.com", "e.com"))
Expect(passedOrgGUID).To(Equal(orgGUID))
})
})
Expand Down
25 changes: 15 additions & 10 deletions integration/push/http_routes_in_manifest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package push

import (
"path/filepath"
"regexp"

"code.cloudfoundry.org/cli/integration/helpers"

Expand All @@ -19,6 +20,7 @@ var _ = Describe("HTTP routes in manifest", func() {
route1 helpers.Route
route2 helpers.Route
routeWithPath helpers.Route
routeStar helpers.Route
)

BeforeEach(func() {
Expand All @@ -28,6 +30,7 @@ var _ = Describe("HTTP routes in manifest", func() {
route1 = helpers.NewRoute(space, domain.Name, helpers.PrefixedRandomName("r1"), "")
route2 = helpers.NewRoute(space, subdomain.Name, helpers.PrefixedRandomName("r2"), "")
routeWithPath = helpers.NewRoute(space, domain.Name, helpers.PrefixedRandomName("r3"), helpers.PrefixedRandomName("p1"))
routeStar = helpers.NewRoute(space, subdomain.Name, "*", "")
})

Context("when the domain exist", func() {
Expand All @@ -47,17 +50,19 @@ var _ = Describe("HTTP routes in manifest", func() {
"routes": []map[string]string{
{"route": route1.String()},
{"route": route2.String()},
{"route": routeStar.String()},
},
},
},
})

session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))

Eventually(session).Should(Say("Creating app with these attributes\\.\\.\\."))
Eventually(session).Should(Say("\\+\\s+name:\\s+%s", app))
Eventually(session).Should(Say("\\s+routes:"))
Eventually(session).Should(Say("(?i)\\+\\s+%s", regexp.QuoteMeta(routeStar.String())))
Eventually(session).Should(Say("(?i)\\+\\s+%s", route1))
Eventually(session).Should(Say("(?i)\\+\\s+%s", route2))
Eventually(session).Should(Exit(0))
Expand Down Expand Up @@ -85,7 +90,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
})

session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))

Eventually(session).Should(Say("Creating app with these attributes\\.\\.\\."))
Expand Down Expand Up @@ -124,7 +129,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
})

session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))

Eventually(session).Should(Say("Creating app with these attributes\\.\\.\\."))
Expand Down Expand Up @@ -164,7 +169,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
})

session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))
Eventually(session.Err).Should(Say("The app cannot be mapped to route %s because the route is not in this space. Apps must be mapped to routes in the same space.", route2))
Eventually(session).Should(Exit(1))
Expand Down Expand Up @@ -192,7 +197,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
})

session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))
Eventually(session.Err).Should(Say("Port not allowed in HTTP domain %s", domain.Name))
Eventually(session).Should(Exit(1))
Expand All @@ -216,7 +221,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
},
})
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))

Eventually(session).Should(Say("Creating app with these attributes\\.\\.\\."))
Expand Down Expand Up @@ -255,7 +260,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
},
})
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))

Eventually(session).Should(Say("Creating app with these attributes\\.\\.\\."))
Expand Down Expand Up @@ -292,7 +297,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
})

session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))

Eventually(session).Should(Say("Creating app with these attributes\\.\\.\\."))
Expand Down Expand Up @@ -330,7 +335,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
})

session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))
Eventually(session.Err).Should(Say("The app cannot be mapped to route %s because the route is not in this space. Apps must be mapped to routes in the same space.", routeWithPath))
Eventually(session).Should(Exit(1))
Expand All @@ -356,7 +361,7 @@ var _ = Describe("HTTP routes in manifest", func() {
},
})

session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName)
session := helpers.CustomCF(helpers.CFEnv{WorkingDirectory: dir}, PushCommandName, "--no-start")
Eventually(session).Should(Say("Getting app info\\.\\.\\."))
Eventually(session.Err).Should(Say("The route %s did not match any existing domains.", route1))
Eventually(session).Should(Exit(1))
Expand Down

0 comments on commit 70e297c

Please sign in to comment.