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

accessControl properties are only added to the coordinator #189

Closed
mgorbatenko opened this issue Jul 5, 2024 · 2 comments · Fixed by #226
Closed

accessControl properties are only added to the coordinator #189

mgorbatenko opened this issue Jul 5, 2024 · 2 comments · Fixed by #226

Comments

@mgorbatenko
Copy link

mgorbatenko commented Jul 5, 2024

accessControl properties need to be on worker nodes to properly implement graceful shutdowns. Without that, the preStop api request will result in a 403. A work around is to add the access control properties via additionalConfigFiles. I believe it would be helpful to have the properties added to the worker configmap as well, so that the properties are present on both the coordinator and worker nodes.

Some of my learnings outlined on the Trino slack here.

@sdaberdaku
Copy link
Member

According to the official Trino documentation:

Access control must be configured on the coordinator. Authorization for operations on specific worker nodes, such a triggering Graceful shutdown, must also be configured on all workers.

which means that the accessControl configuration should only be added to the worker when the graceful shutdown feature is required.

It would be nice if the worker configuration had a simple option to enable graceful shutdown, which would automatically add the required preStop hook and configuration options.

I could provide a PR for the latter. @nineinchnick what do you think?

@nineinchnick
Copy link
Member

There's #213 but looks like it stalled. You can open another PR. We definitely need a test case for this too, to catch any potential regressions in the future.

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.

3 participants