From 765e6ce8a8f475bf7c994b4ef342de510a1b7519 Mon Sep 17 00:00:00 2001 From: "Eric J. Holmes" Date: Thu, 17 Dec 2015 09:50:38 +0700 Subject: [PATCH 1/5] Add tests for existing behavior. --- scheduler/ecs/lb.go | 2 +- scheduler/ecs/lb_test.go | 203 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 scheduler/ecs/lb_test.go diff --git a/scheduler/ecs/lb.go b/scheduler/ecs/lb.go index 69d50fffd..9e77a527a 100644 --- a/scheduler/ecs/lb.go +++ b/scheduler/ecs/lb.go @@ -121,7 +121,7 @@ func (e *LoadBalancerExposureError) Error() string { lbExposure = "public" } - return fmt.Sprintf("Process %s is %s, but load balancer is %s.", e.proc.Type, e.proc.Exposure, lbExposure) + return fmt.Sprintf("Process %s is %s, but load balancer is %s. An update would require me to delete the load balancer.", e.proc.Type, e.proc.Exposure, lbExposure) } // LoadBalancerPortMismatchError is returned when the port stored in the data store does not match the ELB instance port diff --git a/scheduler/ecs/lb_test.go b/scheduler/ecs/lb_test.go new file mode 100644 index 000000000..2695150d0 --- /dev/null +++ b/scheduler/ecs/lb_test.go @@ -0,0 +1,203 @@ +package ecs + +import ( + "testing" + + "github.com/remind101/empire/pkg/lb" + "github.com/remind101/empire/scheduler" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "golang.org/x/net/context" +) + +func TestLBProcessManager_CreateProcess_NoExposure(t *testing.T) { + p := new(mockProcessManager) + l := new(mockLBManager) + m := &LBProcessManager{ + ProcessManager: p, + lb: l, + } + + app := &scheduler.App{} + process := &scheduler.Process{} + + p.On("CreateProcess", app, process).Return(nil) + + err := m.CreateProcess(context.Background(), app, process) + assert.NoError(t, err) + + p.AssertExpectations(t) + l.AssertExpectations(t) +} + +func TestLBProcessManager_CreateProcess_NoExistingLoadBalancer(t *testing.T) { + p := new(mockProcessManager) + l := new(mockLBManager) + m := &LBProcessManager{ + ProcessManager: p, + lb: l, + } + + port := int64(8080) + app := &scheduler.App{ + ID: "appid", + Name: "appname", + } + process := &scheduler.Process{ + Type: "web", + Exposure: scheduler.ExposePrivate, + Ports: []scheduler.PortMap{ + {Host: &port}, + }, + } + + l.On("LoadBalancers", map[string]string{"AppID": "appid", "ProcessType": "web"}).Return([]*lb.LoadBalancer{}, nil) + l.On("CreateLoadBalancer", lb.CreateLoadBalancerOpts{ + InstancePort: 8080, + Tags: map[string]string{"AppID": "appid", "ProcessType": "web", "App": "appname"}, + External: false, + }).Return(&lb.LoadBalancer{ + Name: "lbname", + }, nil) + p.On("CreateProcess", app, &scheduler.Process{ + Type: "web", + Exposure: scheduler.ExposePrivate, + Ports: []scheduler.PortMap{ + {Host: &port}, + }, + LoadBalancer: "lbname", + }).Return(nil) + + err := m.CreateProcess(context.Background(), app, process) + assert.NoError(t, err) + + p.AssertExpectations(t) + l.AssertExpectations(t) +} + +func TestLBProcessManager_CreateProcess_ExistingLoadBalancer(t *testing.T) { + p := new(mockProcessManager) + l := new(mockLBManager) + m := &LBProcessManager{ + ProcessManager: p, + lb: l, + } + + port := int64(8080) + app := &scheduler.App{ + ID: "appid", + Name: "appname", + } + process := &scheduler.Process{ + Type: "web", + Exposure: scheduler.ExposePublic, + Ports: []scheduler.PortMap{ + {Host: &port}, + }, + } + + l.On("LoadBalancers", map[string]string{"AppID": "appid", "ProcessType": "web"}).Return([]*lb.LoadBalancer{ + {Name: "lbname", InstancePort: 8080, External: true}, + }, nil) + p.On("CreateProcess", app, process).Return(nil) + + err := m.CreateProcess(context.Background(), app, process) + assert.NoError(t, err) + + p.AssertExpectations(t) + l.AssertExpectations(t) +} + +func TestLBProcessManager_CreateProcess_ExistingLoadBalancer_MismatchedExposure(t *testing.T) { + p := new(mockProcessManager) + l := new(mockLBManager) + m := &LBProcessManager{ + ProcessManager: p, + lb: l, + } + + port := int64(8080) + app := &scheduler.App{ + ID: "appid", + Name: "appname", + } + process := &scheduler.Process{ + Type: "web", + Exposure: scheduler.ExposePublic, + Ports: []scheduler.PortMap{ + {Host: &port}, + }, + } + + l.On("LoadBalancers", map[string]string{"AppID": "appid", "ProcessType": "web"}).Return([]*lb.LoadBalancer{ + {Name: "lbname", External: false}, + }, nil) + + err := m.CreateProcess(context.Background(), app, process) + assert.EqualError(t, err, "Process web is public, but load balancer is private. An update would require me to delete the load balancer.") + + p.AssertExpectations(t) + l.AssertExpectations(t) +} + +func TestLBProcessManager_CreateProcess_ExistingLoadBalancer_NewCert(t *testing.T) { + p := new(mockProcessManager) + l := new(mockLBManager) + m := &LBProcessManager{ + ProcessManager: p, + lb: l, + } + + port := int64(8080) + app := &scheduler.App{ + ID: "appid", + Name: "appname", + } + process := &scheduler.Process{ + Type: "web", + Exposure: scheduler.ExposePublic, + Ports: []scheduler.PortMap{ + {Host: &port}, + }, + SSLCert: "newcert", + } + + l.On("LoadBalancers", map[string]string{"AppID": "appid", "ProcessType": "web"}).Return([]*lb.LoadBalancer{ + {Name: "lbname", External: true, SSLCert: "oldcert"}, + }, nil) + + err := m.CreateProcess(context.Background(), app, process) + assert.NoError(t, err) + + p.AssertExpectations(t) + l.AssertExpectations(t) +} + +type mockProcessManager struct { + ProcessManager + mock.Mock +} + +func (m *mockProcessManager) CreateProcess(ctx context.Context, app *scheduler.App, process *scheduler.Process) error { + args := m.Called(app, process) + return args.Error(0) +} + +type mockLBManager struct { + mock.Mock +} + +func (m *mockLBManager) CreateLoadBalancer(ctx context.Context, opts lb.CreateLoadBalancerOpts) (*lb.LoadBalancer, error) { + args := m.Called(opts) + return args.Get(0).(*lb.LoadBalancer), args.Error(1) +} + +func (m *mockLBManager) DestroyLoadBalancer(ctx context.Context, lb *lb.LoadBalancer) error { + args := m.Called(lb) + return args.Error(0) +} + +func (m *mockLBManager) LoadBalancers(ctx context.Context, tags map[string]string) ([]*lb.LoadBalancer, error) { + args := m.Called(tags) + return args.Get(0).([]*lb.LoadBalancer), args.Error(1) +} From dc68184ff16d4abbd9fb23fc187f432453c9a025 Mon Sep 17 00:00:00 2001 From: "Eric J. Holmes" Date: Thu, 17 Dec 2015 10:15:16 +0700 Subject: [PATCH 2/5] Update elb if SSL cert or port changed. --- pkg/lb/elb.go | 4 +++ pkg/lb/lb.go | 16 ++++++++++++ scheduler/ecs/lb.go | 53 +++++++++++++++++++--------------------- scheduler/ecs/lb_test.go | 51 +++++++++++++++++++++++++++++++++++++- 4 files changed, 95 insertions(+), 29 deletions(-) diff --git a/pkg/lb/elb.go b/pkg/lb/elb.go index 59016c21c..050bd64c1 100644 --- a/pkg/lb/elb.go +++ b/pkg/lb/elb.go @@ -102,6 +102,10 @@ func (m *ELBManager) CreateLoadBalancer(ctx context.Context, o CreateLoadBalance }, nil } +func (m *ELBManager) UpdateLoadBalancer(ctx context.Context, opts UpdateLoadBalancerOpts) error { + return nil +} + // DestroyLoadBalancer destroys an ELB. func (m *ELBManager) DestroyLoadBalancer(ctx context.Context, lb *LoadBalancer) error { _, err := m.elb.DeleteLoadBalancer(&elb.DeleteLoadBalancerInput{ diff --git a/pkg/lb/lb.go b/pkg/lb/lb.go index dd562a1d3..a5fd80849 100644 --- a/pkg/lb/lb.go +++ b/pkg/lb/lb.go @@ -21,6 +21,19 @@ type CreateLoadBalancerOpts struct { SSLCert string } +// UpdateLoadBalancerOpts are options that can be provided when updating an +// existing load balancer. +type UpdateLoadBalancerOpts struct { + // The name of the load balancer. + Name string + + // The SSL Certificate + SSLCert *string + + // The port to route requests to on the hosts. + InstancePort *int64 +} + // LoadBalancer represents a load balancer. type LoadBalancer struct { // The name of the load balancer. @@ -49,6 +62,9 @@ type Manager interface { // CreateLoadBalancer creates a new LoadBalancer with the given options. CreateLoadBalancer(context.Context, CreateLoadBalancerOpts) (*LoadBalancer, error) + // UpdateLoadBalancer updates an existing load balancer. + UpdateLoadBalancer(context.Context, UpdateLoadBalancerOpts) error + // DestroyLoadBalancer destroys a load balancer by name. DestroyLoadBalancer(ctx context.Context, lb *LoadBalancer) error diff --git a/scheduler/ecs/lb.go b/scheduler/ecs/lb.go index 9e77a527a..af5b47194 100644 --- a/scheduler/ecs/lb.go +++ b/scheduler/ecs/lb.go @@ -35,9 +35,16 @@ func (m *LBProcessManager) CreateProcess(ctx context.Context, app *scheduler.App // want, we'll return an error. Users should manually destroy // the app and re-create it with the proper exposure. if l != nil { - if err = lbOk(p, l); err != nil { + if err = canUpdate(p, l); err != nil { return err } + + if opts := updateOpts(p, l); opts != nil { + opts.Name = l.Name + if err = m.lb.UpdateLoadBalancer(ctx, *opts); err != nil { + return err + } + } } // If this app doesn't have a load balancer yet, create one. @@ -124,28 +131,8 @@ func (e *LoadBalancerExposureError) Error() string { return fmt.Sprintf("Process %s is %s, but load balancer is %s. An update would require me to delete the load balancer.", e.proc.Type, e.proc.Exposure, lbExposure) } -// LoadBalancerPortMismatchError is returned when the port stored in the data store does not match the ELB instance port -type LoadBalancerPortMismatchError struct { - proc *scheduler.Process - lb *lb.LoadBalancer -} - -func (e *LoadBalancerPortMismatchError) Error() string { - return fmt.Sprintf("Process %s instance port is %d, but load balancer instance port is %d.", e.proc.Type, e.proc.Ports[0].Host, e.lb.InstancePort) -} - -// SslCertMismatchError is returned when the ssl cert in the data store does not match the ssl cert on the ELB -type SslCertMismatchError struct { - proc *scheduler.Process - lb *lb.LoadBalancer -} - -func (e *SslCertMismatchError) Error() string { - return fmt.Sprintf("Process ssl certificate (%s) does not match load balancer ssl certificate (%s).", e.proc.SSLCert, e.lb.SSLCert) -} - -// lbOk checks if the load balancer is suitable for the process. -func lbOk(p *scheduler.Process, lb *lb.LoadBalancer) error { +// canUpdate checks if the load balancer is suitable for the process. +func canUpdate(p *scheduler.Process, lb *lb.LoadBalancer) error { if p.Exposure == scheduler.ExposePublic && !lb.External { return &LoadBalancerExposureError{p, lb} } @@ -154,13 +141,23 @@ func lbOk(p *scheduler.Process, lb *lb.LoadBalancer) error { return &LoadBalancerExposureError{p, lb} } - if *p.Ports[0].Host != lb.InstancePort { - return &LoadBalancerPortMismatchError{p, lb} + return nil +} + +func updateOpts(p *scheduler.Process, b *lb.LoadBalancer) *lb.UpdateLoadBalancerOpts { + var opts lb.UpdateLoadBalancerOpts + + if p.SSLCert != b.SSLCert { + opts.SSLCert = &p.SSLCert + } + + if *p.Ports[0].Host != b.InstancePort { + opts.InstancePort = p.Ports[0].Host } - if p.SSLCert != lb.SSLCert { - return &SslCertMismatchError{p, lb} + if opts.SSLCert == nil && opts.InstancePort == nil { + return nil } - return nil + return &opts } diff --git a/scheduler/ecs/lb_test.go b/scheduler/ecs/lb_test.go index 2695150d0..16e1a4bea 100644 --- a/scheduler/ecs/lb_test.go +++ b/scheduler/ecs/lb_test.go @@ -163,8 +163,52 @@ func TestLBProcessManager_CreateProcess_ExistingLoadBalancer_NewCert(t *testing. } l.On("LoadBalancers", map[string]string{"AppID": "appid", "ProcessType": "web"}).Return([]*lb.LoadBalancer{ - {Name: "lbname", External: true, SSLCert: "oldcert"}, + {Name: "lbname", External: true, SSLCert: "oldcert", InstancePort: port}, }, nil) + newcert := "newcert" + l.On("UpdateLoadBalancer", lb.UpdateLoadBalancerOpts{ + Name: "lbname", + SSLCert: &newcert, + }).Return(nil) + p.On("CreateProcess", app, process).Return(nil) + + err := m.CreateProcess(context.Background(), app, process) + assert.NoError(t, err) + + p.AssertExpectations(t) + l.AssertExpectations(t) +} + +func TestLBProcessManager_CreateProcess_ExistingLoadBalancer_NewPort(t *testing.T) { + p := new(mockProcessManager) + l := new(mockLBManager) + m := &LBProcessManager{ + ProcessManager: p, + lb: l, + } + + oldport := int64(8080) + newport := int64(8081) + app := &scheduler.App{ + ID: "appid", + Name: "appname", + } + process := &scheduler.Process{ + Type: "web", + Exposure: scheduler.ExposePublic, + Ports: []scheduler.PortMap{ + {Host: &newport}, + }, + } + + l.On("LoadBalancers", map[string]string{"AppID": "appid", "ProcessType": "web"}).Return([]*lb.LoadBalancer{ + {Name: "lbname", External: true, InstancePort: oldport}, + }, nil) + l.On("UpdateLoadBalancer", lb.UpdateLoadBalancerOpts{ + Name: "lbname", + InstancePort: &newport, + }).Return(nil) + p.On("CreateProcess", app, process).Return(nil) err := m.CreateProcess(context.Background(), app, process) assert.NoError(t, err) @@ -192,6 +236,11 @@ func (m *mockLBManager) CreateLoadBalancer(ctx context.Context, opts lb.CreateLo return args.Get(0).(*lb.LoadBalancer), args.Error(1) } +func (m *mockLBManager) UpdateLoadBalancer(ctx context.Context, opts lb.UpdateLoadBalancerOpts) error { + args := m.Called(opts) + return args.Error(0) +} + func (m *mockLBManager) DestroyLoadBalancer(ctx context.Context, lb *lb.LoadBalancer) error { args := m.Called(lb) return args.Error(0) From 72b62be94f33db0dac5723a345921c8088cbe4f4 Mon Sep 17 00:00:00 2001 From: "Eric J. Holmes" Date: Thu, 17 Dec 2015 10:23:02 +0700 Subject: [PATCH 3/5] Refactor. --- scheduler/ecs/lb.go | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/scheduler/ecs/lb.go b/scheduler/ecs/lb.go index af5b47194..b1984ed54 100644 --- a/scheduler/ecs/lb.go +++ b/scheduler/ecs/lb.go @@ -35,12 +35,13 @@ func (m *LBProcessManager) CreateProcess(ctx context.Context, app *scheduler.App // want, we'll return an error. Users should manually destroy // the app and re-create it with the proper exposure. if l != nil { - if err = canUpdate(p, l); err != nil { + var opts *lb.UpdateLoadBalancerOpts + opts, err = updateOpts(p, l) + if err != nil { return err } - if opts := updateOpts(p, l); opts != nil { - opts.Name = l.Name + if opts != nil { if err = m.lb.UpdateLoadBalancer(ctx, *opts); err != nil { return err } @@ -144,20 +145,31 @@ func canUpdate(p *scheduler.Process, lb *lb.LoadBalancer) error { return nil } -func updateOpts(p *scheduler.Process, b *lb.LoadBalancer) *lb.UpdateLoadBalancerOpts { - var opts lb.UpdateLoadBalancerOpts +func updateOpts(p *scheduler.Process, b *lb.LoadBalancer) (*lb.UpdateLoadBalancerOpts, error) { + // This load balancer can't be updated to make it work for the process. + // Return an error. + if err := canUpdate(p, b); err != nil { + return nil, err + } + + opts := lb.UpdateLoadBalancerOpts{ + Name: b.Name, + } + // Requires an update to the Cert. if p.SSLCert != b.SSLCert { opts.SSLCert = &p.SSLCert } + // Requires an update to the InstancePort. if *p.Ports[0].Host != b.InstancePort { opts.InstancePort = p.Ports[0].Host } + // Load balancer doesn't require an update. if opts.SSLCert == nil && opts.InstancePort == nil { - return nil + return nil, nil } - return &opts + return &opts, nil } From c00e5698d08df94e4b571e41a48095ed0288206a Mon Sep 17 00:00:00 2001 From: "Eric J. Holmes" Date: Thu, 17 Dec 2015 10:28:02 +0700 Subject: [PATCH 4/5] Start with only updates to SSL Cert. --- pkg/lb/lb.go | 3 --- scheduler/ecs/lb.go | 21 +++++++++++++++------ scheduler/ecs/lb_test.go | 7 +------ 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/pkg/lb/lb.go b/pkg/lb/lb.go index a5fd80849..8dca27e18 100644 --- a/pkg/lb/lb.go +++ b/pkg/lb/lb.go @@ -29,9 +29,6 @@ type UpdateLoadBalancerOpts struct { // The SSL Certificate SSLCert *string - - // The port to route requests to on the hosts. - InstancePort *int64 } // LoadBalancer represents a load balancer. diff --git a/scheduler/ecs/lb.go b/scheduler/ecs/lb.go index b1984ed54..6ccd6db23 100644 --- a/scheduler/ecs/lb.go +++ b/scheduler/ecs/lb.go @@ -132,6 +132,16 @@ func (e *LoadBalancerExposureError) Error() string { return fmt.Sprintf("Process %s is %s, but load balancer is %s. An update would require me to delete the load balancer.", e.proc.Type, e.proc.Exposure, lbExposure) } +// LoadBalancerPortMismatchError is returned when the port stored in the data store does not match the ELB instance port +type LoadBalancerPortMismatchError struct { + proc *scheduler.Process + lb *lb.LoadBalancer +} + +func (e *LoadBalancerPortMismatchError) Error() string { + return fmt.Sprintf("Process %s instance port is %d, but load balancer instance port is %d.", e.proc.Type, *e.proc.Ports[0].Host, e.lb.InstancePort) +} + // canUpdate checks if the load balancer is suitable for the process. func canUpdate(p *scheduler.Process, lb *lb.LoadBalancer) error { if p.Exposure == scheduler.ExposePublic && !lb.External { @@ -142,6 +152,10 @@ func canUpdate(p *scheduler.Process, lb *lb.LoadBalancer) error { return &LoadBalancerExposureError{p, lb} } + if *p.Ports[0].Host != lb.InstancePort { + return &LoadBalancerPortMismatchError{p, lb} + } + return nil } @@ -161,13 +175,8 @@ func updateOpts(p *scheduler.Process, b *lb.LoadBalancer) (*lb.UpdateLoadBalance opts.SSLCert = &p.SSLCert } - // Requires an update to the InstancePort. - if *p.Ports[0].Host != b.InstancePort { - opts.InstancePort = p.Ports[0].Host - } - // Load balancer doesn't require an update. - if opts.SSLCert == nil && opts.InstancePort == nil { + if opts.SSLCert == nil { return nil, nil } diff --git a/scheduler/ecs/lb_test.go b/scheduler/ecs/lb_test.go index 16e1a4bea..c20dbe67a 100644 --- a/scheduler/ecs/lb_test.go +++ b/scheduler/ecs/lb_test.go @@ -204,14 +204,9 @@ func TestLBProcessManager_CreateProcess_ExistingLoadBalancer_NewPort(t *testing. l.On("LoadBalancers", map[string]string{"AppID": "appid", "ProcessType": "web"}).Return([]*lb.LoadBalancer{ {Name: "lbname", External: true, InstancePort: oldport}, }, nil) - l.On("UpdateLoadBalancer", lb.UpdateLoadBalancerOpts{ - Name: "lbname", - InstancePort: &newport, - }).Return(nil) - p.On("CreateProcess", app, process).Return(nil) err := m.CreateProcess(context.Background(), app, process) - assert.NoError(t, err) + assert.EqualError(t, err, "Process web instance port is 8081, but load balancer instance port is 8080.") p.AssertExpectations(t) l.AssertExpectations(t) From 8612c9aaabc559f45e41a7f4a5a75464791046f9 Mon Sep 17 00:00:00 2001 From: "Eric J. Holmes" Date: Thu, 17 Dec 2015 10:55:49 +0700 Subject: [PATCH 5/5] Implement UpdateLoadBalancer. --- pkg/lb/elb.go | 15 +++++++++++++++ pkg/lb/elb_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/pkg/lb/elb.go b/pkg/lb/elb.go index 050bd64c1..a457974ae 100644 --- a/pkg/lb/elb.go +++ b/pkg/lb/elb.go @@ -103,9 +103,24 @@ func (m *ELBManager) CreateLoadBalancer(ctx context.Context, o CreateLoadBalance } func (m *ELBManager) UpdateLoadBalancer(ctx context.Context, opts UpdateLoadBalancerOpts) error { + if opts.SSLCert != nil { + if err := m.updateSSLCert(ctx, opts.Name, *opts.SSLCert); err != nil { + return err + } + } + return nil } +func (m *ELBManager) updateSSLCert(ctx context.Context, name, certID string) error { + _, err := m.elb.SetLoadBalancerListenerSSLCertificate(&elb.SetLoadBalancerListenerSSLCertificateInput{ + LoadBalancerName: aws.String(name), + LoadBalancerPort: aws.Int64(443), + SSLCertificateId: aws.String(certID), + }) + return err +} + // DestroyLoadBalancer destroys an ELB. func (m *ELBManager) DestroyLoadBalancer(ctx context.Context, lb *LoadBalancer) error { _, err := m.elb.DeleteLoadBalancer(&elb.DeleteLoadBalancerInput{ diff --git a/pkg/lb/elb_test.go b/pkg/lb/elb_test.go index 0c7dcf8b2..9ea92ba9a 100644 --- a/pkg/lb/elb_test.go +++ b/pkg/lb/elb_test.go @@ -64,6 +64,33 @@ func TestELB_CreateLoadBalancer(t *testing.T) { } } +func TestELB_UpdateLoadBalancer(t *testing.T) { + h := awsutil.NewHandler([]awsutil.Cycle{ + { + Request: awsutil.Request{ + RequestURI: "/", + Body: `Action=SetLoadBalancerListenerSSLCertificate&LoadBalancerName=loadbalancer&LoadBalancerPort=443&SSLCertificateId=newcert&Version=2012-06-01`, + }, + Response: awsutil.Response{ + StatusCode: 200, + Body: ` + +`, + }, + }, + }) + m, s := newTestELBManager(h) + defer s.Close() + + err := m.UpdateLoadBalancer(context.Background(), UpdateLoadBalancerOpts{ + Name: "loadbalancer", + SSLCert: aws.String("newcert"), + }) + if err != nil { + t.Fatal(err) + } +} + func buildLoadBalancerForDestroy() (*ELBManager, *httptest.Server, *LoadBalancer) { h := awsutil.NewHandler([]awsutil.Cycle{ {