trinodb / charts

Apache License 2.0
151 stars 173 forks source link

[Feature request] Support for multiple system access control configurations #225

Closed sdaberdaku closed 1 month ago

sdaberdaku commented 1 month ago

According to the official Trino documentation:

Multiple system access control implementations may be configured at once using the access-control.config-files configuration property. It should contain a comma separated list of the access control property files to use (rather than the default etc/access-control.properties).

Currently only one system access control method can be configured by using the Helm Chart.

Ideally, the user should be able to provide a list of access control configurations.

I can provide a PR for this feature.

nineinchnick commented 1 month ago

Isn't it possible to set access-control.config-files in .Values.server.coordinatorExtraConfig?

sdaberdaku commented 1 month ago

oh I had not thought of that.

So one would leave accessControl empty, and then proceed in creating configmaps for each system access control configuration externally from the chart, and mount them with additionalVolumes and additionalVolumeMounts. Does this sound right?

nineinchnick commented 1 month ago

Yes, exactly.

hashhar commented 1 month ago

Either way layering access controls like this is a recipe for confusing access control. In Trino DENY always wins so as long as any one of the access controls deny the permission is denied. This is good since it fails-closed. But can be confusing to explain and debug.

Just my 2 cents.