From 8a3ed4f15ed1a28d0c138013be9f1ac03a119d96 Mon Sep 17 00:00:00 2001
From: Dongsu Park <dongsu@endocode.com>
Date: Fri, 15 Jul 2016 15:40:07 +0200
Subject: [PATCH 1/2] agent,registry: reenable check for nil hash in unit state
 publisher

Long time ago there were checks for nil unit hashes in
agent.UnitStatePublisher.publishOne and registry.UnitStateToModel, but
given that units could enter the system without populated hashes, the
checks were disabled: 11949e0 ("agent: publish unit state regardless of
hash"). However the hashes are now always populated, so the corresponding
assertions should be reenabled.

Fixes https://github.com/coreos/fleet/issues/989
---
 agent/unit_state.go         |  9 +++------
 registry/unit_state.go      |  7 +++----
 registry/unit_state_test.go | 26 +++++++++-----------------
 3 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/agent/unit_state.go b/agent/unit_state.go
index 9e0fc44ce..a6c3f1b5a 100644
--- a/agent/unit_state.go
+++ b/agent/unit_state.go
@@ -207,12 +207,9 @@ func newPublisher(reg registry.Registry, ttl time.Duration) publishFunc {
 			// Sanity check - don't want to publish incomplete UnitStates
 			// TODO(jonboulle): consider teasing apart a separate UnitState-like struct
 			// so we can rely on a UnitState always being fully hydrated?
-
-			// See https://github.com/coreos/fleet/issues/720
-			//if len(us.UnitHash) == 0 {
-			//	log.Errorf("Refusing to push UnitState(%s), no UnitHash: %#v", name, us)
-
-			if len(us.MachineID) == 0 {
+			if len(us.UnitHash) == 0 {
+				log.Errorf("Refusing to push UnitState(%s), no UnitHash: %#v", name, us)
+			} else if len(us.MachineID) == 0 {
 				log.Errorf("Refusing to push UnitState(%s), no MachineID: %#v", name, us)
 			} else {
 				log.Debugf("Pushing UnitState(%s) to Registry: %#v", name, us)
diff --git a/registry/unit_state.go b/registry/unit_state.go
index 6c6496d29..b0aec4faf 100644
--- a/registry/unit_state.go
+++ b/registry/unit_state.go
@@ -224,10 +224,9 @@ func unitStateToModel(us *unit.UnitState) *unitStateModel {
 	}
 
 	// Refuse to create a UnitState without a Hash
-	// See https://github.com/coreos/fleet/issues/720
-	//if len(us.UnitHash) == 0 {
-	//	return nil
-	//}
+	if len(us.UnitHash) == 0 {
+		return nil
+	}
 
 	usm := unitStateModel{
 		LoadState:   us.LoadState,
diff --git a/registry/unit_state_test.go b/registry/unit_state_test.go
index 02935685d..d6592db2c 100644
--- a/registry/unit_state_test.go
+++ b/registry/unit_state_test.go
@@ -111,14 +111,13 @@ func TestSaveUnitState(t *testing.T) {
 		t.Fatalf("SaveUnitState of nil state should fail but acted unexpectedly!")
 	}
 
-	// Saving unit state with no hash should succeed for now, but should fail
-	// in the future. See https://github.com/coreos/fleet/issues/720.
-	//r.SaveUnitState(j, us, time.Second)
-	//if len(e.sets) != 1 || e.deletes == nil {
-	//	t.Logf("sets: %#v", e.sets)
-	//	t.Logf("deletes: %#v", e.deletes)
-	//	t.Fatalf("SaveUnitState on UnitState with no hash acted unexpectedly!")
-	//}
+	// Saving unit state with no hash should should fail.
+	r.SaveUnitState(j, us, time.Second)
+	if len(e.sets) != 0 || e.deletes != nil {
+		t.Logf("sets: %#v", e.sets)
+		t.Logf("deletes: %#v", e.deletes)
+		t.Fatalf("SaveUnitState on UnitState with no hash acted unexpectedly!")
+	}
 
 	us.UnitHash = "quickbrownfox"
 	r.SaveUnitState(j, us, time.Second)
@@ -195,8 +194,7 @@ func TestUnitStateToModel(t *testing.T) {
 			want: nil,
 		},
 		{
-			// Unit state with no hash and no machineID is OK
-			// See https://github.com/coreos/fleet/issues/720
+			// Unit state with no hash and no machineID is not OK
 			in: &unit.UnitState{
 				LoadState:   "foo",
 				ActiveState: "bar",
@@ -205,13 +203,7 @@ func TestUnitStateToModel(t *testing.T) {
 				UnitHash:    "",
 				UnitName:    "name",
 			},
-			want: &unitStateModel{
-				LoadState:    "foo",
-				ActiveState:  "bar",
-				SubState:     "baz",
-				MachineState: nil,
-				UnitHash:     "",
-			},
+			want: nil,
 		},
 		{
 			// Unit state with hash but no machineID is OK

From d77b8e43e6c2cc4542060d4e77a517dae39632b6 Mon Sep 17 00:00:00 2001
From: Dongsu Park <dongsu@endocode.com>
Date: Fri, 15 Jul 2016 16:05:36 +0200
Subject: [PATCH 2/2] agent: assign fake unit hash for each case in
 TestDefaultPublisher

Now that nil UnitHash is not allowed, we need to set a fake unit hash
string to each test case in TestDefaultPublisher to avoid test failures.
---
 agent/unit_state_test.go | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/agent/unit_state_test.go b/agent/unit_state_test.go
index 33599dd5d..1183c9798 100644
--- a/agent/unit_state_test.go
+++ b/agent/unit_state_test.go
@@ -227,6 +227,7 @@ func TestDefaultPublisher(t *testing.T) {
 				UnitName:    "foo.service",
 				ActiveState: "active",
 				MachineID:   "xyz",
+				UnitHash:    "quickbrownfox",
 			},
 			nil,
 			[]*unit.UnitState{
@@ -234,6 +235,7 @@ func TestDefaultPublisher(t *testing.T) {
 					UnitName:    "foo.service",
 					ActiveState: "active",
 					MachineID:   "xyz",
+					UnitHash:    "quickbrownfox",
 				},
 			},
 		},
@@ -243,6 +245,7 @@ func TestDefaultPublisher(t *testing.T) {
 			&unit.UnitState{
 				UnitName:    "foo.service",
 				ActiveState: "active",
+				UnitHash:    "quickbrownfox",
 			},
 			nil,
 			[]*unit.UnitState{},
@@ -255,6 +258,7 @@ func TestDefaultPublisher(t *testing.T) {
 				unit.UnitState{
 					UnitName:    "foo.service",
 					ActiveState: "active",
+					UnitHash:    "quickbrownfox",
 				},
 			},
 			[]*unit.UnitState{},