validation: take parentRef.port into account

Currently, we cannot have: ```yaml parentRefs: - name: gw1 port: 80 - name: gw1 port: 8080 ``` This seems like an oversight; our validation only takes sectionName into consideration. Note: this DOES allow overlapping parentSection selection. For exampe, i may select port=80 and sectionName=http which refer to the same thing. This is not something we can catch in validation. Perhaps we need to clarify the behavior here in the spec, if its not clear. Note this could always happen with 2 different routes anyways, though.
kubernetes-sigs · May 4, 2023 · b9228d9 · b9228d9
1 parent c67f3fd
commit b9228d9
Show file tree

Hide file tree

Showing 2 changed files with 107 additions and 20 deletions.
diff --git a/apis/v1beta1/validation/common.go b/apis/v1beta1/validation/common.go
@@ -35,30 +35,48 @@ func ValidateParentRefs(parentRefs []gatewayv1b1.ParentReference, path *field.Pa
 		namespace gatewayv1b1.Namespace
 		kind      gatewayv1b1.Kind
 	}
-	parentRefsSectionMap := make(map[sameKindParentRefs][]gatewayv1b1.SectionName)
+	type parentQualifier struct {
+		section gatewayv1b1.SectionName
+		port    gatewayv1b1.PortNumber
+	}
+	parentRefsSectionMap := make(map[sameKindParentRefs]sets.Set[parentQualifier])
 	for i, p := range parentRefs {
 		targetParentRefs := sameKindParentRefs{name: p.Name, namespace: *new(gatewayv1b1.Namespace), kind: *new(gatewayv1b1.Kind)}
-		targetSection := new(gatewayv1b1.SectionName)
+		pq := parentQualifier{}
 		if p.Namespace != nil {
 			targetParentRefs.namespace = *p.Namespace
 		}
 		if p.Kind != nil {
 			targetParentRefs.kind = *p.Kind
 		}
 		if p.SectionName != nil {
-			targetSection = p.SectionName
+			pq.section = *p.SectionName
+		}
+		if p.Port != nil {
+			pq.port = *p.Port
 		}
 		if s, ok := parentRefsSectionMap[targetParentRefs]; ok {
-			if len(s[0]) == 0 || len(*targetSection) == 0 {
-				errs = append(errs, field.Required(path.Child("parentRefs"), "sectionNames must be specified when more than one parentRef refers to the same parent"))
+			if s.UnsortedList()[0] == (parentQualifier{}) || pq == (parentQualifier{}) {
+				errs = append(errs, field.Required(path.Child("parentRefs"), "sectionNames or port must be specified when more than one parentRef refers to the same parent"))
 				return errs
 			}
-			if sets.New(s...).Has(*targetSection) {
-				errs = append(errs, field.Invalid(path.Index(i).Child("parentRefs").Child("sectionName"), targetSection, "must be unique when ParentRefs includes 2 or more references to the same parent"))
+			if s.Has(pq) {
+				fieldPath := path.Index(i).Child("parentRefs")
+				var val any
+				if len(pq.section) > 0 {
+					fieldPath = fieldPath.Child("sectionName")
+					val = pq.section
+				} else {
+					fieldPath = fieldPath.Child("port")
+					val = pq.port
+				}
+				errs = append(errs, field.Invalid(fieldPath, val, "must be unique when ParentRefs includes 2 or more references to the same parent"))
 				return errs
 			}
+			parentRefsSectionMap[targetParentRefs].Insert(pq)
+		} else {
+			parentRefsSectionMap[targetParentRefs] = sets.New(pq)
 		}
-		parentRefsSectionMap[targetParentRefs] = append(parentRefsSectionMap[targetParentRefs], *targetSection)
 	}
 	return errs
 }

diff --git a/apis/v1beta1/validation/common_test.go b/apis/v1beta1/validation/common_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package validation
 
 import (
+	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -36,7 +37,7 @@ func TestValidateParentRefs(t *testing.T) {
 	tests := []struct {
 		name       string
 		parentRefs []gatewayv1b1.ParentReference
-		errCount   int
+		err        string
 	}{{
 		name: "valid ParentRefs includes 1 reference",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -47,7 +48,7 @@ func TestValidateParentRefs(t *testing.T) {
 				SectionName: &sectionA,
 			},
 		},
-		errCount: 0,
+		err: "",
 	}, {
 		name: "valid ParentRefs includes 2 references",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -64,7 +65,7 @@ func TestValidateParentRefs(t *testing.T) {
 				SectionName: &sectionB,
 			},
 		},
-		errCount: 0,
+		err: "",
 	}, {
 		name: "valid ParentRefs when different references have the same section name",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -81,7 +82,7 @@ func TestValidateParentRefs(t *testing.T) {
 				SectionName: &sectionA,
 			},
 		},
-		errCount: 0,
+		err: "",
 	}, {
 		name: "valid ParentRefs includes more references to the same parent",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -104,7 +105,7 @@ func TestValidateParentRefs(t *testing.T) {
 				SectionName: &sectionC,
 			},
 		},
-		errCount: 0,
+		err: "",
 	}, {
 		name: "invalid ParentRefs due to the same section names to the same parentRefs",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -121,7 +122,7 @@ func TestValidateParentRefs(t *testing.T) {
 				SectionName: &sectionA,
 			},
 		},
-		errCount: 1,
+		err: "must be unique when ParentRefs",
 	}, {
 		name: "invalid ParentRefs due to section names not set to the same ParentRefs",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -132,7 +133,7 @@ func TestValidateParentRefs(t *testing.T) {
 				Name: "example",
 			},
 		},
-		errCount: 1,
+		err: "sectionNames or port must be specified",
 	}, {
 		name: "invalid ParentRefs due to more same section names to the same ParentRefs",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -157,7 +158,7 @@ func TestValidateParentRefs(t *testing.T) {
 				SectionName: &sectionA,
 			},
 		},
-		errCount: 1,
+		err: "sectionNames or port must be specified",
 	}, {
 		name: "invalid ParentRefs when one ParentRef section name not set to the same ParentRefs",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -172,7 +173,7 @@ func TestValidateParentRefs(t *testing.T) {
 				SectionName: &sectionA,
 			},
 		},
-		errCount: 1,
+		err: "sectionNames or port must be specified",
 	}, {
 		name: "invalid ParentRefs when next ParentRef section name not set to the same ParentRefs",
 		parentRefs: []gatewayv1b1.ParentReference{
@@ -187,7 +188,67 @@ func TestValidateParentRefs(t *testing.T) {
 				SectionName: nil,
 			},
 		},
-		errCount: 1,
+		err: "sectionNames or port must be specified",
+	}, {
+		name: "valid ParentRefs with multiple port references to the same parent",
+		parentRefs: []gatewayv1b1.ParentReference{
+			{
+				Name:      "example",
+				Namespace: &namespace,
+				Port:      ptrTo(gatewayv1b1.PortNumber(80)),
+			},
+			{
+				Name:      "example",
+				Namespace: &namespace,
+				Port:      ptrTo(gatewayv1b1.PortNumber(81)),
+			},
+		},
+		err: "",
+	}, {
+		name: "valid ParentRefs with multiple mixed references to the same parent",
+		parentRefs: []gatewayv1b1.ParentReference{
+			{
+				Name:      "example",
+				Namespace: &namespace,
+				Port:      ptrTo(gatewayv1b1.PortNumber(80)),
+			},
+			{
+				Name:        "example",
+				Namespace:   &namespace,
+				SectionName: &sectionA,
+			},
+		},
+		err: "",
+	}, {
+		name: "invalid ParentRefs due to same port references to the same parent",
+		parentRefs: []gatewayv1b1.ParentReference{
+			{
+				Name:      "example",
+				Namespace: &namespace,
+				Port:      ptrTo(gatewayv1b1.PortNumber(80)),
+			},
+			{
+				Name:      "example",
+				Namespace: &namespace,
+				Port:      ptrTo(gatewayv1b1.PortNumber(80)),
+			},
+		},
+		err: "port: Invalid value: 80: must be unique when ParentRefs",
+	}, {
+		name: "invalid ParentRefs due to mixed port references to the same parent",
+		parentRefs: []gatewayv1b1.ParentReference{
+			{
+				Name:      "example",
+				Namespace: &namespace,
+				Port:      ptrTo(gatewayv1b1.PortNumber(80)),
+			},
+			{
+				Name:      "example",
+				Namespace: &namespace,
+				Port:      nil,
+			},
+		},
+		err: "Required value: sectionNames or port must be specified",
 	}}
 
 	for _, tc := range tests {
@@ -196,8 +257,16 @@ func TestValidateParentRefs(t *testing.T) {
 				ParentRefs: tc.parentRefs,
 			}
 			errs := ValidateParentRefs(spec.ParentRefs, path.Child("spec"))
-			if len(errs) != tc.errCount {
-				t.Errorf("got %d errors, want %d errors: %s", len(errs), tc.errCount, errs)
+			if tc.err == "" {
+				if len(errs) != 0 {
+					t.Errorf("got %d errors, want none: %s", len(errs), errs)
+				}
+			} else {
+				if errs == nil {
+					t.Errorf("got no errors, want %q", tc.err)
+				} else if !strings.Contains(errs.ToAggregate().Error(), tc.err) {
+					t.Errorf("got %d errors, want %q: %s", len(errs), tc.err, errs)
+				}
 			}
 		})
 	}