feat: enhance ingress exposure validation and add tests for tlsSANs requirements (#1147)

pkg/controller/cluster/cluster.go

@@ -346,17 +346,20 @@ func (c *ClusterReconciler) reconcile(ctx context.Context, cluster *v1beta1.Clus
 	policyName, found := ns.Labels[policy.PolicyNameLabelKey]
 	cluster.Status.PolicyName = policyName
 
-	if found && policyName != "" {
-		var policy v1beta1.VirtualClusterPolicy
-		if err := c.Client.Get(ctx, client.ObjectKey{Name: policyName}, &policy); err != nil {
-			return err
-		}
+	// vcp is nil when the namespace is not bound to any VirtualClusterPolicy.
+	var vcp *v1beta1.VirtualClusterPolicy
 
-		if err := c.validate(cluster, policy); err != nil {
+	if found && policyName != "" {
+		vcp = &v1beta1.VirtualClusterPolicy{}
+		if err := c.Client.Get(ctx, client.ObjectKey{Name: policyName}, vcp); err != nil {
 			return err
 		}
 	}
 
+	if err := c.validate(cluster, vcp); err != nil {
+		return err
+	}
+
 	// if the Version is not specified we will try to use the same Kubernetes version of the host.
 	// This version is stored in the Status object, and it will not be updated if already set.
 	if cluster.Status.HostVersion == "" {
@@ -730,7 +733,7 @@ func (c *ClusterReconciler) ensureIngress(ctx context.Context, cluster *v1beta1.
 	log := ctrl.LoggerFrom(ctx)
 	log.V(1).Info("Ensuring cluster ingress")
 
-	expectedServerIngress := server.Ingress(ctx, cluster)
+	expectedServerIngress := server.Ingress(cluster)
 
 	// delete existing Ingress if Expose or IngressConfig are nil
 	if cluster.Spec.Expose == nil || cluster.Spec.Expose.Ingress == nil {
@@ -983,12 +986,15 @@ func (c *ClusterReconciler) ensureAgent(ctx context.Context, cluster *v1beta1.Cl
 	return agentEnsurer.EnsureResources(ctx)
 }
 
-func (c *ClusterReconciler) validate(cluster *v1beta1.Cluster, policy v1beta1.VirtualClusterPolicy) error {
+// validate validates a Cluster before reconciling it. The policy is nil when the namespace
+// of the Cluster is not bound to any VirtualClusterPolicy: only the checks that depend on it
+// are skipped in that case.
+func (c *ClusterReconciler) validate(cluster *v1beta1.Cluster, policy *v1beta1.VirtualClusterPolicy) error {
 	if cluster.Name == ClusterInvalidName {
 		return fmt.Errorf("%w: invalid cluster name %q", ErrClusterValidation, cluster.Name)
 	}
 
-	if cluster.Spec.Mode != policy.Spec.AllowedMode {
+	if policy != nil && cluster.Spec.Mode != policy.Spec.AllowedMode {
 		return fmt.Errorf("%w: mode %q is not allowed by the policy %q", ErrClusterValidation, cluster.Spec.Mode, policy.Name)
 	}
 
@@ -998,6 +1004,16 @@ func (c *ClusterReconciler) validate(cluster *v1beta1.Cluster, policy v1beta1.Vi
 		}
 	}
 
+	// The Ingress hosts are taken from the tlsSANs, and the IP addresses are not valid.
+	// Without at least one DNS name the generated Ingress would have no rules,
+	// and the API server would reject it.
+	if cluster.Spec.Expose != nil && cluster.Spec.Expose.Ingress != nil {
+		hosts := controller.FilterDNSNames(cluster.Spec.TLSSANs)
+		if len(hosts) == 0 {
+			return fmt.Errorf("%w: expose.ingress requires at least one DNS name in spec.tlsSANs to use as the ingress host", ErrClusterValidation)
+		}
+	}
+
 	return nil
 }
 

pkg/controller/cluster/cluster_test.go

@@ -0,0 +1,128 @@
+package cluster
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/rancher/k3k/pkg/apis/k3k.io/v1beta1"
+)
+
+func Test_validate(t *testing.T) {
+	tests := []struct {
+		name        string
+		clusterName string
+		mode        v1beta1.ClusterMode
+		tlsSANs     []string
+		expose      *v1beta1.ExposeConfig
+		policy      *v1beta1.VirtualClusterPolicy
+		wantErr     string
+	}{
+		{
+			name: "valid cluster without a policy",
+		},
+		{
+			name:   "valid cluster with a policy",
+			policy: newTestPolicy(v1beta1.SharedClusterMode),
+		},
+		{
+			// the name check does not depend on the policy, so it also runs
+			// in namespaces that are not bound to one
+			name:        "invalid cluster name without a policy",
+			clusterName: ClusterInvalidName,
+			wantErr:     "invalid cluster name",
+		},
+		{
+			name:        "invalid cluster name with a policy",
+			clusterName: ClusterInvalidName,
+			policy:      newTestPolicy(v1beta1.SharedClusterMode),
+			wantErr:     "invalid cluster name",
+		},
+		{
+			name:    "mode not allowed by the policy",
+			mode:    v1beta1.VirtualClusterMode,
+			policy:  newTestPolicy(v1beta1.SharedClusterMode),
+			wantErr: "is not allowed by the policy",
+		},
+		{
+			// without a policy there is no allowed mode to check against
+			name: "any mode is allowed without a policy",
+			mode: v1beta1.VirtualClusterMode,
+		},
+		{
+			name:   "expose without ingress",
+			expose: &v1beta1.ExposeConfig{NodePort: &v1beta1.NodePortConfig{}},
+		},
+		{
+			name:    "expose ingress without tlsSANs",
+			expose:  &v1beta1.ExposeConfig{Ingress: &v1beta1.IngressConfig{}},
+			wantErr: "spec.tlsSANs",
+		},
+		{
+			name:    "expose ingress with only IP tlsSANs",
+			tlsSANs: []string{"10.0.0.5", "::1"},
+			expose:  &v1beta1.ExposeConfig{Ingress: &v1beta1.IngressConfig{}},
+			wantErr: "spec.tlsSANs",
+		},
+		{
+			name:    "expose ingress with a DNS tlsSAN",
+			tlsSANs: []string{"10.0.0.5", "my-cluster.example.com"},
+			expose:  &v1beta1.ExposeConfig{Ingress: &v1beta1.IngressConfig{}},
+		},
+		{
+			name:    "no ingress, IP-only tlsSANs is fine",
+			tlsSANs: []string{"10.0.0.5"},
+			expose:  &v1beta1.ExposeConfig{LoadBalancer: &v1beta1.LoadBalancerConfig{}},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			clusterName := tt.clusterName
+			if clusterName == "" {
+				clusterName = "test-cluster"
+			}
+
+			mode := tt.mode
+			if mode == "" {
+				mode = v1beta1.SharedClusterMode
+			}
+
+			cluster := &v1beta1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: "test-namespace"},
+				Spec: v1beta1.ClusterSpec{
+					Mode:    mode,
+					TLSSANs: tt.tlsSANs,
+					Expose:  tt.expose,
+				},
+			}
+
+			// the Client is only needed to validate the customCAs secrets,
+			// which none of these clusters enable
+			reconciler := &ClusterReconciler{}
+
+			err := reconciler.validate(cluster, tt.policy)
+
+			if tt.wantErr == "" {
+				assert.NoError(t, err)
+				return
+			}
+
+			assert.Error(t, err)
+			// the status controller relies on this to report Pending/ValidationFailed
+			// instead of letting the API server reject an invalid resource.
+			assert.True(t, errors.Is(err, ErrClusterValidation))
+			assert.Contains(t, err.Error(), tt.wantErr)
+		})
+	}
+}
+
+func newTestPolicy(allowedMode v1beta1.ClusterMode) *v1beta1.VirtualClusterPolicy {
+	return &v1beta1.VirtualClusterPolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-policy"},
+		Spec:       v1beta1.VirtualClusterPolicySpec{AllowedMode: allowedMode},
+	}
+}

pkg/controller/cluster/server/ingress.go

@@ -1,8 +1,6 @@
 package server
 
 import (
-	"context"
-
 	"k8s.io/utils/ptr"
 
 	networkingv1 "k8s.io/api/networking/v1"
@@ -22,7 +20,7 @@ func IngressName(clusterName string) string {
 	return controller.SafeConcatNameWithPrefix(clusterName, "ingress")
 }
 
-func Ingress(ctx context.Context, cluster *v1beta1.Cluster) networkingv1.Ingress {
+func Ingress(cluster *v1beta1.Cluster) networkingv1.Ingress {
 	ingress := networkingv1.Ingress{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Ingress",
@@ -72,7 +70,9 @@ func ingressRules(cluster *v1beta1.Cluster) []networkingv1.IngressRule {
 		},
 	}
 
-	hosts := cluster.Spec.TLSSANs
+	// the IP addresses of the tlsSANs are skipped: the Ingress API only accepts DNS names
+	hosts := controller.FilterDNSNames(cluster.Spec.TLSSANs)
+
 	for _, host := range hosts {
 		ingressRules = append(ingressRules, networkingv1.IngressRule{
 			Host: host,

pkg/controller/cluster/server/ingress_test.go

@@ -0,0 +1,161 @@
+package server
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/utils/ptr"
+
+	networkingv1 "k8s.io/api/networking/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/rancher/k3k/pkg/apis/k3k.io/v1beta1"
+)
+
+func TestIngress(t *testing.T) {
+	tests := map[string]struct {
+		clusterOpts []func(*v1beta1.Cluster)
+		ingressOpts []func(*networkingv1.Ingress)
+	}{
+		"no expose": {},
+		"expose ingress without tlsSANs has no rules": {
+			clusterOpts: []func(*v1beta1.Cluster){
+				func(c *v1beta1.Cluster) {
+					c.Spec.Expose = &v1beta1.ExposeConfig{
+						Ingress: &v1beta1.IngressConfig{},
+					}
+				},
+			},
+		},
+		"expose ingress with only IP tlsSANs has no rules": {
+			clusterOpts: []func(*v1beta1.Cluster){
+				func(c *v1beta1.Cluster) {
+					c.Spec.TLSSANs = []string{"10.0.0.5", "::1"}
+					c.Spec.Expose = &v1beta1.ExposeConfig{
+						Ingress: &v1beta1.IngressConfig{},
+					}
+				},
+			},
+		},
+		"expose ingress with a wildcard tlsSAN": {
+			clusterOpts: []func(*v1beta1.Cluster){
+				func(c *v1beta1.Cluster) {
+					c.Spec.TLSSANs = []string{"*.example.com"}
+					c.Spec.Expose = &v1beta1.ExposeConfig{
+						Ingress: &v1beta1.IngressConfig{},
+					}
+				},
+			},
+			ingressOpts: []func(*networkingv1.Ingress){
+				func(i *networkingv1.Ingress) {
+					i.Spec.Rules = []networkingv1.IngressRule{testIngressRule("*.example.com")}
+				},
+			},
+		},
+		"expose ingress skips IP tlsSANs": {
+			clusterOpts: []func(*v1beta1.Cluster){
+				func(c *v1beta1.Cluster) {
+					c.Spec.TLSSANs = []string{"10.0.0.5", "my-cluster.example.com"}
+					c.Spec.Expose = &v1beta1.ExposeConfig{
+						Ingress: &v1beta1.IngressConfig{},
+					}
+				},
+			},
+			ingressOpts: []func(*networkingv1.Ingress){
+				func(i *networkingv1.Ingress) {
+					i.Spec.Rules = []networkingv1.IngressRule{testIngressRule("my-cluster.example.com")}
+				},
+			},
+		},
+		"expose ingress with multiple hosts": {
+			clusterOpts: []func(*v1beta1.Cluster){
+				func(c *v1beta1.Cluster) {
+					c.Spec.TLSSANs = []string{"my-cluster.example.com", "other.example.com"}
+					c.Spec.Expose = &v1beta1.ExposeConfig{
+						Ingress: &v1beta1.IngressConfig{},
+					}
+				},
+			},
+			ingressOpts: []func(*networkingv1.Ingress){
+				func(i *networkingv1.Ingress) {
+					i.Spec.Rules = []networkingv1.IngressRule{
+						testIngressRule("my-cluster.example.com"),
+						testIngressRule("other.example.com"),
+					}
+				},
+			},
+		},
+		"expose ingress with class and annotations": {
+			clusterOpts: []func(*v1beta1.Cluster){
+				func(c *v1beta1.Cluster) {
+					c.Spec.TLSSANs = []string{"my-cluster.example.com"}
+					c.Spec.Expose = &v1beta1.ExposeConfig{
+						Ingress: &v1beta1.IngressConfig{
+							IngressClassName: "nginx",
+							Annotations: map[string]string{
+								"nginx.ingress.kubernetes.io/ssl-passthrough": "true",
+							},
+						},
+					}
+				},
+			},
+			ingressOpts: []func(*networkingv1.Ingress){
+				func(i *networkingv1.Ingress) {
+					i.Annotations = map[string]string{
+						"nginx.ingress.kubernetes.io/ssl-passthrough": "true",
+					}
+					i.Spec.IngressClassName = ptr.To("nginx")
+					i.Spec.Rules = []networkingv1.IngressRule{testIngressRule("my-cluster.example.com")}
+				},
+			},
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			cluster := newTestCluster(tt.clusterOpts...)
+			want := newTestIngress(cluster, tt.ingressOpts...)
+			assert.Equal(t, *want, Ingress(cluster))
+		})
+	}
+}
+
+func newTestIngress(cluster *v1beta1.Cluster, opts ...func(*networkingv1.Ingress)) *networkingv1.Ingress {
+	ingress := &networkingv1.Ingress{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Ingress",
+			APIVersion: "networking.k8s.io/v1",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "k3k-test-cluster-ingress",
+			Namespace: cluster.Namespace,
+		},
+	}
+	for _, opt := range opts {
+		opt(ingress)
+	}
+
+	return ingress
+}
+
+func testIngressRule(host string) networkingv1.IngressRule {
+	return networkingv1.IngressRule{
+		Host: host,
+		IngressRuleValue: networkingv1.IngressRuleValue{
+			HTTP: &networkingv1.HTTPIngressRuleValue{
+				Paths: []networkingv1.HTTPIngressPath{
+					{
+						Path:     "/",
+						PathType: ptr.To(networkingv1.PathTypePrefix),
+						Backend: networkingv1.IngressBackend{
+							Service: &networkingv1.IngressServiceBackend{
+								Name: "k3k-test-cluster-service",
+								Port: networkingv1.ServiceBackendPort{Number: 443},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+}

pkg/controller/controller.go

@@ -3,6 +3,7 @@ package controller
 import (
 	"crypto/sha256"
 	"encoding/hex"
+	"net"
 	"slices"
 	"strings"
 	"time"
@@ -45,6 +46,14 @@ func K3SVersion(cluster *v1beta1.Cluster) string {
 	return "latest"
 }
 
+// FilterDNSNames returns only the DNS names of the given list, dropping the IP addresses.
+// It is useful for the fields that cannot hold an IP, like the hosts of an Ingress.
+func FilterDNSNames(names []string) []string {
+	return slices.DeleteFunc(slices.Clone(names), func(name string) bool {
+		return net.ParseIP(name) != nil
+	})
+}
+
 // SafeConcatNameWithPrefix runs the SafeConcatName with extra prefix.
 func SafeConcatNameWithPrefix(name ...string) string {
 	return SafeConcatName(append([]string{namePrefix}, name...)...)

pkg/controller/controller_test.go

@@ -1,6 +1,7 @@
 package controller
 
 import (
+	"slices"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -10,6 +11,46 @@ import (
 	"github.com/rancher/k3k/pkg/apis/k3k.io/v1beta1"
 )
 
+func Test_FilterDNSNames(t *testing.T) {
+	tests := map[string]struct {
+		names    []string
+		expected []string
+	}{
+		"no names": {
+			names:    nil,
+			expected: nil,
+		},
+		"only IPs": {
+			names:    []string{"10.0.0.5", "192.168.1.1", "::1"},
+			expected: []string{},
+		},
+		"only DNS names": {
+			names:    []string{"my-cluster.example.com", "other.example.com"},
+			expected: []string{"my-cluster.example.com", "other.example.com"},
+		},
+		"mixed IPs and DNS names keeps the order": {
+			names:    []string{"10.0.0.5", "my-cluster.example.com", "fd00::1", "other.example.com"},
+			expected: []string{"my-cluster.example.com", "other.example.com"},
+		},
+		"wildcards are kept": {
+			names:    []string{"*.example.com"},
+			expected: []string{"*.example.com"},
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			original := slices.Clone(tt.names)
+
+			filtered := FilterDNSNames(tt.names)
+			assert.Equal(t, tt.expected, filtered)
+
+			// the given slice should be left untouched
+			assert.Equal(t, original, tt.names)
+		})
+	}
+}
+
 func Test_K3S_Image(t *testing.T) {
 	type args struct {
 		cluster  *v1beta1.Cluster