Skip to content

Commit

Permalink
feat!: Use / as the default path for new apps
Browse files Browse the repository at this point in the history
Currently, each Load Balanced Web App is created and is asigned
a path of "/{app name}" on the ALB. This makes sense, but there
are some gotchas.

First, your app has to be prepared to handle paths of "/{app name}".
This is probably a little unlikely unless your lucky. Most apps
expect to be run as if they are receiving requests on the root
path "/" (see every tutorial ever) 😂

What this change does is it defaults your first app to being hosted
on "/" but at the last evaluated rule. This is probably a more
intuitive default (and we've gotten some customer feedback on this).

Subsequent apps still have to use the path based route.

So:

```sh
ecs init --app front-end # produces a URL of http://my-lb.us-west-2.amazon.com/
ecs init --app api       # produces a URL of http://my-lb.us-west-2.amazon.com/api/
```

This change adjusts the priority generator so that "/" is evaluated last.

Customers are still free to change the path in their manifest. They can use
the special "/" path to signify that they want this to be their default route.

__ Why didn't you just use the default action ? __

One kind of odd thing we did here is give the "/" path a really high rule
priority, rather than just setting it as the default route. There are a few
reasons we did that.

1. It's a bit messy as the default route action is managed in the environment stack
   and there's really not a great way to update it.
2. If a customer changes their app to use a named path, rather than the default "/"
   path - it will only require a dpeloyment of the app, rather than then the app
   AND the env (which is a different stack).

This change also has some other E2E bug fixes in it (like setting the healthcheck
path by default).
  • Loading branch information
kohidave committed Mar 17, 2020
1 parent a9e961c commit 7ccb6ca
Show file tree
Hide file tree
Showing 22 changed files with 285 additions and 89 deletions.
8 changes: 6 additions & 2 deletions cf-custom-resources/lib/alb-rule-priority-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

const aws = require('aws-sdk');

// priorityForRootRule is the max priority number that's always set for the listener rule that matches the root path "/"
const priorityForRootRule = "50000"

// These are used for test purposes only
let defaultResponseURL;

Expand Down Expand Up @@ -88,8 +91,9 @@ const calculateNextRulePriority = async function (listenerArn) {
if (rules.length > 0) {
// Take the max rule priority, and add 1 to it.
const rulePriorities = rules.map(rule => {
if (rule.Priority === "default") {
// We treat the default rule as having priority 0
if (rule.Priority === "default" || rule.Priority === priorityForRootRule) {
// Ignore the root rule's priority since it has to be always the max value.
// Ignore the default rule's prority since it's the same as 0.
return 0
}
return parseInt(rule.Priority);
Expand Down
40 changes: 40 additions & 0 deletions cf-custom-resources/test/alb-rule-priority-generator-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,46 @@ describe('ALB Rule Priority Generator', () => {
});
});

test('Create operation returns rule priority 1 when the MAX rule is present', () => {

const describeRulesFake = sinon.fake.resolves(
{
"Rules": [
{
"Priority": "50000",
"Conditions": [],
"RuleArn": "arn:aws:elasticloadbalancing:us-west-2:000000000:listener-rule/app/rule",
"IsDefault": false,
"Actions": [
{
"TargetGroupArn": "arn:aws:elasticloadbalancing:us-west-2:000000000:targetgroup/tg",
"Type": "forward"
}
]
}
]
});

AWS.mock('ELBv2', 'describeRules', describeRulesFake);
const request = nock(ResponseURL).put('/', body => {
return body.Status === 'SUCCESS' && body.Data.Priority == 1;
}).reply(200);

return LambdaTester(albRulePriorityHandler.nextAvailableRulePriorityHandler)
.event({
RequestType: 'Create',
RequestId: testRequestId,
ResourceProperties: {
ListenerArn: testALBListenerArn
}
})
.expectResolve(() => {
sinon.assert.calledWith(describeRulesFake, sinon.match({
ListenerArn: testALBListenerArn,
}));
expect(request.isDone()).toBe(true);
});
});

test('Create operation returns rule priority max + 1', () => {
// This set of rules has the default, 3 and 5 rule priorities. We don't try to fill
Expand Down
8 changes: 3 additions & 5 deletions e2e/addons/addons_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,10 @@ var _ = Describe("addons flow", func() {
// Make a POST request to the API to store the user name in DynamoDB.
route := app.Routes[0]
Expect(route.Environment).To(Equal("test"))
Expect(route.URL).To(HaveSuffix(appName))
Eventually(func() (int, error) {
resp, fetchErr := http.Post(fmt.Sprintf("%s/%s", route.URL, projectName), "application/json", nil)
resp, fetchErr := http.Post(fmt.Sprintf("%s/%s/%s", route.URL, appName, projectName), "application/json", nil)
return resp.StatusCode, fetchErr
}, "10s", "1s").Should(Equal(201))
}, "30s", "1s").Should(Equal(201))
})

It("should be able to retrieve the results", func() {
Expand All @@ -175,11 +174,10 @@ var _ = Describe("addons flow", func() {
// Make a GET request to the API to retrieve the list of user names from DynamoDB.
route := app.Routes[0]
Expect(route.Environment).To(Equal("test"))
Expect(route.URL).To(HaveSuffix(appName))
var resp *http.Response
var fetchErr error
Eventually(func() (int, error) {
resp, fetchErr = http.Get(route.URL))
resp, fetchErr = http.Get(fmt.Sprintf("%s/hello", route.URL))
return resp.StatusCode, fetchErr
}, "10s", "1s").Should(Equal(200))

Expand Down
6 changes: 2 additions & 4 deletions e2e/init/front-end/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
FROM nginx

COPY nginx.conf /etc/nginx/nginx.conf

RUN mkdir -p /www/data/front-end
COPY index.html /www/data/front-end
RUN mkdir -p /www/data/
COPY index.html /www/data/
17 changes: 0 additions & 17 deletions e2e/init/front-end/nginx.conf

This file was deleted.

1 change: 0 additions & 1 deletion e2e/init/init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ var _ = Describe("init flow", func() {
It("should return a valid route", func() {
Expect(len(app.Routes)).To(Equal(1))
Expect(app.Routes[0].Environment).To(Equal("test"))
Expect(app.Routes[0].URL).To(HaveSuffix(appName))
Eventually(func() (int, error) {
resp, fetchErr := http.Get(app.Routes[0].URL)
return resp.StatusCode, fetchErr
Expand Down
2 changes: 1 addition & 1 deletion e2e/multi-app-project/front-end/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@ module github.com/aws/amazon-ecs-cli-v2/e2e/multi-app-project/front-end

go 1.13

require github.com/julienschmidt/httprouter v1.3.0 // indirect
require github.com/julienschmidt/httprouter v1.3.0
14 changes: 2 additions & 12 deletions e2e/multi-app-project/front-end/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,6 @@ import (
"github.com/julienschmidt/httprouter"
)

// HealthCheck just returns true if the service is up.
func HealthCheck(w http.ResponseWriter, req *http.Request, ps httprouter.Params) {
log.Println("🚑 healthcheck ok!")
w.WriteHeader(http.StatusOK)
}

// SimpleGet just returns true no matter what
func SimpleGet(w http.ResponseWriter, req *http.Request, ps httprouter.Params) {
log.Println("Get Succeeded")
Expand Down Expand Up @@ -45,11 +39,7 @@ func ServiceDiscoveryGet(w http.ResponseWriter, req *http.Request, ps httprouter

func main() {
router := httprouter.New()
router.GET("/front-end/", SimpleGet)
router.GET("/front-end/service-discovery-test", ServiceDiscoveryGet)

// Health Check
router.GET("/", HealthCheck)

router.GET("/", SimpleGet)
router.GET("/service-discovery-test", ServiceDiscoveryGet)
log.Fatal(http.ListenAndServe(":80", router))
}
22 changes: 16 additions & 6 deletions e2e/multi-app-project/multi_app_project_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,21 @@ var _ = Describe("Multiple App Project", func() {
// Call each environment's endpoint and ensure it returns a 200
route := app.Routes[0]
Expect(route.Environment).To(Equal("test"))
Expect(route.URL).To(HaveSuffix(appName))
resp, fetchErr := http.Get(route.URL)
Expect(fetchErr).NotTo(HaveOccurred())
Expect(resp.StatusCode).To(Equal(200))
// Since the front end was added first, it should have no suffix.
if appName == "front-end" {
Expect(route.URL).ToNot(HaveSuffix(appName))
}
// Since the back end was added second, it should have app appended
// to the name
if appName == "back-end" {
Expect(route.URL).To(HaveSuffix(appName))
}
var resp *http.Response
var fetchErr error
Eventually(func() (int, error) {
resp, fetchErr = http.Get(route.URL)
return resp.StatusCode, fetchErr
}, "10s", "1s").Should(Equal(200))

// Read the response - our deployed apps should return a body with their
// name as the value.
Expand Down Expand Up @@ -186,8 +197,7 @@ var _ = Describe("Multiple App Project", func() {
// to the backend, and pipe the backend response to us.
route := app.Routes[0]
Expect(route.Environment).To(Equal("test"))
Expect(route.URL).To(HaveSuffix(appName))
resp, fetchErr := http.Get(fmt.Sprintf("http://%s/service-discovery-test/", route.URL))
resp, fetchErr := http.Get(fmt.Sprintf("%s/service-discovery-test/", route.URL))
Expect(fetchErr).NotTo(HaveOccurred())
Expect(resp.StatusCode).To(Equal(200))

Expand Down
1 change: 0 additions & 1 deletion e2e/multi-env-project/multi_env_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ var _ = Describe("Multiple Env Project", func() {
for _, env := range []string{"test", "prod"} {
route := envRoutes[env]
Expect(route.Environment).To(Equal(env))
Expect(route.URL).To(HaveSuffix(appName))
Eventually(func() (int, error) {
resp, fetchErr := http.Get(route.URL)
return resp.StatusCode, fetchErr
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (
github.com/fatih/structs v1.1.0
github.com/gobuffalo/packd v1.0.0
github.com/gobuffalo/packr/v2 v2.8.0
github.com/golang/mock v1.4.2
github.com/golang/mock v1.4.3
github.com/golang/protobuf v1.3.2 // indirect
github.com/google/uuid v1.1.1
github.com/hinshun/vt10x v0.0.0-20180809195222-d55458df857c // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ github.com/golang/mock v1.4.1 h1:ocYkMQY5RrXTYgXl7ICpV0IXwlEQGwKIsery4gyXa1U=
github.com/golang/mock v1.4.1/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt3cw=
github.com/golang/mock v1.4.2 h1:fXIkPzOBCwDUPvYmOPzETABgbqpYlYNigQ2o64eMr5c=
github.com/golang/mock v1.4.2/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt3cw=
github.com/golang/mock v1.4.3 h1:GV+pQPG/EUUbkh47niozDcADz6go/dUwhVzdUQHIVRw=
github.com/golang/mock v1.4.3/go.mod h1:UOMv5ysSaYNkG+OFQykRIcU/QvvxJf3p21QfJ2Bt3cw=
github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.1 h1:YF8+flBXS5eO826T4nzqPrxfhQThhXl0YzfuUPu4SBg=
github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
Expand Down
35 changes: 27 additions & 8 deletions internal/pkg/cli/app_init.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,15 +186,10 @@ func (o *initAppOpts) Execute() error {
}

func (o *initAppOpts) createManifest() (string, error) {
props := &manifest.LBFargateManifestProps{
AppManifestProps: &manifest.AppManifestProps{
AppName: o.AppName,
Dockerfile: o.DockerfilePath,
},
Port: o.AppPort,
manifest, err := o.createLoadBalancedAppManifest()
if err != nil {
return "", err
}
props.Path = o.AppName
manifest := manifest.NewLoadBalancedFargateManifest(props)
manifestPath, err := o.ws.WriteAppManifest(manifest, o.AppName)
if err != nil {
return "", err
Expand All @@ -210,6 +205,30 @@ func (o *initAppOpts) createManifest() (string, error) {
return relPath, nil
}

func (o *initAppOpts) createLoadBalancedAppManifest() (*manifest.LBFargateManifest, error) {
props := &manifest.LBFargateManifestProps{
AppManifestProps: &manifest.AppManifestProps{
AppName: o.AppName,
Dockerfile: o.DockerfilePath,
},
Port: o.AppPort,
Path: "/",
}
existingApps, err := o.appStore.ListApplications(o.ProjectName())
if err != nil {
return nil, err
}
// We default to "/" for the first app, but if there's another
// load balanced web app, we use the app name as the default, instead.
for _, existingApp := range existingApps {
if existingApp.Type == manifest.LoadBalancedWebApplication && existingApp.Name != o.AppName {
props.Path = o.AppName
break
}
}
return manifest.NewLoadBalancedFargateManifest(props), nil
}

func (o *initAppOpts) createAppInProject(projectName string) error {
if err := o.appStore.CreateApplication(&archer.Application{
Project: projectName,
Expand Down

0 comments on commit 7ccb6ca

Please sign in to comment.