Merge pull request #5690 from satnam6502/failputnover

Give better error message for PUTs with no resource version

pkg/api/rest/update_test.go

@@ -32,8 +32,9 @@ func TestBeforeUpdate(t *testing.T) {
 		{
 			obj: &api.Service{
 				ObjectMeta: api.ObjectMeta{
-					Name:      "foo",
+					Name:            "foo",
-					Namespace: "#$%%invalid",
+					ResourceVersion: "1",
+					Namespace:       "#$%%invalid",
 				},
 			},
 			old:       &api.Service{},
@@ -42,14 +43,16 @@ func TestBeforeUpdate(t *testing.T) {
 		{
 			obj: &api.Service{
 				ObjectMeta: api.ObjectMeta{
-					Name:      "foo",
+					Name:            "foo",
-					Namespace: "valid",
+					ResourceVersion: "1",
+					Namespace:       "valid",
 				},
 			},
 			old: &api.Service{
 				ObjectMeta: api.ObjectMeta{
-					Name:      "bar",
+					Name:            "bar",
-					Namespace: "valid",
+					ResourceVersion: "1",
+					Namespace:       "valid",
 				},
 			},
 			expectErr: true,
@@ -57,8 +60,9 @@ func TestBeforeUpdate(t *testing.T) {
 		{
 			obj: &api.Service{
 				ObjectMeta: api.ObjectMeta{
-					Name:      "foo",
+					Name:            "foo",
-					Namespace: "valid",
+					ResourceVersion: "1",
+					Namespace:       "valid",
 				},
 				Spec: api.ServiceSpec{
 					PortalIP: "1.2.3.4",
@@ -66,8 +70,9 @@ func TestBeforeUpdate(t *testing.T) {
 			},
 			old: &api.Service{
 				ObjectMeta: api.ObjectMeta{
-					Name:      "foo",
+					Name:            "foo",
-					Namespace: "valid",
+					ResourceVersion: "1",
+					Namespace:       "valid",
 				},
 				Spec: api.ServiceSpec{
 					PortalIP: "4.3.2.1",
@@ -78,8 +83,9 @@ func TestBeforeUpdate(t *testing.T) {
 		{
 			obj: &api.Service{
 				ObjectMeta: api.ObjectMeta{
-					Name:      "foo",
+					Name:            "foo",
-					Namespace: api.NamespaceDefault,
+					ResourceVersion: "1",
+					Namespace:       api.NamespaceDefault,
 				},
 				Spec: api.ServiceSpec{
 					PortalIP: "1.2.3.4",
@@ -88,8 +94,9 @@ func TestBeforeUpdate(t *testing.T) {
 			},
 			old: &api.Service{
 				ObjectMeta: api.ObjectMeta{
-					Name:      "foo",
+					Name:            "foo",
-					Namespace: api.NamespaceDefault,
+					ResourceVersion: "1",
+					Namespace:       api.NamespaceDefault,
 				},
 				Spec: api.ServiceSpec{
 					PortalIP: "1.2.3.4",

pkg/api/validation/validation.go

@@ -232,6 +232,11 @@ func ValidateObjectMetaUpdate(old, meta *api.ObjectMeta) errs.ValidationErrorLis
 		meta.CreationTimestamp = old.CreationTimestamp
 	}
 
+	// Reject updates that don't specify a resource version
+	if meta.ResourceVersion == "" {
+		allErrs = append(allErrs, errs.NewFieldInvalid("resourceVersion", meta.ResourceVersion, "resourceVersion must be specified for an update"))
+	}
+
 	if old.Name != meta.Name {
 		allErrs = append(allErrs, errs.NewFieldInvalid("name", meta.Name, "field is immutable"))
 	}

pkg/api/validation/validation_test.go

@@ -56,20 +56,20 @@ func TestValidateObjectMetaCustomName(t *testing.T) {
 
 func TestValidateObjectMetaUpdateIgnoresCreationTimestamp(t *testing.T) {
 	if errs := ValidateObjectMetaUpdate(
-		&api.ObjectMeta{Name: "test", CreationTimestamp: util.NewTime(time.Unix(10, 0))},
+		&api.ObjectMeta{Name: "test", ResourceVersion: "1", CreationTimestamp: util.NewTime(time.Unix(10, 0))},
-		&api.ObjectMeta{Name: "test"},
+		&api.ObjectMeta{Name: "test", ResourceVersion: "1"},
 	); len(errs) != 0 {
 		t.Fatalf("unexpected errors: %v", errs)
 	}
 	if errs := ValidateObjectMetaUpdate(
-		&api.ObjectMeta{Name: "test"},
+		&api.ObjectMeta{Name: "test", ResourceVersion: "1"},
-		&api.ObjectMeta{Name: "test", CreationTimestamp: util.NewTime(time.Unix(10, 0))},
+		&api.ObjectMeta{Name: "test", ResourceVersion: "1", CreationTimestamp: util.NewTime(time.Unix(10, 0))},
 	); len(errs) != 0 {
 		t.Fatalf("unexpected errors: %v", errs)
 	}
 	if errs := ValidateObjectMetaUpdate(
-		&api.ObjectMeta{Name: "test", CreationTimestamp: util.NewTime(time.Unix(11, 0))},
+		&api.ObjectMeta{Name: "test", ResourceVersion: "1", CreationTimestamp: util.NewTime(time.Unix(11, 0))},
-		&api.ObjectMeta{Name: "test", CreationTimestamp: util.NewTime(time.Unix(10, 0))},
+		&api.ObjectMeta{Name: "test", ResourceVersion: "1", CreationTimestamp: util.NewTime(time.Unix(10, 0))},
 	); len(errs) != 0 {
 		t.Fatalf("unexpected errors: %v", errs)
 	}
@@ -1208,6 +1208,8 @@ func TestValidatePodUpdate(t *testing.T) {
 	}
 
 	for _, test := range tests {
+		test.a.ObjectMeta.ResourceVersion = "1"
+		test.b.ObjectMeta.ResourceVersion = "1"
 		errs := ValidatePodUpdate(&test.a, &test.b)
 		if test.isValid {
 			if len(errs) != 0 {
@@ -1522,6 +1524,8 @@ func TestValidateReplicationControllerUpdate(t *testing.T) {
 		},
 	}
 	for _, successCase := range successCases {
+		successCase.old.ObjectMeta.ResourceVersion = "1"
+		successCase.update.ObjectMeta.ResourceVersion = "1"
 		if errs := ValidateReplicationControllerUpdate(&successCase.old, &successCase.update); len(errs) != 0 {
 			t.Errorf("expected success: %v", errs)
 		}
@@ -2126,6 +2130,8 @@ func TestValidateMinionUpdate(t *testing.T) {
 		}, true},
 	}
 	for i, test := range tests {
+		test.oldMinion.ObjectMeta.ResourceVersion = "1"
+		test.minion.ObjectMeta.ResourceVersion = "1"
 		errs := ValidateMinionUpdate(&test.oldMinion, &test.minion)
 		if test.valid && len(errs) > 0 {
 			t.Errorf("%d: Unexpected error: %v", i, errs)
@@ -2295,6 +2301,8 @@ func TestValidateServiceUpdate(t *testing.T) {
 			}, true},
 	}
 	for i, test := range tests {
+		test.oldService.ObjectMeta.ResourceVersion = "1"
+		test.service.ObjectMeta.ResourceVersion = "1"
 		errs := ValidateServiceUpdate(&test.oldService, &test.service)
 		if test.valid && len(errs) > 0 {
 			t.Errorf("%d: Unexpected error: %v", i, errs)
@@ -2560,6 +2568,8 @@ func TestValidateNamespaceFinalizeUpdate(t *testing.T) {
 			}, true},
 	}
 	for i, test := range tests {
+		test.namespace.ObjectMeta.ResourceVersion = "1"
+		test.oldNamespace.ObjectMeta.ResourceVersion = "1"
 		errs := ValidateNamespaceFinalizeUpdate(&test.namespace, &test.oldNamespace)
 		if test.valid && len(errs) > 0 {
 			t.Errorf("%d: Unexpected error: %v", i, errs)
@@ -2600,6 +2610,8 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) {
 			}, false},
 	}
 	for i, test := range tests {
+		test.namespace.ObjectMeta.ResourceVersion = "1"
+		test.oldNamespace.ObjectMeta.ResourceVersion = "1"
 		errs := ValidateNamespaceStatusUpdate(&test.oldNamespace, &test.namespace)
 		if test.valid && len(errs) > 0 {
 			t.Errorf("%d: Unexpected error: %v", i, errs)
@@ -2670,6 +2682,8 @@ func TestValidateNamespaceUpdate(t *testing.T) {
 		}, true},
 	}
 	for i, test := range tests {
+		test.namespace.ObjectMeta.ResourceVersion = "1"
+		test.oldNamespace.ObjectMeta.ResourceVersion = "1"
 		errs := ValidateNamespaceUpdate(&test.oldNamespace, &test.namespace)
 		if test.valid && len(errs) > 0 {
 			t.Errorf("%d: Unexpected error: %v", i, errs)

pkg/registry/service/rest_test.go

@@ -129,15 +129,21 @@ func TestServiceStorageValidatesCreate(t *testing.T) {
 func TestServiceRegistryUpdate(t *testing.T) {
 	ctx := api.NewDefaultContext()
 	storage, registry, _ := NewTestREST(t, nil)
-	registry.CreateService(ctx, &api.Service{
+	svc, err := registry.CreateService(ctx, &api.Service{
-		ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault},
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1", Namespace: api.NamespaceDefault},
 		Spec: api.ServiceSpec{
 			Port:     6502,
 			Selector: map[string]string{"bar": "baz1"},
 		},
 	})
+
+	if err != nil {
+		t.Fatalf("Expected no error: %v", err)
+	}
 	updated_svc, created, err := storage.Update(ctx, &api.Service{
-		ObjectMeta: api.ObjectMeta{Name: "foo"},
+		ObjectMeta: api.ObjectMeta{
+			Name:            "foo",
+			ResourceVersion: svc.ResourceVersion},
 		Spec: api.ServiceSpec{
 			Port:            6502,
 			Selector:        map[string]string{"bar": "baz2"},
@@ -305,7 +311,7 @@ func TestServiceRegistryUpdateExternalService(t *testing.T) {
 
 	// Create non-external load balancer.
 	svc1 := &api.Service{
-		ObjectMeta: api.ObjectMeta{Name: "foo"},
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"},
 		Spec: api.ServiceSpec{
 			Port:                       6502,
 			Selector:                   map[string]string{"bar": "baz"},
@@ -546,7 +552,7 @@ func TestServiceRegistryIPUpdate(t *testing.T) {
 	rest.portalMgr.randomAttempts = 0
 
 	svc := &api.Service{
-		ObjectMeta: api.ObjectMeta{Name: "foo"},
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"},
 		Spec: api.ServiceSpec{
 			Selector:        map[string]string{"bar": "baz"},
 			Port:            6502,
@@ -589,7 +595,7 @@ func TestServiceRegistryIPExternalLoadBalancer(t *testing.T) {
 	rest.portalMgr.randomAttempts = 0
 
 	svc := &api.Service{
-		ObjectMeta: api.ObjectMeta{Name: "foo"},
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"},
 		Spec: api.ServiceSpec{
 			Selector: map[string]string{"bar": "baz"},
 			Port:     6502,