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

enhancement: Add Admin API support #27

Closed
Sambigeara opened this issue Jun 19, 2023 · 3 comments · Fixed by #29
Closed

enhancement: Add Admin API support #27

Sambigeara opened this issue Jun 19, 2023 · 3 comments · Fixed by #29
Assignees
Labels
enhancement New feature or request

Comments

@Sambigeara
Copy link
Collaborator

Adding support to interface with the Admin API

@Sambigeara Sambigeara added the enhancement New feature or request label Jun 19, 2023
@Sambigeara Sambigeara self-assigned this Jun 19, 2023
@Sambigeara
Copy link
Collaborator Author

Sambigeara commented Jun 20, 2023

I opted to integrate protobuf/gRPC and in the process have unearthed some complexity (I still think this is a fight worth having). Noting here to log findings:

The protoc generated python uses imports that are equivalent to those in the *.proto files. Therefore, for example, cerbos/engine/v1/engine_pb2.pyi attempts to import dependencies like from cerbos.effect.v1 import effect_pb2 as _effect_pb2. It's written like this because the expectation is that proto generated py files reside at the package root (it's how Google does things).

This is troublesome for a couple of reasons, firstly because in order to fix imports, we'd be required to house all of our generated code in the package root (e.g. with each proto package next to cerbos), and secondly, because we can't do that anyway because we have a namespace collision (proto cerbos against python cerbos) 🤦‍♂️ . It doesn't look like there's a way of explicitly defining the package in a way that might alter the import path in a way that we want.

I played around with trying to store proto generated code in a subdirectory and adding that subdirectory to PYTHONPATH, but that's still impacted by the cerbos namespace collision. We can't change the subdirectory name, because it's embedded in all of our auto-gen'd code, and we can't change the original root package directory, because it'll break the package name for all of our clients. I also tried renaming our cerbos directory to _cerbos_impl and creating a new empty cerbos with an __init__.py which imported what we needed from _cerbos_impl but that also got messy.

At the moment I see two options:

  1. Post-process the generated code to fix the import paths (e.g. swopping from cerbos with from cerbos.gen.cerbos)
  2. We place all generated code at the package root. The contents of generated cerbos is merged with the existing cerbos directory. This will be a nightmare to maintain, plus will expose additional APIs to the client.

Or I just do away with this altogether. I really like auto-generated classes and gRPC services, but this might be more trouble than it's worth. Will continue to explore for a little while and make a decision.

@Sambigeara
Copy link
Collaborator Author

The more I think about it, the happier I am with option 2. The SDK will ultimately be a thin convenience wrapper around the generated code. I might be able to reduce down the required modules, and then hopefully I'll be able to prevent access to each bar cerbos.sdk (?). Will see where I end up

@charithe
Copy link
Contributor

charithe commented Jun 20, 2023

Those are the exact reasons why I decided to not use gRPC for Python SDK. I even tried to write a Rust module to handle the client interaction and make the Python SDK a thin wrapper on top of that. All of those approaches had some pretty gnarly issues to address and it felt like handcrafting the Python classes was much simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants