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

Ratelimit validation #1508

Merged
merged 21 commits into from
Dec 10, 2024

Conversation

kolodziejczak
Copy link
Contributor

@kolodziejczak kolodziejczak commented Dec 2, 2024

Description

Changes proposed in this pull request:

  • Add RateLimit CR static validation
  • Add RateLimit CR in-code validation
  • Introduce in-code RateLimit validation to the RateLimit reconcile loop
  • Update technical design with a new CR and controller
  • Fix RateLimit CRD group to align with the APIRule

Pre-Merge Checklist

  • As a PR reviewer, verify code coverage and evaluate if it is acceptable.

Related issues

@kyma-bot kyma-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cla: yes Indicates the PR's author has signed the CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 2, 2024
@kolodziejczak kolodziejczak marked this pull request as ready for review December 4, 2024 13:39
@kolodziejczak kolodziejczak requested review from a team as code owners December 4, 2024 13:39
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2024
@kolodziejczak kolodziejczak mentioned this pull request Dec 4, 2024
14 tasks
internal/ratelimit/validate.go Show resolved Hide resolved
apis/gateway/v1alpha1/ratelimit_types.go Outdated Show resolved Hide resolved
controllers/gateway/ratelimit_controller.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Ressetkk Ressetkk left a comment

Choose a reason for hiding this comment

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

You should organise code differently.
reconcile: controllers/gateway/ratelimit
api: apis/gateway/v1alpha1/ratelimit

I think we should set naming scheme for the CRDs like so:

  • for APIs - apis/{group}/{version}/{name}
  • for controllers - controllers/{group}/{name}

The code can be easily in the same group but in different packages. Please adjust the code and possibly revert the changes you did to move the code into the gateway package.

@kolodziejczak
Copy link
Contributor Author

You should organise code differently. reconcile: controllers/gateway/ratelimit api: apis/gateway/v1alpha1/ratelimit

I think we should set naming scheme for the CRDs like so:

  • for APIs - apis/{group}/{version}/{name}
  • for controllers - controllers/{group}/{name}

The code can be easily in the same group but in different packages. Please adjust the code and possibly revert the changes you did to move the code into the gateway package.

Restructured slightly different:
for APIs - apis/group/kind/version
for controllers - controllers/group/kind

barchw
barchw previously approved these changes Dec 9, 2024
@kyma-bot kyma-bot added the lgtm Looks good to me! label Dec 9, 2024
Ressetkk
Ressetkk previously approved these changes Dec 9, 2024
@kyma-bot kyma-bot added the lgtm Looks good to me! label Dec 9, 2024
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Dec 10, 2024
@kyma-bot kyma-bot added the lgtm Looks good to me! label Dec 10, 2024
@kolodziejczak kolodziejczak merged commit 9632426 into kyma-project:main Dec 10, 2024
21 checks passed
@kolodziejczak kolodziejczak deleted the ratelimit-validation branch December 10, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants