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

297 arch overview #24

Merged
merged 3 commits into from
Oct 16, 2023
Merged

297 arch overview #24

merged 3 commits into from
Oct 16, 2023

Conversation

maleck13
Copy link
Collaborator

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

@maleck13 maleck13 force-pushed the 297-arch-overview branch 2 times, most recently from 4644629 to c9dcae6 Compare August 29, 2023 08:14
Copy link

@emmanuelbernard emmanuelbernard left a 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.

docs/design/architectural-overview.md Show resolved Hide resolved
docs/design/architectural-overview.md Outdated Show resolved Hide resolved
docs/design/architectural-overview.md Outdated Show resolved Hide resolved
docs/design/architectural-overview.md Show resolved Hide resolved
docs/design/architectural-overview.md Outdated Show resolved Hide resolved
docs/design/architectural-overview.md Outdated Show resolved Hide resolved
docs/design/architectural-overview.md Outdated Show resolved Hide resolved
docs/design/architectural-overview.md Outdated Show resolved Hide resolved
docs/design/architectural-overview.md Outdated Show resolved Hide resolved
docs/design/architectural-overview.md Show resolved Hide resolved
Copy link
Contributor

@david-martin david-martin left a 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 <guicassolato@gmail.com>

Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com>

Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com>

Co-authored-by: Emmanuel Bernard <github@mel.emmanuelbernard.com>

updates post review

update high level overview diagram
@maleck13
Copy link
Collaborator Author

@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)
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@maleck13 maleck13 merged commit 9edd523 into main Oct 16, 2023
1 check passed
@maleck13 maleck13 deleted the 297-arch-overview branch October 16, 2023 09:23
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.

6 participants