Skip to content

feat: managed service accounts for BYOB users#2508

Merged
Ziinc merged 9 commits intomainfrom
ziinc/anl-948-logflare-byob-managed-service-accounts-handling
Jul 9, 2025
Merged

feat: managed service accounts for BYOB users#2508
Ziinc merged 9 commits intomainfrom
ziinc/anl-948-logflare-byob-managed-service-accounts-handling

Conversation

@Ziinc
Copy link
Contributor

@Ziinc Ziinc commented Jul 8, 2025

This PR adds in adding of managed SAs to BYOB users' projects. This ensures that the IAM policy updating is non-destructive. It also requires the permissions to fetch and set the IAM policies (i.e. IAM admin), and thusly this is optional depending on whether the user has enabled it.

Queries are routed through partitioned Goth servers, each service account gets their own partitioned goth server to ensure that token refreshing does not bottleneck requests.
This is only true when:

  1. system has managed SAs enabled and user is not byob
  2. system has managed SAs enabled, and user is BYOB and has managed SAs enabled.
    Otherwise, queries are all routed through Logflare.Goth (whcih is the main service account)

@Ziinc Ziinc requested review from a team and chasers July 8, 2025 17:45
@Ziinc
Copy link
Contributor Author

Ziinc commented Jul 9, 2025

@Logflare/dashbit will need to merge this in first but a review would be appreciated 🙏

@Ziinc Ziinc merged commit 23c0b69 into main Jul 9, 2025
9 checks passed
@Ziinc Ziinc deleted the ziinc/anl-948-logflare-byob-managed-service-accounts-handling branch July 9, 2025 04:50
Copy link

@wojtekmach wojtekmach left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Comment on lines +49 to +60
def append_managed_sa_to_iam_policy(user) do
with {:enabled?, true} <- {:enabled?, BigQueryAdaptor.managed_service_accounts_enabled?()},
{:ok, policy} <- get_iam_policy(user),
{:contains?, _policy, false} <-
{:contains?, policy, contains_managed_service_accounts?(policy)} do
append_managed_service_accounts(user.bigquery_project_id, policy)
else
{:contains?, policy, _} -> {:ok, policy}
{:enabled?, false} -> {:error, :managed_service_accounts_disabled}
{:error, _err} = err -> err
end
end

Choose a reason for hiding this comment

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

This is pretty subjective but in our experience, with/1 + "tagged tuples" decreases readability as one has to go back'n'forth between the "happy" path and the corresponding else clause. For me, with/1 is ideal without else clause or with a single catch-all else clause, anything other than that, to me, gives me a pause what's going on every single time.

I haven't tested it but I think this would be the equivalent:

def append_managed_sa_to_iam_policy(user) do
  if BigQueryAdaptor.managed_service_accounts_enabled?() do
    with {:ok, policy} <- get_iam_policy(user) do
      if contains_managed_service_accounts?(policy) do
        {:ok, policy}
      else
        append_managed_service_accounts(user.bigquery_project_id, policy)
      end
    end
  else
    {:error, :managed_service_accounts_disabled}
  end
end

again, pretty subjective though!


def change do
alter table(:users) do
add :bigquery_enable_managed_service_accounts, :boolean, default: false

Choose a reason for hiding this comment

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

consider adding null: false which would be safe since there's a default.

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.

2 participants