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

EVEREST-107 | API server code refactor & RBAC unit tests #896

Draft
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

mayankshah1607
Copy link
Member

@mayankshah1607 mayankshah1607 commented Dec 4, 2024

Refactor the API server code.

Why?

In its current state, all the API logic is embedded directly into its respective API handler methods. With the addition of the RBAC feature, it seems that the code for different functionalities have intertwined with each other, resulting in code that is very tightly coupled. This has made it harder to include useful unit-testing and could even make it challenging to extend the code with new features in the future.

Key concerns:

  • RBAC is handled (inconsistently) in 4 different ways:
    • by the default HTTP handler (which filters based on HTTP verb and resource name)
    • using the enforce methods (for explicit permissions)
    • using enforce methods, but within validation functions like this.
    • List calls that perform weird transformations on the k8s proxy response for example, this.
  • This inconsistency makes it difficult to debug and write proper unit tests.
  • In general, it seems that the API server code has grown in complexity with the additions of new features/fixes, but the code architecture has not kept up with it, which can lead to maintenance issues.

Proposed refactor

This code architecture proposed in this PR takes inspiration from a very common design pattern called chain of responsibility.

The key idea is simple:

  • Break down the logic into modular components. Each component is responsible just 1 specific task (for example, RBAC enforcement)
  • Process requests sequentially. Components handle incoming requests one at a time. Once a component completes its task, it delegates the execution to the next component in the chain.

Such a design has some advantages:

  • Separation of Concerns - each component is responsible for a distinct piece of functionality, making the code easier to read, test, and maintain.
  • New functionality can be added by simply introducing a new component into the chain without disrupting existing components.

For example, see the newly added RBAC test for the BackupStorages API. The unit test is very simple, easy to understand and requires mocking only 1 dependency, i.e, the "next" handler.

Implementation details

  • Introduces a new interface called Handler - this interface contains the methods that will be called by the API. Each method corresponds to one API call.
  • The API logic can be broken down into 3 categories of work - (1) request validation (2) RBAC, and (3) Kubernetes. Hence, we have 3 components (one for each type of work), that implement this Handler interface.
  • The request execution starts with the validation handler, then to RBAC handler and finally to k8s handler

@mayankshah1607 mayankshah1607 force-pushed the EVEREST-107-api-refactoring branch 2 times, most recently from af4294d to 939db3d Compare December 11, 2024 08:36
@mayankshah1607 mayankshah1607 changed the title EVEREST-107 | WIP EVEREST-107 | API server code refactor & RBAC unit tests Dec 11, 2024
@mayankshah1607 mayankshah1607 force-pushed the EVEREST-107-api-refactoring branch from dd216b6 to a61bfb0 Compare December 11, 2024 13:57
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
Signed-off-by: Mayank Shah <[email protected]>
@mayankshah1607 mayankshah1607 force-pushed the EVEREST-107-api-refactoring branch from e2250da to 08023cc Compare December 12, 2024 10:30
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.

1 participant