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

feat: support envoy ratelimit TLS settings. #1294

Merged
merged 19 commits into from
May 11, 2023

Conversation

qicz
Copy link
Member

@qicz qicz commented Apr 12, 2023

part support #1217

configuration refrence

apiVersion: config.gateway.envoyproxy.io/v1alpha1
kind: EnvoyGateway
gateway:
  controllerName: gateway.envoyproxy.io/gatewayclass-controller
provider:
  type: Kubernetes
  kubernetes:
    rateLimitDeployment:
      replicas: 1
      container:
        env:
        - name: REDIS_POOL_SIZE
          value: 10
        - name: REDIS_PERSECOND
          value: true
        resources:
          requests:
            cpu: 100m
            memory: 512Mi
        securityContext:
          runAsUser: 2000
          allowPrivilegeEscalation: false
      pod:
        annotations:
          key1: val1
          key2: val2
        securityContext:
          runAsUser: 1000
          runAsGroup: 3000
          fsGroup: 2000
          fsGroupChangePolicy: "OnRootMismatch"
rateLimit:
  backend:
    type: Redis
    redis:
      url: localhost:6379
      tls
         certificateRef:
             name: secret_name

cc @arkodg

@qicz qicz requested a review from a team as a code owner April 12, 2023 10:17
@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Merging #1294 (6a66fc1) into main (5f222eb) will decrease coverage by 0.02%.
The diff coverage is 71.69%.

@@            Coverage Diff             @@
##             main    #1294      +/-   ##
==========================================
- Coverage   62.26%   62.25%   -0.02%     
==========================================
  Files          79       79              
  Lines       11184    11254      +70     
==========================================
+ Hits         6964     7006      +42     
- Misses       3764     3791      +27     
- Partials      456      457       +1     
Impacted Files Coverage Δ
api/config/v1alpha1/envoygateway_types.go 100.00% <ø> (ø)
api/config/v1alpha1/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
...ernal/infrastructure/kubernetes/ratelimit_infra.go 66.66% <0.00%> (-33.34%) ⬇️
...al/infrastructure/kubernetes/ratelimit/resource.go 94.73% <90.12%> (-5.27%) ⬇️
...tructure/kubernetes/ratelimit/resource_provider.go 98.24% <100.00%> (-0.18%) ⬇️

... and 2 files with indirect coverage changes

@qicz qicz added kind/enhancement New feature or request area/api API-related issues labels Apr 12, 2023
@qicz qicz added this to the 0.5.0-rc1 milestone Apr 12, 2023
@qicz qicz force-pushed the api-ratelimit-tls branch 2 times, most recently from 334a77d to 2e596b6 Compare April 13, 2023 01:05
@qicz
Copy link
Member Author

qicz commented Apr 14, 2023

ptal @arkodg

@arkodg
Copy link
Contributor

arkodg commented Apr 14, 2023

@qicz can this API PR be broken up into two parts

@qicz
Copy link
Member Author

qicz commented Apr 15, 2023

@qicz can this API PR be broken up into two parts

My plan is also to divide into two PRS to complete this feature when the api like PR description confirmed and merged.

IMO, like the password and certificateRef user can configure directly, and another env configurations can be overrided by eg initial settings.

So current api structure is ok or not?

@arkodg
Copy link
Contributor

arkodg commented Apr 15, 2023

@qicz can this API PR be broken up into two parts

My plan is also to divide into two PRS to complete this feature when the api like PR description confirmed and merged. So current api structure is ok or not?

would prefer if the 2 API pieces were added as separate PRs because they are different features
high level comments on existing API

  • env should live inside rateLimitDeployment.container
  • tls style looks mostly good
  • I dont think we should introduce type so soon , and let the user configure that field using env

@qicz
Copy link
Member Author

qicz commented Apr 15, 2023

@qicz can this API PR be broken up into two parts

My plan is also to divide into two PRS to complete this feature when the api like PR description confirmed and merged. So current api structure is ok or not?

would prefer if the 2 API pieces were added as separate PRs because they are different features high level comments on existing API

  • env should live inside rateLimitDeployment.container
  • tls style looks mostly good
  • I dont think we should introduce type so soon , and let the user configure that field using env

I will do these after #1259 #1337

@qicz qicz changed the title api: Support Envoy RateLimit extension settings. [WIP] feat: support Envoy RateLimit TLS settings. Apr 25, 2023
@qicz qicz closed this May 3, 2023
@qicz qicz reopened this May 3, 2023
@qicz qicz closed this May 3, 2023
@qicz qicz reopened this May 3, 2023
@qicz
Copy link
Member Author

qicz commented May 3, 2023

@arkodg i am testing the reivewers requests but looks missing some permission

@qicz qicz changed the title [WIP] feat: support Envoy RateLimit TLS settings. feat: support Envoy RateLimit TLS settings. May 4, 2023
@qicz qicz requested review from arkodg, a team, kflynn and LanceEa and removed request for a team May 4, 2023 07:03
qicz added 3 commits May 10, 2023 09:24
Signed-off-by: qicz <qiczzhu@gmail.com>
Signed-off-by: qicz <qiczzhu@gmail.com>
@qicz
Copy link
Member Author

qicz commented May 10, 2023

ptal @arkodg all comments resolved

@qicz qicz requested a review from arkodg May 10, 2023 02:41
@qicz qicz requested a review from arkodg May 10, 2023 10:49
arkodg
arkodg previously approved these changes May 10, 2023
qicz added 2 commits May 11, 2023 14:19
Signed-off-by: qicz <qiczzhu@gmail.com>
@arkodg arkodg merged commit 777c770 into envoyproxy:main May 11, 2023
18 checks passed
tanujd11 pushed a commit to tanujd11/gateway that referenced this pull request May 20, 2023
* feat: support Envoy RateLimit TLS settings.

Signed-off-by: qicz <qiczzhu@gmail.com>
Co-authored-by: Xunzhuo <bitliu@tencent.com>
Co-authored-by: zirain <hejianpeng2@huawei.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api API-related issues kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants