diff --git a/state/compat_test.go b/state/compat_test.go index 33c4c2a0f2a..f04e66f1a4b 100644 --- a/state/compat_test.go +++ b/state/compat_test.go @@ -162,9 +162,9 @@ func (s *compatSuite) TestMigratePortsOnOpen(c *gc.C) { err = unit.Refresh() c.Assert(err, gc.IsNil) - // Check if port conflicts are detected. + // Port conflicts should be ignored, OpenPort should not return an error here. err = unit.OpenPort("tcp", 80) - c.Assert(err, gc.ErrorMatches, "cannot open ports 80-80/tcp for unit \"mysql/0\": cannot open ports 80-80/tcp on machine 0 due to conflict") + c.Assert(err, gc.IsNil) err = unit.OpenPort("tcp", 8080) c.Assert(err, gc.IsNil) diff --git a/state/ports.go b/state/ports.go index a3649faa585..515ea65c072 100644 --- a/state/ports.go +++ b/state/ports.go @@ -42,7 +42,7 @@ type PortRange struct { Protocol string } -// NewPortRange create a new port range. +// NewPortRange create a new port range and validate it. func NewPortRange(unitName string, fromPort, toPort int, protocol string) (PortRange, error) { p := PortRange{ UnitName: unitName, @@ -71,21 +71,28 @@ func (p PortRange) Validate() error { return nil } -// ConflictsWith determines if the two port ranges conflict. -func (a PortRange) ConflictsWith(b PortRange) bool { - if a.Protocol != b.Protocol { - return false +// CheckConflicts determines if the two port ranges conflict. +func (a PortRange) CheckConflicts(b PortRange) error { + if err := a.Validate(); err != nil { + return err } - switch { - case a.FromPort >= b.FromPort && a.FromPort <= b.ToPort: - return true - case a.ToPort >= b.FromPort && a.ToPort <= b.ToPort: - return true - case a.FromPort <= b.FromPort && a.ToPort >= b.ToPort: - return true + if err := b.Validate(); err != nil { + return err } - return false + // An exact port range match (including the associated unit name) is not + // considered a conflict due to the fact that many charms issue commands + // to open the same port multiple times. + if a == b { + return nil + } + if a.Protocol != b.Protocol { + return nil + } + if a.ToPort >= b.FromPort && b.ToPort >= a.FromPort { + return fmt.Errorf("port ranges %v (%s) and %v (%s) conflict", a, a.UnitName, b, b.UnitName) + } + return nil } func (p PortRange) String() string { @@ -112,14 +119,15 @@ func (p *Ports) Id() string { return p.doc.Id } -// Check if a port range can be opened. -func (p *Ports) canOpenPorts(newPorts PortRange) bool { +// Check if a port range can be opened, return the conflict error +// for more accurate reporting. +func (p *Ports) canOpenPorts(newPorts PortRange) error { for _, existingPorts := range p.doc.Ports { - if existingPorts.ConflictsWith(newPorts) { - return false + if err := existingPorts.CheckConflicts(newPorts); err != nil { + return err } } - return true + return nil } func (p *Ports) extractPortIdPart(part portIdPart) (string, error) { @@ -168,8 +176,8 @@ func (p *Ports) OpenPorts(portRange PortRange) error { } } - if !ports.canOpenPorts(portRange) { - return nil, fmt.Errorf("cannot open ports %v on machine %v due to conflict", portRange, machineId) + if err := ports.canOpenPorts(portRange); err != nil { + return nil, errors.Annotatef(err, "cannot open ports %v on machine %q", portRange, machineId) } // a new ports document being created @@ -222,8 +230,10 @@ func (p *Ports) ClosePorts(portRange PortRange) error { if existingPortsDef == portRange { found = true continue - } else if existingPortsDef.UnitName == portRange.UnitName && existingPortsDef.ConflictsWith(portRange) { - return nil, fmt.Errorf("mismatched port ranges %v and %v", existingPortsDef, portRange) + } + portConflictErr := existingPortsDef.CheckConflicts(portRange) + if existingPortsDef.UnitName == portRange.UnitName && portConflictErr != nil { + return nil, errors.Annotatef(portConflictErr, "mismatched port ranges") } newPorts = append(newPorts, existingPortsDef) } @@ -274,8 +284,8 @@ func (p *Ports) migratePorts(u *Unit) error { if err != nil { return nil, fmt.Errorf("cannot migrate port %v: %v", port, err) } - if !ports.canOpenPorts(portDef) { - return nil, fmt.Errorf("cannot migrate port %v due to conflict", port) + if err := ports.canOpenPorts(portDef); err != nil { + return nil, fmt.Errorf("cannot migrate port %v: %v", port, err) } migratedPorts[i] = portDef } diff --git a/state/ports_test.go b/state/ports_test.go index d26b87ede9e..101f64425c1 100644 --- a/state/ports_test.go +++ b/state/ports_test.go @@ -4,16 +4,20 @@ package state_test import ( + "strings" + gc "launchpad.net/gocheck" "github.com/juju/juju/state" + "github.com/juju/juju/testing/factory" ) type PortsDocSuite struct { ConnSuite charm *state.Charm service *state.Service - unit *state.Unit + unit1 *state.Unit + unit2 *state.Unit machine *state.Machine ports *state.Ports } @@ -22,17 +26,15 @@ var _ = gc.Suite(&PortsDocSuite{}) func (s *PortsDocSuite) SetUpTest(c *gc.C) { s.ConnSuite.SetUpTest(c) - s.charm = s.AddTestingCharm(c, "wordpress") - var err error - s.service = s.AddTestingService(c, "wordpress", s.charm) - c.Assert(err, gc.IsNil) - s.unit, err = s.service.AddUnit() - c.Assert(err, gc.IsNil) - s.machine, err = s.State.AddMachine("quantal", state.JobHostUnits) - c.Assert(err, gc.IsNil) - err = s.unit.AssignToMachine(s.machine) - c.Assert(err, gc.IsNil) + f := factory.NewFactory(s.State) + s.charm = f.MakeCharm(c, &factory.CharmParams{Name: "wordpress"}) + s.service = f.MakeService(c, &factory.ServiceParams{Name: "wordpress", Charm: s.charm}) + s.machine = f.MakeMachine(c, &factory.MachineParams{Series: "quantal"}) + s.unit1 = f.MakeUnit(c, &factory.UnitParams{Service: s.service, Machine: s.machine}) + s.unit2 = f.MakeUnit(c, &factory.UnitParams{Service: s.service, Machine: s.machine}) + + var err error s.ports, err = state.GetOrCreatePorts(s.State, s.machine.Id()) c.Assert(err, gc.IsNil) c.Assert(s.ports, gc.NotNil) @@ -45,7 +47,7 @@ func (s *PortsDocSuite) TestCreatePorts(c *gc.C) { err = ports.OpenPorts(state.PortRange{ FromPort: 100, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }) c.Assert(err, gc.IsNil) @@ -54,7 +56,7 @@ func (s *PortsDocSuite) TestCreatePorts(c *gc.C) { c.Assert(err, gc.IsNil) c.Assert(ports, gc.NotNil) - c.Assert(ports.PortsForUnit(s.unit.Name()), gc.HasLen, 1) + c.Assert(ports.PortsForUnit(s.unit1.Name()), gc.HasLen, 1) } func (s *PortsDocSuite) TestOpenAndClosePorts(c *gc.C) { @@ -71,13 +73,13 @@ func (s *PortsDocSuite) TestOpenAndClosePorts(c *gc.C) { open: &state.PortRange{ FromPort: 100, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }, close: &state.PortRange{ FromPort: 100, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }, expected: "", @@ -86,30 +88,30 @@ func (s *PortsDocSuite) TestOpenAndClosePorts(c *gc.C) { existing: []state.PortRange{{ FromPort: 100, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }}, open: nil, close: &state.PortRange{ FromPort: 100, ToPort: 150, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }, - expected: "mismatched port ranges 100-200/tcp and 100-150/tcp", + expected: "mismatched port ranges: port ranges 100-200/tcp \\(wordpress/0\\) and 100-150/tcp \\(wordpress/0\\) conflict", }, { about: "close an unopened port range with existing clash from other unit", existing: []state.PortRange{{ FromPort: 100, ToPort: 150, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }}, open: nil, close: &state.PortRange{ FromPort: 100, ToPort: 150, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }, expected: "", @@ -120,7 +122,7 @@ func (s *PortsDocSuite) TestOpenAndClosePorts(c *gc.C) { close: &state.PortRange{ FromPort: 100, ToPort: 150, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }, expected: "", @@ -129,19 +131,78 @@ func (s *PortsDocSuite) TestOpenAndClosePorts(c *gc.C) { existing: []state.PortRange{{ FromPort: 100, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }}, open: nil, close: &state.PortRange{ FromPort: 100, ToPort: 300, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }, - expected: "mismatched port ranges 100-200/tcp and 100-300/tcp", - }, - } + expected: "mismatched port ranges: port ranges 100-200/tcp \\(wordpress/0\\) and 100-300/tcp \\(wordpress/0\\) conflict", + }, { + about: "try to open an overlapping port range with different unit", + existing: []state.PortRange{{ + FromPort: 100, + ToPort: 200, + UnitName: s.unit1.Name(), + Protocol: "TCP", + }}, + open: &state.PortRange{ + FromPort: 100, + ToPort: 300, + UnitName: s.unit2.Name(), + Protocol: "TCP", + }, + expected: "cannot open ports 100-300/tcp on machine \"0\": port ranges 100-200/tcp \\(wordpress/0\\) and 100-300/tcp \\(wordpress/1\\) conflict", + }, { + about: "try to open an identical port range with different unit", + existing: []state.PortRange{{ + FromPort: 100, + ToPort: 200, + UnitName: s.unit1.Name(), + Protocol: "TCP", + }}, + open: &state.PortRange{ + FromPort: 100, + ToPort: 200, + UnitName: s.unit2.Name(), + Protocol: "TCP", + }, + expected: "cannot open ports 100-200/tcp on machine \"0\": port ranges 100-200/tcp \\(wordpress/0\\) and 100-200/tcp \\(wordpress/1\\) conflict", + }, { + about: "try to open a port range with different protocol with different unit", + existing: []state.PortRange{{ + FromPort: 100, + ToPort: 200, + UnitName: s.unit1.Name(), + Protocol: "TCP", + }}, + open: &state.PortRange{ + FromPort: 100, + ToPort: 200, + UnitName: s.unit2.Name(), + Protocol: "UDP", + }, + expected: "", + }, { + about: "try to open a non-overlapping port range with different unit", + existing: []state.PortRange{{ + FromPort: 100, + ToPort: 200, + UnitName: s.unit1.Name(), + Protocol: "TCP", + }}, + open: &state.PortRange{ + FromPort: 300, + ToPort: 400, + UnitName: s.unit2.Name(), + Protocol: "TCP", + }, + expected: "", + }} for i, t := range testCases { c.Logf("test %d: %s", i, t.about) @@ -188,7 +249,7 @@ func (s *PortsDocSuite) TestOpenInvalidRange(c *gc.C) { portRange := state.PortRange{ FromPort: 400, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", } err := s.ports.OpenPorts(portRange) @@ -199,7 +260,7 @@ func (s *PortsDocSuite) TestCloseInvalidRange(c *gc.C) { portRange := state.PortRange{ FromPort: 100, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", } err := s.ports.OpenPorts(portRange) @@ -210,17 +271,17 @@ func (s *PortsDocSuite) TestCloseInvalidRange(c *gc.C) { err = s.ports.ClosePorts(state.PortRange{ FromPort: 150, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", }) - c.Assert(err, gc.ErrorMatches, "mismatched port ranges 100-200/tcp and 150-200/tcp") + c.Assert(err, gc.ErrorMatches, "mismatched port ranges: port ranges 100-200/tcp \\(wordpress/0\\) and 150-200/tcp \\(wordpress/0\\) conflict") } func (s *PortsDocSuite) TestRemovePortsDoc(c *gc.C) { portRange := state.PortRange{ FromPort: 100, ToPort: 200, - UnitName: s.unit.Name(), + UnitName: s.unit1.Name(), Protocol: "TCP", } err := s.ports.OpenPorts(portRange) @@ -247,59 +308,136 @@ type PortRangeSuite struct{} var _ = gc.Suite(&PortRangeSuite{}) +// Create a port range or panic if it is invalid. +func MustPortRange(unitName string, fromPort, toPort int, protocol string) state.PortRange { + portRange, err := state.NewPortRange(unitName, fromPort, toPort, protocol) + if err != nil { + panic(err) + } + return portRange +} + func (p *PortRangeSuite) TestPortRangeConflicts(c *gc.C) { var testCases = []struct { - about string - first state.PortRange - second state.PortRange - expectConflict bool + about string + first state.PortRange + second state.PortRange + expected interface{} }{{ "identical ports", - state.PortRange{"wordpress/0", 80, 80, "TCP"}, - state.PortRange{"wordpress/0", 80, 80, "TCP"}, - true, + MustPortRange("wordpress/0", 80, 80, "TCP"), + MustPortRange("wordpress/0", 80, 80, "TCP"), + nil, + }, { + "identical port ranges", + MustPortRange("wordpress/0", 80, 100, "TCP"), + MustPortRange("wordpress/0", 80, 100, "TCP"), + nil, }, { "different ports", - state.PortRange{"wordpress/0", 80, 80, "TCP"}, - state.PortRange{"wordpress/0", 90, 90, "TCP"}, - false, + MustPortRange("wordpress/0", 80, 80, "TCP"), + MustPortRange("wordpress/0", 90, 90, "TCP"), + nil, }, { "touching ranges", - state.PortRange{"wordpress/0", 100, 200, "TCP"}, - state.PortRange{"wordpress/0", 201, 240, "TCP"}, - false, + MustPortRange("wordpress/0", 100, 200, "TCP"), + MustPortRange("wordpress/0", 201, 240, "TCP"), + nil, }, { "touching ranges with overlap", - state.PortRange{"wordpress/0", 100, 200, "TCP"}, - state.PortRange{"wordpress/0", 200, 240, "TCP"}, - true, + MustPortRange("wordpress/0", 100, 200, "TCP"), + MustPortRange("wordpress/0", 200, 240, "TCP"), + "port ranges .* conflict", + }, { + "identical ports with different protocols", + MustPortRange("wordpress/0", 80, 80, "UDP"), + MustPortRange("wordpress/0", 80, 80, "TCP"), + nil, }, { - "different protocols", - state.PortRange{"wordpress/0", 80, 80, "UDP"}, - state.PortRange{"wordpress/0", 80, 80, "TCP"}, - false, + "overlapping ranges with different protocols", + MustPortRange("wordpress/0", 80, 200, "UDP"), + MustPortRange("wordpress/0", 80, 300, "TCP"), + nil, }, { "outside range", - state.PortRange{"wordpress/0", 100, 200, "TCP"}, - state.PortRange{"wordpress/0", 80, 80, "TCP"}, - false, + MustPortRange("wordpress/0", 100, 200, "TCP"), + MustPortRange("wordpress/0", 80, 80, "TCP"), + nil, }, { "overlap end", - state.PortRange{"wordpress/0", 100, 200, "TCP"}, - state.PortRange{"wordpress/0", 80, 120, "TCP"}, - true, + MustPortRange("wordpress/0", 100, 200, "TCP"), + MustPortRange("wordpress/0", 80, 120, "TCP"), + "port ranges .* conflict", }, { "complete overlap", - state.PortRange{"wordpress/0", 100, 200, "TCP"}, - state.PortRange{"wordpress/0", 120, 140, "TCP"}, - true, + MustPortRange("wordpress/0", 100, 200, "TCP"), + MustPortRange("wordpress/0", 120, 140, "TCP"), + "port ranges .* conflict", + }, { + "overlap with same end", + MustPortRange("wordpress/0", 100, 200, "TCP"), + MustPortRange("wordpress/0", 120, 200, "TCP"), + "port ranges .* conflict", + }, { + "overlap with same start", + MustPortRange("wordpress/0", 100, 200, "TCP"), + MustPortRange("wordpress/0", 100, 120, "TCP"), + "port ranges .* conflict", + }, { + "invalid port range", + state.PortRange{"wordpress/0", 100, 80, "TCP"}, + MustPortRange("wordpress/0", 80, 80, "TCP"), + "invalid port range 100-80", + }, { + "different units, same port", + MustPortRange("mysql/0", 80, 80, "TCP"), + MustPortRange("wordpress/0", 80, 80, "TCP"), + "port ranges .* conflict", + }, { + "different units, different port ranges", + MustPortRange("mysql/0", 80, 100, "TCP"), + MustPortRange("wordpress/0", 180, 280, "TCP"), + nil, + }, { + "different units, overlapping port ranges", + MustPortRange("mysql/0", 80, 100, "TCP"), + MustPortRange("wordpress/0", 90, 280, "TCP"), + "port ranges .* conflict", }} for i, t := range testCases { c.Logf("test %d: %s", i, t.about) - c.Check(t.first.ConflictsWith(t.second), gc.Equals, t.expectConflict) - c.Check(t.second.ConflictsWith(t.first), gc.Equals, t.expectConflict) + if t.expected == nil { + c.Check(t.first.CheckConflicts(t.second), gc.IsNil) + c.Check(t.second.CheckConflicts(t.first), gc.IsNil) + } else if _, isString := t.expected.(string); isString { + c.Check(t.first.CheckConflicts(t.second), gc.ErrorMatches, t.expected.(string)) + c.Check(t.second.CheckConflicts(t.first), gc.ErrorMatches, t.expected.(string)) + } + // change test case protocols and test again + c.Logf("test %d: %s (after protocol swap)", i, t.about) + t.first.Protocol = swapProtocol(t.first.Protocol) + t.second.Protocol = swapProtocol(t.second.Protocol) + c.Logf("%+v %+v %v", t.first, t.second, t.expected) + if t.expected == nil { + c.Check(t.first.CheckConflicts(t.second), gc.IsNil) + c.Check(t.second.CheckConflicts(t.first), gc.IsNil) + } else if _, isString := t.expected.(string); isString { + c.Check(t.first.CheckConflicts(t.second), gc.ErrorMatches, t.expected.(string)) + c.Check(t.second.CheckConflicts(t.first), gc.ErrorMatches, t.expected.(string)) + } + + } +} + +func swapProtocol(protocol string) string { + if strings.ToLower(protocol) == "tcp" { + return "udp" + } + if strings.ToLower(protocol) == "udp" { + return "tcp" } + return protocol } func (p *PortRangeSuite) TestPortRangeString(c *gc.C) {