Skip to content

Add RateLimitFilter support in k8s provider#908

Merged
danehans merged 5 commits intoenvoyproxy:mainfrom
arkodg:rl-provider
Jan 18, 2023
Merged

Add RateLimitFilter support in k8s provider#908
danehans merged 5 commits intoenvoyproxy:mainfrom
arkodg:rl-provider

Conversation

@arkodg
Copy link
Copy Markdown
Contributor

@arkodg arkodg commented Jan 14, 2023

Relates to #670

Signed-off-by: Arko Dasgupta arko@tetrate.io

@arkodg arkodg requested a review from a team as a code owner January 14, 2023 05:00
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 14, 2023

Codecov Report

Merging #908 (baffca3) into main (29a8640) will decrease coverage by 0.14%.
The diff coverage is 48.80%.

@@            Coverage Diff             @@
##             main     #908      +/-   ##
==========================================
- Coverage   63.40%   63.26%   -0.15%     
==========================================
  Files          53       53              
  Lines        7487     7556      +69     
==========================================
+ Hits         4747     4780      +33     
- Misses       2440     2468      +28     
- Partials      300      308       +8     
Impacted Files Coverage Δ
internal/gatewayapi/resource.go 54.83% <0.00%> (-1.83%) ⬇️
internal/gatewayapi/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
internal/provider/kubernetes/controller.go 46.13% <45.09%> (-0.61%) ⬇️
internal/provider/kubernetes/predicates.go 60.73% <46.66%> (-1.20%) ⬇️
internal/provider/kubernetes/routes.go 32.71% <74.28%> (+2.71%) ⬆️
internal/gatewayapi/helpers.go 78.60% <100.00%> (+0.21%) ⬆️
internal/gatewayapi/route.go 89.02% <0.00%> (ø)
internal/gatewayapi/filters.go 82.01% <0.00%> (ø)
internal/gatewayapi/listener.go 100.00% <0.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Copy Markdown
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

@arkodg I have a few nits and the PR needs rebasing. Otherwise, LGTM.

expected: true,
},
{
name: "httproute with one rateLimitfilter",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you also create a test case that has 1 AuthenFilter and 1 RateLimitFilter?

Arko Dasgupta added 4 commits January 17, 2023 13:16
Relates to envoyproxy#670

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@danehans
Copy link
Copy Markdown
Contributor

@arkodg CI is failing b/c the controller-runtime cache is unable to sync. The cache is unable to sync b/c the RateLimitFilter CRD is not installed by the kube-deploy target. You need the following:

$ git diff
diff --git a/internal/provider/kubernetes/config/crd/kustomization.yaml b/internal/provider/kubernetes/config/crd/kustomization.yaml
index 1d85bb9d..c5e344f4 100644
--- a/internal/provider/kubernetes/config/crd/kustomization.yaml
+++ b/internal/provider/kubernetes/config/crd/kustomization.yaml
@@ -4,6 +4,7 @@
 resources:
 - bases/config.gateway.envoyproxy.io_envoyproxies.yaml
 - bases/gateway.envoyproxy.io_authenticationfilters.yaml
+- bases/gateway.envoyproxy.io_ratelimitfilters.yaml
 #+kubebuilder:scaffold:crdkustomizeresource

Creating an HTTPRoute that references a RateLimitFilter results in:

$ kubectl get httproute/backend -o yaml
apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: backend
  namespace: default
...
status:
  parents:
  - conditions:
    - lastTransitionTime: "2023-01-17T21:37:43Z"
      message: 'Reference not found for filter type: ExtensionRef'
      observedGeneration: 1
      reason: BackendNotFound
      status: "False"
      type: ResolvedRefs
    controllerName: gateway.envoyproxy.io/gatewayclass-controller
    parentRef:
      group: gateway.networking.k8s.io
      kind: Gateway
      name: eg

You need to update filters.go in the gatewayapi translator to process the RateLimitFilter. This step can be done in a separate PR.

@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jan 17, 2023

@arkodg CI is failing b/c the controller-runtime cache is unable to sync. The cache is unable to sync b/c the RateLimitFilter CRD is not installed by the kube-deploy target. You need the following:

$ git diff
diff --git a/internal/provider/kubernetes/config/crd/kustomization.yaml b/internal/provider/kubernetes/config/crd/kustomization.yaml
index 1d85bb9d..c5e344f4 100644
--- a/internal/provider/kubernetes/config/crd/kustomization.yaml
+++ b/internal/provider/kubernetes/config/crd/kustomization.yaml
@@ -4,6 +4,7 @@
 resources:
 - bases/config.gateway.envoyproxy.io_envoyproxies.yaml
 - bases/gateway.envoyproxy.io_authenticationfilters.yaml
+- bases/gateway.envoyproxy.io_ratelimitfilters.yaml
 #+kubebuilder:scaffold:crdkustomizeresource

Creating an HTTPRoute that references a RateLimitFilter results in:

$ kubectl get httproute/backend -o yaml
apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: backend
  namespace: default
...
status:
  parents:
  - conditions:
    - lastTransitionTime: "2023-01-17T21:37:43Z"
      message: 'Reference not found for filter type: ExtensionRef'
      observedGeneration: 1
      reason: BackendNotFound
      status: "False"
      type: ResolvedRefs
    controllerName: gateway.envoyproxy.io/gatewayclass-controller
    parentRef:
      group: gateway.networking.k8s.io
      kind: Gateway
      name: eg

You need to update filters.go in the gatewayapi translator to process the RateLimitFilter. This step can be done in a separate PR.

thanks for helping debug this issue ! will make the change in the PR

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@danehans danehans merged commit 5b8b24e into envoyproxy:main Jan 18, 2023
@arkodg arkodg deleted the rl-provider branch January 18, 2023 02:24
@arkodg arkodg mentioned this pull request Jan 24, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants