Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: align using CEL for target ref validation #364

Merged
merged 6 commits into from
Dec 13, 2023
Merged

refactor: align using CEL for target ref validation #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee0a2bd
refactor: use standard lib instead for slices
KevFan Dec 7, 2023
04f7c16
refactor: remove redundant validation code from auth policy
KevFan Dec 7, 2023
737d3c4
refactor: use kubebuilder validation for supported target refs for rlp
KevFan Dec 7, 2023
63f491a
test: target ref validation integrations tests for RLP and AP
KevFan Dec 8, 2023
fbde9c4
refactor: integration test auth policy route selector CEL
KevFan Dec 11, 2023
aa55a03
refactor: rlp route selector validation using CEL
KevFan Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 0 additions & 50 deletions api/v1beta2/authpolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
139 changes: 0 additions & 139 deletions api/v1beta2/authpolicy_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
20 changes: 3 additions & 17 deletions api/v1beta2/ratelimitpolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

// +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
Expand Down Expand Up @@ -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
}

Expand Down
40 changes: 2 additions & 38 deletions api/v1beta2/ratelimitpolicy_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
}
Expand Down
10 changes: 10 additions & 0 deletions bundle/manifests/kuadrant.io_ratelimitpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
10 changes: 10 additions & 0 deletions config/crd/bases/kuadrant.io_ratelimitpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Loading
Loading