Merge pull request #7180 from ddysher/validate-no-dup-ip

Validate Node IPs; clean up validation code

pkg/api/helpers.go

@@ -113,3 +113,20 @@ var standardFinalizers = util.NewStringSet(
 func IsStandardFinalizerName(str string) bool {
 	return standardFinalizers.Has(str)
 }
+
+// AddToNodeAddresses appends the NodeAddresses to the passed-by-pointer slice,
+// only if they do not already exist
+func AddToNodeAddresses(addresses *[]NodeAddress, addAddresses ...NodeAddress) {
+	for _, add := range addAddresses {
+		exists := false
+		for _, existing := range *addresses {
+			if existing.Address == add.Address && existing.Type == add.Type {
+				exists = true
+				break
+			}
+		}
+		if !exists {
+			*addresses = append(*addresses, add)
+		}
+	}
+}

pkg/api/helpers_test.go

@@ -85,3 +85,60 @@ func TestIsStandardResource(t *testing.T) {
 		}
 	}
 }
+
+func TestAddToNodeAddresses(t *testing.T) {
+	testCases := []struct {
+		existing []NodeAddress
+		toAdd    []NodeAddress
+		expected []NodeAddress
+	}{
+		{
+			existing: []NodeAddress{},
+			toAdd:    []NodeAddress{},
+			expected: []NodeAddress{},
+		},
+		{
+			existing: []NodeAddress{},
+			toAdd: []NodeAddress{
+				{Type: NodeExternalIP, Address: "1.1.1.1"},
+				{Type: NodeHostName, Address: "localhost"},
+			},
+			expected: []NodeAddress{
+				{Type: NodeExternalIP, Address: "1.1.1.1"},
+				{Type: NodeHostName, Address: "localhost"},
+			},
+		},
+		{
+			existing: []NodeAddress{},
+			toAdd: []NodeAddress{
+				{Type: NodeExternalIP, Address: "1.1.1.1"},
+				{Type: NodeExternalIP, Address: "1.1.1.1"},
+			},
+			expected: []NodeAddress{
+				{Type: NodeExternalIP, Address: "1.1.1.1"},
+			},
+		},
+		{
+			existing: []NodeAddress{
+				{Type: NodeExternalIP, Address: "1.1.1.1"},
+				{Type: NodeInternalIP, Address: "10.1.1.1"},
+			},
+			toAdd: []NodeAddress{
+				{Type: NodeExternalIP, Address: "1.1.1.1"},
+				{Type: NodeHostName, Address: "localhost"},
+			},
+			expected: []NodeAddress{
+				{Type: NodeExternalIP, Address: "1.1.1.1"},
+				{Type: NodeInternalIP, Address: "10.1.1.1"},
+				{Type: NodeHostName, Address: "localhost"},
+			},
+		},
+	}
+
+	for i, tc := range testCases {
+		AddToNodeAddresses(&tc.existing, tc.toAdd...)
+		if !Semantic.DeepEqual(tc.expected, tc.existing) {
+			t.Error("case[%d], expected: %v, got: %v", i, tc.expected, tc.existing)
+		}
+	}
+}

pkg/api/types.go

@@ -1166,7 +1166,6 @@ type NodeAddress struct {
 
 // NodeResources is an object for conveying resource information about a node.
 // see http://docs.k8s.io/resources.md for more details.
-// TODO: Use ResourceList instead?
 type NodeResources struct {
 	// Capacity represents the available resources of a node
 	Capacity ResourceList `json:"capacity,omitempty"`
@@ -1816,22 +1815,6 @@ const (
 	StrategicMergePatchType PatchType = "application/strategic-merge-patch+json"
 )
 
-// Appends the NodeAddresses to the passed-by-pointer slice, only if they do not already exist
-func AddToNodeAddresses(addresses *[]NodeAddress, addAddresses ...NodeAddress) {
-	for _, add := range addAddresses {
-		exists := false
-		for _, existing := range *addresses {
-			if existing.Address == add.Address && existing.Type == add.Type {
-				exists = true
-				break
-			}
-		}
-		if !exists {
-			*addresses = append(*addresses, add)
-		}
-	}
-}
-
 // Type and constants for component health validation.
 type ComponentConditionType string
 

pkg/api/validation/validation.go

@@ -1074,8 +1074,8 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume) errs.ValidationErrorL
 	return allErrs
 }
 
-// ValidateMinion tests if required fields in the node are set.
+// ValidateNode tests if required fields in the node are set.
-func ValidateMinion(node *api.Node) errs.ValidationErrorList {
+func ValidateNode(node *api.Node) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 	allErrs = append(allErrs, ValidateObjectMeta(&node.ObjectMeta, false, ValidateNodeName).Prefix("metadata")...)
 
@@ -1090,34 +1090,42 @@ func ValidateMinion(node *api.Node) errs.ValidationErrorList {
 	return allErrs
 }
 
-// ValidateMinionUpdate tests to make sure a minion update can be applied.  Modifies oldMinion.
+// ValidateNodeUpdate tests to make sure a node update can be applied.  Modifies oldNode.
-func ValidateMinionUpdate(oldMinion *api.Node, minion *api.Node) errs.ValidationErrorList {
+func ValidateNodeUpdate(oldNode *api.Node, node *api.Node) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
-	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldMinion.ObjectMeta, &minion.ObjectMeta).Prefix("metadata")...)
+	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldNode.ObjectMeta, &node.ObjectMeta).Prefix("metadata")...)
 
 	// TODO: Enable the code once we have better api object.status update model. Currently,
 	// anyone can update node status.
-	// if !api.Semantic.DeepEqual(minion.Status, api.NodeStatus{}) {
+	// if !api.Semantic.DeepEqual(node.Status, api.NodeStatus{}) {
-	// 	allErrs = append(allErrs, errs.NewFieldInvalid("status", minion.Status, "status must be empty"))
+	// 	allErrs = append(allErrs, errs.NewFieldInvalid("status", node.Status, "status must be empty"))
 	// }
 
+	// Validte no duplicate addresses in node status.
+	addresses := make(map[api.NodeAddress]bool)
+	for _, address := range node.Status.Addresses {
+		if _, ok := addresses[address]; ok {
+			allErrs = append(allErrs, fmt.Errorf("duplicate node addresses found"))
+		}
+		addresses[address] = true
+	}
+
 	// TODO: move reset function to its own location
 	// Ignore metadata changes now that they have been tested
-	oldMinion.ObjectMeta = minion.ObjectMeta
+	oldNode.ObjectMeta = node.ObjectMeta
 	// Allow users to update capacity
-	oldMinion.Status.Capacity = minion.Status.Capacity
+	oldNode.Status.Capacity = node.Status.Capacity
 	// Allow users to unschedule node
-	oldMinion.Spec.Unschedulable = minion.Spec.Unschedulable
+	oldNode.Spec.Unschedulable = node.Spec.Unschedulable
 	// Clear status
-	oldMinion.Status = minion.Status
+	oldNode.Status = node.Status
 
 	// TODO: Add a 'real' ValidationError type for this error and provide print actual diffs.
-	if !api.Semantic.DeepEqual(oldMinion, minion) {
+	if !api.Semantic.DeepEqual(oldNode, node) {
-		glog.V(4).Infof("Update failed validation %#v vs %#v", oldMinion, minion)
+		glog.V(4).Infof("Update failed validation %#v vs %#v", oldNode, node)
 		allErrs = append(allErrs, fmt.Errorf("update contains more than labels or capacity changes"))
 	}
 
-	// TODO: validate Spec.Capacity
 	return allErrs
 }
 

pkg/api/validation/validation_test.go

@@ -2055,7 +2055,7 @@ func TestValidateReplicationController(t *testing.T) {
 	}
 }
 
-func TestValidateMinion(t *testing.T) {
+func TestValidateNode(t *testing.T) {
 	validSelector := map[string]string{"a": "b"}
 	invalidSelector := map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "b"}
 	successCases := []api.Node{
@@ -2097,7 +2097,7 @@ func TestValidateMinion(t *testing.T) {
 		},
 	}
 	for _, successCase := range successCases {
-		if errs := ValidateMinion(&successCase); len(errs) != 0 {
+		if errs := ValidateNode(&successCase); len(errs) != 0 {
 			t.Errorf("expected success: %v", errs)
 		}
 	}
@@ -2148,7 +2148,7 @@ func TestValidateMinion(t *testing.T) {
 		},
 	}
 	for k, v := range errorCases {
-		errs := ValidateMinion(&v)
+		errs := ValidateNode(&v)
 		if len(errs) == 0 {
 			t.Errorf("expected failure for %s", k)
 		}
@@ -2168,11 +2168,11 @@ func TestValidateMinion(t *testing.T) {
 	}
 }
 
-func TestValidateMinionUpdate(t *testing.T) {
+func TestValidateNodeUpdate(t *testing.T) {
 	tests := []struct {
-		oldMinion api.Node
+		oldNode api.Node
-		minion    api.Node
+		node    api.Node
-		valid     bool
+		valid   bool
 	}{
 		{api.Node{}, api.Node{}, true},
 		{api.Node{
@@ -2300,14 +2300,50 @@ func TestValidateMinionUpdate(t *testing.T) {
 				Unschedulable: true,
 			},
 		}, true},
+		{api.Node{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo",
+			},
+			Spec: api.NodeSpec{
+				Unschedulable: false,
+			},
+		}, api.Node{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo",
+			},
+			Status: api.NodeStatus{
+				Addresses: []api.NodeAddress{
+					{Type: api.NodeExternalIP, Address: "1.1.1.1"},
+					{Type: api.NodeExternalIP, Address: "1.1.1.1"},
+				},
+			},
+		}, false},
+		{api.Node{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo",
+			},
+			Spec: api.NodeSpec{
+				Unschedulable: false,
+			},
+		}, api.Node{
+			ObjectMeta: api.ObjectMeta{
+				Name: "foo",
+			},
+			Status: api.NodeStatus{
+				Addresses: []api.NodeAddress{
+					{Type: api.NodeExternalIP, Address: "1.1.1.1"},
+					{Type: api.NodeInternalIP, Address: "10.1.1.1"},
+				},
+			},
+		}, true},
 	}
 	for i, test := range tests {
-		test.oldMinion.ObjectMeta.ResourceVersion = "1"
+		test.oldNode.ObjectMeta.ResourceVersion = "1"
-		test.minion.ObjectMeta.ResourceVersion = "1"
+		test.node.ObjectMeta.ResourceVersion = "1"
-		errs := ValidateMinionUpdate(&test.oldMinion, &test.minion)
+		errs := ValidateNodeUpdate(&test.oldNode, &test.node)
 		if test.valid && len(errs) > 0 {
 			t.Errorf("%d: Unexpected error: %v", i, errs)
-			t.Logf("%#v vs %#v", test.oldMinion.ObjectMeta, test.minion.ObjectMeta)
+			t.Logf("%#v vs %#v", test.oldNode.ObjectMeta, test.node.ObjectMeta)
 		}
 		if !test.valid && len(errs) == 0 {
 			t.Errorf("%d: Unexpected non-error", i)

pkg/registry/minion/rest.go

@@ -71,13 +71,13 @@ func (nodeStrategy) PrepareForUpdate(obj, old runtime.Object) {
 // Validate validates a new node.
 func (nodeStrategy) Validate(ctx api.Context, obj runtime.Object) fielderrors.ValidationErrorList {
 	node := obj.(*api.Node)
-	return validation.ValidateMinion(node)
+	return validation.ValidateNode(node)
 }
 
 // ValidateUpdate is the default update validation for an end user.
 func (nodeStrategy) ValidateUpdate(ctx api.Context, obj, old runtime.Object) fielderrors.ValidationErrorList {
-	errorList := validation.ValidateMinion(obj.(*api.Node))
+	errorList := validation.ValidateNode(obj.(*api.Node))
-	return append(errorList, validation.ValidateMinionUpdate(old.(*api.Node), obj.(*api.Node))...)
+	return append(errorList, validation.ValidateNodeUpdate(old.(*api.Node), obj.(*api.Node))...)
 }
 
 type nodeStatusStrategy struct {
@@ -98,7 +98,7 @@ func (nodeStatusStrategy) PrepareForUpdate(obj, old runtime.Object) {
 }
 
 func (nodeStatusStrategy) ValidateUpdate(ctx api.Context, obj, old runtime.Object) fielderrors.ValidationErrorList {
-	return validation.ValidateMinionUpdate(old.(*api.Node), obj.(*api.Node))
+	return validation.ValidateNodeUpdate(old.(*api.Node), obj.(*api.Node))
 }
 
 // ResourceGetter is an interface for retrieving resources by ResourceLocation.