briefercloud / briefer

Dashboards and notebooks in a single place. Create powerful and flexible dashboards using code, or build beautiful Notion-like notebooks and share them with your team.
https://briefer.cloud
GNU Affero General Public License v3.0
3.62k stars 209 forks source link

[feature]: use default GCP authentication #238

Closed guissalustiano closed 4 days ago

guissalustiano commented 5 days ago

Hi, we are running briefer on GCP and would like to manage the permission with a service account instead using a file.

Currently, briefer always uses the credential file https://github.com/briefercloud/briefer/blob/3799057d2588ded39eaab641d20063cee30ca3d0/apps/api/src/python/query/bigquery.ts#L119-L124

but I think we should not require the credential file and try with default credentials.

I would like to work on that, but I first open the PR to check if this feature would be accepted and if there is any suggestion on how to implement that. Thanks!

lucasfcosta commented 4 days ago

Hey @guissalustiano, thank you so much for opening this issue!

I really like this idea, but my main concern here is why that would be necessary. I thought that we'd load the service account anyway when querying from the jupyter pod so we wouldn't need to authenticate through the pod itself. Not sure if GCP has a particular setting to handle scenarios like this.

Ideally, we should be able to run the query without attaching the SA to the pod because then it will run in the exact same way when running outside of K8S.

At first, this seems more like a bug - it seems like we should be able to show more precise errors when there's a permissions-related issue. I think it's possible that we tackle that instead and it would be an amazing PR.

I'll CC @vieiralucas in here so he can chime in as well.


If we do decide to go ahead, IMO the best way to handle this would be to add a toggle on the BigQuery form that says something like Assign this service account to my Jupyter environment.

When that's toggled we should patch the existing jupyter pod within the cluster so it uses the service account. Every query goes through that particular pod, not through the API.

A few attention points here:

Let's wait a bit for @vieiralucas before we move ahead here

lucasfcosta commented 4 days ago

@guissalustiano just discussed this with @vieiralucas on Slack.

We came to the conclusion that the right way to solve this is to fix the bug which prevents real errors from showing up when people connect a data source.

In this case, you'd have seen a message saying something like lacks permissions XYZ.

Do you want to go ahead with a PR? We're happy to fix this on our end too if you're busy. Otherwise, happy to help you contribute.

FYI the reason we decided to do this is to streamline the way the software works so it always works the same way instead of creating a special case and attaching a service account to the pod.

Wdyt?

guissalustiano commented 4 days ago

Hey, thanks for the response! I think even without the bug that makes me explore that, this is a nice to have feature, but I didn't explain enough If the briefer allows using the default credential, it would make a little safer because there is no need to have a credential file, and I think it's easier to manage because I can attach the service account by to terraform instead the UI. But I agree that all these features are really minor and should not be a priority.

The idea is if you are running briefer inside GCP (e.g., computer engine) you can attach a service account and when you use any google library they know how to load the credentials.

Ideally, we should be able to run the query without attaching the SA to the pod because then it will run in the exact same way when running outside of K8S.

I think this is the main point and complete understandable if we decide not implement this, it is just a little number of people who would benefit from that

Do you want to go ahead with a PR? We're happy to fix this on our end too if you're busy. Otherwise, happy to help you contribute.

I'm not comfortable solving the bug because I would look for that only in my spare time and can't guarantee when this would be done.

Thanks so much for the time!