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.

Sign up for GitHub

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

Jump to bottom

Tenant layer introduction to library #549

Merged
merged 50 commits into from
Sep 2, 2024
Merged

Tenant layer introduction to library #549

merged 50 commits into from
Sep 2, 2024

Conversation

mithunbharadwaj
Copy link
Collaborator

No description provided.

@mithunbharadwaj mithunbharadwaj self-assigned this Feb 13, 2024
@ateska
Copy link
Contributor

ateska commented Feb 16, 2024

This will break many things ... it has to be implemented differently.
It is zookeeper provider specific functionality and we need to implement that there.

ateska
ateska requested changes Feb 20, 2024
View reviewed changes
asab/library/contextvars.py Outdated
from contextvars import ContextVar

# Define a context variable for tenant
tenant_var = ContextVar('tenant', default=None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this into Zookeeper provider.
There has to be an official and documented way how an user can set it.

asab/library/providers/abc.py Outdated
@@ -53,3 +53,33 @@ async def subscribe(self, path: str):

"""
raise NotImplementedError("{}.subscribe()".format(self.__class__.__name__))

async def tenant_exists(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is not needed.

asab/library/providers/zookeeper.py Outdated
tenant = tenant_var.get()

if tenant not in [None, ""]:
node_path = self.BasePath + "/.tenants/" + tenant + path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yo need to do two "hits" to zookeeper - first into /.tenants/.... and if the item doesn't exist there, then you do hit the "normal" path.

@ateska
Copy link
Contributor

ateska commented Feb 20, 2024 •
edited
Loading

There are two ways how a microservice can set "tenant" into zookeeper library provider:

  • Globally, when the microservice is started and it is a "tenant-specific" microservice, the tenant is set in init time
  • For a specific web request (or Kafka request), in the handler code, using try/finally construct - so the tenant is set for a context of the specific request

Snippet of the handler code:

t = ....TenantContextVar.set(tenant)
try:
    ... code of the handler
finally:
    ....TenantContextVar.reset(t)

@ateska
Copy link
Contributor

ateska commented May 24, 2024

Please proceed with a merge @mithunbharadwaj

@mithunbharadwaj
Copy link
Collaborator Author

NOTE:

This is directly connected to Merge Request #64 in the asab-library micro service, that introduces authorised API calls. This Merge Request will be merged once #64 is working as per acceptance criteria and approved.

@ateska
Copy link
Contributor

ateska commented Jun 28, 2024

Please remove tenant=... parameters in the call and replace them by the use of the context variable.

ateska
ateska requested changes Jun 28, 2024
View reviewed changes
Copy link
Contributor

@ateska ateska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment.

ateska
ateska requested changes Jul 26, 2024
View reviewed changes
asab/library/providers/zookeeper.py Outdated
@@ -300,13 +300,19 @@ async def list(self, path: str) -> list:
global_nodes = await self.Zookeeper.get_children(global_node_path) or []
global_items = await self.process_nodes(global_nodes, path)

<<<<<<< HEAD
Copy link
Contributor

@ateska ateska Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?? ... likely untested code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No! I am synchronising this PR to latest master.

@ateska
Copy link
Contributor

ateska commented Jul 30, 2024

Let me know when this is ready for a review, please.

ateska
ateska requested changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@ateska ateska left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the workshop:

  • Remove tenant param from LibraryService calls: read, open, list and export ... tenant must be extracted from the TenantContextVar
  • check_disabled keeps the tenant param, it is a responsibility of the caller to obtain tenant (ie from TenantContextVar) or remove tenant param too in a same way as above point
  • Coordinate with other team members so that this breaking change is adopted in every relevant microservice (use a spreadsheet for that)

asab/library/providers/zookeeper.py Outdated
from contextvars import ContextVar

# Define a context variable for tenant
TenantContextVar = ContextVar('tenant', default=None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This MAY go into asab.web.auth ... discuss this with @byewokko

@ateska
Copy link
Contributor

ateska commented Aug 13, 2024

Let me know when this is ready for re-review.

@mithunbharadwaj mithunbharadwaj merged commit 1a5cfca into master Sep 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ateska ateska ateska requested changes

Assignees

@mithunbharadwaj mithunbharadwaj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mithunbharadwaj @ateska