This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

update RateLimitPolicy and API design #75

Merged

maleck13 merged 3 commits into Kuadrant:new-api-impl from rahulanand16nov:new-api-impl

Jan 19, 2022

Contributor

rahulanand16nov commented Jan 12, 2022 •

edited

Loading

This PR updates RateLimitPolicy and API resource designs.

Few notes:

To account for Request with | (pipe) character in authority header is auto-rejected by Limitador limitador#53, rate-limit-cluster name is patched at the same time when filters are added.
Routing info is removed from API and it just acts as a tying resource instead of implementing routing itself.
An example toystore virtualservice resource has been added to make it easy to test.
More changes will be done to RateLimitPolicy to account for virtualhost-level actions in the next PRs.

rahulanand16nov added 3 commits

January 12, 2022 18:43


          example: add virtualservice resource to toystore example

6c80a95


          update ratelimitpolicy and API design

315af51


          update samples for RLP and API resource

60ae620

rahulanand16nov requested a review from eguzki as a code owner

January 12, 2022 13:22

rahulanand16nov requested a review from maleck13

January 12, 2022 13:23

Contributor Author

rahulanand16nov commented Jan 12, 2022

Please ignore tests in CI for now as we are iterating on design at the moment.

maleck13 reviewed

View reviewed changes

Contributor

maleck13 left a comment

Generally looks ok. Will clone locally and try it out. Good work

apis/apim/v1alpha1/api_types.go

-              	AuthConfigSelector map[string]string    `json:"authConfigSelector,omitempty"`
-              	RateLimitSelector  *map[string]string   `json:"rateLimitSelector,omitempty"`
+              	Info                   API_Metadata      `json:"info"`
+              	VirtualServiceSelector map[string]string `json:"virtualServiceSelector,omitempty"`

Contributor

maleck13 Jan 12, 2022

I think having this here for now is fine. But want to discuss alternatives such as annotations on the resource and or a object reference

name: myroute
group: "kuadrant.io/v1alpha1"
Kind: "VirtualService|HTTPRoute"

apis/apim/v1alpha1/ratelimitpolicy_types.go

+              	RateLimitStage_BOTH     RateLimit_Stage = "BOTH"
+              )
+              var RateLimit_Stage_name = map[int32]string{

Contributor

maleck13 Jan 12, 2022

go has a useful type called iota

const (
	C0 = iota
	C1
	C2
)

Contributor Author

rahulanand16nov Jan 13, 2022

I had an integer there but then the string was required to match with the stage names.

config/crd/bases/apim.kuadrant.io_ratelimitpolicies.yaml

                                 type: object
                               type: array
                           required:
                           - limits
-                          - phases
-                          - rules
+                          - routes

Contributor

maleck13 Jan 12, 2022

should routes be required?

Contributor Author

rahulanand16nov Jan 13, 2022

I guess nothing is really required, so will add omit tag to limits and routes.

controllers/apim/api_controller.go

+              		matchingLabels := client.MatchingLabels{}
+              		matchingLabels = api.Spec.VirtualServiceSelector // interface requirements
+              		if err := r.Client().List(ctx, vslist, matchingLabels); err != nil {

Contributor

maleck13 Jan 12, 2022

does this list accross the whole cluster? I wonder should we limit it to the namespace the RateLimitPolicy is created in. Otherwise I could reference a virtualservice I don't own and affect its behaviour

controllers/apim/api_controller.go

		}
		logger.Info("number of virtualservices matched", "count", len(vslist.Items))

Contributor

maleck13 Jan 12, 2022

maybe debug?

Contributor Author

rahulanand16nov Jan 13, 2022

I am not sure what the V-level should be but looks like 1 is for Info and 10 is for trace so 4 maybe?

Contributor

eguzki Jan 13, 2022

logger.V(1).Info for debug

controllers/apim/api_controller.go

+              				logger.Error(err, "failed to add owner ref to RateLimit filters patch")
+              				return ctrl.Result{}, err
+              			}
+              			if err := r.Client().Create(ctx, rateLimitPatch); err != nil {

Contributor

maleck13 Jan 12, 2022

should do a CreateUpdate here as it is in a reconcile loop. base.ReconcileResource

controllers/apim/api_controller.go

+              								logger.Error(err, "failed to add owner ref to route-level ratelimits patch")
+              								return ctrl.Result{}, err
+              							}
+              							if err := r.Client().Create(ctx, routePatch); err != nil {

Contributor

maleck13 Jan 12, 2022

can do create update here also. If they change the name on the VS the next time we reconcile we will want to reflect that in our setup and remove the rate limiting from the named route

controllers/apim/api_controller.go

+              			Context: networkingv1alpha3.EnvoyFilter_GATEWAY,
+              			ObjectTypes: &networkingv1alpha3.EnvoyFilter_EnvoyConfigObjectMatch_Cluster{
+              				Cluster: &networkingv1alpha3.EnvoyFilter_ClusterMatch{
+              					Name: "outbound|8081||limitador.kuadrant-system.svc.cluster.local",

Contributor

maleck13 Jan 12, 2022

lets pull that out to a constant. Likely at some point it would come from configuration

Contributor Author

rahulanand16nov Jan 13, 2022

Done

controllers/apim/api_controller.go

               		return nil
               	}
-              	// A nice trick to make things easy.
+              	// A nice trick to make it easier add-in actions
               	stringPatch := string(patchRaw)
               	stringPatch = strings.Replace(stringPatch, "\"ACTIONS\"", string(jsonActions), 1)
               	fmt.Printf("STRING PATCH: %v", stringPatch)

Contributor

maleck13 Jan 12, 2022

can remove this

Contributor Author

rahulanand16nov Jan 13, 2022

But it's such a nice trick! JK, will do...

Contributor Author

rahulanand16nov Jan 13, 2022

Oh wait, I see, you mean the printf.

eguzki reviewed

View reviewed changes

apis/apim/v1alpha1/api_types.go

-              	AuthConfigSelector map[string]string    `json:"authConfigSelector,omitempty"`
-              	RateLimitSelector  *map[string]string   `json:"rateLimitSelector,omitempty"`
+              	Info                   API_Metadata      `json:"info"`
+              	VirtualServiceSelector map[string]string `json:"virtualServiceSelector,omitempty"`

Contributor

eguzki Jan 13, 2022

I do not get this :)

Contributor

eguzki Jan 13, 2022

The customer will be the owner of the virtualservice objects? In that case, kuadrant API object does no longer make any sense. Annotations and labels can be used to link VirtualServices, AuthConfigs, AuthortizationPolicies and RateLimitPolicies.

IMO the current kuadrant API CRD is a good compromise for an API owner between capabilities and complexity. It is made for API owners, in their own language and terms.

Contributor

maleck13 commented Jan 18, 2022

Tried this out locally and was able to see the rate limiting working

Contributor

maleck13 commented Jan 19, 2022

going to merge to allow building on top of these chnges

maleck13 merged commit 192297e into Kuadrant:new-api-impl

rahulanand16nov added a commit to rahulanand16nov/kuadrant-controller that referenced this pull request


          update RateLimitPolicy and API design (Kuadrant#75)

a727d3b

* example: add virtualservice resource to toystore example

* update ratelimitpolicy and API design

* update samples for RLP and API resource

rahulanand16nov added a commit that referenced this pull request


          Merge new-api-impl into main (#84)

c67bcd5

* add API and RateLimitPolicy resource

* add new apis' controller

* Add finalizer (#74)

* add API and RateLimitPolicy resource

* add new apis' controller

* add owner and finalizer for ratelimit policy

Co-authored-by: Rahul Anand <[email protected]>

* update RateLimitPolicy and API design (#75)

* example: add virtualservice resource to toystore example

* update ratelimitpolicy and API design

* update samples for RLP and API resource

* tie networking and ratelimit using annotations (#76)

* tie networking and ratelimit using annotations

* move network selection into spec from annotations

* reuse ClusterEnvoyPatch from istioprovider

* create AuthorizationPolicy using VirtualService (#79)

* create AuthorizationPolicy using VirtualService

* just log if reconciling w/o provider annotation

* move orphan check into the reconcile logic

* pass logger from main reconciler and small fix

* use predicate funcs to filter events for virtualservice

* make preauth as the first and postauth as the last filter (#80)

* cleanup API APIProduct CRDs (#81)

* cleanup API APIProduct CRDs

* fix tests

* remove duplicated constants

* Harden RateLimitPolicy implementation (#82)

* harden RateLimitPolicy implementation

* change finalizer's name

* fix typo and use correct gateway labels

* fix lint before merging into main

Co-authored-by: Craig Brookes <[email protected]>
Co-authored-by: Eguzki Astiz Lezaun <[email protected]>

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet