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

Refactor auth.groups for easier management #237

Closed
sdaberdaku opened this issue Oct 3, 2024 · 3 comments · Fixed by #240
Closed

Refactor auth.groups for easier management #237

sdaberdaku opened this issue Oct 3, 2024 · 3 comments · Fixed by #240

Comments

@sdaberdaku
Copy link
Member

sdaberdaku commented Oct 3, 2024

Hello,

At the moment, the only way of assigning groups to Trino users (at least when using PASSWORD, JWT, and/or OAUTH2 authentications) is through the File Group Provider.

In the chart, this configuration must be provided like so:

auth:
  refreshPeriod: 60s
  groups: |-
    group1:user1,user2,user3
    group2:user4
    group3:user1,user5

Which can easily become unmanageable when handling a large number of users and groups.

My proposal is to slightly refactor this section and transforming groups into a map of lists, where the keys are the groups and the list elements are the users, like so:

auth:
  refreshPeriod: 60s
  groups: 
    group1:
      - user1
      - user2
      - user3
    group2:
      - user4
    group3:
      - user1
      - user5

This makes the user to group assignments much more maintainable (you can sort the users alphabetically, use the collapse feature in IDEs, etc.).

I can provide a PR for this feature. It would be easy enough to make this backward compatible, I would add a groupProvider section in values.yaml that, if provided, would override the auth.groups configuration.

groupProvider: {}
  # Set users' groups
  # https://trino.io/docs/current/security/group-file.html#file-format
  #  refreshPeriod: 5s
  #  groups:
  #    group_1:
  #      - user_1
  #      - user_2
  #      - user_3
  #    group_2:
  #      - user_4

The content of group.db can then be computed like so:

...
data:
  group.db: |-
    {{- range $k, $v := .Values.groupProvider.groups }}
    {{- printf "%s:%s" $k (join "," $v) | nindent 4 }}
    {{- end }}
...
@sdaberdaku
Copy link
Member Author

Maybe a less pervasive change could be to just make the content of additionalConfigFiles templetable so that users could add Helm templates in the file contents.

@nineinchnick what do you think about the latter option? It would be easy enough to implement and with very limited impact.

@nineinchnick
Copy link
Member

It was suggested before to make more sections of the chart templatable. Sounds like there's a good reason here, so I'm ok with this.

But I also like the suggestion here to use a map. For people keeping values in git, this would simplify the diffs.

@sdaberdaku
Copy link
Member Author

I'll prepare the PR then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants