Simplify hostname validation for HTTPRoute (nginx#504)

Commit 52fab05 added HTTPRoute validation, which included both data-plane agnostic and data-plane specific (NGINX) rules for hostnames. After recent team discussion, we concluded that the data-plane specific rules are not needed, as the agnostic rules provide sufficient validation and ensure support across data planes.
miledxz · Mar 23, 2023 · 72680da · 72680da
1 parent 1fc8942
commit 72680da
Show file tree

Hide file tree

Showing 6 changed files with 6 additions and 144 deletions.
diff --git a/internal/nginx/config/validation/http_validator.go b/internal/nginx/config/validation/http_validator.go
@@ -13,10 +13,3 @@ type HTTPValidator struct {
 }
 
 var _ validation.HTTPFieldsValidator = HTTPValidator{}
-
-var hostnameInServerExamples = []string{"host", "example.com"}
-
-// ValidateHostnameInServer validates a hostname to be used in the server_name directive.
-func (HTTPValidator) ValidateHostnameInServer(hostname string) error {
-	return validateEscapedString(hostname, hostnameInServerExamples)
-}
diff --git a/internal/nginx/config/validation/http_validator_test.go b/internal/nginx/config/validation/http_validator_test.go
diff --git a/internal/state/graph/httproute.go b/internal/state/graph/httproute.go
@@ -212,7 +212,7 @@ func buildRoute(
 		UnattachedSectionNameRefs: map[string]conditions.Condition{},
 	}
 
-	err := validateHostnames(validator, ghr.Spec.Hostnames, field.NewPath("spec").Child("hostnames"))
+	err := validateHostnames(ghr.Spec.Hostnames, field.NewPath("spec").Child("hostnames"))
 	if err != nil {
 		r.Valid = false
 		r.Conditions = append(r.Conditions, conditions.NewRouteUnsupportedValue(err.Error()))
@@ -395,17 +395,14 @@ func getHostname(h *v1beta1.Hostname) string {
 	return string(*h)
 }
 
-func validateHostnames(validator validation.HTTPFieldsValidator, hostnames []v1beta1.Hostname, path *field.Path) error {
+func validateHostnames(hostnames []v1beta1.Hostname, path *field.Path) error {
 	var allErrs field.ErrorList
 
 	for i := range hostnames {
 		if err := validateHostname(string(hostnames[i])); err != nil {
 			allErrs = append(allErrs, field.Invalid(path.Index(i), hostnames[i], err.Error()))
 			continue
 		}
-		if err := validator.ValidateHostnameInServer(string(hostnames[i])); err != nil {
-			allErrs = append(allErrs, field.Invalid(path.Index(i), hostnames[i], err.Error()))
-		}
 	}
 
 	return allErrs.ToAggregate()

diff --git a/internal/state/graph/httproute_test.go b/internal/state/graph/httproute_test.go
@@ -520,29 +520,6 @@ func TestBuildRoute(t *testing.T) {
 			},
 			name: "invalid hostname",
 		},
-		{
-			validator: &validationfakes.FakeHTTPFieldsValidator{
-				ValidateHostnameInServerStub: func(string) error {
-					return errors.New("invalid hostname")
-				},
-			},
-			hr: hr,
-			expected: &Route{
-				Source: hr,
-				Valid:  false,
-				SectionNameRefs: map[string]ParentRef{
-					sectionNameOfCreateHTTPRoute: {
-						Idx:     0,
-						Gateway: gatewayNsName,
-					},
-				},
-				UnattachedSectionNameRefs: map[string]conditions.Condition{},
-				Conditions: []conditions.Condition{
-					conditions.NewRouteUnsupportedValue(`spec.hostnames[0]: Invalid value: "example.com": invalid hostname`),
-				},
-			},
-			name: "invalid hostname by the data-plane",
-		},
 		{
 			validator: validatorInvalidFieldsInRule,
 			hr:        hrInvalidMatches,
@@ -1055,13 +1032,11 @@ func TestValidateHostnames(t *testing.T) {
 	const validHostname = "example.com"
 
 	tests := []struct {
-		validator *validationfakes.FakeHTTPFieldsValidator
 		name      string
 		hostnames []v1beta1.Hostname
 		expectErr bool
 	}{
 		{
-			validator: &validationfakes.FakeHTTPFieldsValidator{},
 			hostnames: []v1beta1.Hostname{
 				validHostname,
 				"example.org",
@@ -1071,30 +1046,13 @@ func TestValidateHostnames(t *testing.T) {
 			name:      "multiple valid",
 		},
 		{
-			validator: &validationfakes.FakeHTTPFieldsValidator{},
 			hostnames: []v1beta1.Hostname{
 				validHostname,
 				"",
 			},
 			expectErr: true,
 			name:      "valid and invalid",
 		},
-		{
-			validator: &validationfakes.FakeHTTPFieldsValidator{
-				ValidateHostnameInServerStub: func(h string) error {
-					if h == validHostname {
-						return nil
-					}
-					return errors.New("invalid hostname")
-				},
-			},
-			hostnames: []v1beta1.Hostname{
-				validHostname,
-				"value", // invalid by the validator
-			},
-			expectErr: true,
-			name:      "valid and invalid by the validator",
-		},
 	}
 
 	path := field.NewPath("test")
@@ -1103,7 +1061,7 @@ func TestValidateHostnames(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			g := NewGomegaWithT(t)
 
-			err := validateHostnames(test.validator, test.hostnames, path)
+			err := validateHostnames(test.hostnames, path)
 
 			if test.expectErr {
 				g.Expect(err).To(HaveOccurred())

diff --git a/internal/state/validation/validationfakes/fake_httpfields_validator.go b/internal/state/validation/validationfakes/fake_httpfields_validator.go
diff --git a/internal/state/validation/validator.go b/internal/state/validation/validator.go
@@ -1,6 +1,9 @@
 package validation
 
 // Validators include validators for Gateway API resources from the perspective of a data-plane.
+// It is used for fields that propagate into the data plane configuration. For example, the path in a routing rule.
+// However, not all such fields are validated: NKG will not validate a field using Validators if it is confident that
+// the field is valid.
 type Validators struct {
 	HTTPFieldsValidator HTTPFieldsValidator
 }
@@ -10,7 +13,6 @@ type Validators struct {
 //
 //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . HTTPFieldsValidator
 type HTTPFieldsValidator interface {
-	ValidateHostnameInServer(hostname string) error
 	ValidatePathInPrefixMatch(path string) error
 	ValidateHeaderNameInMatch(name string) error
 	ValidateHeaderValueInMatch(value string) error