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

filters/ratelimit: apply ratelimitFailClosed regardless of the position in the route #2667

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

AlexanderYastrebov
Copy link
Member

@AlexanderYastrebov AlexanderYastrebov commented Oct 9, 2023

ratelimitFailClosed is a route-wide configuration filter so it should apply to all ratelimit filters regardless of its position.

PR consists of two commits: first refactors tests to simplify and allow routes with multiple filters and second makes ratelimitFailClosed apply regardless of the position in the route.

@AlexanderYastrebov AlexanderYastrebov force-pushed the filters/ratelimit/clarify-fail-closed branch 3 times, most recently from bbf1ee2 to 92b0513 Compare October 9, 2023 16:13
@zalando-robot
Copy link

Docker image "container-registry-test.zalando.net/teapot/skipper-test:sha256:04027c9bc10d536d75196a92107bf8e04661982a73b6e5b5ac273f7d69915e12" is not based on an approved base image. Any production deployment relying on this image will be blocked.

To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Open Source Registry namespace library and use a recommended version as listed in the documentation.

@zalando-robot
Copy link

Docker image "container-registry-test.zalando.net/teapot/skipper-test:sha256:be968010620feee141a47a7450aed8348e1843532f6ed0648492196bf1cec249" is not based on an approved base image. Any production deployment relying on this image will be blocked.

To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Open Source Registry namespace library and use a recommended version as listed in the documentation.

@zalando-robot
Copy link

Docker image "container-registry-test.zalando.net/teapot/skipper-test:sha256:f5aa28cfa6b1032aa021fd77bb817b70288e11c38e74cba15062c4b63838df9c" is not based on an approved base image. Any production deployment relying on this image will be blocked.

To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Open Source Registry namespace library and use a recommended version as listed in the documentation.

@zalando-robot
Copy link

Docker image "container-registry-test.zalando.net/teapot/skipper-test:sha256:bcc5cf9967ac8c9fa930c1924916c6f4ad56ed99dca583dd211bb4a8ced40b8b" is not based on an approved base image. Any production deployment relying on this image will be blocked.

To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Open Source Registry namespace library and use a recommended version as listed in the documentation.

docs/reference/filters.md Outdated Show resolved Hide resolved
@szuecs
Copy link
Member

szuecs commented Oct 9, 2023

@AlexanderYastrebov I think my intention was to have it as it was done. Not position independent for the route, because it would be weird from reading eskip. Don't you think so?
My chat comment likely was miss leading in this direction.

…on in the route

ratelimitFailClosed is a route-wide configuration filter so
it should apply to all ratelimit filters regardless of its position.

Signed-off-by: Alexander Yastrebov <[email protected]>
@AlexanderYastrebov AlexanderYastrebov force-pushed the filters/ratelimit/clarify-fail-closed branch from 92b0513 to e006f7f Compare October 9, 2023 17:14
@AlexanderYastrebov
Copy link
Member Author

@szuecs Yeah, I understand, but is there a usecase to apply it only for the rest of the filter chain? If e.g. redis fails it likely fails for all filters in the chain.
I think by making it position-independent along with clarified docs we would make it less error-prone.

it would be weird from reading eskip

We already have position independent filters, e.g. tracingTag, fadeIn, consistentHashKey, etc

@zalando-robot
Copy link

Docker image "container-registry-test.zalando.net/teapot/skipper-test:sha256:789b9a337be2b4df9bcb531604e306a5956fdb5d306434090db4c32ed98ee9ae" is not based on an approved base image. Any production deployment relying on this image will be blocked.

To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Open Source Registry namespace library and use a recommended version as listed in the documentation.

@zalando-robot
Copy link

Docker image "container-registry-test.zalando.net/teapot/skipper-test:sha256:abcfbbc9f21bf0b2b5f3a22cebef881b2fde17e34c6f7fa1631442054719407b" is not based on an approved base image. Any production deployment relying on this image will be blocked.

To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Open Source Registry namespace library and use a recommended version as listed in the documentation.

@zalando-robot
Copy link

Docker image "container-registry-test.zalando.net/teapot/skipper-test:sha256:910a791cf93f30071f042b51893e0bcbd7ff1786125f8e61a132c4855f4ed579" is not based on an approved base image. Any production deployment relying on this image will be blocked.

To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Open Source Registry namespace library and use a recommended version as listed in the documentation.

@zalando-robot
Copy link

Docker image "container-registry-test.zalando.net/teapot/skipper-test:sha256:9996cc1a95ab5d49fca9427937bb403a8f33e6c33c6c053b2e3667b2db6d9c9b" is not based on an approved base image. Any production deployment relying on this image will be blocked.

To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Open Source Registry namespace library and use a recommended version as listed in the documentation.

@szuecs
Copy link
Member

szuecs commented Oct 10, 2023

👍

@szuecs
Copy link
Member

szuecs commented Oct 10, 2023

@AlexanderYastrebov should we formulate this out as expectation for the user, how a "config" filter will work?
Maybe it's the time to write it down that "config" filters will change all filters of a route.
wdyt?

@AlexanderYastrebov
Copy link
Member Author

👍

@AlexanderYastrebov AlexanderYastrebov merged commit 79a9f35 into master Oct 10, 2023
6 checks passed
@AlexanderYastrebov AlexanderYastrebov deleted the filters/ratelimit/clarify-fail-closed branch October 10, 2023 10:44
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.

4 participants