trinodb / charts

Apache License 2.0
151 stars 173 forks source link

Refactor `auth.groups` for easier management #237

Closed sdaberdaku closed 1 month ago

sdaberdaku commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

I'll prepare the PR then :)