-
Notifications
You must be signed in to change notification settings - Fork 11
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
297 arch overview #24
Conversation
a70b946
to
d34e3e1
Compare
4644629
to
c9dcae6
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.
Here is a first part review that Craig asked me to do.
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.
This is great!
It captures the current architecture really well.
I also like that there's a future direction section to evolve it based on our learnings so far.
Co-authored-by: Guilherme Cassolato <[email protected]> Co-authored-by: Guilherme Cassolato <[email protected]> Co-authored-by: Guilherme Cassolato <[email protected]> Co-authored-by: Emmanuel Bernard <[email protected]> updates post review update high level overview diagram
9931895
to
1cb93d4
Compare
@david-martin @guicassolato keen to get this merged. Ok to give a final review |
Each of these components have the capability to see the request and need to in order to make the required decision. Each of these components can also prevent the request from reaching its intended backend destination based on user configuration. | ||
|
||
|
||
![](./images/request-flow.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.
I don't think we need to change anything here necessarily, but I do wonder whether the depicted connection between Authorino and the external Auth Provider in the request flow diagram could be interpreted as a requirement, when actually it is not.
Partially, it may also be a matter of interpretation about what "external" means.
The part that is true is that Kuadrant does not manage user/client identities (i.e. Kuadrant is not an IAM system). In this sense, Authorino needs to go elsewhere to fetch something that will later allow it to verify the authentication tokens supplied in the requests.
That elsewhere can be an external auth provider (e.g. to fetch a set of JSON Web Keys) or the Kuberentes cluster itself (to pull API key secrets and x509 CA certs). Either way, definitely not part of the request flow, and perhaps not "external" depending on your POV.
Exceptional cases where Authorino will perform a request to a third-party authenticator every time (caching aside), as part of the request flow, are:
- Authentication based on OAuth opaque tokens;
- Kubernetes TokenReview – this one arguably not "external".
As I said, no need to change the diagram I think. It's good that integrating a proper auth provider is somehow represented. Devil's in the details tho. It's not always external, and it's rarely something that happens in the request flow.
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 think it is a fair point. I will merge as is for now so that the overview is present and visible but in future will make sure to take this into account.
Opening this for comment. I have covered the main areas I believe. There are 2 outstanding todos around future arch and also observability that I will update soon