-
Notifications
You must be signed in to change notification settings - Fork 33
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
Update RLP docs and examples for v1beta2 #233
Conversation
Codecov Report
@@ Coverage Diff @@
## main #233 +/- ##
==========================================
- Coverage 64.79% 62.59% -2.21%
==========================================
Files 33 33
Lines 3224 3224
==========================================
- Hits 2089 2018 -71
- Misses 972 1021 +49
- Partials 163 185 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
|
7e8b911
to
a6b9af4
Compare
a6b9af4
to
7ff069f
Compare
7ff069f
to
03456eb
Compare
0731f3b
to
a52e924
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work 🎖️
Minor changes requested
|
||
## How: Implementation details | ||
|
||
### The WASM Filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of pity to lose this piece of information. People will recurrently ask the famous question: "why are you guys using Wasm module instead of Envoy's rate limit filter?" This section captures the rationale behind that decision.
What about moving this part to a separated file called rate-limiting-using-wasm-module.md
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a little while to re-write it, but I've added the section back to the doc.
|
||
Create the namesapce: | ||
|
||
```sh | ||
kubectl create namespace keycloak | ||
``` | ||
|
||
Deploy Keycloak: | ||
Deploy Keycloak with a [bootstrap](https://github.com/kuadrant/authorino-examples#keycloak) realm, users, and clients: | ||
|
||
```sh | ||
kubectl apply -n keycloak -f https://raw.githubusercontent.com/Kuadrant/authorino-examples/main/keycloak/keycloak-deploy.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not for this PR, but we may want to bring (a copy of) these resources to this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that having a shared repo that the different components can pull sample apps from may contribute to keeping consistent user guides and user stories across. Maybe we could make https://github.com/Kuadrant/authorino-examples more about Kuadrant as well? I wouldn't mind renaming it to "kuadrant-examples" if we think that would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My request was not about ownership. In other words authorino-examples
is ok for me. It was about, what if you change those resources to accommodate to new features or spec of Authorino? I fear that it would break the guides in the kuadrant operator. Anyway, raising this for awareness, not for this PR.
@@ -1,41 +1,77 @@ | |||
# Rate-limiting and protecting an API with JSON Web Tokens (JWTs) and Kubernetes authnz using Kuadrant | |||
# Authenticated Rate Limiting with JWTs and Kubernetes RBAC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an idea for the future. It would be very nice to run e2e tests exactly as the user guides define, adding some assertions. Even better would be to run tests reading the markdown files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
Create a HTTPRoute to route traffic to the service via Istio Ingress Gateway: | ||
|
||
![](https://i.imgur.com/rdN8lo3.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be moving to ascii diagrams at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for this PR, unless you want to
@@ -1,5 +1,4 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find any reference to this file. It must be a leftover from and old user guide. I vote for deleting the files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decided offline to keep them for now.
@@ -1,5 +1,4 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find any reference to this file. It must be a leftover from and old user guide. I vote for deleting the files
doc/rate-limiting.md
Closes #215