Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update port conflict checker to distinguish between conflicts and exact matches. #667

Merged
merged 1 commit into from Sep 9, 2014
Merged

Update port conflict checker to distinguish between conflicts and exact matches. #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions state/compat_test.go
View file
Open in desktop
Expand Up @@ -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)
Expand Down
58 changes: 34 additions & 24 deletions state/ports.go
View file
Open in desktop
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some commentary here to discuss the conditions this method handles.

ie. a overlaps b partially, a is completely within b, a encompases b, a and b are the same range, etc

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 {
Copy link

Choose a reason for hiding this comment

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

How does this actually work?

Copy link

Choose a reason for hiding this comment

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

How about if UnitNames differ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If UnitNames differ, this will result in an ErrPortsConflict.

return nil
}
if a.Protocol != b.Protocol {
return nil
}
if a.ToPort >= b.FromPort && b.ToPort >= a.FromPort {
Copy link

Choose a reason for hiding this comment

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

Just trying to understand what we're checking here:

a := PortRange{FromPort: 1000, ToPort: 2000}
b := PortRange{FromPort: 1999, ToPort: 1001}

In the case above this if block will return ErrPortsConflict, but:

  • We're not checking ToPort <= FromPort (and possibly swapping them if so)
  • b as defined is not a valid range, but if b was {FromPort:1001, ToPort: 1999} (a valid range) ErrPortsConflict won't be returned, but it should as there is indeed conflict / overlap.

So I think there are about 10 cases to check:

  1. if a.FromPort > a.ToPort => "%d-%d is not a valid port range", a.FromPort, a.ToPort
  2. if b.FromPort > b.ToPort => "%d-%d is not a valid port range", b.FromPort, b.ToPort
  3. if a.FromPort == b.FromPort && a.ToPort == b.ToPort => ErrPortsMatch
  4. if a.FromPort < b.FromPort && a.ToPort > b.ToPort => ErrPortsConflict (a completely includes b)
  5. if b.FromPort < a.FromPort && b.ToPort > a.ToPort => ErrPortsConflict (b completely includes a)
  6. if a.FromPort < b.FromPort && a.ToPort < b.ToPort => ErrPortsConflict? (a overlaps partially with b)
  7. if b.FromPort < a.FromPort && b.ToPort < a.ToPort => ErrPortsConflict? (b overlaps partially with a)
  8. (only 2 cases left: a before b, b before a - in both cases no overlap or conflict) => nil

return fmt.Errorf("port ranges %v (%s) and %v (%s) conflict", a, a.UnitName, b, b.UnitName)
}
return nil
}

func (p PortRange) String() string {
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
}
Expand Down