Merge lp:~themue/juju-core/go-state-relation-endpoint-verification into lp:~juju/juju-core/trunk

[Frank Mueller]

state: Improved verification of relation endpoints.

AddRelation() consists of several writings to ZooKeeper. Now
the whole verification is done before the writings start.

https://codereview.appspot.com/6305067/

[Roger Peppe (rogpeppe)]

LGTM, although i'm sure i don't understand all the implications.

https://codereview.appspot.com/6305067/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6305067/diff/1/state/relation.go#newcode63
state/relation.go:63: return fmt.Errorf("endpoint has invalid role: %q",
e.RelationRole)
s/://

generally we use ":" for indicating sub-errors, not for information
within a particular error.

https://codereview.appspot.com/6305067/

[William Reade (fwereade)]

I'm not really convinced this helps us much. The only sane way to sanely
construct a RelationEndpoint is from a service+charm in the first
place... are we worried that we'll get bad data from those, or that
ill-disciplined programmers will be constructing them at random and
flinging them at the wall to see what sticks?

(I presume we're really worried about something else I haven't thought
of. Elaborate please?)

https://codereview.appspot.com/6305067/

[William Reade (fwereade)]

I still doubt this will have great value in practice, but LGTM.

https://codereview.appspot.com/6305067/

[Gustavo Niemeyer (niemeyer)]

On 2012/06/11 08:22:47, fwereade wrote:
> I still doubt this will have great value in practice, but LGTM.

Sorry Frank, I have to agree with William, even though the suggestion to
prevent this from happening was mine in the first place.

My misguided suggestion was based on the fact I had wrong memories
regarding the way we handle "deploy serviceA:foo serviceB:bar" commands.
We don't create the relation endpoint directly, but rather call a method
to resolve what we call the *descriptor* "<service>[:<relname>]". This
is the place that must have that validation to prevent errors from the
user from leaking through.

I apologize for wasting your time on this.

https://codereview.appspot.com/6305067/

Unmerged revisions

214. By Frank Mueller on 2012-06-07

state: Improved verification of endpoint before relation is added.

213. By Roger Peppe on 2012-06-07

state: store units inside their respective services

Making this change simplifies a lot of code - no need
to keep a separate sequence numbering system for units,
for example.

(original proposal at https://codereview.appspot.com/6247066/)

R=
CC=
https://codereview.appspot.com/6300060

212. By Gustavo Niemeyer on 2012-06-06

.lbox: update to juju-core

211. By Gustavo Niemeyer on 2012-06-06

Translated all paths to use the juju-core project in Launchpad.

210. By Frank Mueller on 2012-06-06

state: Changed relation service mapping in topology.

The first approach mapped roles to service key and relation
name opposite to todays Python implementation. This reduces
the flexibility for future purposes (e.g. multiple peers).
So the mapping has now been changed back from service keys
to relation role and name.

R=
CC=
https://codereview.appspot.com/6268049

209. By Gustavo Niemeyer on 2012-06-06

state: rename Environment method to EnvironConfig

R=TheMue, fwereade
CC=
https://codereview.appspot.com/6295048

208. By William Reade on 2012-06-06

subordinate service units can now be added to principal service units

This entails the following changes:

* new Service.AddUnitSubordinateTo method, which requires a principal unit
* topology.AddUnit now takes an additional principalKey string argument,
  which will be empty for a principal unit;
* new topology.UnitPrincipalKey, which returns the principal unit key for a
  subordinate unit or an error for a principal unit;
* new Unit.IsPrincipal method, which returns whether the unit is a principal
  unit;
* checks to prevent subordinate units being assigned directly to machines.

Most of the diff lines are a result of the change to topology.AddUnit;
sorry for the noise.

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6268050

207. By Frank Mueller on 2012-06-06

state: Added the method RemoveRelation() to State.

Relation is now an interface defining the single
method relationKey() to allow a function signature
like today in Python. The implementation is now
named relation and can be accessed via type
assertion.

R=niemeyer
CC=
https://codereview.appspot.com/6250076

206. By William Reade on 2012-06-06

trivial: fix inconsistency in deploy command documentation

R=
CC=
https://codereview.appspot.com/6304045

205. By William Reade on 2012-06-06

add AssignmentPolicy to Environ

This is very likely to migrate to a Settings type at some stage, but I don't
think that's justified yet.

R=niemeyer
CC=
https://codereview.appspot.com/6259062

=== modified file 'state/relation.go'
--- state/relation.go	2012-06-01 10:33:35 +0000
+++ state/relation.go	2012-06-07 15:36:23 +0000
@@ -1,6 +1,8 @@
 package state
 
-import ()
+import (
+	"fmt"
+)
 
 // RelationRole defines the role of a relation endpoint.
 type RelationRole string
@@ -47,6 +49,24 @@
 	return e.ServiceName + ":" + e.RelationName
 }
 
+// checkValues performs basic tests on the endpoint data without
+// checking the validity of identifiers.
+func (e *RelationEndpoint) checkValues() error {
+	switch {
+	case e.ServiceName == "":
+		return fmt.Errorf("endpoint has empty service name")
+	case e.Interface == "":
+		return fmt.Errorf("endpoint has empty interface")
+	case e.RelationName == "":
+		return fmt.Errorf("endpoint has empty relation name")
+	case e.RelationRole != RoleProvider && e.RelationRole != RoleRequirer && e.RelationRole != RolePeer:
+		return fmt.Errorf("endpoint has invalid role: %q", e.RelationRole)
+	case e.RelationScope != ScopeGlobal && e.RelationScope != ScopeContainer:
+		return fmt.Errorf("endpoint has invalid scope: %q", e.RelationScope)
+	}
+	return nil
+}
+
 // Relation represents a link between services, or within a
 // service (in the case of peer relations).
 type Relation struct {

=== modified file 'state/state.go'
--- state/state.go	2012-06-07 11:51:54 +0000
+++ state/state.go	2012-06-07 15:36:23 +0000
@@ -317,11 +317,19 @@
 		return nil, nil, fmt.Errorf("relation already exists")
 	}
 	scope := ScopeGlobal
+	serviceKeys := make(map[RelationEndpoint]string)
 	for _, endpoint := range endpoints {
 		if endpoint.RelationScope == ScopeContainer {
 			scope = ScopeContainer
-			break
-		}
+		}
+		if err := endpoint.checkValues(); err != nil {
+			return nil, nil, err
+		}
+		serviceKey, err := t.ServiceKey(endpoint.ServiceName)
+		if err != nil {
+			return nil, nil, err
+		}
+		serviceKeys[endpoint] = serviceKey
 	}
 	// Add a new relation node depending on the scope. Afterwards
 	// create a node and a service relation per endpoint.
@@ -331,10 +339,6 @@
 	}
 	serviceRelations := []*ServiceRelation{}
 	for _, endpoint := range endpoints {
-		serviceKey, err := t.ServiceKey(endpoint.ServiceName)
-		if err != nil {
-			return nil, nil, err
-		}
 		// The relation endpoint node is only created if the scope is 
 		// global. In case of container scoped relations the creation 
 		// per container occurs in ServiceRelation.AddUnit.
@@ -346,7 +350,7 @@
 		serviceRelations = append(serviceRelations, &ServiceRelation{
 			st:            s,
 			relationKey:   relationKey,
-			serviceKey:    serviceKey,
+			serviceKey:    serviceKeys[endpoint],
 			relationScope: endpoint.RelationScope,
 			relationRole:  endpoint.RelationRole,
 			relationName:  endpoint.RelationName,

=== modified file 'state/state_test.go'
--- state/state_test.go	2012-06-07 11:51:54 +0000
+++ state/state_test.go	2012-06-07 15:36:23 +0000
@@ -1223,6 +1223,31 @@
 	c.Assert(err, ErrorMatches, `can't add relations between 3 services`)
 }
 
+func (s *StateSuite) TestAddRelationInvalidEndpoints(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("riak", dummy)
+	// Empty service name.
+	riakep := state.RelationEndpoint{"", "ring", "cache", state.RolePeer, state.ScopeGlobal}
+	_, _, err := s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `endpoint has empty service name`)
+	// Empty interface.
+	riakep = state.RelationEndpoint{"riak", "", "cache", state.RolePeer, state.ScopeGlobal}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `endpoint has empty interface`)
+	// Empty relation name.
+	riakep = state.RelationEndpoint{"riak", "ring", "", state.RolePeer, state.ScopeGlobal}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `endpoint has empty relation name`)
+	// Invalid role.
+	riakep = state.RelationEndpoint{"riak", "ring", "cache", state.RelationRole("foo"), state.ScopeGlobal}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `can't add non-peer relation with a single service`)
+	// Invalid role.
+	riakep = state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.RelationScope("foo")}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `endpoint has invalid scope: "foo"`)
+}
+
 func (s *StateSuite) TestEnvironConfig(c *C) {
 	path, err := s.zkConn.Create("/environment", "type: dummy\nname: foo\n", 0, zookeeper.WorldACL(zookeeper.PERM_ALL))
 	c.Assert(err, IsNil)