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.

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

[rlp-v2] WasmPlugin reconciliation #204

Merged
merged 15 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fec0420
reconciliation of route selectors for RLPs targeting HTTPRoutes
guicassolato Jun 22, 2023
19edca4
add tests for generating a wasmplugin config out of a complex httprou…
guicassolato Jun 24, 2023
3add050
generate wasm rules only for routeSelector-filtered hostnames that be…
guicassolato Jun 25, 2023
eb0fa8d
fix: pattern expression for hostnames with the right operator - intro…
guicassolato Jun 28, 2023
eb89b8d
fix: compute wasm rules only for the applicable hostnames
guicassolato Jul 6, 2023
a6b54fe
do not generate wasm condition patterns for the hostname when all apply
guicassolato Jul 6, 2023
aa52d24
do not repeat RLPs for each hostname within the wasm config
guicassolato Jul 7, 2023
8206ffc
`rlptools.HTTPRouteRulesFromRouteSelector` moved to `api/v1beta2/rout…
guicassolato Jul 7, 2023
de34731
reconciliation of route selectors for RLPs targeting Gateways
guicassolato Jul 10, 2023
90f963e
match the gateway hostnames instead of the httproute ones when buildi…
guicassolato Jul 10, 2023
5de719f
refactor: make HTTPRouteRuleSelector.Selects return sooner if the HTT…
guicassolato Jul 11, 2023
9e38d57
refactor: ensure wasm rule conditions are generated out of selected H…
guicassolato Jul 11, 2023
2bf7311
Prevent the usage of routeSelectors in RLPs that target a Gateway
guicassolato Jul 11, 2023
98cbe4a
do not generate conditions for gateway policy wasm rules from httprou…
guicassolato Jul 12, 2023
a7ed478
avoid adding rlps without any rules to the wasm config
guicassolato Jul 18, 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
27 changes: 11 additions & 16 deletions api/v1beta2/ratelimitpolicy_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
)

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
Expand All @@ -40,13 +39,14 @@ import (
// +kubebuilder:validation:MaxLength=253
type ContextSelector string

// +kubebuilder:validation:Enum:=eq;neq;startswith;incl;excl;matches
// +kubebuilder:validation:Enum:=eq;neq;startswith;endswith;incl;excl;matches
type WhenConditionOperator string

const (
EqualOperator WhenConditionOperator = "eq"
NotEqualOperator WhenConditionOperator = "neq"
StartsWithOperator WhenConditionOperator = "startswith"
EndsWithOperator WhenConditionOperator = "endswith"
IncludeOperator WhenConditionOperator = "incl"
ExcludeOperator WhenConditionOperator = "excl"
MatchesOperator WhenConditionOperator = "matches"
Expand Down Expand Up @@ -83,20 +83,6 @@ type WhenCondition struct {
Value string `json:"value"`
}

// RouteSelector defines semantics for matching an HTTP request based on conditions
// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
type RouteSelector struct {
// Hostnames defines a set of hostname that should match against the HTTP Host header to select a HTTPRoute to process the request
// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
// +optional
Hostnames []gatewayapiv1beta1.Hostname `json:"hostnames,omitempty"`

// Matches define conditions used for matching the rule against incoming HTTP requests.
// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
// +optional
Matches []gatewayapiv1beta1.HTTPRouteMatch `json:"matches,omitempty"`
}

// Limit represents a complete rate limit configuration
type Limit struct {
// RouteSelectors defines semantics for matching an HTTP request based on conditions
Expand Down Expand Up @@ -197,6 +183,15 @@ func (r *RateLimitPolicy) Validate() error {
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 == gatewayapiv1alpha2.Kind("Gateway") {
for _, limit := range r.Spec.Limits {
if len(limit.RouteSelectors) > 0 {
return fmt.Errorf("route selectors not supported when targetting a Gateway")
}
}
}

return nil
}

Expand Down
52 changes: 52 additions & 0 deletions api/v1beta2/route_selectors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package v1beta2

import (
gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"

orderedmap "github.com/elliotchance/orderedmap/v2"

"github.com/kuadrant/kuadrant-operator/pkg/common"
)

// RouteSelector defines semantics for matching an HTTP request based on conditions
// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
type RouteSelector struct {
// Hostnames defines a set of hostname that should match against the HTTP Host header to select a HTTPRoute to process the request
// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
// +optional
Hostnames []gatewayapiv1beta1.Hostname `json:"hostnames,omitempty"`

// Matches define conditions used for matching the rule against incoming HTTP requests.
// https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRouteSpec
// +optional
Matches []gatewayapiv1beta1.HTTPRouteMatch `json:"matches,omitempty"`
}

// SelectRules returns, from a HTTPRoute, all HTTPRouteRules that either specify no HTTRouteMatches or that contain at
// least one HTTRouteMatch whose statements expressly include (partially or totally) the statements of at least one of
// the matches of the selector. If the selector does not specify any matches, then all HTTPRouteRules are selected.
//
// Additionally, if the selector specifies a non-empty list of hostnames, a non-empty intersection between the literal
// hostnames of the selector and set of hostnames specified in the HTTPRoute must exist. Otherwise, the function
// returns nil.
func (s *RouteSelector) SelectRules(route *gatewayapiv1beta1.HTTPRoute) (rules []gatewayapiv1beta1.HTTPRouteRule) {
rulesIndices := orderedmap.NewOrderedMap[int, gatewayapiv1beta1.HTTPRouteRule]()
if len(s.Hostnames) > 0 && !common.Intersect(s.Hostnames, route.Spec.Hostnames) {
return nil
}
if len(s.Matches) == 0 {
return route.Spec.Rules
}
for _, routeSelectorMatch := range s.Matches {
for idx, rule := range route.Spec.Rules {
rs := common.HTTPRouteRuleSelector{HTTPRouteMatch: &routeSelectorMatch}
if rs.Selects(rule) {
rulesIndices.Set(idx, rule)
}
}
}
for el := rulesIndices.Front(); el != nil; el = el.Next() {
rules = append(rules, el.Value)
}
return
}
211 changes: 211 additions & 0 deletions api/v1beta2/route_selectors_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
//go:build unit

package v1beta2

import (
"fmt"
"reflect"
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"

"github.com/kuadrant/kuadrant-operator/pkg/common"
)

func TestRouteSelectors(t *testing.T) {
gatewayHostnames := []gatewayapiv1beta1.Hostname{
"*.toystore.com",
}

gateway := &gatewayapiv1beta1.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: "my-gateway",
},
}

for _, hostname := range gatewayHostnames {
gateway.Spec.Listeners = append(gateway.Spec.Listeners, gatewayapiv1beta1.Listener{Hostname: &hostname})
}

route := &gatewayapiv1beta1.HTTPRoute{
Spec: gatewayapiv1beta1.HTTPRouteSpec{
CommonRouteSpec: gatewayapiv1beta1.CommonRouteSpec{
ParentRefs: []gatewayapiv1beta1.ParentReference{
{
Name: gatewayapiv1beta1.ObjectName(gateway.Name),
},
},
},
Hostnames: []gatewayapiv1beta1.Hostname{"api.toystore.com"},
Rules: []gatewayapiv1beta1.HTTPRouteRule{
{
Matches: []gatewayapiv1beta1.HTTPRouteMatch{
// get /toys*
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0],
Value: &[]string{"/toy"}[0],
},
Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethod("GET")}[0],
},
// post /toys*
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0],
Value: &[]string{"/toy"}[0],
},
Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethod("POST")}[0],
},
},
},
{
Matches: []gatewayapiv1beta1.HTTPRouteMatch{
// /assets*
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0],
Value: &[]string{"/assets"}[0],
},
},
},
},
},
},
}

testCases := []struct {
name string
routeSelector RouteSelector
route *gatewayapiv1beta1.HTTPRoute
expected []gatewayapiv1beta1.HTTPRouteRule
}{
{
name: "empty route selector selects all HTTPRouteRules",
routeSelector: RouteSelector{},
route: route,
expected: route.Spec.Rules,
},
{
name: "route selector selects the HTTPRouteRules whose set of HTTPRouteMatch is a perfect match",
routeSelector: RouteSelector{
Matches: []gatewayapiv1beta1.HTTPRouteMatch{
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0],
Value: &[]string{"/assets"}[0],
},
},
},
},
route: route,
expected: []gatewayapiv1beta1.HTTPRouteRule{route.Spec.Rules[1]},
},
{
name: "route selector selects the HTTPRouteRules whose set of HTTPRouteMatch contains at least one match",
routeSelector: RouteSelector{
Matches: []gatewayapiv1beta1.HTTPRouteMatch{
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0],
Value: &[]string{"/toy"}[0],
},
Method: &[]gatewayapiv1beta1.HTTPMethod{gatewayapiv1beta1.HTTPMethod("POST")}[0],
},
},
},
route: route,
expected: []gatewayapiv1beta1.HTTPRouteRule{route.Spec.Rules[0]},
},
{
name: "route selector with missing part of a HTTPRouteMatch still selects the HTTPRouteRules that match",
routeSelector: RouteSelector{
Matches: []gatewayapiv1beta1.HTTPRouteMatch{
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0],
Value: &[]string{"/toy"}[0],
},
},
},
},
route: route,
expected: []gatewayapiv1beta1.HTTPRouteRule{route.Spec.Rules[0]},
},
{
name: "route selector selects no HTTPRouteRule when no criterion matches",
routeSelector: RouteSelector{
Matches: []gatewayapiv1beta1.HTTPRouteMatch{
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchExact}[0],
Value: &[]string{"/toy"}[0],
},
},
},
},
route: route,
expected: nil,
Copy link
Contributor

@eguzki eguzki Jul 14, 2023

Choose a reason for hiding this comment

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

If the route says /toy*, then the route selector /toy should match, shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should document this very clearly. For me it is counter intuitive.

Copy link
Member

Choose a reason for hiding this comment

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

/toys* should not include /toy, but /toy* would

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither /toy, nor /toy* in a route selector match selects a HTTPRouteRule that specifies a single HTTPRouteMatch /toys*.

For a route selector to select a HTTPRouteRule that specifies a single HTTPRouteMatch /toys*, the route selector MUST specify /toys*. I.e. the fields of route selector's HTTPRouteMatch must be identical to the same fields in HTTPRouteRule's HTTPRouteMatch. Otherwise, we're back to having to deal with the sets and subsets problem all over again.


Here's the semantics to have in mind for each pair of route selector HTTPRouteMatch and HTTPRouteRule's HTTPRouteMatch to decide whether the route selector selects the HTTPRouteRule or not:

  1. A route selector selects a HTTPRouteRule if the HTTPRouteRule contains at least one HTTPRouteMatch that specifies fields that are literally identical to all the fields specified by at least one HTTPRouteMatch of the route selector.
  2. A HTTPRouteMatch within a HTTPRouteRule may include other fields that are not specified in a route selector match and yet the route selector match selects the HTTPRouteRule if rule all fields of the route select match are identically included in the HTTPRouteRule's HTTPRouteMatch; the opposite is NOT true.
  3. Each field path of a HTTPRouteMatch, as well as each field method of a HTTPRouteMatch, as well as each element of the fields headers and queryParams of a HTTPRouteMatch, is atomic – this is true for the HTTPRouteMatches within a HTTPRouteRule, as well as for HTTPRouteMatches of a route selector.

This semantics is described through examples in RFC 0001.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

},
{
name: "route selector selects the HTTPRouteRules whose HTTPRoute's hostnames match the selector",
routeSelector: RouteSelector{
Hostnames: []gatewayapiv1beta1.Hostname{"api.toystore.com"},
},
route: route,
expected: route.Spec.Rules,
},
{
name: "route selector selects the HTTPRouteRules whose HTTPRoute's hostnames match the selector additionally to other criteria",
routeSelector: RouteSelector{
Hostnames: []gatewayapiv1beta1.Hostname{"api.toystore.com"},
Matches: []gatewayapiv1beta1.HTTPRouteMatch{
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0],
Value: &[]string{"/toy"}[0],
},
},
},
},
route: route,
expected: []gatewayapiv1beta1.HTTPRouteRule{route.Spec.Rules[0]},
},
{
name: "route selector does not select HTTPRouteRules whose HTTPRoute's hostnames do not match the selector",
routeSelector: RouteSelector{
Hostnames: []gatewayapiv1beta1.Hostname{"www.toystore.com"},
},
route: route,
expected: nil,
},
{
name: "route selector does not select HTTPRouteRules whose HTTPRoute's hostnames do not match the selector even when other criteria match",
routeSelector: RouteSelector{
Hostnames: []gatewayapiv1beta1.Hostname{"www.toystore.com"},
Matches: []gatewayapiv1beta1.HTTPRouteMatch{
{
Path: &gatewayapiv1beta1.HTTPPathMatch{
Type: &[]gatewayapiv1beta1.PathMatchType{gatewayapiv1beta1.PathMatchPathPrefix}[0],
Value: &[]string{"/toy"}[0],
},
},
},
},
route: route,
expected: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
rules := tc.routeSelector.SelectRules(tc.route)
rulesToStringSlice := func(rules []gatewayapiv1beta1.HTTPRouteRule) []string {
return common.Map(common.Map(rules, common.HTTPRouteRuleToString), func(r string) string { return fmt.Sprintf("{%s}", r) })
}
if !reflect.DeepEqual(rules, tc.expected) {
t.Errorf("expected %v, got %v", rulesToStringSlice(tc.expected), rulesToStringSlice(rules))
}
})
}
}
1 change: 1 addition & 0 deletions bundle/manifests/kuadrant.io_ratelimitpolicies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ spec:
- eq
- neq
- startswith
- endswith
- incl
- excl
- matches
Expand Down
1 change: 1 addition & 0 deletions config/crd/bases/kuadrant.io_ratelimitpolicies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ spec:
- eq
- neq
- startswith
- endswith
- incl
- excl
- matches
Expand Down
Loading