fix(webhook): fix hostname validation logic (#2015)

Signed-off-by: Lukas Boettcher <lukas.boettcher@sick.de>

internal/webhook/ingress/validate_hostnames.go

@@ -136,40 +136,32 @@ func (r *hostnames) validateHostnames(tenant capsulev1beta2.Tenant, hostnames se
 		return nil
 	}
 
-	var valid, matched bool
-
 	tenantHostnameSet := sets.New[string](tenant.Spec.IngressOptions.AllowedHostnames.Exact...)
 
-	var invalidHostnames []string
-
-	if len(hostnames) > 0 {
-		if diff := hostnames.Difference(tenantHostnameSet); len(diff) > 0 {
-			invalidHostnames = append(invalidHostnames, diff.UnsortedList()...)
-		}
-
-		if len(invalidHostnames) == 0 {
-			valid = true
-		}
-	}
-
-	var notMatchingHostnames []string
+	// Only hostnames outside the exact allow-list still need to be checked.
+	notAllowedHostnames := hostnames.Difference(tenantHostnameSet).UnsortedList()
 
 	//nolint:staticcheck
-	if allowedRegex := tenant.Spec.IngressOptions.AllowedHostnames.Regex; len(allowedRegex) > 0 {
-		for currentHostname := range hostnames {
-			matched, _ = regexp.MatchString(allowedRegex, currentHostname)
-			if !matched {
-				notMatchingHostnames = append(notMatchingHostnames, currentHostname)
+	if allowedRegex := tenant.Spec.IngressOptions.AllowedHostnames.Regex; len(allowedRegex) > 0 && len(notAllowedHostnames) > 0 {
+		var failedRegexHostnames []string
+
+		// compile regex once. if compilation fails, the remaining hostnames are not allowed
+		re, err := regexp.Compile(allowedRegex)
+		if err != nil {
+			return caperrors.NewIngressHostnamesNotValid(notAllowedHostnames, *tenant.Spec.IngressOptions.AllowedHostnames)
+		}
+
+		for _, currentHostname := range notAllowedHostnames {
+			if ok := re.MatchString(currentHostname); !ok {
+				failedRegexHostnames = append(failedRegexHostnames, currentHostname)
 			}
 		}
 
-		if len(notMatchingHostnames) == 0 {
-			matched = true
-		}
+		notAllowedHostnames = failedRegexHostnames
 	}
 
-	if !valid && !matched {
-		return caperrors.NewIngressHostnamesNotValid(invalidHostnames, notMatchingHostnames, *tenant.Spec.IngressOptions.AllowedHostnames)
+	if len(notAllowedHostnames) > 0 {
+		return caperrors.NewIngressHostnamesNotValid(notAllowedHostnames, *tenant.Spec.IngressOptions.AllowedHostnames)
 	}
 
 	return nil

internal/webhook/ingress/validate_hostnames_test.go

@@ -0,0 +1,194 @@
+// Copyright 2020-2026 Project Capsule Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package ingress
+
+import (
+	"strings"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+
+	capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2"
+	"github.com/projectcapsule/capsule/pkg/api"
+)
+
+// tenantWithAllowedHostnames builds a minimal Tenant carrying the given exact
+// allow-list and regex under spec.ingressOptions.allowedHostnames.
+func tenantWithAllowedHostnames(exact []string, regex string) capsulev1beta2.Tenant {
+	return capsulev1beta2.Tenant{
+		Spec: capsulev1beta2.TenantSpec{
+			IngressOptions: capsulev1beta2.IngressOptions{
+				AllowedHostnames: &api.AllowedListSpec{
+					Exact: exact,
+					Regex: regex,
+				},
+			},
+		},
+	}
+}
+
+func TestValidateHostnames(t *testing.T) {
+	t.Parallel()
+
+	const (
+		appsRegex = `^[a-z0-9-]{3,40}\.apps\.example\.com$`
+	)
+
+	tests := []struct {
+		name string
+		// tenant carries the allowed hostnames configuration under test.
+		tenant capsulev1beta2.Tenant
+		// hostnames are the Ingress hostnames being validated.
+		hostnames []string
+		// wantErr is true when the hostnames must be denied.
+		wantErr bool
+		// wantDenied lists hostnames that must appear in the denial message.
+		wantDenied []string
+		// wantAbsent lists hostnames that must NOT appear as denied (e.g. valid
+		// via regex but outside the exact list).
+		wantAbsent []string
+	}{
+		{
+			name:      "no allowed hostnames configured allows everything",
+			tenant:    capsulev1beta2.Tenant{},
+			hostnames: []string{"anything.example.com"},
+			wantErr:   false,
+		},
+		{
+			name:      "empty hostname set is allowed",
+			tenant:    tenantWithAllowedHostnames([]string{"a.example.com"}, ""),
+			hostnames: nil,
+			wantErr:   false,
+		},
+		{
+			name:      "all hostnames in exact list are allowed",
+			tenant:    tenantWithAllowedHostnames([]string{"a.example.com", "b.example.com"}, ""),
+			hostnames: []string{"a.example.com", "b.example.com"},
+			wantErr:   false,
+		},
+		{
+			name:       "hostname outside exact list without regex is denied",
+			tenant:     tenantWithAllowedHostnames([]string{"a.example.com"}, ""),
+			hostnames:  []string{"a.example.com", "c.example.com"},
+			wantErr:    true,
+			wantDenied: []string{"c.example.com"},
+		},
+		{
+			name:      "hostnames matching regex are allowed",
+			tenant:    tenantWithAllowedHostnames(nil, `.*\.clastix\.io`),
+			hostnames: []string{"foo.clastix.io", "bar.clastix.io"},
+			wantErr:   false,
+		},
+		{
+			name:      "mixed exact and regex hostnames are allowed",
+			tenant:    tenantWithAllowedHostnames([]string{"a.example.com"}, `.*\.clastix\.io`),
+			hostnames: []string{"a.example.com", "foo.clastix.io"},
+			wantErr:   false,
+		},
+		{
+			name:       "hostname not matching regex is denied",
+			tenant:     tenantWithAllowedHostnames(nil, `.*\.clastix\.io`),
+			hostnames:  []string{"foo.example.com"},
+			wantErr:    true,
+			wantDenied: []string{"foo.example.com"},
+		},
+		{
+			name:   "denies only the hostname that is neither in the exact list nor matches the regex",
+			tenant: tenantWithAllowedHostnames([]string{"allowed.example.com"}, appsRegex),
+			hostnames: []string{
+				"allowed.example.com",  // allowed via exact
+				"web.apps.example.com", // allowed via regex
+				"denied.example.com",   // denied: neither exact nor regex
+			},
+			wantErr:    true,
+			wantDenied: []string{"denied.example.com"},
+			wantAbsent: []string{"web.apps.example.com"},
+		},
+		{
+			name:       "invalid regex denies hostnames outside the exact list",
+			tenant:     tenantWithAllowedHostnames([]string{"a.example.com"}, "("),
+			hostnames:  []string{"a.example.com", "b.example.com"},
+			wantErr:    true,
+			wantDenied: []string{"b.example.com"},
+		},
+		{
+			name:      "invalid regex is ignored when every hostname is in the exact list",
+			tenant:    tenantWithAllowedHostnames([]string{"a.example.com"}, "("),
+			hostnames: []string{"a.example.com"},
+			wantErr:   false,
+		},
+	}
+
+	h := &hostnames{}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			err := h.validateHostnames(tt.tenant, sets.New[string](tt.hostnames...))
+
+			if tt.wantErr && err == nil {
+				t.Fatalf("expected hostnames %v to be denied, got no error", tt.hostnames)
+			}
+
+			if !tt.wantErr {
+				if err != nil {
+					t.Fatalf("expected hostnames %v to be allowed, got error: %v", tt.hostnames, err)
+				}
+
+				return
+			}
+
+			msg := err.Error()
+
+			for _, denied := range tt.wantDenied {
+				if !strings.Contains(msg, denied) {
+					t.Errorf("expected denial message to mention %q, got: %s", denied, msg)
+				}
+			}
+
+			for _, absent := range tt.wantAbsent {
+				if strings.Contains(msg, absent) {
+					t.Errorf("did not expect denial message to mention allowed hostname %q, got: %s", absent, msg)
+				}
+			}
+		})
+	}
+}
+
+// TestValidateHostnamesDeterministic guards against the historical
+// non-deterministic bug: because the hostname set has a randomized iteration
+// order, validation of the same input must always produce the same result.
+func TestValidateHostnamesDeterministic(t *testing.T) {
+	t.Parallel()
+
+	tenant := tenantWithAllowedHostnames(
+		[]string{"allowed.example.com"},
+		`^[a-z0-9-]{3,40}\.apps\.example\.com$`,
+	)
+
+	hostnameSet := sets.New[string](
+		"allowed.example.com",
+		"web.apps.example.com",
+		"denied.example.com",
+	)
+
+	h := &hostnames{}
+
+	for i := 0; i < 50; i++ {
+		err := h.validateHostnames(tenant, hostnameSet)
+		if err == nil {
+			t.Fatalf("iteration %d: expected denial, got no error", i)
+		}
+
+		msg := err.Error()
+		if !strings.Contains(msg, "denied.example.com") {
+			t.Fatalf("iteration %d: expected message to mention denied.example.com, got: %s", i, msg)
+		}
+
+		if strings.Contains(msg, "web.apps.example.com") {
+			t.Fatalf("iteration %d: message wrongly mentions regex-allowed hostname: %s", i, msg)
+		}
+	}
+}

pkg/api/errors/errors_test.go

@@ -80,7 +80,7 @@ func TestErrorConstructors(t *testing.T) {
 		{name: "ingress forbidden", err: apierrors.NewIngressClassForbidden("nginx", allowed), want: "Ingress Class nginx is forbidden"},
 		{name: "ingress collision", err: apierrors.NewIngressHostnameCollision("example.com"), want: "example.com is already used"},
 		{name: "empty ingress hostname", err: apierrors.NewEmptyIngressHostname(api.AllowedListSpec{Exact: []string{"example.com"}, Regex: ".*\\.example\\.com"}), want: "empty hostname is not allowed"},
-		{name: "ingress hostnames invalid", err: apierrors.NewIngressHostnamesNotValid([]string{"bad_host"}, []string{"other.com"}, api.AllowedListSpec{Exact: []string{"example.com"}}), want: "Hostnames [bad_host] are not valid"},
+		{name: "ingress hostnames invalid", err: apierrors.NewIngressHostnamesNotValid([]string{"bad_host"}, api.AllowedListSpec{Exact: []string{"example.com"}}), want: "Hostnames [bad_host] are not valid"},
 		{name: "ingress undefined", err: apierrors.NewIngressClassUndefined(allowed), want: "No Ingress Class is forbidden"},
 		{name: "ingress not valid", err: apierrors.NewIngressClassNotValid("nginx", allowed), want: "Ingress Class nginx is forbidden"},
 		{name: "namespace quota", err: apierrors.NewNamespaceQuotaExceededError(), want: "Cannot exceed Namespace quota"},

pkg/api/errors/ingress.go

@@ -45,9 +45,8 @@ func (i IngressClassForbiddenError) Error() string {
 }
 
 type IngressHostnameNotValidError struct {
-	invalidHostnames     []string
-	notMatchingHostnames []string
-	spec                 api.AllowedListSpec
+	invalidHostnames []string
+	spec             api.AllowedListSpec
 }
 
 type IngressHostnameCollisionError struct {
@@ -76,13 +75,13 @@ func (e EmptyIngressHostnameError) Error() string {
 	return fmt.Sprintf("empty hostname is not allowed for the current Tenant%s", appendHostnameError(e.spec))
 }
 
-func NewIngressHostnamesNotValid(invalidHostnames []string, notMatchingHostnames []string, spec api.AllowedListSpec) error {
-	return &IngressHostnameNotValidError{invalidHostnames: invalidHostnames, notMatchingHostnames: notMatchingHostnames, spec: spec}
+func NewIngressHostnamesNotValid(hostnames []string, spec api.AllowedListSpec) error {
+	return &IngressHostnameNotValidError{invalidHostnames: hostnames, spec: spec}
 }
 
 func (i IngressHostnameNotValidError) Error() string {
-	return fmt.Sprintf("Hostnames %s are not valid for the current Tenant. Hostnames %s not matching for the current Tenant%s",
-		i.invalidHostnames, i.notMatchingHostnames, appendHostnameError(i.spec))
+	return fmt.Sprintf("Hostnames %v are not valid for the current Tenant%s",
+		i.invalidHostnames, appendHostnameError(i.spec))
 }
 
 type IngressClassUndefinedError struct {