refactor: align using CEL for target ref validation (#364)

* refactor: use standard lib instead for slices * refactor: remove redundant validation code from auth policy * refactor: use kubebuilder validation for supported target refs for rlp * test: target ref validation integrations tests for RLP and AP * refactor: integration test auth policy route selector CEL * refactor: rlp route selector validation using CEL
Kuadrant · Dec 13, 2023 · 5ab0be7 · 5ab0be7
1 parent ec33986
commit 5ab0be7
Show file tree

Hide file tree

Showing 17 changed files with 490 additions and 263 deletions.
diff --git a/api/v1beta2/authpolicy_types.go b/api/v1beta2/authpolicy_types.go
@@ -223,52 +223,10 @@ func (ap *AuthPolicy) TargetKey() client.ObjectKey {
 }
 
 func (ap *AuthPolicy) Validate() error {
-	if ap.Spec.TargetRef.Group != ("gateway.networking.k8s.io") {
-		return fmt.Errorf("invalid targetRef.Group %s. The only supported group is gateway.networking.k8s.io", ap.Spec.TargetRef.Group)
-	}
-
-	switch kind := ap.Spec.TargetRef.Kind; kind {
-	case
-		"HTTPRoute",
-		"Gateway":
-	default:
-		return fmt.Errorf("invalid targetRef.Kind %s. The only supported kinds are HTTPRoute and Gateway", kind)
-	}
-
 	if ap.Spec.TargetRef.Namespace != nil && string(*ap.Spec.TargetRef.Namespace) != ap.Namespace {
 		return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *ap.Spec.TargetRef.Namespace)
 	}
 
-	// prevents usage of routeSelectors in a gateway AuthPolicy
-	if ap.Spec.TargetRef.Kind == ("Gateway") {
-		containRouteSelectors := func(config map[string]RouteSelectorsGetter) bool {
-			if config == nil {
-				return false
-			}
-			for _, config := range config {
-				if len(config.GetRouteSelectors()) > 0 {
-					return true
-				}
-			}
-			return false
-		}
-		configs := []map[string]RouteSelectorsGetter{
-			{"": ap.Spec},
-			toRouteSelectorGetterMap(ap.Spec.AuthScheme.Authentication),
-			toRouteSelectorGetterMap(ap.Spec.AuthScheme.Metadata),
-			toRouteSelectorGetterMap(ap.Spec.AuthScheme.Authorization),
-			toRouteSelectorGetterMap(ap.Spec.AuthScheme.Callbacks),
-		}
-		if r := ap.Spec.AuthScheme.Response; r != nil {
-			configs = append(configs, toRouteSelectorGetterMap(r.Success.Headers), toRouteSelectorGetterMap(r.Success.DynamicMetadata))
-		}
-		for _, config := range configs {
-			if containRouteSelectors(config) {
-				return fmt.Errorf("route selectors not supported when targeting a Gateway")
-			}
-		}
-	}
-
 	return nil
 }
 
@@ -333,11 +291,3 @@ func (l *AuthPolicyList) GetItems() []common.KuadrantPolicy {
 func init() {
 	SchemeBuilder.Register(&AuthPolicy{}, &AuthPolicyList{})
 }
-
-func toRouteSelectorGetterMap[T RouteSelectorsGetter](m map[string]T) map[string]RouteSelectorsGetter {
-	result := make(map[string]RouteSelectorsGetter)
-	for k, v := range m {
-		result[k] = v
-	}
-	return result
-}
diff --git a/api/v1beta2/authpolicy_types_test.go b/api/v1beta2/authpolicy_types_test.go
@@ -247,145 +247,6 @@ func TestAuthPolicyValidate(t *testing.T) {
 		valid   bool
 		message string
 	}{
-		{
-			name: "valid policy targeting a httproute",
-			policy: &AuthPolicy{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "my-policy",
-					Namespace: "my-namespace",
-				},
-				Spec: AuthPolicySpec{
-					TargetRef: gatewayapiv1alpha2.PolicyTargetReference{
-						Group: "gateway.networking.k8s.io",
-						Kind:  "HTTPRoute",
-						Name:  "my-route",
-					},
-				},
-			},
-			valid: true,
-		},
-		{
-			name: "valid policy targeting a gateway",
-			policy: &AuthPolicy{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "my-policy",
-					Namespace: "my-namespace",
-				},
-				Spec: AuthPolicySpec{
-					TargetRef: gatewayapiv1alpha2.PolicyTargetReference{
-						Group: "gateway.networking.k8s.io",
-						Kind:  "Gateway",
-						Name:  "my-gw",
-					},
-				},
-			},
-			valid: true,
-		},
-		{
-			name: "invalid targetRef group",
-			policy: &AuthPolicy{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "my-policy",
-					Namespace: "my-namespace",
-				},
-				Spec: AuthPolicySpec{
-					TargetRef: gatewayapiv1alpha2.PolicyTargetReference{
-						Group: "not-gateway.networking.k8s.io.group",
-						Kind:  "HTTPRoute",
-						Name:  "my-non-gwapi-route",
-					},
-				},
-			},
-			message: "invalid targetRef.Group not-gateway.networking.k8s.io.group. The only supported group is gateway.networking.k8s.io",
-		},
-		{
-			name: "invalid targetRef kind",
-			policy: &AuthPolicy{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "my-policy",
-					Namespace: "my-namespace",
-				},
-				Spec: AuthPolicySpec{
-					TargetRef: gatewayapiv1alpha2.PolicyTargetReference{
-						Group: "gateway.networking.k8s.io",
-						Kind:  "TCPRoute",
-						Name:  "my-tcp-route",
-					},
-				},
-			},
-			message: "invalid targetRef.Kind TCPRoute. The only supported kinds are HTTPRoute and Gateway",
-		},
-		{
-			name: "invalid usage of top-level route selectors with a gateway targetRef",
-			policy: &AuthPolicy{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "my-policy",
-					Namespace: "my-namespace",
-				},
-				Spec: AuthPolicySpec{
-					TargetRef: gatewayapiv1alpha2.PolicyTargetReference{
-						Group: "gateway.networking.k8s.io",
-						Kind:  "Gateway",
-						Name:  "my-gw",
-					},
-					RouteSelectors: []RouteSelector{
-						{
-							Hostnames: []gatewayapiv1.Hostname{"*.foo.io"},
-							Matches: []gatewayapiv1.HTTPRouteMatch{
-								{
-									Path: &gatewayapiv1.HTTPPathMatch{
-										Value: ptr.To("/foo"),
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-			message: "route selectors not supported when targeting a Gateway",
-		},
-		{
-			name: "invalid usage of config-level route selectors with a gateway targetRef",
-			policy: &AuthPolicy{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "my-policy",
-					Namespace: "my-namespace",
-				},
-				Spec: AuthPolicySpec{
-					TargetRef: gatewayapiv1alpha2.PolicyTargetReference{
-						Group: "gateway.networking.k8s.io",
-						Kind:  "Gateway",
-						Name:  "my-gw",
-					},
-					AuthScheme: AuthSchemeSpec{
-						Authentication: map[string]AuthenticationSpec{
-							"my-rule": {
-								AuthenticationSpec: authorinoapi.AuthenticationSpec{
-									AuthenticationMethodSpec: authorinoapi.AuthenticationMethodSpec{
-										AnonymousAccess: &authorinoapi.AnonymousAccessSpec{},
-									},
-								},
-								CommonAuthRuleSpec: CommonAuthRuleSpec{
-									RouteSelectors: []RouteSelector{
-										{
-											Hostnames: []gatewayapiv1.Hostname{"*.foo.io"},
-											Matches: []gatewayapiv1.HTTPRouteMatch{
-												{
-													Path: &gatewayapiv1.HTTPPathMatch{
-														Value: ptr.To("/foo"),
-													},
-												},
-											},
-										},
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-			message: "route selectors not supported when targeting a Gateway",
-		},
 		{
 			name: "invalid targetRef namespace",
 			policy: &AuthPolicy{

diff --git a/api/v1beta2/ratelimitpolicy_types.go b/api/v1beta2/ratelimitpolicy_types.go
@@ -114,8 +114,11 @@ func (l Limit) CountersAsStringList() []string {
 }
 
 // RateLimitPolicySpec defines the desired state of RateLimitPolicy
+// +kubebuilder:validation:XValidation:rule="self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x, has(self.limits[x].routeSelectors))",message="route selectors not supported when targeting a Gateway"
 type RateLimitPolicySpec struct {
 	// TargetRef identifies an API object to apply policy to.
+	// +kubebuilder:validation:XValidation:rule="self.group == 'gateway.networking.k8s.io'",message="Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'"
+	// +kubebuilder:validation:XValidation:rule="self.kind == 'HTTPRoute' || self.kind == 'Gateway'",message="Invalid targetRef.kind. The only supported values are 'HTTPRoute' and 'Gateway'"
 	TargetRef gatewayapiv1alpha2.PolicyTargetReference `json:"targetRef"`
 
 	// Limits holds the struct of limits indexed by a unique name
@@ -174,27 +177,10 @@ type RateLimitPolicy struct {
 }
 
 func (r *RateLimitPolicy) Validate() error {
-	if r.Spec.TargetRef.Group != ("gateway.networking.k8s.io") {
-		return fmt.Errorf("invalid targetRef.Group %s. The only supported group is gateway.networking.k8s.io", r.Spec.TargetRef.Group)
-	}
-
-	if r.Spec.TargetRef.Kind != ("HTTPRoute") && r.Spec.TargetRef.Kind != ("Gateway") {
-		return fmt.Errorf("invalid targetRef.Kind %s. The only supported kind types are HTTPRoute and Gateway", r.Spec.TargetRef.Kind)
-	}
-
 	if r.Spec.TargetRef.Namespace != nil && string(*r.Spec.TargetRef.Namespace) != r.Namespace {
 		return fmt.Errorf("invalid targetRef.Namespace %s. Currently only supporting references to the same namespace", *r.Spec.TargetRef.Namespace)
 	}
 
-	// prevents usage of routeSelectors in a gateway RLP
-	if r.Spec.TargetRef.Kind == ("Gateway") {
-		for _, limit := range r.Spec.Limits {
-			if len(limit.RouteSelectors) > 0 {
-				return fmt.Errorf("route selectors not supported when targeting a Gateway")
-			}
-		}
-	}
-
 	return nil
 }
 

diff --git a/api/v1beta2/ratelimitpolicy_types_test.go b/api/v1beta2/ratelimitpolicy_types_test.go
@@ -44,49 +44,13 @@ func testBuildBasicHTTPRouteRLP(name string) *RateLimitPolicy {
 // TestRateLimitPolicyValidation calls rlp.Validate()
 // for a valid return value.
 func TestRateLimitPolicyValidation(t *testing.T) {
-	// valid httproute rlp
 	name := "httproute-a"
-	rlp := testBuildBasicHTTPRouteRLP(name)
-	err := rlp.Validate()
-	if err != nil {
-		t.Fatalf(`rlp.Validate() returned error "%v", wanted nil`, err)
-	}
-
-	// valid gateway rlp
-	name = "gateway-a"
-	rlp = testBuildBasicGatewayRLP(name)
-	err = rlp.Validate()
-	if err != nil {
-		t.Fatalf(`rlp.Validate() returned error "%v", wanted nil`, err)
-	}
-
-	// invalid group
-	rlp = testBuildBasicHTTPRouteRLP(name)
-	rlp.Spec.TargetRef.Group = gatewayapiv1.Group("foo.example.com")
-	err = rlp.Validate()
-	if err == nil {
-		t.Fatal(`rlp.Validate() did not return error and should`)
-	}
-	if !strings.Contains(err.Error(), "invalid targetRef.Group") {
-		t.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err)
-	}
-
-	// invalid kind
-	rlp = testBuildBasicHTTPRouteRLP(name)
-	rlp.Spec.TargetRef.Kind = gatewayapiv1.Kind("Foo")
-	err = rlp.Validate()
-	if err == nil {
-		t.Fatal(`rlp.Validate() did not return error and should`)
-	}
-	if !strings.Contains(err.Error(), "invalid targetRef.Kind") {
-		t.Fatalf(`rlp.Validate() did not return expected error. Instead: %v`, err)
-	}
 
 	// Different namespace
-	rlp = testBuildBasicHTTPRouteRLP(name)
+	rlp := testBuildBasicHTTPRouteRLP(name)
 	otherNS := gatewayapiv1.Namespace(rlp.GetNamespace() + "other")
 	rlp.Spec.TargetRef.Namespace = &otherNS
-	err = rlp.Validate()
+	err := rlp.Validate()
 	if err == nil {
 		t.Fatal(`rlp.Validate() did not return error and should`)
 	}

diff --git a/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml b/bundle/manifests/kuadrant.io_ratelimitpolicies.yaml
@@ -429,9 +429,19 @@ spec:
                 - kind
                 - name
                 type: object
+                x-kubernetes-validations:
+                - message: Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'
+                  rule: self.group == 'gateway.networking.k8s.io'
+                - message: Invalid targetRef.kind. The only supported values are 'HTTPRoute'
+                    and 'Gateway'
+                  rule: self.kind == 'HTTPRoute' || self.kind == 'Gateway'
             required:
             - targetRef
             type: object
+            x-kubernetes-validations:
+            - message: route selectors not supported when targeting a Gateway
+              rule: self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x,
+                has(self.limits[x].routeSelectors))
           status:
             description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy
             properties:

diff --git a/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml b/config/crd/bases/kuadrant.io_ratelimitpolicies.yaml
@@ -428,9 +428,19 @@ spec:
                 - kind
                 - name
                 type: object
+                x-kubernetes-validations:
+                - message: Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'
+                  rule: self.group == 'gateway.networking.k8s.io'
+                - message: Invalid targetRef.kind. The only supported values are 'HTTPRoute'
+                    and 'Gateway'
+                  rule: self.kind == 'HTTPRoute' || self.kind == 'Gateway'
             required:
             - targetRef
             type: object
+            x-kubernetes-validations:
+            - message: route selectors not supported when targeting a Gateway
+              rule: self.targetRef.kind != 'Gateway' || !has(self.limits) || !self.limits.exists(x,
+                has(self.limits[x].routeSelectors))
           status:
             description: RateLimitPolicyStatus defines the observed state of RateLimitPolicy
             properties: